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.

Sign up for GitHub

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

Jump to bottom

Reimplement Wallet, ElectrumExt and Esplora{Async}Ext with redesigned structures. #976

Merged
merged 17 commits into from
Jun 19, 2023
Merged

Reimplement Wallet, ElectrumExt and Esplora{Async}Ext with redesigned structures. #976

merged 17 commits into from
Jun 19, 2023

Conversation

evanlinjin
Copy link
Member

@evanlinjin evanlinjin commented May 11, 2023 •
edited
Loading

Description

Closes #938

  • Update Wallet to use redesigned structures.
  • Update bdk_electrum::ElectrumExt to produce updates for redesigned structures.
  • Update bdk_esplora::EsploraExt and bdk_esplora::EsploraAsyncExt to produce updates for redesigned structures.
  • Added example-crates/example_cli library for implementing examples with redesigned structures.
  • Added example-crate/example_electrum which is an electrum CLI wallet using the redesigned structures.
  • Updated example-crate/{wallet_electrum|wallet_esplora|wallet_esplora_async} examples to use redesigned structures.
  • Remove all old structures.

Notes to the reviewers

These changes bump our all-features MSRV to 1.60.0 because of the introduction of bdk_esplora. As long as the bdk_chain and bdk_wallet crates hit a MSRV of 1.48.0, it will be fine (this work is done in #987). No longer needed due to #993

I had to comment out the examples that use Wallet with our chain sources. Once we update the helper-packages for those chain sources, we can also update the examples.

Possible future improvements for ElectrumExt:

  • Remove requirement to retry obtaining ALL data after reorg is detected. Transactions can be anchored to a lower block (not block tip), and an assume_final_depth value can be used.

  • The logic to finalize an update with confirmation time can be improved during reorgs to not require returning an error.

  • Use the subscription model of electrum, as intended by the API.

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

@evanlinjin evanlinjin changed the title Reimplement Wallet with redesinged structures. Reimplement Wallet with redesigned structures. May 11, 2023
LLFourn
LLFourn requested changes May 11, 2023
View reviewed changes
Copy link
Contributor

@LLFourn LLFourn left a comment

Choose a reason for hiding this comment

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

Design looks good. I think we should try and avoid adding another argument to insert_tx on Wallet.

@nondiremanuel nondiremanuel added this to the 1.0.0-alpha.1 milestone May 11, 2023
@evanlinjin evanlinjin changed the title Reimplement Wallet with redesigned structures. Reimplement Wallet and ElectrumExt with redesigned structures. May 11, 2023
@evanlinjin evanlinjin requested a review from LLFourn May 12, 2023 09:46
LLFourn
LLFourn requested changes May 15, 2023
View reviewed changes
Copy link
Contributor

@LLFourn LLFourn left a comment

Choose a reason for hiding this comment

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

The examples are nice.

Instead of commenting out all these files can you just remove them from workspace for a while?

danielabrozzoni
danielabrozzoni reviewed May 16, 2023
View reviewed changes
Copy link
Member

@danielabrozzoni danielabrozzoni left a comment •
edited
Loading

Choose a reason for hiding this comment

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

A few comments, I'm still not done reviewing electrum v2. I agree with lloyd that the old electrum example files should be deleted :)

@evanlinjin evanlinjin mentioned this pull request May 17, 2023
Closed
@evanlinjin
Copy link
Member Author

Oh yeah I still need to remove the commented-out examples.

@evanlinjin evanlinjin changed the title Reimplement Wallet and ElectrumExt with redesigned structures. Reimplement Wallet, ElectrumExt and Esplora{Async}Ext with redesigned structures. May 18, 2023
@evanlinjin evanlinjin force-pushed the wallet_redesign branch 3 times, most recently from 2f1347b to 2e56f1b Compare May 18, 2023 06:28
LLFourn
LLFourn reviewed May 23, 2023
View reviewed changes
@evanlinjin evanlinjin mentioned this pull request May 23, 2023
Closed
19 tasks
@evanlinjin evanlinjin force-pushed the wallet_redesign branch 3 times, most recently from b0c5552 to d157061 Compare May 24, 2023 04:58
evanlinjin and others added 15 commits June 4, 2023 03:32
@evanlinjin
Add last_seen to the the ConfirmationTime::Unconfirmed variant
7261669 
This allows us to skip adding an extra input to `Wallet::insert_tx`.

Also remove redundant logic.
@evanlinjin
[electrum_redesign] Introduce redesigned ElectrumExt
2e3cee4 
There are a number of improvements that can be done, but it is in a
decent state to be usable.

Possible improvements:

* Remove requirement to retry obtaining ALL data after reorg is
  detected. Transactions can be anchored to a lower block (not block
  tip), and an `assume_final_depth` value can be used.

* The logic to finalize an update with confirmation time can be improved
  during reorgs to not require returning an error.
@evanlinjin
[examples_redesign] Introduce example_cli package
f55974a 
This is the equivalent of `keychain_tracker_example_cli` that works with
the redesigned structures.
@evanlinjin
[examples_redesign] Implemented example_electrum
6a1ac7f 
This is a version of `keychain_tracker_electrum` that uses the
redesigned structures instead.
@LLFourn @evanlinjin
[example-cli] simplify new address logic
a78967e
@evanlinjin
Introduce keychain::LocalChangeSet
50425e9 
This corresponds to `keychain::KeychainChangeSet` but for the redesigned
structures with `LocalChain`.

This structure is now used in `Wallet` as well as the examples.
@evanlinjin
Various tweaks to code arrangement and documentation
92709d0 
As per reviews by @danielabrozzoni and @LLFourn
@evanlinjin
bdk_electrum API improvements and simplifications
78a7920 
* `ElectrumUpdate::missing_full_txs` now returns a `Vec<Txid>` so we
  don't keep a reference to the passed-in `graph`.

* `ElectrumUpdate::finalize*` methods now takes in `missing` txids
  instead of `full_txs`. `Client::batch_transaction_get` is called
within the methods.

Other changes:

* `wallet::ChangeSet` is now made public externally. This is required as
  a wallet db should implement `PersistBackend<wallet::ChangeSet>`.
@evanlinjin
Update the wallet_electrum example
2952341
@evanlinjin
Implement redesigned versions of EsploraExt and EsploraAsyncExt
5860704 
All associated examples are also updated.
@evanlinjin
[chain_redesign] Remove old structures
1c3cbef 
Other changes:

* The `async-https` feature of `bdk_esplora` is no longer default.
* Rename `ObservedAs` to `ChainPosition`.
* Set temporary MSRV to 1.60.0 to compile all workspace members will all
  features.
@Shourya742 @evanlinjin
Rename fields of tx_graph::Additions
ac80829 
* Changed `tx` to `txs`
* Changed `txout` to `txouts`
@evanlinjin
[wallet_redesign] Modified insert_tx to use lowest checkpoint
a7668a2 
Also updated the documentation.
@evanlinjin
[wallet_redesign] Clean up and document address methods
cff9211
@evanlinjin
Update documentation
75f8b81 
* Add `warn(missing_docs)` for `bdk_wallet` and `bdk_chain`.
* Add missing documentation.
* Remove `LocalChain::heights` method.
* Remove old TODOs.
This was referenced Jun 6, 2023
Closed
Closed
Merged
danielabrozzoni
danielabrozzoni reviewed Jun 16, 2023
View reviewed changes
Copy link
Member

@danielabrozzoni danielabrozzoni left a comment

Choose a reason for hiding this comment

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

I finished reviewing electrum as well, I haven't really looked into electrum_ext as I had troubles figuring out what was happening, I left a few comments asking for more comments (lol)

It's probably obvious at this point but I really think it would have helped w/ reviewing and updating if this whole PR was split in at least 3 or 4 smaller ones (wallet, esplora, electrum, example cli). Something to keep in mind for next time!

example-crates/example_cli/src/lib.rs
pub command: Commands<S>,
}

