-
Notifications
You must be signed in to change notification settings - Fork 390
[blockchain] Add traits to reuse Blockchain
s across multiple wallets
#569
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
[blockchain] Add traits to reuse Blockchain
s across multiple wallets
#569
Conversation
6a88c88
to
ae9c01d
Compare
I didn't like how I've also updated all these traits to inherit from I've added tests and documentation, I'm feeling confident to remove the draft state from this PR. |
e010c1c
to
a0cf1f6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM. I commented on some fixes needed in the rpc docs code, and a couple other suggestions.
06159e1
to
a817545
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK a817545
I think this would be really useful for multiple wallets to use the same backend.
Below are some nits and api questions I had..
dc53609
to
0657126
Compare
Rebased and addresses most comments |
de1ab13
to
6f91598
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK 6f91598
Code changes looks good to me. Now I understand the intention better. This will be really useful..
Just one non blocking nit. Can be done in a following PR.
This one needs one more rebase to pickup the new MSRV CI changes and then it should be ready to go. |
This allows using the `testuitils` macro in their tests as well
6f91598
to
6031e23
Compare
Add two new traits: - `StatelessBlockchain` is used to tag `Blockchain`s that don't have any wallet-specic state, i.e. they can be used as-is to sync multiple wallets. - `BlockchainFactory` is a trait for objects that can build multiple blockchains for different descriptors. It's implemented automatically for every `Arc<T>` where `T` is a `StatelessBlockchain`. This allows a piece of code that deals with multiple sub-wallets to just get a `&B: BlockchainFactory` to sync all of them. These new traits have been implemented for Electrum, Esplora and RPC (the first two being stateless and the latter having a dedicated `RpcBlockchainFactory` struct). It hasn't been implemented on the CBF blockchain, because I don't think it would work in its current form (it throws away old block filters, so it's hard to go back and rescan). This is the first step for bitcoindevkit#549, as BIP47 needs to sync many different descriptors internally. It's also very useful for #486.
6031e23
to
8795da4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK 8795da4
Description
Add three new traits:
StatelessBlockchain
is used to tagBlockchain
s that don't have any wallet-specic state, i.e. they can be used as-is to sync multiple wallets.StatefulBlockchain
is the opposite ofStatelessBlockchain
: it provides a method to "clone" aBlockchain
with an updated internal state (a new wallet checksum and, optionally, a different number of blocks to skip from genesis). Potentially this also allows reusing the underlying connection onBlockchain
types that support it.MultiBlockchain
is a generalization of this concept: it's implemented automatically for every type that implementsStatefulBlockchain
and for everyArc<T>
whereT
is aStatelessBlockchain
. This allows a piece of code that deals with multiple sub-wallets to just get a&B: MultiBlockchain
without having to deal with stateful and statless blockchains individually.These new traits have been implemented for Electrum, Esplora and RPC (the first two being stateless and the latter stateful). It hasn't been implemented on the CBF blockchain, because I don't think it would work in its current form (it throws away old block filters, so it's hard to go back and rescan).
This is the first step for #549, as BIP47 needs to sync many different descriptors internally.
It's also very useful for bitcoindevkit/bdk_wallet#188.
Notes to the reviewers
This is still a draft because:
Blockchain
instead of the less-restrictiveWalletSync
+GetHeight
which is the bare minimum to sync a walletChecklists
All Submissions:
cargo fmt
andcargo clippy
before committingNew Features:
CHANGELOG.md