Skip to content

fuzz: improve scriptpubkeyman target#30563

Merged
achow101 merged 1 commit intobitcoin:masterfrom
brunoerg:2024-07-fuzz-spkm
Aug 12, 2024
Merged

fuzz: improve scriptpubkeyman target#30563
achow101 merged 1 commit intobitcoin:masterfrom
brunoerg:2024-07-fuzz-spkm

Conversation

@brunoerg
Copy link
Copy Markdown
Contributor

Fixes #30541

This PR aims to improve scriptpubkeyman target to avoid timeouts. The input provided in #30541 takes too much time to run because it basically calls only MarkUnusedAddresses (300 times * number of spks). The following changes were made to improve it:

  • Reduce keypool size.
  • When calling MarkUnusedAddresses, do it with one of the spks per iteration.
  • Remove the specific AddDescriptorKey call since it is already covered with AddWalletDescriptor.
  • Limit number of iterations to a reasonable value.

@DrahtBot
Copy link
Copy Markdown
Contributor

DrahtBot commented Jul 31, 2024

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 maflcko, achow101

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:

  • #30221 (wallet: Ensure best block matches wallet scan state by achow101)
  • #28333 (wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively by achow101)

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 Jul 31, 2024
@brunoerg
Copy link
Copy Markdown
Contributor Author

cc: @maflcko

Copy link
Copy Markdown
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

calls only MarkUnusedAddresses (300 times * number of spks)

Are they different spks? IIRC when I looked, they looked all identical, which I am not sure is working as intended.

@brunoerg brunoerg force-pushed the 2024-07-fuzz-spkm branch from 4875808 to fd7cd3b Compare August 1, 2024 12:57
@brunoerg
Copy link
Copy Markdown
Contributor Author

brunoerg commented Aug 1, 2024

Are they different spks? IIRC when I looked, they looked all identical, which I am not sure is working as intended.

For that input, it was calling MarkUnusedAddresses with all spks every iteration. Besides that, every time MarkUnusedAddresses is called, the number of spks increases. That's why it takes a lot time to run.

Now, it will just call MarkUnusedAddresses with just one spk (randomly) per iteration. Besides that, the number of iterations was reduced for a reasonable value.

Copy link
Copy Markdown
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

lgtm ACK fd7cd3b8b24ad86dae5c16addbb5acafa18efc0a

The goal of this improvement is to reduce
TopUp calls which can lead to timeouts.
@brunoerg brunoerg force-pushed the 2024-07-fuzz-spkm branch from fd7cd3b to 401cc4e Compare August 1, 2024 14:11
@brunoerg
Copy link
Copy Markdown
Contributor Author

brunoerg commented Aug 1, 2024

Force-pushed addressing #30563 (comment)

@maflcko
Copy link
Copy Markdown
Member

maflcko commented Aug 1, 2024

lgtm ACK 401cc4e

@achow101
Copy link
Copy Markdown
Member

ACK 401cc4e

@achow101 achow101 merged commit 5c5a298 into bitcoin:master Aug 12, 2024
@bitcoin bitcoin locked and limited conversation to collaborators Aug 12, 2025
@brunoerg brunoerg deleted the 2024-07-fuzz-spkm branch December 29, 2025 13:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

scriptpubkeyman fuzz target TopUp is slow (2/N)

4 participants