Skip to content

Introduce universal sync/full-scan structures for spk-based syncing#1413

Merged
evanlinjin merged 4 commits intobitcoindevkit:masterfrom
evanlinjin:issue/1153
May 1, 2024
Merged

Introduce universal sync/full-scan structures for spk-based syncing#1413
evanlinjin merged 4 commits intobitcoindevkit:masterfrom
evanlinjin:issue/1153

Conversation

@evanlinjin
Copy link
Copy Markdown
Member

Fixes #1153
Replaces #1194

Description

Introduce universal structures that represent sync/full-scan requests/results.

Notes to the reviewers

This is based on #1194 but is different in the following ways:

  • The functionality to print scan/sync progress is not reduced.
  • SyncRequest and FullScanRequest is simplified and fields are exposed for more flexibility.

Changelog notice

  • Add universal structures for initiating/receiving sync/full-scan requests/results for spk-based syncing.
  • Updated bdk_esplora chain-source to make use of new universal sync/full-scan structures.

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

@evanlinjin evanlinjin added the api A breaking API change label Apr 25, 2024
@evanlinjin evanlinjin added this to the 1.0.0-alpha milestone Apr 25, 2024
@evanlinjin evanlinjin self-assigned this Apr 25, 2024
@evanlinjin evanlinjin force-pushed the issue/1153 branch 3 times, most recently from b336db8 to 08edd58 Compare April 25, 2024 03:31
evanlinjin and others added 2 commits April 26, 2024 12:55
These structures allows spk-based chain-sources to have a universal API.

Co-authored-by: Steve Myers <steve@notmandatory.org>
* Changed `Wallet::apply_update` to also take in anything that
  implements `Into<Update>`. This allows us to directly apply a
  `FullScanResult` or `SyncResult`.
* Added `start_full_scan` and `start_sync_with_revealed_spks` methods to
  `Wallet`.

Co-authored-by: Steve Myers <steve@notmandatory.org>
Comment on lines -58 to +75
.full_scan(prev_tip, keychain_spks, STOP_GAP, PARALLEL_REQUESTS)
.full_scan(request, STOP_GAP, PARALLEL_REQUESTS)
.await?;
let now = std::time::UNIX_EPOCH.elapsed().unwrap().as_secs();
let _ = update.tx_graph.update_last_seen_unconfirmed(now);
let _ = update.graph_update.update_last_seen_unconfirmed(now);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! I was going through this example yesterday and I was having tons of issues because even without this PR it was un-reproducible code.
Thanks!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry I don't quite understand?

Copy link
Copy Markdown

@storopoli storopoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK.
These are great changes.

I imagine that you are planning to implement this to electrum as well? Either in this PR or in a separate PR?

This allows the caller to track sync progress.
Copy link
Copy Markdown
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK c0374a0

Thanks for the improvements!

The examples ran fine. But I did have to change the wallet examples to use signet and the signet esplora server http://signet.bitcoindevkit.net since others are rate limiting and so the scans were failing with 429 errors.

let mut visited = 0;
move |_| {
visited += 1;
eprintln!(" [ {:>6.2}% ]", (visited * 100) as f32 / total_spks as f32)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😃 nice touch showing progress as percents.


/// Set the [`Script`]s that will be synced against.
///
/// This consumes the [`SyncRequest`] and returns the updated one.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😃 I like how you changed all the Sync/FullScan Request functions to consume and return Self, makes them much easier to use.

@evanlinjin evanlinjin merged commit 08fac47 into bitcoindevkit:master May 1, 2024
This was referenced May 2, 2024
notmandatory added a commit that referenced this pull request May 10, 2024
b45897e feat(electrum): update docs and simplify logic of `ElectrumExt` (志宇)
92fb6cb chore(electrum): do not use `anyhow::Result` directly (志宇)
b2f3cac feat(electrum): include option for previous `TxOut`s for fee calculation (Wei Chen)
c0d7d60 feat(chain)!: use custom return types for `ElectrumExt` methods (志宇)
2945c6b fix(electrum): fixed `sync` functionality (Wei Chen)
9ed33c2 docs(electrum): fixed `full_scan`, `sync`, and crate documentation (Wei Chen)
b1f861b feat: update logging of electrum examples (志宇)
a6fdfb2 feat(electrum)!: use new sync/full-scan structs for `ElectrumExt` (志宇)
653e4fe feat(wallet): cache txs when constructing full-scan/sync requests (志宇)
58f27b3 feat(chain): introduce `TxCache` to `SyncRequest` and `FullScanRequest` (志宇)
721bb7f fix(chain): Make `Anchor` type in `FullScanResult` generic (志宇)
e3cfb84 feat(chain): `TxGraph::insert_tx` reuses `Arc` (志宇)
2ffb656 refactor(electrum): remove `RelevantTxids` and track txs in `TxGraph` (Wei Chen)

Pull request description:

  Fixes #1265
  Possibly fixes #1419

  ### Context

  Previous changes such as

  * Universal structures for full-scan/sync (PR #1413)
  * Making `CheckPoint` linked list query-able (PR #1369)
  * Making `Transaction`s cheaply-clonable (PR #1373)

  has allowed us to simplify the interaction between chain-source and receiving-structures (`bdk_chain`).

  The motivation is to accomplish something like this ([as mentioned here](#1153 (comment))):
  ```rust
  let things_I_am_interested_in = wallet.lock().unwrap().start_sync();
  let update = electrum_or_esplora.sync(things_i_am_interested_in)?;
  wallet.lock().unwrap().apply_update(update)?:
  ```

  ### Description

  This PR greatly simplifies the API of our Electrum chain-source (`bdk_electrum`) by making use of the aforementioned changes. Instead of referring back to the receiving `TxGraph` mid-sync/scan to determine which full transaction to fetch, we provide the Electrum chain-source already-fetched full transactions to start sync/scan (this is cheap, as transactions are wrapped in `Arc`s since #1373).

  In addition, an option has been added to include the previous `TxOut` for transactions received from an external wallet for fee calculation.

  ### Changelog notice

  * Change `TxGraph::insert_tx` to take in anything that satisfies `Into<Arc<Transaction>>`. This allows us to reuse the `Arc` pointer of what is being inserted.
  * Add `tx_cache` field to `SyncRequest` and `FullScanRequest`.
  * Make `Anchor` type in `FullScanResult` generic for more flexibility.
  * Change `ElectrumExt` methods to take in `SyncRequest`/`FullScanRequest` and return `SyncResult`/`FullScanResult`. Also update electrum examples accordingly.
  * Add `ElectrumResultExt` trait which allows us to convert the update `TxGraph` of `SyncResult`/`FullScanResult` for `bdk_electrum`.
  * Added an option for `full_scan` and `sync` to also fetch previous `TxOut`s to allow for fee calculation.

  ### 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

ACKs for top commit:
  ValuedMammal:
    ACK b45897e
  notmandatory:
    ACK b45897e

Tree-SHA512: 1e274546015e7c7257965b36079ffe0cb3c2c0b7c2e0c322bcf32a06925a0c3e1119da1c8fd5318f1dbd82c2e952f6a07f227a9b023c48f506a62c93045d96d3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api A breaking API change

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

[wallet] Consider having a higher-level method for syncing

3 participants