fuzz: Add target for coins database

marcofleon commented at 3:01 pm on May 23, 2025: contributor

This reopens #28216.

The current coins_view target only tests CCoinsViewCache using a basic CCoinsView instance. The addition of the coins_view_db target enables testing with an actual CCoinsViewDB as the backend.

fuzz: avoid underflow in coins_view target 56d878c465

fuzz: move the coins_view target's body into a standalone function

We'll reuse it for a target where the coins view is a DB.

46e14630f7

DrahtBot commented at 3:01 pm on May 23, 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/32602.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	TheCharlatan, maflcko, l0rinc

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32313 (coins: fix cachedCoinsUsage accounting in CCoinsViewCache by l0rinc)

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.

DrahtBot added the label Tests on May 23, 2025

marcofleon commented at 3:03 pm on May 23, 2025: contributor

The coverage between the two targets can be compared here and here.

Instability was a bit of a concern in the last PR, but was answered in #28216 (comment).

in src/test/fuzz/coins_view.cpp:319 in 8625a8fcbd outdated

314+        .path = "",
315+        .cache_bytes = 1 << 20, // 1MiB.
316+        .memory_only = true,
317+    };
318+    CCoinsViewDB coins_db{std::move(db_params), CoinsViewOptions{}};
319+    TestCoinsView(fuzzed_data_provider, coins_db, /* is_db = */ true);

l0rinc commented at 3:08 pm on May 23, 2025:

nit: in other cases in this file the param hints are written as:

0    TestCoinsView(fuzzed_data_provider, coins_db, /*is_db=*/ true);

marcofleon commented at 11:22 am on May 29, 2025:

Done

in src/test/fuzz/coins_view.cpp:315 in 8625a8fcbd outdated

310+FUZZ_TARGET(coins_view_db, .init = initialize_coins_view)
311+{
312+    FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
313+    auto db_params = DBParams{
314+        .path = "",
315+        .cache_bytes = 1 << 20, // 1MiB.

l0rinc commented at 3:08 pm on May 23, 2025:

There’s a helper introduced since:

0        .cache_bytes = 1_MiB,

in src/test/fuzz/coins_view.cpp:72 in 8625a8fcbd outdated

68@@ -69,6 +69,12 @@ FUZZ_TARGET(coins_view, .init = initialize_coins_view)
69                     if (e.what() == std::string{"Attempted to overwrite an unspent coin (when possible_overwrite is false)"}) {
70                         assert(!possible_overwrite);
71                         expected_code_path = true;
72+                        // AddCoin() decreases cachedCoinsUsage by the memory usage of the old coin at the beginning and

l0rinc commented at 3:10 pm on May 23, 2025:

I still think this comment is way to verbose, we don’t need that much context here. Is this still needed after 327ee45 (#32313) where we might not get into the described situation anymore since cachedCoinsUsage update only happens after the mentioned exception now.

marcofleon commented at 11:22 am on May 29, 2025:

I think the comment makes sense here, even if it is a bit verbose. Happy to change it once a fix such as #32313 gets merged.

in src/test/fuzz/coins_view.cpp:45 in 8625a8fcbd outdated

41@@ -41,13 +42,12 @@ void initialize_coins_view()
42     static const auto testing_setup = MakeNoLogFileContext<>();
43 }
44 
45-FUZZ_TARGET(coins_view, .init = initialize_coins_view)
46+void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsView& backend_coins_view, bool is_db = false)

l0rinc commented at 3:16 pm on May 23, 2025:

The default value for is_db may not be intuitive, can we make it explicit instead?

marcofleon commented at 11:23 am on May 29, 2025:

Done.

in src/test/fuzz/coins_view.cpp:91 in 8625a8fcbd outdated

85@@ -80,7 +86,9 @@ FUZZ_TARGET(coins_view, .init = initialize_coins_view)
86                 (void)coins_view_cache.Sync();
87             },
88             [&] {
89-                coins_view_cache.SetBestBlock(ConsumeUInt256(fuzzed_data_provider));
90+                uint256 best_block{ConsumeUInt256(fuzzed_data_provider)};
91+                if (is_db && best_block.IsNull()) best_block = uint256::ONE;

l0rinc commented at 3:20 pm on May 23, 2025:

can you please add a short comment to the best_block = uint256::ONE cases?

marcofleon commented at 11:23 am on May 29, 2025:

Done.

l0rinc commented at 3:22 pm on May 23, 2025: contributor

Concept ACK

Quickly reviewed it again, will do it more deeply later - do we still need the cachedCoinsUsage comment after #32313, which should fix the accounting bug instead of working around it?

fuzz: add a target for the coins database

It reuses the logic from the `coins_view` target, except it uses an
in-memory CCoinsViewDB as the backend.

Note `CCoinsViewDB` will assert the best block hash is never null, so we
slightly modify the coins_view fuzz logic to take care of this.

cfc42ae5b7

marcofleon force-pushed on May 29, 2025

TheCharlatan approved

TheCharlatan commented at 12:20 pm on May 29, 2025: contributor

ACK cfc42ae5b7ef6d3892907cfe36dc3ae923495d37

DrahtBot requested review from l0rinc on May 29, 2025

fanquake requested review from dergoegge on May 29, 2025

maflcko commented at 3:18 pm on June 4, 2025: member

lgtm ACK cfc42ae5b7ef6d3892907cfe36dc3ae923495d37

l0rinc commented at 5:40 pm on June 4, 2025: contributor

code review ACK cfc42ae5b7ef6d3892907cfe36dc3ae923495d37

fanquake merged this on Jun 5, 2025

fanquake closed this on Jun 5, 2025

marcofleon deleted the branch on Jun 9, 2025

fuzz: Add target for coins database #32602

Code Coverage & Benchmarks

Reviews

Conflicts