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.

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.

Nice moving this here, prevents creating block_adjusted_max_weight variable in each loop iteration.

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.

In "fuzz: Fix block size stop gap in mini_miner_selection" 0da2ae3

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.

In "fuzz: Fix block size stop gap in mini_miner_selection" 0da2ae3

Use WITNESS_SCALE_FACTOR here and when calculating reserved_weight

In "fuzz: Fuzz reserved weight option in BlockAssembler" fd68e06

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.

In "fuzz: Fuzz reserved weight option in BlockAssembler" fd68e06

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.

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

In "fuzz: Increase number of transactions in block in mini_miner test" 5702c44

nit: commit message is too long and not readable, should be broken.

In "fuzz: Increase number of transactions in block in mini_miner test" 5702c44

How is this better than just invoking fuzzed_data_provider.ConsumeBool(); in the LIMITED_WHILE?

In "fuzz: Fix block size stop gap in mini_miner_selection" 0da2ae3

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