Native Descriptor Wallets using DescriptorScriptPubKeyMan

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

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)?

https://github.com/bitcoin/bitcoin/blob/master/src/rpc/misc.cpp#L215 https://github.com/bitcoin/bitcoin/blob/master/src/wallet/rpcdump.cpp#L1122

achow101 force-pushed on Aug 12, 2019

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 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 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:

0# Drop coins on a random address in the default wallet
1bitcoin-cli -regtest -rpcwallet="" generatetoaddress 101 `bitcoin-cli -regtest -rpcwallet="" getnewaddress`
2src/bitcoin-cli -regtest createwallet T false true "" true true
3src/bitcoin-cli -regtest -rpcwallet=T importdescriptors '[{"desc": "wpkh([00000001/84h/1h/0h]tprv8ZgxMBicQKsPd7Uf69XL1XwhmjHopUGep8GuEiJDZmbQz6o58LninorQAfcKZWARbtRtfnLcJ5MQ2AtHcQJCCRUcMRvmDUjyEmNUWwx8UbK/0/*)#slxu8rmv", "timestamp": 0, "range": [0,1], "internal": false, "active": true}]'
4bitcoin-cli -regtest -rpcwallet=T getnewaddress
5bitcoin-cli -regtest -rpcwallet=T getaddressinfo bcrt1qm90ugl4d48jv8n6e5t9ln6t9zlpm5th68x4f8g
6src/bitcoin-cli -regtest -rpcwallet="" sendtoaddress bcrt1qm90ugl4d48jv8n6e5t9ln6t9zlpm5th68x4f8g 1
7bitcoin-cli -regtest -rpcwallet="" generatetoaddress 1 `bitcoin-cli -regtest -rpcwallet="" getnewaddress`
8bitcoin-cli -regtest -rpcwallet=T listunspent 0 9999999 '[]' true

The last command should show the UTXO.

Create another wallet and repeat the importdescriptors call. UTXO won’t show up:

0src/bitcoin-cli -regtest createwallet T2 false true "" true true
1src/bitcoin-cli -regtest -rpcwallet=T2 importdescriptors '[{"desc": "wpkh([00000001/84h/1h/0h]tprv8ZgxMBicQKsPd7Uf69XL1XwhmjHopUGep8GuEiJDZmbQz6o58LninorQAfcKZWARbtRtfnLcJ5MQ2AtHcQJCCRUcMRvmDUjyEmNUWwx8UbK/0/*)#slxu8rmv", "timestamp": 0, "range": [0,1], "internal": false, "active": true}]'
2bitcoin-cli -regtest -rpcwallet=T2 listunspent 0 9999999 '[]' true

When you send it new coins, those do show up.

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 Dec 6, 2019

achow101 force-pushed on Dec 13, 2019

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

I wrote a test to check sortedmulti() origins: https://github.com/Sjors/bitcoin/commit/04610844704aee8fb7cd6f94167a767f5473d281

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

Some of my earlier comments are still relevant:

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 dd92677895 outdated

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"

Sjors commented at 1:53 pm on January 30, 2020:

Whether it’s P2SH depends on address_type, so maybe just say “The descriptor for this multisig address"

achow101 commented at 10:32 pm on January 30, 2020:

Dropped the commit, but fixed in the separate PR for descriptors in createmultisig

in src/wallet/scriptpubkeyman.cpp:1489 in e40a833c3b outdated

1485@@ -1486,11 +1486,6 @@ bool DescriptorScriptPubKeyMan::CanGetAddresses(bool internal)
1486     return false;
1487 }
1488 
1489-bool DescriptorScriptPubKeyMan::Upgrade(int prev_version, std::string& error)

Sjors commented at 2:34 pm on January 30, 2020:

In e40a833c3bffb0cf723357238eb809398e3e5237 Create LegacyScriptPubKeyMan when not a descriptor wallet: accidentally dropping Upgrade()?

achow101 commented at 10:32 pm on January 30, 2020:

It should be dropped, but not in that commit. Moved it to the dummy class definition.

in src/wallet/scriptpubkeyman.h:483 in 3f6cbc5bb1 outdated

478+
479+    bool CheckDecryptionKey(const CKeyingMaterial& master_key, bool accept_no_keys = false) override;
480+    bool Encrypt(const CKeyingMaterial& master_key, WalletBatch* batch) override;
481+
482+    bool GetReservedDestination(const OutputType type, bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool) override;
483+    void KeepDestination(int64_t index) override;

Sjors commented at 2:38 pm on January 30, 2020:

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 e40a833c3bffb0cf723357238eb809398e3e5237 Create 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 f83fde4e8e13a254083e9433dfa770a8b52b1053 Create 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 ee6980be43 outdated

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 };
470 
471+class DescriptorScriptPubKeyMan : public ScriptPubKeyMan
472+{
473+public:
474+    using ScriptPubKeyMan::ScriptPubKeyMan;

Sjors commented at 4:36 pm on January 31, 2020:

In ee6980be437c09df6044a5ced7caa91410453027 Introduce 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.

in src/wallet/walletdb.h:64 in 7af1a56956 outdated

59@@ -60,8 +60,10 @@ extern const std::string CRYPTED_KEY;
60 extern const std::string CSCRIPT;
61 extern const std::string DEFAULTKEY;
62 extern const std::string DESTDATA;
63+extern const std::string EXTERNALSPK;

Sjors commented at 4:40 pm on January 31, 2020:

nit: these are added, but not used, in 7af1a569567a595dc26c33075632f406c4ff0904 Store 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:

suggestion: ACTIVEEXTERNALSPK or something

achow101 commented at 8:31 pm on March 25, 2020:

Done

in src/wallet/scriptpubkeyman.h:476 in 7af1a56956 outdated

469@@ -470,8 +470,17 @@ class LegacySigningProvider : public SigningProvider
470 
471 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

Sjors commented at 4:50 pm on January 31, 2020:

In 7af1a569567a595dc26c33075632f406c4ff0904: ScriptPubKeyMap’s uint32_t changes to int32_t in e7e62209a176a00a03657e6fe8fd221bf2821bf5

achow101 commented at 7:35 pm on February 11, 2020:

Changed

in src/wallet/scriptpubkeyman.h:495 in ee6980be43 outdated

490+
491+    bool SetupGeneration(bool force = false) override;
492+
493+    bool HavePrivateKeys() const override;
494+
495+    void RewriteDB() override;

Sjors commented at 6:09 pm on January 31, 2020:

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 d6f0c337f5bbf935cd93ed3884f5c10bcaa5d493 Implement 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

Related 31 January wallet meeting discussion: http://www.erisian.com.au/meetbot/bitcoin-core-dev/2020/bitcoin-core-dev.2020-01-31-19.00.log.html

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 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 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 91d4efc842 outdated

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);

Sjors commented at 2:30 pm on February 19, 2020:

Should this be GetSolvingProvider for watch-only wallets? I’m having difficulty getting bip32 keys added to the PSBT while trying to rebase #16546 , see https://github.com/Sjors/bitcoin/commit/8660421cba60de3dd5ab76c8f285bce1f0b4327b (maybe I should try less of a hack and actually make that subclass…)

Sjors commented at 4:04 pm on February 19, 2020:

Once #18180 is implemented that reduce at least some of my confusion :-)

Sjors commented at 5:16 pm on February 21, 2020:

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

I’ve added #18204 into this now

achow101 force-pushed on Feb 27, 2020

hugohn commented at 10:46 am on February 27, 2020: contributor

Not exactly related but I got a crash due to an assert() failing when I create a native descriptor wallet with avoid_reuse=true (5th parameter)

./bitcoin-cli -testnet createwallet test_wallet true true "" true true

https://github.com/bitcoin/bitcoin/blob/652ffb49f1fdd9be804990e9e350fd8f647ceb81/src/wallet/wallet.cpp#L754

achow101 force-pushed on Feb 27, 2020

achow101 commented at 7:40 pm on February 27, 2020: member

Not exactly related but I got a crash due to an assert() failing when I create a native descriptor wallet with avoid_reuse=true (5th parameter)

./bitcoin-cli -testnet createwallet test_wallet true true "" true true

I can’t replicate this.

hugohn commented at 4:14 am on February 28, 2020: contributor

hmm I synced to tip and now couldn’t reproduce either…

FWIW, it crashed after restarting bitcoind with -testnet -wallet=test_wallet, not when I created the wallet. Will report if reproducible.

in src/wallet/wallet.cpp:4496 in 1b4fd1424f outdated

4491+    WalletLogPrintf("Update existing descriptor: %s\n", desc.descriptor->ToString());
4492+    LOCK2(cs_wallet, spk_manager->cs_desc_man);
4493+
4494+    // Clear the cache if necessary
4495+    if (desc.range_start < spk_manager->GetWalletDescriptor().range_start) {
4496+        spk_manager->ClearDescriptorCache();

Sjors commented at 12:22 pm on February 28, 2020:

Is still necessary now that you’re no longer caching individual keys? If not, then it looks like you can drop ClearDescriptorCache().

Sjors commented at 12:42 pm on February 28, 2020:

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 1b4fd1424f outdated

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());

Sjors commented at 12:28 pm on February 28, 2020:

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.

Sjors commented at 12:41 pm on February 28, 2020:

