Update `bdk_electrum` crate to use sync/full-scan structs by LagginTimes · Pull Request #1403 · bitcoindevkit/bdk

LagginTimes · 2024-04-11T22:16:30Z

Fixes #1265
Possibly fixes #1419

Context

Previous changes such as

Universal structures for full-scan/sync (PR Introduce universal sync/full-scan structures for spk-based syncing #1413)
Making CheckPoint linked list query-able (PR feat(chain): add get and range methods to CheckPoint #1369)
Making Transactions cheaply-clonable (PR Wrap transactions as Arc<Transaction> in TxGraph #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):

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 Arcs 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 TxOuts to allow for fee calculation.

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

ValuedMammal

Looking good so far.

I made a note related to adding the client as a parameter to determine_tx_anchor - you may have already considered this. I see you mentioned potentially doing more batch calls, which is a good idea assuming we can ensure the results are in order consistent with the request.

side note: the commit message could use an extra newline after the subject and before the body.

crates/electrum/src/electrum_ext.rs

evanlinjin

~~It's great that the API is simplified now. However, with the current changes, every call to ElectrumExt::full_scan forces us to download every full tx?~~

EDIT: Ahh, I see your TODOs in your description.

evanlinjin · 2024-04-29T08:41:58Z

For the commit message, remember to put a new line between first line and body.

evanlinjin · 2024-04-29T14:29:06Z

It seems that you intended the sync flow to be:

Copy the wallet's TxGraph and pass it to the chain source.
Perform update, and populate TxGraph with missing transactions/anchors and return it back to the wallet.

However, the downsides of this approach is the following:

Because we are cloning the wallet's TxGraph, and TxGraph stores both transactions and anchors, and only transactions are cheaply copied (not anchors), therfore copying the TxGraph over is non-optimal.
The anchors that are copied from the wallet cannot be used anyway. We need to keep the anchor type in the TxGraph a generic to support all wallet types. However, we wish to return an update TxGraph with the ConfirmationTimeHeightAnchor to represent all information available from Electrum. We cannot guarantee that the wallet's anchor type can be transformed into ConfrimastionTimeHeightAnchor, therefore we end up just re-fetching the anchors from Electrum anyway.

Hence, I think it is better to have a transaction cache HashMap<Txid, Arc<Transaction>> and the TxGraph update as a separate concept.

crates/electrum/src/electrum_ext.rs

evanlinjin

ACK 4ccd86a

This PR removes `RelevantTxids` from the electrum crate and tracks transactions in a `TxGraph`. This removes the need to separately construct a `TxGraph` after a `full_scan` or `sync`.

When we insert a transaction that is already wrapped in `Arc`, we should reuse the `Arc`.

This transaction cache can be provided so the chain-source can avoid re-fetching transactions.

evanlinjin · 2024-05-10T06:44:22Z

This needs a rebase, merging #1203 broke this.

`ElectrumResultExt` trait is also introduced that adds methods which can convert the `Anchor` type for the update `TxGraph`. We also make use of the new `TxCache` fields in `SyncRequest`/`FullScanRequest`. This way, we can avoid re-fetching full transactions from Electrum if not needed. Examples and tests are updated to use the new `ElectrumExt` API.

* Syncing with `example_electrum` now shows progress as a percentage. * Flush stdout more aggressively.

This is more code, but a much more elegant solution than having `ElectrumExt` methods return `SyncResult`/`FullScanResult` and having an `ElectrumResultExt` extention trait.

The previous `TxOut` for transactions received from an external wallet may be optionally added as floating `TxOut`s to `TxGraph` to allow for fee calculation.

Helper method docs are updated to explain what they are updating. Logic is simplified as we do not need to check whether a tx exists already in `update_graph` before inserting it.

ValuedMammal

ACK b45897e

notmandatory

ACK b45897e

Tested with examples: example_electrum and wallet_electrum

LagginTimes force-pushed the electrum_rework branch from 53ed564 to 9033e0e Compare April 11, 2024 22:29

notmandatory assigned LagginTimes Apr 12, 2024

notmandatory added module-blockchain api A breaking API change labels Apr 12, 2024

notmandatory added this to the 1.0.0-alpha milestone Apr 12, 2024

ValuedMammal reviewed Apr 15, 2024

View reviewed changes

crates/electrum/src/electrum_ext.rs Outdated Show resolved Hide resolved

LagginTimes force-pushed the electrum_rework branch 3 times, most recently from 6f39514 to bb84d8f Compare April 17, 2024 22:14

evanlinjin reviewed Apr 19, 2024

View reviewed changes

evanlinjin marked this pull request as draft April 19, 2024 12:46

LagginTimes force-pushed the electrum_rework branch 3 times, most recently from d008d31 to 2540258 Compare April 24, 2024 19:37

evanlinjin mentioned this pull request Apr 30, 2024

Continuation of #1403 #1415

Closed

8 tasks

evanlinjin force-pushed the electrum_rework branch 3 times, most recently from 077ea56 to 2905f09 Compare May 1, 2024 08:30

LagginTimes force-pushed the electrum_rework branch from 2905f09 to 49c0c4e Compare May 3, 2024 13:10

evanlinjin force-pushed the electrum_rework branch 5 times, most recently from f0278f5 to a39a7e9 Compare May 4, 2024 11:21

evanlinjin changed the title ~~refactor(electrum): remove RelevantTxids and track txs in TxGraph~~ Update bdk_electrum crate to use sync/full-scan structs May 4, 2024

evanlinjin marked this pull request as ready for review May 4, 2024 12:08

evanlinjin requested review from LLFourn and ValuedMammal May 4, 2024 12:08

evanlinjin reviewed May 8, 2024

View reviewed changes

crates/electrum/src/electrum_ext.rs Show resolved Hide resolved

LagginTimes force-pushed the electrum_rework branch 2 times, most recently from fdffdd0 to 93a4ae6 Compare May 8, 2024 08:50

evanlinjin requested changes May 8, 2024

View reviewed changes

crates/electrum/src/electrum_ext.rs Outdated Show resolved Hide resolved

crates/electrum/src/electrum_ext.rs Outdated Show resolved Hide resolved

crates/electrum/src/electrum_ext.rs Show resolved Hide resolved

LagginTimes force-pushed the electrum_rework branch 2 times, most recently from 6628855 to 4ccd86a Compare May 9, 2024 03:44

evanlinjin approved these changes May 9, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

evanlinjin mentioned this pull request May 10, 2024

Bump bdk version to 1.0.0-alpha.11 #1437

Merged

3 tasks

LagginTimes and others added 5 commits May 10, 2024 14:11

refactor(electrum): remove RelevantTxids and track txs in TxGraph

2ffb656

This PR removes `RelevantTxids` from the electrum crate and tracks transactions in a `TxGraph`. This removes the need to separately construct a `TxGraph` after a `full_scan` or `sync`.

feat(chain): TxGraph::insert_tx reuses Arc

e3cfb84

When we insert a transaction that is already wrapped in `Arc`, we should reuse the `Arc`.

fix(chain): Make Anchor type in FullScanResult generic

721bb7f

feat(chain): introduce TxCache to SyncRequest and FullScanRequest

58f27b3

This transaction cache can be provided so the chain-source can avoid re-fetching transactions.

feat(wallet): cache txs when constructing full-scan/sync requests

653e4fe

evanlinjin and others added 7 commits May 10, 2024 14:54

feat: update logging of electrum examples

b1f861b

* Syncing with `example_electrum` now shows progress as a percentage. * Flush stdout more aggressively.

docs(electrum): fixed full_scan, sync, and crate documentation

9ed33c2

fix(electrum): fixed sync functionality

2945c6b

feat(chain)!: use custom return types for ElectrumExt methods

c0d7d60

This is more code, but a much more elegant solution than having `ElectrumExt` methods return `SyncResult`/`FullScanResult` and having an `ElectrumResultExt` extention trait.

feat(electrum): include option for previous TxOuts for fee calculation

b2f3cac

The previous `TxOut` for transactions received from an external wallet may be optionally added as floating `TxOut`s to `TxGraph` to allow for fee calculation.

chore(electrum): do not use anyhow::Result directly

92fb6cb

evanlinjin force-pushed the electrum_rework branch from 323bfc7 to 92fb6cb Compare May 10, 2024 06:55

feat(electrum): update docs and simplify logic of ElectrumExt

b45897e

Helper method docs are updated to explain what they are updating. Logic is simplified as we do not need to check whether a tx exists already in `update_graph` before inserting it.

ValuedMammal approved these changes May 10, 2024

View reviewed changes

notmandatory approved these changes May 10, 2024

View reviewed changes

notmandatory merged commit 63e3bbe into bitcoindevkit:master May 10, 2024

This was referenced May 10, 2024

Release 1.0.0-alpha.11 #1435

Closed

Rearchitect bdk_electrum syncing/scanning. #1332

Closed

Syncing/Rescanning super issue #1195

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update `bdk_electrum` crate to use sync/full-scan structs#1403

Update `bdk_electrum` crate to use sync/full-scan structs#1403
notmandatory merged 13 commits intobitcoindevkit:masterfrom
LagginTimes:electrum_rework

LagginTimes commented Apr 11, 2024 •

edited

Loading

Uh oh!

ValuedMammal left a comment

Uh oh!

Uh oh!

evanlinjin left a comment •

edited

Loading

Uh oh!

evanlinjin commented Apr 29, 2024

Uh oh!

evanlinjin commented Apr 29, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

evanlinjin left a comment

Uh oh!

This comment has been minimized.

evanlinjin commented May 10, 2024

Uh oh!

ValuedMammal left a comment

Uh oh!

notmandatory left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

LagginTimes commented Apr 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Description

Changelog notice

Checklists

All Submissions:

New Features:

Uh oh!

ValuedMammal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

evanlinjin 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!

evanlinjin commented Apr 29, 2024

Uh oh!

evanlinjin commented Apr 29, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

evanlinjin left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

evanlinjin commented May 10, 2024

Uh oh!

ValuedMammal left a comment

Choose a reason for hiding this comment

Uh oh!

notmandatory left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

LagginTimes commented Apr 11, 2024 •

edited

Loading

evanlinjin left a comment •

edited

Loading