Multi-Keychain-Redo#1
Open
110CodingP wants to merge 4 commits into
Open
Conversation
110CodingP
force-pushed
the
multi_keychain_redo
branch
from
fa470d1 to
3b89212
Compare
The KeyRing<K> struct was introduced with the following methods: new, network, add_descriptor, add_multipath_descriptor, add_multipath_descriptor_with_range, into_params, list_keychains. The descriptors field within the KeyRing struct is specifically designed for O(1) membership checks. `KeyRing::add_descriptors` was intentionally not included because it is difficult to provide atomicity guarantees without introducing overly complicated logic. Also, while `KeyRing::add_multipath_with_range` is currently internal, it should be made `pub` in the future if we can incorporate a generic range parameter in an elegant manner.
110CodingP
force-pushed
the
multi_keychain_redo
branch
from
3b89212 to
ce9659d
Compare
Introduced `InitError` to report errors at the time of `Wallet` creation since `DescriptorError` does not report duplicate keychains case now. Also introduced `MissingKeychain` error to report the case when address APIs are called with an invalid keychain. `CreateParams::multi_path_descriptor_with_range` follows the logic of `KeyRing::add_multipath_descriptor_with_range`. Removed `ExternalAndInternalAreTheSame` variant from `Error` in `descriptor` in order to avoid propagating the generic to `IntoWalletDescriptor`. Also maintaining whether keychains are the same should be the concern of the `Wallet` level and not the descriptor level. The `from_v3` and `to_v3` APIs are for backwards and forward compatibility respectively such that the `load_from_params` does not need to deal with 2 separate types of `ChangeSets`. Users should call these APIs appropriately. `load_from_v3`, `load_from_v3_async`, `persist_to_v3`, and `persist_to_v3_async` have been added to `PersistedWallet` for users convenience. Refactor of export.rs is NOT COMPLETE. `Utxo` and `WeightedUtxo` were primarily used in `TxBuilder` contexts so they still use `KeychainKind`. Note we have decided that transaction building for multi-keychain `Wallet` will only happen through `create_psbt` API. `Merge` implementation for `ChangeSet` does not take into account the `descriptor` and `change_descriptor` fields because ideally `v4` completely ignores these fields. But `from_sqlite` and `persist_to_sqlite` still load and persist these two fields for backwards/forwards compatibility.
The tests related to signers are yet to be fixed completely.
The new example (electrum_v3) is just for testing purposes and should be removed from the final version of the commit.
110CodingP
force-pushed
the
multi_keychain_redo
branch
from
ce9659d to
a6160e2
Compare
|
Would you be able to do a quick write-up of the public API difference between this and bitcoindevkit#318? I got lost a bit between all the different ideas, and it'd be great to see the rough outline of what this competing option is proposing! |
Owner
Author
|
I added the most important differences in the description. Will keep updating the list and work on the presentation. Edit: Done! |
Owner
Author
|
I have been adding examples here. Added an example according to VM's suggestions that imitates Bitcoin Core's 
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Acc to VM's suggestions.
Notes to the reviewers
Particularly need review on
KeyRingand its APIsfrom_v3andto_v3(for backwards and forwards compatibility respectively) [tested with a db from v3, checkelectrum_v3example.]Mergeimplementation forChangeSet.schema_v2,from_sqliteandpersist_to_sqlite(inchangeset.rs)load_from_params(inwallet/mod.rs)Differences from bitcoindevkit#318 :
KeyRingis not a part of theWalletanymore.Rationale:
Most of the
WalletAPIs except those required for creating or loading theWalletdoes not have to do anything with the descriptors. The ones that need to are related to theindexer. Plus theindexerdoes contain the descriptors anyways so it is duplication. TheKeyRingis now used only to validate descriptors and has a methodinto_paramsthat converts it intoCreateParamspopulated withNetworkanddescriptors.There is no
KeyRing::ChangeSet.ChangeSetjust contains a newdescriptorsfield now.Rationale:
If I understood correctly then a goal of this new approach by VM was to reduce the amount of public API surface. And
KeyRing::ChangeSetis one such public type which is quite unecessary if theKeyRingis only used for creating theWallet.Also we donot remove
descriptorandchange_descriptorfields from theChangeSetin this PR.Rationale:
As VM highlighted, we need the two fields to be present because of backwards compatibility. Also providing backwards compatibility (and forwards) also forces us to handle cases when a user tries to load a v3 wallet. Since such cases only appear for
KeychainKindtype we need to have a separateload_from_changesetfunction for this type. To avoid this (kind of) I introducedfrom_v3(andto_v3). Also do take a look atMergeimpl forChangeSet, changed that too.KeyRingmisses a lot of old APIs. Seeking feedback for the APIs I should add/bring back.There is only one way to create the wallet.
KeyRing->(into_params)CreateParams->(create_wallet_no_persistorcreate_from_params)Wallet/PersistedWallet. Seeking feedback on other ways we would like to introduce here.KeyRingtries to actively avoid duplicate keychain or descriptor.Support For Arbitrary Number (1+) of Keychains bitcoindevkit/bdk_wallet#318 introduced a new sqlite table for the descriptors and network. Here we just create a new descriptors table and still try to
persist to/read fromthe oldbdk_wallettable.Rationale: This makes migration easy! See
electrum_v3example.We replace
KeyRingErrorbyInitErrorand minimise the change in v3Wallet's error types.Rationale: To decrease the amount of breaking changes.