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.

<!--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)
• #34132 (coins: drop error catcher, centralize fatal read handling 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.