Skip to content

fix: properly reset coins_view_cache after catching flush exception#34647

Closed
andrewtoth wants to merge 1 commit intobitcoin:masterfrom
andrewtoth:fix_coins_view_fuzzer
Closed

fix: properly reset coins_view_cache after catching flush exception#34647
andrewtoth wants to merge 1 commit intobitcoin:masterfrom
andrewtoth:fix_coins_view_fuzzer

Conversation

@andrewtoth
Copy link
Contributor

@andrewtoth andrewtoth commented Feb 21, 2026

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

@andrewtoth
fix: properly reset coins_view_cache after catching flush exception
e994c4b 
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.
@andrewtoth andrewtoth mentioned this pull request Feb 21, 2026
Closed
@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 21, 2026 •
edited
Loading

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
Copy link
Member

sipa commented Feb 21, 2026

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: sipa@2170c44.

sipa
sipa reviewed Feb 21, 2026
View reviewed changes
Copy link
Member

@sipa sipa left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@fanquake fanquake added this to the 31.0 milestone Feb 22, 2026
@andrewtoth
Copy link
Contributor Author

andrewtoth commented Feb 22, 2026

Fuzzed overnight ~57M iterations no issues.

@sipa
Copy link
Member

sipa commented Feb 22, 2026 •
edited
Loading

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: bitcoin-core/qa-assets#258

@sipa sipa mentioned this pull request Feb 22, 2026
Closed
@l0rinc l0rinc mentioned this pull request Feb 22, 2026
Closed
@l0rinc
Copy link
Contributor

l0rinc commented Feb 22, 2026 •
edited
Loading

I don't like this direction. As I already commented on this fuzzer in #34165 (review):

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:

bitcoin/src/coins.h

Lines 551 to 555 in d9c7364

//! When check is true, the underlying view may be queried to determine whether an addition is
//! an overwrite.
// TODO: pass in a boolean to limit these possible overwrites to known
// (pre-BIP34) cases.
void AddCoins(CCoinsViewCache& cache, const CTransaction& tx, int nHeight, bool check = false);
Calling it in the fuzzer without any prior state-check

bitcoin/src/test/fuzz/coins_view.cpp

Lines 285 to 287 in d9c7364

