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.
1409@@ -1410,7 +1410,11 @@ CPubKey CWallet::GenerateNewHDMasterKey()
1410 {
1411 CKey key;
1412 key.MakeNewKey(true);
1413+ return GenerateNewHDMasterKey(key);
1414+}
1415
1416+CPubKey CWallet::GenerateNewHDMasterKey(CKey key)
DeriveNewMasterHDKey()
3179+ if (pwallet->HaveKey(key.GetPubKey().GetID()))
3180+ throw JSONRPCError(RPC_WALLET_ERROR, "Key already exists in wallet");
3181+
3182+ masterPubKey = pwallet->GenerateNewHDMasterKey(key);
3183+ } else
3184+ masterPubKey = pwallet->GenerateNewHDMasterKey();
3185+
3186+ if (!pwallet->SetHDMasterKey(masterPubKey))
3187+ throw JSONRPCError(RPC_WALLET_ERROR, "Storing master key failed");
3188+
3189+ if (fFlushKeyPool)
3190+ pwallet->NewKeyPool();
-keypoolsize keys (maybe 100, maybe 1000).
I’d say a flush is mandatory.
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.
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)
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.
@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).
3141+ }
3142+
3143+ if (request.fHelp || request.params.size() > 2) {
3144+ throw std::runtime_error(
3145+ "sethdseed ( \"flushkeypool\" \"seed\" )\n"
3146+ "\nSet or reset the HD wallet seed.\n"
@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.
@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.
3165+ bool fFlushKeyPool = true;
3166+ if (request.params.size() > 0)
3167+ fFlushKeyPool = request.params[0].get_bool();
3168+
3169+ CPubKey masterPubKey;
3170+ if (request.params.size() > 1) {
0if (!request.params[1].isNull()) {
3161+
3162+ LOCK2(cs_main, pwallet->cs_wallet);
3163+ EnsureWalletIsUnlocked(pwallet);
3164+
3165+ bool fFlushKeyPool = true;
3166+ if (request.params.size() > 0)
0if (!request.params[0].isNull()) {
3160+ }
3161+
3162+ LOCK2(cs_main, pwallet->cs_wallet);
3163+ EnsureWalletIsUnlocked(pwallet);
3164+
3165+ bool fFlushKeyPool = true;
0bool flush_key_pool = true;
3168+
3169+ CPubKey masterPubKey;
3170+ if (request.params.size() > 1) {
3171+ CKey key;
3172+ std::string strKey = AccountFromValue(request.params[1]);
3173+ if (strKey.length() != 64 || !IsHex(strKey))
{ }.
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.
3171+ CKey key;
3172+ std::string strKey = AccountFromValue(request.params[1]);
3173+ if (strKey.length() != 64 || !IsHex(strKey))
3174+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Key should be 64 hex characters");
3175+
3176+ std::vector <unsigned char> vch = ParseHex(strKey);
<.
3174+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Key should be 64 hex characters");
3175+
3176+ std::vector <unsigned char> vch = ParseHex(strKey);
3177+ key.Set(vch.begin(), vch.end(), true);
3178+
3179+ if (pwallet->HaveKey(key.GetPubKey().GetID()))
{ }.
3178+
3179+ if (pwallet->HaveKey(key.GetPubKey().GetID()))
3180+ throw JSONRPCError(RPC_WALLET_ERROR, "Key already exists in wallet");
3181+
3182+ masterPubKey = pwallet->DeriveNewMasterHDKey(key);
3183+ } else
{ }.
3181+
3182+ masterPubKey = pwallet->DeriveNewMasterHDKey(key);
3183+ } else
3184+ masterPubKey = pwallet->GenerateNewHDMasterKey();
3185+
3186+ if (!pwallet->SetHDMasterKey(masterPubKey))
{ }.
3184+ masterPubKey = pwallet->GenerateNewHDMasterKey();
3185+
3186+ if (!pwallet->SetHDMasterKey(masterPubKey))
3187+ throw JSONRPCError(RPC_WALLET_ERROR, "Storing master key failed");
3188+
3189+ if (fFlushKeyPool)
{ }.
1409@@ -1410,7 +1410,11 @@ CPubKey CWallet::GenerateNewHDMasterKey()
1410 {
1411 CKey key;
1412 key.MakeNewKey(true);
1413+ return DeriveNewMasterHDKey(key);
1414+}
1415
1416+CPubKey CWallet::DeriveNewMasterHDKey(CKey key)
const CKey& key?
3131@@ -3132,6 +3132,66 @@ UniValue generate(const JSONRPCRequest& request)
3132 return generateBlocks(coinbase_script, num_generate, max_tries, true);
3133 }
3134
3135+UniValue sethdseed(const JSONRPCRequest& request)
3136+{
3137+ CWallet * const pwallet = GetWalletForJSONRPCRequest(request);
*.
3164+
3165+ bool fFlushKeyPool = true;
3166+ if (request.params.size() > 0)
3167+ fFlushKeyPool = request.params[0].get_bool();
3168+
3169+ CPubKey masterPubKey;
master_pub_key?
3175+
3176+ std::vector <unsigned char> vch = ParseHex(strKey);
3177+ key.Set(vch.begin(), vch.end(), true);
3178+
3179+ if (pwallet->HaveKey(key.GetPubKey().GetID()))
3180+ throw JSONRPCError(RPC_WALLET_ERROR, "Key already exists in wallet");
@TheBlueMatt Sure, I’ll get right on that. Hopefully today.
I wasn’t previously aware of the developer notes but now I am.
3167+ fFlushKeyPool = request.params[0].get_bool();
3168+
3169+ CPubKey masterPubKey;
3170+ if (request.params.size() > 1) {
3171+ CKey key;
3172+ std::string strKey = AccountFromValue(request.params[1]);
AccountFromValue? There are no accounts here.
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.
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.
TODO:
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?
utACK https://github.com/bitcoin/bitcoin/commit/ddad8fc0b6e4046ac1ad752f917f55abbe64199a, 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.pyscript?
I think that would make the most sense since this is directly manipulating HD wallets, yep
3193+ "sethdseed ( \"flushkeypool\" \"seed\" )\n"
3194+ "\nSet or reset the HD wallet seed.\n"
3195+ "\nNote that you will need to MAKE A NEW BACKUP of your wallet after setting the HD wallet seed.\n"
3196+ + HelpRequiringPassphrase(pwallet) +
3197+ "\nArguments:\n"
3198+ "1. \"flushkeypool\" (boolean, optional, default=true) Whether to flush old unused addresses from the keypool.\n"
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.
3223+ if (!key.IsValid()) throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Private key outside allowed range");
3224+
3225+ // check that we don't already have this key, compressed or otherwise
3226+ key2.Set(key.begin(), key.end(), !key.IsCompressed());
3227+ if (pwallet->HaveKey(key.GetPubKey().GetID()) || pwallet->HaveKey(key2.GetPubKey().GetID())) {
3228+ throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Already have this key");
@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?
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?