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

Add wallet sync_request and full_scan_request functions #1194

Closed

Conversation

notmandatory
Copy link
Member

@notmandatory notmandatory commented Nov 2, 2023

This is WIP. So far only demonstrating with async esplora, if approach looks good I'll do the rest.

Description

Adding new functions to for esplora and electrum light client ext modules to simplify full_scan and sync operations.

This is another attempt to fix #1153.

Notes to the reviewers

Changelog notice

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

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@notmandatory notmandatory added this to the 1.0.0-alpha.3 milestone Nov 2, 2023
@notmandatory notmandatory self-assigned this Nov 2, 2023
@evanlinjin
Copy link
Member

@notmandatory do you mind adding a more detailed explanation as to how this pushes us in a direction to have a higher-level method/methods for syncing a wallet?

@LLFourn
Copy link
Contributor

LLFourn commented Nov 3, 2023

@notmandatory what about doing it like I suggested here: #1153 (comment)

@notmandatory
Copy link
Member Author

Suggestion from #1153 (comment)

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

@evanlinjin where I'm at right now is trying to figure out what wallet data is needed for scan and sync and how to get what we need from existing wallet functions. I should be able to get back to the suggested solution above but am stuck on a few questions maybe you and @LLFourn can help with.

  1. does it make sense to have different wallet.start_sync and wallet.start_scan since those operations need different info?

  2. things_I_am_interested_in and update need to be tuples right? I don't see any place to put new structs for these without creating unwanted cross-crate-dependencies. It will be trivial to make a impl From to convert the tuple returned from any of the clients back into a wallet Update.

  3. For the things_I_am_interested_in does it make sense to clone and collect what I can? I know the goal is to keep this thread safe, is cloning what I need for syncing the only/best option?

What I've found so far is for a client.scan() we need three things from the wallet:

keychain_spks: BTreeMap<K, impl IntoIterator<IntoIter = impl Iterator<Item = (u32, ScriptBuf)> + Send> + Send,>,
local_chain: &LocalChain,
local_tip: Option<CheckPoint>,

and for a client.sync() we need five things:

spks: Vec<ScriptBuf>,
local_chain: &LocalChain,
local_tip: Option<CheckPoint>,
outpoints: Vec<OutPoint>,
txids: Vec<Txid>,

If you guys have any better ideas for these types I'll try to make them work. Thanks!

@notmandatory notmandatory force-pushed the feat/wallet_start_update branch from 070ee02 to c97ddef Compare November 4, 2023 01:03
@notmandatory
Copy link
Member Author

I added Wallet start_scan and start_sync functions as an example of what I have in mind. Once I have confirmation the types are correct I need to add some tests and probably better docs and then also implement scan and sync for EsploraExt and ElectrumExt and update related docs and examples.

@LLFourn
Copy link
Contributor

LLFourn commented Nov 6, 2023

Suggestion from #1153 (comment)

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

@evanlinjin where I'm at right now is trying to figure out what wallet data is needed for scan and sync and how to get what we need from existing wallet functions. I should be able to get back to the suggested solution above but am stuck on a few questions maybe you and @LLFourn can help with.

1. does it make sense to have different `wallet.start_sync` and `wallet.start_scan` since those operations need different info?

I hadn't thought of making start_scan as well. But I think we're renaming this to rescan and fixing "scan" arguments to just what we need #1112.

2. `things_I_am_interested_in` and `update` need to be tuples right? I don't see any place to put new structs for these without creating unwanted cross-crate-dependencies. It will be trivial to make a `impl From` to convert the tuple returned from any of the clients back into a wallet `Update`.

I don't think they should be tuples? Just a struct definition containing all spks, txids etc. The definition should be in bdk_chain.

3. For the `things_I_am_interested_in` does it make sense to clone and collect what I can? I know the goal is to keep this thread safe, is cloning what I need for syncing the only/best option?

Yes. Things should be cloned and collected out of wallet. FWIW I think we overuse IntoIterator currently. These can just be Vec.

I made another issue to try and collect requirements here: #1195

@notmandatory
Copy link
Member Author

This will need a little rework after #1178 is merged.

@notmandatory
Copy link
Member Author

Also based on poll on discord I'm renaming function names to full_scan and sync, see: https://discord.com/channels/753336465005608961/753367451319926827/1171551358353158164

@notmandatory notmandatory modified the milestones: 1.0.0-alpha.3, 1.0.0-alpha.4 Nov 13, 2023
@notmandatory notmandatory changed the title Add scan and sync functions to EsploraAsyncExt, EsploraExt, and EsploraExt Add full_scan and sync functions to EsploraAsyncExt, EsploraExt, and EsploraExt Dec 20, 2023
@notmandatory notmandatory changed the title Add full_scan and sync functions to EsploraAsyncExt, EsploraExt, and EsploraExt Add wallet start_full_scan and start_sync functions, update esplora_ext and electrum_ext to use Dec 20, 2023
@notmandatory notmandatory changed the title Add wallet start_full_scan and start_sync functions, update esplora_ext and electrum_ext to use Add wallet start_full_scan and start_sync functions Dec 20, 2023
@nondiremanuel nondiremanuel modified the milestones: 1.0.0-alpha.4, 1.0.0 Jan 6, 2024
@notmandatory notmandatory force-pushed the feat/wallet_start_update branch 5 times, most recently from 0ffde1a to 614f2b6 Compare January 10, 2024 21:02
crates/chain/src/request.rs Outdated Show resolved Hide resolved
example-crates/wallet_esplora_async/src/main.rs Outdated Show resolved Hide resolved
Wrapping transactions as `Arc<Transaction>` allows us to share
transactions cheaply between the chain-source and receiving structures.
Therefore the chain-source can keep already-fetched transactions (save
bandwidth) and have a shared pointer to the transactions (save memory).

