coins: fix `cachedCoinsUsage` accounting in `CCoinsViewCache` #32313

pull l0rinc wants to merge 4 commits into bitcoin:master from l0rinc:l0rinc/reenable-coins-sanitizers changing 5 files +66 −41
  1. l0rinc commented at 5:33 PM on April 20, 2025: contributor

    Summary

    This PR fixes cachedCoinsUsage accounting bugs in CCoinsViewCache that caused UBSan unsigned-integer-overflow violations during testing. The issues stemmed from incorrect decrement timing in AddCoin(), unconditional reset in Flush() on failure, and incorrect increment in EmplaceCoinInternalDANGER() when insertion fails.

    Problems Fixed

    1. AddCoin() underflow on exception

    • Previously decremented cachedCoinsUsage before the possible_overwrite validation
    • If validation threw, the map entry remained unchanged but counter was decremented
    • This corrupted accounting and later caused underflow
    • Impact: Test-only in current codebase, but unsound accounting that could affect future changes

    2. Flush() accounting drift on failure

    • Unconditionally reset cachedCoinsUsage to 0, even when BatchWrite() failed
    • Left the map populated while the counter read zero
    • Impact: Test-only (production BatchWrite() returns true), but broke accounting consistency

    3. Cursor redundant usage tracking

    • CoinsViewCacheCursor::NextAndMaybeErase() subtracted usage when erasing spent entries
    • However, SpendCoin() already decremented and cleared the scriptPubKey, leaving DynamicMemoryUsage() at 0
    • Impact: Redundant code that obscured actual accounting behavior

    4. EmplaceCoinInternalDANGER() double-counting

    • Incremented cachedCoinsUsage even when try_emplace did not insert (duplicate key)
    • Inflated the counter on duplicate attempts
    • Impact: Mostly test-reachable (AssumeUTXO doesn't overwrite in production), but incorrect accounting

    Testing

    To reproduce the historical UBSan failures on the referenced baseline and to verify the fix, run:

    MAKEJOBS="-j$(nproc)" FILE_ENV="./ci/test/00_setup_env_native_fuzz.sh" ./ci/test_run_all.sh

    The change was tested with the related unit and fuzz test, and asserted before/after each cachedCoinsUsage change (in production code and fuzz) that the calculations are still correct by recalculating them from scratch.

    <details> <summary>Details</summary>

    bool CCoinsViewCache::CacheUsageValid() const
{
    size_t actual{0};
    for (auto& entry : cacheCoins | std::views::values) actual += entry.coin.DynamicMemoryUsage();
    return actual == cachedCoinsUsage;
}

    or

    diff --git a/src/coins.cpp b/src/coins.cpp
--- a/src/coins.cpp	(revision fd3b1a7f4bb2ac527f23d4eb4cfa40a3215906e5)
+++ b/src/coins.cpp	(revision 872a05633bfdbd06ad82190d7fe34b42d13ebfe9)
@@ -96,6 +96,7 @@
         fresh = !it->second.IsDirty();
     }
     if (!inserted) {
+        Assert(cachedCoinsUsage >= it->second.coin.DynamicMemoryUsage());
         cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage();
     }
     it->second.coin = std::move(coin);
