fuzz: Extend mini_miner fuzz coverage to max block weight #31803

pull fjahr wants to merge 3 commits into bitcoin:master from fjahr:pr31384-fuzz-follow-up changing 1 files +37 −5
  1. fjahr commented at 4:46 pm on February 5, 2025: contributor

    This is follow-up to #31384 which should be merged shortly. Only the last commit is new.

    It expands the fuzz test to cover the full range of allow block sizes in BlockAssembler by utilizing the block_adjusted_max_weight option if necessary.

    See also the brief discussion here for context: https://github.com/bitcoin/bitcoin/pull/31384/files#r1942039833

  2. DrahtBot commented at 4:46 pm on February 5, 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/31803.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Stale ACK ismaelsadeeq, glozow

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

    Conflicts

    No conflicts as of last run.

  3. DrahtBot added the label Tests on Feb 5, 2025
  4. fjahr force-pushed on Feb 10, 2025
  5. fjahr marked this as ready for review on Feb 10, 2025
  6. fjahr commented at 6:35 pm on February 10, 2025: contributor
    #31384 was merged, so this is rebased and ready for review.
  7. in src/test/fuzz/mini_miner.cpp:218 in 8c92624365 outdated
    184@@ -184,6 +185,11 @@ FUZZ_TARGET(mini_miner_selection, .init = initialize_miner)
    185     node::BlockAssembler::Options miner_options;
    186     miner_options.blockMinFeeRate = target_feerate;
    187     miner_options.nBlockMaxWeight = MAX_BLOCK_WEIGHT;
    188+    // Only setting reserved weight when necessary based on the template size
    189+    const auto reserved_weight = MAX_BLOCK_WEIGHT - pool.GetTotalTxSize();
    190+    if (reserved_weight < DEFAULT_BLOCK_RESERVED_WEIGHT) {
    191+        miner_options.block_reserved_weight = reserved_weight;
    192+    }
    


    ismaelsadeeq commented at 10:28 pm on February 10, 2025:

    I don’t think this conditional check is necessary. It will be simpler and more straightforward to just set the minimum rather than creating a new variable and then checking the conditional statement.

    In the worst case, we are creating two variables here, whereas in my suggestion, we are just updating the initial variable.

    0    miner_options.block_reserved_weight = MINIMUM_BLOCK_RESERVED_WEIGHT;  
    

    fjahr commented at 4:14 pm on February 11, 2025:
    Yes, this would be simpler but I am not sure if we want it to be simpler. With fuzzing we usually try to hit as different many scenarios as possible and that’s why I chose this, sometimes setting block_reserved_weight and sometimes not, and setting it to a variety of values. I’m not an expert on fuzzing though, maybe someone with a focus on it can weight in here.

    fjahr commented at 4:42 pm on March 3, 2025:
    @marcofleon @dergoegge Do you have feedback on what your preferred approach would be here?

    marcofleon commented at 1:52 pm on March 4, 2025:

    It seems like the conditional never ends up being true, which you can see in the test’s coverage.

    In general, I do think you’re right about wanting the fuzz test to cover a range of possible values, but I’m not quite familiar enough with the mining code to know what the best approach here would be.


    fjahr commented at 2:32 pm on March 4, 2025:
    Thanks @marcofleon , that the code doesn’t run seems to be because we don’t add enough transactions to hit the limit, see LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 100) and in the coverage you linked the break on line 165 is never hit as well. This requires a bit of additional conceptual review but for now I have increased the loop limit to 5000 and I hope we should see some hits with that. Will the link get updated with the new numbers when I visit it again tomorrow?

    fjahr commented at 2:37 pm on March 4, 2025:
    Thinking about it a bit more, this was probably an optimization to make the test faster. Not sure if increasing the number that much is seen as a downside. I could also add something so that we usually have 0-100 but then with a 10% chance or so I increase the number dramatically so that we hit the number max block limit with some regularity as well. There my understanding performance requirements on fuzz tests is limited so not sure which approach would be preferred.

    fjahr commented at 3:47 pm on March 4, 2025:
    Ok, that hit an error further down below. It seems to me like the cluster limit of 500 tx was hit. Let me try again with a max of 500 tx and increasing the potential number of outputs instead.
  8. in src/test/fuzz/mini_miner.cpp:133 in 8c92624365 outdated
    128@@ -129,6 +129,8 @@ FUZZ_TARGET(mini_miner_selection, .init = initialize_miner)
    129     // Make a copy to preserve determinism.
    130     std::deque<COutPoint> available_coins = g_available_coins;
    131     std::vector<CTransactionRef> transactions;
    132+    // The maximum block template size we expect to produce
    133+    const auto block_adjusted_max_weight = MAX_BLOCK_WEIGHT - MINIMUM_BLOCK_RESERVED_WEIGHT;
    


    ismaelsadeeq commented at 10:30 pm on February 10, 2025:
    Nice moving this here, prevents creating block_adjusted_max_weight variable in each loop iteration.
  9. ismaelsadeeq commented at 10:30 pm on February 10, 2025: member
    Concept ACK
  10. ismaelsadeeq commented at 3:13 pm on March 3, 2025: member
    fwiw ACK 8c926243656b42479aa5e3cd13701450a43922a6
  11. fjahr force-pushed on Mar 4, 2025
  12. fanquake requested review from ismaelsadeeq on Mar 21, 2025
  13. fanquake requested review from glozow on Mar 21, 2025
  14. in src/test/fuzz/mini_miner.cpp:136 in a63bae6e28 outdated
    132@@ -133,12 +133,12 @@ FUZZ_TARGET(mini_miner_selection, .init = initialize_miner)
    133     const auto block_adjusted_max_weight = MAX_BLOCK_WEIGHT - MINIMUM_BLOCK_RESERVED_WEIGHT;
    134 
    135     LOCK2(::cs_main, pool.cs);
    136-    LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 100)
    137+    LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 500)
    


    glozow commented at 12:45 pm on March 25, 2025:
    Worth noting this is pegged to the number at which GatherClusters halts (which would lead to !IsReadyToCalculate)

    fjahr commented at 9:21 pm on March 26, 2025:
    Yepp, I actually ran into this during my first iteration of working on this. I will add a comment to explain it.
  15. glozow commented at 12:46 pm on March 25, 2025: member
    lgtm ACK a63bae6e281a9d4b0d4d6948550107683cf65d3b
  16. fjahr force-pushed on Mar 26, 2025
  17. fjahr commented at 9:22 pm on March 26, 2025: contributor

    I finally got back to this today and it drove me nuts in many different ways! The primary source of frustration was a my lack of experience with fuzzing tests + recent changes on the build system and where the bins are placed + this PR not having these changes but me thinking it had these changes + fuzzing and macos dislike each other. Thanks to @marcofleon for support and hence the rebase here!

    However, I did eventually also find something interesting! :) @marcofleon had already mentioned to me in a DM that the latest iteration of the code here was still not reaching the full-block case within a reasonable amount of iterations. I thought my guestimate numbers were just a bit off and so I cranked up the number of outputs per tx so they would take up more space in the block. (The number of tx per block are limited to 500 by cluster mempool DoS protection as @glozow mentioned above as well.) But this didn’t work.

    Learning 1: Filling up the block with smaller txs

    If the numbers of outputs are higher the number of outputs also increases because we use the number of available coins there. This means we get really large transactions fast, which is what we want to fill up the block. But then it becomes very unlikely that the last (huge) transaction we are adding is actually fitting exactly into the block so that the block size requires a reconfiguration of the reserved weight. So in order to fill up the block completely with a bit of consistency we need to add some smaller transactions in the end. At least that seemed the most intuitive way to deal with this but I am also open to reconsider this. I am sure that there are other solutions possible and I guess it might also be fine if say it’s fine if the block gets only filled up only very seldomly but I think adding something that adds consistency is valuable. So, I am suggesting to fill up the block with a ConsumeBool() chance.

    Learning 2: Apples and Oranges

    This line currently in master actually doesn’t work as expected:

    0if (pool.GetTotalTxSize() + GetVirtualTransactionSize(*tx) >= block_adjusted_max_weight) break;
    

    I compares bytes (left) with weight units (right). It didn’t matter though because the test currently adds so few and small transactions that this condition could never get hit anyway.

    Conclusion

    This is now obviously now increasing in scope more than I had expected. I am pushing a commit that has the changes that I propose and also includes some debug changes so you can test it out and confirm what I found. I would just as for some quick feedback on the approach I have chosen with regards to 1 or if I should try something different. For example, producing these full blocks probably slows the test down a bit, if that’s a concern I could model it a bit differently so we don’t fill up blocks that often.

    EDIT: The debug code isn’t there but can still be seen here: https://github.com/bitcoin/bitcoin/commit/54bf199efaddbf4aa4c1307f5d63d7902f352e67

  18. DrahtBot commented at 11:04 pm on March 26, 2025: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/39476452828

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  19. DrahtBot added the label CI failed on Mar 26, 2025
  20. fuzz: Fix block size stop gap in mini_miner_selection
    The check was comparing bytes (left) to WU (right). The check was not necessary so far because the test never hit high enough numbers for it to be actually necessary.
    0da2ae35c8
  21. fuzz: Fuzz reserved weight option in BlockAssembler fd68e06908
  22. fjahr force-pushed on Mar 27, 2025
  23. fuzz: Increase number of transactions in block in mini_miner test
    At the current configuration there was zero chance to hit the size limit of a block. The new configuration uses larger transactions and conditionally adds the maximum possible number of transactions instead of using the previous limited while loop to ensure that with some regularity the test runs into full blocks.
    5702c4410d
  24. fjahr force-pushed on Mar 27, 2025
  25. fjahr commented at 11:11 pm on March 27, 2025: contributor

    Alright, after spending some more time with this I have convinced myself that the “fill up with small transactions” approach is somewhat reasonable and intuitive. An alternative approach would have been to continue looping after the last large transaction didn’t fit and try to halven the last input and output numbers until we encounter a transaction that fits. This would save us a few loops probably but I think it would be less intuitive.

    What I wasn’t happy with was that we were doing only full blocks with the last iteration which made the test slow. I found a simple way to let the test choose between full block mode and (probably) small block mode which I think is what we want.

    Since I am a bit more confident with these latest changes in the approach I have cleaned up the code and restructured the commits.

  26. DrahtBot removed the label CI failed on Mar 28, 2025

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: 2025-03-29 06:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me