Skip to content

fuzz: Faster wallet_notifications target #28933

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
Merged
fanquake merged 2 commits into from
Nov 27, 2023
Merged

fuzz: Faster wallet_notifications target #28933

fanquake merged 2 commits into from
Nov 27, 2023

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Nov 24, 2023

Avoid read/write from storage to speed the target up.

Export assert from util/check.h
fa971c0 
This avoids having to include both headers when assert and Assert are
used at the same time.
@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 24, 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
ACK dergoegge, brunoerg

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@DrahtBot DrahtBot added the Tests label Nov 24, 2023
@maflcko maflcko force-pushed the 2311-fuzz-wallet-notif- branch from fa092cc to fa4fc99 Compare November 24, 2023 13:01
dergoegge
dergoegge approved these changes Nov 24, 2023
View reviewed changes
Copy link
Member

@dergoegge dergoegge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK fa4fc99b6e2a13736521e6bca1f626ba1d2f59e3

With the mocked database this shouldn't be going to disk at all now, right?

@maflcko
Copy link
Member Author

maflcko commented Nov 24, 2023

With the mocked database this shouldn't be going to disk at all now, right?

Yeah, I guess so. (A datadir is still created, though)

@maflcko maflcko mentioned this pull request Nov 24, 2023
Closed
@dergoegge
Copy link
Member

dergoegge commented Nov 24, 2023

A datadir is still created, though

Is that necessary or just a side effect from using TestingSetup?

@brunoerg
Copy link
Contributor

brunoerg commented Nov 24, 2023 •
edited
Loading

Concept ACK

will review soon!

@maflcko
Copy link
Member Author

maflcko commented Nov 24, 2023

Is that necessary or just a side effect from using TestingSetup?

Both. For one, validation can not exist without datadir. Also, it is created in TestingSetup. But it shouldn't cause any issues, because it will be empty.

@dergoegge
Copy link
Member

dergoegge commented Nov 24, 2023

But it shouldn't cause any issues, because it will be empty.

Empty dirs aren't of size zero and #28811 would still apply (given more timeouts/crashes).

It's a bit of a shame if it's always empty to still create it, but not an issue for this PR in any case.

@maflcko
Copy link
Member Author

maflcko commented Nov 24, 2023

Are there any timeouts left at this point? If yes, it may be good to report them.

@dergoegge
Copy link
Member

dergoegge commented Nov 24, 2023

Are there any timeouts left at this point? If yes, it may be good to report them.

We'll see I guess, initial exec/s looked good but they have now deteriorated to 3 exec/s (per core). No timeouts so far though and CPU utilization is at 100% on all cores, so this is a definite improvement.

@brunoerg
Copy link
Contributor

brunoerg commented Nov 24, 2023 •
edited
Loading

Is there any benefit about using this ImportDescriptor approach compared to simply use SetupDescriptorScriptPubKeyMans?

@maflcko
Copy link
Member Author

maflcko commented Nov 24, 2023

SetupScriptPubKeyMans

Which function do you mean?

$ git grep SetupScriptPubKeyMans | wc -l
0

@brunoerg
Copy link
Contributor

brunoerg commented Nov 24, 2023

Which function do you mean?

Sorry, edited that. It's SetupDescriptorScriptPubKeyMans.

brunoerg
brunoerg approved these changes Nov 24, 2023
View reviewed changes
Copy link
Contributor

@brunoerg brunoerg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fa4fc99b6e2a13736521e6bca1f626ba1d2f59e3

@dergoegge
Copy link
Member

dergoegge commented Nov 27, 2023 •
edited
Loading

I've run into timeouts.

