test: Fix TestFlushBehavior when coin has non-zero dynamic memory usage #33381

1. 
   PiRK commented at 7:45 pm on September 12, 2025: contributor

   If the random coin in this test has a non-null scriptpubkey, cache->SanityCheck(); fails an assertion because the BOOST_CHECK_THROW line leaves CCoinsViewCache::cachedCoinsUsage in a corrupted state: the erroring AddCoin call decrements the value to 0 even though the coin is still in the cache, then the next SpendCoin call decrements the value again causing a size_t underflow.

   Move the BOOST_CHECK_THROW test to its own suite, so that other flush tests are not affected by the caches’s inconsistent state

2. DrahtBot added the label Tests on Sep 12, 2025
3. 
   DrahtBot commented at 7:45 pm on September 12, 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/33381.

   Reviews

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

4. PiRK force-pushed on Sep 12, 2025
5. 
   PiRK commented at 8:01 pm on September 12, 2025: contributor

   I force pushed to fix a use-after-move bug
6. 
   achow101 commented at 8:47 pm on September 17, 2025: member

   This change breaks the test.
7. PiRK force-pushed on Sep 19, 2025
8. 
   PiRK commented at 4:26 pm on September 19, 2025: contributor

   let’s see if this works. I moved the problematic test line to its own test suite, where it does not affect other flush tests.

   The failure I’m trying to solve seems intermittent. It happens about 75% of the time on master if I just add the scriptPubKey to MakeCoin. After my commit I get no failure in 10 runs of ./build/bin/test_bitcoin --run_test=coins_tests

9. 
   PiRK commented at 4:31 pm on September 19, 2025: contributor

   To clarify the rationale for this PR: the SanityCheck test added to flush_all in https://github.com/bitcoin/bitcoin/pull/28280/commits/a14edada8a051e280af6fedd5130be40247e2d7a does not have full coverage if cachedCoinsUsage is not exercised.

   This PR ensure that the cachedCoinsUsage part of SanityCheck now actually does something useful for the TestFlushBehavior suite.

10. DrahtBot added the label Needs rebase on Sep 23, 2025
11. test: Fix TestFlushBehavior when the random coin has non-zero dynamic memory usage

    If the random coin in this test has a non-null scriptpubkey, `cache->SanityCheck();` fails an assertion because the `BOOST_CHECK_THROW` line leaves `CCoinsViewCache::cachedCoinsUsage` in a corrupted state: the erroring `AddCoin` call decrements the value to 0 even though the coin is still in the cache, then the next `SpendCoin` call decrements the value again causing a `size_t` underflow.
    ```
    $  ./build/bin/test_bitcoin  --run_test=coins_tests
    Running 8 test cases...
    test_bitcoin: ./coins.cpp:345: void CCoinsViewCache::SanityCheck() const: Assertion `recomputed_usage == cachedCoinsUsage' failed.
    unknown location(0): fatal error: in "coins_tests/ccoins_flush_behavior": signal: SIGABRT (application abort requested)
    ./test/coins_tests.cpp(987): last checkpoint
    test_bitcoin: ./common/args.cpp:578: 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(1064): last checkpoint: "coins_resource_is_used" fixture ctor
    ```
    
    Move the `BOOST_CHECK_THROW` line to its own suite so it does not corrupt the coins view for further flush tests. .

    c9ad7e5bc3
12. in ci/test/01_base_install.sh:59 in 4d531b7de1 outdated

    55@@ -56,10 +56,9 @@ if [ -n "$PIP_PACKAGES" ]; then
    56 fi
    57 
    58 if [[ -n "${USE_INSTRUMENTED_LIBCPP}" ]]; then
    59+  ${CI_RETRY_EXE} git clone --depth=1 https://github.com/llvm/llvm-project -b "llvmorg-21.1.1" /llvm-project
    

    ---

    ---

    

    l0rinc commented at 4:52 pm on September 23, 2025:

    I don’t understand how this related to the PR. Is this really needed here or can it be split out to a different PR?

    ---

    

    PiRK commented at 5:10 pm on September 23, 2025:

    Not sure why this commit was pulled into my PR the last time i rebased on master. This was the top commit from master that had just been merged (#33364) at the time

    Some kind of github glitch?

    ---

    

    maflcko commented at 6:09 pm on September 23, 2025:

    https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#rebasing-changes
13. PiRK force-pushed on Sep 24, 2025
14. 
    PiRK commented at 6:51 am on September 24, 2025: contributor

    Rebased, the phantom commit is gone now
15. DrahtBot removed the label Needs rebase on Sep 24, 2025
16. 
    l0rinc commented at 2:42 am on October 20, 2025: contributor

    @PiRK, can you please check if this was already fixed by #32313?
17. 
    PiRK commented at 6:48 am on October 20, 2025: contributor

    @l0rinc Yes, #32313 fixes it.

    Tested with this patch:

     0diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp
     1index a1152d245f..a3ee441c42 100644
     2--- a/src/test/coins_tests.cpp
     3+++ b/src/test/coins_tests.cpp
     4@@ -880,6 +880,7 @@ Coin MakeCoin()
     5 {
     6     Coin coin;
     7     coin.out.nValue = m_rng.rand32();
     8+    coin.out.scriptPubKey.assign(m_rng.randbits(6), 0);
     9     coin.nHeight = m_rng.randrange(4096);
    10     coin.fCoinBase = 0;
    11     return coin;
    

    coins_tests fails as expected on the commit just before #32313 was merged

    0./test/coins_tests.cpp(1050): Entering test case "ccoins_flush_behavior"
    1test_bitcoin: ./coins.cpp:345: void CCoinsViewCache::SanityCheck() const: Assertion `recomputed_usage == cachedCoinsUsage' failed.
    

    And the test now passes on master.

    Not sure if setting the scriptPubkey in ccoins_flush_behavior now adds any useful coverage after you’ve added a dedicated unit test and extra assertions in the other PR. I’ll let you judge, and close this PR accordingly.

18. 
    PiRK commented at 7:05 am on October 20, 2025: contributor

    AFAICT the issue my patch raised was only reachable after suppressing a std::logic_error, which shouldn’t happen in production code.
19. 
    l0rinc commented at 7:55 pm on October 20, 2025: contributor

    Was a good find, thanks, I agree that it doesn’t make a lot of sense anymore to keep this around, if you agree, please consider closing the PR
20. PiRK closed this on Oct 20, 2025

    ---

Contributors
PiRK DrahtBot achow101 l0rinc maflcko

Labels
Tests