fix: properly reset coins_view_cache after catching flush exception

andrewtoth commented at 8:47 pm on February 21, 2026: contributor

The parent cache tries to reset the child cache’s state by iterating through the cursor. This doesn’t work since the cursor may have already been partially iterated. When we start again at Begin() it may double decrement m_dirty_count. Fix this by throwing the error and letting the child cache catch it and reset its own state properly.

Fixes #34645

The parent cache tries to reset the child cache's state by iterating
through the cursor. This doesn't work since the cursor may have already
been partially iterated. When we start again at Begin() it may double
decrement m_dirty_count. Fix this by throwing the error and letting
the child cache catch it and reset its own state properly.

e994c4b582

DrahtBot commented at 8:47 pm on February 21, 2026: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	sipa

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34655 (fuzz: keep coins_view fuzzers within caller contracts by l0rinc)

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.

sipa commented at 8:53 pm on February 21, 2026: member

Fuzzers running.

I had Claude Code (Sonnet 4.6) look at the fuzz failure in #34645, and it came up with the following fix, which looks correct - but I like your solution more. This is an uninteresting test case, which is just unfortunately hard to make the harness avoid. We shouldn’t complicate production code like this to deal better with uninteresting test scenarios: https://github.com/sipa/bitcoin/commit/2170c44e645bd7a843eaf06fdc143d21c8ba7d08.

sipa commented at 10:34 pm on February 21, 2026: member

Code review ACK e994c4b5824c55c207d35ca0c1ca75ddc414769c. I think this is a better approach than the old one (ignoring the fact that the old one was broken).

fanquake added this to the milestone 31.0 on Feb 22, 2026

andrewtoth commented at 2:52 pm on February 22, 2026: contributor

Fuzzed overnight ~57M iterations no issues.

sipa commented at 3:00 pm on February 22, 2026: member

I ran the 4 relevant fuzzers (coinscache_sim, coins_view, coins_view_db, coins_view_overlay) for ~11 hours on 16 cores each, no issues: https://github.com/bitcoin-core/qa-assets/pull/258

l0rinc commented at 10:21 pm on February 22, 2026: contributor

I don’t like this direction. As I already commented on this fuzzer in #34165#pullrequestreview-3751479287:

I don’t like the new fuzzers as they are, we’re testing more invalid states now. Instead of catching std::logic_error, we should investigate where our setup wasn’t realistic and fix that, instead of just yolo-mutating the state in ways that the production code cannot. The tests should help increase our confidence in the code; catching std::logic_error won’t help with that.

My confidence in the fuzzers after these changes is reduced even further.

I understand the concern about making fuzzers “too smart” (and introducing bias), but right now some targets here don’t model anything real. We keep expanding the explored state space to include more and more situations that cannot occur in reality and fail unreliably. The added try/catch blocks and combinations of random parameters that cannot coexist in production also make it harder to understand what is actually being tested.

For example, AddCoins documents that check controls whether the view is queried for overwrites (with coinbase being a special case), and the comment explicitly references pre-BIP30 duplicate coinbases: https://github.com/bitcoin/bitcoin/blob/d9c7364ac56781a16c7224b2c7a6db9db97f17d8/src/coins.h#L551-L555 Calling it in the fuzzer without any prior state-check https://github.com/bitcoin/bitcoin/blob/d9c7364ac56781a16c7224b2c7a6db9db97f17d8/src/test/fuzz/coins_view.cpp#L285-L287 makes the call non-representative. Similarly, https://github.com/bitcoin/bitcoin/blob/d9c7364ac56781a16c7224b2c7a6db9db97f17d8/src/coins.h#L434-L438 warns us but we just ignore it in the fuzzer https://github.com/bitcoin/bitcoin/blob/d9c7364ac56781a16c7224b2c7a6db9db97f17d8/src/test/fuzz/coins_view.cpp#L123-L125

If that path fails or passes, it doesn’t tell us much about the health of the application.

So the underlying problem seems to come from the check_for_overwrite in AddCoins and the possible_overwrite in AddCoin - maybe we could use the new PeekCoin to only allow skipping these when no unspent coin exists for this outpoint.

I pushed a draft to https://github.com/l0rinc/bitcoin/pull/124, running the 4 fuzzers @sipa also mentioned during the night to see if this fixes the problem.

Edit: the fuzzers passed, pushed to https://github.com/bitcoin/bitcoin/pull/34655

andrewtoth commented at 5:00 pm on February 23, 2026: contributor

Closing in favor of #34655. We don’t handle these exceptions in production code, so throwing them should break in the fuzzer too. Guiding the fuzzer to not intentionally produce invalid states is a better solution than trying to contort the test to allow them.

andrewtoth closed this on Feb 23, 2026

fix: properly reset coins_view_cache after catching flush exception #34647

Reviews

Conflicts