Nvm, we still cache individual keys for /*h descriptors.

achow101 commented at 5:57 pm on February 28, 2020:

Every Expand will also return the parent xpub. We don’t want to be constantly rewriting that.

instagibbs commented at 4:41 pm on March 4, 2020:

Even if it was “unnecessary” I think it makes the code less brittle to future changes anyways.

in src/wallet/walletutil.h:9 in 4d7998695f outdated

5@@ -6,6 +6,7 @@
6 #define BITCOIN_WALLET_WALLETUTIL_H
7 
8 #include <fs.h>
9+#include <script/descriptor.h>

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 4d7998695f outdated

3867@@ -3868,8 +3868,10 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
3868 
3869         walletInstance->SetWalletFlags(wallet_creation_flags, false);
3870 
3871-        // 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 e6af0ba62b outdated

523@@ -495,6 +524,14 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
524         result = DBErrors::CORRUPT;
525     }
526 
527+    // 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 e6af0ba62b outdated

523@@ -495,6 +524,14 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
524         result = DBErrors::CORRUPT;
525     }
526 
527+    // 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 630a5308c8 outdated

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 630a5308c8 outdated

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 630a5308c8 outdated

1627+    std::string error;
1628+    std::unique_ptr<Descriptor> desc = Parse(desc_str, keys, error, false);
1629+    WalletDescriptor w_desc(std::move(desc), creation_time, 0, 0, 0);
1630+    descriptor = w_desc;
1631+
1632+    // Store the master private key, derived intermediate key, and descriptor

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 8f282ebaf0 outdated

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;

Sjors commented at 9:22 am on February 29, 2020:

Try ExpandFromCache( before Expand(?

achow101 commented at 7:31 pm on March 2, 2020:

Done

in src/wallet/scriptpubkeyman.cpp:1505 in 8f282ebaf0 outdated

1500+void LegacyScriptPubKeyMan::SetType(OutputType type, bool internal) {}
1501+
1502+bool DescriptorScriptPubKeyMan::GetNewDestination(const OutputType type, CTxDestination& dest, std::string& error)
1503+{
1504+    if (!CanGetAddresses(internal)) {
1505+        error = "No private keys available";

Sjors commented at 9:27 am on February 29, 2020:

“No addresses available”

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.

achow101 commented at 7:31 pm on March 2, 2020:

Done. Added a comment.

in src/wallet/scriptpubkeyman.h:495 in 8f282ebaf0 outdated

492+    using ScriptPubKeyMap = std::map<CScript, int32_t>; // Map of scripts to descriptor range index
493+    using CryptedKeyMap = std::map<CKeyID, std::pair<CPubKey, std::vector<unsigned char>>>;
494+    using KeyMap = std::map<CKeyID, CKey>;
495+
496+    ScriptPubKeyMap m_map_script_pub_keys GUARDED_BY(cs_desc_man);
497+    int32_t m_max_desc_index = -1;

Sjors commented at 9:51 am on February 29, 2020:

m_max_cached_index?

achow101 commented at 7:31 pm on March 2, 2020:

done

in src/wallet/scriptpubkeyman.cpp:1520 in 8f282ebaf0 outdated

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)) {

Sjors commented at 9:58 am on February 29, 2020:

Do this before (or instead of) CanGetAddresses() for readability?

achow101 commented at 7:31 pm on March 2, 2020:

I added a comment earlier so that should help. I would prefer to keep the current pattern of always doing CanGetAddresses first.

in src/wallet/scriptpubkeyman.h:546 in 8f282ebaf0 outdated

531+    bool Encrypt(const CKeyingMaterial& master_key, WalletBatch* batch) override;
532+
533+    bool GetReservedDestination(const OutputType type, bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool) override;
534+    void ReturnDestination(int64_t index, bool internal, const CTxDestination& addr) override;
535+
536+    bool TopUp(unsigned int size = 0) override;

Sjors commented at 10:03 am on February 29, 2020:

Worth documenting that:

0This tops up the descriptor cache (and `m_map_script_pub_keys`).
1The cache is stored in the wallet payload
2and used to expand the descriptor upon wallet load. A descriptor
3ScriptPubKeyMan may rely more on ephemeral data than its legacy keypool
4counterpart. For wallets without private keys and with unhardened derivation, the
5keypool is aved as a single xpub, and therefore Topup() does not increase storage
6size.

achow101 commented at 7:32 pm on March 2, 2020:

Added a similar comment.

Sjors commented at 10:19 am on February 29, 2020: member

Some comments while I was looking at this during #18204 review.

achow101 force-pushed on Mar 2, 2020

achow101 force-pushed on Mar 3, 2020

achow101 force-pushed on Mar 4, 2020

achow101 force-pushed on Mar 8, 2020

DrahtBot added the label Needs rebase on Mar 9, 2020

achow101 force-pushed on Mar 10, 2020

DrahtBot removed the label Needs rebase on Mar 10, 2020

achow101 force-pushed on Mar 14, 2020

achow101 commented at 7:37 pm on March 16, 2020: member

Rebased following #18204 merge, so this is ready for review.

laanwj added this to the "Blockers" column in a project

in src/wallet/scriptpubkeyman.h:488 in 10d7447356 outdated

483@@ -484,8 +484,17 @@ class LegacySigningProvider : public SigningProvider
484 
485 class DescriptorScriptPubKeyMan : public ScriptPubKeyMan
486 {
487+private:
488+    WalletDescriptor descriptor GUARDED_BY(cs_desc_man);

instagibbs commented at 7:42 pm on March 20, 2020:

suggestion: s/descriptor/m_wallet_descriptor/

achow101 commented at 8:31 pm on March 25, 2020:

Done

in src/wallet/walletdb.cpp:188 in 6fa9c482c5 outdated

181@@ -179,6 +182,15 @@ bool WalletBatch::WriteMinVersion(int nVersion)
182     return WriteIC(DBKeys::MINVERSION, nVersion);
183 }
184 
185+bool WalletBatch::WriteActiveScriptPubKeyMan(uint8_t type, const uint256& id, bool internal)

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.

Sjors commented at 6:02 pm on March 27, 2020:

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?

achow101 commented at 10:11 pm on March 27, 2020:

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 6fa9c482c5 outdated

200@@ -189,6 +201,8 @@ class CWalletScanState {
201     bool fIsEncrypted{false};
202     bool fAnyUnordered{false};
203     std::vector<uint256> vWalletUpgrade;
204+    std::map<OutputType, uint256> m_external_spk_managers;
205+    std::map<OutputType, uint256> m_internal_spk_managers;

instagibbs commented at 8:10 pm on March 20, 2020:

suggestion: s/m_internal_spk_managers/m_active_internal_spkm/

achow101 commented at 8:31 pm on March 25, 2020:

Done

in src/wallet/walletdb.cpp:246 in 6fa9c482c5 outdated

200@@ -189,6 +201,8 @@ class CWalletScanState {
201     bool fIsEncrypted{false};
202     bool fAnyUnordered{false};
203     std::vector<uint256> vWalletUpgrade;
204+    std::map<OutputType, uint256> m_external_spk_managers;

instagibbs commented at 8:11 pm on March 20, 2020:

suggestion: s/m_external_spk_managers/m_active_external_spkm/

achow101 commented at 8:31 pm on March 25, 2020:

Done

in src/wallet/walletdb.cpp:475 in 6fa9c482c5 outdated

422+            uint256 id;
423+            ssValue >> id;
424+
425+            bool internal = strType == DBKeys::INTERNALSPK;
426+            auto& spk_mans = internal ? wss.m_internal_spk_managers : wss.m_external_spk_managers;
427+            spk_mans[static_cast<OutputType>(type)] = id;

instagibbs commented at 8:12 pm on March 20, 2020:

should this get upset or log if there’s already one in the map?

achow101 commented at 8:48 pm on March 25, 2020:

Added a check that will give an error.

in src/wallet/walletdb.h:67 in 6fa9c482c5 outdated

59@@ -60,8 +60,10 @@ extern const std::string CRYPTED_KEY;
60 extern const std::string CSCRIPT;
61 extern const std::string DEFAULTKEY;
62 extern const std::string DESTDATA;
63+extern const std::string EXTERNALSPK;
64 extern const std::string FLAGS;
65 extern const std::string HDCHAIN;
66+extern const std::string INTERNALSPK;

instagibbs commented at 8:17 pm on March 20, 2020:

suggestion: ACTIVEINTERNALSPK or something

achow101 commented at 8:31 pm on March 25, 2020:

Done

in src/wallet/scriptpubkeyman.cpp:1548 in 7eaee4bd84 outdated

1505@@ -1506,6 +1506,10 @@ bool DescriptorScriptPubKeyMan::GetNewDestination(const OutputType type, CTxDest
1506 
1507 isminetype DescriptorScriptPubKeyMan::IsMine(const CScript& script) const
1508 {
1509+    LOCK(cs_desc_man);
1510+    if (m_map_script_pub_keys.count(script) > 0) {

instagibbs commented at 8:21 pm on March 20, 2020:

sheds single tear for simplicity

in src/wallet/scriptpubkeyman.cpp:1736 in 71f9ce5e93 outdated

1538@@ -1539,6 +1539,17 @@ bool DescriptorScriptPubKeyMan::TopUp(unsigned int size)
1539 
1540 void DescriptorScriptPubKeyMan::MarkUnusedAddresses(const CScript& script)
1541 {
1542+    LOCK(cs_desc_man);
1543+    if (IsMine(script)) {

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.

Sjors commented at 6:24 pm on March 27, 2020:

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 4a0b2986f4 outdated

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?

achow101 commented at 4:39 pm on March 25, 2020:

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 4a0b2986f4 outdated

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     }
562 
563+    // 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

achow101 commented at 8:47 pm on March 25, 2020:

Done

in src/wallet/walletdb.cpp:435 in 4a0b2986f4 outdated

431@@ -430,7 +432,33 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
432             ssKey >> id;
433             WalletDescriptor desc;
434             ssValue >> desc;
435+            wss.m_descriptor_caches[id] = DescriptorCache();

instagibbs commented at 1:21 pm on March 25, 2020:

this means the DBKeys::WALLETDESCRIPTOR must be an earlier record in all cases? Probably deserves a note.

achow101 commented at 8:31 pm on March 25, 2020:

Done

in src/wallet/scriptpubkeyman.cpp:1564 in d117cb1078 outdated

1559@@ -1560,12 +1560,14 @@ bool DescriptorScriptPubKeyMan::IsHDEnabled() const
1560 
1561 bool DescriptorScriptPubKeyMan::CanGetAddresses(bool internal) const
1562 {
1563-    return false;
1564+    LOCK(cs_desc_man);
1565+    return HavePrivateKeys() || descriptor.next_index < descriptor.range_end;

instagibbs commented at 1:39 pm on March 25, 2020:

why is HavePrivateKeys() sufficient here for non-HD?

achow101 commented at 7:02 pm on March 25, 2020:

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?

achow101 commented at 6:02 pm on March 26, 2020:

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 915da4faba outdated

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:

brackets for this conditional block please

achow101 commented at 8:32 pm on March 25, 2020:

Done

in src/wallet/scriptpubkeyman.cpp:1559 in 915da4faba outdated

1555+    LOCK(cs_desc_man);
1556+    unsigned int target_size;
1557+    if (size > 0)
1558+        target_size = size;
1559+    else
1560+        target_size = std::max(gArgs.GetArg("-keypool", DEFAULT_KEYPOOL_SIZE), (int64_t) 0);

instagibbs commented at 2:36 pm on March 25, 2020:

just make the second argument 1, then delete the line below?

achow101 commented at 8:32 pm on March 25, 2020:

Done

in src/wallet/scriptpubkeyman.cpp:1563 in 915da4faba outdated

1559+    else
1560+        target_size = std::max(gArgs.GetArg("-keypool", DEFAULT_KEYPOOL_SIZE), (int64_t) 0);
1561+    target_size = std::max((int32_t)target_size, 1);
1562+
1563+    // Calculate the new range_end
1564+    int32_t new_range_end = std::max(descriptor.next_index + (int32_t)target_size, descriptor.range_end);

instagibbs commented at 2:39 pm on March 25, 2020:

is it important to not set descriptor.range_end here?

achow101 commented at 7:06 pm on March 25, 2020:

In case we exit early, we don’t want to update the descriptor.

in src/wallet/scriptpubkeyman.cpp:1580 in 915da4faba outdated

1576+    WalletBatch batch(m_storage.GetDatabase());
1577+    uint256 id = GetID();
1578+    for (int32_t i = m_max_cached_index + 1; i < new_range_end; ++i) {
1579+        FlatSigningProvider out_keys;
1580+        std::vector<CScript> scripts_temp;
1581+        DescriptorCache cache;

instagibbs commented at 2:52 pm on March 25, 2020:

suggested rename to temp_cache or something to make it visually easier to track what’s what

achow101 commented at 8:32 pm on March 25, 2020:

Done

instagibbs commented at 3:33 pm on March 25, 2020: member

reviewed to “Implement GetNewDestination for DescriptorScriptPubKeyMan” then lost focus

achow101 force-pushed on Mar 25, 2020

instagibbs commented at 2:52 pm on March 26, 2020: member

all changes until "Implement GetNewDestination for DescriptorScriptPubKeyMan" 0b4b742dd57b6413617368b013350b5397711f9f look correct.

in src/wallet/scriptpubkeyman.h:497 in 8da5fdc34a outdated

492+    using KeyMap = std::map<CKeyID, CKey>;
493+
494+    ScriptPubKeyMap m_map_script_pub_keys GUARDED_BY(cs_desc_man);
495+    int32_t m_max_cached_index = -1;
496+
497+    OutputType address_type;

instagibbs commented at 3:55 pm on March 26, 2020:

m_address_type

achow101 commented at 6:03 pm on March 26, 2020:

Done

in src/wallet/scriptpubkeyman.h:498 in 8da5fdc34a outdated

493+
494+    ScriptPubKeyMap m_map_script_pub_keys GUARDED_BY(cs_desc_man);
495+    int32_t m_max_cached_index = -1;
496+
497+    OutputType address_type;
498+    bool internal;

instagibbs commented at 3:55 pm on March 26, 2020:

m_internal

achow101 commented at 6:03 pm on March 26, 2020:

Done

in src/wallet/scriptpubkeyman.cpp:1531 in 0b4b742dd5 outdated

1527+            // We can't generate anymore keys
1528+            error = "Error: Keypool ran out, please call keypoolrefill first";
1529+            return false;
1530+        }
1531+
1532+        if (m_wallet_descriptor.descriptor->IsSingleType()) {

instagibbs commented at 3:57 pm on March 26, 2020:

just assert this at the top of the function?

achow101 commented at 6:03 pm on March 26, 2020:

Done

in src/wallet/scriptpubkeyman.cpp:1511 in 0b4b742dd5 outdated

1507+        error = "No addresses available";
1508+        return false;
1509+    }
1510+    {
1511+        LOCK(cs_desc_man);
1512+        if (m_wallet_descriptor.descriptor->IsSingleType() && type != address_type) {

instagibbs commented at 3:58 pm on March 26, 2020:

we should only be SingleType, right? Assert earlier and then just do simple type check?

achow101 commented at 6:05 pm on March 26, 2020:

Done

in src/wallet/walletutil.h:96 in 8da5fdc34a outdated

91+class WalletDescriptor
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)

instagibbs commented at 4:10 pm on March 26, 2020:

Note if and when this value can change

achow101 commented at 6:05 pm on March 26, 2020:

Done

in src/wallet/walletutil.h:97 in 8da5fdc34a outdated

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?

achow101 commented at 6:05 pm on March 26, 2020:

Done

in src/wallet/scriptpubkeyman.cpp:1522 in 0b4b742dd5 outdated

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?

achow101 commented at 6:08 pm on March 26, 2020:

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 40960f9316 outdated

1945@@ -1946,9 +1946,60 @@ SigningResult DescriptorScriptPubKeyMan::SignMessage(const std::string& message,
1946     return SigningResult::OK;
1947 }
1948 
1949-TransactionError DescriptorScriptPubKeyMan::FillPSBT(PartiallySignedTransaction& psbt, int sighash_type, bool sign, bool bip32derivs) const
1950+TransactionError DescriptorScriptPubKeyMan::FillPSBT(PartiallySignedTransaction& psbtx, int sighash_type, bool sign, bool bip32derivs) const

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 339cc0e6b7 outdated

1219@@ -1220,6 +1220,9 @@ class CWallet final : public WalletStorage, private interfaces::Chain::Notificat
1220 
1221     //! 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:

Descriptor*

achow101 commented at 6:08 pm on March 26, 2020:

Fixed

instagibbs commented at 5:16 pm on March 26, 2020: member

reviewed through:

0commit d40617bf4c69d10bf9714ab1f653fc5b740dbf63
1Author: Andrew Chow <achow101-github@achow101.com>
2Date:   Wed Aug 14 14:25:53 2019 -0400
3
4    Add IsLegacy to CWallet so that the GUI knows whether to show watchonly

achow101 force-pushed on Mar 26, 2020

instagibbs commented at 7:00 pm on March 26, 2020: member

changes through Add IsLegacy to CWallet so that the GUI knows whether to show watchonly https://github.com/bitcoin/bitcoin/pull/16528/commits/1687d1a1681fc1f6cfaa23a48ea99aa0b07acb4a look correct

in src/wallet/wallet.h:1231 in 7d1c6bbeb0 outdated

1225@@ -1226,6 +1226,12 @@ class CWallet final : public WalletStorage, private interfaces::Chain::Notificat
1226 
1227     //! 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:

suggested rename: GetWalletDescriptor

achow101 commented at 9:11 pm on March 27, 2020:

Done

in src/wallet/wallet.cpp:4454 in 7d1c6bbeb0 outdated

4447@@ -4447,3 +4448,91 @@ bool CWallet::IsLegacy() const
4448     auto spk_man = dynamic_cast<LegacyScriptPubKeyMan*>(m_internal_spk_managers.at(OutputType::LEGACY));
4449     return spk_man != nullptr;
4450 }
4451+
4452+DescriptorScriptPubKeyMan* CWallet::HasWalletDescriptor(const WalletDescriptor& desc) const
4453+{
4454+    if (!IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) {

instagibbs commented at 1:54 pm on March 27, 2020:

in add importdescriptors RPC and tests for native descriptor wallets:

nit: This little code block isn’t really necessary. It’ll fail to get non-null spk_manager for the single legacy spkm.

achow101 commented at 9:11 pm on March 27, 2020:

Done

in src/wallet/wallet.cpp:4477 in 7d1c6bbeb0 outdated

4472+        WalletLogPrintf("Cannot add WalletDescriptor to a non-descriptor wallet\n");
4473+        return nullptr;
4474+    }
4475+
4476+    LOCK(cs_wallet);
4477+    auto spk_manager = std::unique_ptr<DescriptorScriptPubKeyMan>(new DescriptorScriptPubKeyMan(*this, desc));

instagibbs commented at 1:56 pm on March 27, 2020:

in add importdescriptors RPC and tests for native descriptor wallets:

s/spk_manager/new_spk_man to line up name with old_spk_man

achow101 commented at 9:11 pm on March 27, 2020:

Done

in src/wallet/wallet.cpp:4472 in 7d1c6bbeb0 outdated

4487+        }
4488+
4489+        // Remove from maps of spkMans
4490+        for (bool internal : {false, true}) {
4491+            for (OutputType t : OUTPUT_TYPES) {
4492+                auto active_spk_man = GetScriptPubKeyMan(t, internal);

instagibbs commented at 2:06 pm on March 27, 2020:

Not this PR’s fault, but this should really be named GetActiveScriptPubKeyMan even though the args make it implicit.

instagibbs commented at 2:08 pm on March 27, 2020:

another not-PR comment: m_internal_spk_managers and m_external_spk_managers should also be marked as active, at least in comments.

in src/wallet/wallet.cpp:4491 in a03a8cbd1f outdated

4486+        {
4487+            LOCK(old_spk_man->cs_desc_man);
4488+            spk_manager->SetCache(old_spk_man->GetWalletDescriptor().cache);
4489+        }
4490+
4491+        // Remove from maps of spkMans

instagibbs commented at 2:12 pm on March 27, 2020:

of active spkMans

achow101 commented at 9:11 pm on March 27, 2020:

Done

in src/wallet/rpcdump.cpp:1511 in 7d1c6bbeb0 outdated

1506+                    throw JSONRPCError(RPC_INVALID_PARAMETER, "next_index is out of range");
1507+                }
1508+            }
1509+        }
1510+
1511+        // Active descriptor must be ranged

instagibbs commented at 2:28 pm on March 27, 2020:

add importdescriptors RPC and tests for native descriptor wallets:

s/Active descriptor/Active descriptors/

achow101 commented at 9:12 pm on March 27, 2020:

Done

in src/wallet/rpcdump.cpp:1517 in 7d1c6bbeb0 outdated

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:

needs test

achow101 commented at 9:12 pm on March 27, 2020:

Done

in src/wallet/rpcdump.cpp:1522 in 7d1c6bbeb0 outdated

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

needs test

achow101 commented at 9:12 pm on March 27, 2020:

Done

in src/wallet/rpcdump.cpp:1527 in 7d1c6bbeb0 outdated

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

needs test

achow101 commented at 9:12 pm on March 27, 2020:

Done

in src/wallet/rpcdump.cpp:1538 in 7d1c6bbeb0 outdated

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

needs test

achow101 commented at 9:12 pm on March 27, 2020:

Done

in test/functional/wallet_importdescriptors.py:209 in 7d1c6bbeb0 outdated

204+                              'range' : [0, 2],
205+                              'timestamp': 'now'
206+                             },
207+                             success=True)
208+
209+        for i in range(0, 4):

instagibbs commented at 2:56 pm on March 27, 2020:

add importdescriptors RPC and tests for native descriptor wallets

can you calculate 4 in code instead of magic?

achow101 commented at 9:13 pm on March 27, 2020:

Iterated over addresses instead.

in test/functional/wallet_importdescriptors.py:210 in 7d1c6bbeb0 outdated

205+                              'timestamp': 'now'
206+                             },
207+                             success=True)
208+
209+        for i in range(0, 4):
210+            addr = w1.getnewaddress('', 'bech32')

instagibbs commented at 2:57 pm on March 27, 2020:

add importdescriptors RPC and tests for native descriptor wallets

we should also check import didn’t somehow add an internal spkm with change addresses

achow101 commented at 9:13 pm on March 27, 2020:

Done

in test/functional/wallet_importdescriptors.py:319 in 7d1c6bbeb0 outdated

243+                            "timestamp": "now"},
244+                            success=True,
245+                            wallet=wmulti_priv)
246+        self.test_importdesc({"desc":"wsh(multi(2,tprv8ZgxMBicQKsPevADjDCWsa6DfhkVXicu8NQUzfibwX2MexVwW4tCec5mXdCW8kJwkzBRRmAay1KZya4WsehVvjTGVW6JLqiqd8DdZ4xSg52/84h/1h/0h/*,tprv8ZgxMBicQKsPdSNWUhDiwTScDr6JfkZuLshTRwzvZGnMSnGikV6jxpmdDkC3YRc4T3GD6Nvg9uv6hQg73RVv1EiTXDZwxVbsLugVHU8B1aq/84h/1h/0h/*,tprv8ZgxMBicQKsPeonDt8Ka2mrQmHa61hQ5FQCsvWBTpSNzBFgM58cV2EuXNAHF14VawVpznnme3SuTbA62sGriwWyKifJmXntfNeK7zeqMCj1/84h/1h/0h/*))#q3sztvx5",
247+                            "active": True,
248+                            "internal" : True,

instagibbs commented at 2:59 pm on March 27, 2020:

add importdescriptors RPC and tests for native descriptor wallets

should test the fact that correct change addresses can be drawn now

achow101 commented at 9:13 pm on March 27, 2020:

Done

in test/functional/wallet_importdescriptors.py:353 in 7d1c6bbeb0 outdated

273+                            "timestamp": "now"},
274+                            success=True,
275+                            wallet=wmulti_pub)
276+        self.test_importdesc({"desc":"wsh(multi(2,[7b2d0242/84h/1h/0h]tpubDCXqdwWZcszwqYJSnZp8eARkxGJfHAk23KDxbztV4BbschfaTfYLTcSkSJ3TN64dRqwa1rnFUScsYormKkGqNbbPwkorQimVevXjxzUV9Gf/*,[59b09cd6/84h/1h/0h]tpubDCYfZY2ceyHzYzMMVPt9MNeiqtQ2T7Uyp9QSFwYXh8Vi9iJFYXcuphJaGXfF3jUQJi5Y3GMNXvM11gaL4txzZgNGK22BFAwMXynnzv4z2Jh/*,[e81a0532/84h/1h/0h]tpubDC6UGqnsQStngYuGD4MKsMy7eD1Yg9NTJfPdvjdG2JE5oZ7EsSL3WHg4Gsw2pR5K39ZwJ46M1wZayhedVdQtMGaUhq5S23PH6fnENK3V1sb/*))#c08a2rzv",
277+                            "active": True,
278+                            "internal" : True,

instagibbs commented at 2:59 pm on March 27, 2020:

add importdescriptors RPC and tests for native descriptor wallets

should test the fact that correct change addresses can be drawn now

achow101 commented at 9:13 pm on March 27, 2020:

Done

in test/functional/wallet_importdescriptors.py:70 in 7d1c6bbeb0 outdated

63+        w1 = self.nodes[1].get_wallet_rpc('w1')
64+        assert_equal(w1.getwalletinfo()['keypoolsize'], 0)
65+
66+        self.nodes[1].createwallet(wallet_name="wpriv", disable_private_keys=False, blank=True, descriptors=True)
67+        wpriv = self.nodes[1].get_wallet_rpc("wpriv")
68+        assert_equal(wpriv.getwalletinfo()['keypoolsize'], 0)

instagibbs commented at 3:00 pm on March 27, 2020:

add importdescriptors RPC and tests for native descriptor wallets

We should be testing that keypoolsize ends up the expected size post-active-imports, and not before

achow101 commented at 9:13 pm on March 27, 2020:

Done

in test/functional/wallet_importdescriptors.py:139 in 7d1c6bbeb0 outdated

122+                              error_message="Active descriptor must be ranged")
123+
124+        self.log.info("Should import a (non-active) p2sh-p2wpkh descriptor")
125+        self.test_importdesc({"desc": descsum_create("sh(wpkh(" + key.pubkey + "))"),
126+                               "timestamp": "now",
127+                               "active": False,

instagibbs commented at 3:03 pm on March 27, 2020:

add importdescriptors RPC and tests for native descriptor wallets

should test both ranged and non-ranged descriptors (non-active) don’t replace “keypool”

achow101 commented at 9:13 pm on March 27, 2020:

Done

in test/functional/wallet_descriptor.py:36 in 15b044fd16 outdated

30+        self.log.info("Checking wallet info")
31+        wallet_info = self.nodes[0].getwalletinfo()
32+        assert_equal(wallet_info['keypoolsize'], 300)
33+        assert_equal(wallet_info['keypoolsize_hd_internal'], 300)
34+
35+        # Check that getnewaddress works

instagibbs commented at 3:08 pm on March 27, 2020:

need to check they’re actually returning the right type as well for all 3 cases (maybe this is caught in a --descriptor variant of tests)

achow101 commented at 9:13 pm on March 27, 2020:

Done

in test/functional/wallet_importdescriptors.py:149 in 7d1c6bbeb0 outdated

131+        test_address(w1,
132+                     key.p2sh_p2wpkh_addr,
133+                     ismine=True,
134+                     solvable=True)
135+
136+        # # Test importing of a multisig descriptor

instagibbs commented at 3:11 pm on March 27, 2020:

add importdescriptors RPC and tests for native descriptor wallets

We should also test sh(wsh()) and make sure that the various imports aren’t “displacing” the other types. (I didn’t see it might have missed it)

achow101 commented at 9:14 pm on March 27, 2020:

Not sure what you mean by “displacing”

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.

achow101 commented at 9:04 pm on March 28, 2020:

Added a test for that case.

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
3
4    Return error when no ScriptPubKeyMan is available for specified type
5    
6    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”.

that-and-comments-below-otherwise-utACK https://github.com/bitcoin/bitcoin/pull/16528/commits/a03a8cbd1f42636834263135e904900cc808d346

in src/wallet/rpcdump.cpp:1582 in a03a8cbd1f outdated

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”

achow101 commented at 4:53 pm on March 27, 2020:

pk(xpub...) should hit this. I’ll try adding a test.

achow101 commented at 9:14 pm on March 27, 2020:

Added a test.

in src/wallet/rpcdump.cpp:1608 in a03a8cbd1f outdated

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:

default is actually false in the code

achow101 commented at 9:14 pm on March 27, 2020:

Fixed

in test/functional/wallet_importdescriptors.py:6 in a03a8cbd1f outdated

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

achow101 commented at 9:10 pm on March 27, 2020:

How so?

instagibbs commented at 5:13 pm on March 28, 2020:

Make a test to ensure that active is actually default true(or not)

achow101 commented at 9:06 pm on March 28, 2020:

Done

in src/wallet/walletutil.h:110 in f0a15b8aa1 outdated

105+        if (ser_action.ForRead()) {
106+            std::string desc;
107+            std::string error;
108+            READWRITE(desc);
109+            FlatSigningProvider keys;
110+            descriptor = Parse(desc, keys, error, true);

Sjors commented at 5:40 pm on March 27, 2020:

What happens if this fails? Would be good to have a test for a malformed descriptor.

achow101 commented at 11:11 pm on March 27, 2020:

Changed this to check the result and throw an exception. Added a unit test for that.

in src/wallet/walletutil.h:115 in f0a15b8aa1 outdated

107+            std::string error;
108+            READWRITE(desc);
109+            FlatSigningProvider keys;
110+            descriptor = Parse(desc, keys, error, true);
111+        } else {
112+            READWRITE(descriptor->ToString());

Sjors commented at 5:43 pm on March 27, 2020:

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.

achow101 commented at 10:09 pm on March 27, 2020:

Descriptors are inherently strings. There is no alternative serialization that doesn’t eventually result in a descriptor string as we know now.

Sjors commented at 10:19 am on March 28, 2020:

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 ed75ce9649 outdated

1213@@ -1214,6 +1214,12 @@ class CWallet final : public WalletStorage, private interfaces::Chain::Notificat
1214 
1215     //! 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

Sjors commented at 6:06 pm on March 27, 2020:

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)

achow101 commented at 11:12 pm on March 27, 2020:

Added more comments.

in src/wallet/scriptpubkeyman.cpp:1546 in 68cef400b9 outdated

1538@@ -1539,6 +1539,17 @@ bool DescriptorScriptPubKeyMan::TopUp(unsigned int size)
1539 
1540 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__);

Sjors commented at 6:22 pm on March 27, 2020:

Nit: can you add index to the log message?

achow101 commented at 11:12 pm on March 27, 2020:

Done

in src/wallet/scriptpubkeyman.cpp:1851 in fbd356690f outdated

1553@@ -1554,7 +1554,8 @@ void DescriptorScriptPubKeyMan::MarkUnusedAddresses(const CScript& script)
1554 
1555 bool DescriptorScriptPubKeyMan::IsHDEnabled() const
1556 {
1557-    return false;
1558+    LOCK(cs_desc_man);
1559+    return m_wallet_descriptor.descriptor->IsRange();

Sjors commented at 6:28 pm on March 27, 2020:

You may have to change the wallet to use |= instead of &=.

achow101 commented at 10:19 pm on March 27, 2020:

Why?

achow101 commented at 11:12 pm on March 27, 2020:

|= would be incorrect. I’ve instead changed IsHDEnabled to iterate over GetActiveScriptPubKeyMans().

Sjors commented at 10:21 am on March 28, 2020:

If unranged descriptors can’t be active, then that’s fine.

instagibbs commented at 5:18 pm on March 28, 2020:

unranged active descriptors would result in a single new address grab per import, kind of pointless

in src/wallet/scriptpubkeyman.cpp:2080 in 0708467ad1 outdated

1626@@ -1627,4 +1627,8 @@ uint256 DescriptorScriptPubKeyMan::GetID() const
1627     return id;
1628 }
1629 
1630-void DescriptorScriptPubKeyMan::SetType(OutputType type, bool internal) {}
1631+void DescriptorScriptPubKeyMan::SetType(OutputType type, bool internal)

Sjors commented at 6:33 pm on March 27, 2020:

You’re already using this function in ed75ce9649ce7a26146b45bb85584f5522945a3d, so may want to move this commit up (though it does compile).

achow101 commented at 11:12 pm on March 27, 2020:

Done

in src/wallet/scriptpubkeyman.cpp:1643 in 0abf7b960c outdated

1638+    LOCK(cs_desc_man);
1639+    m_wallet_descriptor.cache = cache;
1640+    for (int32_t i = m_wallet_descriptor.range_start; i < m_wallet_descriptor.range_end; ++i) {
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);

Sjors commented at 6:37 pm on March 27, 2020:

Should check if this fails.

achow101 commented at 11:13 pm on March 27, 2020:

Done

in src/wallet/scriptpubkeyman.cpp:2101 in 0abf7b960c outdated

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;

Sjors commented at 6:39 pm on March 27, 2020:

Do we want to panic if m_map_script_pub_keys[script] exists and != i?

achow101 commented at 11:14 pm on March 27, 2020:

Done

in src/wallet/walletdb.cpp:494 in 0abf7b960c outdated

447+            uint32_t der_index;
448+            ssKey >> desc_id;
449+            ssKey >> key_exp_index;
450+
451+            // if the der_index exists, it's a derived xpub
452+            try

Sjors commented at 6:47 pm on March 27, 2020:

I’m not a fan. Let’s just have WALLETDESCRIPTORCACHEPARENTXPUB and WALLETDESCRIPTORCACHEDERIVEDXPUB, and maybe some underscores :-)

achow101 commented at 10:29 pm on March 27, 2020:

I wanted to leave this open to different caching structures in the future and not have a ton of cache structure specific records like that.

Sjors commented at 10:22 am on March 28, 2020:

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.

achow101 commented at 8:54 pm on March 28, 2020:

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 0abf7b960c outdated

568@@ -536,6 +569,13 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
569         pwallet->SetActiveScriptPubKeyMan(spk_man_pair.second, spk_man_pair.first, /* internal */ true, /* memonly */ true);
570     }
571 
572+    // Set the descriptor caches
573+    for (auto desc_cache_pair : wss.m_descriptor_caches) {
574+        auto spk_man = pwallet->GetScriptPubKeyMan(desc_cache_pair.first);
575+        assert(spk_man);
576+        ((DescriptorScriptPubKeyMan*)spk_man)->SetCache(desc_cache_pair.second);

Sjors commented at 6:49 pm on March 27, 2020:

Do we want to check the cache for gaps and emit a warning?

achow101 commented at 10:32 pm on March 27, 2020:

That’s inherently done by not being able to expand from cache during SetCache.

in src/wallet/scriptpubkeyman.cpp:1858 in a03a8cbd1f outdated

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)

