fuzz: Fix mini_miner_selection running out of coin #27806
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
|
|
murchandamus
force-pushed
the
2023-06-miniminer-out-of-coin
branch
from
bbb9277
to
de5d862
Compare
|
Added the additional assert suggested above in #27806 (comment) |
|
I presume this is a fix for https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=59385, which was closed unexpectedly and incorrectly by OSS-Fuzz. Also, the issue should ideally reproduce outside of msan. To fix both, I've just enabled |
Noted, thanks! |
src/test/fuzz/mini_miner.cpp
Outdated
| const size_t num_outputs = fuzzed_data_provider.ConsumeIntegralInRange<size_t>(2, 5); | ||
| assert(!available_coins.empty()); | ||
| const size_t num_inputs = std::min(size_t{2}, available_coins.size()); | ||
| const size_t num_outputs = fuzzed_data_provider.ConsumeIntegralInRange<size_t>(3, 5); |
There was a problem hiding this comment.
Any reason this is set to 3, or previously to 2, when it could be 1 to allow for transactions with one output?
I don't think we should reduce test coverage with the rationale "belt-and-suspenders".
There was a problem hiding this comment.
The point of this fuzz test is to test the calculation of bump fees. If all outputs of transactions are spent, there are no bump fees to be calculated. We create at least two outputs to chain one other transaction off of the first (which we add to available_coins, and to have another output left hanging to calculate bump fees on later. We could also permit a single output, if we just make it conclude transaction creation if it runs out of available_coins. I guess that would still achieve our test’s goals on the higher side of outputs.
With the std::min(2, available_coins.size()) the (2, 5) should also not crash, though. I’ll test that and return it to that if my understanding is substantiated.
There was a problem hiding this comment.
I’ve reset it to (2, 5) after fuzzing ~100k execs and the problematic seed. I’m not sure (1, 5) would be an improvement in this particular fuzz seed, but happy to change it if people are convinced otherwise.
Fixes a bug in the mini_miner_selection fuzz test found by fuzzing: It was possible for the mini_miner_selection fuzz test to generated transactions that created fewer new spendable outputs than the two inputs they each spend. If the fuzz seed did so consistently, eventually it would cause a `pop_front()` on an empty available_coins. Fixed by: - asserting that available_coins is not empty before generating tx - allowing to build tx with a single coin if only one is available
murchandamus
force-pushed
the
2023-06-miniminer-out-of-coin
branch
from
de5d862
to
76c5ea7
Compare
|
lgtm ACK 76c5ea7 |
76c5ea7 fuzz: Fix mini_miner_selection running out of coin (Murch) Pull request description: Fixes a bug in the mini_miner_selection fuzz test found by fuzzing: It was possible for the mini_miner_selection fuzz test to generated transactions that created fewer new outputs than the two inputs they each spent. If the fuzz seed did so consistently, eventually it would cause a `pop_front()` on an empty available_coins which resulted in undefined behavior. Fixed per belt-suspender approach: - assert that available_coins is not empty before generating tx - generate at least two coins per new tx - allow building tx with a single input if only one coin is available ACKs for top commit: MarcoFalke: lgtm ACK 76c5ea7 dergoegge: reACK 76c5ea7 Tree-SHA512: 5b7ffd1905a712733ad5364958ad79874dd8c31bd50069b0d3e6f734da0f2d496cb08cbe0afa47115674313e1cb7166a6087f2ccbce289774caddc790583e241
Fixes a bug in the mini_miner_selection fuzz test found by fuzzing: It was possible for the mini_miner_selection fuzz test to generated transactions that created fewer new outputs than the two inputs they each spent. If the fuzz seed did so consistently, eventually it would cause a
pop_front()on an empty available_coins which resulted in undefined behavior.Fixed per belt-suspender approach: