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

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

Merged
merged 13 commits into from
May 10, 2024

Conversation

LagginTimes
Copy link
Contributor

@LagginTimes LagginTimes commented Apr 11, 2024

Fixes #1265
Possibly fixes #1419

Context

Previous changes such as

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

Copy link
Contributor

@ValuedMammal ValuedMammal left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
@LagginTimes LagginTimes force-pushed the electrum_rework branch 3 times, most recently from 6f39514 to bb84d8f Compare April 17, 2024 22:14
Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

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

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 evanlinjin marked this pull request as draft April 19, 2024 12:46
@LagginTimes LagginTimes force-pushed the electrum_rework branch 3 times, most recently from d008d31 to 2540258 Compare April 24, 2024 19:37
@evanlinjin
Copy link
Member

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

@evanlinjin
Copy link
Member

It seems that you intended the sync flow to be:

  1. Copy the wallet's TxGraph and pass it to the chain source.
  2. 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:

  1. 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.
  2. 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.

@evanlinjin evanlinjin mentioned this pull request Apr 30, 2024
8 tasks
@evanlinjin evanlinjin force-pushed the electrum_rework branch 3 times, most recently from 077ea56 to 2905f09 Compare May 1, 2024 08:30
@evanlinjin evanlinjin force-pushed the electrum_rework branch 5 times, most recently from f0278f5 to a39a7e9 Compare May 4, 2024 11:21
@evanlinjin 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 evanlinjin marked this pull request as ready for review May 4, 2024 12:08
@evanlinjin evanlinjin requested review from ValuedMammal and LLFourn May 4, 2024 12:08
@LagginTimes LagginTimes force-pushed the electrum_rework branch 2 times, most recently from fdffdd0 to 93a4ae6 Compare May 8, 2024 08:50
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 Outdated Show resolved Hide resolved
@LagginTimes LagginTimes force-pushed the electrum_rework branch 2 times, most recently from 6628855 to 4ccd86a Compare May 9, 2024 03:44
Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

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

ACK 4ccd86a

@evanlinjin

This comment has been minimized.

LagginTimes and others added 5 commits May 10, 2024 14:11
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
Copy link
Member

This needs a rebase, merging #1203 broke this.

evanlinjin and others added 7 commits May 10, 2024 14:54
`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.
Copy link
Contributor

@ValuedMammal ValuedMammal left a comment

Choose a reason for hiding this comment

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

ACK b45897e

Copy link
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.

ACK b45897e

Tested with examples: example_electrum and wallet_electrum

@notmandatory notmandatory merged commit 63e3bbe into bitcoindevkit:master May 10, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api A breaking API change module-blockchain
Projects
Archived in project
4 participants