Remove blockchain from wallet #535

LLFourn · 2022-01-27T07:28:44Z

While trying to finish #490 I thought that it'd be better to try the idea of getting rid of a lot of the async macros and just having tow different traits for sync and async stuff. While trying to do that I felt that this needed to be done first.

The goal of this change is to decouple the wallet from the blockchain trait. A wallet is something that keeps track of UTXOs and transactions (and can sign things). The only reason it should need to talk to the blockchain is if doing a sync. So we remove all superfluous calls to the blockchain and precisely define the requirements for the blockchain to be used to sync with two new traits: WalletSync and GetHeight.

Stop requesting height when wallet is created
new_offline is just new now.
a WalletSync + GetHeight is now the first argument to sync.
SyncOptions replaces the existing options to sync and allows for less friction when making breaking changes in the fuutre (no more noop_progress!).
We Box the Progress now to avoid type parameters
broadcast has been removed from Wallet. You just use the blockchain directly to broadcast now.

Notes to the reviewers

This needs #502 before it can be merged but can reviewed now.
Be on the look up for stale documentation from this change.
Our doc build looks broken because of ureq or something.

Checklists

All Submissions:

I've signed all my commits
I followed the contribution guidelines
I ran cargo fmt and cargo clippy before committing
I've updated CHANGELOG.md

afilini

I think the code looks really good, but I haven't checked the documentation yet.

I will try to see if I can make it skip our dependencies in the doc build, it should also make it a lot faster.

notmandatory · 2022-02-03T16:26:26Z

I created #538 for the broken docs, there's a quick way to fix it but will lose a docs feature from nightly we use.

afilini · 2022-02-04T10:41:20Z

I was looking at #502, i figured we could make the SyncOptions struct a type defined by the WalletSync trait itself. The idea is that different backends might naturally have different options, for instance electrum and esplora could have an extra option to verify the transactions.

I don't think it would be too bad in terms of API, Wallet::sync would become something like:

