Skip to content
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

refactor(chain)! SyncRequest ergonomics improvements #1528

Closed

Conversation

LLFourn
Copy link
Contributor

@LLFourn LLFourn commented Jul 30, 2024

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 inspect method which takes a closure which gets richer information than the previous ones. It takes:

(SyncItem<'a, I>, usize, usize)

where the usizes are the index of the current item and the number of items remaining respectively.
SyncItem is defined as:

/// An item reported to [`inspect`]
///
/// [`inspect`]: SyncRequest::inspect
pub enum SyncItem<'a, I> {
    /// Script pubkey sycn item
    Spk(I, &'a Script),
    /// Transaction id sync item
    Txid(Txid),
    /// Transaction outpoint sync item
    OutPoint(OutPoint),
}

Note carefully the type parameter I inside 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.

WIP esplora done but electrum not done
@evanlinjin
Copy link
Member

Replaced by #1478

@evanlinjin evanlinjin closed this Aug 9, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants