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

[PiRK]

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

[DrahtBot]

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.

[PiRK]

I force pushed to fix a use-after-move bug

[achow101]

This change breaks the test.

[PiRK]

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

[PiRK]

To clarify the rationale for this PR: the SanityCheck test added to flush_all in a14edad 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.

[l0rinc]

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]

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]

https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#rebasing-changes

[PiRK]

Rebased, the phantom commit is gone now

[l0rinc]

@PiRK, can you please check if this was already fixed by #32313?

[PiRK]

@l0rinc Yes, #32313 fixes it.

Tested with this patch:

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

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

./test/coins_tests.cpp(1050): Entering test case "ccoins_flush_behavior"
test_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.

[PiRK]

AFAICT the issue my patch raised was only reachable after suppressing a std::logic_error, which shouldn't happen in production code.

[l0rinc]

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