fuzz: wallet: add target for MigrateToDescriptor
#32624
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/32624. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste |
08b574d to
5f2fb4d
Compare
|
08b574dde2c5f13a4c9dadcb9d8bded158c59623..5f2fb4dd333f8d1db27c78414afc84454e6ee4a5: Address #32624 (comment) |
5f2fb4d to
73439a7
Compare
73439a7 to
6d29c9a
Compare
|
lgtm ACK 6d29c9a6d1b4239a0844791d4a53599ae3a79cd0 |
6d29c9a to
6ced804
Compare
|
Force-pushed addressing #32624 (comment). Thanks, @marcofleon. |
6ced804 to
6436ad7
Compare
marcofleon
left a comment
There was a problem hiding this comment.
Left some questions below.
Also, coverage report after about a day of fuzzing.
6436ad7 to
6227441
Compare
|
Force-pushed 6436ad7 to 6227441:
|
6227441 to
51917d9
Compare
|
Nice, lgtm ACK 51917d9 |
|
@maflcko friendly review ping :) |
frankomosh
left a comment
There was a problem hiding this comment.
Left a few observations and questions below.
|
|
||
| FUZZ_TARGET(spkm_migration, .init = initialize_spkm_migration) | ||
| { | ||
| SeedRandomStateForTest(SeedRand::ZEROS); |
There was a problem hiding this comment.
SeedRandomStateForTest(SeedRand::ZEROS) call appears to be unused in this target. Or is there something I am not seeing?
There was a problem hiding this comment.
What do you mean with unused? It is to avoid instability when it is using the global random state.
| if (legacy_data.LoadWatchOnly(script_dest)) watch_only = true; | ||
|
|
||
| size_t added_script{0}; | ||
| bool good_data{true}; |
There was a problem hiding this comment.
'good_data' is only cleared when a custom script deserialization fails. other failures leave it true(like when AddCScript returns false).
Is it intentional to leave a potentially incomplete state in those other cases?
There was a problem hiding this comment.
Did not know that. Thanks for the context.
| auto result{legacy_data.MigrateToDescriptor()}; | ||
| assert(result); | ||
| size_t added_chains{static_cast<size_t>(add_hd_chain) + static_cast<size_t>(add_inactive_hd_chain)}; | ||
| if ((add_hd_chain && version >= CHDChain::VERSION_HD_CHAIN_SPLIT) || (!add_hd_chain && add_inactive_hd_chain)) { |
There was a problem hiding this comment.
There might be a small issue with how version is used in the assertion here. The version variable is initialized at line 244 and only assigned to the active HD chain when add_hd_chain is true (line 249). However, when add_inactive_hd_chain is true, the inactive chain gets its own version choice at line 257.
Challenge arises when add_hd_chain is false but add_inactive_hd_chain is true. In this scenario, condition (!add_hd_chain && add_inactive_hd_chain) would trigger added_chains *= 2, but this doesn't account for whether the inactive chain's version is actually VERSION_HD_CHAIN_SPLIT or not , as it just assumes any inactive chain should double the count.
If you consider or think that these sequence of events might cause assertion miscalculations then maybe a solution might be store and checking the inactive chain's version separately or checking hd_chain.nVersion directly after it's set.
This PR adds fuzz coverage for the scriptpubkeyman migration (
MigrateToDescriptor). Note that it's a test for the migration of the scriptpubkey manager, not for the whole migration process as tried in #29694, because:This target loads keys, HDChain (even inactive ones), watch only and might add tons of different scripts, then calls
MigrateToDescriptor. It does not play with encrypted stuff because it would make the target super slow. Also, after the migration there are some assertions that would work as a regression test for #31452, for example.