fuzz: fix a couple incorrect assertions in the coins_view target
#28215
Conversation
It is incorrect to assert that `cache.HaveCoin()` will always be `true` if `backend.HaveCoin()` is. The coin could well have been marked as spent in the cache but not yet flushed, in which case `cache.HaveCoin()` would return `false`. Note this was never hit because `exists_using_have_coin_in_backend` is currently never `true` (it's the default implementation of `CCoinsView`. However this might change if we were to add a target where the backend is a `CCoinsViewDB`.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
Concept ACK |
There was a problem hiding this comment.
lgtm (only one small suggestion inline)
See commits messages for details.
I'd prefer if you summarize what the PR does in the description. Mostly the fact that these assertions are only correct if we use the default CCoinsView but incorrect if we were using an actual backend impl.
Again, this was not hit because the default implementation of `CCoinsView` return `false` for `GetCoin`.
Fair enough, my PR description was sloppy. Done.
I would disagree here. While it's true the code is "correct" as in the fuzz target doesn't crash, the logic itself is not correct:
|
darosior
force-pushed
the
fix_coins_view_fuzz_target
branch
from
4f62dca
to
e417c98
Compare
…coins_view` target e417c98 fuzz: coins_view: remove an incorrect assertion (Antoine Poinsot) c5f6b1d fuzz: coins_view: correct an incorrect assertion (Antoine Poinsot) Pull request description: The `coins_view` fuzz target would assert in two places that the cache is consistent with the backend. But it's never the case (that's the whole point of using a cache). The only reason this didn't result in a crash was that we would never actually hit these assertions. I ran into this while introducing a new target with an in-memory `CCoinsViewDB` as the backend view (see bitcoin#28216) which made the code paths with those assertions actually reachable. ACKs for top commit: dergoegge: Code review ACK e417c98 Tree-SHA512: 5847bb2744a2f2831dace62d32b79cc491bf54e2af4ce425411d245d566622d9aff816d9be5ec8e830d10851c13f2500bf4f0c004d88b4d7cca1d483ef8960a6
The
coins_viewfuzz target would assert in two places that the cache is consistent with the backend. But it's never the case (that's the whole point of using a cache).The only reason this didn't result in a crash was that we would never actually hit these assertions. I ran into this while introducing a new target with an in-memory
CCoinsViewDBas the backend view (see #28216) which made the code paths with those assertions actually reachable.