Add 'sethdseed' RPC to initialize or replace HD seed. #11085

dooglus · 2017-08-18T08:49:31Z

As mentioned in #11070, I wanted to be able to use HD addresses in my legacy wallet.

Is a change along these lines acceptable? What needs changing?

I'm aware it's missing tests. I'll add those if there's a chance this could be merged. If not, my wallet now uses HD addresses, so that's good enough for me.

jonasschnelli

Not entirely sure if we should allow mixed wallets (IMO multiwallet would provide a more sane path to run HD and nonHD side a side).

I could imagine that sethdseed is available and marked as expert feature. It's one of the commands that has the possibility to footgun and may results in lost funds (==careful concept-thinking required)

jonasschnelli · 2017-08-18T09:13:30Z

src/wallet/wallet.cpp


+CPubKey CWallet::GenerateNewHDMasterKey(CKey key)


I think this should be called DeriveNewMasterHDKey()

Good point.

jonasschnelli · 2017-08-18T09:16:02Z

src/wallet/rpcwallet.cpp

+
+        masterPubKey = pwallet->GenerateNewHDMasterKey(key);
+    } else
+        masterPubKey = pwallet->GenerateNewHDMasterKey();


What does prevent from setting a new seed in an existing HD wallet? Or do we want to allow that? If so, we should at least warn in the help text or even add an additional boolean to make sure the user is aware of the implications (hopefully).

It allows you to replace your existing HD seed with a new one, for example if you are worried that your old seed was exposed to a third party, you can switch to a new one without having to lose transaction history.

But shouldn't that worry (compromised seed key or compromised sub-key) lead to moving the funds to a new seed? It may hurts privacy is done in a single N-to-1.

Yes, you would want to move funds from keys you suspect are compromised. The user is responsible for deciding how and when to move the funds. This pull request simply makes it possible to switch to a new uncompromised set of addresses.

jonasschnelli · 2017-08-18T09:17:25Z

src/wallet/rpcwallet.cpp

+        throw JSONRPCError(RPC_WALLET_ERROR, "Storing master key failed");
+
+    if (fFlushKeyPool)
+        pwallet->NewKeyPool();


Not flushing the pool may lead to unrecoverable funds. If one sets an HD seed without flushing, backup directly, he may miss -keypoolsize keys (maybe 100, maybe 1000).
I'd say a flush is mandatory.

I don't understand. How do old un-flushed keys in the mempool cause loss of funds if the HD seed is changed but not if it isn't? They're either backed up or they're not.

Your right. It would only be a problem if the user considers only backing up the seed (user may thinks I just need to backup the 64 char hex string) at this point.

If he only backs up the seed, he loses all his funds in pre-HD addresses whether we flush the keypool or not.

We default to flushing the keypool. If a user chooses not to flush the pool I think he likely understands what he's doing.

dooglus · 2017-08-18T09:28:55Z

IMO multiwallet would provide a more sane path to run HD and nonHD side a side

I wasn't aware of 'multiwallet' until I merged my changes to the master branch, and still haven't looked into it in any detail. If it allows me to see my pre-HD and post-HD transactions in a single unified list then I agree.

achow101 · 2017-08-19T00:03:49Z

This RPC should probably also bump the version number to at 139900 so that HD chain split is supported as well.

TheBlueMatt · 2017-08-21T01:17:56Z

@jonasschnelli hmm, multiwallet has a rather significant usability burden (eg not being able to spend from both sides) vs just allowing people to add an HD master key to their wallet. I'm not sure why we wouldn't allow people to upgrade their wallet using the same -upgradewallet path.

Independantly, I do think we need the option to allow someone to rotate their HD master key if they desire (with appropriate backup warnings).

TheBlueMatt · 2017-08-21T01:21:08Z

src/wallet/rpcwallet.cpp

