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

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

Closed
danielabrozzoni opened this issue Oct 6, 2023 · 6 comments · Fixed by #1413
Closed

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

danielabrozzoni opened this issue Oct 6, 2023 · 6 comments · Fixed by #1413
Assignees
Labels
Milestone

Comments

@danielabrozzoni
Copy link
Member

Looking at wallet_esplora or wallet_electrum, syncing takes quite a bit of code, and I suppose than 99% of users will just want to update the whole wallet in one big batch, without any fancy optimization. We should consider having a higher level method similar to the old wallet.sync() to reduce the amount of code copy-pasted from our examples.

Maybe we could have sync_esplora, sync_electrum, sync_bitcoind and all of them need a wallet passed in?

@danielabrozzoni danielabrozzoni added the new feature New feature or request label Oct 6, 2023
@notmandatory
Copy link
Member

I've started preliminary work on this in #1139 :-)

@notmandatory notmandatory added this to BDK Oct 6, 2023
@notmandatory notmandatory moved this to Todo in BDK Oct 6, 2023
@notmandatory notmandatory added this to the 1.0.0-alpha.3 milestone Oct 6, 2023
@LLFourn
Copy link
Contributor

LLFourn commented Oct 9, 2023

I think there is almost nothing copy pasted between the bitcoind and electrum examples?
In any case they are architecturally different. In the bitcoind example, your application has a thread that is always running and pulling in new blocks and dumping the mempool every so often. In electrum and esplora, you opportunistically sync whenever you think something might have changed i.e. after giving out a new address, or broadcasting a transaction etc

You could certainly do this for electrum and esplora. See: #1139 (comment) on how I would approach this. Basically Wallet could be capable of producing a bunch of "things I am interested in" + checkpoint and the electrum and esplora code could produce an update from that generically.

So something like:

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

@danielabrozzoni
Copy link
Member Author

I think there is almost nothing copy pasted between the bitcoind and electrum examples?

Sorry if I wasn't clear, I meant users copying from our examples to their code.

@LLFourn this looks fantastic.

To be clear, I wasn't suggesting to have one method that works for every backend (as wallet.sync() did); I'm just suggesting to hide some of the complexity inside some auxiliary methods (such as wallet.start_sync(), as you said)

@evanlinjin
Copy link
Member

evanlinjin commented Nov 4, 2023

Going along with @LLFourn's suggestion, this is my proposal:

Wallet::start_sync returns a struct that implements a trait that gets the start sync data (let's call this StartSyncGetter for now).

pub trait StartSyncGetter {
    fn local_chain_tip(&self) -> CheckPoint;
    /* all methods required to get initial data to start sync/scan */
}

The chain source has a method that takes in StartSyncGetter and outputs a struct that contains all data/structures needed for the chain source to start fetching data. The returned structure MUST not hold a reference on the StartSyncGetter input.

The chain source has a second method that actually does IO (fetch data).


P.S. I accidentally clicked on "comment". This proposal is premature.

@LLFourn
Copy link
Contributor

LLFourn commented Nov 5, 2023

Why do we need a trait for this. What's wrong with a struct that just has all the fields you need.

@storopoli
Copy link
Contributor

I don't think this is closed since we are still missing electrum at least right, @evanlinjin?

notmandatory added a commit that referenced this issue 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