Sjors commented at 7:02 pm on March 27, 2020:

Shouldn’t we allow CanGetAddresses from an unranged descriptor once?

achow101 commented at 9:18 pm on March 27, 2020:

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 8ae9bf1239 outdated

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)

Sjors commented at 7:17 pm on March 27, 2020:

for (const auto& mi : m_map_crypted_keys) { and then below mi.second.first; compiles too, though maybe I missed something.

achow101 commented at 11:14 pm on March 27, 2020:

Done

Sjors commented at 6:58 pm on April 22, 2020:

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.

achow101 commented at 10:06 pm on April 22, 2020:

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 ee03d46178 outdated

1615@@ -1616,6 +1616,13 @@ bool DescriptorScriptPubKeyMan::GetReservedDestination(const OutputType type, bo
1616 
1617 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

Sjors commented at 7:20 pm on March 27, 2020:

nit: maybe swap eb9c41e9240fe01f6404cf23abe1fc35ca146586 and ee03d46178a3e8d511c33f734892820b36ed7a1a.

A test would be nice.

achow101 commented at 11:15 pm on March 27, 2020:

Swapped the commits.

A general test for GetReserveDestination and ReturnDestination in the wallet feels out of scope for this PR. IMO should be a separate PR.

Sjors commented at 10:24 am on March 28, 2020:

Doesn’t walletcreatefundedpsbt call those two methods? So the test just has to check the keypool doesn’t shrink.

achow101 commented at 9:03 pm on March 28, 2020:

Again, seems out of scope and a general wallet test rather than specific to descriptor wallets.

in src/wallet/scriptpubkeyman.cpp:1897 in 50c3b4eee3 outdated

1877@@ -1878,9 +1878,34 @@ int64_t DescriptorScriptPubKeyMan::GetTimeFirstKey() const
1878     return m_wallet_descriptor.creation_time;
1879 }
1880 
1881+std::unique_ptr<FlatSigningProvider> DescriptorScriptPubKeyMan::GetSigningProvider(const CScript& script, bool include_private) const

Sjors commented at 7:22 pm on March 27, 2020:

This commit message in 50c3b4eee31e095a5080fcb4109cb5d8f7bfbbe7 speaks of GetSolvingProvider

achow101 commented at 10:36 pm on March 27, 2020:

And? We modify GetSolvingProvider below. This is an internal function.

in src/wallet/scriptpubkeyman.cpp:1937 in 50c3b4eee3 outdated

1895+    if (!m_wallet_descriptor.descriptor->ExpandFromCache(index, m_wallet_descriptor.cache, scripts_temp, *out_keys)) return nullptr;
1896+
1897+    if (include_private) {
1898+        FlatSigningProvider master_provider;
1899+        master_provider.keys = GetKeys();
1900+        m_wallet_descriptor.descriptor->ExpandPrivate(index, master_provider, *out_keys);

Sjors commented at 7:25 pm on March 27, 2020:

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.

achow101 commented at 11:15 pm on March 27, 2020:

Done

in src/qt/createwalletdialog.cpp:64 in f5204ed3fe outdated

59@@ -60,3 +60,8 @@ bool CreateWalletDialog::isMakeBlankWalletChecked() const
60 {
61     return ui->blank_wallet_checkbox->isChecked();
62 }
63+
64+bool CreateWalletDialog::isDescriptorWalletChecked() const

Sjors commented at 7:29 pm on March 27, 2020:

f5204ed3fe1b2a059e35e54248d38eac88c629e0 could be split between GUI and RPC.

achow101 commented at 10:44 pm on March 27, 2020:

Meh, it’s not really a big difference.

Sjors commented at 10:27 am on March 28, 2020:

Yeah, just keep in in mind if something else needs to be changed there, or if - god forbid - we get bike shedding over the new check box.

in src/wallet/walletdb.h:255 in f5204ed3fe outdated

251@@ -252,13 +252,13 @@ class WalletBatch
252     bool WriteDescriptorDerivedCache(const uint256& desc_id, uint32_t key_exp_index, uint32_t der_index, const CExtPubKey& xpub);
253     bool WriteDescriptorParentCache(const uint256& desc_id, uint32_t key_exp_index, const CExtPubKey& xpub);
254 
255+    bool WriteActiveScriptPubKeyMan(uint8_t type, const uint256& id, bool internal);

Sjors commented at 7:31 pm on March 27, 2020:

f5204ed3fe1b2a059e35e54248d38eac88c629e0 moves WriteActiveScriptPubKeyMan a few lines up for some reason.

achow101 commented at 11:15 pm on March 27, 2020:

Removed it.

in src/wallet/wallet.cpp:588 in 2de265c2b6 outdated

584@@ -585,8 +585,11 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)
585         Lock();
586         Unlock(strWalletPassphrase);
587 
588-        // 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

Sjors commented at 7:33 pm on March 27, 2020:

Descriptors don’t contain private keys, so maybe make it more clear that we’re generating a new seed, and thus need new descriptors for that.

achow101 commented at 11:15 pm on March 27, 2020:

Done

in test/functional/wallet_importdescriptors.py:15 in 7d1c6bbeb0 outdated

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

Sjors commented at 7:40 pm on March 27, 2020:

test_address or another function could also test getnewaddress (in particular for an unranged descriptor, which imo should work exactly once)

achow101 commented at 10:47 pm on March 27, 2020:

Disagree, on both counts.

Sjors commented at 10:29 am on March 28, 2020:

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

in src/wallet/scriptpubkeyman.cpp:1905 in d034592cf8 outdated

1900+    if (!m_wallet_descriptor.descriptor->ExpandFromCache(index, m_wallet_descriptor.cache, scripts_temp, *out_keys)) return nullptr;
1901+
1902+    if (include_private) {
1903+        FlatSigningProvider master_provider;
1904+        master_provider.keys = GetKeys();
1905+        assert(m_wallet_descriptor.descriptor->ExpandPrivate(index, master_provider, *out_keys));

instagibbs commented at 5:25 pm on March 28, 2020:

I don’t think we’re supposed to be asserting on a line with state changes, cache the result and then assert below

achow101 commented at 9:07 pm on March 28, 2020:

Done

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 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

It was from Travis run at the time of my review.

On Sat, Mar 28, 2020, 5:12 PM Andrew Chow notifications@github.com wrote:

looks like unit test is failing sometimes:

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”

Not seeing this fail at anytime.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/16528#issuecomment-605520140, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABMAFUZF5UVTJSWLGQKWUKLRJZR3ZANCNFSM4IIZW2TA .

instagibbs commented at 1:31 pm on March 30, 2020: member

re-ACK https://github.com/bitcoin/bitcoin/pull/16528/commits/54be087eebdfefb70c018f67476f2160ae8ca29b

only assert cleanup and additional suggested tests

instagibbs commented at 1:33 pm on March 30, 2020: member

~~direct link to unit test failure: https://travis-ci.org/github/bitcoin/bitcoin/jobs/668186151#L4252~~

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 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 3f5e0bfc5d outdated

1912@@ -1913,7 +1913,16 @@ bool DescriptorScriptPubKeyMan::CanProvide(const CScript& script, SignatureData&
1913 
1914 bool DescriptorScriptPubKeyMan::SignTransaction(CMutableTransaction& tx, const std::map<COutPoint, Coin>& coins, int sighash, std::map<int, std::string>& input_errors) const
1915 {
1916-    return false;
1917+    std::unique_ptr<FlatSigningProvider> keys = MakeUnique<FlatSigningProvider>();
1918+    for (const auto& coin_pair : coins) {
1919+        std::unique_ptr<FlatSigningProvider> coin_keys = std::move(GetSigningProvider(coin_pair.second.out.scriptPubKey, true));

Sjors commented at 11:20 am on March 31, 2020:

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).

0wallet/scriptpubkeyman.cpp:1927:58: warning: moving a temporary object prevents copy elision [-Wpessimizing-move]
1        std::unique_ptr<FlatSigningProvider> coin_keys = std::move(GetSigningProvider(coin_pair.second.out.scriptPubKey, true));
2                                                         ^
3wallet/scriptpubkeyman.cpp:1927:58: note: remove std::move call here
4        std::unique_ptr<FlatSigningProvider> coin_keys = std::move(GetSigningProvider(coin_pair.second.out.scriptPubKey, true));
5                                                         ^~~~~~~~~~                                                           ~

achow101 commented at 7:46 pm on March 31, 2020:

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 f2c3e36479 outdated

1987+            continue;
1988+        }
1989+        SignatureData sigdata;
1990+        input.FillSignatureData(sigdata);
1991+
1992+        std::unique_ptr<FlatSigningProvider> keys = GetSigningProvider(script, true);

instagibbs commented at 1:37 pm on March 31, 2020:

I think the boolean arg should be sign to avoid the assertion failure mentioned by @Sjors

achow101 commented at 7:46 pm on March 31, 2020:

Done

achow101 force-pushed on Mar 31, 2020

in src/wallet/wallet.cpp:759 in 3da39d72a8 outdated

770+        if (GetDestData(dest, "used", nullptr)) {
771+            return true;
772+        }
773+        if (IsLegacy()) {
774+            LegacyScriptPubKeyMan* spk_man = GetLegacyScriptPubKeyMan();
775+            // When descriptor wallets arrive, these additional checks are

instagibbs commented at 7:45 pm on March 31, 2020:

out of date comment

achow101 commented at 8:16 pm on March 31, 2020:

removed

achow101 force-pushed on Mar 31, 2020

achow101 commented at 7:46 pm on March 31, 2020: member

Fixed the IsSpentKey() issue.

instagibbs approved

instagibbs commented at 7:48 pm on March 31, 2020: member

reACK https://github.com/bitcoin/bitcoin/pull/16528/commits/a608207a2c343d70a30e4534783200c8a429ab27

achow101 force-pushed on Mar 31, 2020

instagibbs commented at 9:12 pm on March 31, 2020: member

reACK https://github.com/bitcoin/bitcoin/pull/16528/commits/7ea9f0fb848e3ccc69e05b4c1bad465e73bc1963

in src/wallet/scriptpubkeyman.cpp:1635 in 1dd95d7e91 outdated

1630+    m_wallet_descriptor = w_desc;
1631+
1632+    // Store the master private key, and descriptor
1633+    WalletBatch batch(m_storage.GetDatabase());
1634+    if (!AddDescriptorKeyWithDB(batch, master_key.key, master_key.key.GetPubKey())) {
1635+        throw std::runtime_error(std::string(__func__) + ": writing descriptor master rivate key failed");

Sjors commented at 4:20 pm on April 1, 2020:

Nit 1dd95d7e91fb31ee97cff2aa0f1ccfd820e78e1e: missing p in “master rivate key”

achow101 commented at 8:49 pm on April 1, 2020:

Fixed

in src/wallet/scriptpubkeyman.cpp:1828 in 1dd95d7e91 outdated

1623+    std::string desc_str = desc_prefix + "/0'" + internal_path + desc_suffix;
1624+
1625+    // Make the descriptor
1626+    FlatSigningProvider keys;
1627+    std::string error;
1628+    std::unique_ptr<Descriptor> desc = Parse(desc_str, keys, error, false);

Sjors commented at 4:23 pm on April 1, 2020:

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/*)?

achow101 commented at 4:49 pm on April 1, 2020:

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

ACK 7ea9f0fb848e3ccc69e05b4c1bad465e73bc1963. 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

reACK https://github.com/bitcoin/bitcoin/pull/16528/commits/4fac5c2b382135a86e4c66c05a2abdd912174a50

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

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 e75d67d237 outdated

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

instagibbs commented at 1:08 pm on April 6, 2020:

The index matters, I think you mean:

“It doesn’t matter which of many valid indexes the pubkey has, we just need an index where we can derive it and it’s private key”

achow101 commented at 4:29 pm on April 6, 2020:

Done

in src/wallet/scriptpubkeyman.cpp:2107 in e75d67d237 outdated

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

instagibbs commented at 1:08 pm on April 6, 2020:

The index matters, I think you mean:

“It doesn’t matter which of many valid indexes the pubkey has, we just need an index where we can derive it and it’s private key”

achow101 commented at 4:29 pm on April 6, 2020:

Done

instagibbs commented at 1:20 pm on April 6, 2020: member

tACK https://github.com/bitcoin/bitcoin/pull/16528/commits/e75d67d237241c094f425a999a38e00b5fbba635 the logic changes but I have not reviewed the latest test changes and it appears they’re failing in some cases. Going to defer diving into those until things seem more stable.

The specific logic fixes to support multisig PSBT will also need regression tests.

achow101 force-pushed on Apr 6, 2020

achow101 commented at 4:30 pm on April 6, 2020: member

The travis failures should be fixed. I’m not sure what’s wrong with appveyor, but it seems to be unrelated.

instagibbs commented at 4:32 pm on April 6, 2020: member

reACK https://github.com/bitcoin/bitcoin/pull/16528/commits/7fee7cd034b9c38244a6e284bd3c719482b01a46 with comment changes requested. Still need to review tests top to bottom once things settle.

achow101 force-pushed on Apr 7, 2020

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

Writeup at https://gist.github.com/achow101/94d889715afd49181f8efdca1f9faa25 is fantastic. Really helpful to me, and probably others who want to catch up with this. Better than anything else I’ve read because it focuses more on why than what of the design.

Would be good to link to from project https://github.com/bitcoin/bitcoin/projects/12 and maybe move to the dev wiki

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 a7a814fb59 outdated

1230@@ -1231,6 +1231,12 @@ class CWallet final : public WalletStorage, private interfaces::Chain::Notificat
1231 
1232     //! 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;

fjahr commented at 10:21 pm on April 10, 2020:

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.

achow101 commented at 0:09 am on April 20, 2020:

Renamed it to GetDescriptorScriptPubKeyMan.

in src/wallet/scriptpubkeyman.cpp:1550 in 6edf8249ee outdated

1548@@ -1553,12 +1549,61 @@ isminetype DescriptorScriptPubKeyMan::IsMine(const CScript& script) const
1549 
1550 bool DescriptorScriptPubKeyMan::CheckDecryptionKey(const CKeyingMaterial& master_key, bool accept_no_keys)
1551 {
1552-    return false;
1553+    {

fjahr commented at 10:31 pm on April 10, 2020:

I think these brackets can be removed since they only exclude the return

achow101 commented at 0:09 am on April 20, 2020:

Done

in src/wallet/scriptpubkeyman.cpp:1874 in 7cd6574061 outdated

1851@@ -1852,7 +1852,7 @@ bool DescriptorScriptPubKeyMan::HavePrivateKeys() const
1852 
1853 int64_t DescriptorScriptPubKeyMan::GetOldestKeyPoolTime() const
1854 {
1855-    return GetTime();
1856+    return 0;

fjahr commented at 10:35 pm on April 10, 2020:

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.

achow101 commented at 0:09 am on April 20, 2020:

Added a comment.

in src/wallet/wallet.cpp:3819 in 4049fe4c83 outdated

3818-                    error = _("Unable to generate initial keys").translated;
3819-                    return nullptr;
3820+            if (walletInstance->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) {
3821+                walletInstance->SetupDescriptorScriptPubKeyMans();
3822+            } else {
3823+                // SetupDescriptorScriptPubKeyMans already calls SetupGeneration for us

fjahr commented at 10:48 pm on April 10, 2020:

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.

achow101 commented at 0:09 am on April 20, 2020:

Moved it and added a better one here.

in src/wallet/rpcdump.cpp:1536 in a7a814fb59 outdated

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

fjahr commented at 10:50 pm on April 10, 2020:

Typo: ExpandPrvate

achow101 commented at 0:09 am on April 20, 2020:

Fixed

fjahr commented at 11:05 pm on April 10, 2020: member

Code review ACK d42a6edd89a313ee25683226a9aa573087de1223

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

tACK d42a6edd89a313ee25683226a9aa573087de1223

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-ACK 837aba9a3680922acbf383df37485d53790b19ae

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-ACK 4c841356c2296cc011bcd678ba71ccba28129a67

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 b4c6a40ac4 outdated

1546@@ -1553,12 +1547,61 @@ isminetype DescriptorScriptPubKeyMan::IsMine(const CScript& script) const
1547 
1548 bool DescriptorScriptPubKeyMan::CheckDecryptionKey(const CKeyingMaterial& master_key, bool accept_no_keys)
1549 {
1550-    return false;
1551+    {
1552+        LOCK(cs_desc_man);
1553+        if (!m_map_keys.empty())
1554+            return false;

Sjors commented at 6:58 pm on April 22, 2020:

In b4c6a40, in case you have to touch this, please use brackets or move return false directly after if.

achow101 commented at 10:06 pm on April 22, 2020:

Done

in src/wallet/rpcdump.cpp:1618 in 4c841356c2 outdated

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"

jonatack commented at 8:31 pm on April 22, 2020:

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"

achow101 commented at 10:06 pm on April 22, 2020:

Done

in src/wallet/rpcdump.cpp:1607 in 4c841356c2 outdated

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"

jonatack commented at 8:42 pm on April 22, 2020:

nit: s/timestamp,/timestamp;/

achow101 commented at 10:06 pm on April 22, 2020:

Done

in src/wallet/rpcdump.cpp:1608 in 4c841356c2 outdated

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 8:42 pm on April 22, 2020:

s/exists/exist/

achow101 commented at 10:06 pm on April 22, 2020:

Done

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-tACK 4c84135. 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):
 4  File "/Users/sjors/dev/bitcoin-desc/test/functional/test_framework/test_framework.py", line 112, in main
 5    self.run_test()
 6  File "test/functional/wallet_balance.py", line 62, in run_test
 7    self.nodes[0].importaddress(ADDRESS_WATCHONLY)
 8  File "/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 ed0113820b outdated

26@@ -27,8 +27,10 @@ const std::string CRYPTED_KEY{"ckey"};
27 const std::string CSCRIPT{"cscript"};
28 const std::string DEFAULTKEY{"defaultkey"};
29 const std::string DESTDATA{"destdata"};
30+const std::string ACTIVEEXTERNALSPK{"activeexternalspk"};
31 const std::string FLAGS{"flags"};
32 const std::string HDCHAIN{"hdchain"};
33+const std::string ACTIVEINTERNALSPK{"activeinternalspk"};

jonatack commented at 8:34 am on April 23, 2020:

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

achow101 commented at 5:58 pm on April 23, 2020:

Moved. They used to be named differently.

in src/wallet/walletdb.cpp:193 in ed0113820b outdated

184@@ -179,6 +185,54 @@ bool WalletBatch::WriteMinVersion(int nVersion)
185     return WriteIC(DBKeys::MINVERSION, nVersion);
186 }
187 
188+bool WalletBatch::WriteActiveScriptPubKeyMan(uint8_t type, const uint256& id, bool internal)
189+{
190+    std::string key = DBKeys::ACTIVEEXTERNALSPK;
191+    if (internal) {
192+        key = DBKeys::ACTIVEINTERNALSPK;
193+    }

jonatack commented at 8:45 am on April 23, 2020:

nit suggestion

0-    std::string key = DBKeys::ACTIVEEXTERNALSPK;
1-    if (internal) {
2-        key = DBKeys::ACTIVEINTERNALSPK;
3-    }
4+    std::string key;
5+    key = internal ? DBKeys::ACTIVEINTERNALSPK : DBKeys::ACTIVEEXTERNALSPK;

like what you do below line 473

achow101 commented at 6:00 pm on April 23, 2020:

Done

in src/wallet/wallet.cpp:1351 in ed0113820b outdated

1345@@ -1332,8 +1346,8 @@ CAmount CWallet::GetChange(const CTransaction& tx) const
1346 bool CWallet::IsHDEnabled() const
1347 {
1348     bool result = true;
1349-    for (const auto& spk_man_pair : m_spk_managers) {
1350-        result &= spk_man_pair.second->IsHDEnabled();
1351+    for (const auto& spk_man : GetActiveScriptPubKeyMans()) {
1352+        result &= spk_man->IsHDEnabled();

jonatack commented at 9:30 am on April 23, 2020:

Maybe comment here that all must be true for the result to be true.

achow101 commented at 6:00 pm on April 23, 2020:

Done

in src/wallet/scriptpubkeyman.cpp:1858 in ed0113820b outdated

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 11:03 am on April 23, 2020:

nit: s/has/have/

achow101 commented at 6:00 pm on April 23, 2020:

Done

in src/wallet/scriptpubkeyman.h:510 in ed0113820b outdated

505+    bool SetCrypted();
506+
507+    //! keeps track of whether Unlock has run a thorough check before
508+    bool m_decryption_thoroughly_checked = false;
509+
510+    bool AddDescriptorKeyWithDB(WalletBatch& batch, const CKey& key, const CPubKey &pubkey);

jonatack commented at 11:21 am on April 23, 2020:

Maybe just me, but I find the “With” part of the name AddDescriptorKeyWithDB confusing.

Some of these additions could use Doxygen comments, e.g. here and SetCrypted, etc.

achow101 commented at 5:33 pm on April 23, 2020:

It follows the naming convention used elsewhere.

in src/wallet/scriptpubkeyman.cpp:1775 in ed0113820b outdated

1770+            return false;
1771+        }
1772+
1773+        m_map_crypted_keys[pubkey.GetID()] = make_pair(pubkey, crypted_secret);
1774+        batch.WriteCryptedDescriptorKey(GetID(), pubkey, crypted_secret);
1775+        return true;

jonatack commented at 11:29 am on April 23, 2020:

Why do we not return the result of batch.WriteCryptedDescriptorKey(GetID(), pubkey, crypted_secret); here?

achow101 commented at 6:00 pm on April 23, 2020:

Done

in src/wallet/walletdb.h:253 in ed0113820b outdated

245@@ -240,11 +246,19 @@ class WalletBatch
246 
247     bool WriteMinVersion(int nVersion);
248 
249+    bool WriteDescriptorKey(const uint256& desc_id, const CPubKey& pubkey, const CPrivKey& privkey);
250+    bool WriteCryptedDescriptorKey(const uint256& desc_id, const CPubKey& pubkey, const std::vector<unsigned char>& secret);
251+    bool WriteDescriptor(const uint256& desc_id, const WalletDescriptor& descriptor);
252+    bool WriteDescriptorDerivedCache(const uint256& desc_id, uint32_t key_exp_index, uint32_t der_index, const CExtPubKey& xpub);
253+    bool WriteDescriptorParentCache(const uint256& desc_id, uint32_t key_exp_index, const CExtPubKey& xpub);

jonatack commented at 12:09 pm on April 23, 2020:

What does “exp” stand for in key_exp_index, expiry/expired?

Sjors commented at 3:25 pm on April 23, 2020:

Key expression index, see descriptor.h

jonatack commented at 4:18 pm on April 23, 2020:

Thanks @Sjors

in src/wallet/walletdb.h:252 in ed0113820b outdated

245@@ -240,11 +246,19 @@ class WalletBatch
246 
247     bool WriteMinVersion(int nVersion);
248 
249+    bool WriteDescriptorKey(const uint256& desc_id, const CPubKey& pubkey, const CPrivKey& privkey);
250+    bool WriteCryptedDescriptorKey(const uint256& desc_id, const CPubKey& pubkey, const std::vector<unsigned char>& secret);
251+    bool WriteDescriptor(const uint256& desc_id, const WalletDescriptor& descriptor);
252+    bool WriteDescriptorDerivedCache(const uint256& desc_id, uint32_t key_exp_index, uint32_t der_index, const CExtPubKey& xpub);

jonatack commented at 12:12 pm on April 23, 2020:

Perhaps put uint32_t der_index last, so the first two parameters are the same as in WriteDescriptorParentCache() on the following line

achow101 commented at 6:01 pm on April 23, 2020:

I moved xpub to the front so that the only difference is that WriteDescriptorDerivedCache has der_index at the end.

in src/wallet/rpcwallet.cpp:2483 in ed0113820b outdated

2479     obj.pushKV("unconfirmed_balance", ValueFromAmount(bal.m_mine_untrusted_pending));
2480     obj.pushKV("immature_balance", ValueFromAmount(bal.m_mine_immature));
2481     obj.pushKV("txcount",       (int)pwallet->mapWallet.size());
2482-    obj.pushKV("keypoololdest", pwallet->GetOldestKeyPoolTime());
2483+    if (kp_oldest > 0) {
2484+        obj.pushKV("keypoololdest", kp_oldest);

jonatack commented at 12:52 pm on April 23, 2020:

Maybe update getwalletinfo RPCHelpMan that keypoololdest is now only displayed for non-descriptor/legacy wallets.

achow101 commented at 6:00 pm on April 23, 2020:

Done

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 ed0113820b outdated

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");

jonatack commented at 1:42 pm on April 23, 2020:

s/descriptor/descriptors/ (plural like the other error messages, if you change this be sure to update the functional test as well)

achow101 commented at 6:01 pm on April 23, 2020:

Done

in src/wallet/rpcdump.cpp:1625 in ed0113820b outdated

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)"},

jonatack commented at 1:51 pm on April 23, 2020:

s/Stating whether/Whether/ s/also known as/e.g./

achow101 commented at 6:01 pm on April 23, 2020:

Done

in src/wallet/rpcdump.cpp:1746 in ed0113820b outdated

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)));

jonatack commented at 2:30 pm on April 23, 2020:

Could avoid calling GetImportTimestamp() twice:

 0-                if (scanned_time <= GetImportTimestamp(request, now) || results.at(i).exists("error")) {
 1+                int64_t import_time = GetImportTimestamp(request, now);
 2+                if (scanned_time <= import_time || results.at(i).exists("error")) {
 3                     response.push_back(results.at(i));
 4                 } else {
 5                     UniValue result = UniValue(UniValue::VOBJ);
 6@@ -1739,7 +1740,7 @@ UniValue importdescriptors(const JSONRPCRequest& main_request) {
 7                                       "caused by pruning or data corruption (see bitcoind log for details) and could "
 8                                       "be dealt with by downloading and rescanning the relevant blocks (see -reindex "
 9                                       "and -rescan options).",
10-                                GetImportTimestamp(request, now), scanned_time - TIMESTAMP_WINDOW - 1, TIMESTAMP_WINDOW)));
11+                                import_time, scanned_time - TIMESTAMP_WINDOW - 1, TIMESTAMP_WINDOW)));

jonatack commented at 2:35 pm on April 23, 2020:

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 ed0113820b outdated

4323@@ -4314,6 +4324,7 @@ static const CRPCCommand commands[] =
4324     { "wallet",             "importprunedfunds",                &importprunedfunds,             {"rawtransaction","txoutproof"} },
4325     { "wallet",             "importpubkey",                     &importpubkey,                  {"pubkey","label","rescan"} },
4326     { "wallet",             "importwallet",                     &importwallet,                  {"filename"} },
4327+    { "wallet",             "importdescriptors",                &importdescriptors,             {"requests"} },

jonatack commented at 2:36 pm on April 23, 2020:

nit: sort

achow101 commented at 6:01 pm on April 23, 2020:

Done

in src/wallet/wallet.cpp:4481 in ed0113820b outdated

4476+                    }
4477+                    break;
4478+                }
4479+            }
4480+        }
4481+        m_spk_managers.erase(old_spk_man->GetID());

jonatack commented at 2:57 pm on April 23, 2020:

unsure but maybe worth pulling this out of the loop

 0+        auto old_spk_man_id {old_spk_man->GetID()};
 1         for (bool internal : {false, true}) {
 2             for (OutputType t : OUTPUT_TYPES) {
 3                 auto active_spk_man = GetScriptPubKeyMan(t, internal);
 4-                if (active_spk_man && active_spk_man->GetID() == old_spk_man->GetID()) {
 5+                if (active_spk_man && active_spk_man->GetID() == old_spk_man_id) {
 6                     if (internal) {
 7                         m_internal_spk_managers.erase(t);
 8                     } else {
 9@@ -4478,7 +4479,7 @@ ScriptPubKeyMan* CWallet::AddWalletDescriptor(WalletDescriptor& desc, const Flat
10                 }
11             }
12         }
13-        m_spk_managers.erase(old_spk_man->GetID());
14+        m_spk_managers.erase(old_spk_man_id);

achow101 commented at 6:01 pm on April 23, 2020:

Done

in test/functional/wallet_descriptor.py:33 in ed0113820b outdated

28+
29+        # A descriptor wallet should have 100 addresses * 3 types = 300 keys
30+        self.log.info("Checking wallet info")
31+        wallet_info = self.nodes[0].getwalletinfo()
32+        assert_equal(wallet_info['keypoolsize'], 300)
33+        assert_equal(wallet_info['keypoolsize_hd_internal'], 300)

jonatack commented at 3:33 pm on April 23, 2020:

suggest adding:

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()

achow101 commented at 6:01 pm on April 23, 2020:

Done

jonatack commented at 4:22 pm on April 23, 2020: member

Code review ACK ed0113820b498f1e904ca9a0b1205708a6f68dca

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 it 06620302c7

Introduce DescriptorScriptPubKeyMan as a dummy class 6b8119af53

Add WALLET_FLAG_DESCRIPTORS 96accc73f0

Return nullptr from GetLegacyScriptPubKeyMan if descriptor wallet aeac157c9d

Create LegacyScriptPubKeyMan when not a descriptor wallet 6b13cd3fa8

Introduce WalletDescriptor class

WalletDescriptor is a Descriptor with other wallet metadata

3194a7f88a

Add a lock cs_desc_man for DescriptorScriptPubKeyMan d8132669e1

Store WalletDescriptor in DescriptorScriptPubKeyMan 834de0300c

Implement SetType in DescriptorScriptPubKeyMan 78f8a92910

achow101 force-pushed on Apr 23, 2020

Add LoadDescriptorScriptPubKeyMan and SetActiveScriptPubKeyMan to CWallet db7177af8c

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 DescriptorScriptPubKeyMan 741122d4c1

Implement IsHDEnabled in DescriptorScriptPubKeyMan ec2f9e1178

Implement GetID for DescriptorScriptPubKeyMan 46c46aebb7

Load the descriptor cache from the wallet file 2363e9fcaa

Implement loading of keys for DescriptorScriptPubKeyMan 953feb3d27

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 file 46dfb99768

Implement SetupGeneration for DescriptorScriptPubKeyMan e014886a34

Implement TopUp in DescriptorScriptPubKeyMan 58c7651821

Implement GetNewDestination for DescriptorScriptPubKeyMan bfdd073486

Implement Unlock and Encrypt in DescriptorScriptPubKeyMan a775f7c7fd

Implement GetReservedDestination in DescriptorScriptPubKeyMan f866957979

Implement ReturnDestination in DescriptorScriptPubKeyMan 586b57a9a6

Implement GetKeypoolOldestTime and only display it if greater than 0 f1ca5feb4a

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 DescriptorScriptPubKeyMan bde7c9fa38

Implement SignMessage for descriptor wallets 84b4978c02

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 DescriptorScriptPubKeyMan b713baa75a

Be able to create new wallets with DescriptorScriptPubKeyMans as backing 82ae02b165

Generate new descriptors when encrypting 1cb42b22b1

Add IsLegacy to CWallet so that the GUI knows whether to show watchonly ce24a94494

add importdescriptors RPC and tests for native descriptor wallets

Co-authored-by: Andrew Chow <achow101-github@achow101.com>

f193ea889d

Functional tests for descriptor wallets 1346e14831

Change wallet_encryption.py to use signmessage instead of dumpprivkey 388ba94231

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-LegacySPKMans 886e0d75f5

Correctly check for default wallet cf06062859

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

utACK 223588b1bbc63dc57098bbd0baa48635e0cc0b82 (rebased, nits addressed)

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

Code review re-ACK 223588b1bbc63dc57098bbd0baa48635e0cc0b82.

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-ACK 223588b1bbc63dc57098bbd0baa48635e0cc0b82

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-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.

in src/wallet/scriptpubkeyman.cpp:1841 in 223588b1bb

1836+    }
1837+    if (!batch.WriteDescriptor(GetID(), m_wallet_descriptor)) {
1838+        throw std::runtime_error(std::string(__func__) + ": writing descriptor failed");
1839+    }
1840+
1841+    // TopUp

meshcollider commented at 12:35 pm on April 26, 2020:

Helpful comment 😅

in src/wallet/scriptpubkeyman.h:505 in 223588b1bb

500+    bool m_internal;
501+
502+    KeyMap m_map_keys GUARDED_BY(cs_desc_man);
503+    CryptedKeyMap m_map_crypted_keys GUARDED_BY(cs_desc_man);
504+
505+    bool SetCrypted();

meshcollider commented at 12:37 pm on April 26, 2020:

I don’t think this should be here should it

meshcollider commented at 11:41 pm on April 26, 2020: contributor

Code review ACK 223588b1bbc63dc57098bbd0baa48635e0cc0b82

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

DrahtBot locked this on Aug 18, 2022

Native Descriptor Wallets using DescriptorScriptPubKeyMan #16528

Conflicts