-
Notifications
You must be signed in to change notification settings - Fork 393
feat(electrum): optimize merkle proof validation with batching #1957
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
base: master
Are you sure you want to change the base?
Conversation
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.
Thanks for moving this forward.
This is not a full review, but I think it's enough to push this PR in a good direction.
// Batch validate all collected transactions. | ||
if !txs_to_validate.is_empty() { | ||
let proofs = self.batch_fetch_merkle_proofs(&txs_to_validate)?; | ||
self.batch_validate_merkle_proofs(tx_update, proofs)?; | ||
} |
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.
Instead of having every populate_with_{}
method call this internally, it will be more efficient and make more logical sense if we extract this so that we only call it at the end of full_scan
and sync
.
In other words, populate_with_{}
should no longer fetch anchors. Instead, they should either mutate, or return a list of (Txid, BlockId)
for which we try to fetch anchors for in a separate step.
It will be even better if full txs are fetched in a separate step too.
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.
Partially resolved. This is the next TODO:
It will be even better if full txs are fetched in a separate step too.
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 will likely be included in a separate PR.
Fetching all full txs in a batch call at the beginning of sync
actually ended up doubling sync
time.
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.
@LagginTimes did you figure out why though?
d69907b
to
149807c
Compare
dc08959
to
bf38a8e
Compare
@LagginTimes could you provide the benchmark results in the PR description and compare it to results before the changes in this PR? |
Based on above benchmark results it looks like this change is 1s faster on sync, is that due to a small test size? Do we expect it to make more of a difference with wallets with many addresses? |
de14241
to
bb26525
Compare
@LagginTimes Edit: how about we just test with the example-cli with a pre-populated signet wallet? I suggested writing benchmarks with the assumption that local io (against testenv) would be slower than allocating memory (collecting requests into vec before batch requesting), however that assumption seems incorrect. |
90a0018
to
591b51a
Compare
591b51a
to
70495e2
Compare
99fdd57
to
e4191f9
Compare
e4191f9
to
93f15e7
Compare
93f15e7
to
65e8a08
Compare
Replaces #1908, originally authored by @Keerthi421.
Fixes #1891.
Description
This PR optimizes
sync
/full_scan
performance by batching and caching key RPC calls to slash network round-trips and eliminate redundant work.Key improvements:
blockchain.transaction.get_merkle
calls into a singlebatch_call
request.batch_script_get_history
instead of many individualscript_get_history
calls.batch_block_header
to fetch all needed block headers in one call rather than repeatedly callingblock_header
.Anchor Caching Performance Improvements
Results suggest a significant speed up with a warmed up cache. Tested on local Electrum server with:
Results before this PR (https://github.com/LagginTimes/bdk/tree/1957-master-branch):
Results after this PR:
Batch Call Performance Improvements
No persisted data was carried over between runs, so each test started with cold caches and measured only raw batching performance. Tested with
example_electrum
out of https://github.com/LagginTimes/bdk/tree/example_electrum_timing with the following parameters:Results before this PR:
Results after this PR (using this PR's
bdk_electrum_client.rs
):Changelog notice
Checklists
All Submissions:
cargo fmt
andcargo clippy
before committingNew Features:
Bugfixes: