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

feat: add blockchain traits module with esplora_async impl #1139

Closed

Conversation

notmandatory
Copy link
Member

Description

WIP to make a simpler API for users to do common sync operations.

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 force-pushed the feat/blockchain branch 2 times, most recently from 4ce9011 to 8856611 Compare September 30, 2023 22:39
@LLFourn
Copy link
Contributor

LLFourn commented Oct 3, 2023

Concept NACK.

  1. This reintroduces the problem of keeping a lock over the Wallet while doing IO. Removing this is the main goal of v1. You can overcome this by taking in a Arc<Mutex<Wallet>>.
  2. This introduces lots of dependencies to bdk::Wallet making long term MSRV policy difficult and we'd be back with different MSRVs per feature.
  3. These traits are only appropriate for esplora and electrum i.e. spks style syncing. By having them it makes it seem as if we support this kind of syncing as a more first class citizen to others. My personal feeling is that we should push development of CBF as the best way to use the library when that line of development is accomplished.
  4. If we really want some helper methods to simplify use make a trait in bdk_chain that Wallet (or Arc<Mutex<Wallet>>) can implement, then you can have DWIM methods in bdk_esplora and bdk_electrum that sync against any type that implements this trait. The trait would let you query (i) current local chain tip checkpoint and (ii) spks, txids etc of interest.

Comment on lines +27 to +49
/// Defines the options when syncing spks with an Electrum or Esplora blockchain client.
#[derive(Debug)]
pub struct SpkSyncMode {
/// Sync all spks the wallet has ever derived
pub all_spks: bool,
/// Sync only derived spks that have not been used, only applies if `all_spks` is false
pub unused_spks: bool,
/// Sync wallet utxos
pub utxos: bool,
/// Sync unconfirmed transactions
pub unconfirmed_tx: bool,
}

impl Default for SpkSyncMode {
fn default() -> Self {
Self {
all_spks: false,
unused_spks: true,
utxos: true,
unconfirmed_tx: true,
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

From our discussion in FFI bindings meeting: can this be an enum?

@tnull
Copy link
Contributor

tnull commented Oct 3, 2023

I want to @LLFourn's points that re-introducing separate async/blocking traits might make it hard to switch between syncing methods during runtime. If these are introduced, I'd prefer if we could make sure all relevant traits are implemented for all syncing methods (i.e., also have async bitcoind/electrum implementations, for instance).

@evanlinjin
Copy link
Member

Is the purpose of introducing these traits to make it esier to implement FFI bindings?

@notmandatory
Copy link
Member Author

The initial reason I started looking into this, as @evanlinjin mentioned above, is so the mobile bindings and Rust wallet users don't need to rewrite the same sync logic themselves. Instead I thought I could just throw the the scan/sync logic from the wallet_* examples behind some trait impls.

Concept NACK.

1. This reintroduces the problem of keeping a lock over the Wallet while doing IO. Removing this is the main goal of `v1`. You can overcome this by taking in a `Arc<Mutex<Wallet>>`.

Based on below I can see even if I could do what I'm trying to do without a Mutex it isn't the right approach.

2. This introduces lots of dependencies to `bdk::Wallet` making long term MSRV policy difficult and we'd be back with different MSRVs per feature.

Yes this is a problem and I agree we don't want to get back into all the MSRV and feature problems.

3. These traits are only appropriate for esplora and electrum i.e. spks style syncing. By having them it makes it seem as if we support this kind of syncing as a more first class citizen to others. My personal feeling is that we should push development of CBF as the best way to use the library when that line of development is accomplished.

Long term I'd like to see users using CBF too, but most users today need esplora/electrum so I want to make their lives as easy as possible.

4. If we really want some helper methods to simplify use make a trait in `bdk_chain` that `Wallet` (or `Arc<Mutex<Wallet>>`) can implement, then you can have DWIM methods in `bdk_esplora` and `bdk_electrum` that sync against any type that implements this trait. The trait would let you query (i) current local chain tip checkpoint and (ii) spks, txids etc of interest.

I'm going to close this PR, but I like your suggestion for how to add some sort of helper traits in the bdk_chain crate. I'll try noodling around with that in a new PR. Thanks for all the feedback !

@notmandatory notmandatory removed this from the 1.0.0-alpha.3 milestone Oct 13, 2023
@LLFourn
Copy link
Contributor

LLFourn commented Oct 13, 2023

I'm going to close this PR, but I like your suggestion for how to add some sort of helper traits in the bdk_chain crate. I'll try noodling around with that in a new PR. Thanks for all the feedback !

Awesome. I'd say start with something like this: #1153 (comment) which simplifies it down to three lines. Shouldn't need any traits either. That approach is better than what I was hinting at above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants