Closes #11243
Adds "Requires a new wallet backup" text to addwitnessaddress, importprivkey, importmulti, importaddress, importpubkey, and addmultisigaddress. Also adds a warning to dumpwallet that backing up the seed alone is not sufficient to back up non-HD addresses
61 | @@ -62,7 +62,7 @@ std::string DecodeDumpString(const std::string &str) { 62 | for (unsigned int pos = 0; pos < str.length(); pos++) { 63 | unsigned char c = str[pos]; 64 | if (c == '%' && pos+2 < str.length()) { 65 | - c = (((str[pos+1]>>6)*9+((str[pos+1]-'0')&15)) << 4) | 66 | + c = (((str[pos+1]>>6)*9+((str[pos+1]-'0')&15)) << 4) |
Unrelated whitespace change.
I mentioned that in the PR, is it an issue?
I isn't really a problem, but you should try to avoid making such changes. Atom can be set to not make any whitespace changes at all.
It is a problem, as it often breaks backporting cherry-picks.
Fixed.
643 | @@ -644,7 +644,7 @@ UniValue dumpwallet(const JSONRPCRequest& request) 644 | file << strprintf("# mined on %s\n", EncodeDumpTime(chainActive.Tip()->GetBlockTime())); 645 | file << "\n"; 646 | 647 | - // add the base58check encoded extended master if the wallet uses HD 648 | + // add the base58check encoded extended master if the wallet uses HD
Unrelated whitespace change.
Concept ACK
Mind to add to dumpwallet that just backing up the private masterkey is likely insufficient?
Done :+1:
Rebased to fix .bitcoin directory travis failure
utACK 288dfaea9d987399b92514121f98a674e74d5580
600 | @@ -601,6 +601,7 @@ UniValue dumpwallet(const JSONRPCRequest& request) 601 | throw std::runtime_error( 602 | "dumpwallet \"filename\"\n" 603 | "\nDumps all wallet keys in a human-readable format.\n" 604 | + "Note that if your wallet contains keys which are not derived from your HD seed (e.g. imported keys), these are not covered by only backing up the seed itself, and must be backed up too.\n"
This leaves me confused. The things I need to back up are in dumpwallet, just not the master key listed in dump wallet, right? So just the dumpwallet file is a sufficient backup (with rescan, including imported segwit addresses and p2sh redeemScripts?), but not only the master key?
I've changed it, lmk what you think now
I suppose for completeness importwallet may need the (rather astoundingly obvious) "Requires a new backup" note.
I guess we dont want to note anything about keypool topup leading to needing backups in things like "getnewaddress" (for non-HD wallets)? What about explicit ones like "keypoolrefill"? And things that always wipe keypool like encryptwallet? I guess probably not?
Similarly, I suppose we dont care about listing things like move/sendfrom which change wallet fileds for F$#King accounts?
600 | @@ -601,6 +601,7 @@ UniValue dumpwallet(const JSONRPCRequest& request)
601 | throw std::runtime_error(
602 | "dumpwallet \"filename\"\n"
603 | "\nDumps all wallet keys in a human-readable format.\n"
@TheBlueMatt would changing this string to something like Dumps all wallet derived keys in a human-readable format. help cleanup the confusion?
Rebased + hopefully clarified the text as per @TheBlueMatt comments
Wait, now I'm even more confused. AFAICT, dumpwallet does dump all imported keys, not just "derived" keys, so no need to back those up. It does not, however, dump imported scripts, so witness addresses and multisig addresses will be missed, right?
That's what I meant by (e.g. ensure you back up the whole dumpfile) (note that I didn't follow the suggested edit by mess110 because it's wrong, is that what you were looking at?). Are you suggesting mentioning that those imported scripts aren't backed up, or?
Hmm, maybe I missed the parenthetical. Do we want to mention anything about the segwit addresses and imported scripts?
The description does currently specifically refer to 'keys' not scripts or segwit addresses. But is there any reason scripts aren't also dumped too?
The description does currently specifically refer to 'keys' not scripts or segwit addresses. But is there any reason scripts aren't also dumped too?
No. just that (AFAIK) no one implemented that yet. Ideally a dumpwallet would not lose any relevant information.
I dont see why scripts shouldnt be dumped either. I'm not sure if its really in scope for this PR, but would be a welcome addition IMO.
@laanwj @TheBlueMatt I'll look into it and try get a PR open for that soon too then
Rebased
600 | @@ -601,6 +601,7 @@ UniValue dumpwallet(const JSONRPCRequest& request) 601 | throw std::runtime_error( 602 | "dumpwallet \"filename\"\n" 603 | "\nDumps all wallet keys in a human-readable format to a server-side file. This does not allow overwriting existing files.\n" 604 | + "Note that if your wallet contains keys which are not derived from your HD seed (e.g. imported keys), these are not covered by only backing up the seed itself, and must be backed up too (e.g. ensure you back up the whole dumpfile).\n"
Note that you may want to wrap this line.
utACK a38bfbc51d0b98fffd8d79457f31c6fb196ff580. Would like to see the dumpwallet-includes-imported-scripts followup.
I've wrapped the text, and added a note about scripts not being included, until I get the other PR up. Will remove that note in the other PR when it happens.
1109 | @@ -1110,7 +1110,7 @@ UniValue addmultisigaddress(const JSONRPCRequest& request) 1110 | if (request.fHelp || request.params.size() < 2 || request.params.size() > 3) 1111 | { 1112 | std::string msg = "addmultisigaddress nrequired [\"key\",...] ( \"account\" )\n" 1113 | - "\nAdd a nrequired-to-sign multisignature address to the wallet.\n" 1114 | + "\nAdd a nrequired-to-sign multisignature address to the wallet. Requires a new wallet backup.\n"
"nrequired-to-sign multisignature"
Only comment, I'm assuming this isn't a typo, and is a reference to the number of required signatures needed for the multisig address?
Correct :) And note that that text is not changed in this PR, it was there before
Ok sweet, yeah I saw that it wasn't a change from this commit, but wasn't sure so figured I'd ask. Thanks! Is it typically frowned upon to comment on things not directly changed by the PR/Commit/Issue in question?
No, if something looks wrong then feel free to point it out :) I was just clarifying
Awesome, thanks again.