Import key origin data through descriptors in importmulti

achow101 commented at 3:23 am on August 22, 2018: member

This PR allows for key origin data as defined by the descriptors document to be imported to the wallet when importing a descriptor using importmulti. This allows the walletprocesspsbt to include the BIP 32 derivation paths for keys that it is watching that are from a different HD wallet.

In order to make this easier to use, a new field hdmasterkeyfingerprint has been added to getaddressinfo. Additionally I have removed hdmasterkeyid as was planned. I think that this API change is fine since it was going to be removed in 0.18 anyways. CKeyMetadata has also been extended to store key origin info to facilitate this.

fanquake added the label Wallet on Aug 22, 2018

fanquake added the label RPC/REST/ZMQ on Aug 22, 2018

instagibbs commented at 1:19 pm on August 22, 2018: member

Nice! strong concept ACK

achow101 force-pushed on Aug 23, 2018

achow101 force-pushed on Aug 25, 2018

achow101 commented at 3:56 pm on August 25, 2018: member

Fixed a bug where the keymetadata wasn’t being written to the wallet file. Also added a test for that case.

achow101 force-pushed on Aug 26, 2018

DrahtBot added the label Needs rebase on Aug 28, 2018

achow101 force-pushed on Aug 28, 2018

achow101 commented at 9:48 pm on August 28, 2018: member

Rebased and also changed the keymeta stuff to add the hd master key id at key generation and when the wallet is first loaded.

achow101 force-pushed on Aug 28, 2018

DrahtBot removed the label Needs rebase on Aug 28, 2018

achow101 force-pushed on Aug 29, 2018

in src/utilstrencodings.cpp:565 in 8065a31ad8 outdated

