fuzz: Add target for coins database #32602

1. 
   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.

2. fuzz: avoid underflow in coins_view target 56d878c465
3. 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
4. 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.

   8625a8fcbd
5. 
   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
   Concept ACK	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.

6. DrahtBot added the label Tests on May 23, 2025
7. 
   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).

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

   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);
   

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

   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,
   

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

    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.
11. in src/test/fuzz/coins_view.cpp:45 in 8625a8fcbd

    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?
12. in src/test/fuzz/coins_view.cpp:90 in 8625a8fcbd

    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?
13. 
    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?

Contributors
marcofleon DrahtBot l0rinc

Labels
Tests