+    if (request.fHelp || request.params.size() > 2) {
+        throw std::runtime_error(
+            "sethdseed ( \"flushkeypool\" \"seed\" )\n"
+            "\nSet or reset the HD wallet seed.\n"


Needs way more documentation here (note it keeps old keys, you probably want to backup immediately after using this and before otherwise using your wallet, etc).

dooglus · 2017-08-21T20:19:52Z

@TheBlueMatt > I'm not sure why we wouldn't allow people to upgrade their wallet using the same -upgradewallet path.

I don't think -upgradewallet should invalidate old backups, whereas setting the HD wallet seed does invalidate old backups if it flushes the keypool, which it does do by default. Perhaps that's an argument for keeping the seed-setting code separate from -upgradewallet, especially since a user may want to change his HD seed after setting it.

It would make sense to have -upgradewallet set an HD seed if it's not already set, but not flush the keypool. In that way the old backup isn't invalidated, and the user's old strategy of "make new backup every 100 transactions" remains safe.

TheBlueMatt · 2017-08-21T20:52:05Z

@dooglus hmm, well if we dont want to use the same system (we could, of course, call the argument something different for this upgrade), then we've horribly fucked up (we used a wallet version number for HD support, so if we dont allow upgrade using the same version number stuff then we're left without any good options to add any other future features, eg segwit support).

I'm fine with, eg, refusing to upgrade past HD unless you use -upgradetohd, and also it seems perfectly reasonable to not flush keypool with the HD upgrade (why tie them together? The user can flush separately if they want).

I do think we need the ability to rotate the HD key before we add any other features past HD (cause we shouldnt force users into HD unless they can still use it as before by rotating their HD seed), so thanks for this.

promag

Missing tests for errors and success case.

promag · 2017-08-21T21:44:03Z

src/wallet/rpcwallet.cpp

+        fFlushKeyPool = request.params[0].get_bool();
+
+    CPubKey masterPubKey;
+    if (request.params.size() > 1) {


if (!request.params[1].isNull()) {

promag · 2017-08-21T21:44:49Z

src/wallet/rpcwallet.cpp

+    EnsureWalletIsUnlocked(pwallet);
+
+    bool fFlushKeyPool = true;
+    if (request.params.size() > 0)


if (!request.params[0].isNull()) {

promag · 2017-08-21T21:45:15Z

src/wallet/rpcwallet.cpp

+    LOCK2(cs_main, pwallet->cs_wallet);
+    EnsureWalletIsUnlocked(pwallet);
+
+    bool fFlushKeyPool = true;


bool flush_key_pool = true;

promag · 2017-08-21T21:45:55Z

src/wallet/rpcwallet.cpp

+    if (request.params.size() > 1) {
+        CKey key;
+        std::string strKey = AccountFromValue(request.params[1]);
+        if (strKey.length() != 64 || !IsHex(strKey))


Missing { }.

promag · 2017-08-21T21:46:18Z

src/wallet/rpcwallet.cpp

+        if (strKey.length() != 64 || !IsHex(strKey))
+            throw JSONRPCError(RPC_INVALID_PARAMETER, "Key should be 64 hex characters");
+
+        std::vector <unsigned char> vch = ParseHex(strKey);


Remove space before <.

promag · 2017-08-21T21:48:04Z

src/wallet/rpcwallet.cpp

+    if (!pwallet->SetHDMasterKey(masterPubKey))
+        throw JSONRPCError(RPC_WALLET_ERROR, "Storing master key failed");
+
+    if (fFlushKeyPool)


Missing { }.

promag · 2017-08-21T21:49:10Z

src/wallet/wallet.cpp


+CPubKey CWallet::DeriveNewMasterHDKey(CKey key)


const CKey& key?

promag · 2017-08-21T21:50:21Z

src/wallet/rpcwallet.cpp

@@ -3132,6 +3132,66 @@ UniValue generate(const JSONRPCRequest& request)
    return generateBlocks(coinbase_script, num_generate, max_tries, true);
 }

+UniValue sethdseed(const JSONRPCRequest& request)
+{
+    CWallet * const pwallet = GetWalletForJSONRPCRequest(request);


Remove space before *.

promag · 2017-08-21T21:54:31Z

src/wallet/rpcwallet.cpp

+    if (request.params.size() > 0)
+        fFlushKeyPool = request.params[0].get_bool();
+
+    CPubKey masterPubKey;


master_pub_key?

promag · 2017-08-21T21:59:39Z

src/wallet/rpcwallet.cpp

+        key.Set(vch.begin(), vch.end(), true);
+
+        if (pwallet->HaveKey(key.GetPubKey().GetID()))
+            throw JSONRPCError(RPC_WALLET_ERROR, "Key already exists in wallet");


Similar error in https://github.com/bitcoin/bitcoin/blame/master/src/wallet/rpcdump.cpp#L825.

TheBlueMatt · 2017-09-05T22:26:13Z

@dooglus any plans on addressing @promag's comments? The ability to rotate the HD master key is a very, very nice change which I'd like to see much sooner rather than later to resolve some outstanding HD stuff by upgrading people to HD aggressively.

dooglus · 2017-09-06T15:52:10Z

@TheBlueMatt Sure, I'll get right on that. Hopefully today.

I wasn't previously aware of the developer notes but now I am.

promag · 2017-09-06T16:12:50Z

Needs rebase too.

achow101

Concept ACK. Comments inline.

This also needs rebase, more documentation, and tests.

I'm not sure if it is necessary or useful to be able to set your own seed. Or if it is necessary or useful to have the option to not regenerate the keypool from the seed.

achow101 · 2017-09-06T18:37:15Z

src/wallet/rpcwallet.cpp

+    CPubKey masterPubKey;
+    if (request.params.size() > 1) {
+        CKey key;
+        std::string strKey = AccountFromValue(request.params[1]);


Why are you doing AccountFromValue? There are no accounts here.

achow101 · 2017-09-06T18:55:14Z

src/wallet/rpcwallet.cpp

+    if (request.params.size() > 1) {
+        CKey key;
+        std::string strKey = AccountFromValue(request.params[1]);
+        if (strKey.length() != 64 || !IsHex(strKey))


Why not use a WIF key instead of a hex seed?

Good question. I wanted to import the HD seed from the wallet in which I had been testing HD derivation into my main wallet, and happened to have it in hex format.

I'll switch to requiring a WIF key.

dooglus · 2017-09-07T16:44:10Z

Addressed @promag's formatting nits. Removed trailing period from commit message. Rebased. Squashed.

Didn't add tests or backup warnings yet.

dooglus · 2017-09-07T16:56:31Z

@achow101

I'm not sure if it is necessary or useful to be able to set your own seed.

I want to be able to make a backup of my wallet on paper, by writing down nothing but my seed.

In order to restore from the paper backup I need to be able to set my own seed.

For this use case we would also need a 'dumpprivkey' for the master seed.

Or if it is necessary or useful to have the option to not regenerate the keypool from the seed.

It's possible the user makes regular weekly backups of his wallet. If we regenerate the keypool, he's at risk of loss of funds until the next weekly backup is made. If we give him the option of not regenerating the keypool, his previous backup will remain valid until the next backup is made. It's also possible he makes a backup immediately, but it is somehow corrupted. If we allow him to not regenerate his keypool then he has the possibility of recovering funds using older backups. If we insist he regenerates his keypool, we're forcing him to rely entirely on the new backup he made, for no good reason.

dooglus · 2017-09-07T16:59:10Z

TODO:

add tests
add backup warnings
use WIF keys, not hex ones

dooglus · 2017-09-07T20:54:58Z

Now the code accepts WIF keys, not hex, and I added a warning to the doc string saying that the user needs to make a new backup.

Should I add tests to the existing test/functional/wallet-hd.py script?

meshcollider · 2017-09-09T09:06:03Z

utACK ddad8fc, awaiting tests

~~This still wouldn't offer support for the 512-bit seeds generated with BIP 39 though, maybe that could be considered~~

Travis error is unrelated and was fixed in #11271

@dooglus Should I add tests to the existing test/functional/wallet-hd.py script?

I think that would make the most sense since this is directly manipulating HD wallets, yep

TheBlueMatt

In-line comments are mostly nits, but we need some thing to keep people from putting an HD key in their wallet without a walletupgrade here, probably making SetHDMasterKey fail unless wallet version supports HD. In the future we may have an upgrade function which this could use (with appropriate documentation).

TheBlueMatt · 2017-09-11T15:16:06Z

src/wallet/rpcwallet.cpp

+            "\nNote that you will need to MAKE A NEW BACKUP of your wallet after setting the HD wallet seed.\n"
+            + HelpRequiringPassphrase(pwallet) +
+            "\nArguments:\n"
+            "1. \"flushkeypool\"       (boolean, optional, default=true) Whether to flush old unused addresses from the keypool.\n"


Maybe clarify a bit further what this implies? (eg "ie the next address from getnewaddress will be from this new seed. If this is not set, the old seed's derived addresses will be used until the keypool has been depleted.").

So:

"1. \"flushkeypool\" (boolean, optional, default=true) Whether to flush old unused addresses from the keypool.\n" " If true, the next address from getnewaddress will be from this new seed.\n" " If false, addresses from the existing keypool will be used until it has been depleted.\n"

Note that the keypool may not contain addresses derived from an old seed, and may contain pre-HD random addresses.

TheBlueMatt · 2017-09-11T15:30:43Z

src/wallet/rpcwallet.cpp

+        // check that we don't already have this key, compressed or otherwise
+        key2.Set(key.begin(), key.end(), !key.IsCompressed());
+        if (pwallet->HaveKey(key.GetPubKey().GetID()) || pwallet->HaveKey(key2.GetPubKey().GetID())) {
+            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Already have this key");


nit: maybe "(either as an HD seed or as a loose private key)" to note that you cant use an existing loose private key as an HD seed.

In this comment, @promag said that there was a similar error elsewhere. I think he meant that I should use the same string so as not to unnecessarily increase the required translation effort. I'll switch to using your string.

sipa · 2017-09-11T22:42:43Z

@dooglus Can you rebase? There were some unrelated Travis that affected your PR, but have been fixed in master since.

dooglus · 2017-09-12T02:48:09Z

Rebased.

dooglus · 2017-09-12T20:49:42Z

@TheBlueMatt > we need some thing to keep people from putting an HD key in their wallet without a walletupgrade here, probably making SetHDMasterKey fail unless wallet version supports HD

I'm not familiar with walletupgrade. I made my wallet in 2011 and never (to my knowledge) ran walletupgrade. I was able to use this PR to add an HD key to my 2011 wallet and it seems to work. What am I missing?

TheBlueMatt · 2017-09-14T15:23:47Z

@dooglus I think it may work, but a) it wont use hd-split (which it should, if possible) and b) old versions opening the wallet which has had HD key added to it will (I believe) open normally and add non-HD keys instead of failing to open, which doing a -walletupgrade will ensure.

achow101 · 2018-01-22T23:47:40Z

Needs rebase

Wasn't the plan to have this (or something similar) in for 0.16 since creating non-HD wallets is now a thing?

achow101 · 2018-02-27T02:10:15Z

@dooglus Are you still working on this?

dooglus · 2018-02-27T07:27:50Z

No I'm not. I don't think I have what it takes to get this merged.

achow101 · 2018-02-27T17:46:29Z

I've picked this up in #12560

dooglus · 2018-02-27T18:08:53Z

Thank you.

achow101 · 2018-03-14T18:11:34Z

This should be closed as it is replaced by #12560

dooglus · 2018-03-14T19:56:11Z

OK.

a8da482 Bump wallet version for pre split keypool (Andrew Chow) dfcd9f3 Use a keypool of presplit keys after upgrading to hd chain split (Andrew Chow) 5c50e93 Allow -upgradewallet to upgradewallets to HD (Andrew Chow) 2bcf2b5 Test sethdseed (Andrew Chow) b5ba01a Add 'sethdseed' RPC to initialize or replace HD seed (Chris Moore) dd3c07a Separate HaveKey function that checks whether a key is in a keystore (Andrew Chow) Pull request description: 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. Tree-SHA512: e56c792e150590429ac4a1061e8d6f7b20cca06366e184eb9bbade4cd6ae82699a28fe84f87031eadba97ad2c1606517a105f00fb7b45779c979243020071adb

jonasschnelli suggested changes Aug 18, 2017

View reviewed changes

jonasschnelli added the Wallet label Aug 18, 2017

TheBlueMatt reviewed Aug 21, 2017

View reviewed changes

promag reviewed Aug 21, 2017

View reviewed changes

achow101 reviewed Sep 6, 2017

View reviewed changes

dooglus force-pushed the set_hd_seed branch from 81c1d0c to b0f418b Compare September 7, 2017 16:42

dooglus force-pushed the set_hd_seed branch from b0f418b to 11ab0e6 Compare September 7, 2017 16:50

dooglus force-pushed the set_hd_seed branch from 11ab0e6 to ddad8fc Compare September 7, 2017 20:03

TheBlueMatt reviewed Sep 11, 2017

View reviewed changes

dooglus force-pushed the set_hd_seed branch from ddad8fc to e0cb3a3 Compare September 12, 2017 02:47

Add 'sethdseed' RPC to initialize or replace HD seed

4a565cb

dooglus force-pushed the set_hd_seed branch from e0cb3a3 to 4a565cb Compare September 12, 2017 21:01

dooglus mentioned this pull request Sep 12, 2017

Attempting to create new non-HD wallet ignores -usehd=0 without warning #11313

Closed

TheBlueMatt mentioned this pull request Sep 29, 2017

[Wallet] add option for a custom extended master privat key (xpriv) #8735

Closed

TheBlueMatt mentioned this pull request Nov 19, 2017

WIP [RPC] [Wallet] walletdowngrade command (which can remove HD) #11730

Closed

3 tasks

achow101 mentioned this pull request Feb 27, 2018

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

Merged

dooglus closed this Mar 14, 2018

HashUnlimited mentioned this pull request Sep 7, 2018

[wallet] Upgrade path for non-HD wallets to HD chaincoin/chaincoin#323

Merged

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Add 'sethdseed' RPC to initialize or replace HD seed. #11085

Add 'sethdseed' RPC to initialize or replace HD seed. #11085

Uh oh!

Conversation

dooglus commented Aug 18, 2017

Uh oh!

jonasschnelli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dooglus commented Aug 18, 2017

Uh oh!

achow101 commented Aug 19, 2017

Uh oh!

TheBlueMatt commented Aug 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dooglus commented Aug 21, 2017

Uh oh!

TheBlueMatt commented Aug 21, 2017

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Sep 5, 2017

Uh oh!

dooglus commented Sep 6, 2017

Uh oh!

promag commented Sep 6, 2017

Uh oh!

achow101 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

meshcollider commented Sep 9, 2017 •

edited

Loading