559@@ -560,6 +560,14 @@ bool ParseHDKeypath(const std::string& keypath_str, std::vector<uint32_t>& keypa
560         // Finds whether it is hardened
561         uint32_t path = 0;
562         size_t pos = item.find("'");
563+        if (pos == std::string::npos) {
564+            // Look for h

luke-jr commented at 5:48 pm on August 29, 2018:

Why?

instagibbs commented at 6:20 pm on August 29, 2018:

why not? This is quite standard.

achow101 commented at 8:34 pm on August 29, 2018:

It’s fairly standard to use h or H as the indicator, not just '. In fact, the BIP uses H, but as a subscript. ' is not actually used as the indicator for hardened in the BIP even though that is what everyone uses.

The main reason for allowing this is so that you don’t have to do any annoying escaping stuff for the ' when entering the keypaths on the command line.

achow101 force-pushed on Aug 29, 2018

DrahtBot added the label Needs rebase on Aug 30, 2018

achow101 commented at 1:08 pm on September 4, 2018: member

Rebased

achow101 force-pushed on Sep 4, 2018

DrahtBot removed the label Needs rebase on Sep 4, 2018

laanwj commented at 2:54 pm on September 13, 2018: member

Concept ACK, thanks for working on this

achow101 commented at 9:07 pm on October 15, 2018: member

I will rework this to not require #14019 as soon as I have time (in a few days)

achow101 commented at 4:27 am on October 30, 2018: member

Closing for now due to the big changes to importmulti from #14454 and #14565. Perhaps it would be better to put more effort into supporting this through output descriptors rather than adding more data to importmulti.

achow101 closed this on Oct 30, 2018

Sjors commented at 6:40 pm on November 5, 2018: member

+1 for only supporting this with descriptors. Suggest rebasing on #14491, as that currently doesn’t save origin info into the wallet.

achow101 reopened this on Nov 6, 2018

achow101 force-pushed on Nov 6, 2018

achow101 commented at 5:51 am on November 6, 2018: member

Rebased onto #14491

achow101 renamed this:
~~Import key origin data through importmulti~~
Import key origin data through descriptors in importmulti
on Nov 6, 2018

DrahtBot added the label Needs rebase on Nov 6, 2018

achow101 force-pushed on Nov 6, 2018

DrahtBot removed the label Needs rebase on Nov 6, 2018

Sjors commented at 8:45 am on November 6, 2018: member

@achow101 can you do a git commit -a --amend --date "$(date)" on your own commits so that they appear after the unmerged upstream stuff? It’s a bit hard to tell now which commits to review.

Sjors commented at 9:51 am on November 6, 2018: member

Seeing some warnings (might be upstream):

Key origin info from importmulti now nicely shows up now in getaddressinfo. However, like the upstream PR, it generate invalid psbt’s. I tried with a watch-only wallet, manually providing a change address (which isn’t used):

 0src/bitcoin-cli -rpcwallet=... walletcreatefundedpsbt '[{"txid": "...", "vout": 0}]' '[{"...": 0.1}]' 0 '{"includeWatching":  true, "subtractFeeFromOutputs": [0], "changeAddress": "..."}' true
 1
 2{
 3  "psbt": "...",
 4  "fee": 0.00002200,
 5  "changepos": -1
 6}
 7
 8src/bitcoin-cli -rpcwallet=... decodepsbt "..."
 9error code: -22
10error message:
11TX decode failed Size of key was not the expected size for the type BIP32 keypath: unspecified iostream_category error

Sjors commented at 10:04 am on November 6, 2018: member

If we narrow this PR to WALLET_FLAG_DISABLE_PRIVATE_KEYS wallets, would that allow you to just reuse hdseedid rather than undeprecating and repurposing hdmasterkeyid ?

And/or if we set hdseedid during importmulti, would that save you from having to upgrade the wallet version? I.e. get rid of b0c7bbd4c460fdeb5a91d34f75e96eb115e37598?

achow101 commented at 2:21 pm on November 6, 2018: member

If we narrow this PR to WALLET_FLAG_DISABLE_PRIVATE_KEYS wallets, would that allow you to just reuse hdseedid rather than undeprecating and repurposing hdmasterkeyid ?

Yes, but I don’t really like that. It would mean that hdseedid has two meanings, depending on whether private keys are disabled. It’s definition would not be consistent.

achow101 force-pushed on Nov 6, 2018

achow101 commented at 2:41 pm on November 6, 2018: member

@Sjors I’ve updated the dates on my commits and fixed the lock issue I believe. I am unable to replicate the creation of invalid PSBTs though. The PSBTs I have created for testing are correct.

achow101 force-pushed on Nov 6, 2018

Sjors commented at 3:47 pm on November 6, 2018: member

Isn’t the only difference between hdseedid and hdmasterkeyid that the key with hdseedid MAY be present in the wallet (for !WALLET_FLAG_DISABLE_PRIVATE_KEYS)? It MAY also be absent for keys generated before encryptwallet (which replaces the wallet seed). They both refer to a master key that generated a set of keys in the wallet.

If we migrate to a descriptor based wallet, there would be no difference in treatment between keys with hdseedid and keys hdmasterkeyid . Or am I missing something? cc @sipa

achow101 commented at 4:01 pm on November 6, 2018: member

hdseedid is the ID of the seed. To get the master key id, the seed needs to be used to generate the master private key, and then the masterkeyid generated from that. There’s one extra step of abstraction. The hd seed is not the same as the master private key.

Sjors commented at 10:28 am on November 7, 2018: member

@achow101 ah, I didn’t know about the extra step from seed to master key. That makes sense then.

DrahtBot commented at 6:09 am on November 9, 2018: 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:

#14075 (Import watch only pubkeys to the keypool if private keys are disabled by achow101)
#13756 (wallet: “avoid_reuse” wallet flag for improved privacy by kallewoof)
#13751 (Utils and libraries: Drops the boost/algorithm/string/split.hpp dependency by 251Labs)
#13541 (wallet/rpc: sendrawtransaction maxfeerate by kallewoof)
#11803 (Bugfix: RPC/Wallet: Include HD key metadata in dumpwallet by luke-jr)
#10973 (Refactor: separate wallet from node by ryanofsky)
#10102 ([experimental] Multiprocess bitcoin by ryanofsky)

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.

in src/util/strencodings.h:204 in 0999596382 outdated

201@@ -202,6 +202,10 @@ bool ConvertBits(const O& outfn, I it, I end) {
202 /** Parse an HD keypaths like "m/7/0'/2000". */
203 bool ParseHDKeypath(const std::string& keypath_str, std::vector<uint32_t>& keypath);
204 
205+/** Write HD keypaths as strings */
206+std::string WriteHdKeypath(const std::vector<uint32_t> keypath);

practicalswift commented at 8:34 am on November 9, 2018:

Should be const reference?

in src/util/strencodings.cpp:600 in 0999596382 outdated

595+        if (i >> 31) ret += '\'';
596+    }
597+    return ret;
598+}
599+
600+std::string WriteHdKeypath(const std::vector<uint32_t> keypath)

practicalswift commented at 8:35 am on November 9, 2018:

Should be const reference?

meshcollider commented at 7:11 am on November 18, 2018:

IMO this function name is misleading, it is not “writing” anywhere, it is a formatting function

ajtowns commented at 1:39 am on February 12, 2019:

Naming it FormatHDKeypath and const& keypath would seem more consistent with ParseHDKeypath

fanquake deleted a comment on Nov 9, 2018

MarcoFalke deleted a comment on Nov 12, 2018

DrahtBot added the label Needs rebase on Nov 13, 2018

in src/wallet/wallet.cpp:4386 in 1e60b5eb28 outdated

4389+    LOCK(cs_wallet);
4390+    std::vector<unsigned char> fingerprint_data(20, 0);
4391+    fingerprint_data[0] = info.fingerprint[0];
4392+    fingerprint_data[1] = info.fingerprint[1];
4393+    fingerprint_data[2] = info.fingerprint[2];
4394+    fingerprint_data[3] = info.fingerprint[3];

meshcollider commented at 7:29 am on November 18, 2018:

why not std::copy()?

in src/wallet/rpcwallet.cpp:3690 in f1c55f7b3c outdated

3535@@ -3536,7 +3536,9 @@ UniValue getaddressinfo(const JSONRPCRequest& request)
3536         if (!meta->hdKeypath.empty()) {
3537             ret.pushKV("hdkeypath", meta->hdKeypath);
3538             ret.pushKV("hdseedid", meta->hd_seed_id.GetHex());
3539-            ret.pushKV("hdmasterkeyid", meta->hd_seed_id.GetHex());
3540+            if (!meta->master_key_id.IsNull()) {
3541+                ret.pushKV("hdmasterkeyid", meta->master_key_id.GetHex());

meshcollider commented at 9:04 am on November 18, 2018:

master_key_id will often be only filled with the fingerprint rather than the entire ID, perhaps make a note in the help text above or only output the first 4 bytes in that case rather than a lot of zeroes too?

An alternative is to just always store the fingerprint in metadata, never the whole key, because I don’t think it’s used anywhere else.

achow101 commented at 7:20 pm on December 24, 2018:

I’ve changed this to use the fingerprint by adding KeyOriginInfo to CKeyMetadata.

in test/functional/wallet_importmulti.py:632 in 0999596382 outdated

638@@ -632,5 +639,52 @@ def run_test (self):
639         result = self.nodes[1].getaddressinfo(address["address"])
640         assert_equal(result['ismine'], True)
641 
642+        # Import pubkeys with key origin info
643+        self.log.info("Addresses should have hd keypath and master key id after import with key origin")
644+        pub_addr = self.nodes[1].getnewaddress()
645+        pub_addr = self.nodes[1].getnewaddress()

meshcollider commented at 9:11 am on November 18, 2018:

Why call twice? Is it just to have a non-zero keypath index? Perhaps add comment if deliberate

achow101 commented at 7:21 pm on December 24, 2018:

Added a comment.

meshcollider commented at 9:17 am on November 18, 2018: contributor

utACK https://github.com/bitcoin/bitcoin/pull/14021/commits/09995963827b7be1163a50d894490daf182b7a78

A few comments inline

Sjors commented at 5:19 pm on November 20, 2018: member

Without meaning to expand the number of PR’s we need to keep track of, but if this is merged then the dumpwallet improvement in #11803 should be updated to look for master_key_id. Currently it won’t print master_key_id and skips origin info.

achow101 force-pushed on Dec 16, 2018

achow101 commented at 7:13 am on December 16, 2018: member

Rebased

DrahtBot removed the label Needs rebase on Dec 16, 2018

achow101 force-pushed on Dec 16, 2018

DrahtBot added the label Needs rebase on Dec 24, 2018

achow101 force-pushed on Dec 24, 2018

achow101 commented at 3:27 pm on December 24, 2018: member

Rebased

achow101 force-pushed on Dec 24, 2018

DrahtBot removed the label Needs rebase on Dec 28, 2018

achow101 force-pushed on Jan 23, 2019

DrahtBot added the label Needs rebase on Feb 1, 2019

achow101 force-pushed on Feb 5, 2019

DrahtBot removed the label Needs rebase on Feb 5, 2019

achow101 force-pushed on Feb 8, 2019

meshcollider added this to the milestone 0.18.0 on Feb 8, 2019

in src/wallet/walletdb.h:191 in b1b35b88a2 outdated

176@@ -177,6 +177,7 @@ class WalletBatch
177     bool WriteTx(const CWalletTx& wtx);
178     bool EraseTx(uint256 hash);
179 
180+    bool WriteKeyMetadata(const CKeyMetadata& meta, const CPubKey& pubkey, const bool overwrite);

Sjors commented at 12:19 pm on February 9, 2019:

Nit: add documentation

achow101 commented at 2:17 am on February 11, 2019:

Added a comment

in src/wallet/walletdb.h:101 in 066762f021 outdated

 98+    static const int VERSION_WITH_KEY_ORIGIN = 12;
 99+    static const int CURRENT_VERSION=VERSION_WITH_KEY_ORIGIN;
100     int nVersion;
101     int64_t nCreateTime; // 0 means unknown
102-    std::string hdKeypath; //optional HD/bip32 keypath
103+    std::string hdKeypath; //optional HD/bip32 keypath. Still used to determine whether a key is a seed. ALso kept for backwards compatibility

Sjors commented at 12:25 pm on February 9, 2019:

Nit: ALso

See above comment, I prefer adding a bool to explicitly track if a key is a seed.

achow101 commented at 2:17 am on February 11, 2019:

Fixed typo

in src/wallet/rpcwallet.cpp:3717 in 066762f021 outdated

3714@@ -3715,10 +3715,10 @@ UniValue getaddressinfo(const JSONRPCRequest& request)
3715     }
3716     if (meta) {
3717         ret.pushKV("timestamp", meta->nCreateTime);
3718-        if (!meta->hdKeypath.empty()) {
3719-            ret.pushKV("hdkeypath", meta->hdKeypath);
3720+        if (!meta->key_origin.IsNull()) {

Sjors commented at 12:29 pm on February 9, 2019:

Note for other reviewers: we process hdKeyPath and add key_origin equivalents in CWallet::UpgradeKeyMetadata(). So there’s no case where key_origin.IsNull() without also meta->hdKeypath.empty()

in src/wallet/rpcwallet.cpp:3728 in 066762f021 outdated

3719-            ret.pushKV("hdkeypath", meta->hdKeypath);
3720+        if (!meta->key_origin.IsNull()) {
3721+            ret.pushKV("hdkeypath", WriteHdKeypath(meta->key_origin.path));
3722             ret.pushKV("hdseedid", meta->hd_seed_id.GetHex());
3723-            ret.pushKV("hdmasterkeyid", meta->hd_seed_id.GetHex());
3724+            ret.pushKV("hdmasterfingerprint", HexStr(meta->key_origin.fingerprint, meta->key_origin.fingerprint + 4));

Sjors commented at 12:29 pm on February 9, 2019:

Nit: key_origin should have a ToString() method (you’re adding something like this in a later commit, though that’s for the whole origin)

achow101 commented at 2:03 am on February 11, 2019:

A key_origin ToString() method would print both the fingerprint and path. This only needs the fingerprint.

in src/script/sign.h:43 in 066762f021 outdated

38+        READWRITE(path);
39+    }
40+
41+    void SetNull()
42+    {
43+        memset(fingerprint, 0, 4);

Sjors commented at 12:49 pm on February 9, 2019:

Nit: why not make fingerprint a ~ std::vector<unsigned char> and use clear() on it too?

achow101 commented at 2:04 am on February 11, 2019:

Arrays use less space. This field doesn’t need to be dynamic.

instagibbs commented at 3:02 am on February 11, 2019:

I don’t think the space savings is worth dealing with IsNull verbosity and the magic number(4).

achow101 commented at 4:15 am on February 11, 2019:

This was something that was already here and I don’t think it is necessary to change it.

ajtowns commented at 4:43 am on February 13, 2019:

You could use sizeof(fingerprint) to get rid of the magic number 4? But seems fine as is to me.

in src/wallet/wallet.cpp:346 in 066762f021 outdated

341+{
342+    AssertLockHeld(cs_wallet); // mapKeyMetadata
343+
344+    for (auto& meta_pair : mapKeyMetadata) {
345+        CKeyMetadata& meta = meta_pair.second;
346+        if (!meta.hd_seed_id.IsNull() && meta.key_origin.IsNull() && meta.hdKeypath != "s") {

Sjors commented at 1:19 pm on February 9, 2019:

Nit: add comment to explain what hdKeypath != "s" means.

achow101 commented at 2:17 am on February 11, 2019:

Done

in src/script/sign.h:32 in 066762f021 outdated

28@@ -29,6 +29,25 @@ struct KeyOriginInfo
29     {
30         return std::equal(std::begin(a.fingerprint), std::end(a.fingerprint), std::begin(b.fingerprint)) && a.path == b.path;
31     }
32+

Sjors commented at 1:26 pm on February 9, 2019:

Nit: suggest adding a comment to unsigned char fingerprint[4]; above:

0// BIP32 defines the key identifier as the RIPEMD160 after SHA256 of
1// the serialized ECDSA public key K, ignoring the chain code. The first 32 bits
2// of this is the key fingerprint.

achow101 commented at 2:17 am on February 11, 2019:

Added a comment

in src/wallet/wallet.cpp:385 in 066762f021 outdated

350+            masterKey.SetSeed(key.begin(), key.size());
351+            // Add to map
352+            CKeyID master_id = masterKey.key.GetPubKey().GetID();
353+            std::copy(master_id.begin(), master_id.begin() + 4, meta.key_origin.fingerprint);
354+            if (!ParseHDKeypath(meta.hdKeypath, meta.key_origin.path)) {
355+                throw std::runtime_error("Invalid stored hdKeypath");

Sjors commented at 1:36 pm on February 9, 2019:

Nit: add This wallet can't be safely upgraded and can't be opened by this version of Bitcoin Core.

Is this entire upgrade atomic, or is it atomic per WriteKeyMetadata? Otherwise you might end up with some upgraded keys and then an error. Then the previous version of the wallet can’t open it anymore (if only for the user to call dumpwallet). In that case it’s safer to call ParseHDKeypath on all keys before saving any.

achow101 commented at 2:09 am on February 11, 2019:

It is atomic per WriteKeyMetadata. However I believe that the changes to CKeyMetadata are backwards compatible. My understanding of the serialization code is that data at the end of the stream is ignored. So upgraded CKeyMetadata should still be open-able in older software.

Sjors commented at 8:29 am on February 11, 2019:

Yes, I also noticed I was able to open (non-blank) 0.18 wallets in 0.17.

sipa commented at 10:15 pm on February 14, 2019:

That’s correct. If you don’t deserialize things at the end of a stream, they just get ignored.

in src/wallet/wallet.cpp:1463 in 066762f021 outdated

1417@@ -1380,6 +1418,7 @@ CPubKey CWallet::DeriveNewSeed(const CKey& key)
1418 
1419     // set the hd keypath to "s" -> Seed, refers the seed to itself
1420     metadata.hdKeypath     = "s";

Sjors commented at 1:44 pm on February 9, 2019:

nit-ish: let’s just add a boolean IsSeed to metadata, so we don’t have to keep hdKeypath around forever (can be done later, but would require another upgrade function).

achow101 commented at 2:10 am on February 11, 2019:

We need to keep hdKeypath forever as CKeyMetadata is backwards compatible. Might as well use it.

ajtowns commented at 2:09 am on February 12, 2019:

If hdKeypath is being kept forever, does it actually make sense to have the key_origin added to CKeyMetadata? You can calculate key_origin from hdKeypath using ParseHDKeypath() if needed, but everywhere that uses key_origin is converting it back to a string via WriteHdKeypath already as far as I can see? AddKeyOrigin is already filling out hdKeyPath so it would still achieve the point of the PR by the looks?

achow101 commented at 5:10 pm on February 12, 2019:

CWallet::GetKeyOrigin uses the key_origin in CKeyMetadata. I strongly prefer storing key origin info in this form instead of having to parse strings every time key origin info is needed to build a PSBT.

in src/wallet/rpcdump.cpp:1269 in 7e3eaebd0c outdated

1260@@ -1260,6 +1261,8 @@ static UniValue ProcessImport(CWallet * const pwallet, const UniValue& data, con
1261             if (!pwallet->GetPubKey(id, temp) && !pwallet->AddWatchOnly(GetScriptForRawPubKey(pubkey), timestamp)) {
1262                 throw JSONRPCError(RPC_WALLET_ERROR, "Error adding address to wallet");
1263             }
1264+            pwallet->AddKeyOrigin(pubkey, import_data.key_origins[id]);
1265+            pwallet->mapKeyMetadata[id].nCreateTime = timestamp;

Sjors commented at 1:53 pm on February 9, 2019:

Didn’t the imported keys already have a metadata entry? If so, then shouldn’t you only replace the timestamp if absent?

achow101 commented at 2:12 am on February 11, 2019:

Only new things can be imported, so there shouldn’t already be an existing metadata entry.

Sjors approved

Sjors commented at 2:12 pm on February 9, 2019: member

tACK 39b9de4, modulo depth-4 issue. I don’t fully understand the READWRITE stuff in 066762f02178b2f39581f903841e75694853d8b7, so someone else should double check that.

Why are you hardcoding a maximum derivation depth of 4? The BIP32 spec implicitly sets the maximum depth to 256, by having one byte for it. BIP44/49/84 uses 5 levels, though we of course can assume the coin_type=={0,1} and get away with 4 levels. But future fancy things like Taproot / MAST might benefit from more levels (e.g. using unique subkeys for various redeem conditions, or a derivation path matching the branch in a Merkle tree).

If it’s easy to change the max depth later I don’t mind too much.

A bunch of nits inline, maybe better for a followup.

it would be nice to also expand the test for dumpwallet.
a future backwards compatibility test (#12134) could make sure old versions don’t mess with key_origin metadata (I tested manually). Conversely it could test the upgrade functionality (though you can already do that by adding a wallet binary to the test suite).
for followup, or #15226 if that’s merged after this: importwallet ignores the origin info that’s now added to dumpwallet.

Unrelated: it’s weird that, according to dumpwallet, keys are in random order.

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

achow101 commented at 1:58 am on February 11, 2019: member

Why are you hardcoding a maximum derivation depth of 4?

I’m not. Where do you see that it does?

achow101 force-pushed on Feb 11, 2019

in src/wallet/wallet.h:859 in 82396ec29d outdated

854@@ -855,6 +855,7 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface
855     //! Load metadata (used by LoadWallet)
856     void LoadKeyMetadata(const CKeyID& keyID, const CKeyMetadata &metadata) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
857     void LoadScriptMetadata(const CScriptID& script_id, const CKeyMetadata &metadata) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
858+    void UpgradeKeyMetadata();

instagibbs commented at 3:12 am on February 11, 2019:

This needs a description of what it’s doing.

achow101 commented at 4:24 am on February 11, 2019:

Done

in src/wallet/wallet.cpp:378 in 82396ec29d outdated

344+
345+    for (auto& meta_pair : mapKeyMetadata) {
346+        CKeyMetadata& meta = meta_pair.second;
347+        if (!meta.hd_seed_id.IsNull() && meta.key_origin.IsNull() && meta.hdKeypath != "s") { // If the hdKeypath is "s", that's the seed and it doesn't have a key origin
348+            CKey key;
349+            GetKey(meta.hd_seed_id, key);

instagibbs commented at 3:15 am on February 11, 2019:

Since this is called on load, does the user need to unlock an encrypted wallet?

achow101 commented at 4:19 am on February 11, 2019:

Hmm. I think it will. There is currently no way to get the master key fingerprint without generating it from the hd seed.

instagibbs commented at 3:49 pm on February 11, 2019:

I’d like to know what’s going to happen here, but we should at least be checking the result here to not calculate bogus data, yes?

achow101 commented at 6:09 pm on February 11, 2019:

I think it will write bogus data.

There are a few options to solve this. We could not upgrade the key metadata if we can’t calculate the master key id. Or we could use the hd seed id instead if the correct master key id can’t be calculated.

I am tending to go towards the second solution. While it technically is not the master key id, this information is really only used for/needed for descriptors and PSBT. Any wallet that already exists does not need this information. If this ends up in PSBT, that’s fine since the BIP 32 derivation paths are just information for the signer. In this case, the signer is Bitcoin Core, and it doesn’t use the derivation paths. We just need to put something there that won’t cause problems for other signers.

sipa commented at 10:16 pm on February 14, 2019:

It seems there is an IsLocked() at the top now, so you’ve picked the first solution (not upgrading when locked)? If so, I prefer that.

in src/wallet/walletdb.h:97 in 82396ec29d outdated

93@@ -93,11 +94,13 @@ class CKeyMetadata
94 public:
95     static const int VERSION_BASIC=1;
96     static const int VERSION_WITH_HDDATA=10;
97-    static const int CURRENT_VERSION=VERSION_WITH_HDDATA;
98+    static const int VERSION_WITH_KEY_ORIGIN = 12;

instagibbs commented at 3:20 am on February 11, 2019:

why no 11 :(

achow101 commented at 4:20 am on February 11, 2019:

A previous version of this pr used version 11 and I did not want to cause wallets that had tested that pr to become unusable.

Sjors commented at 8:31 am on February 11, 2019:

From my experience older hww branch wallets are already unusable so I wouldn’t worry too much about that :-)

achow101 force-pushed on Feb 11, 2019

Sjors commented at 8:39 am on February 11, 2019: member

@achow101 oops, I was confusing the fingerprint (fixed size 4) with path (already a vector). All good then!

utACK 7f84a25, changes since last tACK:

more comments
using insert:

in src/wallet/rpcwallet.cpp:3663 in 0cf1c4ca12 outdated

3651@@ -3652,7 +3652,7 @@ UniValue getaddressinfo(const JSONRPCRequest& request)
3652             "  \"timestamp\" : timestamp,      (number, optional) The creation time of the key if available in seconds since epoch (Jan 1 1970 GMT)\n"
3653             "  \"hdkeypath\" : \"keypath\"       (string, optional) The HD keypath if the key is HD and available\n"
3654             "  \"hdseedid\" : \"<hash160>\"      (string, optional) The Hash160 of the HD seed\n"
3655-            "  \"hdmasterkeyid\" : \"<hash160>\" (string, optional) alias for hdseedid maintained for backwards compatibility. Will be removed in V0.18.\n"

ajtowns commented at 0:09 am on February 12, 2019:

This PR removes some but not all of the hdmasterkeyid fields, suggest having a separate commit that removes all of them at once.

achow101 commented at 5:04 pm on February 12, 2019:

Done

in src/script/sign.h:49 in 0cf1c4ca12 outdated

45+        path.clear();
46+    }
47+
48+    bool IsNull() const
49+    {
50+        return path.empty() && fingerprint[0] == 0 && fingerprint[1] == 0 && fingerprint[2] == 0 && fingerprint[3] == 0;

ajtowns commented at 0:52 am on February 12, 2019:

This is a bit confusing. fingerprint has a 1 in 4 billion chance of legitimately being [0,0,0,0] from what I can see, and there’s a code path that sets fingerprint but clears path (script/descriptor.cpp:ConstPubkeyProvider::GetPubKey). So I think this function has a small chance of returning inconsistent results.

laanwj commented at 9:08 am on February 12, 2019:

:+1: Don’t special-case values that can actually occur

Sjors commented at 10:17 am on February 12, 2019:

Can we mandate that KeyOriginInfo must always contain a fingerprint? So in that case justing checking path.empty() is enough. Otherwise we need another boolean that’s also serialised.

achow101 commented at 3:34 pm on February 12, 2019:

How else then would we determine that a KeyOriginInfo is null?

Can we mandate that KeyOriginInfo must always contain a fingerprint? So in that case justing checking path.empty() is enough.

I don’t think so. It is perfectly valid to have a fingerprint and no path. This indicates that the path is just m.

Sjors commented at 5:55 pm on February 12, 2019:

So then it seems we need an explicit “null” for the fingerprint to indicate it’s not set.

achow101 commented at 0:04 am on February 13, 2019:

Adding a null field would cause a lot of code churn. Instead I’ve changed CKeyMetadata to have a boolean that indicates whether the key_origin is valid. Let me know what you think of that; if it’s good, I’ll squash the commit down.

ajtowns commented at 4:47 am on February 13, 2019:

Looks better to me

in src/wallet/wallet.h:859 in 0cf1c4ca12 outdated

854@@ -855,6 +855,8 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface
855     //! Load metadata (used by LoadWallet)
856     void LoadKeyMetadata(const CKeyID& keyID, const CKeyMetadata &metadata) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
857     void LoadScriptMetadata(const CScriptID& script_id, const CKeyMetadata &metadata) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
858+    //! Upgrade stored CKeyMetadata objects to store key origin info as KeyOriginInfo
859+    void UpgradeKeyMetadata();

ajtowns commented at 0:56 am on February 12, 2019:

Add EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) annotation?

achow101 commented at 5:04 pm on February 12, 2019:

Done

in src/wallet/walletdb.cpp:538 in 0cf1c4ca12 outdated

533@@ -534,6 +534,9 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
534     if (wss.fAnyUnordered)
535         result = pwallet->ReorderTransactions();
536 
537+    // Upgrade all of the wallet keymetadata to have the hd master key id
538+    pwallet->UpgradeKeyMetadata();

ajtowns commented at 1:06 am on February 12, 2019:

This throws std::runtime_error on an invalid hdKeypath, should that be caught here and translated into DBErrors::CORRUPT ? Also, should this be done before any writing so that if the wallet can’t be loaded, it isn’t changed first making it harder to debug?

Sjors commented at 10:21 am on February 12, 2019:

@ajtowns the new metadata is still readable by older wallets: #14021 (review)

That said, I also have a preference for atomically updating as a general pattern.

achow101 commented at 5:05 pm on February 12, 2019:

I’ve added a catch for the std::runtime_error

in src/script/sign.h:25 in ce437d6866 outdated

21@@ -22,13 +22,32 @@ struct CMutableTransaction;
22 
23 struct KeyOriginInfo
24 {
25-    unsigned char fingerprint[4];
26+    unsigned char fingerprint[4]; // First 32 bits of the Hash160 of the public key at the root of the path

laanwj commented at 9:06 am on February 12, 2019:

//!< please so that it shows up in doxygen

achow101 commented at 5:05 pm on February 12, 2019:

Done

in src/util/strencodings.cpp:590 in ce437d6866 outdated

586@@ -587,6 +587,21 @@ bool ParseHDKeypath(const std::string& keypath_str, std::vector<uint32_t>& keypa
587     return true;
588 }
589 
590+std::string FormatKeyPath(const std::vector<uint32_t>& path)

laanwj commented at 9:10 am on February 12, 2019:

It was never the intent for strencodings.cpp to gain so many bitcoin-application-specific utilities, and this application is very narrow. Maybe make a new header with bip32/key specific utils?

achow101 commented at 5:05 pm on February 12, 2019:

Moved to bip32.{h,cpp}

laanwj commented at 9:12 am on February 12, 2019: member

In order to make this easier to use, the hdmasterkeyid field in getaddressinfo has been changed to actually give the HD master key id instead of the seed id

This is no longer true with the code change, please update the post (as it will be included in git).

achow101 force-pushed on Feb 12, 2019

achow101 commented at 5:06 pm on February 12, 2019: member

Addressed comments and updated OP

in src/wallet/walletdb.cpp:539 in fa1014c2a1 outdated

533@@ -529,6 +534,13 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
534     if (wss.fAnyUnordered)
535         result = pwallet->ReorderTransactions();
536 
537+    // Upgrade all of the wallet keymetadata to have the hd master key id
538+    try {

Sjors commented at 6:04 pm on February 12, 2019:

Nit: add comment that this is not atomic, but that individually updated keymetadata entries don’t prevent older wallets from loading.

achow101 commented at 3:35 pm on February 13, 2019:

Done

Sjors commented at 6:11 pm on February 12, 2019: member

utACK fa1014c modulo the fingerprint == 0 edge case

Sjors commented at 9:01 am on February 13, 2019: member

@achow101 wrote inline:

Adding a null field would cause a lot of code churn. Instead I’ve changed CKeyMetadata to have a boolean that indicates whether the key_origin is valid. Let me know what you think of that; if it’s good, I’ll squash the commit down. @ajtowns wrote: Looks better to me

Agreed, utACK 4908b37 (modulo squash)

achow101 force-pushed on Feb 13, 2019

achow101 commented at 3:35 pm on February 13, 2019: member

Squashed

in src/wallet/rpcwallet.cpp:2421 in 1d23f6bf0c outdated

2405@@ -2406,7 +2406,6 @@ static UniValue getwalletinfo(const JSONRPCRequest& request)
2406             "  \"unlocked_until\": ttt,             (numeric) the timestamp in seconds since epoch (midnight Jan 1 1970 GMT) that the wallet is unlocked for transfers, or 0 if the wallet is locked\n"
2407             "  \"paytxfee\": x.xxxx,                (numeric) the transaction fee configuration, set in " + CURRENCY_UNIT + "/kB\n"
2408             "  \"hdseedid\": \"<hash160>\"            (string, optional) the Hash160 of the HD seed (only present when HD is enabled)\n"
2409-            "  \"hdmasterkeyid\": \"<hash160>\"       (string, optional) alias for hdseedid retained for backwards-compatibility. Will be removed in V0.18.\n"

instagibbs commented at 3:42 pm on February 13, 2019:

all instances(aside from notes) indeed appear to be deleted

instagibbs commented at 4:05 pm on February 13, 2019: member

Explicit null key_origin is welcomed change.

I don’t think you have addressed the encrypted wallet migration issue with your preferred fix?

achow101 force-pushed on Feb 13, 2019

achow101 commented at 6:30 pm on February 13, 2019: member

I don’t think you have addressed the encrypted wallet migration issue with your preferred fix?

Hmm. I thought I pushed that. It must’ve gotten lost in a rebase somewhere. Pushed it now.

instagibbs commented at 6:35 pm on February 13, 2019: member

utACK https://github.com/bitcoin/bitcoin/pull/14021/commits/4486b36762a7ab395d1f01b941d83243af6450d4

Sjors commented at 7:35 pm on February 13, 2019: member

@instagibbs wrote inline:

Since this is called on load, does the user need to unlock an encrypted wallet? @achow101 wrote: There are a few options to solve this. We could not upgrade the key metadata if we can’t calculate the master key id. Or we could use the hd seed id instead if the correct master key id can’t be calculated.

I am tending to go towards the second solution. While it technically is not the master key id, this information is really only used for/needed for descriptors and PSBT. Any wallet that already exists does not need this information. If this ends up in PSBT, that’s fine since the BIP 32 derivation paths are just information for the signer. In this case, the signer is Bitcoin Core, and it doesn’t use the derivation paths. We just need to put something there that won’t cause problems for other signers.

I don’t like this approach; more assumptions than necessary. It seems safer to also run this from CWallet::Unlock.

The rest of 4486b36 looks good to me.

instagibbs commented at 7:40 pm on February 13, 2019: member

@Sjors would it make the first unlock surprisingly long?

Sjors commented at 7:53 pm on February 13, 2019: member

I’m assuming updating metadata entries is extremely fast, but haven’t tested that. You could potentially spit out a warning on first unlock (although that might stress out applications that consume std::out).

achow101 commented at 8:08 pm on February 13, 2019: member

I don’t like this approach; more assumptions than necessary. It seems safer to also run this from CWallet::Unlock.

There’s a bit of a problem with doing that. If the wallet is not unlocked but a user decides to use walletprocesspsbt with bip32derivs set to true, then the resulting psbt will have a bogus master key fingerprint. It’s writing uninitialized memory to the psbt. This is both a problem in this PR (without the change I just pushed) and in master.

I’m assuming updating metadata entries is extremely fast

it takes several seconds.

achow101 force-pushed on Feb 13, 2019

achow101 commented at 9:16 pm on February 13, 2019: member

I’ve reverted that change. In the new push, the key metadata will be upgraded on unlocking the wallet or on loading if the wallet is unencrypted. However, due to the issue I mentioned earlier, I’ve changed GetKeyOrigin to return the KeyOriginInfo for keys whose metadata has not been upgraded to just be as if it were an independent key.

I also added release notes which mention this change.

Sjors commented at 9:47 am on February 14, 2019: member

tACK b384060ff modulo squash.

AppVeyor failure is same as on master.

achow101 commented at 3:21 pm on February 14, 2019: member

Squashed

achow101 force-pushed on Feb 14, 2019

Sjors commented at 3:47 pm on February 14, 2019: member

utACK a8a7103

in src/wallet/wallet.cpp:502 in d24c52d0e3 outdated

464@@ -422,6 +465,8 @@ bool CWallet::Unlock(const SecureString& strWalletPassphrase, bool accept_no_key
465             if (!crypter.Decrypt(pMasterKey.second.vchCryptedKey, _vMasterKey))
466                 continue; // try another master key
467             if (CCryptoKeyStore::Unlock(_vMasterKey, accept_no_keys))
468+                // Now that we've unlocked, upgrade the key metadata
469+                UpgradeKeyMetadata();

instagibbs commented at 5:55 pm on February 14, 2019:

This means wallets will iterate through all key metadata every unlock?

Sjors commented at 6:07 pm on February 14, 2019:

Perhaps a more efficient approach is to set a wallet flag once all metadata is upgraded?

instagibbs commented at 6:37 pm on February 14, 2019:

This is a point in favor of atomicity, then marking as complete.

achow101 commented at 6:59 pm on February 14, 2019:

I’ve added a non-mandatory wallet flag which is set upon the upgrade’s completion. This will make sure that the upgrade happens only once.

achow101 force-pushed on Feb 14, 2019

Refactor keymetadata writing to a separate method c45415f73a

Add WriteHDKeypath function and move *HDKeypath to util/bip32.{h,cpp}

Creates new files util/bip32.h and util/bip32.cpp for containing
BIP 32 stuff.
Moves FormatKeyPath from descriptor.cpp to util/bip32.
Adds a wrapper around it to prepent the 'm' for when just the
BIP 32 style keypath is needed.

e7652d3f64

Add a method to CWallet to write just CKeyMetadata bac8c676a7

Remove hdmasterkeyid 345bff6013

achow101 force-pushed on Feb 14, 2019

instagibbs commented at 7:05 pm on February 14, 2019: member

utACK https://github.com/bitcoin/bitcoin/pull/14021/commits/8fff73cb092e609505f6a0a14c2fe4d8d80479dd

in src/wallet/wallet.h:135 in 8fff73cb09 outdated

134@@ -135,6 +135,9 @@ enum WalletFlags : uint64_t {
135     // wallet flags in the upper section (> 1 << 31) will lead to not opening the wallet if flag is unknown

Sjors commented at 7:39 pm on February 14, 2019:

Should this say upper section (> 0 << 31) ?

achow101 commented at 7:45 pm on February 14, 2019:

Not my problem :p

Also, no, it shouldn’t. Shifting 0 is still 0.

in src/wallet/wallet.h:139 in 8fff73cb09 outdated

134@@ -135,6 +135,9 @@ enum WalletFlags : uint64_t {
135     // wallet flags in the upper section (> 1 << 31) will lead to not opening the wallet if flag is unknown
136     // unknown wallet flags in the lower section <= (1 << 31) will be tolerated
137 
138+    // Indicates that the metadata has already been upgraded to contain key origins
139+    WALLET_FLAG_KEY_ORIGIN_METADATA = (1ULL << 0),

Sjors commented at 7:42 pm on February 14, 2019:

Maybe use 1? https://github.com/bitcoin/bitcoin/pull/13756/files#diff-12635a58447c65585f51d32b7e04075bR140

achow101 commented at 7:48 pm on February 14, 2019:

Done

Sjors commented at 7:43 pm on February 14, 2019: member

utACK 8fff73c modulo Travis; that was smaller change than I expected

I can’t reproduce the failing QT test on Travis on macOS 10.14.3, so I restarted the job.

achow101 force-pushed on Feb 14, 2019

instagibbs commented at 7:52 pm on February 14, 2019: member

only change is WALLET_FLAG_KEY_ORIGIN_METADATA flag from 0 to 1.

re-utACK https://github.com/bitcoin/bitcoin/pull/14021/commits/1b1c6aa276f24ff85392915c3b557e2343c88252

Sjors commented at 7:52 pm on February 14, 2019: member

utACK 1b1c6aa

ajtowns commented at 10:06 pm on February 14, 2019: member

utACK 1b1c6aa276f24ff85392915c3b557e2343c88252

in src/wallet/rpcdump.cpp:1265 in 35d6bc5793 outdated

1261@@ -1261,6 +1262,8 @@ static UniValue ProcessImport(CWallet * const pwallet, const UniValue& data, con
1262             if (!pwallet->GetPubKey(id, temp) && !pwallet->AddWatchOnly(GetScriptForRawPubKey(pubkey), timestamp)) {
1263                 throw JSONRPCError(RPC_WALLET_ERROR, "Error adding address to wallet");
1264             }
1265+            pwallet->AddKeyOrigin(pubkey, import_data.key_origins[id]);

sipa commented at 10:23 pm on February 14, 2019:

Should this be called when id is not a member of import_data.key_origins? The non-descriptor imports won’t have this map entries created.

achow101 commented at 10:49 pm on February 14, 2019:

It should check that. I’ve added the check and a test for this.

in doc/release-notes-14021.md:4 in 1b1c6aa276 outdated

0@@ -0,0 +1,11 @@
1+Miscellaneous RPC Changes
2+-------------------------
3+- Descriptors with key origin information imported through `importmulti` will have their key origin information stored in the wallet for use with creating PSBTs.
4+- If `bip32derivs` of both `walletprocesspsbt` and `walletcreatefundedpsbt` is set to true but the key metadata for a public key has not been updated yet, then that key will have a derivation path as if it were just an independent key (i.e. no derivation path and it's master fingerprint is itself)

sipa commented at 10:23 pm on February 14, 2019:

Typo: it's -> its

achow101 commented at 10:49 pm on February 14, 2019:

Done

sipa commented at 10:24 pm on February 14, 2019: member

utACK, only nits

achow101 force-pushed on Feb 14, 2019

achow101 commented at 10:52 pm on February 14, 2019: member

oops, screwed up that most recent push, don’t merge yet

achow101 force-pushed on Feb 14, 2019

Store key origin info in key metadata

Store the master key fingerprint and derivation path in the
key metadata. hdKeypath is kept to indicate the seed and for
backwards compatibility, but all key derivation path output
uses the key origin info instead of hdKeypath.

eab63bc264

Implement a function to add KeyOriginInfo to a wallet 3d235dff5d

Import KeyOriginData when importing descriptors 02d6586d7a

Test importing descriptors with key origin information 4c75a69f36

Add release notes for importing key origin info change cb3511b9d5

achow101 force-pushed on Feb 14, 2019

meshcollider commented at 11:10 pm on February 14, 2019: contributor

Changes since @sipa’s are just nit-fixes, a new test, and a missing brace bugfix :+1:

meshcollider merged this on Feb 14, 2019

meshcollider closed this on Feb 14, 2019

meshcollider referenced this in commit 8d0ec74801 on Feb 14, 2019

meshcollider removed this from the "Blockers" column in a project

deadalnix referenced this in commit eb725bf601 on May 22, 2020

deadalnix referenced this in commit 179223f0b8 on May 22, 2020

deadalnix referenced this in commit 63a0aeceba on May 22, 2020

deadalnix referenced this in commit 600f5a86a2 on May 22, 2020

deadalnix referenced this in commit c4f1a7bf5d on May 22, 2020

deadalnix referenced this in commit ed9768d52c on May 22, 2020

deadalnix referenced this in commit 5dd2d94fb9 on May 22, 2020

deadalnix referenced this in commit 54e7526f0b on May 22, 2020

ftrader referenced this in commit 0caa89dacf on Aug 17, 2020

vijaydasmp referenced this in commit 8083d77dbe on Sep 28, 2021

vijaydasmp referenced this in commit ee699760c2 on Sep 28, 2021

vijaydasmp referenced this in commit 14d28c5641 on Sep 28, 2021

vijaydasmp referenced this in commit 8a3b0bcbf1 on Sep 28, 2021

vijaydasmp referenced this in commit 6f21b073ff on Dec 31, 2021

vijaydasmp referenced this in commit 61612e701f on Dec 31, 2021

vijaydasmp referenced this in commit ccc73a9b05 on Dec 31, 2021

vijaydasmp referenced this in commit 08d83c783d on Jan 25, 2022

vijaydasmp referenced this in commit a81de89f84 on Jan 25, 2022

DrahtBot locked this on Feb 15, 2022

Import key origin data through descriptors in importmulti #14021

Conflicts