Skip to content

fuzz: Delete wallet_notifications #28882

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

fuzz: Delete wallet_notifications #28882

wants to merge 1 commit into from

Conversation

dergoegge
Copy link
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:

Component revisions (build r202311100604):
Bitcoin-core: b3898e946cf81e2e7b573e1c5204bd29af2feecd
Botan: 201cfe586e6d529360fbcde6f216f1da0c3db48f
Cryptofuzz: 537263836eae2cf6174689719a1dcfd92fb875c7
Secp256k1: c891c5c2f41912607f5c551d354c84bb4439e751
Trezor-firmware: ebeea4a209c7c9ebb885d8d7727c9e6e2bfe2bc0
Wycheproof: b063b4aedae951c69df014cd25fa6d69ae9e8cb9

Bot name: oss-fuzz-linux-zone1-host-2dwk-5
Return code: 1

Command: /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
Time ran: 6610.037380456924

INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 3511012882
INFO: Loaded 1 modules   (1703626 inline 8-bit counters): 1703626 [0x5ad2b8073180, 0x5ad2b821304a),
INFO: Loaded 1 PC tables (1703626 PCs): 1703626 [0x5ad2b8213050,0x5ad2b9c11cf0),
INFO:        0 files found in /mnt/scratch0/clusterfuzz/bot/inputs/fuzzer-testcases-disk/temp-359/new
INFO:     1049 files found in /mnt/scratch0/clusterfuzz/bot/inputs/data-bundles/bitcoin-core_wallet_notifications
INFO: seed corpus: files: 1049 min: 1b max: 4194302b total: 19233546b rss: 143Mb
#2	pulse  ft: 20459 exec/s: 0 rss: 143Mb
#4	pulse  cov: 10609 ft: 24082 corp: 2/2b exec/s: 0 rss: 144Mb
#8	pulse  cov: 10650 ft: 26458 corp: 6/6b exec/s: 0 rss: 144Mb
#16	pulse  cov: 10651 ft: 28496 corp: 14/14b exec/s: 0 rss: 144Mb
#32	pulse  cov: 10656 ft: 30865 corp: 30/30b exec/s: 0 rss: 144Mb
Slowest unit: 10 s:
artifact_prefix='/mnt/scratch0/clusterfuzz/bot/inputs/fuzzer-testcases/'; Test unit written to /mnt/scratch0/clusterfuzz/bot/inputs/fuzzer-testcases/slow-unit-6dcd4ce23d88e2ee9568ba546c007c63d9131c1b
Base64: QQ==
#64	pulse  cov: 10660 ft: 33168 corp: 62/62b exec/s: 0 rss: 144Mb
#128	pulse  cov: 10660 ft: 34990 corp: 126/129b exec/s: 0 rss: 144Mb
#256	pulse  cov: 12470 ft: 40927 corp: 254/722b exec/s: 0 rss: 145Mb
#512	pulse  cov: 13145 ft: 49807 corp: 509/4070b exec/s: 0 rss: 145Mb
cf::fuzzing_strategies: random_max_len:1,value_profile:1,extra_sanitizers:1

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 15, 2023 •
edited
Loading

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.

@DrahtBot DrahtBot added the Tests label Nov 15, 2023
@dergoegge dergoegge force-pushed the 2023-11-fuzz-nuke-wn branch from c2966e8 to 6367805 Compare November 15, 2023 14:18
@maflcko
Copy link
Member

maflcko commented Nov 15, 2023

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.

@brunoerg
Copy link
Contributor

Concept ACK

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

@maflcko
Copy link
Member

maflcko commented Nov 15, 2023

I believe it is not deterministic.

Can you add steps to reproduce?

@brunoerg
Copy link
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.

// Make a seed
CKey seed_key;
seed_key.MakeNewKey(true);
CPubKey seed = seed_key.GetPubKey();
assert(seed_key.VerifyPubKey(seed));

// Get the extended key
CExtKey master_key;
master_key.SetSeed(seed_key);

SetupDescriptorScriptPubKeyMans(master_key);

This was referenced Nov 15, 2023
Merged
Merged
Open
@dergoegge
Copy link
Member Author

I am running it on my servers and I agree it should be improved.

@maflcko thoughts on deleting it?

@maflcko
Copy link
Member

maflcko commented Nov 16, 2023

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?

@dergoegge
Copy link
Member Author

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.

@maflcko
Copy link
Member

maflcko commented Nov 16, 2023

Ah, looks like it was never merged #23444

@dergoegge
Copy link
Member Author

Ah, looks like it was never merged #23444

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

@dergoegge
Copy link
Member Author

🤷

@dergoegge dergoegge closed this Nov 20, 2023
@maflcko
Copy link
Member

maflcko commented Nov 24, 2023

See #28933

@maflcko
Copy link
Member

maflcko commented Nov 29, 2023

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?

@bitcoin bitcoin locked and limited conversation to collaborators Nov 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dergoegge @DrahtBot @maflcko @brunoerg