Skip to content

[WIP] wallet: use BlockFilterIndex in ScanForWalletTransactions #19116

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 4 commits into from
Closed

[WIP] wallet: use BlockFilterIndex in ScanForWalletTransactions #19116

wants to merge 4 commits into from

Conversation

pstratem
Copy link
Contributor

goal is to improve rescan performance significantly, lots more work to do

@pstratem
wallet: add ScriptPubKeyMan::GetScriptPubKeys virtual function
55c9ced 
LegacyScriptPubKey::GetScriptPubKeys is a stub
@pstratem pstratem force-pushed the 2020-05-29-generate-pubkeys branch from facceac to 55c9ced Compare May 30, 2020 22:30
@maflcko
Copy link
Member

maflcko commented May 30, 2020

Have you seen #15845 ? It might be easier to take that code and then fix up the two (or three) bugs.

@maflcko
Copy link
Member

maflcko commented May 30, 2020

Concept ACK either way

@pstratem
Copy link
Contributor Author

@MarcoFalke I hadn't, it seems you took basically the same approach that I intend to. I'll certainly review your PR and read the comments.

@DrahtBot
Copy link
Contributor

DrahtBot commented May 31, 2020 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

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 mentioned this pull request May 31, 2020
Merged
@pstratem
Copy link
Contributor Author

pstratem commented Jun 1, 2020

this actually works for p2pkh transactions as it is now, benchmarks incoming

@pstratem pstratem force-pushed the 2020-05-29-generate-pubkeys branch 2 times, most recently from 4f96f9e to a1d1bc6 Compare June 1, 2020 17:51
This was referenced Jun 2, 2020
Merged
Closed
Draft
@pstratem
wallet: Use blockfilterindex "basic" to accelerate rescan time
fe107fc 
we avoid issues with the keypool expanding during a rescan with the
script_pub_key_filter_set flag
@pstratem pstratem force-pushed the 2020-05-29-generate-pubkeys branch from a1d1bc6 to fe107fc Compare June 2, 2020 22:10
@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 2, 2020

🐙 This pull request conflicts with the target branch and needs rebase.

@pstratem
Copy link
Contributor Author

pstratem commented Jun 6, 2020

some rescanblockchain benchmarks, from my laptop:

42 minutes on master
20 minutes using the BASIC filter
4 minutes using a filter with fixed siphash parameters and avoiding recalculating the siphashes for every block

@pstratem pstratem marked this pull request as draft June 7, 2020 00:26
@jonatack
Copy link
Member

jonatack commented Jun 7, 2020

Concept ACK

ryanofsky
ryanofsky reviewed Jun 30, 2020
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

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

Concept & approach ACK. Seems to pick up nicely where #15845 left off, updated to work for descriptor and legacy wallets

@pstratem pstratem mentioned this pull request Sep 9, 2020
Closed
@jb55
Copy link
Contributor

jb55 commented Nov 26, 2020

Concept ACK as I grumpily wait while my wallets are rescanning

fanquake
fanquake reviewed Mar 15, 2021
View reviewed changes
src/interfaces/chain.cpp
@@ -186,6 +187,20 @@ class ChainImpl : public Chain
}
return nullopt;
}
Optional<bool> filterMatchesAny(const uint256& hash, const GCSFilter::ElementSet& filter_set) override
Copy link
Member

Choose a reason for hiding this comment

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

Please use std::optional in new code.

@DrahtBot
Copy link
Contributor

There hasn't been much activity lately and the patch still needs rebase. What is the status here?
  • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
  • Is it no longer relevant? ➡️ Please close.
  • Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

@maflcko maflcko mentioned this pull request Dec 15, 2021
Closed
4 tasks
@DrahtBot
Copy link
Contributor

There hasn't been much activity lately and the patch still needs rebase. What is the status here?
  • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
  • Is it no longer relevant? ➡️ Please close.
  • Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

@maflcko maflcko closed this Mar 21, 2022
@maflcko
Copy link
Member

maflcko commented Mar 21, 2022

Marked up for grabs, along with #15845

@maflcko
Copy link
Member

maflcko commented Sep 8, 2022

Picked up in #25957

@bitcoin bitcoin locked and limited conversation to collaborators Sep 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@fanquake fanquake fanquake left review comments

@ryanofsky ryanofsky ryanofsky left review comments

Assignees
No one assigned
Labels
Needs rebase Wallet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@pstratem @maflcko @DrahtBot @jonatack @jb55 @fanquake @ryanofsky