validation: merge `PeekCoin` into `GetCoin` #35078

Problem

Review on #34124 made it clear that the new PeekCoin method is hard to distinguish from the existing GetCoin at call sites where caching does not matter.

Fix

Merge PeekCoin into GetCoin by adding a peek_only flag to the CCoinsView interface and removing the separate virtual method. This preserves the non-caching lookup behavior in CCoinsViewCache and CoinsViewOverlay, while allowing backends without caches to ignore the flag.

Details

Tests and fuzz scaffolding now call GetCoin(..., true) only where side-effect-free reads matter, and CCoinsViewCache now shares the common result handling between peeking and caching lookups. The post-Flush() overlay assertions use plain GetCoin() because they only check the flushed spentness and do not need to preserve parent cache state.

Context

The unclear boundary between PeekCoin and GetCoin was flagged during review by:

• Russ Yanofsky in #34165 (review), whose original suggestion (prototyped in https://github.com/bitcoin/bitcoin/commit/642fa06f0af7321d35959f599e2edf74e72c6bd9) was a different approach - making GetCoin, PeekCoin, and HaveCoin non-virtual public methods implemented on top of two simpler virtual hooks (LookupCoin and MutableLookupCoin), so subclasses couldn't accidentally violate the caching contract. Russ noted that a fuller solution would enforce the distinction at compile time with const, which would in turn require splitting the coin lookup interface from the coin writing interface (dropping BatchWrite from the "view" side into a separate "store" interface).
• Anthony Towns in #34124 (comment), who sketched out the single-method GetCoin(outpoint, peek_only) shape this PR takes (see https://github.com/ajtowns/bitcoin/commit/bff58741b76ef4ca5f4a26da29e9582a49f71d2c). This change is forward-compatible with #31132 as well.

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35078.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #34864 (coins: make cache freshness imply dirtiness and remove invalid test states by l0rinc)
• #31132 (validation: fetch block inputs on parallel threads by andrewtoth)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

<!--85328a0da195eb286784d51f73fa0af9-->

🚧 At least one of the CI tasks failed. _{Task test ancestor commits: https://github.com/bitcoin/bitcoin/actions/runs/24478465952/job/71536804214} _{LLM reason (✨ experimental): CI failed due to a C++ build/compilation error while compiling txdb.cpp for bitcoin_node (bitcoin_node.dir/txdb.cpp.o).}

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

• Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

• A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

• An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

This is known as a Boolean Trap. Many design guidelines forbid the use of boolean parameters and instead recommend having separate functions. This PR seems to go into the opposite direction.

@purpleKarrot, I agree, it's an anti-pattern, it's why we avoided it originally, but in this case the end-result is easier to work with and several reviewers mentioned they would prefer it. Can you suggest a third version that's better than these two?

Maybe a Non-Virtual Interface? You could make the virtual function private and call it from two public functions.

Maybe a Non-Virtual Interface? You could make the virtual function private and call it from two public functions.

Yeah, my original suggestion (https://github.com/bitcoin/bitcoin/commit/642fa06f0af7321d35959f599e2edf74e72c6bd9 #34165 (review)) was to make GetCoin PeekCoin and HaveCoin nonvirtual and implement them in terms of virtual hooks for subclasses. I also gave further suggestions there for enforcing GetCoin/PeekCoin distinction at compile time with const, but these would require separating the coin lookup interface from the coin writing interface.

In any case 88928819632513b800b3415598bb4896037d60e8 seems like an improvement over status quo. IMO the booleans are not a significant problem, and could also be avoided with minimal changes by passing enum class values like CacheUpdate::Yes CacheUpdate::No.

So concept ACK for 88928819632513b800b3415598bb4896037d60e8 or anything similar. (Would maybe just clarify the citation in the description since I did suggest a different approach.)

Would maybe just clairfy description since I'm mentioned that I did suggest a different approach

Thanks for the reminder, I have updated the PR description, please let me know if it reflects your opinion accurately.

these would require separating the coin lookup interface from the coin writing interface

I rechecked your approach; splitting the reader and writer is something I would like to move toward. One of the goals of these cleanups was segregating the interface. Do you think this current PR gets us closer there? It makes a few other coins refactors simpler (noticed while rebasing them). I'm curious what others think - I don't mind pushing a full alternative PR based on @ryanofsky's prototype - I'd like to do it in small incremental steps.

Do you think this current PR gets us closer there?

Yes I think current PR is a basically a strict improvement. I just wanted to mention some other possible alternatives and goals. The only possible downside I see of current PR is that if you combine caching and noncaching lookups into one method it may require a few extra call sites to be updated later if separate const and non-const lookup methods are added. But this does not seem like a big deal since there is no const protection for the cache now. I guess I have a pretty clear idea of how I would design this API differently but many approaches seem reasonable here.

I do plan to review this too soon.

Drafting to receive more conceptual review: does the existing split make sense (similarly to the existing flush/sync split) or should we unify similar methods to minimize needless overwrites? If we're going towards splitting the reader and writer behavior (and the caching and other side-effects), merging these two (just to re-split them later) would be wasteful. Ping: @ryanofsky, @andrewtoth, @ajtowns, @optout21

Ditto #35078 (comment)

Changing the argument to an enum class (PEEK_ONLY ?) might be better if we're really worried about boolean arguments.

I think having cache/peek behaviour be part of the type/interface (dbcache->GetROView() vs dbcache->GetWritableView() or something) might be better, and would likely obsolete this change, but a change like that is probably a lot more work and will take a while, so doing something like this PR in the meantime would be worthwhile. YMMV.

Concept NACK 88928819632513b800b3415598bb4896037d60e8

Considering the pros and cons, I feel that the benefits are small. Moreover, this change would preclude the possibility of separating the immutable parts of the interface, hence I'd argue against it.

Background. The implementation of parallel input fetch (#31132) called for a non-caching version of GetCoin(), so PeekCoin() was introduced recently as a separate method (#34165).

My pro/con analysis:

Pro:

• One less method in the interface(s), some less source code lines
• Minimal performance: one less entry in coins cache instances' vtable.
• Unified implementation of the two flavors

Con:

• Less clear conceptual separation between the two flavors.
• Minimal performance: some overhead due to an extra bool parameter.

However, my main issue is that with a common method it is not possible to place them in separate interfaces, should it be separated into 2 (or 3) levels based on mutability (immutable read, cache-mutable read & write). I've made a comment on [#31132](/bitcoin-bitcoin/31132/) that it would be nice if there was some extra code-level safeguard that the multithreaded accesses make only cache-immutable calls. One way to do this is separating off the cache-immutable read part of the CCoinsViewCache interface (e.g. see https://github.com/optout21/bitcoin/commit/a15d2f9ca4899efd24994fadac49d6c6e0fb6c49). The present PR would make it impossible to separate the non-caching and caching versions into different interfaces. (Note: another experiment at interface separation in: https://github.com/optout21/bitcoin/pull/6 .)

Thanks, I will investigate if we can split out the sub-interfaces without needing to merge these two - drafted in the meantime.