[wallet] Upgrade path for non-HD wallets to HD

achow101 commented at 4:31 pm on February 27, 2018: member

Revival/rebase of #11085

Adds a new command sethdseed which allows you to either set or generate a new HD seed to be used. A new keypool can be generated or the original one kept and new keys added to the keypool will come from the new HD seed.

Wallets that are not HD will be upgraded to be version FEATURE_HD_SPLIT when the sethdseed RPC command is used.

I have also add some tests for this.

Additionally -upgradewallet can now be used to upgrade a wallet from non-HD to HD. When it is used for such an upgrade, the keypool will be regenerated.

achow101 force-pushed on Feb 27, 2018

in src/wallet/rpcwallet.cpp:3807 in 48f09bd640 outdated

3802+            "\nSet or generate a new HD wallet seed. Non-HD wallets will be upgraded to being a HD wallet. Wallets that are already\n"
3803+            "HD will have a new HD seed set so that new keys added to the keypool will be derived from this new seed."
3804+            "\nNote that you will need to MAKE A NEW BACKUP of your wallet after setting the HD wallet seed.\n"
3805+            + HelpRequiringPassphrase(pwallet) +
3806+            "\nArguments:\n"
3807+            "1. \"flushkeypool\"       (boolean, optional, default=true) Whether to flush old unused addresses from the keypool.\n"

instagibbs commented at 5:00 pm on February 27, 2018:

Text should also tell user that change addresses will be removed(or not)

achow101 commented at 5:39 pm on February 27, 2018:

Done. Also included some more text explaining hd chain split stuff.

achow101 force-pushed on Feb 27, 2018

fanquake added the label Wallet on Feb 27, 2018

fanquake added the label RPC/REST/ZMQ on Feb 27, 2018

in src/wallet/rpcwallet.cpp:3833 in d40de04228 outdated

