refactor(chain)! SyncRequest ergonomics improvements#1528
Closed
LLFourn wants to merge 1 commit intobitcoindevkit:masterfrom
Closed
refactor(chain)! SyncRequest ergonomics improvements#1528LLFourn wants to merge 1 commit intobitcoindevkit:masterfrom
LLFourn wants to merge 1 commit intobitcoindevkit:masterfrom
Conversation
WIP esplora done but electrum not done
Merged
5 tasks
Member
|
Replaced by #1478 |
notmandatory
added a commit
that referenced
this pull request
Aug 14, 2024
…ullScanRequest`/`SyncRequest` structures 6d77e2e refactor(chain)!: Rename `spks_with_labels` to `spks_with_indices` (志宇) 584b10a docs(esplora): README example, uncomment async import (志宇) 3eb5dd1 fix(chain): correct `Iterator::size_hint` impl (志宇) 96023c0 docs(chain): improve `SyncRequestBuilder::spks_with_labels` docs (志宇) 0234f70 docs(esplora): Fix typo (志宇) 38f86fe fix: no premature collect (志宇) 44e2a79 feat!: rework `FullScanRequest` and `SyncRequest` (志宇) 16c1c2c docs(esplora): Simplify crate docs (志宇) c93e6fd feat(esplora): always fetch prevouts (志宇) cad3533 feat(esplora): make ext traits more flexible (志宇) Pull request description: Closes #1528 ### Description Some users use `bdk_esplora` to update `bdk_chain` structures *without* a `LocalChain`. ~~This PR introduces "low-level" methods to `EsploraExt` and `EsploraAsyncExt` which populates a `TxGraph` update with associated data.~~ We change `FullScanRequest`/`SyncRequest` to take in the `chain_tip` parameter as an option. Spk-based chain sources (`bdk_electrum` and `bdk_esplora`) will not fetch a chain-update if `chain_tip` is `None`, allowing callers to opt-out of receiving updates for `LocalChain`. We change `FullScanRequest`/`SyncRequest` to have better ergonomics when inspecting the progress of syncing (refer to #1528). We change `FullScanRequest`/`SyncRequest` to be constructed with a builder pattern. This is a better API since we separate request-construction and request-consumption. Additionally, much of the `bdk_esplora` logic has been made more efficient (less calls to Esplora) by utilizing the `/tx/:txid` endpoint (`get_tx_info` method). This method returns the tx and tx_status in one call. The logic for fetching updates for outpoints has been reworked to support parallelism. Documentation has also been updated. ### Notes to reviewers This PR has evolved somewhat. Initially, we were adding more methods on `EsploraExt`/`EsploraAsyncExt` to make syncing/scanning more modular. However, it turns out we can just make the `chain_tip` parameter of the request structures optional. Since we are changing the request structures, we might as well go further and improve the ergonomics of the whole structures (refer to #1528). This is where we are at with this PR. Unfortunately, the changes are now breaking. I think this is an okay tradeoff for the API improvements (before we get to stable). ### Changelog notice * Change request structures in `bdk_chain::spk_client` to be constructed via a builder pattern, make providing a `chain_tip` optional, and have better ergonomics for inspecting progress while syncing. * Change `bdk_esplora` to be more efficient by reducing the number of calls via the `/tx/:txid` endpoint. The logic for fetching outpoint updates has been reworked to support parallelism. * Change `bdk_esplora` to always add prev-txouts to the `TxGraph` update. ### 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: * [ ] I've added tests for the new feature * [x] I've added docs for the new feature ACKs for top commit: ValuedMammal: ACK 6d77e2e notmandatory: ACK 6d77e2e Tree-SHA512: 806cb159a8801f4e33846d18e6053b65d105e452b0f3f9d639b0c3f2e48fb665e632898bf42977901526834587223b0d7ec7ba1f73bb796b5fd8fe91e6f287f7
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is an attempt to improve the ergonomics of sync request especially with regards to inspecting the progress of the sync.
Now there is a single
inspectmethod which takes a closure which gets richer information than the previous ones. It takes:where the
usizes are the index of the current item and the number of items remaining respectively.SyncItemis defined as:Note carefully the type parameter
Iinside which is just to help log out the index (or whatever metadata you want) along with the spk.This is not ready for review but I thought @evanlinjin might be interested in finishing this work.