coins: use dirty entry count for flush warnings and disk space checks #33512

pull l0rinc wants to merge 3 commits into bitcoin:master from l0rinc:l0rinc/warn-dirty-coin-coint changing 6 files +68 −26
  1. l0rinc commented at 2:24 AM on October 1, 2025: contributor

    Problem

    Now that non-wiping flushes are possible (#28280, #28233), the cache may be mostly clean at flush time. But the flush warning, disk-space check, and benchmark logging still used total cache size, so a node with a 10 GiB cache that only needs to write a small fraction of dirty entries could still trigger a scary warning via the disk-space checks.

    The previous DynamicMemoryUsage metric was also fundamentally wrong for estimating disk writes, even before non-wiping flushes. In-memory coin size differs from on-disk write size due to LevelDB overhead, log doubling, and compaction.

    The warning also only fired in FlushStateToDisk, so AssumeUTXO snapshot loads never warned at all.

    Fix

    This PR tracks the actual number of dirty entries via m_dirty_count in CCoinsViewCache, maintained alongside the existing dirty-flag linked list, SanityCheck cross-validating both counts.

    The warning and benchmark log move from FlushStateToDisk down to CCoinsViewDB::BatchWrite, where the actual I/O happens. This is the single place all flush paths converge (regular flushes, syncs, and snapshot loads), so the warning now fires correctly for AssumeUTXO too. The threshold changes from 1 GiB of memory to 10 million dirty entries, which is roughly equivalent but avoids the in-memory vs on-disk size confusion.

    The disk-space safety check now uses GetDirtyCount() with the existing conservative 48-byte-per-entry estimate, preventing unnecessary shutdowns when the cache is large but mostly clean.

    Note: the first commit adds fuzz coverage for EmplaceCoinInternalDANGER in SimulationTest to exercise the accounting paths before modifying them. Note: this is a revival of #31703 with all outstanding review feedback addressed.

  2. DrahtBot commented at 2:24 AM on October 1, 2025: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33512.

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK andrewtoth, Eunovo, sipa, sedited

    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:

    • #34514 (refactor: remove unnecessary std::move for trivially copyable types by l0rinc)
    • #34435 (refactor: use _MiB/_GiB consistently for byte conversions by l0rinc)
    • #34124 (refactor: make CCoinsView a pure virtual interface by l0rinc)
    • #33018 (coins: remove SetFresh method from CCoinsCacheEntry by andrewtoth)
    • #30342 (kernel, logging: Pass Logger instances to kernel objects by ryanofsky)
    • #29700 (kernel, refactor: return error status on all fatal errors by ryanofsky)

    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-->

  3. fanquake commented at 2:49 AM on October 1, 2025: member

    https://github.com/bitcoin/bitcoin/actions/runs/18149289661/job/51656823259?pr=33512#step:9:2059:

    Run coins_view_db with args ['/home/admin/actions-runner/_work/_temp/build/bin/fuzz', '-runs=1', PosixPath('/home/admin/actions-runner/_work/_temp/ci/scratch/qa-assets/fuzz_corpora/coins_view_db')]INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 1436700023
INFO: Loaded 1 modules   (611994 inline 8-bit counters): 611994 [0x6082fc8a27f8, 0x6082fc937e92), 
INFO: Loaded 1 PC tables (611994 PCs): 611994 [0x6082fc937e98,0x6082fd28e838), 
INFO:     2396 files found in /home/admin/actions-runner/_work/_temp/ci/scratch/qa-assets/fuzz_corpora/coins_view_db
INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 275937 bytes
INFO: seed corpus: files: 2396 min: 2b max: 275937b total: 27207582b rss: 109Mb
/home/admin/actions-runner/_work/_temp/src/coins.h:288:23: runtime error: unsigned integer overflow: 0 - 1 cannot be represented in type 'size_t' (aka 'unsigned long')
    [#0](/bitcoin-bitcoin/0/) 0x6082f97eb6c4 in CoinsViewCacheCursor::NextAndMaybeErase(std::pair<COutPoint const, CCoinsCacheEntry>&) /home/admin/actions-runner/_work/_temp/build/src/test/fuzz/./coins.h:288:23
    [#1](/bitcoin-bitcoin/1/) 0x6082fb3e30bb in CCoinsViewDB::BatchWrite(CoinsViewCacheCursor&, uint256 const&) /home/admin/actions-runner/_work/_temp/build/src/./txdb.cpp:137:21
    [#2](/bitcoin-bitcoin/2/) 0x6082f9ec4573 in CCoinsViewCache::Flush() /home/admin/actions-runner/_work/_temp/build/src/./coins.cpp:265:22
    [#3](/bitcoin-bitcoin/3/) 0x6082f97cb77f in TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_0::operator()() const /home/admin/actions-runner/_work/_temp/build/src/test/fuzz/./test/fuzz/coins_view.cpp:77:48
    [#4](/bitcoin-bitcoin/4/) 0x6082f97cb77f in unsigned long CallOneOf<TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_0, TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_1, TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_2, TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_3, TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_4, TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_5, TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_6, TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_7, TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_8, TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_9, TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_10>(FuzzedDataProvider&, TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_0, TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_1, TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_2, TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_3, TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_4, TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_5, TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_6, TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_7, TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_8, TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_9, TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_10) /home/admin/actions-runner/_work/_temp/build/src/test/fuzz/./test/fuzz/util.h:42:27
    [#5](/bitcoin-bitcoin/5/) 0x6082f97cb77f in TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool) /home/admin/actions-runner/_work/_temp/build/src/test/fuzz/./test/fuzz/coins_view.cpp:56:9
    [#6](/bitcoin-bitcoin/6/) 0x6082f97cfc0c in coins_view_db_fuzz_target(std::span<unsigned char const, 18446744073709551615ul>) /home/admin/actions-runner/_work/_temp/build/src/test/fuzz/./test/fuzz/coins_view.cpp:323:5
    [#7](/bitcoin-bitcoin/7/) 0x6082f9e6efae 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/13/../../../../include/c++/13/bits/std_function.h:591:9
    [#8](/bitcoin-bitcoin/8/) 0x6082f9e6efae in test_one_input(std::span<unsigned char const, 18446744073709551615ul>) /home/admin/actions-runner/_work/_temp/build/src/test/fuzz/util/./test/fuzz/fuzz.cpp:88:5
    [#9](/bitcoin-bitcoin/9/) 0x6082f9e6efae in LLVMFuzzerTestOneInput /home/admin/actions-runner/_work/_temp/build/src/test/fuzz/util/./test/fuzz/fuzz.cpp:216:5
    [#10](/bitcoin-bitcoin/10/) 0x6082f947d66f in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/home/admin/actions-runner/_work/_temp/build/bin/fuzz+0x1a8366f) (BuildId: c46e5f78c23c1d0b2ce0fb85b0817bc7fae97dd8)
    [#11](/bitcoin-bitcoin/11/) 0x6082f947cc79 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) (/home/admin/actions-runner/_work/_temp/build/bin/fuzz+0x1a82c79) (BuildId: c46e5f78c23c1d0b2ce0fb85b0817bc7fae97dd8)
    [#12](/bitcoin-bitcoin/12/) 0x6082f947e9e2 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (/home/admin/actions-runner/_work/_temp/build/bin/fuzz+0x1a849e2) (BuildId: c46e5f78c23c1d0b2ce0fb85b0817bc7fae97dd8)
    [#13](/bitcoin-bitcoin/13/) 0x6082f947ef00 in fuzzer::Fuzzer::Loop(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (/home/admin/actions-runner/_work/_temp/build/bin/fuzz+0x1a84f00) (BuildId: c46e5f78c23c1d0b2ce0fb85b0817bc7fae97dd8)
    [#14](/bitcoin-bitcoin/14/) 0x6082f946b585 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/home/admin/actions-runner/_work/_temp/build/bin/fuzz+0x1a71585) (BuildId: c46e5f78c23c1d0b2ce0fb85b0817bc7fae97dd8)
    [#15](/bitcoin-bitcoin/15/) 0x6082f9497946 in main (/home/admin/actions-runner/_work/_temp/build/bin/fuzz+0x1a9d946) (BuildId: c46e5f78c23c1d0b2ce0fb85b0817bc7fae97dd8)
    [#16](/bitcoin-bitcoin/16/) 0x756d565fa1c9  (/lib/x86_64-linux-gnu/libc.so.6+0x2a1c9) (BuildId: 274eec488d230825a136fa9c4d85370fed7a0a5e)
    [#17](/bitcoin-bitcoin/17/) 0x756d565fa28a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2a28a) (BuildId: 274eec488d230825a136fa9c4d85370fed7a0a5e)
    [#18](/bitcoin-bitcoin/18/) 0x6082f945fb54 in _start (/home/admin/actions-runner/_work/_temp/build/bin/fuzz+0x1a65b54) (BuildId: c46e5f78c23c1d0b2ce0fb85b0817bc7fae97dd8)

SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow /home/admin/actions-runner/_work/_temp/src/coins.h:288:23 
MS: 0 ; base unit: 0000000000000000000000000000000000000000
0x1,0xff,0xff,0xff,0xf2,0xfc,0xff,0x2f,0x1f,0xa9,0x5d,0x30,0x30,0x90,0x0,0x0,0x0,0x14,0x69,0x0,0x0,0x0,0x0,0x0,0x0,0xc1,0xc1,0xc1,0xc1,0xc1,0xc1,0xb7,0xc1,0xc1,0x5c,0x91,0x3b,0x72,0x0,0xff,0xfa,0x21,
\001\377\377\377\362\374\377/\037\251]00\220\000\000\000\024i\000\000\000\000\000\000\301\301\301\301\301\301\267\301\301\\\221;r\000\377\372!
artifact_prefix='./'; Test unit written to ./crash-ebc99ca6d40a3c7a37202cdba9a8b36a2cb6007b
Base64: Af////L8/y8fqV0wMJAAAAAUaQAAAAAAAMHBwcHBwbfBwVyRO3IA//oh

INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 1436700023
INFO: Loaded 1 modules   (611994 inline 8-bit counters): 611994 [0x6082fc8a27f8, 0x6082fc937e92), 
INFO: Loaded 1 PC tables (611994 PCs): 611994 [0x6082fc937e98,0x6082fd28e838), 
INFO:     2396 files found in /home/admin/actions-runner/_work/_temp/ci/scratch/qa-assets/fuzz_corpora/coins_view_db
INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 275937 bytes
INFO: seed corpus: files: 2396 min: 2b max: 275937b total: 27207582b rss: 109Mb
/home/admin/actions-runner/_work/_temp/src/coins.h:288:23: runtime error: unsigned integer overflow: 0 - 1 cannot be represented in type 'size_t' (aka 'unsigned long')
    [#0](/bitcoin-bitcoin/0/) 0x6082f97eb6c4 in CoinsViewCacheCursor::NextAndMaybeErase(std::pair<COutPoint const, CCoinsCacheEntry>&) /home/admin/actions-runner/_work/_temp/build/src/test/fuzz/./coins.h:288:23
    [#1](/bitcoin-bitcoin/1/) 0x6082fb3e30bb in CCoinsViewDB::BatchWrite(CoinsViewCacheCursor&, uint256 const&) /home/admin/actions-runner/_work/_temp/build/src/./txdb.cpp:137:21
    [#2](/bitcoin-bitcoin/2/) 0x6082f9ec4573 in CCoinsViewCache::Flush() /home/admin/actions-runner/_work/_temp/build/src/./coins.cpp:265:22
    [#3](/bitcoin-bitcoin/3/) 0x6082f97cb77f in TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_0::operator()() const /home/admin/actions-runner/_work/_temp/build/src/test/fuzz/./test/fuzz/coins_view.cpp:77:48
    [#4](/bitcoin-bitcoin/4/) 0x6082f97cb77f in unsigned long CallOneOf<TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_0, TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_1, TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_2, TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_3, TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_4, TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_5, TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_6, TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_7, TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_8, TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_9, TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_10>(FuzzedDataProvider&, TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_0, TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_1, TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_2, TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_3, TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_4, TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_5, TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_6, TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_7, TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_8, TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_9, TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool)::$_10) /home/admin/actions-runner/_work/_temp/build/src/test/fuzz/./test/fuzz/util.h:42:27
    [#5](/bitcoin-bitcoin/5/) 0x6082f97cb77f in TestCoinsView(FuzzedDataProvider&, CCoinsView&, bool) /home/admin/actions-runner/_work/_temp/build/src/test/fuzz/./test/fuzz/coins_view.cpp:56:9
    [#6](/bitcoin-bitcoin/6/) 0x6082f97cfc0c in coins_view_db_fuzz_target(std::span<unsigned char const, 18446744073709551615ul>) /home/admin/actions-runner/_work/_temp/build/src/test/fuzz/./test/fuzz/coins_view.cpp:323:5
    [#7](/bitcoin-bitcoin/7/) 0x6082f9e6efae 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/13/../../../../include/c++/13/bits/std_function.h:591:9
    [#8](/bitcoin-bitcoin/8/) 0x6082f9e6efae in test_one_input(std::span<unsigned char const, 18446744073709551615ul>) /home/admin/actions-runner/_work/_temp/build/src/test/fuzz/util/./test/fuzz/fuzz.cpp:88:5
    [#9](/bitcoin-bitcoin/9/) 0x6082f9e6efae in LLVMFuzzerTestOneInput /home/admin/actions-runner/_work/_temp/build/src/test/fuzz/util/./test/fuzz/fuzz.cpp:216:5
    [#10](/bitcoin-bitcoin/10/) 0x6082f947d66f in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/home/admin/actions-runner/_work/_temp/build/bin/fuzz+0x1a8366f) (BuildId: c46e5f78c23c1d0b2ce0fb85b0817bc7fae97dd8)
    [#11](/bitcoin-bitcoin/11/) 0x6082f947cc79 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) (/home/admin/actions-runner/_work/_temp/build/bin/fuzz+0x1a82c79) (BuildId: c46e5f78c23c1d0b2ce0fb85b0817bc7fae97dd8)
    [#12](/bitcoin-bitcoin/12/) 0x6082f947e9e2 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (/home/admin/actions-runner/_work/_temp/build/bin/fuzz+0x1a849e2) (BuildId: c46e5f78c23c1d0b2ce0fb85b0817bc7fae97dd8)
    [#13](/bitcoin-bitcoin/13/) 0x6082f947ef00 in fuzzer::Fuzzer::Loop(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (/home/admin/actions-runner/_work/_temp/build/bin/fuzz+0x1a84f00) (BuildId: c46e5f78c23c1d0b2ce0fb85b0817bc7fae97dd8)
    [#14](/bitcoin-bitcoin/14/) 0x6082f946b585 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/home/admin/actions-runner/_work/_temp/build/bin/fuzz+0x1a71585) (BuildId: c46e5f78c23c1d0b2ce0fb85b0817bc7fae97dd8)
    [#15](/bitcoin-bitcoin/15/) 0x6082f9497946 in main (/home/admin/actions-runner/_work/_temp/build/bin/fuzz+0x1a9d946) (BuildId: c46e5f78c23c1d0b2ce0fb85b0817bc7fae97dd8)
    [#16](/bitcoin-bitcoin/16/) 0x756d565fa1c9  (/lib/x86_64-linux-gnu/libc.so.6+0x2a1c9) (BuildId: 274eec488d230825a136fa9c4d85370fed7a0a5e)
    [#17](/bitcoin-bitcoin/17/) 0x756d565fa28a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2a28a) (BuildId: 274eec488d230825a136fa9c4d85370fed7a0a5e)
    [#18](/bitcoin-bitcoin/18/) 0x6082f945fb54 in _start (/home/admin/actions-runner/_work/_temp/build/bin/fuzz+0x1a65b54) (BuildId: c46e5f78c23c1d0b2ce0fb85b0817bc7fae97dd8)

SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow /home/admin/actions-runner/_work/_temp/src/coins.h:288:23 
MS: 0 ; base unit: 0000000000000000000000000000000000000000
0x1,0xff,0xff,0xff,0xf2,0xfc,0xff,0x2f,0x1f,0xa9,0x5d,0x30,0x30,0x90,0x0,0x0,0x0,0x14,0x69,0x0,0x0,0x0,0x0,0x0,0x0,0xc1,0xc1,0xc1,0xc1,0xc1,0xc1,0xb7,0xc1,0xc1,0x5c,0x91,0x3b,0x72,0x0,0xff,0xfa,0x21,
\001\377\377\377\362\374\377/\037\251]00\220\000\000\000\024i\000\000\000\000\000\000\301\301\301\301\301\301\267\301\301\\\221;r\000\377\372!
artifact_prefix='./'; Test unit written to ./crash-ebc99ca6d40a3c7a37202cdba9a8b36a2cb6007b
Base64: Af////L8/y8fqV0wMJAAAAAUaQAAAAAAAMHBwcHBwbfBwVyRO3IA//oh

⚠️ Failure generated from target with exit code 1: ['/home/admin/actions-runner/_work/_temp/build/bin/fuzz', '-runs=1', PosixPath('/home/admin/actions-runner/_work/_temp/ci/scratch/qa-assets/fuzz_corpora/coins_view_db')]
  4. l0rinc force-pushed on Oct 1, 2025
  5. l0rinc commented at 2:51 AM on October 1, 2025: contributor

    @fanquake the fuzz test is fixed in a separate PR, I have reverted a similar attempt here

  6. l0rinc force-pushed on Oct 1, 2025
  7. l0rinc renamed this:
    Use number of dirty cache entries in flush warnings/logs
    coins: use number of dirty cache entries in flush warnings/logs
    on Oct 1, 2025
  8. DrahtBot added the label UTXO Db and Indexes on Oct 1, 2025
  9. l0rinc force-pushed on Oct 1, 2025
  10. in src/test/coins_tests.cpp:198 in 7afa718777 outdated 
     193 | @@ -194,8 +194,11 @@ void SimulationTest(CCoinsView* base, bool fake_best_block)
 194 |                      (coin.IsSpent() ? added_an_entry : updated_an_entry) = true;
 195 |                      coin = newcoin;
 196 |                  }
 197 | -                bool is_overwrite = !coin.IsSpent() || m_rng.rand32() & 1;
 198 | -                stack.back()->AddCoin(COutPoint(txid, 0), std::move(newcoin), is_overwrite);
 199 | +                if (COutPoint op(txid, 0); !stack.back()->map().contains(op) && !coin.IsSpent() && m_rng.randbool()) {
 200 | +                    stack.back()->EmplaceCoinInternalDANGER(std::move(op), std::move(newcoin));
    andrewtoth commented at 1:35 AM on October 2, 2025:

    Adds test coverage by randomly calling EmplaceCoinInternalDANGER in SimulationTest to verify the accounting remains correct.

    Where do we verify that the accounting is correct? This also checks that the map does not contain this coin, so the if (inserted) { block will always be true and we never test the other case.

    l0rinc commented at 3:06 AM on October 2, 2025: 
        /**
     * Emplace a coin into cacheCoins without performing any checks, marking
     * the emplaced coin as dirty.
     *
     * NOT FOR GENERAL USE. Used only when loading coins from a UTXO snapshot.
     * [@sa](/bitcoin-bitcoin/contributor/sa/) ChainstateManager::PopulateAndValidateSnapshot()
     */
    void EmplaceCoinInternalDANGER(COutPoint&& outpoint, Coin&& coin);

    EmplaceCoinInternalDANGER is not meant to be used for an overwrite according to the docs, so to mimick the assumeutxo inserts I only called this when it's not already in the set. Not exactly sure why there's an if (inserted) { branch in EmplaceCoinInternalDANGER the first place, but don't want to modify too much here. What do you suggest?

    l0rinc commented at 3:21 PM on October 2, 2025:

    You also answered something similar in #31132 (review)

    andrewtoth commented at 2:49 PM on October 4, 2025:

    I see, so if this gets merged the test here can be modified in #31132 to test for insertion if already exists?

    I don't see where we verify the accounting though. You are referring to the cachedCoinsUsage accounting? This test doesn't seem to check that. Maybe that can be added?

    l0rinc commented at 4:34 AM on October 7, 2025:

    Thanks, added a fuzz test and allowed existing entries to be attempted in EmplaceCoinInternalDANGER

    l0rinc commented at 3:04 AM on October 8, 2025:

    Update: had to remove the fuzz test, the accounting is broken here, but it's fixed in another PR, see: #32313 (comment)

    andrewtoth commented at 12:10 AM on October 12, 2025:

    Does this PR depend on #32313?

    l0rinc commented at 12:27 AM on October 12, 2025:

    The two are related, but I made sure we can merge either of them - but the other one will likely need to be rebase after that

    sipa commented at 5:02 PM on October 17, 2025:

    I think this is resolved now, with the merge of #32313?

    andrewtoth commented at 5:44 PM on October 17, 2025:

    Yes, I think we can now remove the conditions !stack.back()->map().contains(op) && !coin.IsSpent().

    l0rinc commented at 5:45 AM on October 18, 2025:

    I wanted to make it more realistic, but the tests do pass now without them, thanks for the feedback, removed.

    l0rinc commented at 8:21 PM on October 20, 2025:

    Seems we were wrong, the test assumes that after this call the given COutPoint will correspond to the current coins - which isn't the case with EmplaceCoinInternalDANGER which never replaces, so I have reverted https://github.com/bitcoin/bitcoin/compare/62cb2a2ed84e695c0959d8e27fc5bedd9b5a135e..54151d612b63f4e42fe03a143540e597728a4cc3

    See the previous errors that the suggestions resulted in: https://github.com/bitcoin/bitcoin/actions/runs/18611576758/job/53070427535?pr=33512#step:7:40482

    andrewtoth commented at 11:34 PM on October 20, 2025:

    the test assumes that after this call the given COutPoint will correspond to the current coins - which isn't the case with EmplaceCoinInternalDANGER which never replaces

    Right, so we still need !stack.back()->map().contains(op) but we don't need the !coin.IsSpent() check. Also, a few lines up we randomly set the pubkey to unspendable. In AddCoin we check if it is unspendable and don't add it, but in EmplaceCoinInternalDANGER we would add it. So I think we should also not use EmplaceCoinInternalDANGER if newcoin.out.scriptPubKey.IsUnspendable().

  11. in src/test/coins_tests.cpp:102 in 6aecb42c40 outdated 
      98 | @@ -99,7 +99,7 @@ class CCoinsViewCacheTest : public CCoinsViewCache
  99 |  
 100 |      CCoinsMap& map() const { return cacheCoins; }
 101 |      CoinsCachePair& sentinel() const { return m_sentinel; }
 102 | -    size_t& usage() const { return cachedCoinsUsage; }
 103 | +    void AddUsage(size_t usage) const { cachedCoinsUsage += usage; }
    andrewtoth commented at 1:48 AM on October 2, 2025:

    The benefit of the change in this commit is unclear. I think it could just be removed to make review easier.

    l0rinc commented at 3:15 AM on October 2, 2025:

    it's meant to avoid structures like cache.usage() += InsertCoinsMapEntry(cache.map(), but I don't mind reverting if you find that more readable.

  12. in src/coins.cpp:215 in db66bc45bf outdated 
     209 | @@ -205,8 +210,9 @@ bool CCoinsViewCache::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &ha
 210 |                  } else {
 211 |                      entry.coin = it->second.coin;
 212 |                  }
 213 | -                cachedCoinsUsage += entry.coin.DynamicMemoryUsage();
 214 |                  CCoinsCacheEntry::SetDirty(*itUs, m_sentinel);
 215 | +                ++m_dirty_count;
 216 | +                cachedCoinsUsage += entry.coin.DynamicMemoryUsage();
    andrewtoth commented at 1:55 AM on October 2, 2025:

    Why was this line moved?

    l0rinc commented at 3:15 AM on October 2, 2025:

    To make sure the m_dirty_count updates and the cachedCoinsUsage updates are always done in the same order for consistency.

  13. in src/coins.cpp:280 in db66bc45bf outdated 
     277 |  {
 278 | -    auto cursor{CoinsViewCacheCursor(cachedCoinsUsage, m_sentinel, cacheCoins, /*will_erase=*/false)};
 279 | +    auto cursor{CoinsViewCacheCursor(cachedCoinsUsage, m_dirty_count, m_sentinel, cacheCoins, /*will_erase=*/false)};
 280 |      bool fOk = base->BatchWrite(cursor, hashBlock);
 281 |      if (fOk) {
 282 |          if (m_sentinel.second.Next() != &m_sentinel) {
    andrewtoth commented at 2:00 AM on October 2, 2025:

    Should we also throw here if m_dirty_count is not zero?

    l0rinc commented at 3:15 AM on October 2, 2025:

    Sure, we can add a Assume(m_dirty_count == 0); like in ReallocateCache

    andrewtoth commented at 1:40 AM on November 13, 2025:

    This behavior will conflict with #33866 then right? It will abort during tests if Sync is called with a raw CCoinsView. Should we throw instead and then we'll be able to catch it? Then whichever is merged second can add the catch condition?

  14. in src/txdb.cpp:157 in bedb06362f 
     153 | @@ -148,9 +154,9 @@ bool CCoinsViewDB::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashB
 154 |      batch.Erase(DB_HEAD_BLOCKS);
 155 |      batch.Write(DB_BEST_BLOCK, hashBlock);
 156 |  
 157 | -    LogDebug(BCLog::COINDB, "Writing final batch of %.2f MiB\n", batch.ApproximateSize() * (1.0 / 1048576.0));
 158 | +    LogDebug(BCLog::COINDB, "Writing final batch of %.2f MiB", batch.ApproximateSize() * (1.0 / 1048576.0));
    andrewtoth commented at 2:09 AM on October 2, 2025:

    This was touched just to remove the \n?

    l0rinc commented at 3:15 AM on October 2, 2025:

    to unify it with the line below - not sure why you'd want to leave them different, but reverted it.

  15. in src/validation.cpp:2888 in bedb06362f 
    2884 | @@ -2891,7 +2885,7 @@ bool Chainstate::FlushStateToDisk(
2885 |                      (uint32_t)mode,
2886 |                      (uint64_t)coins_count,
2887 |                      (uint64_t)coins_mem_usage,
2888 | -                    (bool)fFlushForPrune);
2889 | +                    fFlushForPrune);
    andrewtoth commented at 2:10 AM on October 2, 2025:

    Why was this line changed?

    l0rinc commented at 3:15 AM on October 2, 2025:

    since we're casting a boolean to boolean, but I've reverted it

  16. l0rinc force-pushed on Oct 2, 2025
  17. in src/coins.cpp:81 in 223c849071 outdated 
      75 | @@ -76,9 +76,6 @@ void CCoinsViewCache::AddCoin(const COutPoint &outpoint, Coin&& coin, bool possi
  76 |      bool inserted;
  77 |      std::tie(it, inserted) = cacheCoins.emplace(std::piecewise_construct, std::forward_as_tuple(outpoint), std::tuple<>());
  78 |      bool fresh = false;
  79 | -    if (!inserted) {
  80 | -        cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage();
  81 | -    }
  82 |      if (!possible_overwrite) {
  83 |          if (!it->second.coin.IsSpent()) {
  84 |              throw std::logic_error("Attempted to overwrite an unspent coin (when possible_overwrite is false)");
    andrewtoth commented at 2:50 PM on October 4, 2025:

    Could we add a unit test to trigger this accounting error?

    l0rinc commented at 4:36 AM on October 7, 2025:

    The modified fuzz already covers that - what would be the advantage of adding a unit test for it as well?

    andrewtoth commented at 12:08 AM on October 12, 2025:

    A unit test would check for regression on every test run.

    l0rinc commented at 3:06 AM on October 12, 2025:

    Here I'm only modifying them because the build was failing otherwise, but I have indeed added tests to #32313 which will ideally be merged before this. Either way the other one needs rebasing.

  18. l0rinc force-pushed on Oct 7, 2025
  19. l0rinc force-pushed on Oct 8, 2025
  20. l0rinc force-pushed on Oct 8, 2025
  21. DrahtBot added the label Needs rebase on Oct 15, 2025
  22. l0rinc force-pushed on Oct 16, 2025
  23. l0rinc commented at 12:33 AM on October 16, 2025: contributor

    Rebased after #32313, the PR is simpler and better tested this way.

  24. DrahtBot removed the label Needs rebase on Oct 16, 2025
  25. l0rinc force-pushed on Oct 18, 2025
  26. l0rinc force-pushed on Oct 20, 2025
  27. in src/test/coins_tests.cpp:198 in 62cb2a2ed8 
     194 | @@ -194,8 +195,11 @@ void SimulationTest(CCoinsView* base, bool fake_best_block)
 195 |                      (coin.IsSpent() ? added_an_entry : updated_an_entry) = true;
 196 |                      coin = newcoin;
 197 |                  }
 198 | -                bool is_overwrite = !coin.IsSpent() || m_rng.rand32() & 1;
 199 | -                stack.back()->AddCoin(COutPoint(txid, 0), std::move(newcoin), is_overwrite);
 200 | +                if (COutPoint op(txid, 0); !stack.back()->map().contains(op) && !coin.IsSpent() && m_rng.randbool()) {
    andrewtoth commented at 12:00 AM on October 21, 2025: 
                    if (COutPoint op(txid, 0); !stack.back()->map().contains(op) && !newcoin.out.scriptPubKey.IsUnspendable() && m_rng.randbool()) {
    l0rinc commented at 2:04 AM on October 21, 2025:

    Done, added you as coauthor, thanks

  28. l0rinc force-pushed on Oct 21, 2025
  29. l0rinc force-pushed on Nov 10, 2025
  30. l0rinc commented at 5:31 PM on November 10, 2025: contributor

    Rebased, ready for review.

  31. in src/test/coins_tests.cpp:671 in dcbc4c2fd3 outdated 
     666 | @@ -666,7 +667,8 @@ static void WriteCoinsViewEntry(CCoinsView& view, const MaybeCoin& cache_coin)
 667 |      CCoinsMapMemoryResource resource;
 668 |      CCoinsMap map{0, CCoinsMap::hasher{}, CCoinsMap::key_equal{}, &resource};
 669 |      if (cache_coin) InsertCoinsMapEntry(map, sentinel, *cache_coin);
 670 | -    auto cursor{CoinsViewCacheCursor(sentinel, map, /*will_erase=*/true)};
 671 | +    size_t dirty_count{cache_coin && cache_coin->IsDirty() ? 1U : 0U};
 672 | +    auto cursor{CoinsViewCacheCursor(dirty_count, sentinel, map, /*will_erase=*/true)};
    optout21 commented at 4:38 PM on November 15, 2025:

    Can a check on dirty_count be added after the BatchWrite below?

    l0rinc commented at 1:32 PM on November 21, 2025:

    You mean like:

        BOOST_CHECK_EQUAL(dirty_count, 0U);

    ? Done

    l0rinc commented at 5:10 PM on January 29, 2026:

    Thanks, added you as coauthor

  32. in src/coins.h:469 in dcbc4c2fd3 outdated 
     465 | @@ -461,6 +466,9 @@ class CCoinsViewCache : public CCoinsViewBacked
 466 |      //! Calculate the size of the cache (in number of transaction outputs)
 467 |      unsigned int GetCacheSize() const;
 468 |  
 469 | +    //! Calculate the number of dirty cache entries (transaction outputs)
    optout21 commented at 4:40 PM on November 15, 2025:

    Misleading comment, no calculation takes place here, it's just an accessor.

    l0rinc commented at 1:34 PM on November 21, 2025:

    Fair, changed both:

        //! Size of the cache (in number of transaction outputs)
    unsigned int GetCacheSize() const;

    //! Number of dirty cache entries (transaction outputs)
    size_t GetDirtyCount() const noexcept { return m_dirty_count; }
  33. in src/coins.cpp:104 in dcbc4c2fd3 outdated 
      95 | @@ -96,10 +96,12 @@ void CCoinsViewCache::AddCoin(const COutPoint &outpoint, Coin&& coin, bool possi
  96 |          fresh = !it->second.IsDirty();
  97 |      }
  98 |      if (!inserted) {
  99 | +        m_dirty_count -= it->second.IsDirty();
 100 |          cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage();
 101 |      }
 102 |      it->second.coin = std::move(coin);
 103 |      CCoinsCacheEntry::SetDirty(*it, m_sentinel);
 104 | +    ++m_dirty_count;
    optout21 commented at 5:00 PM on November 15, 2025:

    I suggest moving this line one line down, such that m_dirty_count and cachedCoinsUsage updates are on adjacent lines, as they are logically related.

    l0rinc commented at 1:43 PM on November 21, 2025:

    I have grouped dirty setter and dirty count updates deliberately:

    % git grep -A1 'CCoinsCacheEntry::SetDirty' src/coins.cpp
src/coins.cpp:    CCoinsCacheEntry::SetDirty(*it, m_sentinel);
src/coins.cpp-    ++m_dirty_count;
--
src/coins.cpp:        CCoinsCacheEntry::SetDirty(*it, m_sentinel);
src/coins.cpp-        ++m_dirty_count;
--
src/coins.cpp:        CCoinsCacheEntry::SetDirty(*it, m_sentinel);
src/coins.cpp-        ++m_dirty_count;
--
src/coins.cpp:                CCoinsCacheEntry::SetDirty(*itUs, m_sentinel);
src/coins.cpp-                ++m_dirty_count;
--
src/coins.cpp:                    CCoinsCacheEntry::SetDirty(*itUs, m_sentinel);
src/coins.cpp-                    ++m_dirty_count;
  34. optout21 commented at 5:13 PM on November 15, 2025: contributor

    A general observation: keeping a count like the dirty count is very efficient, but prone to going out of sync, e.g with future incorrect code changes. For eg., a missing decrement can result in the value creeping higher, a missing increment can result the value going lower, event to underflow -- size_t is unsigned. One mitigation is to create (inlinable) inc/dec methods, with sanity checks (in debug mode); pre-decrease the value cannot be 0, pre-increase it cannot be equal to the total entries. Event thought this value is used only for logging, checking for sub-0 underflow would make sense, as a guard against future bugs.

  35. l0rinc force-pushed on Nov 21, 2025
  36. l0rinc commented at 1:52 PM on November 21, 2025: contributor

    Thanks for the comments @optout21, since I wasn't invalidating any ACKs, I have applied your suggestions (with rebase) - let me know if this is what you meant.

    keeping a count like the dirty count is very efficient, but prone to going out of sync

    yeah, we have a few of these - but the sanitizers and sanity checks and fuzzers usually reveal the problems (sanitizers break on underflow and the sanity checks recalculate everything during testing).

  37. in src/coins.h:274 in 1d14979d5b outdated 
     270 | @@ -271,10 +271,11 @@ struct CoinsViewCacheCursor
 271 |      //! This is an optimization compared to erasing all entries as the cursor iterates them when will_erase is set.
 272 |      //! Calling CCoinsMap::clear() afterwards is faster because a CoinsCachePair cannot be coerced back into a
 273 |      //! CCoinsMap::iterator to be erased, and must therefore be looked up again by key in the CCoinsMap before being erased.
 274 | -    CoinsViewCacheCursor(CoinsCachePair& sentinel LIFETIMEBOUND,
 275 | +    CoinsViewCacheCursor(size_t& dirty_count LIFETIMEBOUND,
    andrewtoth commented at 2:16 AM on December 8, 2025:

    Do we really need to pass the dirty count here? We always iterate through the entire cursor, so we can just set m_dirty_count = 0; after a Sync or Flush (we already do for Flush).

    l0rinc commented at 10:17 AM on December 9, 2025:

    Hmm, how would you display the warning in https://github.com/bitcoin/bitcoin/blob/3631ca738f76266633dbb15b332fe59d5936179f/src/txdb.cpp#L116 without knowing the dirty count in advance?

    andrewtoth commented at 1:09 AM on January 16, 2026:

    Right, we need that since we're moving the checks to the db. You can resolve this conversation.

  38. DrahtBot added the label Needs rebase on Dec 11, 2025
  39. l0rinc force-pushed on Dec 11, 2025
  40. DrahtBot removed the label Needs rebase on Dec 11, 2025
  41. l0rinc commented at 1:07 PM on December 11, 2025: contributor

    Clean rebase, reviews are welcome again.

  42. DrahtBot added the label CI failed on Dec 11, 2025
  43. l0rinc force-pushed on Dec 11, 2025
  44. DrahtBot removed the label CI failed on Dec 11, 2025
  45. DrahtBot added the label Needs rebase on Jan 3, 2026
  46. l0rinc force-pushed on Jan 3, 2026
  47. l0rinc commented at 11:37 AM on January 3, 2026: contributor

    Rebased after #33866

    git range-diff -U0 master eac63b2 0ac4cb6:

    • Adjusted the Flush/Sync changes;
    • Removed a useless move from: stack.back()->AddCoin(op, std::move(newcoin), ...;

    Ready for review again!

  48. DrahtBot removed the label Needs rebase on Jan 3, 2026
  49. in src/coins.cpp:275 in 0ac4cb69b5 outdated 
     271 |      cacheCoins.clear();
 272 |      if (will_reuse_cache) {
 273 |          ReallocateCache();
 274 |      }
 275 |      cachedCoinsUsage = 0;
 276 | +    m_dirty_count = 0;
    andrewtoth commented at 12:30 AM on January 16, 2026:

    We call Assume(m_dirty_count == 0); in ReallocateCache(); above, but only set m_dirty_count = 0; here. We should just Assume(m_dirty_count == 0); after BatchWrite like we do with Sync.

    l0rinc commented at 5:10 PM on January 29, 2026:

    Thanks, good observation, adjusted and added you as coauthor.

  50. andrewtoth approved
  51. andrewtoth commented at 1:12 AM on January 16, 2026: contributor

    utACK 0ac4cb69b59f382bf45a3cacc97e49b1eea1dbe4

    This removes redundant logging of the large flush warning when running in pruned mode or in steady state, the latter of which happens more often now that we're flushing every hour.

    This also does a more accurate disk space check, which currently might needlessly shutdown nodes even though the flush is actually small and can fit in the available disk space.

  52. l0rinc force-pushed on Jan 28, 2026
  53. l0rinc commented at 11:04 PM on January 28, 2026: contributor

    Trivial rebase after #34207, ready for review again!

  54. in src/coins.cpp:332 in ea9a918666 
     328 | @@ -316,6 +329,7 @@ void CCoinsViewCache::SanityCheck() const
 329 |  {
 330 |      size_t recomputed_usage = 0;
 331 |      size_t count_dirty = 0;
 332 | +    size_t dirty_count = 0;
    andrewtoth commented at 2:18 AM on January 29, 2026:

    We don't need this dirty_count anymore, it is redundant with count_dirty.

    l0rinc commented at 5:10 PM on January 29, 2026:

    Hah, beautiful, fixed and adjusted the PR description.

  55. l0rinc force-pushed on Jan 29, 2026
  56. l0rinc force-pushed on Jan 29, 2026
  57. andrewtoth commented at 7:01 PM on February 1, 2026: contributor

    ACK 5c117ced207b8758926962ab5a355a5dffc408f6

  58. achow101 added this to the milestone 31.0 on Feb 5, 2026
  59. DrahtBot added the label Needs rebase on Feb 8, 2026
  60. fuzz: call `EmplaceCoinInternalDANGER` as well in `SimulationTest`
    Adds test coverage by randomly calling `EmplaceCoinInternalDANGER` in `SimulationTest` to verify it remains correct as we modify it in a future commit.

Co-authored-by: Andrew Toth <andrewstoth@gmail.com>
    7e52b1b945
  61. l0rinc force-pushed on Feb 8, 2026
  62. coins: Keep track of number of dirty entries in `CCoinsViewCache`
    Adds `m_dirty_count` member to track the running count of dirty cache entries as follows:
* Incremented when entries are marked dirty via `CCoinsCacheEntry::SetDirty`
* Decremented when dirty entries are removed or cleaned
* Passed through `CoinsViewCacheCursor` and updated during iteration

The dirty count is needed because after non-wiping flushes (introduced in #28280 and #28233), the percentage of dirty entries in the cache may be far below 100%. Using total cache size for flush warnings and disk space checks is therefore misleading.

Updates all test code to properly initialize and maintain the dirty count.

Co-authored-by: l0rinc <pap.lorinc@gmail.com>
Co-authored-by: Andrew Toth <andrewstoth@gmail.com>
Co-authored-by: optout <13562139+optout21@users.noreply.github.com>
    b413491a1c
  63. validation: Use dirty entry count in flush warnings and disk space checks
    Changes flush warnings to use the actual number of dirty entries being written rather than total cache size or memory usage:
* Moves warning from `FlushStateToDisk` to `CCoinsViewDB::BatchWrite` so it applies to both regular flushes and `AssumeUTXO` snapshot writes
* Changes threshold from `WARN_FLUSH_COINS_SIZE` (1 GiB) to `WARN_FLUSH_COINS_COUNT` (10M entries), approximately equivalent - this also helps with the confusion caused by UTXO size difference on-disk vs in-memory
* Moves benchmark logging to `BatchWrite` where the actual disk I/O occurs to make sure AssumeUTXO also warns
* Uses dirty count for disk space check (48 bytes per entry estimate)
* Removes redundant `changed` counter since `dirty_count` is now tracked

This ensures users are warned appropriately even when only a fraction of the cache is dirty, and provides accurate warnings during `AssumeUTXO` loads.

Co-authored-by: l0rinc <pap.lorinc@gmail.com>
    afb1bc120e
  64. l0rinc force-pushed on Feb 8, 2026
  65. DrahtBot added the label CI failed on Feb 8, 2026
  66. DrahtBot commented at 7:24 AM on February 8, 2026: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

    🚧 At least one of the CI tasks failed. <sub>Task Alpine (musl): https://github.com/bitcoin/bitcoin/actions/runs/21794006003/job/62878385889</sub> <sub>LLM reason (✨ experimental): txvalidationcache_tests failed with a subprocess abort (CoinsViewCache::Flush assertion).</sub>

    <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>

  67. DrahtBot removed the label Needs rebase on Feb 8, 2026
  68. DrahtBot removed the label CI failed on Feb 8, 2026
  69. l0rinc commented at 9:58 PM on February 8, 2026: contributor

    Rebased after silent conflict with https://github.com/bitcoin/bitcoin/pull/34164/changes#diff-f0ed73d62dae6ca28ebd3045e5fc0d5d02eaaacadb4c2a292985a3fbd7e1c77cR281. Ready for review again, would be great if it could make it into v31.

  70. andrewtoth approved
  71. andrewtoth commented at 4:40 PM on February 10, 2026: contributor

    re-ACK afb1bc120ecce2bf663093e15c93f5592c0d4a98

    Added m_dirty_count to Reset, and covered it in the Reset unit test.

  72. in src/test/coins_tests.cpp:193 in 7e52b1b945 outdated 
     188 | @@ -189,8 +189,11 @@ void SimulationTest(CCoinsView* base, bool fake_best_block)
 189 |                      (coin.IsSpent() ? added_an_entry : updated_an_entry) = true;
 190 |                      coin = newcoin;
 191 |                  }
 192 | -                bool is_overwrite = !coin.IsSpent() || m_rng.rand32() & 1;
 193 | -                stack.back()->AddCoin(COutPoint(txid, 0), std::move(newcoin), is_overwrite);
 194 | +                if (COutPoint op(txid, 0); !stack.back()->map().contains(op) && !newcoin.out.scriptPubKey.IsUnspendable() && m_rng.randbool()) {
 195 | +                    stack.back()->EmplaceCoinInternalDANGER(std::move(op), std::move(newcoin));
  73. in src/coins.cpp:236 in b413491a1c outdated 
     232 | @@ -227,6 +233,7 @@ void CCoinsViewCache::BatchWrite(CoinsViewCacheCursor& cursor, const uint256& ha
 233 |              if (itUs->second.IsFresh() && it->second.coin.IsSpent()) {
 234 |                  // The grandparent cache does not have an entry, and the coin
 235 |                  // has been spent. We can just delete it from the parent cache.
 236 | +                m_dirty_count -= itUs->second.IsDirty();
    Eunovo commented at 11:38 AM on February 16, 2026:

    https://github.com/bitcoin/bitcoin/pull/33512/commits/b413491a1cdd9a51f2aa10b775650f54f6785e3e:

    I noticed that this line is not covered in the test.

    l0rinc commented at 2:00 PM on February 16, 2026:

    The fuzzer should fail immediately if you remove that. And the Flush assertion is immediately triggered by the unit tests as well:

    Running 11 test cases...
./coins.cpp:268 void CCoinsViewCache::Flush(bool): Assertion `m_dirty_count == 0' failed.
unknown location(0): fatal error: in "coins_tests/updatecoins_simulation_test": signal: SIGABRT (application abort requested)
./test/coins_tests.cpp(457): last checkpoint
test_bitcoin: ./common/args.cpp:582: void ArgsManager::AddArg(const std::string&, const std::string&, unsigned int, const OptionsCategory&): Assertion `ret.second' failed.
unknown location(0): fatal error: in "coins_tests/ccoins_serialization": signal: SIGABRT (application abort requested)
./test/coins_tests.cpp(521): last checkpoint: "ccoins_serialization" fixture ctor
test_bitcoin: ./common/args.cpp:582: void ArgsManager::AddArg(const std::string&, const std::string&, unsigned int, const OptionsCategory&): Assertion `ret.second' failed.
unknown location(0): fatal error: in "coins_tests/ccoins_access": signal: SIGABRT (application abort requested)
./test/coins_tests.cpp(693): last checkpoint: "ccoins_access" fixture ctor
test_bitcoin: ./common/args.cpp:582: void ArgsManager::AddArg(const std::string&, const std::string&, unsigned int, const OptionsCategory&): Assertion `ret.second' failed.
unknown location(0): fatal error: in "coins_tests/ccoins_spend": signal: SIGABRT (application abort requested)
./test/coins_tests.cpp(723): last checkpoint: "ccoins_spend" fixture ctor
test_bitcoin: ./common/args.cpp:582: void ArgsManager::AddArg(const std::string&, const std::string&, unsigned int, const OptionsCategory&): Assertion `ret.second' failed.
unknown location(0): fatal error: in "coins_tests/ccoins_add": signal: SIGABRT (application abort requested)
./test/coins_tests.cpp(759): last checkpoint: "ccoins_add" fixture ctor
test_bitcoin: ./common/args.cpp:582: void ArgsManager::AddArg(const std::string&, const std::string&, unsigned int, const OptionsCategory&): Assertion `ret.second' failed.
unknown location(0): fatal error: in "coins_tests/ccoins_write": signal: SIGABRT (application abort requested)
./test/coins_tests.cpp(804): last checkpoint: "ccoins_write" fixture ctor
test_bitcoin: ./common/args.cpp:582: void ArgsManager::AddArg(const std::string&, const std::string&, unsigned int, const OptionsCategory&): Assertion `ret.second' failed.
unknown location(0): fatal error: in "coins_tests/ccoins_flush_behavior": signal: SIGABRT (application abort requested)
./test/coins_tests.cpp(1048): last checkpoint: "ccoins_flush_behavior" fixture ctor
test_bitcoin: ./common/args.cpp:582: void ArgsManager::AddArg(const std::string&, const std::string&, unsigned int, const OptionsCategory&): Assertion `ret.second' failed.
unknown location(0): fatal error: in "coins_tests/coins_resource_is_used": signal: SIGABRT (application abort requested)
./test/coins_tests.cpp(1062): last checkpoint: "coins_resource_is_used" fixture ctor
test_bitcoin: ./common/args.cpp:582: void ArgsManager::AddArg(const std::string&, const std::string&, unsigned int, const OptionsCategory&): Assertion `ret.second' failed.
unknown location(0): fatal error: in "coins_tests/ccoins_addcoin_exception_keeps_usage_balanced": signal: SIGABRT (application abort requested)
./test/coins_tests.cpp(1087): last checkpoint: "ccoins_addcoin_exception_keeps_usage_balanced" fixture ctor
test_bitcoin: ./common/args.cpp:582: void ArgsManager::AddArg(const std::string&, const std::string&, unsigned int, const OptionsCategory&): Assertion `ret.second' failed.
unknown location(0): fatal error: in "coins_tests/ccoins_emplace_duplicate_keeps_usage_balanced": signal: SIGABRT (application abort requested)
./test/coins_tests.cpp(1105): last checkpoint: "ccoins_emplace_duplicate_keeps_usage_balanced" fixture ctor
test_bitcoin: ./common/args.cpp:582: void ArgsManager::AddArg(const std::string&, const std::string&, unsigned int, const OptionsCategory&): Assertion `ret.second' failed.
unknown location(0): fatal error: in "coins_tests/ccoins_reset_guard": signal: SIGABRT (application abort requested)
./test/coins_tests.cpp(1123): last checkpoint: "ccoins_reset_guard" fixture ctor
    Eunovo commented at 2:28 PM on February 16, 2026:

    Commenting out this line doesn't break the unit tests, but I have confirmed that it does break the fuzzer.

    l0rinc commented at 3:27 PM on February 16, 2026:

    I ran

    cmake -B build -DCMAKE_BUILD_TYPE=Debug && \
cmake --build build -j$(nproc) && \
./build/bin/test_bitcoin --run_test=coins_tests

    with

    diff --git a/src/coins.cpp b/src/coins.cpp
index 1bc01d20a5..2fec6bec79 100644
--- a/src/coins.cpp
+++ b/src/coins.cpp
@@ -233,7 +233,7 @@ void CCoinsViewCache::BatchWrite(CoinsViewCacheCursor& cursor, const uint256& ha
             if (itUs->second.IsFresh() && it->second.coin.IsSpent()) {
                 // The grandparent cache does not have an entry, and the coin
                 // has been spent. We can just delete it from the parent cache.
-                m_dirty_count -= itUs->second.IsDirty();
+                // TODO m_dirty_count -= itUs->second.IsDirty();
                 cachedCoinsUsage -= itUs->second.coin.DynamicMemoryUsage();
                 cacheCoins.erase(itUs);
             } else {

    and got the error above:

    ./coins.cpp:268 void CCoinsViewCache::Flush(bool): Assertion `m_dirty_count == 0' failed

    Maybe you compiled in Release mode and the assertions were optimized out - can you retry in Debug mode instead?

    Eunovo commented at 7:25 AM on February 17, 2026:

    Confirmed.

  74. Eunovo commented at 12:00 PM on February 16, 2026: contributor

    Concept ACK https://github.com/bitcoin/bitcoin/pull/33512/commits/afb1bc120ecce2bf663093e15c93f5592c0d4a98

    I will test soon

  75. Eunovo commented at 9:30 AM on February 17, 2026: contributor

    Tested ACK https://github.com/bitcoin/bitcoin/commit/afb1bc120ecce2bf663093e15c93f5592c0d4a98

    Tested with this diff

    diff --git a/src/txdb.cpp b/src/txdb.cpp
index 2c39cf2767..21bc1c9ae2 100644
--- a/src/txdb.cpp
+++ b/src/txdb.cpp
@@ -27,7 +27,7 @@ static constexpr uint8_t DB_HEAD_BLOCKS{'H'};
 static constexpr uint8_t DB_COINS{'c'};
 
 // Threshold for warning when writing this many dirty cache entries to disk.
-static constexpr size_t WARN_FLUSH_COINS_COUNT{10'000'000};
+static constexpr size_t WARN_FLUSH_COINS_COUNT{298};
 
 bool CCoinsViewDB::NeedsUpgrade()
 {
diff --git a/test/functional/feature_assumeutxo.py b/test/functional/feature_assumeutxo.py
index 7420dfe02a..766b870948 100755
--- a/test/functional/feature_assumeutxo.py
+++ b/test/functional/feature_assumeutxo.py
@@ -524,7 +524,8 @@ class AssumeutxoTest(BitcoinTestFramework):
         self.log.info(f"Loading snapshot into second node from {dump_output['path']}")
         # This node's tip is on an ancestor block of the snapshot, which should
         # be the normal case
-        loaded = n1.loadtxoutset(dump_output['path'])
+        with n1.assert_debug_log(["Flushing large"], timeout=60):
+            loaded = n1.loadtxoutset(dump_output['path'])
         assert_equal(loaded['coins_loaded'], SNAPSHOT_BASE_HEIGHT)
         assert_equal(loaded['base_height'], SNAPSHOT_BASE_HEIGHT)
  76. sedited requested review from sipa on Feb 19, 2026
  77. sipa commented at 6:16 PM on February 19, 2026: member

    Code review ACK afb1bc120ecce2bf663093e15c93f5592c0d4a98

  78. in src/txdb.cpp:163 in afb1bc120e 
     159 | @@ -154,7 +160,7 @@ void CCoinsViewDB::BatchWrite(CoinsViewCacheCursor& cursor, const uint256& hashB
 160 |  
 161 |      LogDebug(BCLog::COINDB, "Writing final batch of %.2f MiB\n", batch.ApproximateSize() * (1.0 / 1048576.0));
 162 |      m_db->WriteBatch(batch);
 163 | -    LogDebug(BCLog::COINDB, "Committed %u changed transaction outputs (out of %u) to coin database...\n", (unsigned int)changed, (unsigned int)count);
 164 | +    LogDebug(BCLog::COINDB, "Committed %u changed transaction outputs (out of %u) to coin database...", (unsigned int)dirty_count, (unsigned int)count);
    sedited commented at 7:53 PM on February 19, 2026:

    Nit: Is there a reason why we are casting to unsigned int here?

    l0rinc commented at 7:57 PM on February 19, 2026:

    just to minimize the diff - we could have used %s instead which would avoid the cast. I took over the PR and didn't want to add extra cleanups.

    sipa commented at 8:00 PM on February 19, 2026:

    The cast isn't needed in any case; Log* functions use tinyformat, which doesn't care about the arguments' type matching the format string; %i %u %d ... are all equivalent.

  79. in src/test/fuzz/coins_view.cpp:142 in b413491a1c 
     138 | @@ -139,6 +139,7 @@ void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsView& backend
 139 |              [&] {
 140 |                  CoinsCachePair sentinel{};
 141 |                  sentinel.second.SelfRef(sentinel);
 142 | +                size_t dirty_count{0};
    sedited commented at 7:54 PM on February 19, 2026:

    Nit: Why not uint64_t? I would not expect this count to vary based on the underlying architecture.

    l0rinc commented at 8:29 PM on February 19, 2026:

    We're not using these very consistently, I also don't like it but had to stay consistent with the surrounding and original PR.

  80. sedited commented at 8:06 PM on February 19, 2026: contributor

    I feel like the changes to the disk space calculation are not elaborated on enough in the PR description. The title also seems to imply that this does not change behaviour outside of logs, but I think the disk space check is a more involved change. Can you try and consolidate the text into a single description. Now that it has received review, it does not seem too useful to compare this to prior work.

    Feel free to ignore the two type nits.

  81. l0rinc renamed this:
    coins: use number of dirty cache entries in flush warnings/logs
    coins: use dirty entry count for flush warnings and disk space checks
    on Feb 19, 2026
  82. l0rinc commented at 8:33 PM on February 19, 2026: contributor

    Edited the title and PR description, let me know if this seems more accurate and helpful.

  83. sedited approved
  84. sedited commented at 9:04 PM on February 19, 2026: contributor

    ACK afb1bc120ecce2bf663093e15c93f5592c0d4a98

    I feel like this change is a bit noisy, and it is not trivial to check that dirty_count is mutated correctly. Don't have a good suggestion for encapsulating it either though.

  85. sipa commented at 9:06 PM on February 19, 2026: member

    FWIW, I tried introducing a few errors in the m_dirty_count accounting (moving things in and out of if branches, for example), and saw that the fuzz tests (instantly) fail.

  86. sedited merged this on Feb 19, 2026
  87. sedited closed this on Feb 19, 2026
Contributors
l0rincDrahtBotfanquakeandrewtothsipaoptout21Eunovosedited
Labels
UTXO Db and Indexes

Milestone
31.0

Linked (view graph)
#28233 validation: don't clear cache on periodic flush: >2x block connection speed#28280 Don't empty dbcache on prune flushes: >30% faster IBD#31703 Use number of dirty cache entries in flush warnings/logs#34280 Coins Cache Cleanup checklist
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-02 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me