@@ -133,6 +134,7 @@
 bool CCoinsViewCache::SpendCoin(const COutPoint &outpoint, Coin* moveout) {
     CCoinsMap::iterator it = FetchCoin(outpoint);
     if (it == cacheCoins.end()) return false;
+    Assert(cachedCoinsUsage >= it->second.coin.DynamicMemoryUsage());
     cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage();
     TRACEPOINT(utxocache, spent,
            outpoint.hash.data(),
@@ -226,10 +228,12 @@
             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.
+                Assert(cachedCoinsUsage >= itUs->second.coin.DynamicMemoryUsage());
                 cachedCoinsUsage -= itUs->second.coin.DynamicMemoryUsage();
                 cacheCoins.erase(itUs);
             } else {
                 // A normal modification.
+                Assert(cachedCoinsUsage >= itUs->second.coin.DynamicMemoryUsage());
                 cachedCoinsUsage -= itUs->second.coin.DynamicMemoryUsage();
                 if (cursor.WillErase(*it)) {
                     // Since this entry will be erased,
@@ -279,6 +283,7 @@
 {
     CCoinsMap::iterator it = cacheCoins.find(hash);
     if (it != cacheCoins.end() && !it->second.IsDirty() && !it->second.IsFresh()) {
+        Assert(cachedCoinsUsage >= it->second.coin.DynamicMemoryUsage());
         cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage();
         TRACEPOINT(utxocache, uncache,
                hash.hash.data(),

    </details>

  2. DrahtBot commented at 5:33 PM on April 20, 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/32313.

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK andrewtoth, optout21, sipa, w0xlt
    Stale ACK ryanofsky

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #33602 ([IBD] coins: reduce lookups in dbcache layer propagation by l0rinc)
    • #33512 (coins: use number of dirty cache entries in flush warnings/logs by l0rinc)
    • #33018 (coins: remove SetFresh method from CCoinsCacheEntry by andrewtoth)
    • #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.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

  3. in src/coins.cpp:82 in 4bd4805217 outdated 
      75 | @@ -76,13 +76,13 @@ 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 (!possible_overwrite && !it->second.coin.IsSpent()) { // check before any other partial state change
  80 | +        throw std::logic_error("Attempted to overwrite an unspent coin (when possible_overwrite is false)");
  81 | +    }
  82 |      if (!inserted) {
    andrewtoth commented at 5:36 PM on April 20, 2025:

    Would it be simpler to just move this if block below the if block that throws, instead of pulling the throw out?

    l0rinc commented at 8:57 PM on April 20, 2025:

    If you think that's better, I don't mind - done

  4. in src/coins.cpp:115 in 4bd4805217 outdated 
     114 | -    cachedCoinsUsage += coin.DynamicMemoryUsage();
 115 | -    auto [it, inserted] = cacheCoins.try_emplace(std::move(outpoint), std::move(coin));
 116 | -    if (inserted) CCoinsCacheEntry::SetDirty(*it, m_sentinel);
 117 | +void CCoinsViewCache::EmplaceCoinInternalDANGER(COutPoint&& outpoint, Coin&& coin)
 118 | +{
 119 | +    auto [it, inserted] = cacheCoins.try_emplace(std::move(outpoint));
    andrewtoth commented at 5:41 PM on April 20, 2025:

    I think this would be simpler if we kept the original behavior of moving the coin here as well, and then only increment the cachedCoinsUsage if the coin is inserted.

    In practice it's not possible to get !inserted unless the assume utxo payload was generated incorrectly.

    l0rinc commented at 8:57 PM on April 20, 2025:

    In practice it's not possible to get !inserted unless the assume utxo payload was generated incorrectly.

    As far as I can see that's not a guarantee, but I've also noted it in the commit message that currently this is the case.

    kept the original behavior of moving the coin here as well

    Sure, if you think that's better - changed!

  5. in src/coins.cpp:204 in 4bd4805217 outdated 
     200 | @@ -195,6 +201,7 @@ bool CCoinsViewCache::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &ha
 201 |                  // and mark it as dirty.
 202 |                  itUs = cacheCoins.try_emplace(it->first).first;
 203 |                  CCoinsCacheEntry& entry{itUs->second};
 204 | +                cachedCoinsUsage -= entry.coin.DynamicMemoryUsage();
    andrewtoth commented at 5:43 PM on April 20, 2025:

    The entry has an empty coin here, it was just created. So this is just subtracting zero.

    l0rinc commented at 8:57 PM on April 20, 2025:

    Thanks, I'll add an assert in that case to explain why this branch wasn't symmetric with the other one.

    andrewtoth commented at 11:23 PM on May 5, 2025:

    I don't think we should add an assert here to explain. The comment right above says Create the coin in the parent cache.

    l0rinc commented at 7:14 AM on May 6, 2025:

    Doesn't the presence of the original cache-usage-update indicate that the comment isn't enough?

    andrewtoth commented at 2:05 PM on May 6, 2025:

    Sorry, what is the original cache-usage-update? AFAICT this line was added in this PR, and there is no cache-usage-update here?

    l0rinc commented at 2:30 PM on May 6, 2025:

    I prefer code over comments (the comment can be wrong, the code validates) - and since the usual pattern is basically:

    cachedCoinsUsage -= itUs->second.coin.DynamicMemoryUsage();
if (cursor.WillErase(*it)) {
    itUs->second.coin = std::move(it->second.coin);
} else {
    itUs->second.coin = it->second.coin;
}
cachedCoinsUsage += itUs->second.coin.DynamicMemoryUsage();

    it makes sense to explain (via code) why here we don't need the first part:

    assert(entry.coin.DynamicMemoryUsage() == 0); // i.e. we didn't forget the symmetry, it's on purpose
if (cursor.WillErase(*it)) {
    entry.coin = std::move(it->second.coin);
} else {
    entry.coin = it->second.coin;
}
cachedCoinsUsage += entry.coin.DynamicMemoryUsage();

    Do you still think we don't need it?

  6. l0rinc force-pushed on Apr 20, 2025
  7. l0rinc renamed this:
    RFC: Fix `cachedCoinsUsage` usages in `CCoinsViewCache`
    coins: Fix `cachedCoinsUsage` usages in `CCoinsViewCache`
    on Apr 20, 2025
  8. DrahtBot added the label UTXO Db and Indexes on Apr 20, 2025
  9. l0rinc marked this as ready for review on Apr 20, 2025
  10. DrahtBot added the label CI failed on Apr 28, 2025
  11. l0rinc commented at 9:50 AM on April 29, 2025: contributor

    Rebased to allow CI running the new tests.

  12. l0rinc force-pushed on Apr 29, 2025
  13. l0rinc renamed this:
    coins: Fix `cachedCoinsUsage` usages in `CCoinsViewCache`
    coins: fix `cachedCoinsUsage` usages in `CCoinsViewCache`
    on Apr 29, 2025
  14. l0rinc renamed this:
    coins: fix `cachedCoinsUsage` usages in `CCoinsViewCache`
    coins: fix `cachedCoinsUsage` accounting in `CCoinsViewCache`
    on Apr 29, 2025
  15. DrahtBot removed the label CI failed on Apr 29, 2025
  16. in test/sanitizer_suppressions/ubsan:54 in a1fe87eb66 outdated 
      45 | @@ -46,9 +46,6 @@ unsigned-integer-overflow:CRollingBloomFilter::insert
  46 |  unsigned-integer-overflow:RollingBloomHash
  47 |  unsigned-integer-overflow:CCoinsViewCache::AddCoin
  48 |  unsigned-integer-overflow:CCoinsViewCache::BatchWrite
  49 | -unsigned-integer-overflow:CCoinsViewCache::DynamicMemoryUsage
  50 | -unsigned-integer-overflow:CCoinsViewCache::SpendCoin
  51 | -unsigned-integer-overflow:CCoinsViewCache::Uncache
    maflcko commented at 8:46 PM on April 29, 2025:

    it would be better to file this separately and explain why it isn't needed (or rather since when)

    l0rinc commented at 7:54 AM on April 30, 2025:

    They were introduced in https://github.com/bitcoin/bitcoin/pull/28865/files#diff-bca63b54ad7fcc2df12f3237fd8775810c1b849c49110f702920655778fcc248R45-R49, I don't see any concrete explanation for what the problem was - and couldn't reproduce any fuzzing problem when I've removed these. @fanquake, can you help us out here, do you remember why these were added?

    l0rinc commented at 12:51 PM on April 30, 2025:

    I ran the following on Linux for a1fe87eb668e8a1eeeef335951547882b5fb52d8:

    cmake --preset=libfuzzer && cmake --build build_fuzz && \
  FUZZ=coin_grinder build_fuzz/bin/fuzz -runs=500000 && \
  FUZZ=coin_grinder_is_optimal build_fuzz/bin/fuzz -runs=500000 && \
  FUZZ=coincontrol build_fuzz/bin/fuzz -runs=500000 && \
  FUZZ=coins_deserialize build_fuzz/bin/fuzz -runs=500000 && \
  FUZZ=coins_view build_fuzz/bin/fuzz -runs=500000 && \
  FUZZ=coinscache_sim build_fuzz/bin/fuzz -runs=500000 && \
  FUZZ=coinselection_bnb build_fuzz/bin/fuzz -runs=500000 && \
  FUZZ=coinselection_knapsack build_fuzz/bin/fuzz -runs=500000 && \
  FUZZ=coinselection_srd build_fuzz/bin/fuzz -runs=500000

    They all passed (actually ran a few until -runs=1000000 but got bored at the end). Am I missing anything important here?

    maflcko commented at 1:31 PM on April 30, 2025:

    Am I missing anything important here?

    Yes, the preset doesn't check for those. You'll have to run with the integer sanitizer to be able to detect integer sanitizer issues.

    The testing steps are also explained in the comment: #28865#issue-1991140953

    l0rinc commented at 9:15 PM on April 30, 2025:

    As mentioned in #32296 (comment), I can't always reproduce these locally, it's why I used the CI to verify why these were needed in the first place. I have tried every combination I could think of locally, I couldn't reproduce any such errors in coins. If you can, please let me know how to do that exactly. Is there any other CI run I should be looking at to see if there are other failures?

    maflcko commented at 5:36 PM on May 4, 2025:

    The CI should be deterministic and trivially reproducible, as explained in https://github.com/bitcoin/bitcoin/blob/master/ci/README.md. If you can't reproduce it, please share the exact steps you tried, starting from a fresh system. For example, you can start with a fresh Hetzner Cloud VM (or any other fresh VM of your choice).

    The integer sanitizer should equally be deterministic and reproducible.

    l0rinc commented at 5:54 PM on May 4, 2025:

    I meant I couldn't reproduce any of the supposed failures that suppressions were meant to avoid - but neither is the CI, so I'm not sure what to explain about the removals: they don't seem to be needed for CI to pass. Unless there's a secret CI which shows these failures - or any other way that you can reproduce them, please let me know how to do it.

    maflcko commented at 6:17 PM on May 4, 2025:

    In #32313 (review) I was saying it would be good to known since when they are unused. To test an earlier commit, you can just switch to it in git.

    l0rinc commented at 8:20 PM on May 4, 2025:

    Whenever I tried going back to pre-cmake times, I couldn't get it to compile and run again (on Mac or on Linux). Not the first time I try to do a git bisect, but it always turns bad really quickly.

    I tried pushing a PR to my fork with the original PR that introduced the suppressions + removing DynamicMemoryUsage, SpendCoin and Uncache suppressionss from ubsan. The build is failing with similar problems that I see locally but don't see any unsigned-integer-overflow complaints. #28865 https://github.com/l0rinc/bitcoin/pull/10

    Can you give me a hint of what else to try?

    fanquake commented at 1:41 PM on May 9, 2025:

    They were introduced in https://github.com/bitcoin/bitcoin/pull/28865/files#diff-bca63b54ad7fcc2df12f3237fd8775810c1b849c49110f702920655778fcc248R45-R49, @fanquake, can you help us out here, do you remember why these were added?

    You can recreate the failures by checking out to that commit (fd30e9688e15fe6e0f8b64202a6e9c7d96333394), applying this diff (which includes the supression being done here):

    diff --git a/ci/test/00_setup_env_native_fuzz.sh b/ci/test/00_setup_env_native_fuzz.sh
index 3585b2b417..ce45917e99 100755
--- a/ci/test/00_setup_env_native_fuzz.sh
+++ b/ci/test/00_setup_env_native_fuzz.sh
@@ -6,7 +6,7 @@
 
 export LC_ALL=C.UTF-8
 
-export CI_IMAGE_NAME_TAG="docker.io/ubuntu:23.10"  # This version will reach EOL in Jul 2024, and can be replaced by "ubuntu:24.04" (or anything else that ships the wanted clang version).
+export CI_IMAGE_NAME_TAG="docker.io/ubuntu:24.04"  # This version will reach EOL in Jul 2024, and can be replaced by "ubuntu:24.04" (or anything else that ships the wanted clang version).
 export CONTAINER_NAME=ci_native_fuzz
 export PACKAGES="clang-17 llvm-17 libclang-rt-17-dev libevent-dev libboost-dev libsqlite3-dev"
 export NO_DEPENDS=1
diff --git a/test/sanitizer_suppressions/ubsan b/test/sanitizer_suppressions/ubsan
index 63d2215aab..d3fc2a59ff 100644
--- a/test/sanitizer_suppressions/ubsan
+++ b/test/sanitizer_suppressions/ubsan
@@ -42,11 +42,6 @@ unsigned-integer-overflow:arith_uint256.h
 unsigned-integer-overflow:CBloomFilter::Hash
 unsigned-integer-overflow:CRollingBloomFilter::insert
 unsigned-integer-overflow:RollingBloomHash
-unsigned-integer-overflow:CCoinsViewCache::AddCoin
-unsigned-integer-overflow:CCoinsViewCache::BatchWrite
-unsigned-integer-overflow:CCoinsViewCache::DynamicMemoryUsage
-unsigned-integer-overflow:CCoinsViewCache::SpendCoin
-unsigned-integer-overflow:CCoinsViewCache::Uncache
 unsigned-integer-overflow:CompressAmount
 unsigned-integer-overflow:DecompressAmount
 unsigned-integer-overflow:crypto/

    and running the native_fuzz CI job (on aarch64):

    time env -i HOME="$HOME" PATH="$PATH" USER="$USER" bash -c 'MAKEJOBS="-j17" FILE_ENV="./ci/test/00_setup_env_native_fuzz.sh" ./ci/test_run_all.sh'
<snip>
[#170921](/bitcoin-bitcoin/170921/)	REDUCE cov: 1694 ft: 6197 corp: 478/22Kb lim: 240 exec/s: 3351 rss: 208Mb L: 20/219 MS: 1 EraseBytes-
[#171261](/bitcoin-bitcoin/171261/)	REDUCE cov: 1694 ft: 6200 corp: 479/22Kb lim: 240 exec/s: 3358 rss: 208Mb L: 193/219 MS: 5 ChangeBinInt-ChangeBit-ChangeByte-CrossOver-CrossOver-
[#171550](/bitcoin-bitcoin/171550/)	REDUCE cov: 1694 ft: 6200 corp: 479/22Kb lim: 240 exec/s: 3363 rss: 208Mb L: 42/219 MS: 4 CopyPart-ChangeByte-PersAutoDict-EraseBytes- DE: "\024\000"-
[#171657](/bitcoin-bitcoin/171657/)	REDUCE cov: 1694 ft: 6200 corp: 479/22Kb lim: 240 exec/s: 3365 rss: 208Mb L: 51/219 MS: 2 InsertByte-EraseBytes-
coins.cpp:132:22: runtime error: unsigned integer overflow: 0 - 64 cannot be represented in type 'size_t' (aka 'unsigned long')
    [#0](/bitcoin-bitcoin/0/) 0xaaaab73bc134 in CCoinsViewCache::SpendCoin(COutPoint const&, Coin*) src/coins.cpp:132:22
    [#1](/bitcoin-bitcoin/1/) 0xaaaab57cbc94 in coins_view_fuzz_target(Span<unsigned char const>)::$_4::operator()() const src/test/fuzz/coins_view.cpp:89:40
    [#2](/bitcoin-bitcoin/2/) 0xaaaab57cbc94 in unsigned long CallOneOf<coins_view_fuzz_target(Span<unsigned char const>)::$_0, coins_view_fuzz_target(Span<unsigned char const>)::$_1, coins_view_fuzz_target(Span<unsigned char const>)::$_2, coins_view_fuzz_target(Span<unsigned char const>)::$_3, coins_view_fuzz_target(Span<unsigned char const>)::$_4, coins_view_fuzz_target(Span<unsigned char const>)::$_5, coins_view_fuzz_target(Span<unsigned char const>)::$_6, coins_view_fuzz_target(Span<unsigned char const>)::$_7, coins_view_fuzz_target(Span<unsigned char const>)::$_8, coins_view_fuzz_target(Span<unsigned char const>)::$_9, coins_view_fuzz_target(Span<unsigned char const>)::$_10>(FuzzedDataProvider&, coins_view_fuzz_target(Span<unsigned char const>)::$_0, coins_view_fuzz_target(Span<unsigned char const>)::$_1, coins_view_fuzz_target(Span<unsigned char const>)::$_2, coins_view_fuzz_target(Span<unsigned char const>)::$_3, coins_view_fuzz_target(Span<unsigned char const>)::$_4, coins_view_fuzz_target(Span<unsigned char const>)::$_5, coins_view_fuzz_target(Span<unsigned char const>)::$_6, coins_view_fuzz_target(Span<unsigned char const>)::$_7, coins_view_fuzz_target(Span<unsigned char const>)::$_8, coins_view_fuzz_target(Span<unsigned char const>)::$_9, coins_view_fuzz_target(Span<unsigned char const>)::$_10) src/./test/fuzz/util.h:43:27
    [#3](/bitcoin-bitcoin/3/) 0xaaaab57cbc94 in coins_view_fuzz_target(Span<unsigned char const>) src/test/fuzz/coins_view.cpp:58:9
    [#4](/bitcoin-bitcoin/4/) 0xaaaab5d2319c in std::function<void (Span<unsigned char const>)>::operator()(Span<unsigned char const>) const /usr/bin/../lib/gcc/aarch64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:591:9
    [#5](/bitcoin-bitcoin/5/) 0xaaaab5d2319c in LLVMFuzzerTestOneInput src/test/fuzz/fuzz.cpp:178:5
    [#6](/bitcoin-bitcoin/6/) 0xaaaab5545050 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/ci_container_base/ci/scratch/build/bitcoin-aarch64-unknown-linux-gnu/src/test/fuzz/fuzz+0x1925050) (BuildId: 82e180570288de2f70ceeee1ed5d7cd3729696ab)
    [#7](/bitcoin-bitcoin/7/) 0xaaaab5544798 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) (/ci_container_base/ci/scratch/build/bitcoin-aarch64-unknown-linux-gnu/src/test/fuzz/fuzz+0x1924798) (BuildId: 82e180570288de2f70ceeee1ed5d7cd3729696ab)
    [#8](/bitcoin-bitcoin/8/) 0xaaaab5545f90 in fuzzer::Fuzzer::MutateAndTestOne() (/ci_container_base/ci/scratch/build/bitcoin-aarch64-unknown-linux-gnu/src/test/fuzz/fuzz+0x1925f90) (BuildId: 82e180570288de2f70ceeee1ed5d7cd3729696ab)
    [#9](/bitcoin-bitcoin/9/) 0xaaaab5546cdc in fuzzer::Fuzzer::Loop(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (/ci_container_base/ci/scratch/build/bitcoin-aarch64-unknown-linux-gnu/src/test/fuzz/fuzz+0x1926cdc) (BuildId: 82e180570288de2f70ceeee1ed5d7cd3729696ab)
    [#10](/bitcoin-bitcoin/10/) 0xaaaab5534e70 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/ci_container_base/ci/scratch/build/bitcoin-aarch64-unknown-linux-gnu/src/test/fuzz/fuzz+0x1914e70) (BuildId: 82e180570288de2f70ceeee1ed5d7cd3729696ab)
    [#11](/bitcoin-bitcoin/11/) 0xaaaab555e0ac in main (/ci_container_base/ci/scratch/build/bitcoin-aarch64-unknown-linux-gnu/src/test/fuzz/fuzz+0x193e0ac) (BuildId: 82e180570288de2f70ceeee1ed5d7cd3729696ab)
    [#12](/bitcoin-bitcoin/12/) 0xffffb4aa84c0  (/lib/aarch64-linux-gnu/libc.so.6+0x284c0) (BuildId: 1d7249a4f207d07166ff4be43acdc68a01faaa04)
    [#13](/bitcoin-bitcoin/13/) 0xffffb4aa8594 in __libc_start_main (/lib/aarch64-linux-gnu/libc.so.6+0x28594) (BuildId: 1d7249a4f207d07166ff4be43acdc68a01faaa04)
    [#14](/bitcoin-bitcoin/14/) 0xaaaab552afac in _start (/ci_container_base/ci/scratch/build/bitcoin-aarch64-unknown-linux-gnu/src/test/fuzz/fuzz+0x190afac) (BuildId: 82e180570288de2f70ceeee1ed5d7cd3729696ab)

SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow coins.cpp:132:22 in 
MS: 1 CopyPart-; base unit: 81111e927920a57969bb7efb23d89de00d09a5ec
0xff,0xef,0xfe,0x0,0x1,0x3,0x0,0x1,0xff,0xff,0xff,0xff,0xff,0x3,0x0,0x1,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0x3,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xfe,0xff,0xfa,0x0,0x0,0x0,0x0,0x80,0x76,0x0,0x0,0x0,0x0,0x68,0x5c,0xff,0xff,0xff,0xff,0xef,0xfe,0x0,0x1,0x3,0x0,0x1,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0x3,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0x3,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xfe,0xff,0xfa,0x0,0x0,0x0,0x0,0x80,0x76,0x0,0x0,0x0,0x0,0x68,0x5c,0xff,0xff,0xff,0xff,0xef,0xfe,0x0,0x1,0x3,0x0,0x1,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0x3,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xfe,0xff,0xfa,0x7,
\377\357\376\000\001\003\000\001\377\377\377\377\377\003\000\001\377\377\377\377\377\377\377\003\377\377\377\377\377\377\377\377\377\377\376\377\372\000\000\000\000\200v\000\000\000\000h\\\377\377\377\377\357\376\000\001\003\000\001\377\377\377\377\377\377\377\003\377\377\377\377\377\377\377\377\377\377\377\003\377\377\377\377\377\377\377\377\377\377\376\377\372\000\000\000\000\200v\000\000\000\000h\\\377\377\377\377\357\376\000\001\003\000\001\377\377\377\377\377\377\377\003\377\377\377\377\377\377\377\377\377\377\376\377\372\007
artifact_prefix='./'; Test unit written to ./crash-2ea01576a6e0a74d403920761ea2ff6f0bf41752
Base64: /+/+AAEDAAH//////wMAAf////////8D//////////////7/+gAAAACAdgAAAABoXP/////v/gABAwAB/////////wP//////////////wP//////////////v/6AAAAAIB2AAAAAGhc/////+/+AAEDAAH/////////A//////////////+//oH

Target ['/ci_container_base/ci/scratch/build/bitcoin-aarch64-unknown-linux-gnu/src/test/fuzz/fuzz', '-max_total_time=60'] failed with exit code 1
    l0rinc commented at 9:52 PM on May 10, 2025:

    You can recreate the failures

    Thanks a lot @fanquake, I managed to reproduce it this way (seeing that https://github.com/l0rinc/bitcoin/pull/10 was failing, I was trying to run them manually instead of relying on the ci scripts).

    why it isn't needed (or rather since when)

    Thanks @maflcko for insisting, I have changed the order of commits to place the suppression removal at the very end since we still had a few failures for the first few commits - it's easier to remove them all at the end only, where I couldn't reproduce any such failure anymore.

    maflcko commented at 2:43 PM on May 13, 2025:

    Seems fine, but it may be better to remove the suppression in the exact commit that removes the need for it. This would make it easier to revert the commit atomically in one go, if there is need for whatever reason.

    l0rinc commented at 3:09 PM on May 13, 2025:

    Any accounting inconsistency could've caused it to become negative and fail, so instead of removing these suppressions in the last commit that fixed the problem (giving the impression that it was just one change that enabled us removing it), I did it separately instead. Is this important, is this holding back the ACK from your part?

  17. l0rinc force-pushed on May 10, 2025
  18. l0rinc requested review from fanquake on May 21, 2025
  19. l0rinc requested review from maflcko on May 21, 2025
  20. l0rinc requested review from andrewtoth on May 21, 2025
  21. DrahtBot added the label Needs rebase on Jun 5, 2025
  22. l0rinc force-pushed on Jun 10, 2025
  23. l0rinc commented at 11:36 AM on June 10, 2025: contributor

    Rebased after fuzz conflicts, ready for review again.

  24. DrahtBot removed the label Needs rebase on Jun 10, 2025
  25. l0rinc force-pushed on Oct 1, 2025
  26. l0rinc commented at 8:18 PM on October 1, 2025: contributor

    Rebased and adjusted the commits to minimize conflict with https://github.com/bitcoin/bitcoin/pull/33512

  27. l0rinc force-pushed on Oct 8, 2025
  28. l0rinc commented at 3:03 AM on October 8, 2025: contributor

    Added an AssumeUTXO fuzz test here that was requested in #33512 (review) - but it needs the fixes here to work.

  29. ryanofsky commented at 6:13 PM on October 8, 2025: contributor

    Just a quick code review for 2b11c47f60b85fe72ab9d093c9f8e5f597182fbc. Sorry if I missed anything obvious, my comments below are mostly asking for things to be explained a little better. Overall the changes seem positive and I did not see any problems.

    • Can PR description be updated to say what types of "violations" are being "fixed" and clarify what purpose of the changes are? Would be helpful if description was self contained and did not start out referencing a comment in another PR.

    • Purpose of first commit 6b842138c33d8663603d1d0a528cb90c54c70163 seems opaque. Commit description says a check is being moved but not why, and it is not clear what effect this change has or if it can be tested.

    • Third commit 62c600f79a13e8772f3d6d7aaf6593c6dc55ddfe seems to be a refactoring that does not change behavior. Would be helpful to say this explicitly and maybe start commit title with "refactor".

    • From what I can tell the second and third commits both seem to be refactorings that do not depend on each other and aren't really related to the rest of the PR. If would be nice if commit messages clarified if they are independent cleanups or if something depends on them. It might also be good to move these refactorings before the bugfix in the first commit if they do not depend on it.

    • Fourth commit f1791d64b437eab31ad941fb4c9e4051e0274c4a seems like a good bugfix. But would be good if commit description could say what impact of bug is. Is it a rare thing that happens only when disk writes fail? What's the actual effect if the bug does happen? Too much memory is allocated? Something else?

    • Last commit a9eb8dfab1c35c2563d3267432b64bb6a812f08f seems to be doing a few unrelated things and would be nice to split up. The main thing it seems to be doing is dropping the "coins_view_cache.Flush()" workaround because of an accounting bug "fixed in a previous commit". It would be much nicer IMO if the workaround could be dropped in the same commit which actually fixes the accounting bug. Moving this change should make the bugfix and test change both easier to understand.

    • Additionally, the last commit seems to be dropping the expected_code_path check which makes sure that after many writes, at least one coin will be overwritten. Unclear why that check is being dropped and would be good to explain in commit message. Adding the EmplaceCoinInternalDANGER call there does seem good and useful to check the accounting.

  30. l0rinc force-pushed on Oct 8, 2025
  31. l0rinc commented at 10:03 PM on October 8, 2025: contributor

    Thank you for the quick review, I have restructured the change to tell a more intuitive story and added more commit message guidance for the context and rewrote the intro to make more sense (it helped that I wrote this a longer time ago and I also didn't remember the context).

    The new structure starts with the simple refactorings and assertions and adding extra guards to document why and where the accounting is incorrect. Most of the accounting problems are test-only, so it's unlikely to fix anything in production - but it enabled us writing more correct tests for coins.

    Based on the feedback I merged the changes in AddCoin and Flush since this way I can also enable the suppressions in the same commit, proving that they fix the problem. I have also split out the Flush() workaround in coins_view which removed the workaround in https://github.com/bitcoin/bitcoin/pull/32602/files#r2113749842 (cc: @marcofleon, @darosior). The expected_code_path didn't check any overwrites, just that all codepaths result in success - which is simplified in the last commit.

    The latest push is zero-diff, only reorganized and explained better - thanks for the review.

  32. in src/test/fuzz/coins_view.cpp:62 in bf1f9907d4 outdated 
      66 | -                    coins_view_cache.AddCoin(random_out_point, std::move(coin), possible_overwrite);
  67 | -                    expected_code_path = true;
  68 | -                } catch (const std::logic_error& e) {
  69 | -                    if (e.what() == std::string{"Attempted to overwrite an unspent coin (when possible_overwrite is false)"}) {
  70 | -                        assert(!possible_overwrite);
  71 | +                COutPoint outpoint{random_out_point};
    optout21 commented at 9:33 AM on October 9, 2025:

    nit: This declaration of outpoint is enough to be placed in the else branch

    diff --git a/src/test/fuzz/coins_view.cpp b/src/test/fuzz/coins_view.cpp
index 3928120a3f..055c3c4b31 100644
--- a/src/test/fuzz/coins_view.cpp
+++ b/src/test/fuzz/coins_view.cpp
@@ -59,13 +59,12 @@ void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsView& backend
                 if (random_coin.IsSpent()) {
                     return;
                 }
-                COutPoint outpoint{random_out_point};
                 Coin coin{random_coin};
                 if (fuzzed_data_provider.ConsumeBool()) {
                     bool expected_code_path = false;
                     const bool possible_overwrite = fuzzed_data_provider.ConsumeBool();
                     try {
-                        coins_view_cache.AddCoin(outpoint, std::move(coin), possible_overwrite);
+                        coins_view_cache.AddCoin(random_out_point, std::move(coin), possible_overwrite);
                         expected_code_path = true;
                     } catch (const std::logic_error& e) {
                         if (e.what() == std::string{"Attempted to overwrite an unspent coin (when possible_overwrite is false)"}) {
@@ -81,6 +80,7 @@ void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsView& backend
                     }
                     assert(expected_code_path);
                 } else {
+                    COutPoint outpoint{random_out_point};
                     coins_view_cache.EmplaceCoinInternalDANGER(std::move(outpoint), std::move(coin));
                 }
             },
    l0rinc commented at 12:44 PM on October 9, 2025:

    I have extracted it on purpose to signal that both branches are doing something similar and have the same dependencies

  33. optout21 commented at 9:38 AM on October 9, 2025: none

    ACK c4293c7b21971e7ddd813ad3fbe574eb5605d60e

    With the recent commit restructuring the steps are clear. The code change is sound and looks well tested. I've left one non-blocking nit, no other comment.

  34. in src/coins.cpp:116 in c4293c7b21 outdated 
     110 | @@ -111,9 +111,12 @@ void CCoinsViewCache::AddCoin(const COutPoint &outpoint, Coin&& coin, bool possi
 111 |  }
 112 |  
 113 |  void CCoinsViewCache::EmplaceCoinInternalDANGER(COutPoint&& outpoint, Coin&& coin) {
 114 | -    cachedCoinsUsage += coin.DynamicMemoryUsage();
 115 | +    const auto mem_usage{coin.DynamicMemoryUsage()};
 116 |      auto [it, inserted] = cacheCoins.try_emplace(std::move(outpoint), std::move(coin));
 117 | -    if (inserted) CCoinsCacheEntry::SetDirty(*it, m_sentinel);
 118 | +    if (inserted) {
    w0xlt commented at 12:19 AM on October 10, 2025:

    If inserted=false, you've still moved from coin. Is this expected behavior ?

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

    Yes. coin is an rvalue, so it is already moved out of the caller. inserted should always be true anyways.

  35. in src/coins.cpp:201 in 8d5e323b5f outdated 
     194 | @@ -195,6 +195,7 @@ bool CCoinsViewCache::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &ha
 195 |                  // and mark it as dirty.
 196 |                  itUs = cacheCoins.try_emplace(it->first).first;
 197 |                  CCoinsCacheEntry& entry{itUs->second};
 198 | +                assert(entry.coin.DynamicMemoryUsage() == 0);
    ryanofsky commented at 7:31 PM on October 10, 2025:

    In commit "refactor: assert newly-created parent cache entry has zero memory usage" (8d5e323b5f475fe9a2bed1402a20e48a40a45137)

    Note: this assert will be true because itUs == cacheCoins.end() condition above means try_emplace will always insert and itUs->second will be a new empty coin

    l0rinc commented at 2:11 AM on October 12, 2025:

    I think that's what I also wrote in the commit message, but rephrased it slightly, let me know if it's better.

  36. ryanofsky approved
  37. ryanofsky commented at 8:09 PM on October 10, 2025: contributor

    Code review ACK c4293c7b21971e7ddd813ad3fbe574eb5605d60e. The changes are nice simplifications addition to fixing overflows, and they are all nicely explained. Few suggestions you could consider (but ignore them if you prefer current implementation):

    • I feel like the new asserts added in b905a592d429b4b4edd48c8b0fa29ea4fdf8d251 and removed later don't really add much value or explain much since they can basically only trigger when there are one or two coins in the cache. I'd consider dropping them from the PR, and just posting them a diff if they are helpful for testing, so the PR is simpler.

    • Seems like it would be nice to order 33a8150315d8880be75c4117aa9457b7c2eb5ce1 before bf1f9907d47fc93cc0ca67fe65a03466d93381c3 so 33a8150315d8880be75c4117aa9457b7c2eb5ce1 can drop the flush workaround from the fuzz test, and bf1f9907d47fc93cc0ca67fe65a03466d93381c3 can just add the EmplaceCoinInternalDANGER call to the fuzz test. This way the fuzz test changes would illustrate the code changes and be minimal.

    • In the last commit I'm still not sure why expected_code_path assert is being removed. You last comment suggests that expected_code_path is removed because it just checks "that all codepaths result in success." But I don't think this is true. It looks to me like the expected_code_path assert is confirming that the std::logic_error exception throws at least once and that test coverage is complete. It seems ok to drop this, but I don't think it's redundant or no longer necessary due to changes here.

  38. optout21 commented at 10:00 PM on October 10, 2025: none
    • It looks to me like the expected_code_path assert is confirming that the std::logic_error exception throws at least once and that test coverage is complete.

    I don't think it checks that exception is thrown at least once, but it checks that either there is no exception, or if there is, it is that specific exception. With this change there is an assert to make sure the exception is the one expected. So I think the test code change is correct, but I agree that the "that all codepaths result in success." description is not precise.

  39. refactor: assert newly-created parent cache entry has zero memory usage
    During `BatchWrite`, the parent entry is created under a guard that guarantees insertion, so the new `Coin` is default-constructed and empty.
Assert this invariant to document why there is no `cachedCoinsUsage` decrement before the assignment at this site.

Co-authored-by: Andrew Toth <andrewstoth@gmail.com>
    67cff8bec9
  40. refactor: remove redundant usage tracking from `CoinsViewCacheCursor`
    When a coin is spent via `SpendCoin()`, `cachedCoinsUsage` is already decremented and the coin's `scriptPubKey` is cleared, so `DynamicMemoryUsage()` is `0`.

`CoinsViewCacheCursor::NextAndMaybeErase()` was subtracting usage again when erasing spent entries.
Replace it with an assert that documents spent coins have zero dynamic memory usage by the time the cursor encounters them.

Remove the now-unnecessary `usage` reference from the cursor's constructor and member variables.
    39cf8bb3d0
  41. coins: fix `cachedCoinsUsage` accounting to prevent underflow
    Move the `cachedCoinsUsage` subtract in `AddCoin()` to after the `possible_overwrite` check.
Previously a throw before assignment decremented the counter without changing the entry, which corrupted accounting and later underflowed.

In `Flush()`, reset `cachedCoinsUsage` to `0` only when `BatchWrite()` succeeds and `cacheCoins` is actually cleared. In production `BatchWrite()` returns `true`, so this mostly affects tests. On failure, leave the counter unchanged to keep it in sync with the cache.

The existing `Flush()` workaround in fuzzing was also removed now that the source of the problem was fixed, so the fuzzer no longer needs `coins_view_cache.Flush()` to realign `cachedCoinsUsage` after an exception.
Replace the prior `expected_code_path` tracking with direct assertions. The role of the variable was to verify that code execution follows only expected paths, either successful addition, or if it's an exception, the message is verified and checked that overwrite was disallowed.

With these changes the counter stays consistent across success and exception paths, so we can finally remove the `UBSan` suppressions for `CCoinsViewCache` that were masking the issue.

Included a unit test as well, attempting to add a different coin to the same outpoint without allowing overwrites and make sure it throws.
We use `SelfTest()` to validates accounting, and check that the cache remains usable.

Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Co-authored-by: w0xlt <woltx@protonmail.com>
    d7c9d6c291
  42. l0rinc force-pushed on Oct 12, 2025
  43. l0rinc commented at 2:12 AM on October 12, 2025: contributor

    Thanks for the comments, the latest push makes it even simpler, removed the temporary asserts, added unit tests instead (thanks @w0xlt), changed the order of the AddCoin and EmplaceCoinInternalDANGER commits which did indeed allow me to fixup the latest commit with the fuzz simplifications into the AddCoin one.

    In the last commit I'm still not sure why expected_code_path assert is being removed

    The role of the variable was to verify that code execution follows only expected paths, either successful addition, or if it's an exception, the message is verified and checked that overwrite was disallowed in the first place.

  44. coins: only adjust `cachedCoinsUsage` on `EmplaceCoinInternalDANGER` insert
    `EmplaceCoinInternalDANGER()` incremented `cachedCoinsUsage` even when `try_emplace` did not insert (duplicate key), inflating the counter.
This is mostly reachable in tests today since `AssumeUTXO` does not overwrite.

Increment only on successful insert, and capture `coin.DynamicMemoryUsage()` before the move so accounting uses the correct value.

Fuzz: add an `EmplaceCoinInternalDANGER` path to exercise insert-only accounting.
Unit test: emplace two different coins at the same outpoint (with different `DynamicMemoryUsage()`), verify `SelfTest()` passes and `AccessCoin(outpoint)` returns the first coin.

Co-authored-by: Andrew Toth <andrewstoth@gmail.com>
Co-authored-by: w0xlt <woltx@protonmail.com>
    24d861da78
  45. in src/test/coins_tests.cpp:1116 in 0d5f7d24bd 
    1111 | +    const COutPoint outpoint{Txid::FromUint256(m_rng.rand256()), m_rng.rand32()};
1112 | +    const Coin coin{CTxOut{m_rng.randrange(10), CScript{} << m_rng.randbytes(CScriptBase::STATIC_SIZE + 1)}, 1, false};
1113 | +
1114 | +    cache.EmplaceCoinInternalDANGER(COutPoint{outpoint}, Coin{coin});
1115 | +    cache.SelfTest();
1116 | +    cache.EmplaceCoinInternalDANGER(COutPoint{outpoint}, Coin{coin});
    andrewtoth commented at 4:18 PM on October 12, 2025:

    We should try and insert a different coin at the same outpoint, like we do for the AddCoin test above. That way the below check cache.AccessCoin(outpoint) == coin makes sure the original coin emplaced was not overwritten by the second one.

    We should also make sure the two coins have different DynamicMemoryUsage so the accounting is correct for the coin that was actually inserted.

    l0rinc commented at 4:38 PM on October 12, 2025:

    Good idea, done

  46. l0rinc force-pushed on Oct 12, 2025
  47. andrewtoth commented at 7:29 PM on October 12, 2025: contributor

    ACK 24d861da7894add47747eff69dd3fc71fbcdd7d0

  48. DrahtBot requested review from optout21 on Oct 12, 2025
  49. DrahtBot requested review from ryanofsky on Oct 12, 2025
  50. optout21 commented at 9:42 AM on October 14, 2025: none

    reACK 24d861da7894add47747eff69dd3fc71fbcdd7d0

  51. sipa commented at 2:23 PM on October 14, 2025: member

    ACK 24d861da7894add47747eff69dd3fc71fbcdd7d0

  53. glozow merged this on Oct 15, 2025
  54. glozow closed this on Oct 15, 2025
  55. l0rinc deleted the branch on Oct 16, 2025
Contributors
l0rincDrahtBotandrewtothmaflckofanquakeryanofskyoptout21w0xltsipa


fanquakemaflckoryanofskyoptout21

Labels
UTXO Db and Indexes
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-04-29 03:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me