pub fn sync<B: WalletSync + GetHeight>(
    &self,
    blockchain: &B,
    sync_opts: <B as WalletSync>::SyncOptions,
) -> Result<(), Error> {

LLFourn · 2022-02-05T22:00:09Z

@afilini It'd be nice to change it to a runtime flag. I think if we want to go that direction it'd be fine to have a flag that is present for all backends that you just ignore in the case of RPC.

Part of my more longer term goal here is to make it possible to Arc<dyn SyncWallet> around a blockchain eventually. Adding an associated type would prevent this.

notmandatory

Concept ACK plus a couple doc suggestions.

src/blockchain/any.rs

src/wallet/mod.rs

examples/compiler.rs

Although somewhat convenient to have, coupling the Wallet with the blockchain trait causes development friction and complexity. What if sometimes the wallet is "offline" (no access to the blockchain) but sometimes its online? The only thing the Wallet needs the blockchain for is to sync. But not all applications will even use the sync method and the sync method doesn't require the full blockchain functionality. So we instead pass the blockchain in when we want to sync. - To further reduce the coupling with blockchain I removed the get_height call from `new` and just use the height of the last sync in the database. - I split up the blockchain trait a bit into subtraits.

The current options are awkward and it would be good if we could introduce more in the future without breaking changes.

to make supporting verify_tx easier

For some reason while doing the "remove blockchain from wallet PR" I changed the tests around in what I thought was a benign way. But it meant (apparently) that both parties "test_sync_double_receive" were using the same "blockchain". This meant that when the blockchain was RPC they both imported their addresses to it and got each other's results when syncing. This bugged out the sync and this commit fixes that.

Co-authored-by: Steve Myers <[email protected]>

LLFourn · 2022-02-24T10:08:37Z

This is ready for final review and merge. I just pushed a few fixes:

get_tx has also been rolled into its own trait because it was convenient for fixing the tests for verify_tx. c0e75fc
I had to fix a bug in bitcoin rpc's sync which meant that if two wallets used the same blockchain they would get each other's transactions in their sync (because they both called loadwallet to the same backend). I'm not 100% clear why this wasn't happening before but it's fixed now. 035307e

icota · 2022-03-01T15:31:12Z

This PR makes perfect sense. I was somewhat unhappy with the fact online wallets can't be initalised offline or why there even was a distinction between the two.

FWIW I've rebased a project I'm working on on top of this and all the tests pass. Code looks good too. Thanks @LLFourn

tACK fbb50ad

LLFourn · 2022-03-03T02:17:15Z

Conflicts resolved. Ready for merge.

notmandatory

tACK d03aa85

Tested with bdk-cli (bitcoindevkit/bdk-cli#73) and all looks good.

notmandatory · 2022-03-04T05:08:01Z

@afilini do you want to give this a once over before it goes in?

You can do this with ensure_addresses_cached if you really want to.

LLFourn · 2022-03-07T00:06:31Z

Since this hadn't been merged yet I took the opportunity to remove max_addresses as a sync parameter because:

Nobody uses it and there are no tests for it.
My next work will hopefully get rid of the address caching in the sync pipeline except when doing a full sync for the first time.
We have a method to do this explicitly now with refactor wallet address caching into its own public method #537

CHANGELOG.md

afilini

Just a couple of minor things, the code looks good to me.

There was one CI job stuck and I've just restarted it, hopefully it was just a random timeout and not an issue with the test.

afilini · 2022-03-08T11:34:36Z

CHANGELOG.md

+
+- Removed `Blockchain` from Wallet.
+- Removed `Wallet::broadcast` (just use `Blockchain::broadcast`)
+- Depreciated `Wallet::new_offline` (all wallets are offline now)


Typo

Suggested change

- Depreciated `Wallet::new_offline` (all wallets are offline now)

- Deprecated `Wallet::new_offline` (all wallets are offline now)

afilini · 2022-03-08T11:42:26Z

src/wallet/mod.rs

-    D: BatchDatabase,
-{
-    // Return a newly derived address for the specified `keychain`.
+    // Return a newly derived address using the external descriptor


Is it always the external descriptor?

@notmandatory

0cc4700bd67be84bb5cb0814bf5c8aa9fc3f3cdc Fix typo in CHANGELOG and doc in wallet/mod.rs (Steve Myers) 660faab1e20defe86a7baf132e52928f538b9cbb Fix typo in CHANGELOG (LLFourn) 45767fcaf7c65d5020f99fdc35c147acd6e8d037 Remove max_addresses sync param (LLFourn) fbb50ad1c886a9cb716146fa823558d7d655c7b7 apply doc suggestions from @notmandatory (Lloyd Fournier) 035307ef54ebc330b1760bbdd17bdab3b167cfeb [rpc] Filter out unrelated transactions (LLFourn) ad0284b Split get_tx into its own trait (LLFourn) dcd90f8b61287164dae60f039848d3605eab8616 Restore but depreciate new_offline (LLFourn) 58a8250 Add SyncOptions as the second argument to Wallet::sync (LLFourn) 03d8f9d Remove Blockchain from wallet (LLFourn) Pull request description: While trying to finish #490 I thought that it'd be better to try the idea of getting rid of a lot of the async macros and just having tow different traits for sync and async stuff. While trying to do that I felt that this needed to be done first. The goal of this change is to decouple the wallet from the blockchain trait. A wallet is something that keeps track of UTXOs and transactions (and can sign things). The only reason it should need to talk to the blockchain is if doing a `sync`. So we remove all superfluous calls to the blockchain and precisely define the requirements for the blockchain to be used to sync with two new traits: `WalletSync` and `GetHeight`. 1. Stop requesting height when wallet is created 2. `new_offline` is just `new` now. 3. a `WalletSync + GetHeight` is now the first argument to `sync`. 4. `SyncOptions` replaces the existing options to `sync` and allows for less friction when making breaking changes in the fuutre (no more noop_progress!). 5. We `Box` the `Progress` now to avoid type parameters 6. broadcast has been removed from `Wallet`. You just use the blockchain directly to broadcast now. ### Notes to the reviewers This needs #502 before it can be merged but can reviewed now. Be on the look up for stale documentation from this change. Our doc build looks broken because of ureq or something. ### 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 * [x] I've updated `CHANGELOG.md` Top commit has no ACKs. Tree-SHA512: a8f3e21e45359be642b1f30d4ac1ba74785439e1b770dbeab0a5a4b8aab1eef4bb6b855aea4382e289257bc890fa713ca832a8b6c9655f7a59e96d412b4da3e6

notmandatory assigned LLFourn Feb 1, 2022

afilini reviewed Feb 2, 2022

View reviewed changes

notmandatory added this to the Release 0.17.0 Feature Freeze milestone Feb 8, 2022

notmandatory reviewed Feb 10, 2022

View reviewed changes

src/blockchain/any.rs Outdated Show resolved Hide resolved

src/wallet/mod.rs Outdated Show resolved Hide resolved

notmandatory mentioned this pull request Feb 11, 2022

Add wallet creation example using RPC #519

Merged

3 tasks

LLFourn commented Feb 16, 2022

View reviewed changes

examples/compiler.rs Show resolved Hide resolved

LLFourn mentioned this pull request Feb 18, 2022

Futures returned in library are not send #165

Closed

LLFourn force-pushed the remove-blockchain-from-wallet branch 2 times, most recently from deb3caf to 7d9e1f5 Compare February 22, 2022 23:54

notmandatory mentioned this pull request Feb 24, 2022

Reduce dependencies #474

Closed

LLFourn force-pushed the remove-blockchain-from-wallet branch from 7d9e1f5 to f1a5597 Compare February 24, 2022 09:35

LLFourn added 5 commits February 24, 2022 20:39

Add SyncOptions as the second argument to Wallet::sync

410a513

The current options are awkward and it would be good if we could introduce more in the future without breaking changes.

Restore but depreciate new_offline

dcd90f8

Split get_tx into its own trait

c0e75fc

to make supporting verify_tx easier

LLFourn force-pushed the remove-blockchain-from-wallet branch from f1a5597 to 035307e Compare February 24, 2022 09:39

apply doc suggestions from @notmandatory

fbb50ad

Co-authored-by: Steve Myers <[email protected]>

LLFourn mentioned this pull request Feb 24, 2022

refactor wallet address caching into its own public method #537

Merged

5 tasks

Merge branch 'master' into remove-blockchain-from-wallet

d03aa85

notmandatory approved these changes Mar 4, 2022

View reviewed changes

Remove max_addresses sync param

45767fc

You can do this with ensure_addresses_cached if you really want to.

thunderbiscuit reviewed Mar 7, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Fix typo in CHANGELOG

660faab

LLFourn force-pushed the remove-blockchain-from-wallet branch from 4e5329b to 660faab Compare March 8, 2022 03:00

afilini reviewed Mar 8, 2022

View reviewed changes

Fix typo in CHANGELOG and doc in wallet/mod.rs

0cc4700

notmandatory merged commit 3e4678d into bitcoindevkit:master Mar 9, 2022

This was referenced Mar 23, 2022

Expose more getters in Wallet and other useful descriptor traits #562

Merged

Upgrade for bdk v0.17.0 bitcoindevkit/bdk-cli#81

Closed

notmandatory mentioned this pull request Apr 14, 2022

no_std support #205

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove blockchain from wallet #535

Remove blockchain from wallet #535

LLFourn commented Jan 27, 2022

afilini left a comment

notmandatory commented Feb 3, 2022

afilini commented Feb 4, 2022 •

edited

Loading

LLFourn commented Feb 5, 2022

notmandatory left a comment

LLFourn commented Feb 24, 2022 •

edited

Loading

icota commented Mar 1, 2022

LLFourn commented Mar 3, 2022

notmandatory left a comment

notmandatory commented Mar 4, 2022

LLFourn commented Mar 7, 2022

afilini left a comment

afilini Mar 8, 2022

afilini Mar 8, 2022

	- Depreciated `Wallet::new_offline` (all wallets are offline now)
	- Deprecated `Wallet::new_offline` (all wallets are offline now)

Remove blockchain from wallet #535

Remove blockchain from wallet #535

Conversation

LLFourn commented Jan 27, 2022

Notes to the reviewers

Checklists

All Submissions:

afilini left a comment

Choose a reason for hiding this comment

notmandatory commented Feb 3, 2022

afilini commented Feb 4, 2022 • edited Loading

LLFourn commented Feb 5, 2022

notmandatory left a comment

Choose a reason for hiding this comment

LLFourn commented Feb 24, 2022 • edited Loading

icota commented Mar 1, 2022

LLFourn commented Mar 3, 2022

notmandatory left a comment

Choose a reason for hiding this comment

notmandatory commented Mar 4, 2022

LLFourn commented Mar 7, 2022

afilini left a comment

Choose a reason for hiding this comment

afilini Mar 8, 2022

Choose a reason for hiding this comment

afilini Mar 8, 2022

Choose a reason for hiding this comment

afilini commented Feb 4, 2022 •

edited

Loading

LLFourn commented Feb 24, 2022 •

edited

Loading