-
Notifications
You must be signed in to change notification settings - Fork 307
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
Introduce universal sync/full-scan structures for spk-based syncing #1413
Conversation
b336db8
to
08edd58
Compare
These structures allows spk-based chain-sources to have a universal API. Co-authored-by: Steve Myers <[email protected]>
* 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 <[email protected]>
.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); |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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?
There was a problem hiding this 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.
There was a problem hiding this 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) |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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.
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
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:
SyncRequest
andFullScanRequest
is simplified and fields are exposed for more flexibility.Changelog notice
bdk_esplora
chain-source to make use of new universal sync/full-scan structures.Checklists
All Submissions:
cargo fmt
andcargo clippy
before committingNew Features: