Native Descriptor Wallets using DescriptorScriptPubKeyMan#16528
Native Descriptor Wallets using DescriptorScriptPubKeyMan#16528meshcollider merged 43 commits intobitcoin:masterfrom
Conversation
achow101
force-pushed
the
wallet-of-the-glorious-future
branch
from
acbd19c to
6f8afc0
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
Strong Concept ACK, this should be much nicer now it is preceded by the rework. |
achow101
force-pushed
the
wallet-of-the-glorious-future
branch
2 times, most recently
from
ef59fb7 to
90a631f
Compare
achow101
force-pushed
the
wallet-of-the-glorious-future
branch
2 times, most recently
from
ef63001 to
755c4d1
Compare
|
Approach ACK. It looks pretty straight forward and thanks to The Box feels cleaner than the previous attempt. I suggest that, after a bit more progress on #16341, we review this in parallel. That ensures that we don't mess up the division of labour between Also concept ACK on using BIP44/49/84 for new descriptor wallets. That may need some discussion, because it means individual addresses are no longer hardened. Some issues I found perusing the commits:
|
|
Concept ACK. @achow101 I just noticed that both the existing https://github.com/bitcoin/bitcoin/blob/master/src/rpc/misc.cpp#L215 |
achow101
force-pushed
the
wallet-of-the-glorious-future
branch
from
755c4d1 to
d777554
Compare
I guess so. Latest pushed should do that. |
achow101
force-pushed
the
wallet-of-the-glorious-future
branch
from
693c8cc to
84fb7d7
Compare
achow101
force-pushed
the
wallet-of-the-glorious-future
branch
3 times, most recently
from
655ea88 to
a208faa
Compare
achow101
force-pushed
the
wallet-of-the-glorious-future
branch
from
a208faa to
f581c8c
Compare
achow101
force-pushed
the
wallet-of-the-glorious-future
branch
2 times, most recently
from
f8edc40 to
69ad25c
Compare
achow101
force-pushed
the
wallet-of-the-glorious-future
branch
from
69ad25c to
98b7838
Compare
|
Due to your latest change upstream, this now complains: |
achow101
force-pushed
the
wallet-of-the-glorious-future
branch
from
98b7838 to
d80de7d
Compare
Co-authored-by: Andrew Chow <[email protected]>
When a CWallet doesn't have a ScriptPubKeyMan for the requested type in GetNewDestination, give a meaningful error. Also handle this in Qt which did not do anything with errors.
RPCOverloadWrapper overloads some deprecated or disabled RPCs with an implementation using other RPCs to avoid having a ton of code churn around replacing those RPCs.
Adds a --descriptors option globally to the test framework. This will make the test create and use descriptor wallets. However some tests may not work with this. Some tests are modified to work with --descriptors and run with that option in test_runer: * wallet_basic.py * wallet_encryption.py * wallet_keypool.py * wallet_keypool_topup.py * wallet_labels.py * wallet_avoidreuse.py
|
utACK 223588b (rebased, nits addressed)
I might be seeing two different issues. When I run the full test suite that particular test sometimes fails with |
|
Code review re-ACK 223588b. Rebuilt, re-ran all tests, bitcoind and a few |
|
re-ACK 223588b Changes were only rebase and addressing nits. FWIW, I did not see any failures from |
|
light re-ACK 223588b Read carefully through the descriptor-specific tests one more time, as well as the discussion since my last review. Admittedly a light re-review. Some more advanced use-cases may require additional tooling later and that's ok. |
meshcollider
left a comment
meshcollider
left a comment
There was a problem hiding this comment.
Code review ACK 223588b
Only lightly looked at the functional tests, but the actual code looks great. Thanks for carrying this through achow101. Great to have this in so early in the 0.21 release cycle too.
| throw std::runtime_error(std::string(__func__) + ": writing descriptor failed"); | ||
| } | ||
|
|
||
| // TopUp |
| KeyMap m_map_keys GUARDED_BY(cs_desc_man); | ||
| CryptedKeyMap m_map_crypted_keys GUARDED_BY(cs_desc_man); | ||
|
|
||
| bool SetCrypted(); |
There was a problem hiding this comment.
I don't think this should be here should it
|
People interested in this recently merged PR might be interested in reviewing the small follow-up PR #18782 ("wallet: Make sure no DescriptorScriptPubKeyMan or WalletDescriptor members are left uninitialized after construction") :) |
12 participants
Introducing the wallet of the glorious future (again): native descriptor wallets. With native descriptor wallets, addresses are generated from descriptors. Instead of generating keys and deriving addresses from keys, addresses come from the scriptPubKeys produced by a descriptor. Native descriptor wallets will be optional for now and can only be created by using
createwallet.Descriptor wallets will store descriptors, master keys from the descriptor, and descriptor cache entries. Keys are derived from descriptors on the fly. In order to allow choosing different address types, 6 descriptors are needed for normal use. There is a pair of primary and change descriptors for each of the 3 address types. With the default keypool size of 1000, each descriptor has 1000 scriptPubKeys and descriptor cache entries pregenerated. This has a side effect of making wallets large since 6000 pubkeys are written to the wallet by default, instead of the current 2000. scriptPubKeys are kept only in memory and are generated every time a descriptor is loaded. By default, we use the standard BIP 44, 49, 84 derivation paths with an external and internal derivation chain for each.
Descriptors can also be imported with a new
importdescriptorsRPC.Native descriptor wallets use the
ScriptPubKeyManinterface introduced in #16341 to add aDescriptorScriptPubKeyMan. This defines a different IsMine which uses the simpler model of "does this scriptPubKey exist in this wallet". Furthermore,DescriptorScriptPubKeyMandoes not have watchonly, so with native descriptor wallets, it is not possible to have a wallet with both watchonly and non-watchonly things. Rather a wallet withdisable_private_keysneeds to be used for watchonly things.A
--descriptoroption was added to some tests (wallet_basic.py,wallet_encryption.py,wallet_keypool.py,wallet_keypool_topup.py, andwallet_labels.py) to allow for these tests to use descriptor wallets. Additionally, several RPCs are disabled for descriptor wallets (importprivkey,importpubkey,importaddress,importmulti,addmultisigaddress,dumpprivkey,dumpwallet,importwallet, andsethdseed).