fuzz: wallet: add target for `CreateTransaction`

brunoerg commented at 7:43 pm on April 22, 2024: contributor

This PR adds a fuzz target for the CreateTransaction function. It is a regression target for #27271 and can be testing by applying:

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

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

DrahtBot commented at 7:43 pm on April 22, 2024: contributor

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
ACK	marcofleon, maflcko

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:

#31149 (tinyformat: enforce compile-time checks for format string literals by stickies-v)
#30928 (tinyformat: refactor: increase compile-time checks and don’t throw for tfm::format_error by stickies-v)
#30221 (wallet: Ensure best block matches wallet scan state by achow101)
#28710 (Remove the legacy wallet and BDB dependency by achow101)
#25979 ([WIP] wallet: standardize change output detection process by furszy)

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.

DrahtBot added the label Tests on Apr 22, 2024

brunoerg force-pushed on Apr 22, 2024

DrahtBot added the label CI failed on Apr 22, 2024

brunoerg commented at 9:21 pm on April 22, 2024: contributor

cc: @maflcko

DrahtBot removed the label CI failed on Apr 23, 2024

in src/wallet/test/fuzz/notifications.cpp:57 in cbda244274 outdated

83-        }
84-    }
85-}
86-
87 /**
88  * Wraps a descriptor wallet for fuzzing.

maflcko commented at 8:41 am on April 23, 2024:

Maybe move this to util.cpp as well? In theory it is the wrong place, because it depends on FuzzedDataProvider, but I think this isn’t too important in tests.

brunoerg commented at 9:37 am on April 23, 2024:

Sgtm, will address it.

brunoerg commented at 2:44 pm on April 23, 2024:

Done. I’ve created a util file for wallet into test/fuzz/util.

brunoerg commented at 4:24 pm on April 24, 2024:

It seems the addition of a new util file makes this PR conflicts with other ones. Perhaps it’s better to put everything in wallet/test/util or leave as is? What do you think @maflcko?

maflcko commented at 4:42 pm on April 24, 2024:

Actually I don’t understand why the last push compiles. CI says:

https://cirrus-ci.com/task/6011306526900224?logs=ci#L2291 CXX test/fuzz/util/libtest_fuzz_a-wallet.o
but the wallet is disabled: with wallet = no https://cirrus-ci.com/task/6011306526900224?logs=ci#L1385

Shouldn’t this result in a link or even compile error?

brunoerg commented at 5:08 pm on April 24, 2024:

You’re right. I just tested here without wallet and it compiles. We don’t have any verification for ENABLE_WALLET in Makefile.test_fuzz.include like in Makefile.test.include. I could include it.

brunoerg commented at 9:16 pm on April 24, 2024:

Done.

maflcko approved

maflcko commented at 8:42 am on April 23, 2024: member

lgtm, nice!

brunoerg force-pushed on Apr 23, 2024

brunoerg force-pushed on Apr 24, 2024

DrahtBot added the label Needs rebase on Apr 30, 2024

brunoerg force-pushed on Apr 30, 2024

brunoerg commented at 9:05 pm on April 30, 2024: contributor

Rebased

brunoerg force-pushed on Apr 30, 2024

DrahtBot removed the label Needs rebase on Apr 30, 2024

brunoerg requested review from maflcko on May 1, 2024

DrahtBot added the label CI failed on Jun 20, 2024

DrahtBot commented at 1:31 pm on June 20, 2024: contributor

🚧 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 force-pushed on Jun 20, 2024

brunoerg commented at 4:42 pm on June 20, 2024: contributor

Rebased and fixed ToString usage.

DrahtBot removed the label CI failed on Jun 20, 2024

marcofleon commented at 2:03 pm on June 27, 2024: contributor

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 commented at 9:36 pm on June 27, 2024: contributor

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 force-pushed on Jun 28, 2024

brunoerg commented at 2:15 pm on June 28, 2024: contributor

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

Force-pushed adding it.

DrahtBot added the label CI failed on Jun 28, 2024

DrahtBot commented at 3:34 pm on June 28, 2024: contributor

🚧 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

brunoerg force-pushed on Jun 28, 2024

DrahtBot removed the label CI failed on Jun 28, 2024

marcofleon commented at 10:39 am on July 12, 2024: contributor

Nice, coverage looks good now. The issue I’m seeing is the harness has really low stability. It’s at 28% with the seed corpus I’m using. The only obvious source of randomness I’m seeing is in CreateTransactionInternal: https://github.com/bitcoin/bitcoin/blob/4d6af61d879914a660e73db5c2f2e6c4d0aa8243/src/wallet/spend.cpp#L1164-L1166

Sometimes change_pos isn’t set so it does hit that line. Maybe there are other reasons for the low stability somewhere else?

maflcko commented at 10:57 am on July 12, 2024: member

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 commented at 2:34 pm on July 12, 2024: contributor

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 commented at 2:54 pm on July 12, 2024: contributor

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 on Jul 12, 2024

brunoerg commented at 4:41 pm on July 12, 2024: contributor

Force-pushed adding SeedRandomForTest

brunoerg requested review from marcofleon on Jul 18, 2024

hebasto added the label Needs CMake port on Aug 16, 2024

marcofleon commented at 1:58 pm on August 19, 2024: contributor

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.

DrahtBot added the label Needs rebase on Aug 28, 2024

brunoerg force-pushed on Aug 28, 2024

brunoerg commented at 10:01 pm on August 28, 2024: contributor

Rebased

DrahtBot removed the label Needs rebase on Aug 28, 2024

hebasto commented at 12:54 pm on August 29, 2024: member

Rebased

Missed

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

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

brunoerg force-pushed on Aug 29, 2024

brunoerg commented at 1:42 pm on August 29, 2024: contributor

Thanks, @hebasto. Just added it.

hebasto removed the label Needs CMake port on Aug 29, 2024

DrahtBot added the label Needs rebase on Sep 2, 2024

brunoerg force-pushed on Sep 3, 2024

brunoerg commented at 1:16 pm on September 3, 2024: contributor

Rebased

DrahtBot removed the label Needs rebase on Sep 3, 2024

brunoerg commented at 7:44 pm on September 20, 2024: contributor

ping review @maflcko @marcofleon

wallet: move `ImportDescriptors`/`FuzzedWallet` to util 3db68e29ec

fuzz: wallet: add target for `CreateTransaction` c495731a31

in src/test/fuzz/util/wallet.h:25 in 59b4a4029c outdated

20+/**
21+ * Wraps a descriptor wallet for fuzzing.
22+ */
23+struct FuzzedWallet {
24+    std::shared_ptr<CWallet> wallet;
25+    FuzzedWallet(interfaces::Chain* chain, const std::string& name, const std::string& seed_insecure)

maflcko commented at 1:30 pm on September 27, 2024:

nit in 59b4a4029cce8f2eb09c26a7c8b972df21967d9b: chain can’t be nullptr, so it would be better to use & instead of * for self-documenting code.

This should also make the dimmed zebra smaller.

brunoerg commented at 5:39 pm on September 27, 2024:

Done.

brunoerg force-pushed on Sep 27, 2024

brunoerg commented at 5:40 pm on September 27, 2024: contributor

Force-pushed addressing #29936 (review)

marcofleon commented at 4:18 pm on October 4, 2024: contributor

ACK c495731a316d9c97ee05a08cf5087c5535f84bd4

Coverage report: wallet_create_transaction

brunoerg commented at 1:48 pm on October 9, 2024: contributor

review ping @maflcko

in src/wallet/test/fuzz/spend.cpp:71 in c495731a31

66+        if (all_values > MAX_MONEY) return;
67+        tx.vout[0].nValue = n_value;
68+        tx.vout[0].scriptPubKey = GetScriptForDestination(fuzzed_wallet.GetDestination(fuzzed_data_provider));
69+        LOCK(fuzzed_wallet.wallet->cs_wallet);
70+        auto txid{tx.GetHash()};
71+        auto ret{fuzzed_wallet.wallet->mapWallet.emplace(std::piecewise_construct, std::forward_as_tuple(txid), std::forward_as_tuple(MakeTransactionRef(std::move(tx)), TxStateConfirmed{chainstate.m_chain.Tip()->GetBlockHash(), chainstate.m_chain.Height(), /*index=*/0}))};

maflcko commented at 3:23 pm on October 9, 2024:

nit in c495731a316d9c97ee05a08cf5087c5535f84bd4: starting with C++17, you can use try_emplace:

 0diff --git a/src/wallet/test/fuzz/spend.cpp b/src/wallet/test/fuzz/spend.cpp
 1index 9abd9e402a..cd092256c1 100644
 2--- a/src/wallet/test/fuzz/spend.cpp
 3+++ b/src/wallet/test/fuzz/spend.cpp
 4@@ -68,7 +68,7 @@ FUZZ_TARGET(wallet_create_transaction, .init = initialize_setup)
 5         tx.vout[0].scriptPubKey = GetScriptForDestination(fuzzed_wallet.GetDestination(fuzzed_data_provider));
 6         LOCK(fuzzed_wallet.wallet->cs_wallet);
 7         auto txid{tx.GetHash()};
 8-        auto ret{fuzzed_wallet.wallet->mapWallet.emplace(std::piecewise_construct, std::forward_as_tuple(txid), std::forward_as_tuple(MakeTransactionRef(std::move(tx)), TxStateConfirmed{chainstate.m_chain.Tip()->GetBlockHash(), chainstate.m_chain.Height(), /*index=*/0}))};
 9+        auto ret{fuzzed_wallet.wallet->mapWallet.try_emplace(txid, MakeTransactionRef(std::move(tx)), TxStateConfirmed{chainstate.m_chain.Tip()->GetBlockHash(), chainstate.m_chain.Height(), /*index=*/0})};
10         assert(ret.second);
11     }
12

brunoerg commented at 1:18 pm on October 15, 2024:

Thanks, in case I touch it again I can address it.

maflcko approved

maflcko commented at 3:31 pm on October 9, 2024: member

ACK c495731a316d9c97ee05a08cf5087c5535f84bd4 🏻

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: ACK c495731a316d9c97ee05a08cf5087c5535f84bd4 🏻
3CQqOfhILxAACVT6MSHfbt2SPr/M0NTHuBhtH94qiGP10o5bfwLIb7JNUKSRTeC8GrR4cBeU/jSTPIyoolFRQBQ==

allecakes9 approved

maflcko commented at 7:27 am on October 25, 2024: member

rfm?

fanquake merged this on Oct 25, 2024

fanquake closed this on Oct 25, 2024

fuzz: wallet: add target for CreateTransaction #29936

Code Coverage

Reviews

Conflicts

fuzz: wallet: add target for `CreateTransaction` #29936