bdk icon indicating copy to clipboard operation
bdk copied to clipboard

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

Open LagginTimes opened this issue 1 year ago • 5 comments

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 Transactions 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):

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:

  • [x] I've signed all my commits
  • [x] I followed the contribution guidelines
  • [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

LagginTimes avatar Apr 11 '24 22:04 LagginTimes

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

evanlinjin avatar Apr 29 '24 08:04 evanlinjin

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 avatar Apr 29 '24 14:04 evanlinjin

@LagginTimes thanks for cleaning it up!

evanlinjin avatar May 06 '24 09:05 evanlinjin

What do you see as the next logical step to improving the electrum crate, for example in relation to Rearchitect bdk_electrum syncing/scanning. #1332 and Option for Electrum and Esplora APIs to include floating TxOuts in updates for fee calculation #1265

I would tacke #1265 first. This will make bdk_electrum "feature complete". We can worry about the internals of it after 1.0 is released.

In electrum_ext: do we really need the hard-coded genesis txid? Is this condition even possible?

I would assume not, however someone needs to have a look into it. Additionally, the hardcoded txid is not complete as it only includes the genesis coinbase tx for mainnet.

In electrum_ext: Can we add comments for the populate_with_* functions? what's being populated, who's doing the populating, etc

Good idea.

evanlinjin avatar May 07 '24 04:05 evanlinjin

I will start poking around with the electrum side of #1265 as well.

LagginTimes avatar May 07 '24 04:05 LagginTimes

A test to see whether fee is actually calculated will be even better.

evanlinjin avatar May 09 '24 08:05 evanlinjin

This needs a rebase, merging #1203 broke this.

evanlinjin avatar May 10 '24 06:05 evanlinjin