This is better than the current way we do things, which is to refer back
to the receiving structures mid-sync.

Documentation for `TxGraph` is also updated.
These methods allow us to query for checkpoints contained within the
linked list by height. This is useful to determine checkpoints to fetch
for chain sources without having to refer back to the `LocalChain`.

Currently this is not implemented efficiently, but in the future, we
will change `CheckPoint` to use a skip list structure.
We impl `PartialEq` on `CheckPoint` instead of directly on `LocalChain`.
We also made the implementation more efficient.
This creates a checkpoint linked list which contains all blocks.
Previously, we would update the `TxGraph` and `KeychainTxOutIndex`
first, then create a second update for `LocalChain`. This required
locking the receiving structures 3 times (instead of twice, which
is optimal).

This PR eliminates this requirement by making use of the new `query`
method of `CheckPoint`.

Examples are also updated to use the new API.
These methods are no longer needed as we can determine missing heights
directly from the `CheckPoint` tip.
This gets the genesis hash of the env blockchain.
We ensure that calling `finalize_chain_update` does not result in a
chain which removed previous heights and all anchor heights are
included.
@evanlinjin evanlinjin mentioned this pull request Mar 26, 2024
5 tasks
@notmandatory notmandatory force-pushed the feat/wallet_start_update branch from 49c850b to 72c438a Compare March 29, 2024 15:25
@notmandatory
Copy link
Member Author

notmandatory commented Mar 29, 2024

I rebased this PR on #1380. In the process I also figured out a way to use the generic K for the function passed into FullScanRequest::inspect_spks(). To do it I had to wrap the Boxed function in an Arc and use the Fn type instead of FnMut. Before I was making a copy of the inspect function for every inspected item, this new way using a reference to the same function which should be more efficient.

evanlinjin added a commit that referenced this pull request Apr 18, 2024
fac2283 feat(chain)!: make `KeychainTxOutIndex` more range based (LLFourn)

Pull request description:

  KeychainTxOut index should try and avoid "all" kind of queries. There may be subranges of interest. If the user wants "all" they can just query "..".

  The ideas is that KeychainTxOutIndex should be designed to be able to incorporate many unrelated keychains that can be managed in the same index. We should be able to see the "net_value" of a transaction to a specific subrange. e.g. imagine a collaborative custody service that manages all their user descriptors inside the same `KeychainTxOutIndex`. One user in their service may pay another so when you are analyzing how much a transaction is spending for a particular user you need to do analyze a particular sub-range.

  ### Notes to the reviewers

  - I didn't change `unused_spks` to follow this rule because I want to delete that method some time in the future. `unused_spks` is being used in the examples for syncing but it shouldn't be (the discussion as to why will probably surface in #1194).
  - I haven't applied this reasoning to the methods that return `BTreeMap`s e.g. `all_unbounded_spk_iters`. It probably should be but I haven't made up my mind yet.

  This probably belongs after #1194

  ### Changelog notice

  - `KeychainTxOutIndex` methods modified to take ranges of keychains instead.

  ### 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:
  evanlinjin:
    ACK fac2283

Tree-SHA512: ec1e75f19d79f71de4b6d7748ef6da076ca92c2f3fd07e0f0dc88e091bf80c61268880ef78be4bed5e0dbab2572e22028f868f33e68a67d47813195d38d78ba5
@notmandatory
Copy link
Member Author

closing in favor of #1413

evanlinjin added a commit that referenced this pull request May 1, 2024
…sed syncing

c0374a0 feat(chain): `SyncRequest` now uses `ExactSizeIterator`s (志宇)
0f94f24 feat(esplora)!: update to use new sync/full-scan structures (志宇)
4c52f3e feat(wallet): make wallet compatible with sync/full-scan structures (志宇)
cdfec5f feat(chain): add sync/full-scan structures for spk-based syncing (志宇)

Pull request description:

  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:
  * The functionality to print scan/sync progress is not reduced.
  * `SyncRequest` and `FullScanRequest` is simplified and fields are exposed for more flexibility.

  ### Changelog notice

  * Add universal structures for initiating/receiving sync/full-scan requests/results for spk-based syncing.
  * Updated `bdk_esplora` chain-source to make use of new universal sync/full-scan structures.

  ### 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:
  notmandatory:
    tACK c0374a0

Tree-SHA512: c2ad66d972a6785079bca615dfd128edcedf6b7a02670651a0ab1ce5b5174dd96f54644680eedbf55e3f1955fe5c34f632eadbd3f71d7ffde658753c6c6d42be
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[wallet] Consider having a higher-level method for syncing
6 participants