achow101
commented at 2:01 am on August 2, 2019:
member
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 importdescriptors RPC.
Native descriptor wallets use the ScriptPubKeyMan interface introduced in #16341 to add a DescriptorScriptPubKeyMan. This defines a different IsMine which uses the simpler model of “does this scriptPubKey exist in this wallet”. Furthermore, DescriptorScriptPubKeyMan does 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 with disable_private_keys needs to be used for watchonly things.
A --descriptor option was added to some tests (wallet_basic.py, wallet_encryption.py, wallet_keypool.py, wallet_keypool_topup.py, and wallet_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, and sethdseed).
fanquake added the label
Wallet
on Aug 2, 2019
fanquake added this to the "PRs" column in a project
achow101 force-pushed
on Aug 2, 2019
DrahtBot
commented at 3:23 am on August 2, 2019:
member
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Conflicts
Reviewers, this pull request conflicts with the following ones:
#18774 (test: added test for upgradewallet RPC by brakmic)
#18727 (test: Add CreateWalletFromFile test by ryanofsky)
#18699 (wallet: Avoid translating RPC errors by MarcoFalke)
#18654 (rpc: separate bumpfee’s psbt creation function into psbtbumpfee by achow101)
#18618 (gui: Drop RecentRequestsTableModel dependency to WalletModel by promag)
#18617 (test: add factor option to adjust test timeouts by brakmic)
#18608 (refactor: Remove CAddressBookData::destdata by ryanofsky)
#18570 (rpc: return block hash & height in getbalances, gettransaction & getwalletinfo JSONs by brakmic)
#18479 (RPC: Show fee in results for signrawtransaction* for segwit inputs by luke-jr)
#18244 (rpc: fundrawtransaction and walletcreatefundedpsbt respect locks even with manual coin selection by Sjors)
#17977 ([WIP] Implement BIP 340-342 validation (Schnorr/taproot/tapscript) by sipa)
#17877 (qt, refactor: Make enums in BitcoinUnits class scoped by hebasto)
#16463 ([BIP 174] Implement serialization support for GLOBAL_XPUB field. by achow101)
#16432 (qt: Add privacy to the Overview page by hebasto)
#16224 (gui: Bilingual GUI error messages by hebasto)
#11413 ([wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option by kallewoof)
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.
meshcollider
commented at 3:25 am on August 2, 2019:
contributor
Strong Concept ACK, this should be much nicer now it is preceded by the rework.
achow101 force-pushed
on Aug 2, 2019
achow101 force-pushed
on Aug 2, 2019
achow101 force-pushed
on Aug 2, 2019
achow101 force-pushed
on Aug 2, 2019
Sjors
commented at 8:23 am on August 3, 2019:
member
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 ScriptPubKeyMan and its subclasses.
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:
when you restart bitcoind and load a (watch-only) wallet the keypool is empty and getnewaddress no longer works
importdescriptors should no longer require a range argument (also I would prefer a singular RPC nvm that makes rescan slow)
when calling getnewaddress with an address type for which the wallet misses a descriptor, it returns a blank error message:
can you give each ScriptPubKeyManager it’s own file?
deleting GetMetadata and Upgrade inside Introduce WalletDescriptor; should have its own commit?
ScriptPubKeyMap could be introduced in Implement IsMine instead of the earlier commit. Would it make sense to move this into the Descriptor class? How are infinite range descriptors handled, expanded and cached keypoolsize items at a time?
Am I reading this wrong or is SetupGeneration creating a fresh seed for each descriptor?
I’m not a fan of determining the output type in an indirect manner by expanding the descriptor Optional<OutputType> out_script_type = DetermineOutputType(scripts_temp[0], out_keys);; that information is already encoded in the descriptor. Shameless plug for #15590.
fanquake added the label
Descriptors
on Aug 4, 2019
hugohn
commented at 2:57 pm on August 12, 2019:
contributor
Concept ACK.
@achow101 I just noticed that both the existing deriveaddressses & importmulti commands treat descriptor [range_start, range_end] as inclusive, which is consistent with the common understanding of the notation []. Should we update the new importdescriptors command to do the same (make range_end inclusive)?
achow101
commented at 4:20 pm on August 12, 2019:
member
Should we update the new importdescriptors command to do the same (make range_end inclusive)?
I guess so. Latest pushed should do that.
achow101 force-pushed
on Aug 14, 2019
achow101 force-pushed
on Sep 4, 2019
achow101 force-pushed
on Sep 4, 2019
achow101 force-pushed
on Sep 4, 2019
achow101 force-pushed
on Sep 4, 2019
achow101 force-pushed
on Sep 17, 2019
achow101 force-pushed
on Sep 18, 2019
achow101 force-pushed
on Oct 10, 2019
Sjors
commented at 4:50 pm on October 10, 2019:
member
Due to your latest change upstream, this now complains: scriptpubkeyman.h:516:10: warning: 'CheckDecryptionKey' overrides a member function but is not marked 'override'
achow101 force-pushed
on Oct 10, 2019
achow101
commented at 7:13 pm on October 10, 2019:
member
Fixed the warning.
achow101 force-pushed
on Oct 14, 2019
achow101 force-pushed
on Oct 15, 2019
achow101 force-pushed
on Oct 25, 2019
achow101 force-pushed
on Oct 26, 2019
achow101 force-pushed
on Oct 29, 2019
achow101 force-pushed
on Oct 29, 2019
achow101 force-pushed
on Nov 4, 2019
Sjors
commented at 11:18 am on November 5, 2019:
member
Rescanning is broken for (some?) descriptor wallets, both when importing descriptors and when calling rescanblockchain. It does detect new transactions. Example:
Also note the address is incorrectly marked as ischange (but the derivation is correct). Might be unrelated though; the wallet determines IsChange() by checking if it’s in the address book (yuck).
Update: I can no longer reproduce this (d7ca9abe4b817913852abbd48082bde64df8e9d9)
achow101 force-pushed
on Nov 6, 2019
achow101 force-pushed
on Nov 7, 2019
achow101 force-pushed
on Nov 7, 2019
achow101 force-pushed
on Nov 7, 2019
achow101 force-pushed
on Dec 6, 2019
achow101 force-pushed
on Dec 13, 2019
achow101 force-pushed
on Jan 6, 2020
achow101 force-pushed
on Jan 6, 2020
achow101 force-pushed
on Jan 17, 2020
Sjors
commented at 6:53 pm on January 20, 2020:
member
Unfortunately this test passes, so I can’t reproduce the bug I was chasing just yet. I was seeing an origin-pubkey mismatch for sortedmulti() descriptors.
Update: fairly certain I was chasing a ghost. Hopefully the tests are useful.
achow101
commented at 6:29 pm on January 21, 2020:
member
I could not replicate any bugs with sortedmulti descriptors.
achow101 force-pushed
on Jan 30, 2020
achow101 marked this as ready for review
on Jan 30, 2020
achow101 renamed this:
[WIP] Native Descriptor Wallets (take 2)
Native Descriptor Wallets using DescriptorScriptPubKeyMan
on Jan 30, 2020
achow101
commented at 5:00 am on January 30, 2020:
member
Rebased onto master, now ready for review.
achow101 force-pushed
on Jan 30, 2020
Sjors
commented at 12:38 pm on January 30, 2020:
member
Concept and approach re-ACK
I rebased my HWI support PRs on top of this: #16546 (RPC) and #16549 (GUI).
The first commit could go to an independent PR:
Output a descriptor in createmultisig
And a few that could be done before this PR, just to get the number of commits down a bit :-)
Introduce SetType function to tell ScriptPubKeyMans the type and internal-ness of it (empty for legacy)
Change wallet_encryption.py to use signmessage instead of dumpprivkey
d3819b681f766a17deef4650bd6297178924f04b Change GetMetadata to use unique_ptr<CKeyMetadata>
c80e25d8743d7e5bfe866d32bf394cf55166bde5 Add a function to determine the OutputType of a scriptPubKey (DetermineOutputType should be contrasted to getting this information directly from the descriptor like in #15590)
0fc666f32ff8472215e05e85dd609901a02dc3ae Add IsLegacy to CWallet so that the GUI knows whether to show watchonly
I’m ambivalent about using BIP44/49/84 for new descriptor wallets. Downside is that individual addresses are no longer hardened. Upside is that this many other wallets use this standard and we could even consider BIP39 backup at some point (#17748). Perhaps createwallet (and the GUI) could make the use of BIP44/49/84 optional.
importdescriptors should no longer require a range argument
importdescriptor could be a singular RPC (less tedious when used manually). It could drop the timestamp argument in favor of just calling rescanblockchain after import.
I think we should split scriptpubkeyman.{h,cpp} into scriptpubkeyman.{h,cpp} , legacy_scriptpubkeyman.{h,cpp} and descriptor_scriptpubkeyman.{h,cpp} (although meh because it creates large diff, but maybe better to get it over with).
in
src/rpc/misc.cpp:86
in
dd92677895outdated
82@@ -83,6 +83,7 @@ static UniValue createmultisig(const JSONRPCRequest& request)
83 "{\n"
84 " \"address\":\"multisigaddress\", (string) The value of the new multisig address.\n"
85 " \"redeemScript\":\"script\" (string) The string value of the hex-encoded redemption script.\n"
86+ " \"descriptor\":\"descriptor\" (string) The descriptor for the P2SH address for this multisig\n"
The new dummy class in 3f6cbc5bb16f8a021bbe88d998b12698205aaeac introduces several compiler problems (rebase slippage?), e.g:
0./wallet/scriptpubkeyman.h:483:41: error: non-virtual member function marked 'override' hides virtual member function
1 void KeepDestination(int64_t index) override;
achow101
commented at 10:33 pm on January 30, 2020:
Since this gets dropped later, dropped this at the dummy class definition.
Sjors
commented at 2:42 pm on January 30, 2020:
member
I reviewed the first ~5 commits until e40a833c3bffb0cf723357238eb809398e3e5237Create LegacyScriptPubKeyMan when not a descriptor wallet, but started to run into compiler issues, so will continue later. Found one nit too:
laanwj added this to the "Blockers" column in a project
achow101 force-pushed
on Jan 30, 2020
achow101
commented at 10:34 pm on January 30, 2020:
member
Moved Output a descriptor in createmultisig to it’s own PR and dropped it from here since it isn’t actually used in descriptor wallets.
I will investigate breaking out the other commits.
* I'm ambivalent about using BIP44/49/84 for new descriptor wallets. Downside is that individual addresses are no longer hardened. Upside is that this many other wallets use this standard and we could even consider BIP39 backup at some point (#17748). Perhaps `createwallet` (and the GUI) could make the use of BIP44/49/84 optional.
AFAICT, the reasoning for using exclusively hardened derivation is because we could export private keys with dumpprivkey and that has issues. But since that is disabled for descriptor wallets, I think it’s fine.
* `importdescriptors` should no longer require a range argument
Why?
* `importdescriptor` could be a singular RPC (less tedious when used manually). It could drop the `timestamp` argument in favor of just calling `rescanblockchain` after import.
I suppose, but I don’t really want there to be multiple commands that do fundamentally the same thing.
I think we should split scriptpubkeyman.{h,cpp} into scriptpubkeyman.{h,cpp} , legacy_scriptpubkeyman.{h,cpp} and descriptor_scriptpubkeyman.{h,cpp} (although meh because it creates large diff, but maybe better to get it over with).
Meh.
achow101 closed this
on Jan 30, 2020
achow101 reopened this
on Jan 30, 2020
Sjors
commented at 10:35 am on January 31, 2020:
member
Descriptors can be expanded indefinitely, so why specify a (mandatory) range on import? It might make sense as an optional field to set a boundary, e.g. if you know the external signing device has a range limit.
AFAICT, the reasoning for using exclusively hardened derivation is because we could export private keys with dumpprivkey and that has issues. But since that is disabled for descriptor wallets, I think it’s fine.
I’m not familiar with the history. Another reason could be that if one private key is compromised in a side-channel attack, the other keys are still safe. But that assumes such a side-channel attack, couldn’t also compromise the master key. Or perhaps a (horrible) bug in the signing code causes a private key to leak on chain. Or someone wants to reenable dumpprivkey again to redeem some fork coin. Using m/{0,1,2}'/{0,1}'/k by default and making BIP44/49/84 opt-in (in a followup) seems less of change from status quo.
I don’t really want there to be multiple commands that do fundamentally the same thing.
Do you mean having similar commands (importmulti and importdescriptors) with different options? I like consistency, as do developers who want a simple path to switch their integration over to descriptor wallets. But I also like the rare opportunity to clean up the RPC. And when people have to copy-paste some magic to import keys from a hardware wallet, this new syntax looks less intimidating. Though ideally that shouldn’t be needed at all.
The AppVeyor failure might be legit wallet_importdescriptors.py encounters JSONRPCException: bad-txns-inputs-missingorspent (-25), albeit odd.
In f83fde4e8e13a254083e9433dfa770a8b52b1053Create LegacyScriptPubKeyMan when not a descriptor wallet you’re also introducing class WalletDescriptor; that was supposed to be a separate commit?
You’re serialising the descriptor as a string (in WalletDescriptor). That’s fine with me, I’d rather not delay this work, and we can change the serialisation later. However there’s been some mailinglist discussion about serialising descriptors into something that’s easier to copy/paste and put in a QR code. Maybe that’s worth working out in a separate PR and then to use that for the wallet as well. Added a an issue for it #18043.
in
src/wallet/scriptpubkeyman.h:474
in
ee6980be43outdated
467@@ -467,4 +468,49 @@ class LegacySigningProvider : public SigningProvider
468 bool GetKeyOrigin(const CKeyID& keyid, KeyOriginInfo& info) const override { return m_spk_man.GetKeyOrigin(keyid, info); }
469 };
470471+class DescriptorScriptPubKeyMan : public ScriptPubKeyMan
472+{
473+public:
474+ using ScriptPubKeyMan::ScriptPubKeyMan;
In ee6980be437c09df6044a5ced7caa91410453027Introduce DescriptorScriptPubKeyMan as a dummy class: this line (using ScriptPubKeyMan::ScriptPubKeyMan;) is dropped in 7af1a569567a595dc26c33075632f406c4ff0904.
achow101
commented at 6:47 pm on February 11, 2020:
That’s intentional. The using ScriptPubKeyMan::ScriptPubKeyMan; is needed to use the default ScriptPubKeyMan constructor until we add a custom one for DescriptorScriptPubKeyMan.
nit: these are added, but not used, in 7af1a569567a595dc26c33075632f406c4ff0904Store WalletDescriptor in DescriptorScriptPubKeyMan which seems out of the blue. b784a150b23e10ee794d1d2616a0df5c0f317fef seems a better place.
achow101
commented at 7:35 pm on February 11, 2020:
Moved
instagibbs
commented at 8:16 pm on March 20, 2020:
in
src/wallet/scriptpubkeyman.h:476
in
7af1a56956outdated
469@@ -470,8 +470,17 @@ class LegacySigningProvider : public SigningProvider
470471 class DescriptorScriptPubKeyMan : public ScriptPubKeyMan
472 {
473+private:
474+ WalletDescriptor descriptor GUARDED_BY(cs_desc_man);
475+
476+ using ScriptPubKeyMap = std::map<CScript, uint32_t>; // Map of scripts to descriptor range index
ee6980be437c09df6044a5ced7caa91410453027: void RewriteDB() override; is dropped in fd995af379b73b63b78b276b5b8f31d39b2d2b67
achow101
commented at 7:36 pm on February 11, 2020:
Dropped
Sjors
commented at 6:11 pm on January 31, 2020:
member
Code reviewed the first 15 commits up to fd995af379b73b63b78b276b5b8f31d39b2d2b67, looks pretty good.
Regarding private key serialisation:
In d6f0c337f5bbf935cd93ed3884f5c10bcaa5d493Implement loading of keys for DescriptorScriptPubKeyMan the comment // hash pubkey/privkey to accelerate wallet load, is explained elsewhere:
0 // Old wallets store keys as DBKeys::KEY [pubkey] => [privkey]
1 // ... which was slow for wallets with lots of keys, because the public key is re-derived from the private key
2 // using EC operations as a checksum.
3 // Newer wallets store keys as DBKeys::KEY [pubkey] => [privkey][hash(pubkey,privkey)], which is much faster while
4 // remaining backwards-compatible.
There’s no need to be backwards-compatible. I haven’t thought about this scheme deeply, but can we improve it?
Do we want to use a descriptor cache for all private keys, like we do public keys? And then checksum the entire cache instead of individual keys?
For descriptors without hardened derivation, should we refrain from storing private keys at all? (including for encrypted wallets, since we can derive from seed after decryption)
Update: see IRC wallet meeting log of today, I tripped over the name here; these are apparently master keys.
jonatack
commented at 7:59 pm on January 31, 2020:
member
DrahtBot added the label
Needs rebase
on Feb 4, 2020
MarcoFalke referenced this in commit
75fb37ce68
on Feb 9, 2020
achow101 force-pushed
on Feb 11, 2020
DrahtBot removed the label
Needs rebase
on Feb 11, 2020
achow101 force-pushed
on Feb 11, 2020
achow101 force-pushed
on Feb 11, 2020
achow101 force-pushed
on Feb 12, 2020
achow101
commented at 0:37 am on February 12, 2020:
member
I’ve rebased this on top of #18115 and #18034. I’ll start addressing other issues tomorrow.
I believe I’ve fixed the random failure of wallet_importdescriptors.py as well.
achow101 force-pushed
on Feb 12, 2020
laanwj removed this from the "Blockers" column in a project
achow101 force-pushed
on Feb 14, 2020
achow101 force-pushed
on Feb 14, 2020
achow101 force-pushed
on Feb 14, 2020
achow101
commented at 8:16 pm on February 14, 2020:
member
I’ve removed the requirement for range in importdescriptors. It will give a warning and use the default keypool range.
sidhujag referenced this in commit
c71c448328
on Feb 18, 2020
in
src/wallet/scriptpubkeyman.cpp:2004
in
91d4efc842outdated
1999+ SignPSBTInput(HidingSigningProvider(keys.get(), !sign, !bip32derivs), psbtx, i, sighash_type);
2000+ }
2001+
2002+ // Fill in the bip32 keypaths and redeemscripts for the outputs so that hardware wallets can identify change
2003+ for (unsigned int i = 0; i < psbtx.tx->vout.size(); ++i) {
2004+ std::unique_ptr<FlatSigningProvider> keys = GetSigningProvider(psbtx.tx->vout.at(i).scriptPubKey, true);
I figured out what confused me. I was trying to call the Descriptor SKPMan version of FillPSBT() but that doesn’t include the UTXOs, which are normally added in CWallet. I overhauled my design to avoid that problem.
achow101 force-pushed
on Feb 20, 2020
achow101 force-pushed
on Feb 21, 2020
instagibbs
commented at 2:57 pm on February 24, 2020:
member
We should make IsChange check for derivation path for descriptor wallets and not the ad-hoc “is this address in my address book”, which we have to support for legacy wallets forever.
edit: nevermind, not derivation path, just whether or not it’s in m_internal_spk_mans or not
DrahtBot added the label
Needs rebase
on Feb 25, 2020
achow101 force-pushed
on Feb 25, 2020
DrahtBot removed the label
Needs rebase
on Feb 25, 2020
achow101 force-pushed
on Feb 25, 2020
achow101 force-pushed
on Feb 27, 2020
achow101
commented at 1:46 am on February 27, 2020:
member
We still cache individual keys for /*h descriptors, but they’re indexed rather than sequential, so the question remains here.
achow101
commented at 8:35 pm on February 28, 2020:
Yes, it is no longer needed. I’ve removed it.
in
src/wallet/scriptpubkeyman.cpp:1691
in
1b4fd1424foutdated
1685+ // Add all of the scriptPubKeys to the scriptPubKey set
1686+ for (const CScript& script : scripts_temp) {
1687+ m_map_script_pub_keys[script] = i;
1688+ }
1689+ // Write the cache
1690+ std::map<KeyOriginInfo, CExtPubKey> newly_cached = descriptor.cache.GetNotCached(cache.GetCachedExtPubKeys());
Now that you’re no longer caching individual keys, can there still be a situation where you have a non-empty cache that’s incomplete? If not, then you might be able to drop GetNotCached.
instagibbs
commented at 6:20 pm on February 28, 2020:
I don’t think these changes are related to the commit message?
achow101
commented at 0:20 am on February 29, 2020:
Hmm. I think the commit they were part of was accidentally squashed. Moved back into their own commit.
in
src/wallet/wallet.cpp:3872
in
4d7998695foutdated
3867@@ -3868,8 +3868,10 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
38683869 walletInstance->SetWalletFlags(wallet_creation_flags, false);
38703871- // Always create LegacyScriptPubKeyMan for now
3872- walletInstance->SetupLegacyScriptPubKeyMan();
3873+ // Only create LegacyScriptPubKeyMan when not descriptor wallet
3874+ if (!(wallet_creation_flags & WALLET_FLAG_DESCRIPTORS)) {
instagibbs
commented at 6:23 pm on February 28, 2020:
Just a thought you can nack: You can check for the wallet flags since you just set them above. Easier pattern matching for reviewers who are expecting IsWalletFlagSet.
achow101
commented at 0:20 am on February 29, 2020:
Done
in
src/wallet/walletdb.cpp:529
in
e6af0ba62boutdated
523@@ -495,6 +524,14 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
524 result = DBErrors::CORRUPT;
525 }
526527+ // Set the active ScriptPubKeyMans
528+ for (auto spk_man_pair : wss.m_external_spk_managers) {
529+ pwallet->SetActiveScriptPubKeyMan(spk_man_pair.second, spk_man_pair.first, false, true);
instagibbs
commented at 6:53 pm on February 28, 2020:
please annotate bool args
achow101
commented at 0:21 am on February 29, 2020:
Done
in
src/wallet/walletdb.cpp:532
in
e6af0ba62boutdated
523@@ -495,6 +524,14 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
524 result = DBErrors::CORRUPT;
525 }
526527+ // Set the active ScriptPubKeyMans
528+ for (auto spk_man_pair : wss.m_external_spk_managers) {
529+ pwallet->SetActiveScriptPubKeyMan(spk_man_pair.second, spk_man_pair.first, false, true);
530+ }
531+ for (auto spk_man_pair : wss.m_internal_spk_managers) {
532+ pwallet->SetActiveScriptPubKeyMan(spk_man_pair.second, spk_man_pair.first, true, true);
instagibbs
commented at 6:53 pm on February 28, 2020:
please annotate bool args
achow101
commented at 0:21 am on February 29, 2020:
Done
in
src/wallet/scriptpubkeyman.cpp:1614
in
630a5308c8outdated
1609+ break;
1610+ }
1611+ default: assert(false);
1612+ }
1613+
1614+ // Mainnet derives at 0',testnet and regtest derive at 1'
instagibbs
commented at 8:30 pm on February 28, 2020:
0',testnet missing space
achow101
commented at 0:21 am on February 29, 2020:
Done
in
src/wallet/scriptpubkeyman.cpp:1594
in
630a5308c8outdated
1589+
1590+ int64_t creation_time = GetTime();
1591+
1592+ std::string xpub = EncodeExtPubKey(master_key.Neuter());
1593+
1594+ // Add the seed to the wallet
instagibbs
commented at 8:31 pm on February 28, 2020:
Seems like Build descriptor string goes here?
achow101
commented at 0:21 am on February 29, 2020:
Moved
in
src/wallet/scriptpubkeyman.cpp:1632
in
630a5308c8outdated
instagibbs
commented at 8:34 pm on February 28, 2020:
I don’t see intermediate key being written?
achow101
commented at 0:21 am on February 29, 2020:
Removed. The change that did that was reverted given that we now use the descriptor xpub cache.
achow101 force-pushed
on Feb 28, 2020
instagibbs
commented at 8:47 pm on February 28, 2020:
member
Reviewed up to 51012bb297c589bbca59024acb64a59cf9283918 “Add IsSingleType to Descriptors”
will keep going later
achow101 force-pushed
on Feb 29, 2020
in
src/wallet/scriptpubkeyman.cpp:1683
in
8f282ebaf0outdated
1678+ FlatSigningProvider out_keys;
1679+ std::vector<CScript> scripts_temp;
1680+ DescriptorCache cache;
1681+ if (!descriptor.descriptor->Expand(i, provider, scripts_temp, out_keys, &cache)) {
1682+ // Maybe we have a cached xpub and we can expand from cache
1683+ if (!descriptor.descriptor->ExpandFromCache(i, descriptor.cache, scripts_temp, out_keys)) return false;
It may also be worth clarifying that although CanGetAddresses() considers descriptor.next_index < descriptor.range_end, each topup bumps range_end, except for wallets with hardened derivation, with encrypted or without private keys. Those wallets need to call keypoolrefill.
in
src/wallet/scriptpubkeyman.cpp:1520
in
8f282ebaf0outdated
1514+ TopUp();
1515+
1516+ // Get the scriptPubKey from the descriptor
1517+ FlatSigningProvider out_keys;
1518+ std::vector<CScript> scripts_temp;
1519+ if (descriptor.range_end <= m_max_desc_index && !TopUp(1)) {
instagibbs
commented at 8:07 pm on March 20, 2020:
Placeholder for this thought: As I mentioned elsewhere, I’d really like something smarter for detecting IsChange. We could instead store the internall-ness via record with the spkm themselves, then the active record needs one less thing as well, and know what is change even on wallet restore.
It would be nice to avoid this ACTIVE[INTERNAL/EXTERNAL]SPK record altogether and just add two fields to WALLETDESCRIPTOR. I vaguely recall there was a problem with loading order, as the reason we have separate records?
These records are generic spkman records, not descriptor wallet specific records. It does not make sense to make these part of WALLETDESCRIPTOR when not all spkmans are descriptors.
in
src/wallet/walletdb.cpp:247
in
6fa9c482c5outdated
instagibbs
commented at 8:24 pm on March 20, 2020:
Just noting that this is strictly a “softfork” over LegacySPKM: previously any detected pubkey in involved in a script would boot an entry.
The new behavior is much simpler and easier to reason about. Just noting it’s different in case somebody does some sort of idiotic key-sharing wallet between chains.
nit: you do an early return here
@instagibbs IIUC earlier behaviour can still be mimicked with a combo() descriptor.
DrahtBot added the label
Needs rebase
on Mar 23, 2020
achow101 force-pushed
on Mar 24, 2020
DrahtBot removed the label
Needs rebase
on Mar 24, 2020
in
src/wallet/scriptpubkeyman.cpp:2096
in
4a0b2986f4outdated
1639+ descriptor.cache = cache;
1640+ for (int32_t i = descriptor.range_start; i < descriptor.range_end; ++i) {
1641+ FlatSigningProvider out_keys;
1642+ std::vector<CScript> scripts_temp;
1643+ descriptor.descriptor->ExpandFromCache(i, descriptor.cache, scripts_temp, out_keys);
1644+ // Add all of the scriptPubKeys to the scriptPubKey set
instagibbs
commented at 1:11 pm on March 25, 2020:
under what circumstances would multiple scriptpubkeys be generated for a descriptor wallet? Isn’t this just for combo which is inapplicable?
Just combo which can be imported. TopUp is called for all imports to generate the scriptPubKeys, scripts, etc.
instagibbs
commented at 2:44 pm on March 26, 2020:
Ah right I got confused about active arg. In that case the importdescriptors help should be noting that combo cannot be active. The error is helpful enough but I think it warrants note.
in
src/wallet/walletdb.cpp:668
in
4a0b2986f4outdated
559@@ -532,6 +560,12 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
560 pwallet->SetActiveScriptPubKeyMan(spk_man_pair.second, spk_man_pair.first, /* internal */ true, /* memonly */ true);
561 }
562563+ // Set the descriptor caches
564+ for (auto desc_cache_pair : wss.m_descriptor_caches) {
565+ auto spk_man = pwallet->GetScriptPubKeyMan(desc_cache_pair.first);
instagibbs
commented at 1:19 pm on March 25, 2020:
should be asserting or aborting the next line when spkm cannot be found(nullptr) for whatever reason
It isn’t. CanGetAddresses should return false for non-ranged descriptors because you cannot get new addresses from such a descriptor. HavePrivateKeys here is for when the cache runs on hardened derivation. If we have private keys, then we can continue to derive those hardened keys.
instagibbs
commented at 2:41 pm on March 26, 2020:
CanGetAddresses should return false for non-ranged descriptors because you cannot get new addresses from such a descriptor
Hm? If you import a non-ranged descriptor that includes a private key, CanGetAddress will return true here.
Whatever the result of this discussion is, I think this code section requires a comment.
instagibbs
commented at 3:52 pm on March 26, 2020:
But we may still be unable to get addresses, these conditions are caught later.
from GetNewDestination. Might want to move that comment to where I’m asking and explain exactly what additional checks are required?
I’ve tightened up the checks that CanGetAddresses does. It will do a IsSingleType and IsRange check now to disallow non-ranged descriptor and combo descriptors.
in
src/wallet/scriptpubkeyman.cpp:1556
in
915da4fabaoutdated
1552 bool DescriptorScriptPubKeyMan::TopUp(unsigned int size)
1553 {
1554- return false;
1555+ LOCK(cs_desc_man);
1556+ unsigned int target_size;
1557+ if (size > 0)
instagibbs
commented at 2:34 pm on March 25, 2020:
in
src/wallet/walletutil.h:97
in
8da5fdc34aoutdated
92+{
93+public:
94+ std::shared_ptr<Descriptor> descriptor;
95+ uint64_t creation_time;
96+ int32_t range_start; // First item in range; start of range, inclusive, i.e. [range_start, range_end)
97+ int32_t range_end; // Item after the last; end of range, exclusive, i.e. [range_start, range_end)
instagibbs
commented at 4:10 pm on March 26, 2020:
Note if and when this value is set and can change e.g., TopUp?
in
src/wallet/scriptpubkeyman.cpp:1522
in
0b4b742dd5outdated
1516+ TopUp();
1517+
1518+ // Get the scriptPubKey from the descriptor
1519+ FlatSigningProvider out_keys;
1520+ std::vector<CScript> scripts_temp;
1521+ if (m_wallet_descriptor.range_end <= m_max_cached_index && !TopUp(1)) {
instagibbs
commented at 4:25 pm on March 26, 2020:
Can you explain why we do a TopUp(1) here then a few lines later do the ExpandFromCache?
TopUp(1) generates the next cache item if we have run out of cached things. ExpandFromCache can then use that cached thing. Otherwise TopUp(1) is a no-op.
in
src/wallet/scriptpubkeyman.cpp:1986
in
40960f9316outdated
instagibbs
commented at 4:59 pm on March 26, 2020:
future work: looks like a lot of duplicated code with a couple differences from legacy.
in
src/wallet/wallet.h:1227
in
339cc0e6b7outdated
1219@@ -1220,6 +1220,9 @@ class CWallet final : public WalletStorage, private interfaces::Chain::Notificat
12201221 //! Sets the active ScriptPubKeyMan for the specified type and internal
1222 void SetActiveScriptPubKeyMan(uint256 id, OutputType type, bool internal, bool memonly = false);
1223+
1224+ //! Create new DescriptoScriptPubKeyMans and add them to the wallet
instagibbs
commented at 5:04 pm on March 26, 2020:
instagibbs
commented at 5:16 pm on March 26, 2020:
member
reviewed through:
0commit d40617bf4c69d10bf9714ab1f653fc5b740dbf631Author: Andrew Chow <achow101-github@achow101.com>
2Date: Wed Aug 14 14:25:53 2019 -0400
34 Add IsLegacy to CWallet so that the GUI knows whether to show watchonly
achow101 force-pushed
on Mar 26, 2020
achow101 force-pushed
on Mar 26, 2020
instagibbs
commented at 7:00 pm on March 26, 2020:
member
1225@@ -1226,6 +1226,12 @@ class CWallet final : public WalletStorage, private interfaces::Chain::Notificat
12261227 //! Create new DescriptorScriptPubKeyMans and add them to the wallet
1228 void SetupDescriptorScriptPubKeyMans();
1229+
1230+ //! Check if the wallet already has a descriptor
1231+ DescriptorScriptPubKeyMan* HasWalletDescriptor(const WalletDescriptor& desc) const;
instagibbs
commented at 1:48 pm on March 27, 2020:
in
src/wallet/rpcdump.cpp:1517
in
7d1c6bbeb0outdated
1513+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Active descriptor must be ranged");
1514+ }
1515+
1516+ // Ranged descriptors should not have a label
1517+ if (data.exists("range") && data.exists("label")) {
1518+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Ranged descriptors should not have a label");
instagibbs
commented at 2:51 pm on March 27, 2020:
add importdescriptors RPC and tests for native descriptor wallets:
in
src/wallet/rpcdump.cpp:1522
in
7d1c6bbeb0outdated
1518+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Ranged descriptors should not have a label");
1519+ }
1520+
1521+ // Internal addresses should not have a label either
1522+ if (internal && data.exists("label")) {
1523+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Internal addresses should not have a label");
instagibbs
commented at 2:52 pm on March 27, 2020:
add importdescriptors RPC and tests for native descriptor wallets
in
src/wallet/rpcdump.cpp:1527
in
7d1c6bbeb0outdated
1523+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Internal addresses should not have a label");
1524+ }
1525+
1526+ // Combo descriptor check
1527+ if (active && !parsed_desc->IsSingleType()) {
1528+ throw JSONRPCError(RPC_WALLET_ERROR, "Combo descriptors cannot be set to active");
instagibbs
commented at 2:52 pm on March 27, 2020:
add importdescriptors RPC and tests for native descriptor wallets
in
src/wallet/rpcdump.cpp:1538
in
7d1c6bbeb0outdated
1543+ }
1544+ }
1545+
1546+ // If private keys are enabled, abort if private keys are not provided
1547+ if (!pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && !have_privkeys) {
1548+ throw JSONRPCError(RPC_WALLET_ERROR, "Cannot import descriptor without private keys to a wallet with private keys enabled");
instagibbs
commented at 2:53 pm on March 27, 2020:
add importdescriptors RPC and tests for native descriptor wallets
instagibbs
commented at 5:14 pm on March 28, 2020:
In other words, make sure that importing legacy doesn’t accidentally replace p2sh somehow, and p2sh doesn’t accidentally also replace bech32, and so on. Just thinking about what could have gone wrong under the hood.
instagibbs
commented at 3:16 pm on March 27, 2020:
member
Finished review through:
0ommit a03a8cbd1f42636834263135e904900cc808d346 (HEAD -> wallet-of-the-glor)
1Author: Andrew Chow <achow101-github@achow101.com>
2Date: Thu Feb 13 21:06:29 2020 -0500
34 Return error when no ScriptPubKeyMan is available for specified type
56 When a CWallet doesn't have a ScriptPubKeyMan for the requested type
7 in GetNewDestination, give a meaningful error. Also handle this in
8 Qt which did not do anything with errors.
Looking pretty good so far. My least confident part of review is the encrypted wallet portions, so I don’t vouch for those parts.
I still think we should fix IsChange() detection by recording the internal-ness directly in each spkm wallet record to allow for better detection of change on wallet restoration without the user just having to getnewaddress a bunch before it sees on-chain funds. Seems really confusing for a user to import an existing HWW-based BIP44/49/84 descriptor, sync, then have all of its funds in “change”.
in
src/wallet/rpcdump.cpp:1582
in
a03a8cbd1foutdated
1566+ }
1567+
1568+ // Set descriptor as active if necessary
1569+ if (active) {
1570+ if (!w_desc.descriptor->GetOutputType()) {
1571+ warnings.push_back("Unknown output type, cannot set descriptor to active.");
instagibbs
commented at 3:32 pm on March 27, 2020:
note: not sure how this can be hit. Has to be a ranged descriptor that is “null”
in
src/wallet/rpcdump.cpp:1608
in
a03a8cbd1foutdated
1603+ {"requests", RPCArg::Type::ARR, RPCArg::Optional::NO, "Data to be imported",
1604+ {
1605+ {"", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED, "",
1606+ {
1607+ {"desc", RPCArg::Type::STR, RPCArg::Optional::NO, "Descriptor to import."},
1608+ {"active", RPCArg::Type::BOOL, /* default */ "true", "Set this descriptor to be the active descriptor for the corresponding output type/externality"},
instagibbs
commented at 3:47 pm on March 27, 2020:
in
test/functional/wallet_importdescriptors.py:6
in
a03a8cbd1foutdated
0@@ -0,0 +1,293 @@
1+#!/usr/bin/env python3
2+# Copyright (c) 2019 The Bitcoin Core developers
3+# Distributed under the MIT software license, see the accompanying
4+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+"""Test the importdescriptors RPC.
6+
instagibbs
commented at 3:50 pm on March 27, 2020:
this test needs to check default arguments like active
I’m still not very excited about string serialisation, because it sets descriptors in stone. On the other hand, we can also write a straight-forward upgrade script, since descriptors are not encrypted.
It doesn’t have to be that way. We describe “wrapped segwit script” with “sh(wsh(script))”, but it might just as well have been “p2sh[wsh[script]]”. There’s already talk of tweaking descriptors to make them more compatible with miniscript. We use base58 encoded xpubs, but we might as well switch to something bech32 encoded in the future. It makes sense to have a more computer-friendly representation, that’s mapped 1 to 1 with these strings. See also #18043.
It’s just that I’d rather not delay this PR for that.
instagibbs
commented at 5:17 pm on March 28, 2020:
We’re not going to get this right on the first shot likely no matter what we decide. As long as it’s not a supreme burden to upgrade I think current is fine.
in
src/wallet/wallet.h:1244
in
ed75ce9649outdated
1213@@ -1214,6 +1214,12 @@ class CWallet final : public WalletStorage, private interfaces::Chain::Notificat
12141215 //! Connect the signals from ScriptPubKeyMans to the signals in CWallet
1216 void ConnectScriptPubKeyManNotifiers();
1217+
1218+ //! Instantiate a descriptor ScriptPubKeyMan from the WalletDescriptor and load it
1219+ void LoadDescriptorScriptPubKeyMan(uint256 id, WalletDescriptor& desc);
1220+
1221+ //! Sets the active ScriptPubKeyMan for the specified type and internal
memonly could use some documentation (IIUC it depends on if you’re loading an existing wallet, or inserting a new descriptor; it’s not some test suite hack)
in
src/wallet/scriptpubkeyman.cpp:1546
in
68cef400b9outdated
1538@@ -1539,6 +1539,17 @@ bool DescriptorScriptPubKeyMan::TopUp(unsigned int size)
15391540 void DescriptorScriptPubKeyMan::MarkUnusedAddresses(const CScript& script)
1541 {
1542+ LOCK(cs_desc_man);
1543+ if (IsMine(script)) {
1544+ int32_t index = m_map_script_pub_keys[script];
1545+ if (index >= m_wallet_descriptor.next_index) {
1546+ WalletLogPrintf("%s: Detected a used keypool item, mark all keypool items up to this item as used\n", __func__);
in
src/wallet/scriptpubkeyman.cpp:2101
in
0abf7b960coutdated
1641+ FlatSigningProvider out_keys;
1642+ std::vector<CScript> scripts_temp;
1643+ m_wallet_descriptor.descriptor->ExpandFromCache(i, m_wallet_descriptor.cache, scripts_temp, out_keys);
1644+ // Add all of the scriptPubKeys to the scriptPubKey set
1645+ for (const CScript& script : scripts_temp) {
1646+ m_map_script_pub_keys[script] = i;
I was thinking the other way around: have different record for different things, so it’s easy to add new things and wipe / ignore stuff we don’t need anymore.
More specifically, I’ve been thinking about a way to unify the descriptor cache structure so that we don’t have to have separate records like that. In particular, I’ve considered setting a bogus derivation index for the parent xpubs so that a single record can be used, as well as a single map within DescriptorCache too.
in
src/wallet/walletdb.cpp:670
in
0abf7b960coutdated
That’s inherently done by not being able to expand from cache during SetCache.
in
src/wallet/scriptpubkeyman.cpp:1858
in
a03a8cbd1foutdated
1847+}
1848+
1849+bool DescriptorScriptPubKeyMan::CanGetAddresses(bool internal) const
1850+{
1851+ // We can only give out addresses from descriptors that are single type (not combo), ranged,
1852+ // and either has cached keys or can generate more keys (ignoring encryption)
No. CanGetAddresses implies that more addresses can be fetched. You can’t do that for an unranged descriptor, we consider their address already fetched and used.
in
src/wallet/scriptpubkeyman.cpp:1558
in
8ae9bf1239outdated
1559+ return false;
1560+
1561+ bool keyPass = m_map_crypted_keys.empty(); // Always pass when there are no encrypted keys
1562+ bool keyFail = false;
1563+ CryptedKeyMap::const_iterator mi = m_map_crypted_keys.begin();
1564+ for (; mi != m_map_crypted_keys.end(); ++mi)
In b4c6a40ac46459a679dabbb84b168101e6cd6c5f I suggested for (const auto& mi : m_map_crypted_keys) {, which you did, but not it’s gone again in DescriptorScriptPubKeyMan (it’s there in LegacyScriptPubKeyMan ). Probably not worth touching the code for, but you may to check if you didn’t lose anything else.
No, it seems like I’ve accidentally applied that change to LegacyScriptPubKeyMan. Applied these changes to the right place.
in
src/wallet/scriptpubkeyman.cpp:1621
in
ee03d46178outdated
1615@@ -1616,6 +1616,13 @@ bool DescriptorScriptPubKeyMan::GetReservedDestination(const OutputType type, bo
16161617 void DescriptorScriptPubKeyMan::ReturnDestination(int64_t index, bool internal, const CTxDestination& addr)
1618 {
1619+ LOCK(cs_desc_man);
1620+ // Only return when the index was the most recent
ExpandPrivate() is a void, but it uses GetPrivKey internally and just ignores failure. Might be worth making ExpandPrivate() a bool and adding an assert here.
in
src/wallet/wallet.cpp:588
in
2de265c2b6outdated
584@@ -585,8 +585,11 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)
585 Lock();
586 Unlock(strWalletPassphrase);
587588- // if we are using HD, replace the HD seed with a new one
589- if (auto spk_man = GetLegacyScriptPubKeyMan()) {
590+ // If we are using descriptors, make new descriptors
in
test/functional/wallet_importdescriptors.py:15
in
7d1c6bbeb0outdated
10+
11+- `get_key()` is called to generate keys on node0 and return the privkeys,
12+ pubkeys and all variants of scriptPubKey and address.
13+- `test_importdesc()` is called to send an importdescriptors call to node1, test
14+ success, and (if unsuccessful) test the error code and error message returned.
15+- `test_address()` is called to call getaddressinfo for an address on node1
See above. It makes sense to disallow getnewaddress on unranged descriptors. And they can’t be active (I assume that restriction is tested in the importdescriptor tests). So in that case there’s nothing to test here.
Sjors
commented at 7:44 pm on March 27, 2020:
member
Did a light to moderate review for a03a8cbd1f42636834263135e904900cc808d346, it’s looking pretty good. Lots of commits, but they’re mostly short and focussed. I like how we get lots of tests for free with --descriptors in test_runner.py, but I’d like to see more direct coverage of wallet (de)serialisation code.
achow101 force-pushed
on Mar 27, 2020
achow101 force-pushed
on Mar 27, 2020
in
src/wallet/scriptpubkeyman.cpp:1905
in
d034592cf8outdated
instagibbs
commented at 5:27 pm on March 28, 2020:
member
ACK changes, modulo assert change
instagibbs
commented at 5:29 pm on March 28, 2020:
member
looks like unit test is failing sometimes:
0wallet/test/wallet_tests.cpp(651): error: in "wallet_tests/wallet_descriptor_test": exception "std::ios_base::failure" raised as expected: validation on the raised exception through predicate "malformed_descriptor"
achow101 force-pushed
on Mar 28, 2020
achow101 force-pushed
on Mar 28, 2020
achow101
commented at 9:12 pm on March 28, 2020:
member
looks like unit test is failing sometimes:
0wallet/test/wallet_tests.cpp(651): error: in "wallet_tests/wallet_descriptor_test": exception "std::ios_base::failure" raised as expected: validation on the raised exception through predicate "malformed_descriptor"
Not seeing this fail at anytime.
instagibbs
commented at 0:09 am on March 29, 2020:
member
wallet/test/wallet_tests.cpp(651): error: in “wallet_tests/wallet_descriptor_test”: exception “std::ios_base::failure” raised as expected: validation on the raised exception through predicate “malformed_descriptor”
hmm, that’s an expected error, I’m not sure why Travis is complaining actually
Sjors
commented at 2:15 pm on March 30, 2020:
member
I restarted job 4 and 13 as well as AppVeyor; they all fail again.
I get the same error on macOS:
0wallet/test/wallet_tests.cpp:651: error: in "wallet_tests/wallet_descriptor_test": exception "std::ios_base::failure" raised as expected: validation on the raised exception through predicate "malformed_descriptor"
achow101 force-pushed
on Mar 30, 2020
achow101 force-pushed
on Mar 30, 2020
achow101
commented at 4:53 pm on March 30, 2020:
member
Rebased this on master to see if that fixes the travis failures. Also added a change to that test to make it less restrictive so hopefully it passes now.
achow101 force-pushed
on Mar 30, 2020
in
src/wallet/scriptpubkeyman.cpp:1918
in
3f5e0bfc5doutdated
In 3f5e0bfc5dc2ea37223c1e5820c66a5ce11d6b81: clang on macOS complains about this std::move, and afaik there’s no need for it, because the result of GetSigningProvider() is an rvalue. Ditto for SignMessage (70345ca67f42ce49b764024f5c62bf8a9a7bd188).
IIRC gcc used to complain about this. But it seems not anymore, so I’ve removed these too.
Sjors
commented at 1:02 pm on March 31, 2020:
member
f2c3e36 is getting close
When you create a descriptor wallet with avoid_reuse and then call listunspent, it calls IsSpentKey() which asserts the presence of a legacy SPKM. In fact, it even crashed for me when using importdescriptors with bech32 keys from HWI.
Creating a PSBT in the GUI now crashes (without reuse flag):
0Assertion failed: (expanded), function GetSigningProvider, file wallet/scriptpubkeyman.cpp, line 1907.
When I create a watch-only wallet, the log shows External scriptPubKey Manager for output type 0 does not exist, several times for each combination. Would be nice if that can be avoided.
instagibbs
commented at 1:21 pm on March 31, 2020:
member
IsSpentKey()
Right, there is even a TODO for descriptor wallets in the function. It’s a good time to get rid of that TODO.
in
src/wallet/scriptpubkeyman.cpp:1992
in
f2c3e36479outdated
In 1dd95d7e91fb31ee97cff2aa0f1ccfd820e78e1e IIUC the descriptor is stored as wpkh(xpub/84'/0'/0'/0/*) which means it can only be expanded with the aid of the master private key. Why not expand the account level xpub(s), so you can store it as wpkh([84'/0'/0']xpub/0/*)?
We would then have to store the private key at the account level too. With the xpub cache, this isn’t a problem as it can always be expanded using the cached xpub.
Sjors approved
Sjors
commented at 4:44 pm on April 1, 2020:
member
ACK7ea9f0fb848e3ccc69e05b4c1bad465e73bc1963. Added food for thought about if we should store account level xpubs.
achow101 force-pushed
on Apr 1, 2020
achow101
commented at 8:49 pm on April 1, 2020:
member
Turns out IsSpentKey was not fully fixed. I’ve added an additional check for HavePrivateKeys before trying to do ExpandPrivate.
achow101
commented at 10:26 pm on April 1, 2020:
member
So I’ve just realized that existing multisig workflows are completely non-functional under descriptor wallets. If you make a multisig that includes a key that a descriptor wallet has, it won’t be able to sign that multisig. This is because the multisig script does not match any of the scripts produced by the descriptors so none of the DescriptorScriptPubKeyMans return true for CanProvide and therefore do not attempt to sign. Furthermore, private keys cannot currently be exported, and we currently require all private keys to be present to import a descriptor with private keys. So you couldn’t even create a descriptor containing your private key(s) and import that.
One possible solution is to simply have every ScriptPubKeyMan sign always, regardless of CanProvide. But this runs into the whole key mutation thing we are trying to avoid. It would then be possible to have a key for one address type be able to sign for a different address type for the same key. At least the wallet would not be watching for such a mutation. But I suppose the whole multisig thing is effectively abusing mutating keys like that.
Any other suggestions?
instagibbs
commented at 0:38 am on April 2, 2020:
member
from IRC:
0<instagibbs> It seems like a very reasonable use-case to support. 1<instagibbs> Core-generated key being in a multisig :)
2<achow101> my original thoughts about the multisig stuff would be you have 2 wallets, one generated normally, and another watch only one for the multisig. so you then use psbt and multiwallet
3<instagibbs> what's the hold-up on export? feature creep? 4<achow101> security questions about unhardened derivation
5<instagibbs> oh im overthinking, use-case is supported, *if* you can get the account-level xpub
6<instagibbs> i can't recall what your PR supports 7<achow101> currently no exports at all whatoever
8<instagibbs> ok so I think public descriptor export is a thing we'd want to support 9achow101> yeah
10<achow101> that'll be next
achow101 force-pushed
on Apr 2, 2020
achow101
commented at 1:11 am on April 2, 2020:
member
I’ve changed it to try signing with all ScriptPubKeyMans.
Sjors
commented at 8:40 am on April 2, 2020:
member
One possible solution is to simply have every ScriptPubKeyMan sign always, regardless of CanProvide. But this runs into the whole key mutation thing we are trying to avoid. It would then be possible to have a key for one address type be able to sign for a different address type for the same key. At least the wallet would not be watching for such a mutation. But I suppose the whole multisig thing is effectively abusing mutating keys like that.
I’m not a fan of this. I think a wallet should only sign for scripts that derive from its descriptors. Let’s just support exporting account level xpubs for the multisig use case. We can later add a convenience RPC that, given external xpub(s), produces a multisig with itself and imports those descriptors.
instagibbs
commented at 12:57 pm on April 2, 2020:
member
@Sjors I think signing eagerly is fine provided it doesn’t somehow expand “is mine” definition in any way or otherwise include ways to trick the user.
instagibbs
commented at 1:16 pm on April 2, 2020:
member
the “attempt signing with all” changes actually make the code easier to read as well
achow101 force-pushed
on Apr 3, 2020
achow101
commented at 3:19 am on April 3, 2020:
member
I’ve made hopefully that last major change to this. I had to fix FillPSBT because the earlier change wasn’t actually signing multisig PSBTs either. The change for that is that we are also going to hold in memory all of the pubkeys that were produced during descriptor expansion. This is done during SetCache and TopUp when the descriptor is expanded. GetSigningProvider is modified and overloaded to also accept a pubkey. This is so that the descriptor can be expanded at the correct index for a given pubkey, regardless of the scripts that pubkey was attached too.
This now allows FillPSBT to use the pubkeys given in the the hd_keypaths to get a SigningProvider with private keys in order to sign the PSBT.
The other big change that I made was to the tests. I’ve changed how the tests are being setup so that the whole thing can be more generic and be slightly easier and cleaner to update additional tests to work with descriptor wallets. I’ve also enabled --descriptors for wallet_avoidreuse.py, rpc_createmultisig.py, wallet_hd.py, and rpc_psbt.py. These additional tests should cover the bugs that we have found so far (guess how I’ve found a couple of them). Additionally, to make these tests work, I’ve RPCOverloadWrapper which wraps the RPC and overloads the importprivkey, importaddress, importpubkey, and addmultisigaddress RPCs. This works for both cli and AuthServiceProxy. For non-descriptor wallets, we use the actual RPC commands. For descriptor wallets, it turns the RPC arguments into something that works with importdescriptors. It’s similar to what we did to the generate RPC in test_node.py.
I’ve replaced a few instances of dumpprivkey with ECKey from test_framework/key.py which can generate a private key. So we use this along with a few new functions to convert that into WIF to do some of the things that dumpprivkey was being used for.
To avoid code churn in test cases themselves, I’ve modified setup_nodes in test_framework.py to create the wallets using createwallet. So nodes will always start initially with -nowallet and have any specific wallets added via createwallet. This gives us the ability to create a default wallet that is a descriptor wallet so we don’t need to put things to choose/create the correct descriptor wallet throughout the tests. This particular change also fixes a bug(?) in our wallet creation code that wasn’t allowing the creation of a default wallet (wallet with the empty string as the name) when a default wallet didn’t already exist.
I’ll continue to investigate the remaining tests and trying to get them to work with descriptor wallets. The goal is to get all of them to pass so that descriptor wallets can become the default wallet type. This will help this PR as it will uncover bugs as already done. But I think most of those issues have already been identified.
achow101 force-pushed
on Apr 3, 2020
achow101 force-pushed
on Apr 3, 2020
achow101 force-pushed
on Apr 3, 2020
achow101 force-pushed
on Apr 3, 2020
Sjors
commented at 12:40 pm on April 3, 2020:
member
Another issue I have with “blindly” signing something for which you don’t have an exact descriptor, is that change detection doesn’t work.
achow101
commented at 4:29 pm on April 3, 2020:
member
I’m not a fan of this. I think a wallet should only sign for scripts that derive from its descriptors. Let’s just support exporting account level xpubs for the multisig use case. We can later add a convenience RPC that, given external xpub(s), produces a multisig with itself and imports those descriptors.
Exporting xpubs doesn’t help with signing. The crux of this issue is that we only use the private keys of the ScriptPubKeyMan for the particular scriptPubKey that is being signed. So even if you imported a multisig descriptor into a wallet that has some private keys for it, we still wouldn’t be able to sign because that particular descriptor doesn’t have the private keys for signing.
Another issue I have with “blindly” signing something for which you don’t have an exact descriptor, is that change detection doesn’t work.
I don’t think change detection matters here. We already don’t do change detection when doing signing with the RPCs. Changing how this signing works does not affect IsMine at all. And I don’t think this has an effect on hardware wallets either.
An alternative solution would be to have a SigningProvider at the wallet level which is shared by all ScriptPubKeyMans. But this is a much larger change and becomes way more complicated with deriving keys on the fly from descriptors as we want to do now.
Sjors
commented at 5:58 pm on April 3, 2020:
member
I guess it doesn’t matter too much.
even if you imported a multisig descriptor into a wallet that has some private keys for it, we still wouldn’t be able to sign because that particular descriptor doesn’t have the private keys for signing
That’s a good point. What about signing everything that’s IsMine? Alternatively, a boolean that opts into this more broad signing behaviour.
We already don’t do change detection when doing signing with the RPCs.
True, and we don’t really know what a user intended when there’s multiple outputs. We could add a walletanalyzepsbt feature for that later.
For the GUI, I suppose we can add a change address safety feature on top of #18027, when loading a PSBT.
in
src/wallet/scriptpubkeyman.cpp:1687
in
e75d67d237outdated
1682+ }
1683+ for (const auto& pk_pair : out_keys.pubkeys) {
1684+ const CPubKey& pubkey = pk_pair.second;
1685+ if (m_map_pubkeys.count(pubkey) != 0) {
1686+ // We don't need to give an error here.
1687+ // It doesn't matter what index the pubkey has, we just need an index where we can derive it and it's private key
in
src/wallet/scriptpubkeyman.cpp:2107
in
e75d67d237outdated
2102+ }
2103+ for (const auto& pk_pair : out_keys.pubkeys) {
2104+ const CPubKey& pubkey = pk_pair.second;
2105+ if (m_map_pubkeys.count(pubkey) != 0) {
2106+ // We don't need to give an error here.
2107+ // It doesn't matter what index the pubkey has, we just need an index where we can derive it and it's private key
achow101
commented at 6:41 pm on April 7, 2020:
member
Made a few more test framework changes, particularly to have createwallet make wallets based on the startup options unless overridden. This avoids having to put descriptors=self.options.descriptors in every createwallet call. Also changed wallet_importdescriptors.py to not rely on dumpprivkey.
achow101 force-pushed
on Apr 10, 2020
achow101
commented at 0:05 am on April 10, 2020:
member
I’ve changed importdescriptors to allow importing descriptors that have some but not all private keys. A test has also been added for this. Since this change requires ExpandPrivate to expand all the way, The commit that changed ExpandPrivate to return a bool has been dropped.
ryanofsky
commented at 8:33 am on April 10, 2020:
member
instagibbs
commented at 1:28 pm on April 10, 2020:
member
I’ve changed importdescriptors to allow importing descriptors that have some but not all private keys.
So this is going to make certain actions strange, due to the various places we use IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) to trigger PSBT vs “sign and send” type behavior:
bumpfee: Instead of returning a PSBT, it will attempt to sign and send the transaction, and fail
QT: Flow I think will let you click “send” and whatnot, and it will simply fail to sign the transaction. User won’t be able to craft a PSBT from the GUI.
I think in the ideal case any wallet where the user cannot fully sign would take the current IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) style behavior which hands the user a (partially signed)PSBT to sign elsewhere.
@ryanofsky notes in your document a similar strategy I suggested of allowing the “I expect to be able to fully sign for this descriptor”-ness in the descriptor record itself.
ryanofsky
commented at 3:35 pm on April 10, 2020:
member
So this is going to make certain actions strange, due to the various places we use IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) to trigger PSBT vs “sign and send” type behavior:
This is just an uninformed opinion, but it would seem less surprising to me for the “Disable Private Keys” option you see creating a wallet to just be a safeguard against unintentionally generating and importing private keys, and not something signing/psbt code would look at directly.
achow101
commented at 5:21 pm on April 10, 2020:
member
I’ve changed importdescriptors to allow importing descriptors that have some but not all private keys.
So this is going to make certain actions strange, due to the various places we use IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) to trigger PSBT vs “sign and send” type behavior:
1. bumpfee: Instead of returning a PSBT, it will attempt to sign and send the transaction, and fail
2. QT: Flow I think will let you click "send" and whatnot, and it will simply fail to sign the transaction. User won't be able to craft a PSBT from the GUI.
I think in the ideal case any wallet where the user cannot fully sign would take the current IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) style behavior which hands the user a (partially signed)PSBT to sign elsewhere.
Instead of branching on IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS), wouldn’t it be better to just always make a PSBT, try to sign it, and if it does fully sign, then return the txid, otherwise return the PSBT? I think that would be more robust.
Don’t existing multisigs have the same problem anyways? If you do addmultisigaddress and one of the keys is yours, it wouldn’t be able to sign either.
@ryanofsky notes in your document a similar strategy I suggested of allowing the “I expect to be able to fully sign for this descriptor”-ness in the descriptor record itself.
This strategy still results in a mixed wallet which, as mentioned before, has issues and is what I’m trying to avoid. But I’m also not sure how that would help with the issues you mentioned above or if it’s actually useful to do that.
achow101 force-pushed
on Apr 10, 2020
achow101
commented at 6:35 pm on April 10, 2020:
member
Since descriptor wallets is still experimental and not the default, I’m find with some of the weird, less supported, use/edge cases not entirely working. These will need to be fixed in the future, but I would like to at least get the basic functionality in. Especially when those cases require more significant concentrated thought, e.g. at a CoreDev event where we can all sit around a whiteboard and talk for a few hours.
I’ve pushed a change that will add a warning if not all private keys are provided. It will let users know that there could be unexpected errors by doing such an import. Also, hopefully I fixed travis.
achow101 force-pushed
on Apr 10, 2020
achow101
commented at 7:39 pm on April 10, 2020:
member
On IRC, @sipa points out that we can just use separate RPCs and buttons for bumpfee, PSBT GUI, and whatever else is switching on WALLET_FLAG_DISABLE_PRIVATE_KEYS instead of having functions that change their behavior based on that flag.
in
src/wallet/wallet.h:1236
in
a7a814fb59outdated
1230@@ -1231,6 +1231,12 @@ class CWallet final : public WalletStorage, private interfaces::Chain::Notificat
12311232 //! Create new DescriptorScriptPubKeyMans and add them to the wallet
1233 void SetupDescriptorScriptPubKeyMans();
1234+
1235+ //! Return the DescriptorScriptPubKeyMan for a WalletDescriptor if it is already in the wallet
1236+ DescriptorScriptPubKeyMan* GetWalletDescriptor(const WalletDescriptor& desc) const;
I think this function should be renamed. It is confusing that there is a function by the same name in DescriptorScriptPubKeyMan that actually returns a WalletDescriptor while this one returns DescriptorScriptPubKeyMan. It becomes especially apparent in ProcessDescriptorImport where those functions are called within a few lines of code.
I think this would deserve a comment on why this is always 0. I also thought maybe the ScriptPubKeyMan class could do this instead of GetTime() so that it would not need to be overridden but I think that would not help clarity of the code.
I found this comment more confusing than helpful here because it explains something about the descriptor code in a non-descriptor code branch. I think it could just be removed.
in
src/wallet/rpcdump.cpp:1536
in
a7a814fb59outdated
1531+ // If the wallet disabled private keys, abort if private keys exist
1532+ if (pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && !keys.keys.empty()) {
1533+ throw JSONRPCError(RPC_WALLET_ERROR, "Cannot import private keys to a wallet with private keys disabled");
1534+ }
1535+
1536+ // Need to ExpandPrvate to check if private keys are available for all pubkeys
My comments are all in the nit category I also tested earlier builds with some multisig stuff but will redo those as soon as possible since they did not have the most recent changes. Will also review the IRC discussion of the last two days, most of my review effort was done before those took place.
fjahr
commented at 9:05 pm on April 17, 2020:
member
tACKd42a6edd89a313ee25683226a9aa573087de1223
I have manually created a normal descriptor wallet with a private key and a watch-only wallet for multisig descriptor and tested basic functionalities like sending, receiving and info calls. Also ran all automated tests locally.
Regarding the latest discussion about the current limitations: I think this can be merged as is but I would suggest adding a warning to createwallet which mentions current limitations around exports and differences to legacy wallets like unhardened derivation. That is probably not the right way to use the warnings, I think, but it makes it much harder to miss this information.
achow101 force-pushed
on Apr 20, 2020
fjahr
commented at 7:59 pm on April 21, 2020:
member
Re-ACK837aba9a3680922acbf383df37485d53790b19ae
Only changes were addressing my nit comments. Not sure why the build is failing, I don’t see the error locally.
achow101 force-pushed
on Apr 21, 2020
achow101
commented at 9:10 pm on April 21, 2020:
member
Only changes were addressing my nit comments. Not sure why the build is failing, I don’t see the error locally.
Looks like there’s a hidden conflict with master. I’ve rebased this and fixed the issues.
achow101 force-pushed
on Apr 21, 2020
fjahr
commented at 3:14 pm on April 22, 2020:
member
re-ACK4c841356c2296cc011bcd678ba71ccba28129a67
Only code-changes since last review were small fixups in wallet/rpcdump.cpp and test/functional/wallet_keypool.py.
in
src/wallet/scriptpubkeyman.cpp:1553
in
b4c6a40ac4outdated
in
src/wallet/rpcdump.cpp:1618
in
4c841356c2outdated
1613+ {
1614+ {"desc", RPCArg::Type::STR, RPCArg::Optional::NO, "Descriptor to import."},
1615+ {"active", RPCArg::Type::BOOL, /* default */ "false", "Set this descriptor to be the active descriptor for the corresponding output type/externality"},
1616+ {"range", RPCArg::Type::RANGE, RPCArg::Optional::OMITTED, "If a ranged descriptor is used, this specifies the end or the range (in the form [begin,end]) to import"},
1617+ {"next_index", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "If a ranged descriptor is set to active, this specifies the next index to generate addresses from"},
1618+ {"timestamp", RPCArg::Type::NUM, RPCArg::Optional::NO, "Time to start rescanning the blockchain for this descriptor, in seconds since epoch (Jan 1 1970 GMT).\n"
Use the UNIX_EPOCH_TIME constant when describing UNIX epoch time or timestamps
s/Time to start/Time from which to start/
e.g.
0- "Time to start rescanning the blockchain for this descriptor, in seconds since epoch (Jan 1 1970 GMT).\n"
1+ "Time from which to start rescanning the blockchain for this descriptor, in " + UNIX_EPOCH_TIME + "\n"
in
src/wallet/rpcdump.cpp:1607
in
4c841356c2outdated
1602+ return NullUniValue;
1603+ }
1604+
1605+ RPCHelpMan{"importdescriptors",
1606+ "\nImport descriptors. This will trigger a rescan of the blockchain based on the earliest timestamp of all descriptors being imported. Requires a new wallet backup.\n"
1607+ "\nNote: This call can take over an hour to complete if using an early timestamp, during that time, other rpc calls\n"
in
src/wallet/rpcdump.cpp:1608
in
4c841356c2outdated
1603+ }
1604+
1605+ RPCHelpMan{"importdescriptors",
1606+ "\nImport descriptors. This will trigger a rescan of the blockchain based on the earliest timestamp of all descriptors being imported. Requires a new wallet backup.\n"
1607+ "\nNote: This call can take over an hour to complete if using an early timestamp, during that time, other rpc calls\n"
1608+ "may report that the imported keys, addresses or scripts exists but related transactions are still missing.\n",
jonatack
commented at 9:27 pm on April 22, 2020:
member
WIP review, LGTM up to b642c6b5e6971c and builds/tests look reliable.
Sjors approved
Sjors
commented at 9:30 pm on April 22, 2020:
member
re-tACK4c84135. Tested by rebasing #16549, importing keys from a hardware wallet, performing a rescan and spending. I like the RPCOverloadWrapper.
achow101 force-pushed
on Apr 22, 2020
Sjors
commented at 7:37 am on April 23, 2020:
member
In ed01138 locally the following test fails on macOS:
0 test/functional/wallet_balance.py --descriptors
12020-04-23T07:36:07.090000Z TestFramework (INFO): Initializing test directory /var/folders/h6/qrb4j9vn6530kp7j4ymj934h0000gn/T/bitcoin_func_test_5klxvr5f
22020-04-23T07:36:09.863000Z TestFramework (ERROR): JSONRPC error
3Traceback (most recent call last):
4File"/Users/sjors/dev/bitcoin-desc/test/functional/test_framework/test_framework.py", line 112, in main
5 self.run_test()
6File"test/functional/wallet_balance.py", line 62, in run_test
7 self.nodes[0].importaddress(ADDRESS_WATCHONLY)
8File"/Users/sjors/dev/bitcoin-desc/test/functional/test_framework/test_node.py", line 714, in importaddress
9 raise JSONRPCException(res['error'])
10test_framework.authproxy.JSONRPCException: Cannot import descriptor without private keys to a wallet with private keys enabled (-4)
In case you need to change anything, it would be great if you can rebase, so I resolve a conflict between #17509 and #16549.
in
src/wallet/walletdb.cpp:33
in
ed0113820boutdated
nit: here and in walletdb.h I’m curious why ACTIVEEXTERNALSPK AND ACTIVEINTERNALSPK are placed here rather than sorted like the others… maybe sort, or comment why
in
src/wallet/scriptpubkeyman.cpp:1858
in
ed0113820boutdated
1853+}
1854+
1855+bool DescriptorScriptPubKeyMan::CanGetAddresses(bool internal) const
1856+{
1857+ // We can only give out addresses from descriptors that are single type (not combo), ranged,
1858+ // and either has cached keys or can generate more keys (ignoring encryption)
jonatack
commented at 12:59 pm on April 23, 2020:
member
Reviewed up to “Implement GetSolvingProvider for DescriptorScriptPubKeyMan” so far, a bit more than halfway through. While reviewing, built with gcc and with clang and ran all tests several times on Debian… all green.
in
src/wallet/rpcdump.cpp:1508
in
ed0113820boutdated
1503+ }
1504+ }
1505+
1506+ // Active descriptors must be ranged
1507+ if (active && !parsed_desc->IsRange()) {
1508+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Active descriptor must be ranged");
in
src/wallet/rpcdump.cpp:1625
in
ed0113820boutdated
1620+ " \"now\" can be specified to bypass scanning, for outputs which are known to never have been used, and\n"
1621+ " 0 can be specified to scan the entire blockchain. Blocks up to 2 hours before the earliest timestamp\n"
1622+ " of all descriptors being imported will be scanned.",
1623+ /* oneline_description */ "", {"timestamp | \"now\"", "integer / string"}
1624+ },
1625+ {"internal", RPCArg::Type::BOOL, /* default */ "false", "Stating whether matching outputs should be treated as not incoming payments (also known as change)"},
in
src/wallet/rpcdump.cpp:1746
in
ed0113820boutdated
1737+ "could contain transactions pertaining to the desc. As a result, transactions "
1738+ "and coins using this desc may not appear in the wallet. This error could be "
1739+ "caused by pruning or data corruption (see bitcoind log for details) and could "
1740+ "be dealt with by downloading and rescanning the relevant blocks (see -reindex "
1741+ "and -rescan options).",
1742+ GetImportTimestamp(request, now), scanned_time - TIMESTAMP_WINDOW - 1, TIMESTAMP_WINDOW)));
Actually nvm, the 40 lines of this section are essentially a duplicate of code in importmulti. It’s outside the scope of this long PR, but they could be de-duped.
in
src/wallet/rpcwallet.cpp:4327
in
ed0113820boutdated
0 assert_equal(wallet_info['keypoolsize_hd_internal'], 300)
1+ # Expect getwalletinfo to not return "keypoololdest" for descriptor wallets, only legacy ones
2+ assert 'keypoololdest' not in wallet_info.keys()
No blockers from what I could see. Good work on the tests. Feel free to ignore the nit comments; I don’t mind re-reviewing the diff if you retouch. Built/ran tests several times on Debian with no warnings or failures.
achow101
commented at 5:24 pm on April 23, 2020:
member
In ed01138 locally the following test fails on macOS:
That’s intended. Not all tests have been reworked to work with descriptor wallets. wallet_balance.py is one of those tests that need some modifications.
Introduce SetType function to tell ScriptPubKeyMans the type and internal-ness of it06620302c7
Introduce DescriptorScriptPubKeyMan as a dummy class6b8119af53
Add WALLET_FLAG_DESCRIPTORS96accc73f0
Return nullptr from GetLegacyScriptPubKeyMan if descriptor walletaeac157c9d
Create LegacyScriptPubKeyMan when not a descriptor wallet6b13cd3fa8
Introduce WalletDescriptor class
WalletDescriptor is a Descriptor with other wallet metadata
3194a7f88a
Add a lock cs_desc_man for DescriptorScriptPubKeyMand8132669e1
Store WalletDescriptor in DescriptorScriptPubKeyMan834de0300c
Implement SetType in DescriptorScriptPubKeyMan78f8a92910
achow101 force-pushed
on Apr 23, 2020
Add LoadDescriptorScriptPubKeyMan and SetActiveScriptPubKeyMan to CWalletdb7177af8c
Implement IsMine for DescriptorScriptPubKeyMan
Adds a set of scriptPubKeys that DescriptorScriptPubKeyMan tracks.
If the given script is in that set, it is considered ISMINE_SPENDABLE
2db7ca765c
Implement MarkUnusedAddresses in DescriptorScriptPubKeyMan741122d4c1
Implement IsHDEnabled in DescriptorScriptPubKeyManec2f9e1178
Implement GetID for DescriptorScriptPubKeyMan46c46aebb7
Load the descriptor cache from the wallet file2363e9fcaa
Implement loading of keys for DescriptorScriptPubKeyMan953feb3d27
Add IsSingleType to Descriptors
IsSingleType will return whether the descriptor will give one or multiple scriptPubKeys
d1ec3e4f19
Implement several simple functions in DescriptorScriptPubKeyMan
Implements a bunch of one liners: UpgradeKeyMetadata, IsFirstRun, HavePrivateKeys,
KeypoolCountExternalKeys, GetKeypoolSize, GetTimeFirstKey, CanGetAddresses,
RewriteDB
4cb9b69be0
Implement writing descriptorkeys, descriptorckeys, and descriptors to wallet file46dfb99768
Implement SetupGeneration for DescriptorScriptPubKeyMane014886a34
Implement TopUp in DescriptorScriptPubKeyMan58c7651821
Implement GetNewDestination for DescriptorScriptPubKeyManbfdd073486
Implement Unlock and Encrypt in DescriptorScriptPubKeyMana775f7c7fd
Implement GetReservedDestination in DescriptorScriptPubKeyManf866957979
Implement ReturnDestination in DescriptorScriptPubKeyMan586b57a9a6
Implement GetKeypoolOldestTime and only display it if greater than 0f1ca5feb4a
Implement GetSolvingProvider for DescriptorScriptPubKeyMan
Internally, a GetSigningProvider function is introduced which allows for
some private keys to be optionally included. This can be called with a
script as the argument (i.e. a scriptPubKey from our wallet when we are
signing) or with a pubkey. In order to know what index to expand the
private keys for that pubkey, we need to also cache all of the pubkeys
involved when we expand the descriptor. So SetCache and TopUp are
updated to do this too.
d50c8ddd41
Implement SignTransaction in DescriptorScriptPubKeyManbde7c9fa38
Implement SignMessage for descriptor wallets84b4978c02
Implement FillPSBT in DescriptorScriptPubKeyMan
FillPSBT will add our own scripts to the PSBT if those inputs are ours.
If an input also lists pubkeys that we happen to know the private keys
for, we will sign those inputs too.
72a9540df9
Change GetMetadata to use unique_ptr<CKeyMetadata>8b9603bd0b
Implement GetMetadata in DescriptorScriptPubKeyManb713baa75a
Be able to create new wallets with DescriptorScriptPubKeyMans as backing82ae02b165
Generate new descriptors when encrypting1cb42b22b1
Add IsLegacy to CWallet so that the GUI knows whether to show watchonlyce24a94494
add importdescriptors RPC and tests for native descriptor wallets
Co-authored-by: Andrew Chow <achow101-github@achow101.com>
f193ea889d
Functional tests for descriptor wallets1346e14831
Change wallet_encryption.py to use signmessage instead of dumpprivkey388ba94231
Return error when no ScriptPubKeyMan is available for specified type
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.
3c19fdd2a2
Implement CWallet::IsSpentKey for non-LegacySPKMans886e0d75f5
Correctly check for default walletcf06062859
tests: Add RPCOverloadWrapper which overloads some disabled RPCs
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.
869f7ab30a
Add a --descriptors option to various tests
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
223588b1bb
achow101 force-pushed
on Apr 23, 2020
achow101
commented at 6:01 pm on April 23, 2020:
member
Addressed @jonatack’s comments and rebased as requested by @Sjors
Sjors
commented at 6:48 pm on April 23, 2020:
member
That’s intended. Not all tests have been reworked to work with descriptor wallets. wallet_balance.py is one of those tests that need some modifications.
I might be seeing two different issues. When I run the full test suite that particular test sometimes fails with [node 0] bitcoind exited with status 1 during initialization, but not every time, and it also happens with other tests, including non-wallet ones. And given that Travis macOS passes, I think it’s unrelated to this PR and maybe a (new) dev setup problem.
jonatack
commented at 9:38 pm on April 23, 2020:
member
Rebuilt, re-ran all tests, bitcoind and a few importdescriptors rpc commands as a sanity check. I did not test the GUI changes yet.
fjahr
commented at 3:32 pm on April 24, 2020:
member
re-ACK223588b1bbc63dc57098bbd0baa48635e0cc0b82
Changes were only rebase and addressing nits. FWIW, I did not see any failures from wallet_balance.py.
instagibbs
commented at 7:32 pm on April 24, 2020:
member
light re-ACK223588b
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.
in
src/wallet/scriptpubkeyman.cpp:1841
in
223588b1bb
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.
meshcollider merged this
on Apr 27, 2020
meshcollider closed this
on Apr 27, 2020
fanquake removed this from the "Blockers" column in a project
meshcollider moved this from the "PRs" to the "Done" column in a project
sidhujag referenced this in commit
46ed584a4a
on Apr 27, 2020
domob1812 referenced this in commit
03bd0d8527
on Apr 27, 2020
domob1812 referenced this in commit
16a50a80b0
on Apr 27, 2020
practicalswift
commented at 2:31 pm on April 27, 2020:
contributor
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”) :)
meshcollider referenced this in commit
ec79b5f86b
on May 5, 2020
sidhujag referenced this in commit
31e63beaae
on May 5, 2020
meshcollider referenced this in commit
df303ceb65
on May 22, 2020
sidhujag referenced this in commit
5c5677cfef
on May 22, 2020
laanwj referenced this in commit
ef4c7c4e0b
on Nov 2, 2020
sidhujag referenced this in commit
a6e65a2c6e
on Nov 10, 2020
jasonbcox referenced this in commit
1d9ce97437
on Nov 14, 2020
jasonbcox referenced this in commit
ec3e0eb46d
on Nov 14, 2020
jasonbcox referenced this in commit
8df16fce7c
on Nov 14, 2020
jasonbcox referenced this in commit
e9201a674f
on Nov 14, 2020
jasonbcox referenced this in commit
f71ec45eb0
on Nov 14, 2020
jasonbcox referenced this in commit
9549dd30e3
on Nov 14, 2020
jasonbcox referenced this in commit
58e5c16a1e
on Nov 14, 2020
jasonbcox referenced this in commit
3f54b65837
on Nov 14, 2020
jasonbcox referenced this in commit
fa34b26b08
on Nov 14, 2020
deadalnix referenced this in commit
8d87af1d95
on Nov 15, 2020
jasonbcox referenced this in commit
26d8378ff2
on Nov 15, 2020
jasonbcox referenced this in commit
519c218800
on Nov 15, 2020
jasonbcox referenced this in commit
3c1c535e95
on Nov 15, 2020
jasonbcox referenced this in commit
bdb10f67a2
on Nov 15, 2020
jasonbcox referenced this in commit
d15b9f5026
on Nov 15, 2020
jasonbcox referenced this in commit
a493b3bce9
on Nov 15, 2020
jasonbcox referenced this in commit
479c54739f
on Nov 15, 2020
jasonbcox referenced this in commit
1227eefb4e
on Nov 15, 2020
jasonbcox referenced this in commit
8810118223
on Nov 15, 2020
jasonbcox referenced this in commit
20931601ca
on Nov 16, 2020
jasonbcox referenced this in commit
1031d71bff
on Nov 16, 2020
jasonbcox referenced this in commit
1390ead60f
on Nov 16, 2020
jasonbcox referenced this in commit
8f3b9050ec
on Nov 17, 2020
jasonbcox referenced this in commit
370080ae4a
on Nov 17, 2020
jasonbcox referenced this in commit
39b171eb1f
on Nov 17, 2020
jasonbcox referenced this in commit
c3e6f224b7
on Nov 18, 2020
jasonbcox referenced this in commit
d9256979b0
on Nov 18, 2020
jasonbcox referenced this in commit
79aee7021f
on Nov 18, 2020
jasonbcox referenced this in commit
52063277f8
on Nov 18, 2020
jasonbcox referenced this in commit
69c82b89f1
on Nov 19, 2020
jasonbcox referenced this in commit
f18cf5b360
on Nov 19, 2020
jasonbcox referenced this in commit
0fcd2b0ffa
on Nov 19, 2020
jasonbcox referenced this in commit
2ab0aaff66
on Nov 19, 2020
jasonbcox referenced this in commit
73cf12785f
on Nov 19, 2020
jasonbcox referenced this in commit
36c7820069
on Nov 19, 2020
deadalnix referenced this in commit
8b377d31cd
on Nov 19, 2020
jasonbcox referenced this in commit
a58b36f37f
on Nov 19, 2020
jasonbcox referenced this in commit
f53059d18e
on Nov 19, 2020
jasonbcox referenced this in commit
ecaa314bf7
on Nov 21, 2020
jasonbcox referenced this in commit
c321c8ac89
on Nov 21, 2020
jasonbcox referenced this in commit
f24e79e45b
on Nov 23, 2020
Mengerian referenced this in commit
2cfbab661d
on Jan 5, 2021
Fabcien referenced this in commit
b740449f35
on Jan 27, 2021
Fabcien referenced this in commit
e854278f6d
on Oct 5, 2021
Fabcien referenced this in commit
63f2f3ea1c
on Jan 26, 2022
PiRK referenced this in commit
09bb4d91b9
on Aug 16, 2022
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2025-06-03 09:12 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me