const bool possible_overwrite = fuzzed_data_provider.ConsumeBool();
try {
AddCoins(coins_view_cache, transaction, height, possible_overwrite);
makes the call non-representative. Similarly,

bitcoin/src/coins.h

Lines 434 to 438 in d9c7364

/**
* Add a coin. Set possible_overwrite to true if an unspent version may
* already exist in the cache.
*/
void AddCoin(const COutPoint& outpoint, Coin&& coin, bool possible_overwrite);
warns us but we just ignore it in the fuzzer

bitcoin/src/test/fuzz/coins_view.cpp

Lines 123 to 125 in d9c7364

const bool possible_overwrite{fuzzed_data_provider.ConsumeBool()};
try {
coins_view_cache.AddCoin(outpoint, std::move(coin), possible_overwrite);

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 l0rinc#124, running the 4 fuzzers @sipa also mentioned during the night to see if this fixes the problem.

Edit: the fuzzers passed, pushed to #34655

@l0rinc l0rinc mentioned this pull request Feb 23, 2026
Merged
@andrewtoth
Copy link
Contributor Author

andrewtoth commented Feb 23, 2026

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 andrewtoth closed this Feb 23, 2026
achow101 added a commit that referenced this pull request Feb 24, 2026
@achow101
Merge #34655: fuzz: keep coins_view fuzzers within caller contracts
76eb04b 
3281824 fuzz: prevent invalid `FRESH` entries and surface `BatchWrite` errors (Lőrinc)
780f460 fuzz: avoid invalid `AddCoin` overwrites (Lőrinc)
d7e0d51 fuzz: make `AddCoins` query view for overwrites (Lőrinc)
b8fa6f0 util: introduce `TrySub` to prevent unsigned underflow (Lőrinc)

Pull request description:

  ### Problem
  This is an alternative approach to #34647, fixes #34645.

  ### Fix
  First, add `CheckedSub` and use it for decrements of `m_dirty_count` and `cachedCoinsUsage`, so unsigned underflows turn into immediate failures instead of silently wrapping and only failing later.

  <details><summary>Assertion `j <= i' failed.</summary>

  ```bash
  util/overflow.h:44 T CheckedSub(const T, const U) [T = unsigned long, U = bool]: Assertion `j <= i' failed.
  ==72817== ERROR: libFuzzer: deadly signal
      #0 0x556e9225eab5 in __sanitizer_print_stack_trace (/mnt/my_storage/bitcoin/build_fuzz/bin/fuzz+0x191dab5) (BuildId: d77c4d5f9dfd38ea06fab463f49341735205e109)
      #1 0x556e921acafc in fuzzer::PrintStackTrace() (/mnt/my_storage/bitcoin/build_fuzz/bin/fuzz+0x186bafc) (BuildId: d77c4d5f9dfd38ea06fab463f49341735205e109)
      #2 0x556e92191bb7 in fuzzer::Fuzzer::CrashCallback() (/mnt/my_storage/bitcoin/build_fuzz/bin/fuzz+0x1850bb7) (BuildId: d77c4d5f9dfd38ea06fab463f49341735205e109)
      #3 0x7164cfc458cf  (/lib/x86_64-linux-gnu/libc.so.6+0x458cf) (BuildId: ae7440bbdce614e0e79280c3b2e45b1df44e639c)
      #4 0x7164cfca49bb in __pthread_kill_implementation nptl/pthread_kill.c:43:17
      #5 0x7164cfca49bb in __pthread_kill_internal nptl/pthread_kill.c:89:10
      #6 0x7164cfca49bb in pthread_kill nptl/pthread_kill.c:100:10
      #7 0x7164cfc4579d in raise signal/../sysdeps/posix/raise.c:26:13
      #8 0x7164cfc288cc in abort stdlib/abort.c:73:3
      #9 0x556e92f9d591 in assertion_fail(std::source_location const&, std::basic_string_view<char, std::char_traits<char>>) /mnt/my_storage/bitcoin/src/util/check.cpp:41:5
      #10 0x556e9250daf0 in bool&& inline_assertion_check<false, bool>(bool&&, std::source_location const&, std::basic_string_view<char, std::char_traits<char>>) /mnt/my_storage/bitcoin/src/util/check.h:90:13
      #11 0x556e9250daf0 in unsigned long CheckedSub<unsigned long, bool>(unsigned long, bool) /mnt/my_storage/bitcoin/src/util/overflow.h:44:5
      #12 0x556e9250daf0 in CoinsViewCacheCursor::NextAndMaybeErase(std::pair<COutPoint const, CCoinsCacheEntry>&) /mnt/my_storage/bitcoin/src/coins.h:282:25
      #13 0x556e92507eb2 in (anonymous namespace)::MutationGuardCoinsViewCache::BatchWrite(CoinsViewCacheCursor&, uint256 const&) /mnt/my_storage/bitcoin/src/test/fuzz/coins_view.cpp:90:75
      #14 0x556e92c17a2b in CCoinsViewCache::Flush(bool) /mnt/my_storage/bitcoin/src/coins.cpp:282:11
      #15 0x556e924fb732 in TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_1::operator()() const /mnt/my_storage/bitcoin/src/test/fuzz/coins_view.cpp:135:34
      #16 0x556e924fb732 in unsigned long CallOneOf<TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_0, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_1, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_2, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_3, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_4, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_5, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_6, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_7, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_8, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_9, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_10, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_11>(FuzzedDataProvider&, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_0, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_1, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_2, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_3, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_4, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_5, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_6, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_7, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_8, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_9, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_10, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_11) /mnt/my_storage/bitcoin/src/test/fuzz/util.h:42:27
      #17 0x556e924fb732 in TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool) /mnt/my_storage/bitcoin/src/test/fuzz/coins_view.cpp:114:9
      #18 0x556e92503b0c in coins_view_overlay_fuzz_target(std::span<unsigned char const, 18446744073709551615ul>) /mnt/my_storage/bitcoin/src/test/fuzz/coins_view.cpp:404:5
      #19 0x556e92bcb7a5 in std::function<void (std::span<unsigned char const, 18446744073709551615ul>)>::operator()(std::span<unsigned char const, 18446744073709551615ul>) const /usr/lib/gcc/x86_64-linux-gnu/15/../../../../include/c++/15/bits/std_function.h:593:9
      #20 0x556e92bcb7a5 in test_one_input(std::span<unsigned char const, 18446744073709551615ul>) /mnt/my_storage/bitcoin/src/test/fuzz/fuzz.cpp:88:5
      #21 0x556e92bcb7a5 in LLVMFuzzerTestOneInput /mnt/my_storage/bitcoin/src/test/fuzz/fuzz.cpp:216:5
      #22 0x556e9219318f in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/mnt/my_storage/bitcoin/build_fuzz/bin/fuzz+0x185218f) (BuildId: d77c4d5f9dfd38ea06fab463f49341735205e109)
      #23 0x556e92192799 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) (/mnt/my_storage/bitcoin/build_fuzz/bin/fuzz+0x1851799) (BuildId: d77c4d5f9dfd38ea06fab463f49341735205e109)
      #24 0x556e92194139 in fuzzer::Fuzzer::MutateAndTestOne() (/mnt/my_storage/bitcoin/build_fuzz/bin/fuzz+0x1853139) (BuildId: d77c4d5f9dfd38ea06fab463f49341735205e109)
      #25 0x556e92194c95 in fuzzer::Fuzzer::Loop(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (/mnt/my_storage/bitcoin/build_fuzz/bin/fuzz+0x1853c95) (BuildId: d77c4d5f9dfd38ea06fab463f49341735205e109)
      #26 0x556e92181255 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/mnt/my_storage/bitcoin/build_fuzz/bin/fuzz+0x1840255) (BuildId: d77c4d5f9dfd38ea06fab463f49341735205e109)
      #27 0x556e921ad696 in main (/mnt/my_storage/bitcoin/build_fuzz/bin/fuzz+0x186c696) (BuildId: d77c4d5f9dfd38ea06fab463f49341735205e109)
      #28 0x7164cfc2a577 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
      #29 0x7164cfc2a63a in __libc_start_main csu/../csu/libc-start.c:360:3
      #30 0x556e921757e4 in _start (/mnt/my_storage/bitcoin/build_fuzz/bin/fuzz+0x18347e4) (BuildId: d77c4d5f9dfd38ea06fab463f49341735205e109)

  NOTE: libFuzzer has rudimentary signal handlers.
        Combine libFuzzer with AddressSanitizer or similar for better crash reports.
  SUMMARY: libFuzzer: deadly signal
  MS: 2 PersAutoDict-CopyPart- DE: "\005\000"-; base unit: ecb626aff8724f0fdde38a0a6965718f2096d474
  artifact_prefix='/tmp/fuzz_artifacts/'; Test unit written to /tmp/fuzz_artifacts/crash-1d19026c1a23f08bfe693fd684a56ce51187c6e5
  ./build_fuzz/bin/fuzz /tmp/fuzz_corpus/coins_view_overlay -max_total_time=3600 -rss_limit_mb=2560 -artifact_prefix=/tmp/fuzz_artifacts/ >fuzz-16.log 2>&1
  ```

  </details>

  The coins view fuzz targets can call `AddCoin`/`AddCoins` and construct `BatchWrite` cursors in ways that violate `CCoinsViewCache` caller contracts. These invalid states can trigger `BatchWrite` `std::logic_error` and can desync dirty-entry accounting (caught by `Assume(m_dirty_count == 0)` currently).

  Make the fuzzer avoid generating invalid states instead of catching and resetting:
  * Derive `AddCoin`’s `possible_overwrite` from `PeekCoin`, so `possible_overwrite=false` is only used when the outpoint is absent - similarly to https://github.com/bitcoin/bitcoin/blob/67c0d1798e6147f48d4bafc2c9e5ff30f2a62340/src/test/fuzz/coinscache_sim.cpp#L312-L317
  - Only use `AddCoins(check=false)` when we have confirmed the txid has no unspent outputs; otherwise fall back to `check=true` so `AddCoins` determines overwrites via the view.
  - When constructing a `CoinsViewCacheCursor`, avoid setting `FRESH` when the parent already has an unspent coin, and ensure `FRESH` implies `DIRTY`.

  ### Fuzzing
  The original error could be reproduced in ~10 minutes using `coins_view_overlay`. I ran the `coins_view`, `coins_view_db`, `coins_view_overlay`, and `coinscache_sim` fuzzers for this PR overnight and they didn't fail anymore.

ACKs for top commit:
  achow101:
    ACK 3281824
  sipa:
    ACK 3281824. Ran the 4 relevant fuzz tests for ~1 CPU day each. Will run more overnight.
  andrewtoth:
    ACK 3281824

Tree-SHA512: b8155e8d21740eb7800e373c27a8a1457eb84468c24af879bac5a1ed251ade2aec99c34a350a31f2ebb74e41bb7380bf20214d38d14fe23310a43282d2434fb7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sipa sipa sipa left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

31.0

Development

Successfully merging this pull request may close these issues.

oss-fuzz: coins_view_overlay: ASSERT: m_dirty_count == 0

5 participants

@andrewtoth @DrahtBot @sipa @l0rinc @fanquake