#[allow(clippy::almost_swapped)]
Copy link
Member

Choose a reason for hiding this comment

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

Weird, I don't have warnings for clippy on cargo 1.69 nor 1.57 🤷🏻‍♀️

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it was caused by this regression (rust-lang/rust-clippy#10421) which is fixed in the latest version for cargo clippy. Will remove.

example-crates/example_cli/src/lib.rs
@@ -0,0 +1,736 @@
pub use anyhow;
Copy link
Member

Choose a reason for hiding this comment

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

This whole file would be way easier to review if there was one move-only commit, followed by commits changing stuff, if needed. In this way, I could just review the commits changing the logic. Just something to keep in mind for next time!

Copy link
Member Author

Choose a reason for hiding this comment

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

Apologies. Will keep in mind for next time.

example-crates/example_electrum/src/main.rs
.blocks()
.iter()
.rev()
.take(ASSUME_FINAL_DEPTH)
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit lost. It seems that you're taking the latest ASSUME_FINAL_DEPTH blocks. Why? Can you add a comment in the code explaining what's happening?

Copy link
Member Author

Choose a reason for hiding this comment

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

Will add a comment. Basically anything as deep or deeper than ASSUME_FINAL_DEPTH will not be reorged out (based on our assumption). This is essentially the minimum checkpoints from the tip to include (from the original chain) to guarantee that we will find a point of agreement.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that #1002 is removing this ASSUME_FINAL_DEPTH thing.

crates/electrum/src/electrum_ext.rs
.collect()
}

