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:

    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.

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

    • #30928 (tinyformat: refactor: increase compile-time checks and don’t throw for tfm::format_error by stickies-v)

    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

    🚧 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

  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

    🚧 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

  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

    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.

  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)


brunoerg DrahtBot maflcko marcofleon hebasto


marcofleon

Labels
Tests

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: 2024-09-28 22:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me