coins: use number of dirty cache entries in flush warnings/logs #33512

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

    Revival of #31703 with all outstanding review feedback addressed.

    It’s also related to #32313 which fixes a few more accounting inconsistencies in this area


    Since #28280 and #28233, the percentage of dirty entries in the cache when a flush happens may in practice be far from 100%. The log output, the decision to warn about a large flush (#31534), and the disk-full detection, however always use the entire size of the cache.

    Fix this by keeping track of the number of dirty entries in CCoinsViewCache, and using that number. We’ve dropped the usage of DynamicMemoryUsage as it’s the wrong metric, even before the non-wiping flushes were introduced (if everything is dirty, memory usage is correlated with, but not the same as, the amount of disk space written or used). They could be improved by keeping track of proper write size estimates in a follow-up, but here we’re just using a fixed 48 bytes per entry estimate.

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

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

    Code Coverage & Benchmarks

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

    Reviews

    See the guideline for information on the review process. A summary of reviews will appear here.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #33042 (refactor: inline constant return values from dbwrapper write methods by l0rinc)
    • #33018 (coins: remove SetFresh method from CCoinsCacheEntry by andrewtoth)
    • #32313 (coins: fix cachedCoinsUsage accounting in CCoinsViewCache by l0rinc)
    • #31132 (validation: fetch block inputs on parallel threads >10% faster IBD by andrewtoth)

    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.

  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:

     0Run 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).
     1INFO: Seed: 1436700023
     2INFO: Loaded 1 modules   (611994 inline 8-bit counters): 611994 [0x6082fc8a27f8, 0x6082fc937e92), 
     3INFO: Loaded 1 PC tables (611994 PCs): 611994 [0x6082fc937e98,0x6082fd28e838), 
     4INFO:     2396 files found in /home/admin/actions-runner/_work/_temp/ci/scratch/qa-assets/fuzz_corpora/coins_view_db
     5INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 275937 bytes
     6INFO: seed corpus: files: 2396 min: 2b max: 275937b total: 27207582b rss: 109Mb
     7/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')
     8    [#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
     9    [#1](/bitcoin-bitcoin/1/) 0x6082fb3e30bb in CCoinsViewDB::BatchWrite(CoinsViewCacheCursor&, uint256 const&) /home/admin/actions-runner/_work/_temp/build/src/./txdb.cpp:137:21
    10    [#2](/bitcoin-bitcoin/2/) 0x6082f9ec4573 in CCoinsViewCache::Flush() /home/admin/actions-runner/_work/_temp/build/src/./coins.cpp:265:22
    11    [#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
    12    [#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
    13    [#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
    14    [#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
    15    [#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
    16    [#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
    17    [#9](/bitcoin-bitcoin/9/) 0x6082f9e6efae in LLVMFuzzerTestOneInput /home/admin/actions-runner/_work/_temp/build/src/test/fuzz/util/./test/fuzz/fuzz.cpp:216:5
    18    [#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)
    19    [#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)
    20    [#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)
    21    [#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)
    22    [#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)
    23    [#15](/bitcoin-bitcoin/15/) 0x6082f9497946 in main (/home/admin/actions-runner/_work/_temp/build/bin/fuzz+0x1a9d946) (BuildId: c46e5f78c23c1d0b2ce0fb85b0817bc7fae97dd8)
    24    [#16](/bitcoin-bitcoin/16/) 0x756d565fa1c9  (/lib/x86_64-linux-gnu/libc.so.6+0x2a1c9) (BuildId: 274eec488d230825a136fa9c4d85370fed7a0a5e)
    25    [#17](/bitcoin-bitcoin/17/) 0x756d565fa28a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2a28a) (BuildId: 274eec488d230825a136fa9c4d85370fed7a0a5e)
    26    [#18](/bitcoin-bitcoin/18/) 0x6082f945fb54 in _start (/home/admin/actions-runner/_work/_temp/build/bin/fuzz+0x1a65b54) (BuildId: c46e5f78c23c1d0b2ce0fb85b0817bc7fae97dd8)
    27
    28SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow /home/admin/actions-runner/_work/_temp/src/coins.h:288:23 
    29MS: 0 ; base unit: 0000000000000000000000000000000000000000
    300x1,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,
    31\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!
    32artifact_prefix='./'; Test unit written to ./crash-ebc99ca6d40a3c7a37202cdba9a8b36a2cb6007b
    33Base64: Af////L8/y8fqV0wMJAAAAAUaQAAAAAAAMHBwcHBwbfBwVyRO3IA//oh
    34
    35INFO: Running with entropic power schedule (0xFF, 100).
    36INFO: Seed: 1436700023
    37INFO: Loaded 1 modules   (611994 inline 8-bit counters): 611994 [0x6082fc8a27f8, 0x6082fc937e92), 
    38INFO: Loaded 1 PC tables (611994 PCs): 611994 [0x6082fc937e98,0x6082fd28e838), 
    39INFO:     2396 files found in /home/admin/actions-runner/_work/_temp/ci/scratch/qa-assets/fuzz_corpora/coins_view_db
    40INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 275937 bytes
    41INFO: seed corpus: files: 2396 min: 2b max: 275937b total: 27207582b rss: 109Mb
    42/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')
    43    [#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
    44    [#1](/bitcoin-bitcoin/1/) 0x6082fb3e30bb in CCoinsViewDB::BatchWrite(CoinsViewCacheCursor&, uint256 const&) /home/admin/actions-runner/_work/_temp/build/src/./txdb.cpp:137:21
    45    [#2](/bitcoin-bitcoin/2/) 0x6082f9ec4573 in CCoinsViewCache::Flush() /home/admin/actions-runner/_work/_temp/build/src/./coins.cpp:265:22
    46    [#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
    47    [#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
    48    [#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
    49    [#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
    50    [#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
    51    [#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
    52    [#9](/bitcoin-bitcoin/9/) 0x6082f9e6efae in LLVMFuzzerTestOneInput /home/admin/actions-runner/_work/_temp/build/src/test/fuzz/util/./test/fuzz/fuzz.cpp:216:5
    53    [#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)
    54    [#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)
    55    [#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)
    56    [#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)
    57    [#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)
    58    [#15](/bitcoin-bitcoin/15/) 0x6082f9497946 in main (/home/admin/actions-runner/_work/_temp/build/bin/fuzz+0x1a9d946) (BuildId: c46e5f78c23c1d0b2ce0fb85b0817bc7fae97dd8)
    59    [#16](/bitcoin-bitcoin/16/) 0x756d565fa1c9  (/lib/x86_64-linux-gnu/libc.so.6+0x2a1c9) (BuildId: 274eec488d230825a136fa9c4d85370fed7a0a5e)
    60    [#17](/bitcoin-bitcoin/17/) 0x756d565fa28a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2a28a) (BuildId: 274eec488d230825a136fa9c4d85370fed7a0a5e)
    61    [#18](/bitcoin-bitcoin/18/) 0x6082f945fb54 in _start (/home/admin/actions-runner/_work/_temp/build/bin/fuzz+0x1a65b54) (BuildId: c46e5f78c23c1d0b2ce0fb85b0817bc7fae97dd8)
    62
    63SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow /home/admin/actions-runner/_work/_temp/src/coins.h:288:23 
    64MS: 0 ; base unit: 0000000000000000000000000000000000000000
    650x1,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,
    66\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!
    67artifact_prefix='./'; Test unit written to ./crash-ebc99ca6d40a3c7a37202cdba9a8b36a2cb6007b
    68Base64: Af////L8/y8fqV0wMJAAAAAUaQAAAAAAAMHBwcHBwbfBwVyRO3IA//oh
    69
    70⚠️ 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:199 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:
    0    /**
    1     * Emplace a coin into cacheCoins without performing any checks, marking
    2     * the emplaced coin as dirty.
    3     *
    4     * NOT FOR GENERAL USE. Used only when loading coins from a UTXO snapshot.
    5     * [@sa](/bitcoin-bitcoin/contributor/sa/) ChainstateManager::PopulateAndValidateSnapshot()
    6     */
    7    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)
  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:281 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
  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?
  18. l0rinc force-pushed on Oct 7, 2025
  19. l0rinc force-pushed on Oct 8, 2025
  20. coins: Only update `cachedCoinsUsage` when entry is inserted in `EmplaceCoinInternalDANGER`
    The `EmplaceCoinInternalDANGER` method was unconditionally adding to `cachedCoinsUsage`, but should only do so when `try_emplace` actually inserts a new entry. If the entry already exists, no memory is allocated and usage should not change.
    
    Adds test coverage by randomly calling `EmplaceCoinInternalDANGER` in `SimulationTest` to verify it remains correct.
    
    Co-authored-by: Pieter Wuille <pieter@wuille.net>
    Co-authored-by: Andrew Toth <andrewstoth@gmail.com>
    f377bd7468
  21. coins: check unspent‑overwrite before `cachedCoinsUsage` change in `AddCoin`
    The exception could be triggered during fuzz testing which leaves the accounting in a bad state.
    The related fuzz test cannot be adjusted yet since other similar accounting adjustments have to be made for that to be possible.
    5820b01b49
  22. 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
    * Validated in `SanityCheck()` by recomputing from scratch
    
    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>
    649cd7a139
  23. 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>
    0fe4ba2a85
  24. l0rinc force-pushed on Oct 8, 2025

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: 2025-10-10 15:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me