Electrum: optimize merkle proof validation with batching #1908
Conversation
|
Concept ACK In 3ef0c83: I don't think |
| /// The header cache | ||
| block_header_cache: Mutex<HashMap<u32, Header>>, | ||
| /// The Merkle proof cache | ||
| merkle_cache: Mutex<HashMap<(Txid, u32), GetMerkleRes>>, |
There was a problem hiding this comment.
Let's say a tx is confirmed in block of height 100 and hash A. We then cache it's merkle proof. Then a reorg happens and the tx is now in block of hash B (but still height 100).
With the current implementation, we will never fetch the merkle proof for block B because we already have a merkle proof cached for a txid at the same height as block B.
Is there a way to cache merkle proofs against (txid, blockhash)?
I apologize in advance for how bad the Electrum API is designed.
There was a problem hiding this comment.
@evanlinjin , @notmandatory Please Review the changes made
There was a problem hiding this comment.
@Keerthi421 thank you for your attempt but you have not addressed the problem.
See if a transaction with txid get's confirmed in a block of height:hash 100:A and then we do a sync. We will have an entry: (txid, 100) : merkle_res_for_txid_in_A.
If there is a reorg, and the block at height 100 is now A', the entry will stay as it is and we will never fetch an anchor for "txid in A'". This will mean that this transaction will appear to never confirm on the wallet (until we restart and clear the cache).
|
@notmandatory please review the changes |
@LagginTimes is a better person to review and he said he's taking a look at this PR. |
|
Here are my fixes and suggestions based on your work so far LagginTimes@412d800: 1. Cache keyed by
|
|
@Keerthi421 thank you for this work. @LagginTimes will be taking over since it'll be good to have this soon. @LagginTimes please make a new PR. |
…tching 156cbab test(electrum): Improve benchmark (志宇) 4ea5ea6 feat(electrum): batch `transaction.get_merkle` calls via `batch_call` (Wei Chen) ec4fd97 feat(electrum): batched `Header`s and `script_get_history` (Wei Chen) f21a21d test(electrum): add `criterion` benchmark for `sync` (Wei Chen) b57768d fix(electrum): improve tx validation and gap limit scanning (keerthi) 7a18cad feat(electrum): optimize merkle proof validation with batching (Wei Chen) Pull request description: 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: * Gather all `blockchain.transaction.get_merkle` calls into a single `batch_call` request. * Use `batch_script_get_history` instead of many individual `script_get_history` calls. * Use `batch_block_header` to fetch all needed block headers in one call rather than repeatedly calling `block_header`. * Introduce a cache of transaction anchors to skip re-validating already confirmed transactions. #### Anchor Caching Performance Improvements Results suggest a significant speed up with a warmed up cache. Tested on local Electrum server with: ``` $ cargo bench -p bdk_electrum --bench test_sync ``` Results before this PR (https://github.com/LagginTimes/bdk/tree/1957-master-branch): ``` sync_with_electrum time: [1.3702 s 1.3732 s 1.3852 s] ``` Results after this PR: ``` sync_with_electrum time: [851.31 ms 853.26 ms 856.23 ms] ``` #### 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: ``` $ example_electrum init "tr([62f3f3af/86'/1'/0']tpubDD4Kse29e47rSP5paSuNPhWnGMcdEDAuiG42LEd5yaRDN2CFApWiLTAzxQSLS7MpvxrpxvRJBVcjhVPRk7gec4iWfwvLrEhns1LA4h7i3c2/0/*)#cn4sudyq" $ example_electrum scan tcp://signet-electrumx.wakiyamap.dev:50001 ``` Results before this PR: ``` FULL_SCAN TIME: 8.145874476s ``` Results after this PR (using this PR's [`bdk_electrum_client.rs`](https://github.com/bitcoindevkit/bdk/blob/70495e2010541acbb5d62f9b5692de20924ac53f/crates/electrum/src/bdk_electrum_client.rs)): ``` FULL_SCAN TIME: 2.594050112s ``` ### Changelog notice * Add transaction anchor cache to prevent redundant network calls. * Batch Merkle proof, script history, and header requests. ### Checklists #### All Submissions: * [x] I've signed all my commits * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md) * [x] I ran `cargo fmt` and `cargo clippy` before committing #### New Features: * [x] I've added tests for the new feature * [x] I've added docs for the new feature #### Bugfixes: * [ ] This pull request breaks the existing API * [x] I've added tests to reproduce the issue which are now passing * [x] I'm linking the issue being fixed by this PR ACKs for top commit: oleonardolima: tACK 156cbab evanlinjin: ACK 156cbab Tree-SHA512: dc7dc1d7de938223cc03293d8bb8ae12c8799c7ec8ba8c7faec5cf2076c96a1b1e50b406cbcc90cbd6cbe7a311c0c11dd036691c03ed067c469a26260903993b
5 participants
This PR optimizes the Electrum client's performance by improving Merkle proof validation, addressing the significant performance regression in BDK 1.1.0 where full sync time increased from 4s to 26s.
Key improvements:
Notes to the reviewers
The optimization approach focuses on three main areas:
The batch size is set to 100 as a balance between performance and memory usage. This value can be adjusted based on testing results.
Changelog notice
Added
Solves issue Electrum client Performance issues #1891