Allow opting out of getting `LocalChain` updates with `FullScanRequest`/`SyncRequest` structures by evanlinjin · Pull Request #1478 · bitcoindevkit/bdk

evanlinjin · 2024-06-18T11:01:31Z

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:

I've signed all my commits
I followed the contribution guidelines
I ran cargo fmt and cargo clippy before committing

New Features:

I've added tests for the new feature
I've added docs for the new feature

storopoli

Some high-level crate doc comments.
I might go over the code until the end of the weekend.

crates/esplora/src/lib.rs

notmandatory · 2024-06-20T17:22:17Z

Since this is a refactor and won't break the bdk_wallet API I'm not going to assign it a milestone until we get together for a team backlog prioritization call.

crates/esplora/src/blocking_ext.rs

evanlinjin · 2024-06-21T11:52:32Z

Since this is a refactor and won't break the bdk_wallet API I'm not going to assign it a milestone until we get together for a team backlog prioritization call.

That is fine for me. Because some users are waiting on this, I would appreciate it if we have it in soon. It doesn't break any API since it only adds new exposed methods.

LLFourn · 2024-06-24T06:56:40Z

Concept ACK

crates/esplora/src/blocking_ext.rs

LLFourn

Small doc improvements requested.

crates/chain/src/changeset.rs

crates/esplora/src/lib.rs

LLFourn · 2024-07-05T01:10:56Z

Hmm I wonder what would happen if we just made updating the chain optional as part of SyncRequest/SyncResult?

oleonardolima

Concept ACK

crates/esplora/src/lib.rs

ValuedMammal

After looking over the changes to esplora, I did confirm it's possible to calculate fee for all wallet transactions after doing a full scan

evanlinjin · 2024-08-13T01:55:09Z

@ValuedMammal it should also be possible to calculate fee for all wallet transactions after doing sync

notmandatory

