Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34264. ReviewsSee the guideline for information on the review process. ConflictsNo conflicts as of last run. |
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
|
From CI: |
frankomosh
left a comment
frankomosh
left a comment
There was a problem hiding this comment.
I think that the variables int next_locktime and CAmount all_values become dead code after you’ve refactored?
Also some inline nits below.
src/wallet/test/fuzz/spend.cpp
Outdated
| int num_inputs = fuzzed_data_provider.ConsumeIntegralInRange<int>(0, available_outpoints.size()); | ||
| for (int i = 0; i < num_inputs; i++) { | ||
| auto out = PickValue(fuzzed_data_provider, available_outpoints); | ||
| tx.vin.emplace_back(CTxIn(out)); |
There was a problem hiding this comment.
Could simplify to tx.vin.emplace_back(out)
src/wallet/test/fuzz/spend.cpp
Outdated
| if (fuzzed_data_provider.ConsumeBool()) change_pos = fuzzed_data_provider.ConsumeIntegral<unsigned int>(); | ||
| bool lockUnspents = fuzzed_data_provider.ConsumeBool(); | ||
|
|
||
| (void)FundTransaction(*fuzzed_wallet.wallet, tx, recipients, change_pos, lockUnspents, coin_control); |
There was a problem hiding this comment.
Also, should FundTransaction have wallet lock protection i.e wrap it in LOCK(fuzzed_wallet.wallet -> cs_wallet), as has been done in ListCoins ?
There was a problem hiding this comment.
Not really, FundTransaction() doesn't require an exclusive lock unlike ListCoins() does.
src/wallet/test/fuzz/spend.cpp
Outdated
| tx.vin.emplace_back(CTxIn(out)); | ||
| } | ||
| std::vector<CRecipient> recipients; | ||
| LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 50) { |
There was a problem hiding this comment.
I notice that this logic appears twice. Would it make sense to extract it into a lambda helper at the top of the function?
I believe |
Update the `wallet_create_transaction` fuzz target to move the transaction creation logic inside a `CallOneOf` block. This refactoring is a structural preparation for stateful fuzzing, allowing commits to easily add and interleave other wallet operations within the same loop.
Move the `CCoinControl` parameter configuration inside the fuzzing loop to allow mutation of settings between operations. Additionally, introduce logic to randomly `Select` or `UnSelect` specific available outpoints within `coin_control`. This allows the fuzzer to simulate scenarios where a user manually mandates specific inputs.
Extend the target to cover `FundTransaction()`.
Extend the fuzz target to cover `ListCoins()`.
4 participants
Make the
wallet/spendfuzz target stateful and extend coverage for several missing methods.