fuzz: Delete wallet_notifications #28882

pull dergoegge wants to merge 1 commits into bitcoin:master from dergoegge:2023-11-fuzz-nuke-wn changing 2 files +0 −190
  1. dergoegge commented at 2:12 pm on November 15, 2023: member

    This harness is not doing much since it is incredibly slow. It’s probably better to remove it until someone finds the time to improve it. Otherwise this just wastes resources in our CI and oss-fuzz.

    On oss-fuzz for example, it never even starts fuzzing because it times-out while running through the existing corpus:

     0Component revisions (build r202311100604):
 1Bitcoin-core: b3898e946cf81e2e7b573e1c5204bd29af2feecd
 2Botan: 201cfe586e6d529360fbcde6f216f1da0c3db48f
 3Cryptofuzz: 537263836eae2cf6174689719a1dcfd92fb875c7
 4Secp256k1: c891c5c2f41912607f5c551d354c84bb4439e751
 5Trezor-firmware: ebeea4a209c7c9ebb885d8d7727c9e6e2bfe2bc0
 6Wycheproof: b063b4aedae951c69df014cd25fa6d69ae9e8cb9
 7
 8Bot name: oss-fuzz-linux-zone1-host-2dwk-5
 9Return code: 1
10
11Command: /mnt/scratch0/clusterfuzz/resources/platform/linux/extra_sanitizers /mnt/scratch0/clusterfuzz/bot/builds/clusterfuzz-builds_bitcoin-core_fcaf00df4dbc83b967efedfb94c0da52db5f507f/revisions/wallet_notifications -timeout=25 -rss_limit_mb=2560 -max_len=9961 -use_value_profile=1 -artifact_prefix=/mnt/scratch0/clusterfuzz/bot/inputs/fuzzer-testcases/ -max_total_time=6600 -print_final_stats=1 /mnt/scratch0/clusterfuzz/bot/inputs/fuzzer-testcases-disk/temp-359/new /mnt/scratch0/clusterfuzz/bot/inputs/data-bundles/bitcoin-core_wallet_notifications
12Time ran: 6610.037380456924
13
14INFO: Running with entropic power schedule (0xFF, 100).
15INFO: Seed: 3511012882
16INFO: Loaded 1 modules   (1703626 inline 8-bit counters): 1703626 [0x5ad2b8073180, 0x5ad2b821304a),
17INFO: Loaded 1 PC tables (1703626 PCs): 1703626 [0x5ad2b8213050,0x5ad2b9c11cf0),
18INFO:        0 files found in /mnt/scratch0/clusterfuzz/bot/inputs/fuzzer-testcases-disk/temp-359/new
19INFO:     1049 files found in /mnt/scratch0/clusterfuzz/bot/inputs/data-bundles/bitcoin-core_wallet_notifications
20INFO: seed corpus: files: 1049 min: 1b max: 4194302b total: 19233546b rss: 143Mb
21[#2](/bitcoin-bitcoin/2/)	pulse  ft: 20459 exec/s: 0 rss: 143Mb
22[#4](/bitcoin-bitcoin/4/)	pulse  cov: 10609 ft: 24082 corp: 2/2b exec/s: 0 rss: 144Mb
23[#8](/bitcoin-bitcoin/8/)	pulse  cov: 10650 ft: 26458 corp: 6/6b exec/s: 0 rss: 144Mb
24[#16](/bitcoin-bitcoin/16/)	pulse  cov: 10651 ft: 28496 corp: 14/14b exec/s: 0 rss: 144Mb
25[#32](/bitcoin-bitcoin/32/)	pulse  cov: 10656 ft: 30865 corp: 30/30b exec/s: 0 rss: 144Mb
26Slowest unit: 10 s:
27artifact_prefix='/mnt/scratch0/clusterfuzz/bot/inputs/fuzzer-testcases/'; Test unit written to /mnt/scratch0/clusterfuzz/bot/inputs/fuzzer-testcases/slow-unit-6dcd4ce23d88e2ee9568ba546c007c63d9131c1b
28Base64: QQ==
29[#64](/bitcoin-bitcoin/64/)	pulse  cov: 10660 ft: 33168 corp: 62/62b exec/s: 0 rss: 144Mb
30[#128](/bitcoin-bitcoin/128/)	pulse  cov: 10660 ft: 34990 corp: 126/129b exec/s: 0 rss: 144Mb
31[#256](/bitcoin-bitcoin/256/)	pulse  cov: 12470 ft: 40927 corp: 254/722b exec/s: 0 rss: 145Mb
32[#512](/bitcoin-bitcoin/512/)	pulse  cov: 13145 ft: 49807 corp: 509/4070b exec/s: 0 rss: 145Mb
33cf::fuzzing_strategies: random_max_len:1,value_profile:1,extra_sanitizers:1
  2. DrahtBot commented at 2:12 pm on November 15, 2023: 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
    Concept ACK brunoerg

    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:

    • #28710 (Remove the legacy wallet and BDB dependency by achow101)
    • #28578 (fuzz: add target for DescriptorScriptPubKeyMan by brunoerg)
    • #25665 (refactor: Add util::Result failure values, multiple error and warning messages by ryanofsky)

    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 Nov 15, 2023
  4. fuzz: Delete wallet_notifications 6367805632
  5. dergoegge force-pushed on Nov 15, 2023
  6. DrahtBot added the label CI failed on Nov 15, 2023
  7. maflcko commented at 2:22 pm on November 15, 2023: member

    This harness is not doing much since it is incredibly slow. It’s probably better to remove it until someone finds the time to improve it. Otherwise this just wastes resources in our CI and oss-fuzz.

    I think it was added as a regression fuzz test, so it can find at least that one regression. I am running it on my servers and I agree it should be improved.

  8. brunoerg commented at 3:12 pm on November 15, 2023: contributor

    Concept ACK

    I started working on improving it, perhaps I could prioritize it. Besides being slow, I believe it is not deterministic.

  9. DrahtBot removed the label CI failed on Nov 15, 2023
  10. maflcko commented at 3:31 pm on November 15, 2023: member

    I believe it is not deterministic.

    Can you add steps to reproduce?

  11. brunoerg commented at 6:24 pm on November 15, 2023: contributor

    Can you add steps to reproduce?

    The way it creates the wallet, calls SetupDescriptorScriptPubKeyMans without passing a key, it means that the function will create it internally.

     0// Make a seed
 1CKey seed_key;
 2seed_key.MakeNewKey(true);
 3CPubKey seed = seed_key.GetPubKey();
 4assert(seed_key.VerifyPubKey(seed));
 5
 6// Get the extended key
 7CExtKey master_key;
 8master_key.SetSeed(seed_key);
 9
10SetupDescriptorScriptPubKeyMans(master_key);
  12. dergoegge commented at 1:36 pm on November 16, 2023: member

    I am running it on my servers and I agree it should be improved. @maflcko thoughts on deleting it?

  13. maflcko commented at 1:39 pm on November 16, 2023: member

    No objection to deleting it, once there is a replacement. Though, it seems there is no rush to delete it, before there is a replacement.

    Maybe #28578 (fuzz: add target for DescriptorScriptPubKeyMan by brunoerg) could be reviewed, and confirmed that it can also catch the regression, or be adjusted to catch the regression?

  14. dergoegge commented at 3:08 pm on November 16, 2023: member

    Maybe #28578 (fuzz: add target for DescriptorScriptPubKeyMan by brunoerg) could be reviewed, and confirmed that it can also catch the regression, or be adjusted to catch the regression?

    What was the regression? The initial PR #23334 does not mention it.

  15. maflcko commented at 3:13 pm on November 16, 2023: member
    Ah, looks like it was never merged https://github.com/bitcoin/bitcoin/pull/23444
  16. dergoegge commented at 3:15 pm on November 16, 2023: member

    Ah, looks like it was never merged #23444

    Soooo, should we delete it and then someone can re-introduce with #23444 applied + perf improvements?

  17. dergoegge commented at 4:45 pm on November 20, 2023: member
    🤷
  18. dergoegge closed this on Nov 20, 2023
  19. maflcko commented at 1:57 pm on November 24, 2023: member
    See https://github.com/bitcoin/bitcoin/pull/28933
  20. maflcko commented at 2:45 pm on November 29, 2023: member

    On oss-fuzz for example, it never even starts fuzzing because it times-out while running through the existing corpus:

    Is there a link available, so that others can look up the information for themselves?

  21. bitcoin locked this on Nov 28, 2024


dergoegge DrahtBot maflcko brunoerg

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: 2025-05-25 21:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me