pub fn finalize(
Copy link
Member

Choose a reason for hiding this comment

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

A comment explaining what it means to finalize an update would be helpful

crates/electrum/src/electrum_ext.rs

impl ElectrumExt<ConfirmationHeightAnchor> for Client {
fn get_tip(&self) -> Result<(u32, BlockHash), Error> {
// TODO: unsubscribe when added to the client, or is there a better call to use here?
Copy link
Member

Choose a reason for hiding this comment

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

What does it mean to "unsubscribe when added to the client"?

Copy link
Member Author

Choose a reason for hiding this comment

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

The electrum API has a subscription model. We are subscribing to new tips coming in (we get the first tip), but we don't process later tips, so we should unsubscribe from receiving more tips.

Copy link
Member

Choose a reason for hiding this comment

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

The electrum API has a subscription model. We are subscribing to new tips coming in (we get the first tip), but we don't process later tips, so we should unsubscribe from receiving more tips.

Yep, but why "when added to the client" - unsubscribe when what is added to the client, the first tip?

crates/electrum/src/electrum_ext.rs
}
}

fn populate_with_outpoints<K>(
Copy link
Member

Choose a reason for hiding this comment

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

Pls put a comment explaining what this method does. What is being populate with outpoints? The update?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the update. We already have a comment on scan explaining the outpoints input though?

crates/electrum/src/electrum_ext.rs
None => continue,
};
// attempt to find the following transactions (alongside their chain positions), and
// add to our sparsechain `update`:
Copy link
Member

Choose a reason for hiding this comment

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

not a sparsechain anymore :)

crates/electrum/src/electrum_ext.rs
}
}

// Insert the new tip so new transactions will be accepted into the sparsechain.
Copy link
Member

Choose a reason for hiding this comment

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

Not a sparsechain anymore

crates/electrum/src/electrum_ext.rs
.map(|data| (data.height as u32, data.header.block_hash()))
}

fn scan<K: Ord + Clone>(
Copy link
Member

Choose a reason for hiding this comment

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

I think this whole method needs comments explaining what's going on

LLFourn
LLFourn approved these changes Jun 19, 2023
View reviewed changes
Copy link
Contributor

@LLFourn LLFourn left a comment

Choose a reason for hiding this comment

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

ACK 75f8b81

I think the way to resolve the remaining discussion here is to make issues for them if the concerns still exist after #1002

example-crates/example_electrum/src/main.rs
.rev()
.take(ASSUME_FINAL_DEPTH)
.map(|(k, v)| (*k, *v))
.collect::<BTreeMap<u32, BlockHash>>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Made issue about the ergonomics here: #997

example-crates/example_electrum/src/main.rs
.blocks()
.iter()
.rev()
.take(ASSUME_FINAL_DEPTH)
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that #1002 is removing this ASSUME_FINAL_DEPTH thing.

danielabrozzoni
danielabrozzoni approved these changes Jun 19, 2023
View reviewed changes
Copy link
Member

@danielabrozzoni danielabrozzoni left a comment

Choose a reason for hiding this comment

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

Partial ACK 75f8b81 - the Wallet code looks good to me, I don't have a good enough understanding of the esplora/electrum code to confidently ACK it.

@danielabrozzoni danielabrozzoni merged commit 97d542c into bitcoindevkit:master Jun 19, 2023
@vincenzopalazzo vincenzopalazzo mentioned this pull request Jun 19, 2023
Closed
This was referenced Jun 19, 2023
Closed
Closed
@evanlinjin evanlinjin mentioned this pull request Jun 22, 2023
Closed
3 tasks
@vladimirfomene vladimirfomene mentioned this pull request Jun 27, 2023
Closed
@notmandatory notmandatory mentioned this pull request Jul 4, 2023
Closed
24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@notmandatory notmandatory notmandatory left review comments

@LLFourn LLFourn LLFourn approved these changes

@danielabrozzoni danielabrozzoni danielabrozzoni approved these changes

@afilini afilini Awaiting requested review from afilini

@rajarshimaitra rajarshimaitra Awaiting requested review from rajarshimaitra

@vladimirfomene vladimirfomene Awaiting requested review from vladimirfomene

Assignees

@evanlinjin evanlinjin

Labels
None yet
Projects
BDK Wallet
Archived in project
Milestone
1.0.0-alpha.1
Development

Successfully merging this pull request may close these issues.

Update Wallet to work with IndexedTxGraph
7 participants
@evanlinjin @notmandatory @LLFourn @vladimirfomene @danielabrozzoni @nondiremanuel @Shourya742