I tested all the examples (rebased on #1549) and confirmed scan, sync. I have one question/request but otherwise this PR looks good to merge.

I also have one complaint, this PR should have been split up into at least two PRs, 1. to refactor the electrum and esplora scan and sync and make chain optional, and 2. to refactor sync and scan requests with the builder and more progress details. Big PRs like this are hard to properly review and prioritize.

But since I'd like to get this in for a beta.2 this week we don't have time to split and re-review separate PRs for this one. For all PR authors going forward we need to work on making smaller more focused single purpose PRs.

crates/chain/src/spk_client.rs

crates/esplora/src/async_ext.rs

notmandatory

Looks like a few more spots where SpkLabel needs to be renamed to I.

crates/chain/src/spk_client.rs

Some users would like to use esplora updates with custom `ChainOracle`s (not `LocalChain`). We introduce "low-level" extension methods that populate an update `TxGraph` with associated data. Additionally, much of the logic has been made more efficient. We make use of the `/tx/:txid` endpoint (`get_tx_info` method) to do a single call to get both the tx and tx_status. If the tx already exists, we only fetch the tx_status. The logic for fetching data based on outpoints has been reworked to be more efficient and it now supports parallelism Additionally, the logic for fetching data based on outpoints has been reworked to be more efficient and it now supports parallelism. Documentation has also been reworked. Note that this is NOT a breaking change because the API of `full_scan` and `sync` are not changed.

Prevouts are needed to calculate fees for transactions. They are introduced as floating txouts in the update `TxGraph`. A helper method `insert_prevouts` is added to insert the floating txouts using the `Vin`s returned from Esplora. Also replaced `anchor_from_status` with `insert_anchor_from_status` as we always insert the anchor into the update `TxGraph` after getting it. Also removed `bitcoin` dependency as `bdk_chain` already depends on `bitcoin` (and it's re-exported).

Change `FullScanRequest` and `SyncRequest` take in a `chain_tip` as an option. In turn, `FullScanResult` and `SyncResult` are also changed to return the update `chain_tip` as an option. This allows the caller to opt-out of getting a `LocalChain` update. Rework `FullScanRequest` and `SyncRequest` to have better ergonomics when inspecting the progress of items of a sync request. Richer progress data is provided to the inspect closure. Introduce `FullScanRequestBuilder` and `SyncRequestBuilder`. Separating out request-construction and request-consumption in different structs simplifies the API and method names. Simplify `EsploraExt` and `EsploraAsyncExt` back to having two methods (`full_scan` and `sync`). The caller can still opt out of fetching a `LocalChain` update with the new `FullScanRequest` and `SyncRequest`.

and use consistent generic parameter names across `SyncRequest` and `SyncRequestBuilder`.

ValuedMammal

ACK 6d77e2e

ValuedMammal · 2024-08-14T13:29:35Z

crates/chain/src/spk_client.rs

+    /// indexer.insert_descriptor("descriptor_a", descriptor_a)?;
+    /// indexer.insert_descriptor("descriptor_b", descriptor_b)?;
+    ///
+    /// /* Assume that the caller does more mutations to the `indexer` here... */


What extra assumptions are needed for this example to make sense? (I would think none)

rustaceanrob · 2024-08-14T14:07:57Z

It is non-intuitive to me to have SyncProgress be a collection of usize fields. It seems as though you can "consume" the SPK and still have the request fail? I might be interpreting this diff wrong but I feel as though the SyncProgress should be derived from a collection of SyncItem rather than being a distinct object

i.e. impl SyncProgress for Vec<SyncItem>

notmandatory

ACK 6d77e2e

Comments were addressed.

notmandatory · 2024-08-14T15:15:25Z

It is non-intuitive to me to have SyncProgress be a collection of usize fields. It seems as though you can "consume" the SPK and still have the request fail? I might be interpreting this diff wrong but I feel as though the SyncProgress should be derived from a collection of SyncItem rather than being a distinct object

i.e. impl SyncProgress for Vec<SyncItem>

I see your point but in practice the SyncProgress will be used to just show the percent completed for these items and especially for mobile apps using language bindings transferring smaller and simpler structs is better.

notmandatory assigned evanlinjin Jun 18, 2024

notmandatory added the module-blockchain label Jun 18, 2024

evanlinjin force-pushed the modular_esplora branch 6 times, most recently from 95dfb37 to 1924240 Compare June 19, 2024 10:29

evanlinjin marked this pull request as ready for review June 19, 2024 11:45

evanlinjin mentioned this pull request Jun 19, 2024

chore(esplora): bump client version to 0.9.0 #1475

Closed

3 tasks

storopoli reviewed Jun 19, 2024

View reviewed changes

crates/esplora/src/lib.rs Outdated Show resolved Hide resolved

crates/esplora/src/lib.rs Outdated Show resolved Hide resolved

evanlinjin force-pushed the modular_esplora branch from 1924240 to 8da3fab Compare June 19, 2024 12:08

LagginTimes reviewed Jun 21, 2024

View reviewed changes

crates/esplora/src/blocking_ext.rs Outdated Show resolved Hide resolved

crates/esplora/src/blocking_ext.rs Outdated Show resolved Hide resolved

LLFourn reviewed Jun 24, 2024

View reviewed changes

crates/esplora/src/blocking_ext.rs Outdated Show resolved Hide resolved

evanlinjin force-pushed the modular_esplora branch 4 times, most recently from e6ec3f1 to 2599f1d Compare July 1, 2024 08:57

LLFourn previously requested changes Jul 3, 2024

View reviewed changes

crates/chain/src/changeset.rs Outdated Show resolved Hide resolved

crates/esplora/src/lib.rs Outdated Show resolved Hide resolved

crates/esplora/src/lib.rs Outdated Show resolved Hide resolved

evanlinjin force-pushed the modular_esplora branch 3 times, most recently from 954b3c8 to f981105 Compare July 21, 2024 12:33

notmandatory added this to the 1.0.0-beta milestone Jul 21, 2024

oleonardolima reviewed Jul 27, 2024

View reviewed changes

ValuedMammal reviewed Jul 29, 2024

View reviewed changes

crates/esplora/src/lib.rs Outdated Show resolved Hide resolved

evanlinjin marked this pull request as draft August 1, 2024 09:30

evanlinjin requested review from ValuedMammal and notmandatory August 12, 2024 04:24

evanlinjin force-pushed the modular_esplora branch from ba0a1e7 to 7e046f3 Compare August 12, 2024 10:36

ValuedMammal reviewed Aug 13, 2024

View reviewed changes

notmandatory requested changes Aug 13, 2024

View reviewed changes

crates/chain/src/spk_client.rs Outdated Show resolved Hide resolved

ValuedMammal reviewed Aug 13, 2024

View reviewed changes

crates/esplora/src/async_ext.rs Outdated Show resolved Hide resolved

evanlinjin force-pushed the modular_esplora branch from 5e12c39 to 741bfa2 Compare August 13, 2024 08:31

evanlinjin mentioned this pull request Aug 13, 2024

fix: wallet examples #1442

Closed

8 tasks

evanlinjin force-pushed the modular_esplora branch from 741bfa2 to ac4e189 Compare August 13, 2024 09:14

notmandatory requested changes Aug 13, 2024

View reviewed changes

crates/chain/src/spk_client.rs Outdated Show resolved Hide resolved

crates/chain/src/spk_client.rs Outdated Show resolved Hide resolved

evanlinjin added 10 commits August 14, 2024 08:16

docs(esplora): Simplify crate docs

16c1c2c

fix: no premature collect

38f86fe

docs(esplora): Fix typo

0234f70

docs(chain): improve SyncRequestBuilder::spks_with_labels docs

96023c0

fix(chain): correct Iterator::size_hint impl

3eb5dd1

docs(esplora): README example, uncomment async import

584b10a

refactor(chain)!: Rename spks_with_labels to spks_with_indices

6d77e2e

and use consistent generic parameter names across `SyncRequest` and `SyncRequestBuilder`.

evanlinjin force-pushed the modular_esplora branch from ac4e189 to 6d77e2e Compare August 14, 2024 08:30

evanlinjin requested a review from notmandatory August 14, 2024 08:33

ValuedMammal approved these changes Aug 14, 2024

View reviewed changes

notmandatory approved these changes Aug 14, 2024

View reviewed changes

notmandatory merged commit cc84872 into bitcoindevkit:master Aug 14, 2024

notmandatory mentioned this pull request Aug 25, 2024

Release 1.0.0-beta.2 #1571

Closed

31 tasks

Conversation

evanlinjin commented Jun 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Notes to reviewers

Changelog notice

Checklists

All Submissions:

New Features:

Uh oh!

storopoli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

notmandatory commented Jun 20, 2024

Uh oh!

Uh oh!

Uh oh!

evanlinjin commented Jun 21, 2024

Uh oh!

LLFourn commented Jun 24, 2024

Uh oh!

Uh oh!

LLFourn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LLFourn commented Jul 5, 2024

Uh oh!

oleonardolima left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ValuedMammal left a comment

Choose a reason for hiding this comment

Uh oh!

evanlinjin commented Aug 13, 2024

Uh oh!

notmandatory left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

notmandatory left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ValuedMammal left a comment

Choose a reason for hiding this comment

Uh oh!

ValuedMammal Aug 14, 2024

Choose a reason for hiding this comment

Uh oh!

rustaceanrob commented Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

notmandatory left a comment

Choose a reason for hiding this comment

Uh oh!

notmandatory commented Aug 14, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

evanlinjin commented Jun 18, 2024 •

edited

Loading

notmandatory left a comment •

edited

Loading

rustaceanrob commented Aug 14, 2024 •

edited

Loading