fuzz: wallet: add target for `CreateTransaction`

[brunoerg]

This PR adds a fuzz target for the CreateTransaction function. It is a regression target for https://github.com/bitcoin/bitcoin/pull/27271 and can be testing by applying:

@@ -1110,7 +1110,7 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
     // This can only happen if feerate is 0, and requested destinations are value of 0 (e.g. OP_RETURN)
     // and no pre-selected inputs. This will result in 0-input transaction, which is consensus-invalid anyways
     if (selection_target == 0 && !coin_control.HasSelected()) {
-        return util::Error{_("Transaction requires one destination of non-0 value, a non-0 feerate, or a pre-selected input")};
+       // return util::Error{_("Transaction requires one destination of non-0 value, a non-0 feerate, or a pre-selected input")};
     }

Also, it moves ImportDescriptors function to src/wallet/test/util.h to avoid to duplicate same code.

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Stale ACK	marcofleon

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #30221 (wallet: Ensure best block matches wallet scan state by achow101)
• #28710 (Remove the legacy wallet and BDB dependency by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

[brunoerg]

cc: @maflcko

[brunoerg]

Rebased

[DrahtBot]

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/24446674984}

[brunoerg]

Rebased and fixed ToString usage.

[marcofleon]

Let me know if I'm missing something but at what point is the fuzzed wallet funded? I generated a coverage report and the test seems to blocked at SelectCoins in CreateTransactionInternal. It always results in an insufficient funds error, which means the fuzzer never gets past this line in CreateTransaction. https://github.com/bitcoin/bitcoin/blob/b27afb7fb774809c9c075d56e44657e0b607a00c/src/wallet/spend.cpp#L1367

[brunoerg]

Let me know if I'm missing something but at what point is the fuzzed wallet funded? I generated a coverage report and the test seems to blocked at SelectCoins in CreateTransactionInternal. It always results in an insufficient funds error, which means the fuzzer never gets past this line in CreateTransaction.

It is not being funded, I will add it. Thanks.

[brunoerg]

It is not being funded, I will add it. Thanks.

Force-pushed adding it.

[DrahtBot]

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/26812851101}

[maflcko]

The only obvious source of randomness I'm seeing is in CreateTransactionInternal:

Does it improve if you call SeedRandomForTest before every execution of every fuzz input?

[marcofleon]

Does it improve if you call SeedRandomForTest before every execution of every fuzz input?

Yes, calling SeedRandomForTest(SeedRand::ZEROS) at the beginning of every iteration improves stability from 28% to 70%.

[brunoerg]

Yes, calling SeedRandomForTest(SeedRand::ZEROS) at the beginning of every iteration improves stability from 28% to 70%.

Cool, I'll address it.

[brunoerg]

Force-pushed adding SeedRandomForTest

[marcofleon]

Sorry for taking a bit here. I still would like to figure out the stability issues for this harness (it's at about 70%), but it's turning into a whole rabbithole of its own. I don't think it needs to be a blocker for this PR. It would be good to have the test in and I'll continue to use this harness as my example for my "identifying instability" tool.

ACK 22f4d12eb25d34d8f2f6a23b44074dc1fb73f71d.

[brunoerg]

Rebased

[hebasto]

Rebased

Missed

--- a/src/wallet/test/fuzz/CMakeLists.txt
+++ b/src/wallet/test/fuzz/CMakeLists.txt
@@ -11,6 +11,7 @@ target_sources(fuzz
     $<$<BOOL:${USE_SQLITE}>:${CMAKE_CURRENT_LIST_DIR}/notifications.cpp>
     parse_iso8601.cpp
     $<$<BOOL:${USE_SQLITE}>:${CMAKE_CURRENT_LIST_DIR}/scriptpubkeyman.cpp>
+    spend.cpp
     wallet_bdb_parser.cpp
 )
 target_link_libraries(fuzz bitcoin_wallet)

This case convinced me that an internal build system check is required.

[brunoerg]

Thanks, @hebasto. Just added it.

[brunoerg]

Rebased