e.g. this input takes >5s to execute on my machine. 
QV9BKr6un2BgAAD/S6+fn2CAYJ9gn5+f3wCABP+fn2Bg/489AGCf8AAAJn8AAAB/AJ9kYJ+fk0FB
QQAZAADBvkFEIiT5////IiIjIiIgvF1G/90AAOR/D/+7QUFBPb1hvkFBV0G+Qfi+4IJBQQcjByIg
jf4iIiISDwAAAAB///+7VBC1RCIk+f///yIiIyIiILxdRv/dAADkfw//u5gjRLW73dsG//8gIiIj
IgAAQeDh4eHh4eEtLUEtLQMaGgMeKSopKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSEpKSkpKSkp
KSkpKRgpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkp
KSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKTkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkp
KSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSlFKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkp
KSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKdbWKSkpKSkpKSkpKSkpKSkpKSkpKSkp
KSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkp
yBID/wAC0QMDQQNBAQBBTmRCKSkpKSkpKSkpKSkpKSkpKSkpKSkpKTIpKSkpKSkpKSkpKSkpKSkp
KSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkp
KSkpKSkpKSkpKSkpKSkpKSkpHykpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSki
KSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKQ4pKSkpKSkpKSkpKSkpKSkp
KSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpDikpKSkpKSkpKSkpKSkpKSlLKSkpKSkpKSkpKSkpKSkp
KSkpKSkpKSkpyBID/wAC0QMDQQNBAQBBTkFCCgMbAwMEABQDABADZAMDA0EAAAMDAwMKAx1DQ0Nm
f34mQ0FkAwMDQQAAAwMDA0ED738AAEEDAuETGxTBwcHBwcHBwcHBwcHBwcHBwcEDABADQgoDZAP/
/wNBDQMDKSkpKSkpKSkpKSkpLikpKSkpCikpKX//KSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkp
KSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkp
KSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpSSkpKSkpKSkpCCkpKSkpKSkpKSkpKSkp
KSkpKSkpKSkpKUQpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKS8pKSkpKSkpKSkpKSkpKSopKSkp
KSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkp
KSkpKSkpKSkpKSkpKSkpKSkpKSkpKTkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKX8pKWkpKSkp
KSkpKSkpKSkpKSkpGCkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkp
KSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkp
KdbWKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSopKSkpKSkpKSkpKSkpKSkp
SSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkp
KSkpKSkpKSkpKSkpKSkpKSkpKSopKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkp
KSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkp
KSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkp
KSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkp
KSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkp
KSkpKSkpKSkpKdYpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkp
KSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSlCKSkpKSkpKSkpKSkpKSkpKSkpKSkp
KSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkp
KSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkp
KSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkp
KSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKcgS/AAAAtEDA0EDQQEAQU5k
QgoDGwMDBAAUAwAQA2QDAwNBAAADAwMDCnsdQ0NDZn9+JkNBZAMDA0EAAAMDAwNBA/9/AABBAwLh
EwMUwcHBwcHBwcHBwcHBwcHBwcHBAwAQA0IKA5QD//8DQQ0DAykpKSkpKSkQKSkpKSkpKSkpKSkp
KSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkp
KSkpKSkpKSkpKSkpKSkpKSkpSSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkp
KSkpKSkpDEkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkp
KSkpKSkpfwAAACkpKSkpKSkpKSkpKSkpKSkqKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkp
KSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkp
KSkpKSkpKSkpKSkpKSkpKSkpKSkpKZUpKSkpKSkpKSnW1ikpKSkpKSkpKRgpKSkpKSkpKSkpKSkp
KSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKKkpKSkpKSkpKSkpKSkp
KSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkaAwPhqwAAAwMDAwP/5+fn5+fn

Afl++ stability also dropped to ~70% which suggests that there is some non-determinism in the target.

@maflcko
Copy link
Member Author

maflcko commented Nov 27, 2023

Afl++ stability also dropped to ~70% which suggests that there is some non-determinism in the target.

This is expected, because the wallet internally uses system random, similar to the p2p fuzz targets. This should be fixed in a follow-up for all fuzz targets.

@maflcko maflcko force-pushed the 2311-fuzz-wallet-notif- branch from fa4fc99 to fa15861 Compare November 27, 2023 11:06
@maflcko
Copy link
Member Author

maflcko commented Nov 27, 2023

I've run into timeouts.

Thanks, fixed by reducing the number of new addresses that are generated each run

dergoegge
dergoegge approved these changes Nov 27, 2023
View reviewed changes
Copy link
Member

@dergoegge dergoegge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reACK fa15861

@DrahtBot DrahtBot requested a review from brunoerg November 27, 2023 11:46
brunoerg
brunoerg approved these changes Nov 27, 2023
View reviewed changes
Copy link
Contributor

@brunoerg brunoerg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reACK fa15861

@fanquake fanquake merged commit 794f971 into bitcoin:master Nov 27, 2023
@maflcko maflcko deleted the 2311-fuzz-wallet-notif- branch November 28, 2023 07:39
@maflcko maflcko mentioned this pull request Nov 29, 2023
Closed
@dergoegge dergoegge mentioned this pull request Feb 19, 2024
Merged
@maflcko maflcko mentioned this pull request Apr 17, 2024
Open
19 tasks
@bitcoin bitcoin locked and limited conversation to collaborators Nov 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@dergoegge dergoegge dergoegge approved these changes

@brunoerg brunoerg brunoerg approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@maflcko @DrahtBot @dergoegge @brunoerg @fanquake