coins_view target
#28215
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 #28216) which made the code paths with those assertions actually reachable.
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.
See 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.
Reviewers, 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.
164 Coin coin_using_backend_get_coin;
165 if (backend_coins_view.GetCoin(random_out_point, coin_using_backend_get_coin)) {
166 assert(exists_using_have_coin_in_backend);
167- assert(coin_using_get_coin == coin_using_backend_get_coin);
168+ // Note we can't assert that `coin_using_get_coin == coin_using_backend_get_coin` because the coin in
169+ // the cache may have been modified above in the cache but not yet flushed.
0 // Note we can't assert that `coin_using_get_coin == coin_using_backend_get_coin` because the coin in
1 // the cache may have been modified but not yet flushed.
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.
154@@ -155,8 +155,9 @@ FUZZ_TARGET(coins_view, .init = initialize_coins_view)
155 }
156 assert((exists_using_access_coin && exists_using_have_coin_in_cache && exists_using_have_coin && exists_using_get_coin) ||
157 (!exists_using_access_coin && !exists_using_have_coin_in_cache && !exists_using_have_coin && !exists_using_get_coin));
158+ // If HaveCoin on the backend is true, it must also be on the cache if the coin wasn't spent.
159 const bool exists_using_have_coin_in_backend = backend_coins_view.HaveCoin(random_out_point);
160- if (exists_using_have_coin_in_backend) {
161+ if (!coin_using_access_coin.IsSpent() && exists_using_have_coin_in_backend) {
In https://github.com/bitcoin/bitcoin/pull/28215/commits/c5f6b1db56f67f529377bfb61f58c0a8c17b0127:
0Note this was never hit because `exists_using_have_coin_in_backend` is
1currently never `true` (it's the default implementation of `CCoinsView`.
2However this might change if we were to add a target where the backend
3is a `CCoinsViewDB`.
What do you mean by “add a target”? Because if exists_using_have_coin_in_backend is always false, (!coin_using_access_coin.IsSpent() && exists_using_have_coin_in_backend) will never be reachable, so wouldn’t it be better to remove it? Or do you intend to modify this target to use CCoinsViewDB? Sorry if I’m missing anything.
Again, this was not hit because the default implementation of
`CCoinsView` return `false` for `GetCoin`.
I’d prefer if you summarize what the PR does in the description.
Fair enough, my PR description was sloppy. Done.
Mostly the fact that these assertions are only correct if we use the default CCoinsView
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: