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
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31803.
<!--021abf342d371248e50ceaed478a90ca-->
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.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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.
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 | + }
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.
miner_options.block_reserved_weight = MINIMUM_BLOCK_RESERVED_WEIGHT;
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.
@marcofleon @dergoegge Do you have feedback on what your preferred approach would be here?
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.
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?
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.
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.
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;
Nice moving this here, prevents creating block_adjusted_max_weight variable in each loop iteration.
Concept ACK
fwiw ACK 8c926243656b42479aa5e3cd13701450a43922a6
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)
Worth noting this is pegged to the number at which GatherClusters halts (which would lead to !IsReadyToCalculate)
Yepp, I actually ran into this during my first iteration of working on this. I will add a comment to explain it.
lgtm ACK a63bae6e281a9d4b0d4d6948550107683cf65d3b
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.
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.
This line currently in master actually doesn't work as expected:
if (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.
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
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed. <sub>Debug: https://github.com/bitcoin/bitcoin/runs/39476452828</sub>
<details><summary>Hints</summary>
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.
</details>
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.
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.
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.
FUZZ=mini_miner_selection ./fuzzbuild/bin/fuzz minimized-from-2d45d726fcc50840467fc32bae9a88c23560f4b6
./fuzzbuild/bin/fuzz: Running 1 inputs 1 time(s) each.
Running: minimized-from-2d45d726fcc50840467fc32bae9a88c23560f4b6
fuzz: ../../../../src/test/fuzz/mini_miner.cpp:237: void (anonymous namespace)::mini_miner_selection_fuzz_target(FuzzBufferType): Assertion `mock_template_txids.size() == blocktemplate->block.vtx.size()
' failed.
<details> <summary>base64</summary>
hyUP2dn9JwDRLpVgfX19/////wEAAAAAAAAWMDNhABk5YbkmuSwPMDU1DwAE2dlZAgAAAAAAAP0/
/T/Z2dnZ2dn////wJ9nRB11//2FgJQ84MmAlD/9hDzzZ////MScn8NnZ2dnZ////MScn8P0n8NnZ
2f3TAAAAAAAAAADZJ/D9J/DZ2dn90/Da2dnZ2WAlDzkxYCUxDycn8P0n8NnZx9PH2fDax/3Z2dnZ
2dnZ2dnZ////MScn8P0n8NnZ2dnZ2f0n8NnZx9PH2fDax/3Z2dnZ2dnZ2dnZ////MScn8P0n8NnZ
2dnZ2dnZ2dEHXX//YWAlDzgyYCUPJ/DZ2dn90/Da2dnZ2WAlDzkxYCUxDycn8P0n8NnZx9PH2fDa
x/3Z2dnZ2dnZ2dnZ////MScn8P0n8NnZ2dnZ2dnZ2dEHXX//YWAlDzgyYCUPf/9h//8PODJgJQ85
MWAlMQ8nJ/D9J/DZ2cfTx9nw2sf92dnZ2dnZ2dnZ2f///zEnJ/D9J/DZ2dnZ2dnZ2dnRB11//2Fg
JQ84MmAlDw==
</details>
I did a bit of debugging and target_feerate is 0 for this input, which I think affects the way mini miner constructs its block. The mini miner mock template puts in every transaction from the fuzz test's mempool, which ends up being one more tx than the template that the actual miner creates.
160 | @@ -161,7 +161,7 @@ FUZZ_TARGET(mini_miner_selection, .init = initialize_miner)
161 | const auto block_adjusted_max_weight = MAX_BLOCK_WEIGHT - DEFAULT_BLOCK_RESERVED_WEIGHT;
162 | // Stop if pool reaches block_adjusted_max_weight because BlockAssembler will stop when the
163 | // block template reaches that, but the MiniMiner will keep going.
In "fuzz: Fix block size stop gap in mini_miner_selection" 0da2ae35c82637e9c2830a2324c993756c94f15f
I think just the header of the commit is okay as the commit message, later when the check is necessary the commit message will be misleading.
We can add this note here in the PR discussions, or as a comment in the code; which should be updated when the check is necessary.
160 | @@ -161,7 +161,7 @@ FUZZ_TARGET(mini_miner_selection, .init = initialize_miner) 161 | const auto block_adjusted_max_weight = MAX_BLOCK_WEIGHT - DEFAULT_BLOCK_RESERVED_WEIGHT; 162 | // Stop if pool reaches block_adjusted_max_weight because BlockAssembler will stop when the 163 | // block template reaches that, but the MiniMiner will keep going. 164 | - if (pool.GetTotalTxSize() + GetVirtualTransactionSize(*tx) >= block_adjusted_max_weight) break; 165 | + if ((pool.GetTotalTxSize() + GetVirtualTransactionSize(*tx)) * 4 >= block_adjusted_max_weight) break;
In "fuzz: Fix block size stop gap in mini_miner_selection" 0da2ae35c82637e9c2830a2324c993756c94f15f
Use WITNESS_SCALE_FACTOR here and when calculating reserved_weight
if ((pool.GetTotalTxSize() + GetVirtualTransactionSize(*tx)) * WITNESS_SCALE_FACTOR >= block_adjusted_max_weight) break;
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;
In "fuzz: Fuzz reserved weight option in BlockAssembler" fd68e0690805d7e8980b84c3601a06c2b4291476
I think we should be consistent and use DEFAULT_BLOCK_MAX_WEIGHT here to reflect what is being used in the mining code, but does not matter much since they are equal.
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() * 4; 190 | + if (reserved_weight < DEFAULT_BLOCK_RESERVED_WEIGHT) { 191 | + miner_options.block_reserved_weight = reserved_weight - 1;
In "fuzz: Fuzz reserved weight option in BlockAssembler" https://github.com/bitcoin/bitcoin/commit/fd68e0690805d7e8980b84c3601a06c2b4291476
Why subtract 1 from the reserved weight, this would lead to mini miner selecting one transaction more than the block assembler?
I think leads to crash reported by @marcofleon
Here is the crash input coverage, which may help somewhat. In the fuzz harness it doesn't seem this line is ever hit, so it must be something else.
I think the crash has something to do with this block of code. https://github.com/bitcoin/bitcoin/blob/51166559808c3528f148b5c0c38cb4481e536dd8/src/node/mini_miner.cpp#L262-L265
From what I understand, mini miner would stop building the block here. But because target_feerate is 0 for this input, the break isn't hit so mini miner includes every transaction (439 txs in this case) from the mempool. Meanwhile, BlockAssembler builds its template based on the weight limit, which ends up including only 438 mempool transactions.
@marcofleon I think the issue is due to rounding error;
If I subtract one from the reserved weight to account for that block assembler also selects the last transaction and the input passes.
diff --git a/src/test/fuzz/mini_miner.cpp b/src/test/fuzz/mini_miner.cpp
index 8185fa3ab67..60e14651943 100644
--- a/src/test/fuzz/mini_miner.cpp
+++ b/src/test/fuzz/mini_miner.cpp
@@ -215,6 +215,8 @@ FUZZ_TARGET(mini_miner_selection, .init = initialize_miner)
const auto reserved_weight = MAX_BLOCK_WEIGHT - pool.GetTotalTxSize() * 4;
if (reserved_weight < DEFAULT_BLOCK_RESERVED_WEIGHT) {
miner_options.block_reserved_weight = reserved_weight - 1;
+ } else {
+ miner_options.block_reserved_weight = DEFAULT_BLOCK_MAX_WEIGHT - 1;
}
miner_options.test_block_validity = false;
miner_options.coinbase_output_script = CScript() << OP_0;
abubakarismail@Abubakars-MacBook-Pro ~/D/W/b/bitcoin-fuzz ((5702c441))> FUZZ=mini_miner_selection build_fuzz/bin/fuzz miniminer_crash.txt
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 1055565189
INFO: Loaded 1 modules (3275780 inline 8-bit counters): 3275780 [0x110168000, 0x110487c04),
INFO: Loaded 1 PC tables (3275780 PCs): 3275780 [0x110487c08,0x113683c48),
build_fuzz/bin/fuzz: Running 1 inputs 1 time(s) each.
Running: miniminer_crash.txt
Executed miniminer_crash.txt in 1907 ms
130 | @@ -131,14 +131,32 @@ FUZZ_TARGET(mini_miner_selection, .init = initialize_miner)
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;
In "fuzz: Increase number of transactions in block in mini_miner test" 5702c4410de5cd0c1ebc325a184e635e87752075
nit: commit message is too long and not readable, should be broken.
141 | + auto should_continue = [&]() -> bool { 142 | + if (use_limited_loop) { 143 | + return fuzzed_data_provider.ConsumeBool(); 144 | + } 145 | + return true; 146 | + };
In "fuzz: Increase number of transactions in block in mini_miner test" https://github.com/bitcoin/bitcoin/commit/5702c4410de5cd0c1ebc325a184e635e87752075
How is this better than just invoking fuzzed_data_provider.ConsumeBool(); in the LIMITED_WHILE?
186 | + // transactions and break when the block is close to the possible max 187 | + if (!min_size_tx.has_value()) { 188 | + min_size_tx = fuzzed_data_provider.ConsumeBool(); 189 | + if (!min_size_tx.value()) break; 190 | + } 191 | + break;
In "fuzz: Fix block size stop gap in mini_miner_selection" https://github.com/bitcoin/bitcoin/commit/0da2ae35c82637e9c2830a2324c993756c94f15f
Shouldn't you continue to the next iteration when min_size_tx value is true?
IIUC this code still breaks and we don't end up adding any small transaction?
The recent input coverage from @marcofleon verified this claim https://marcofleon.github.io/coverage/miniminer_crash/coverage/Users/mcomp/contrib/bitcoin/src/test/fuzz/mini_miner.cpp.html
Code review 5702c4410de5cd0c1ebc325a184e635e87752075
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.
I actually like this approach better—why is it considered less intuitive?
You should skip adding the last large transaction when it doesn’t fit, start adding smaller ones until we can’t add any more without exceeding the limit then bail out.
See POC: https://github.com/bitcoin/bitcoin/commit/e5e4acdabcdb93894b923d00debfc2d5510e0d73 will test if this makes sense.
Given the mini_miner_selection fuzz target is removed in #33629 (see https://github.com/bitcoin/bitcoin/pull/33629/commits/c88360cd82b6ba59b345d6d573b791e4964930d2), it might be appropriate to close this? Thanks for working on this fwiw!