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

pull brunoerg wants to merge 2 commits into bitcoin:master from brunoerg:2024-04-fuzz-spend changing 4 files +240 −115
  1. 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:

    @@ -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.

  2. DrahtBot commented at 7:43 PM on April 22, 2024: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage

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

    <!--021abf342d371248e50ceaed478a90ca-->

    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.

    <!--174a7506f384e20aa4161008e828411d-->

    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.

  3. DrahtBot added the label Tests on Apr 22, 2024
  4. brunoerg force-pushed on Apr 22, 2024
  5. DrahtBot added the label CI failed on Apr 22, 2024
  6. brunoerg commented at 9:21 PM on April 22, 2024: contributor

    cc: @maflcko

  7. DrahtBot removed the label CI failed on Apr 23, 2024
  8. 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:

    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.

  9. maflcko approved
  10. maflcko commented at 8:42 AM on April 23, 2024: member

    lgtm, nice!

  11. brunoerg force-pushed on Apr 23, 2024
  12. brunoerg force-pushed on Apr 24, 2024
  13. DrahtBot added the label Needs rebase on Apr 30, 2024
  14. brunoerg force-pushed on Apr 30, 2024
  15. brunoerg commented at 9:05 PM on April 30, 2024: contributor

    Rebased

  16. brunoerg force-pushed on Apr 30, 2024
  17. DrahtBot removed the label Needs rebase on Apr 30, 2024
  18. brunoerg requested review from maflcko on May 1, 2024
  19. DrahtBot added the label CI failed on Jun 20, 2024
  20. DrahtBot commented at 1:31 PM on June 20, 2024: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

    🚧 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.

    <sub>Debug: https://github.com/bitcoin/bitcoin/runs/24446674984</sub>

  21. brunoerg force-pushed on Jun 20, 2024
  22. brunoerg commented at 4:42 PM on June 20, 2024: contributor

    Rebased and fixed ToString usage.

  23. DrahtBot removed the label CI failed on Jun 20, 2024
  24. 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

  25. 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.

  26. brunoerg force-pushed on Jun 28, 2024
  27. 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.

  28. DrahtBot added the label CI failed on Jun 28, 2024
  29. DrahtBot commented at 3:34 PM on June 28, 2024: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

    🚧 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.

    <sub>Debug: https://github.com/bitcoin/bitcoin/runs/26812851101</sub>

  30. brunoerg force-pushed on Jun 28, 2024
  31. brunoerg force-pushed on Jun 28, 2024
  32. DrahtBot removed the label CI failed on Jun 28, 2024
  33. 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?

  34. 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?

  35. 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%.

  36. 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.

  37. brunoerg force-pushed on Jul 12, 2024
  38. brunoerg commented at 4:41 PM on July 12, 2024: contributor

    Force-pushed adding SeedRandomForTest

  39. brunoerg requested review from marcofleon on Jul 18, 2024
  40. hebasto added the label Needs CMake port on Aug 16, 2024
  41. 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.

  42. DrahtBot added the label Needs rebase on Aug 28, 2024
  43. brunoerg force-pushed on Aug 28, 2024
  44. brunoerg commented at 10:01 PM on August 28, 2024: contributor

    Rebased

  45. DrahtBot removed the label Needs rebase on Aug 28, 2024
  46. hebasto commented at 12:54 PM on August 29, 2024: member

    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.

  47. brunoerg force-pushed on Aug 29, 2024
  48. brunoerg commented at 1:42 PM on August 29, 2024: contributor

    Thanks, @hebasto. Just added it.

  49. hebasto removed the label Needs CMake port on Aug 29, 2024
  50. DrahtBot added the label Needs rebase on Sep 2, 2024
  51. brunoerg force-pushed on Sep 3, 2024
  52. brunoerg commented at 1:16 PM on September 3, 2024: contributor

    Rebased

  53. DrahtBot removed the label Needs rebase on Sep 3, 2024
  54. brunoerg commented at 7:44 PM on September 20, 2024: contributor

    ping review @maflcko @marcofleon

  55. wallet: move `ImportDescriptors`/`FuzzedWallet` to util 3db68e29ec
  56. fuzz: wallet: add target for `CreateTransaction` c495731a31
  57. 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.

  58. brunoerg force-pushed on Sep 27, 2024
  59. brunoerg commented at 5:40 PM on September 27, 2024: contributor

    Force-pushed addressing #29936 (review)

  60. marcofleon commented at 4:18 PM on October 4, 2024: contributor

    ACK c495731a316d9c97ee05a08cf5087c5535f84bd4

    Coverage report: wallet_create_transaction

  61. brunoerg commented at 1:48 PM on October 9, 2024: contributor

    review ping @maflcko

  62. 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:

    diff --git a/src/wallet/test/fuzz/spend.cpp b/src/wallet/test/fuzz/spend.cpp
index 9abd9e402a..cd092256c1 100644
--- a/src/wallet/test/fuzz/spend.cpp
+++ b/src/wallet/test/fuzz/spend.cpp
@@ -68,7 +68,7 @@ FUZZ_TARGET(wallet_create_transaction, .init = initialize_setup)
         tx.vout[0].scriptPubKey = GetScriptForDestination(fuzzed_wallet.GetDestination(fuzzed_data_provider));
         LOCK(fuzzed_wallet.wallet->cs_wallet);
         auto txid{tx.GetHash()};
-        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}))};
+        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})};
         assert(ret.second);
     }
    brunoerg commented at 1:18 PM on October 15, 2024:

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

  63. maflcko approved
  64. maflcko commented at 3:31 PM on October 9, 2024: member

    ACK c495731a316d9c97ee05a08cf5087c5535f84bd4 🏻

    <details><summary>Show signature</summary>

    Signature:

    untrusted 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}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK c495731a316d9c97ee05a08cf5087c5535f84bd4 🏻
CQqOfhILxAACVT6MSHfbt2SPr/M0NTHuBhtH94qiGP10o5bfwLIb7JNUKSRTeC8GrR4cBeU/jSTPIyoolFRQBQ==

    </details>

  65. allecakes9 approved
  66. maflcko commented at 7:27 AM on October 25, 2024: member

    rfm?

  67. fanquake merged this on Oct 25, 2024
  68. fanquake closed this on Oct 25, 2024
  69. TheCharlatan referenced this in commit a73b2bd0f0 on Nov 14, 2024
  70. bug-castercv502 referenced this in commit fdcc066ca0 on Sep 28, 2025
  71. bitcoin locked this on Oct 25, 2025
Contributors
brunoergDrahtBotmaflckomarcofleonhebastoallecakes9


marcofleon

Labels
Tests
Linked (view graph)
#27271 RPC: Fix fund transaction crash when at 0-value, 0-fee#29901 Wallet fuzzing tracking issue
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-21 18:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me