-
Notifications
You must be signed in to change notification settings - Fork 586
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.
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
chain: use neutrino filters to speed up bitcoind seed recovery #889
base: master
Are you sure you want to change the base?
chain: use neutrino filters to speed up bitcoind seed recovery #889
Conversation
6c9113e
to
a0b0db1
Compare
We also abstract how blocks are fetched in the first place, as bitcoind uses a different name for the RPC to fetch filters.
In this commit, we use neutrino filters to speed up bitcoind seed recovery. We use the recently created `maybeShouldFetchBlock` function to check the filters to see if we need to fetch a block at all. This saves us from fetching, decoding, then scanning the block contents if we know nothing is present in them. At this point, we can also further consolidate the `FilterBlocks` methods between the two backends, as they're now identical.
a0b0db1
to
2cb1df2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a couple of fixes. Was able to get it running with a small patch and can confirm this massively speeds up scanning blocks.
My quick benchmark, scanning 2000 blocks took:
Without this PR: ~1m 44s
With this PR: ~26s
@@ -197,6 +197,65 @@ func (c *RPCClient) BlockStamp() (*waddrmgr.BlockStamp, error) { | |||
} | |||
} | |||
|
|||
// fetchBlockFilter fetches the GCS filter for a block from the remote node. | |||
func (c *RPCClient) fetchBlockFilter(blkHash chainhash.Hash, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I still can't get over this formatting tbh... Do you really prefer this over:
// fetchBlockFilter fetches the GCS filter for a block from the remote node.
func (c *RPCClient) fetchBlockFilter(
blkHash chainhash.Hash(*gcs.Filter, error) {
?
} | ||
|
||
resp, err := c.chainConn.client.RawRequest( | ||
bitcoindFilterRPC, []json.RawMessage{jsonFilterReq}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This threw an error for me, looks like we need to JSON encode each field individually, not the whole message. This worked for me:
hash, err := json.Marshal(blkHash.String())
if err != nil {
return nil, fmt.Errorf("cannot marshal hash: %w", err)
}
filterType, err := json.Marshal(bitcoindFilterType)
if err != nil {
return nil, fmt.Errorf("cannot marshal hash: %w", err)
}
resp, err := c.chainConn.client.RawRequest(
bitcoindFilterRPC, []json.RawMessage{hash, filterType},
)
if err != nil {
return nil, fmt.Errorf("cannot send request: %w", err)
}
// block, then we don't need to fetch it, as there're no false | ||
// negatives. | ||
if !shouldFetchBlock { | ||
log.Infof("Skipping block height=%d hash=%v, no "+ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very spammy. Maybe demote to debug?
continue | ||
} | ||
|
||
log.Infof("Fetching block height=%d hash=%v", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is quite useful to debug! I synced my mainnet node and found it very interesting that we seem to fetch around 90 blocks per 2000 block scanning batch. Is it possible that the filters have that high of a false positive rate? Because I used an xprv
I didn't have a birthday block encoded and started at height 481596 (segwit activation), where my wallet definitely didn't have any transactions yet...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is a property of the filters themselves or whether we can optimize something with how we construct the filter matcher on our side? Or maybe we have a weird value (e.g. all zero) that skews the matcher into more false positives?
// | ||
// The getblockfilter call was added in version 19.0.0, so we return | ||
// for versions >= 190000. | ||
return info.Version >= 190000, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This just gives us whether bitcoind
is new enough to have filters. But don't we also need to detect whether they are enabled? Since on bitcoind
they aren't turned on by default (are they in btcd
?) as far as I know.
I just tried this locally with the index disabled and got: -1: Index is not enabled for filtertype basic
In this commit, we use neutrino filters to speed up bitcoind seed
recovery. We use the recently created
maybeShouldFetchBlock
functionto check the filters to see if we need to fetch a block at all. This
saves us from fetching, decoding, then scanning the block contents if we
know nothing is present in them.
At this point, we can also further consolidate the
FilterBlocks
methods between the two backends, as they're now identical.
Another follow up here would be to start to prefetch both filters and blocks. We know that we'll need to fetch every filter, so we can fetch them all in a single RPC request (batch call). Once we have the filters, we can sort of check them all in parallel, but need to mind the
FoundOutPoints
field, as once we have something like a change addr we know of, we want to then watch that for subsequent spends.