coins: split coins view cache and storage contracts#158
Draft
l0rinc wants to merge 6 commits into
Draft
Conversation
The coins view fuzz helper currently derives persistent-storage coverage from the same backend pointer it mutates with cache backend switching, and it detects DB behavior with a `dynamic_cast`. Give the helper an explicit DB best-block policy and use its active backend only for reader and cache behavior. Move DB cursor, size, and head-block checks into the DB-specific fuzz target, where the storage object is stable. This records the fuzz behavior change before the storage interface split, so the next commit can remove storage methods from the generic reader without also changing what the fuzz target is trying to prove.
The generic coins reader exposes persistent storage iteration and sizing hooks that cache and backed views do not naturally support. Introduce `CCoinsViewStorage` for cursor, head-block, and size users, and move coinstats, RPC, snapshot validation, and remaining fuzz callers to request that storage contract directly. `CCoinsView` now keeps coin lookup, best-block reads, and batch writes, while cache and backed adapters stop providing storage-only fallbacks. `CCoinsViewStorage` deliberately repeats `GetBestBlock` so storage traversal can use the associated block hash without depending on the coin lookup interface.
`SetBackend` only mutates the temporary backend of `CCoinsViewCache`, but the setter lived on every generic backed view. Make `CCoinsViewCache` own its backend pointer directly and keep backend switching on the cache type that actually uses it. This leaves other backed adapters unchanged for now while removing the split where `CCoinsViewBacked` stored the pointer and `CCoinsViewCache` exposed the mutation. `CCoinsViewBacked` hides different fallback contracts behind one broad forwarding base. Remove the generic backed adapter and let each remaining adapter spell out its forwarding directly. `CCoinsViewCache` already owns the mutable backend slot; `CCoinsViewMemPool` and `CCoinsViewErrorCatcher` now keep their own base pointers with reference constructors. This keeps mempool lookup behavior unchanged: `GetCoin` can see mempool outputs, while `PeekCoin`, `HaveCoin`, best-block reads, and batch writes still fall through to the base view.
`CCoinsViewCache` always needs a live backend view, and backend switching already uses a reference. Make cache construction take `CCoinsView&`, so the non-null backend contract is part of the signature instead of a pointer that is immediately asserted. `CTxMemPool::check` already built a duplicate cache from `active_coins_tip` and allowed cache-populating reads through a `const_cast`. Make that parameter mutable and build the duplicate through the same backend-reference contract, removing the cast and making the caching side effect explicit before the backend capability is narrowed further.
`BatchWrite` is only needed by objects that can receive cache flushes, but keeping it on `CCoinsView` makes every reader advertise a write target. Introduce `CCoinsViewCacheBackend` as the same-object contract for cache backends: callers get the reader surface from `CCoinsView`, and caches require a backend that also accepts batch writes. This keeps the writer requirement explicit without passing separate reader and writer instances, and without runtime detection or cast-based constructor plumbing. Cache-on-cache construction sites keep named `CCoinsViewCacheBackend&` bindings where needed to select the backend constructor without casts; direct construction from a `CCoinsViewCache` lvalue would otherwise bind to the deleted copy constructor.
Constructing a cache on top of another cache currently needs a named `CCoinsViewCacheBackend&` reference, because direct construction from a `CCoinsViewCache` lvalue would otherwise prefer the deleted copy constructor.
Add a non-const parent-cache constructor that delegates through the same backend initialization path, using private tag dispatch to avoid cast-based disambiguation. This keeps copy construction deleted while allowing `CCoinsViewCache child{parent};` to spell the layering intent directly.
Update the test helper to inherit the base constructors and add the same non-const parent-cache path for cache-test layers.
l0rinc
force-pushed
the
l0rinc/coins-view-interface-split
branch
from
8997e08 to
0e3f171
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
CCoinsViewcurrently exposes coin lookup, cache flushing, persistent storage traversal, storage sizing, and backend switching through the same base interface.This makes cache overlays, mempool views, error-catching views, and database-backed storage advertise capabilities that only some of them naturally own.
It also leaves the cache backend contract implicit: a cache must read misses and flush writes through the same state object, not through separate reader and writer instances.
Fix
This PR narrows
CCoinsViewto coin lookup and best-block reads, introducesCCoinsViewStoragefor persistent storage traversal and sizing, and introducesCCoinsViewCacheBackendfor same-object cache backends that can also receiveBatchWrite.Backend switching moves down to
CCoinsViewCache, the genericCCoinsViewBackedforwarding adapter is removed, and cache construction now takes live backend references.The stack also removes the stale
CTxMemPool::checkconst_castby making its cache-populating read side effect explicit in the parameter type.GetCoinandPeekCoinintentionally remain separate becauseCoinsViewOverlaystill needs a non-caching lookup path.A further split that lets temporary mempool overlays be read-only cache backends would be a natural follow-up, but this PR keeps the current flush behavior unchanged.