3828+    if (!request.params[0].isNull()) flush_key_pool = request.params[0].get_bool();
3829+
3830+    CPubKey master_pub_key;
3831+    if (!request.params[1].isNull()) {
3832+        CBitcoinSecret secret;
3833+        if (!secret.SetString(request.params[1].get_str())) throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid private key encoding");

Empact commented at 0:48 am on March 1, 2018:

Nit: IMO this inline throw style makes the code less readable, and it’s not used elsewhere in the file.

achow101 commented at 10:21 pm on March 1, 2018:

Fixed this and other places that have similar inline ifs that are hard to read.

jonasschnelli commented at 12:45 pm on March 1, 2018: contributor

I think this can be useful, though the following potential risks may be harmful in certain scenarios:

Missing key rotation: generating a new hd seed may imply for novice users that this protects from a compromised seed/masterkey.
seed option: if someone uses the optional seed parameter, it’s possible that the child keys have already been used to send funds to. Eventually a rescan or a UTXO-set scan should be considered?
“Upgrading” existing non-hd wallets: leads to a mix of HD non HD-keys. Hard to “untangle” the non HD keys in case one wants to forward all funds to the new hd keyspace.

achow101 commented at 10:14 pm on March 1, 2018: member

Missing key rotation: generating a new hd seed may imply for novice users that this protects from a compromised seed/masterkey.

The help text could be updated to indicate that only coins sent to newly generated addresses will have keys generated with the new seed.

seed option: if someone uses the optional seed parameter, it’s possible that the child keys have already been used to send funds to. Eventually a rescan or a UTXO-set scan should be considered?

I could add a rescan option that is disabled by default. It could only be enabled if explicitly set and if the keypool is regenerated.

“Upgrading” existing non-hd wallets: leads to a mix of HD non HD-keys. Hard to “untangle” the non HD keys in case one wants to forward all funds to the new hd keyspace.

At the time of upgrade, one could send all coins to a new address.

achow101 force-pushed on Mar 1, 2018

unknown commented at 10:41 pm on March 5, 2018: none

OK

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

in src/wallet/rpcwallet.cpp:3838 in 6e8eb0830c outdated

3831+
3832+    CPubKey master_pub_key;
3833+    if (!request.params[1].isNull()) {
3834+        CBitcoinSecret secret;
3835+        if (!secret.SetString(request.params[1].get_str())) {
3836+            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid private key encoding");

promag commented at 9:39 am on March 16, 2018:

Missing test for this error and another for params[1].get_str().

achow101 commented at 5:08 am on March 18, 2018:

Done

in src/wallet/rpcwallet.cpp:3843 in 6e8eb0830c outdated

3836+            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid private key encoding");
3837+        }
3838+
3839+        CKey key = secret.GetKey(), key2;
3840+        if (!key.IsValid()) {
3841+            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Private key outside allowed range");

promag commented at 9:39 am on March 16, 2018:

Missing test for this error.

achow101 commented at 5:08 am on March 18, 2018:

Done

in src/wallet/rpcwallet.cpp:4127 in 6e8eb0830c outdated

3824+    LOCK2(cs_main, pwallet->cs_wallet);
3825+    EnsureWalletIsUnlocked(pwallet);
3826+
3827+    bool flush_key_pool = true;
3828+    if (!request.params[0].isNull()) {
3829+        flush_key_pool = request.params[0].get_bool();

promag commented at 9:40 am on March 16, 2018:

Missing test get_bool().

achow101 commented at 5:08 am on March 18, 2018:

Done

in src/wallet/rpcwallet.cpp:4140 in 6e8eb0830c outdated

3842+        }
3843+
3844+        // check that we don't already have this key, compressed or otherwise
3845+        key2.Set(key.begin(), key.end(), !key.IsCompressed());
3846+        if (pwallet->HaveKey(key.GetPubKey().GetID()) || pwallet->HaveKey(key2.GetPubKey().GetID())) {
3847+            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Already have this key (either as an HD seed or as a loose private key)");

promag commented at 9:41 am on March 16, 2018:

Missing test for this error.

achow101 commented at 5:09 am on March 18, 2018:

Done

in src/wallet/rpcwallet.cpp:3858 in 6e8eb0830c outdated

3853+    }
3854+
3855+    // Upgrade the wallet to version FEATURE_HD_SPLIT if it isn't aleady
3856+    pwallet->SetMinVersion(FEATURE_HD_SPLIT);
3857+
3858+    if (!pwallet->SetHDMasterKey(master_pub_key)) {

promag commented at 9:44 am on March 16, 2018:

This always returns true so it’s not possible to test the error raised below 🙄

achow101 commented at 5:09 am on March 18, 2018:

Removed the if block

in src/wallet/rpcwallet.cpp:4151 in 6e8eb0830c outdated

3858+    if (!pwallet->SetHDMasterKey(master_pub_key)) {
3859+        throw JSONRPCError(RPC_WALLET_ERROR, "Storing master key failed");
3860+    }
3861+    if (flush_key_pool) pwallet->NewKeyPool();
3862+
3863+    return true;

promag commented at 9:53 am on March 16, 2018:

Should test return value (never false). However consider returning an object as per developer notes.

achow101 commented at 5:09 am on March 18, 2018:

Changed to NullUniValue

sipa commented at 8:41 pm on May 9, 2018:

No, not changed.

in src/wallet/rpcwallet.cpp:3808 in 6e8eb0830c outdated

3803+            "HD will have a new HD seed set so that new keys added to the keypool will be derived from this new seed. Wallets that do\n"
3804+            "not support the HD Chain split feature will be upgraded to do so and all new change addresses will come from the internal keychain.\n"
3805+            "\nNote that you will need to MAKE A NEW BACKUP of your wallet after setting the HD wallet seed.\n"
3806+            + HelpRequiringPassphrase(pwallet) +
3807+            "\nArguments:\n"
3808+            "1. \"flushkeypool\"       (boolean, optional, default=true) Whether to flush old unused addresses, including change addresses, from the keypool.\n"

promag commented at 9:57 am on March 16, 2018:

Maybe should be newkeypool?

achow101 commented at 5:09 am on March 18, 2018:

Done

in src/wallet/rpcwallet.cpp:4147 in 6e8eb0830c outdated

3856+    pwallet->SetMinVersion(FEATURE_HD_SPLIT);
3857+
3858+    if (!pwallet->SetHDMasterKey(master_pub_key)) {
3859+        throw JSONRPCError(RPC_WALLET_ERROR, "Storing master key failed");
3860+    }
3861+    if (flush_key_pool) pwallet->NewKeyPool();

promag commented at 9:59 am on March 16, 2018:

Return value of NewkeyPool ignored and nothing is logged. Maybe that’s fine?

achow101 commented at 5:09 am on March 18, 2018:

That’s fine. It only returns false when the wallet is locked, and that is caught earlier with EnsureWalletIsUnlocked

in src/wallet/rpcwallet.cpp:4090 in 6e8eb0830c outdated

3794+
3795+    if (!EnsureWalletIsAvailable(pwallet, request.fHelp)) {
3796+        return NullUniValue;
3797+    }
3798+
3799+    if (request.fHelp || request.params.size() > 2) {

promag commented at 10:01 am on March 16, 2018:

Could have a test for extra parameters. See example.

achow101 commented at 5:09 am on March 18, 2018:

Done

in src/wallet/rpcwallet.cpp:3833 in 6e8eb0830c outdated

3828+    if (!request.params[0].isNull()) {
3829+        flush_key_pool = request.params[0].get_bool();
3830+    }
3831+
3832+    CPubKey master_pub_key;
3833+    if (!request.params[1].isNull()) {

promag commented at 10:03 am on March 16, 2018:

Maybe:

0if (request.params[1].isNull()) {
1    master_pub_key = pwallet->GenerateNewHDMasterKey();
2} else {

achow101 commented at 5:10 am on March 18, 2018:

Done

achow101 force-pushed on Mar 18, 2018

achow101 commented at 7:30 pm on March 18, 2018: member

Had to rebase this.

in test/functional/wallet_hd.py:130 in 1bc2bf2a31 outdated

123+        orig_masterkeyid = self.nodes[1].getwalletinfo()['hdmasterkeyid']
124+        self.nodes[1].sethdseed()
125+        new_masterkeyid = self.nodes[1].getwalletinfo()['hdmasterkeyid']
126+        assert orig_masterkeyid != new_masterkeyid
127+        addr = self.nodes[1].getnewaddress()
128+        assert_equal(self.nodes[1].getaddressinfo(addr)['hdkeypath'], 'm/0\'/0\'/0\'') # Make sure the new address is the first from the keypool

instagibbs commented at 5:14 pm on March 23, 2018:

nit: also check it’s not the same key(not just path)

achow101 commented at 6:13 pm on March 28, 2018:

Done

in test/functional/wallet_hd.py:151 in 1bc2bf2a31 outdated

143+        addr = self.nodes[1].getnewaddress()
144+        assert_equal(new_masterkeyid, self.nodes[1].getaddressinfo(addr)['hdmasterkeyid'])
145+        assert_equal(self.nodes[1].getaddressinfo(addr)['hdkeypath'], 'm/0\'/0\'/0\'') # Make sure the new address continues previous keypool
146+
147+        # Sethdseed parameter validity
148+        assert_raises_rpc_error(-1, 'sethdseed', self.nodes[0].sethdseed, False, new_seed, 0)

instagibbs commented at 5:28 pm on March 23, 2018:

is this for invalid named arguments? Without running this I’m not sure what it’s doing

achow101 commented at 6:00 pm on March 28, 2018:

This just tests that an error is thrown when there are too many arguments

in src/wallet/rpcwallet.cpp:3839 in a5c95dd4b3 outdated

3834+            "\nArguments:\n"
3835+            "1. \"newkeypool\"         (boolean, optional, default=true) Whether to flush old unused addresses, including change addresses, from the keypool and regenerate it.\n"
3836+            "                             If true, the next address from getnewaddress and change address from getrawchangeaddress will be from this new seed.\n"
3837+            "                             If false, addresses (including change addresses if the wallet already had HD Chain Split enabled) from the existing \n"
3838+            "                             keypool will be used until it has been depleted.\n"
3839+            "2. \"seed\"               (string, optional) The WIF private key to use as the new HD seed;\n"

instagibbs commented at 5:41 pm on March 23, 2018:

I think it stands to be noted that Core software will never(?) return this value on a currently running wallet. You can get address’ keys, but not the seed itself. I can easily see users getting confused and thinking that backing up some address key will save their wallet in the future.

You may want to leave a note here saying how this specific key can be extracted from the wallet, through dumpwallet->hdmaster=1 key

achow101 commented at 6:17 pm on March 28, 2018:

Done

in src/wallet/rpcwallet.cpp:3869 in 1bc2bf2a31 outdated

3863+        CKey key = DecodeSecret(request.params[1].get_str()), key2;
3864+        if (!key.IsValid()) {
3865+            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid private key");
3866+        }
3867+
3868+        // check that we don't already have this key, compressed or otherwise

jimpo commented at 5:32 pm on March 27, 2018:

What is the reason for disallowing master keys with opposite compressedness? Unless another party knows the master xpubkey, they would not be able to tell that any child keys are related through a shared master key.

achow101 commented at 6:07 pm on March 28, 2018:

The seed value is interpreted and saved as private key. We don’t want to have a seed where we already have that seed value as a key in the wallet. The only way to check whether we have a given private key is to derive its public key, get the id of that public key, and look it up. So to ensure that we don’t have the seed value already, we need to derive both compressed and uncompressed public keys, get their ids, and look them up.

It’s important to note that the seed is not actually a private key so it doesn’t have a corresponding public key. We just use WIF because its a common format.

jimpo commented at 6:58 pm on March 28, 2018:

What is the problem with having a seed that is the same private key with opposite compressedness? It wouldn’t have an existing entry in the the wallet file because the CKeyID is different.

If there is a good reason to protect against this, it might make sense to add a bool HaveKey(CKey& key) const method on CKeyStore.

achow101 commented at 4:33 pm on March 31, 2018:

If we use a key that already exists in the wallet but with opposite compressedness, that key could be exportable with dumpprivkey. But the user would not necessarily know that the key that is exported is their seed just with opposite compressedness. I don’t think that we want the seed to be exportable like that, and if we do want the seed to be exported in some way, it should be via its own RPC and not through dumpprivkey.

jimpo commented at 4:32 pm on April 2, 2018:

OK. I’m still not convinced (I could make similar arguments about rejecting keys with the lowest bit toggled), but it doesn’t really matter.

I would prefer to see the lookups moved over to a HaveKey(CKey& key) method on KeyStore though.

sipa commented at 7:13 pm on April 12, 2018:

@jimpo I don’t understand what HaveKey(CKey) would do. There is a 1-to-1 correspondence both ways between CKeys and CPubKeys.

jimpo commented at 8:05 pm on April 12, 2018:

@sipa It would replicate the logic here that checks whether there is a CPubKey corresponding to either the provided CKey or the one with opposite compressedness. Basically, I don’t think that this RPC function is the best place for that logic. Maybe the method would be better named HaveKeyIgnoringCompressedness.

sipa commented at 8:13 pm on April 12, 2018:

@jimpo Ah, I see. My concern is adding extra unnecessary functions to the CKeyStore interface (which need to be implemented by all implementations, even though there’s just one now). What would you think about having a function (not method) in keystore.h/cpp that does this, which takes a const CKeyStore& as argument?

achow101 commented at 9:37 pm on April 12, 2018:

I implemented @sipa’s suggestion

achow101 force-pushed on Mar 28, 2018

TheBlueMatt commented at 4:47 pm on March 30, 2018: member

At the risk of expanding the scope, can we fix -upgradewallet to work for HD as well here? The fact that upgradewallet will work to upgrade your wallet to HD-1 and then you have to call sethdseed to upgrade seems….strange. Separately, can we either check that we’re fully synced before allowing an HD master rotate or keep around old HD keys for key derivation? I’d prefer the second, but its obviously a ton more complicated, so just ensuring that we’re at least synced first is likely sufficient to ensure people dont rotate and miss some payments.

achow101 force-pushed on Mar 31, 2018

achow101 renamed this:
~~[wallet][RPC] Set or generate a new HD seed~~
[wallet] Upgrade path for non-HD wallets to HD
on Mar 31, 2018

achow101 commented at 6:26 pm on March 31, 2018: member

I changed -upgradewallet so it upgrades non-HD wallets to HD and HD chain split. I’ve updated the OP and title to indicate as such.

Unfortunately automated tests can’t be done with -upgradewallet.

Separately, can we either check that we’re fully synced before allowing an HD master rotate

This seems incompatible with -upgradewallet as an upgrade method.

or keep around old HD keys for key derivation? I’d prefer the second, but its obviously a ton more complicated, so just ensuring that we’re at least synced first is likely sufficient to ensure people dont rotate and miss some payments.

I think this should be done as a separate PR. It’s quite a bit of scope creep.

achow101 force-pushed on Mar 31, 2018

TheBlueMatt commented at 1:58 pm on April 2, 2018: member

This seems incompatible with -upgradewallet as an upgrade method.

I dont care as much about it for first-upgrade (first-upgrade-to-HD is actually no different from just using your existing wallet from a backup/keypool perspective, as long as you dont flush keypool, its just a new way to generate keypool entries), but for HD master rotation, I think making sure we dont need any future new keys is pretty easy to add, no?

achow101 force-pushed on Apr 5, 2018

achow101 commented at 7:55 pm on April 5, 2018: member

I dont care as much about it for first-upgrade (first-upgrade-to-HD is actually no different from just using your existing wallet from a backup/keypool perspective

Right, duh.

I rebased this and added a check for IBD so it won’t set a new seed if we are still in IBD.

achow101 force-pushed on Apr 12, 2018

jimpo commented at 1:56 am on April 13, 2018: contributor

Thinking about this a bit more, it definitely feels odd that I can’t sethdseed to a past value. If I have a seed set, then change it to a new one, then want to change it back, I’m not allowed to do that. Not a big deal I guess…

utACK modulo the linter error:

0src/wallet/wallet.cpp:                LogPrintf("Upgrading wallet to use HD chain split");
1^---- failure generated from contrib/devtools/lint-logs.sh

achow101 force-pushed on Apr 13, 2018

achow101 commented at 2:38 am on April 13, 2018: member

Fixed the linter I think

in src/wallet/wallet.cpp:4002 in 3bed0c2019 outdated

4006-        // generate a new master key
4007-        CPubKey masterPubKey = walletInstance->GenerateNewHDMasterKey();
4008-        if (!walletInstance->SetHDMasterKey(masterPubKey))
4009-            throw std::runtime_error(std::string(__func__) + ": Storing master key failed");
4010+        // Upgrade to HD if necessary
4011+        LOCK(walletInstance->cs_wallet);

sipa commented at 2:27 am on April 17, 2018:

Perhaps you wanted this LOCK inside the scope that follows? As is, the lock is held during the call to SetBestChain below.

achow101 commented at 5:26 pm on April 17, 2018:

Yes, that’s what I wanted to do. Fixed.

sipa commented at 2:28 am on April 17, 2018: member

Concept ACK

achow101 force-pushed on Apr 17, 2018

in src/wallet/rpcwallet.cpp:3877 in 077afcc502 outdated

3872+
3873+        master_pub_key = pwallet->DeriveNewMasterHDKey(key);
3874+    }
3875+
3876+    // Upgrade the wallet to version FEATURE_HD_SPLIT if it isn't aleady
3877+    pwallet->SetMinVersion(FEATURE_HD_SPLIT);

TheBlueMatt commented at 6:34 pm on April 17, 2018:

Should probably not silently upgrade HD-but-not-HD_SPLIT wallets to HD_SPLIT here.

achow101 commented at 7:36 pm on April 17, 2018:

This is explicitly mentioned in the help text.

TheBlueMatt commented at 7:38 pm on April 17, 2018:

Still probably shouldn’t do it…there isnt any reason for it, is there?

achow101 commented at 7:45 pm on April 17, 2018:

I suppose there isn’t. But then if the version number is changed only for non-HD wallets to upgrade them, then theres still the same issue with keypools.

in src/wallet/rpcwallet.cpp:4147 in 077afcc502 outdated

3874+    }
3875+
3876+    // Upgrade the wallet to version FEATURE_HD_SPLIT if it isn't aleady
3877+    pwallet->SetMinVersion(FEATURE_HD_SPLIT);
3878+    pwallet->SetHDMasterKey(master_pub_key);
3879+    if (flush_key_pool) pwallet->NewKeyPool();

TheBlueMatt commented at 7:03 pm on April 17, 2018:

This is not sufficient to ensure old wallets continue to operate using the existing keypool. In the case of HD split, you’re gonna have an empty internal keypool and will instantly refresh your keypool, resulting in all change addresses being from the new HD seed. I dont see an obvious fix, but its definitely a major issue.

achow101 commented at 7:37 pm on April 17, 2018:

Do you think that it would be better then to not touch the version number in sethdseed and leave that to -upgradewallet? So then sethdseed would only work for HD wallets and retain whatever splitness is already in the wallet.

TheBlueMatt commented at 7:46 pm on April 17, 2018:

I think that would solve it for sethdseed and would certainly make this easier to review (as its otherwise the first time we’re doing an upgrade at runtime), though it does limit the utility a good bit. It would not, however, solve the identical issue during -upgradewallet.

in src/wallet/wallet.h:92 in 077afcc502 outdated

94@@ -95,7 +95,7 @@ enum WalletFeature
95 
96     FEATURE_NO_DEFAULT_KEY = 159900, // Wallet without a default key written
97 
98-    FEATURE_LATEST = FEATURE_COMPRPUBKEY // HD is optional, use FEATURE_COMPRPUBKEY as latest version
99+    FEATURE_LATEST = FEATURE_NO_DEFAULT_KEY

TheBlueMatt commented at 7:05 pm on April 17, 2018:

I dont think we should “upgrade” to NO_DEFAULT_KEY (unless we delete the default key, but that seems useless). Rather, we should upgrade to HD_SPLIT and then upgrade to NO_DEFAULT_KEY when we next bump FEATURE_LATEST (probably leave a comment to that effect here).

achow101 commented at 7:49 pm on April 17, 2018:

I don’t see why not upgrade to NO_DEFAULT_KEY. FEATURE_LATEST is only used for new wallets and upgraded wallets; users should expect new and upgraded wallets to not be backwards compatible.

TheBlueMatt commented at 6:08 pm on April 19, 2018:

NO_DEFAULT_KEY doesnt even have a meaning in this case, I mean you’re upgrading to nVersion == NO_DEFAULT_KEY but you still have a default key? Its just needless incompatibility at that point (though, I agree, not a huge deal given the user asked for that).

sipa commented at 9:04 pm on May 12, 2018:

@TheBlueMatt Does that matter? This is just the default for new wallets or explicit upgrades. I don’t really care about gratuitous incompatibility when you’re explicitly not caring about that.

UPDATE: especially with the new PRE_SPLIT type added afterwards this is probably inevitable.

in src/wallet/wallet.cpp:4067 in 077afcc502 outdated

4022+                hd_upgrade = true;
4023+            }
4024+            // Upgrade to HD chain split if necessary
4025+            if (walletInstance->CanSupportFeature(FEATURE_HD_SPLIT)) {
4026+                LogPrintf("Upgrading wallet to use HD chain split\n");
4027+                walletInstance->SetMinVersion(FEATURE_HD_SPLIT);

TheBlueMatt commented at 7:08 pm on April 17, 2018:

Same comment here, this implicitly will generate a new keypool on next internal key usage.

achow101 commented at 7:48 pm on April 17, 2018:

Is that a problem though? An upgrade was explicitly asked for. Would it be better to regenerate the keypool in this instance too? Either way, both the internal and external keypools will still be generated from the same seed.

TheBlueMatt commented at 6:08 pm on April 19, 2018:

No, this significantly changes the upgradewallet semantics, before you could upgrade and you’d still be able to use your backup in the same way based on the keypool size, now that is no longer true, which needs huge flashing warnings, at a minimum.

sipa commented at 6:57 pm on April 19, 2018:

Good point, @TheBlueMatt. I was forgetting about the distinction “need a new backup” and “need new software in the future”.

What would you suggest as an ideal solution? -upgradewallet by default never making a change that requires a new backup, while having another option (-fullupgradewallet ?) to indicate you’re aware a new backup is needed?

achow101 commented at 7:59 pm on April 19, 2018:

What about an InitWarning message that appears when the keypool is regenerated that a new backup will need to be made?

in src/wallet/wallet.cpp:4031 in 077afcc502 outdated

3999-        if (!gArgs.GetBoolArg("-usehd", true)) {
4000+        if (!gArgs.GetBoolArg("-usehd", true) && fFirstRun) {
4001             InitError(strprintf(_("Error creating %s: You can't create non-HD wallets with this version."), walletFile));
4002             return nullptr;
4003         }
4004-        walletInstance->SetMinVersion(FEATURE_NO_DEFAULT_KEY);

TheBlueMatt commented at 7:09 pm on April 17, 2018:

Why is it ok to remove this if (fFirstRun) SetMinVersion(NO_DEFAULT_KEY)?

achow101 commented at 7:42 pm on April 17, 2018:

-upgradewallet is implicitly true (and 0) on the first run. So the wallet version will be set to FEATURE_LATESTwhich is FEATURE_NO_DEFAULT_KEY.

TheBlueMatt commented at 4:09 am on April 28, 2018:

Hmm, but what if I override it? Can I start my node accidentally with upgradewallet=0 have it create a wallet and end up with a borked wallet?

TheBlueMatt commented at 8:54 pm on May 1, 2018:

Still needs addressing, I think -upgradewallet=0 may break first-init after this patch.

achow101 commented at 5:02 am on May 5, 2018:

I changed the if statement to if (gArgs.GetBoolArg("-upgradewallet", fFirstRun) || fFirstRun)

TheBlueMatt commented at 6:31 pm on May 7, 2018:

I still dont think this is right - I dont see where an fFirstRun wallet with upgradewallet=something non-0 in the bitcoin.conf will get its MinVersion set to at least FEATURE_NO_DEFAULT_KEY.

sipa commented at 10:54 pm on May 9, 2018:

@TheBlueMatt I’m confused by the history here - does this still apply?

achow101 force-pushed on Apr 17, 2018

achow101 commented at 8:12 pm on April 17, 2018: member

I changed sethdseed to only work on HD wallets and not modify the wallet version.

sipa commented at 1:34 am on April 19, 2018: member

utACK afafc9b92dc1c501ca1ff63a5042e1925b7ac55a. I think it’s fine that running with -upgradewallet defaults to NO_DEFAULT_KEY.

TheBlueMatt commented at 7:01 pm on April 19, 2018: member

One option might be splitting the existing keypool between internal and external at the time of the upgrade, though that also violates previous API guarantees. The only “full” fix would require the new-key-from-pool callsites to be aware of the upgrade, though I don’t think that’d be a ton of code.

On April 19, 2018 6:57:17 PM UTC, Pieter Wuille notifications@github.com wrote:

sipa commented on this pull request.
       LOCK(walletInstance->cs_wallet);
       bool hd_upgrade = false;
       if (walletInstance->CanSupportFeature(FEATURE_HD) &&
!walletInstance->IsHDEnabled()) {
           LogPrintf("Upgrading wallet to HD\n");
           walletInstance->SetMinVersion(FEATURE_HD);
           // generate a new master key
           CPubKey masterPubKey =
walletInstance->GenerateNewHDMasterKey();
           if (!walletInstance->SetHDMasterKey(masterPubKey))
               throw std::runtime_error(std::string(__func__) +
“: Storing master key failed”);
           hd_upgrade = true;
       }
       // Upgrade to HD chain split if necessary
       if (walletInstance->CanSupportFeature(FEATURE_HD_SPLIT)) {
           LogPrintf("Upgrading wallet to use HD chain split\n");
           walletInstance->SetMinVersion(FEATURE_HD_SPLIT);
Good point, @TheBlueMatt. I was forgetting about the distinction “need a new backup” and “need new software in the future”.

What would you suggest as an ideal solution? -upgradewallet by default never making a change that requires a new backup, while having another option (-fullupgradewallet ?) to indicate you’re aware a new backup is needed?

– You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/12560#discussion_r182850939

TheBlueMatt commented at 8:06 pm on April 19, 2018: member

I don’t think InitWarning is sufficient. For GUI I’d expect a pop-up asking if I’d like to continue, on bitcoind people don’t read their debug.log, so unless we want to implement the “knowledge of past upgrade” in new key logic (which I think is probably doable), we probably need a different option :(.

On April 19, 2018 7:59:53 PM UTC, Andrew Chow notifications@github.com wrote:

achow101 commented on this pull request.

       LOCK(walletInstance->cs_wallet);

```
       bool hd_upgrade = false;
```

       if (walletInstance->CanSupportFeature(FEATURE_HD) &&

!walletInstance->IsHDEnabled()) {

           LogPrintf("Upgrading wallet to HD\n");

           walletInstance->SetMinVersion(FEATURE_HD);

           // generate a new master key

```
           CPubKey masterPubKey =
```

walletInstance->GenerateNewHDMasterKey();

           if (!walletInstance->SetHDMasterKey(masterPubKey))

               throw std::runtime_error(std::string(__func__) +

“: Storing master key failed”);

```
           hd_upgrade = true;
```
```
       }
```

       // Upgrade to HD chain split if necessary

       if (walletInstance->CanSupportFeature(FEATURE_HD_SPLIT)) {

           LogPrintf("Upgrading wallet to use HD chain split\n");

           walletInstance->SetMinVersion(FEATURE_HD_SPLIT);

What about an InitWarning message that appears when the keypool is regenerated that a new backup will need to be made?

– You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/12560#discussion_r182867406

achow101 commented at 8:18 pm on April 19, 2018: member

For GUI I’d expect a pop-up asking if I’d like to continue

There’s UIInterface.ThreadSafeQuestion which can be used for that.

on bitcoind people don’t read their debug.log

InitError, InitWarning, and ThreadSafeQuestion all print to stderr for bitcoind. Also a recent change makes -printtoconsole default when using without -daemon.

~~I suppose the case with -daemon can really only be accounted for with a separate option.~~ Actually it should still print even with -daemon.

(I don’t like the idea of introducing another option for this)

TheBlueMatt commented at 8:22 pm on April 19, 2018: member

If you get a chance to implement tracking upgrade and remaining-original-keypool stuff I have a feeling it’ll be workable.

On April 19, 2018 8:18:22 PM UTC, Andrew Chow notifications@github.com wrote:

For GUI I’d expect a pop-up asking if I’d like to continue

There’s UIInterface.ThreadSafeQuestion which can be used for that.

on bitcoind people don’t read their debug.log

InitError, InitWarning, and ThreadSafeQuestion all print to stderr for bitcoind (unless -daemon). Also a recent change makes -printtoconsole default when using without -daemon.

I suppose the case with -daemon can really only be accounted for with a separate option.

– You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/12560#issuecomment-382867483

achow101 commented at 8:30 pm on April 19, 2018: member

If you get a chance to implement tracking upgrade and remaining-original-keypool stuff I have a feeling it’ll be workable.

So basically use the original keypool until it runs out, and then new keys are generated from the seed? So the callers (particularly getting change keys) will need to know whether it should use external keypool if it was upgraded but not run out yet?

I assume that this will still require warning that a new backup is required.

TheBlueMatt commented at 8:39 pm on April 19, 2018: member

Yes, but that doesn’t need a new backup any more than using up your current keypool does. The real issue is I believe we’d need a version bump for it (and, thus, you’d need to upgrade to the new version and wouldn’t be able to upgrade to HD/HD_SPLIT/NO_DEFAULT_KEY but would have to skip them and upgrade to the new version).

On April 19, 2018 8:30:18 PM UTC, Andrew Chow notifications@github.com wrote:

If you get a chance to implement tracking upgrade and remaining-original-keypool stuff I have a feeling it’ll be workable.

So basically use the original keypool until it runs out, and then new keys are generated from the seed? So the callers (particularly getting change keys) will need to know whether it should use external keypool if it was upgraded but not run out yet?

I assume that this will still require warning that a new backup is required.

– You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/12560#issuecomment-382870871

achow101 commented at 10:10 pm on April 19, 2018: member

I don’t think that a wallet version upgrade is necessarily required.

For upgrade from non-HD to HD, it’s easy to tell when you have exhausted the original keypool: the CKeyMetadata hdMsaterKeyID for each key will change from Null to the current hdMasterKeyID.

Non chain split to chain split is harder. However it would be very easy if a new seed was used. I don’t think there would be any adverse effects if the upgrade from non split to split resulted in a new seed being set, although that may be undesireable.

achow101 commented at 10:40 pm on April 19, 2018: member

Actually bumping the version number and having another field is easier to do and has less weirdness with -salvagewallet scenarios, so I’m going to to that instead.

achow101 commented at 10:59 pm on April 19, 2018: member

Hmm, -salvagewallet isn’t actually a problem because pool entries aren’t retained. Also there’s no guarantees of anything when that is used.

The question then is whether we want to generate a new seed for non-split to split upgrade or if there is some other way to detect that upgrade.

achow101 force-pushed on Apr 21, 2018

achow101 commented at 7:17 am on April 21, 2018: member

I have decided to go with this method (from the commit message):

After upgrading to HD chain split, we want to continue to use keys from the old keypool. To do this, before we generate any new keys after upgrading, we mark all of the keypool entries as being pre-chain split and move them to a separate pre chain split keypool. Keys are fetched from that keypool until it is mptied. Only then are the new internal and external keypools used.

With this approach, we don’t need to change the wallet version.

Also rebased

achow101 force-pushed on Apr 21, 2018

achow101 force-pushed on Apr 26, 2018

achow101 commented at 6:12 pm on April 26, 2018: member

Merge/review beg

achow101 force-pushed on Apr 26, 2018

TheBlueMatt commented at 6:30 pm on April 27, 2018: member

Thanks, I do like the pre-split keypool thing. However, I do think you need to bump the version for it - otherwise you could 1) upgrade to HD, 2) downgrade one version to 0.16 (will still open your wallet), 3) end up using keys from the new keypool without realizing it. Its not exactly a huge concern, but fixing it seems to me to be ~free, so we might as well.

achow101 commented at 3:35 am on April 28, 2018: member

I bumped the wallet version number. I’ve also disallowed upgrades to any version between HD_SPLIT and PRE_SPLIT_KEYPOOL.

TheBlueMatt commented at 4:15 am on April 28, 2018: member

Looks good, will give it a full review next week!

achow101 force-pushed on Apr 28, 2018

in src/wallet/rpcwallet.cpp:4108 in be06fb1ba8 outdated

4141+            "true|false              (boolean) Returns true if successful\n"
4142+            "\nExamples:\n"
4143+            + HelpExampleCli("sethdseed", "")
4144+            + HelpExampleCli("sethdseed", "false")
4145+            + HelpExampleCli("sethdseed", "true \"wifkey\"")
4146+            + HelpExampleRpc("sethdseed", "true, \"wifkey\"")

TheBlueMatt commented at 8:30 pm on May 1, 2018:

Is this supposed to have a comma in the string?

achow101 commented at 4:46 am on May 5, 2018:

Yes

sipa commented at 8:48 pm on May 12, 2018:

Really? That’s not even valid JSON :)

achow101 commented at 11:41 pm on May 12, 2018:

Really? Looks like valid to me once the full string is constructed:

> curl --user myusername --data-binary '{"jsonrpc": "1.0", "id":"curltest", "method": "sethdseed", "params": [true, "wifkey"] }' -H 'content-type: text/plain;' http://127.0.0.1:8332/

sipa commented at 2:58 am on May 13, 2018:

Oops, you’re right. This is very confusing, though. Can’t we make HelpExampleRpc take a list of UniValue arguments, and have it construct the correct JSON code for it automatically (not for this PR, obviously).

in src/wallet/rpcwallet.cpp:4119 in be06fb1ba8 outdated

4150+    if (IsInitialBlockDownload()) {
4151+        throw JSONRPCError(RPC_CLIENT_IN_INITIAL_DOWNLOAD, "Cannot set a new HD seed while still in Initial Block Download");
4152+    }
4153+
4154+    // Do not do anything to non-HD wallets
4155+    if (!pwallet->IsHDEnabled()) {

TheBlueMatt commented at 8:33 pm on May 1, 2018:

This needs cs_wallet.

achow101 commented at 5:01 am on May 5, 2018:

Done

in src/wallet/rpcwallet.cpp:4171 in be06fb1ba8 outdated

4166+
4167+    CPubKey master_pub_key;
4168+    if (request.params[1].isNull()) {
4169+        master_pub_key = pwallet->GenerateNewHDMasterKey();
4170+    } else {
4171+        CKey key = DecodeSecret(request.params[1].get_str()), key2;

TheBlueMatt commented at 8:34 pm on May 1, 2018:

key2 looks unused.

achow101 commented at 5:01 am on May 5, 2018:

Removed

TheBlueMatt commented at 8:54 pm on May 1, 2018: member

Need to take appropriate action on the pre_split keys when set.*KeyPool is changed - at least the DBErrors::NEED_REWRITE handling, NewKeyPool, and ReturnKey handling looks like it should be updated.

Separate HaveKey function that checks whether a key is in a keystore dd3c07acce

achow101 force-pushed on May 5, 2018

achow101 commented at 5:02 am on May 5, 2018: member

Addressed @TheBlueMatt’s comments, also rebased.

Oh, forgot to do the things with pre_split_keypool

achow101 force-pushed on May 5, 2018

achow101 commented at 5:17 am on May 5, 2018: member

I have added checks for whether to use set_pre_split_keypool in places where keypools are chosen.

achow101 force-pushed on May 5, 2018

in src/wallet/wallet.cpp:4040 in 0444c769a8 outdated

4036@@ -4002,9 +4037,10 @@ CWallet* CWallet::CreateWalletFromFile(const std::string& name, const fs::path&
4037         }
4038     }
4039 
4040-    if (gArgs.GetBoolArg("-upgradewallet", fFirstRun))
4041+    if (gArgs.GetBoolArg("-upgradewallet", fFirstRun) || fFirstRun)

TheBlueMatt commented at 6:29 pm on May 7, 2018:

Grr, the whole first if block was dead code and is no longer, making this harder to review. I think its mostly equivalent to the old stuff, but would be really nice to get the diff cleaned up here. Can we just not merge out the two if statements?

TheBlueMatt commented at 6:31 pm on May 7, 2018: member

Can you try to simplify the diff on the CreateWalletFromFile stuff, its very hard to review (and I think subtly wrong in some strange edge-cases).

achow101 force-pushed on May 8, 2018

achow101 commented at 5:17 pm on May 8, 2018: member

@TheBlueMatt I simplified the diff by removing the combination of those two if blocks. The hd upgrading code has been moved to its own if block that enters if -upgradewallet is actually explicitly set.

in src/wallet/rpcwallet.cpp:4100 in 8db939a7b9 outdated

4095+            "\nNote that you will need to MAKE A NEW BACKUP of your wallet after setting the HD wallet seed.\n"
4096+            + HelpRequiringPassphrase(pwallet) +
4097+            "\nArguments:\n"
4098+            "1. \"newkeypool\"         (boolean, optional, default=true) Whether to flush old unused addresses, including change addresses, from the keypool and regenerate it.\n"
4099+            "                             If true, the next address from getnewaddress and change address from getrawchangeaddress will be from this new seed.\n"
4100+            "                             If false, addresses (including change addresses if the wallet already had HD Chain Split enabled) from the existing \n"

kallewoof commented at 9:41 am on May 9, 2018:

μ-nit: unnecessary space before \n at end of line.

achow101 commented at 4:46 pm on May 9, 2018:

Done

in src/wallet/rpcwallet.cpp:4149 in 8db939a7b9 outdated

4146+    }
4147+
4148+    pwallet->SetHDMasterKey(master_pub_key);
4149+    if (flush_key_pool) pwallet->NewKeyPool();
4150+
4151+    return NullUniValue;

kallewoof commented at 9:44 am on May 9, 2018:

Isn’t this supposed to return true or false depending on success? From L4105 above (result).

achow101 commented at 4:46 pm on May 9, 2018:

Fixed

achow101 commented at 2:29 am on May 10, 2018:

Actually I’ll keep it as NullUnivalue and I updated the docs.

kallewoof commented at 6:27 am on May 10, 2018:

Makes sense to me.

in src/wallet/wallet.cpp:4062 in 8db939a7b9 outdated

4056@@ -4021,14 +4057,57 @@ CWallet* CWallet::CreateWalletFromFile(const std::string& name, const fs::path&
4057         walletInstance->SetMaxVersion(nMaxVersion);
4058     }
4059 
4060+    // Upgrade to HD if explicit upgrade
4061+    if (gArgs.GetBoolArg("-upgradewallet", false))
4062+    {

kallewoof commented at 9:47 am on May 9, 2018:

Nit: { on same line as if (..)

achow101 commented at 4:46 pm on May 9, 2018:

Fixed

in src/wallet/wallet.cpp:4068 in 8db939a7b9 outdated

4063+        LOCK(walletInstance->cs_wallet);
4064+
4065+        // Do not upgrade versions to any version between HD_SPLIT and FEATURE_PRE_SPLIT_KEYPOOL unless already supporting HD_SPLIT
4066+        int nMaxVersion = walletInstance->nWalletVersion;
4067+        if (!walletInstance->CanSupportFeature(FEATURE_HD_SPLIT) && nMaxVersion >=FEATURE_HD_SPLIT && nMaxVersion < FEATURE_PRE_SPLIT_KEYPOOL) {
4068+            InitError(_("Cannot upgrade a non HD split wallet without upgrading to support pre split keypool"));

kallewoof commented at 9:48 am on May 9, 2018:

This error would scare me (since I’m juggling privkeys and it’s not a very easy to understand message). Maybe be nice and do [...]split keypool. To do this, run [XYZ] first. or similar?

achow101 commented at 4:47 pm on May 9, 2018:

I edited the error to say what to do instead.

in src/wallet/wallet.cpp:4066 in 8db939a7b9 outdated

4061+    if (gArgs.GetBoolArg("-upgradewallet", false))
4062+    {
4063+        LOCK(walletInstance->cs_wallet);
4064+
4065+        // Do not upgrade versions to any version between HD_SPLIT and FEATURE_PRE_SPLIT_KEYPOOL unless already supporting HD_SPLIT
4066+        int nMaxVersion = walletInstance->nWalletVersion;

kallewoof commented at 9:50 am on May 9, 2018:

μ: Should try to avoid the type prefix camel case style in new code (max_version?).

achow101 commented at 4:47 pm on May 9, 2018:

Fixed

kallewoof commented at 9:53 am on May 9, 2018: member

utACK 8db939a7b953335c5014147dd0d85069b606e0fb with some μ-nits

achow101 force-pushed on May 9, 2018

in src/wallet/wallet.cpp:4038 in dfeabfb47b outdated

4033+            LogPrintf("Upgrading wallet to HD\n");
4034+            walletInstance->SetMinVersion(FEATURE_HD);
4035+
4036+            // generate a new master key
4037+            CPubKey masterPubKey = walletInstance->GenerateNewHDMasterKey();
4038+            if (!walletInstance->SetHDMasterKey(masterPubKey))

sipa commented at 10:53 pm on May 9, 2018:

Style nit: braces for if spanning multiple lines.

achow101 commented at 2:26 am on May 10, 2018:

Fixed.

in src/wallet/wallet.h:148 in d4707852c1 outdated

141@@ -141,9 +142,18 @@ class CKeyPool
142                    (this will be the case for any wallet before the HD chain split version) */
143                 fInternal = false;
144             }
145+            try {
146+                READWRITE(m_pre_split);

sipa commented at 11:06 pm on May 9, 2018:

You can test for this using !(ser_action.ForRead() && s.empty()) instead of trying/catching.

achow101 commented at 2:25 am on May 10, 2018:

I don’t quite understand what you mean.

Also, I would like to keep this consistent with the block above which does the same thing.

jonasschnelli commented at 10:16 am on May 12, 2018:

I think sipa meant to check during read if there are more bytes in the stream because this changed only appends data at the end.

sipa commented at 6:36 pm on May 12, 2018:

Indeed, what @jonasschnelli says. If it’s how similar already operates we can change it later in one go, too.

achow101 force-pushed on May 10, 2018

in src/wallet/rpcwallet.cpp:4105 in ae810ff486 outdated

4100+            "                             If false, addresses (including change addresses if the wallet already had HD Chain Split enabled) from the existing\n"
4101+            "                             keypool will be used until it has been depleted.\n"
4102+            "2. \"seed\"               (string, optional) The WIF private key to use as the new HD seed; if not provided a random seed will be used.\n"
4103+            "                             The seed value can be retrieved using the dumpwallet command. It is the private key marked hdmaster=1\n"
4104+            "\nResult:\n"
4105+            "null                    (boolean) Returns null if successful\n"

kallewoof commented at 6:29 am on May 10, 2018:

Just leave the result section out – e.g. like in setlabel (L326).

achow101 commented at 5:15 pm on May 12, 2018:

Done

kallewoof commented at 6:30 am on May 10, 2018: member

re-utACK ae810ff48650a07160877199f9283edfda1b3632

jonasschnelli added this to the milestone 0.15.2 on May 12, 2018

in src/keystore.cpp:203 in ae810ff486 outdated

194@@ -195,3 +195,10 @@ CKeyID GetKeyForDestination(const CKeyStore& store, const CTxDestination& dest)
195     }
196     return CKeyID();
197 }
198+
199+bool HaveKey(const CKeyStore& store, const CKey& key)
200+{
201+    CKey key2;
202+    key2.Set(key.begin(), key.end(), !key.IsCompressed());
203+    return store.HaveKey(key.GetPubKey().GetID()) || store.HaveKey(key2.GetPubKey().GetID());

jonasschnelli commented at 10:11 am on May 12, 2018:

Does this have performance impacts? What are the downside of not checking the non-compressed (or compressed if non-compressed)?

achow101 commented at 5:13 pm on May 12, 2018:

I don’t think there are performance impacts, or if there are, they are not noticeable. The reason to check for both is to avoid having a seed value which was already in the wallet before and thus could have been revealed.

sipa commented at 8:47 pm on May 12, 2018:

If there’s a noticable performance impact (it should perhaps be 50-100 microseconds extra), we can create a function to simultaneously compute the compressed and uncompressed pubkey for a secret. Can be done later, and it’s probably not necessarily.

in src/wallet/rpcwallet.cpp:4113 in ae810ff486 outdated

4110+            + HelpExampleRpc("sethdseed", "true, \"wifkey\"")
4111+            );
4112+    }
4113+
4114+    if (IsInitialBlockDownload()) {
4115+        throw JSONRPCError(RPC_CLIENT_IN_INITIAL_DOWNLOAD, "Cannot set a new HD seed while still in Initial Block Download");

jonasschnelli commented at 10:13 am on May 12, 2018:

Can you elaborate why a IBD check is necessary and why the wallet-key-state is connected to IBD?

achow101 commented at 5:12 pm on May 12, 2018:

From Matt’s earlier comments:

Separately, can we either check that we’re fully synced before allowing an HD master rotate or keep around old HD keys for key derivation? I’d prefer the second, but its obviously a ton more complicated, so just ensuring that we’re at least synced first is likely sufficient to ensure people dont rotate and miss some payments.

jonasschnelli commented at 10:17 am on May 12, 2018: contributor

Concept ACK, will test more later.

Add 'sethdseed' RPC to initialize or replace HD seed b5ba01a187

Test sethdseed 2bcf2b52ae

Allow -upgradewallet to upgradewallets to HD

Changes the maximum upgradewallet version to the latest wallet version
number, 159900. Non-HD wallets will be upgraded to use HD derivation.
Non HD chain split wallets will be upgraded to HD chain split.

If a non-HD wallet is upgraded to HD, the keypool will be entirely
regenerated.

Since upgradewallet is effectively run during a first run, all of the
first run initial setup stuff is combined with the upgrade to HD

5c50e93d52

Use a keypool of presplit keys after upgrading to hd chain split

After upgrading to HD chain split, we want to continue to use keys
from the old keypool. To do this, before we generate any new keys after
upgrading, we mark all of the keypool entries as being pre-chain
split and move them to a separate pre chain split keypool. Keys are
fetched from that keypool until it is emptied. Only then are the new
internal and external keypools used.

dfcd9f3e6a

Bump wallet version for pre split keypool

Bump the wallet version to indicate support for the pre split keypool.
Also prevents any wallets from upgrading to versions between HD_SPLIT
and PRE_SPLIT_KEYPOOL.

a8da482a8b

achow101 force-pushed on May 12, 2018

sipa commented at 9:08 pm on May 12, 2018: member

utACK a8da482a8bc87ff26194612727d4a7b86b2fb60d

MarcoFalke commented at 2:51 pm on May 13, 2018: member

@jonasschnelli You added this to the 15.2 milestone without the “needs backport” label. Was this done in mistake? I think a backport should be discussed shortly on irc

laanwj commented at 9:15 am on May 14, 2018: member

utACK a8da482a8bc87ff26194612727d4a7b86b2fb60d (hope this works, the github unicorn is preventing me from going to this PR while logged in)

laanwj merged this on May 14, 2018

laanwj closed this on May 14, 2018

laanwj referenced this in commit e03c0db08f on May 14, 2018

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

sipa added the label Needs release notes on Aug 14, 2018

fanquake removed the label Needs release note on Mar 20, 2019

meshcollider referenced this in commit 0475a2378b on Nov 16, 2021

sidhujag referenced this in commit 52cf54f1e4 on Nov 16, 2021

DrahtBot locked this on Dec 16, 2021

[wallet] Upgrade path for non-HD wallets to HD #12560