fuzz: add target for coinselection algorithms #24602
Conversation
mzumsande
force-pushed
the
202203_fuzz_coinselection
branch
from
08f41b7
to
30579ee
Compare
mzumsande
force-pushed
the
202203_fuzz_coinselection
branch
from
30579ee
to
e21d0b9
Compare
|
Nice, concept ACK |
| bool valid_outputgroup{false}; | ||
| for (auto& coin : coins) { | ||
| output_group.Insert(coin, /*depth=*/0, /*from_me=*/true, /*ancestors=*/0, /*descendants=*/0, positive_only); | ||
| valid_outputgroup = !positive_only || output_group.GetSelectionAmount() > 0; |
There was a problem hiding this comment.
Looking at the body of OutputGroup::Insert() I can't see how valid_outputgroup can become false. What do I miss?
There was a problem hiding this comment.
The problem I tried to address here is that OutputGroup::Insert() may not insert anything but just return if positive_only is true. In that case, we might have an empty OutputGroup with GetSelectionAmount() == 0 which would make SelectCoinsBnB() assert, so it cannot be submitted.
I added a comment for this.
| LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 10000) | ||
| { | ||
| const int nInput{fuzzed_data_provider.ConsumeIntegralInRange<int>(0, 10)}; | ||
| const int nInputBytes{fuzzed_data_provider.ConsumeIntegralInRange<int>(100, 10000)}; | ||
| const CAmount amount{fuzzed_data_provider.ConsumeIntegralInRange<CAmount>(1, MAX_MONEY)}; | ||
| AddCoin(utxo_pool, amount, nInput, nInputBytes, ++nextLockTime); | ||
| total_balance += amount; | ||
| } |
There was a problem hiding this comment.
A few iterations of this could lead to total_balance being bigger than MAX_MONEY. Could this trigger some assert in the tested functions?
There was a problem hiding this comment.
Good catch! I'm actually not sure. But I added a break statement to the loop for this case, which (at least I hope!) can't happen in reality and therefore doesn't need fuzzing.
mzumsande
force-pushed
the
202203_fuzz_coinselection
branch
from
e21d0b9
to
38915d9
Compare
|
Thanks for the review! I addressed the comments by @vasild and rebased on master. |
mzumsande
force-pushed
the
202203_fuzz_coinselection
branch
from
38915d9
to
591c108
Compare
This creates random OutputGroups and runs the existing coinselection algorithms for them.
mzumsande
force-pushed
the
202203_fuzz_coinselection
branch
from
591c108
to
21520b9
Compare
|
Pushed an update addressing feedback. |
|
ACK 21520b9 |
This adds a fuzz target for the coinselection algorithms by creating random
OutputGroups and running all three coin selection algorithms for them.It does not fuzz higher-level wallet logic for selecting eligible coins (as in
SelectCoins()), thought it probably would make sense to have a fuzz target for that too.