Bugfix: RPC/Wallet: Include HD key metadata in dumpwallet

luke-jr commented at 8:32 pm on November 30, 2017: member

When restoring a backup, we should restore the wallet metadata. This moves some of it out of “comment” sections so that it can be sensibly handled by parsing tools.

(importwallet doesn’t currently support it, but that’s another matter to address)

fanquake added the label RPC/REST/ZMQ on Nov 30, 2017

fanquake added the label Wallet on Nov 30, 2017

in src/wallet/rpcdump.cpp:817 in 1068d93655 outdated

659@@ -660,19 +660,26 @@ UniValue dumpwallet(const JSONRPCRequest& request)
660         std::string strAddr = CBitcoinAddress(keyid).ToString();
661         CKey key;
662         if (pwallet->GetKey(keyid, key)) {
663+            CKeyMetadata meta;

promag commented at 0:40 am on December 1, 2017:

In this case how about:

0CKeyMetadata meta = pwallet->mapKeyMetadata[keyid];

luke-jr commented at 3:15 am on December 1, 2017:

operator[] can also create map items. Even if this never occurs, it needs to generate code for it.

promag commented at 9:58 am on January 30, 2018:

Right, off topic, but if above was changed to const CWallet * const pwallet then that would be false.

luke-jr commented at 4:21 pm on February 26, 2018:

operator[] isn’t valid at all on const maps AFAIK…?

promag commented at 4:29 pm on February 26, 2018:

Indeed! Sorry for the noise.

luke-jr commented at 11:00 pm on November 3, 2018:

n.b. C++11 added std::map::at, but this isn’t useful since we need to correctly handle the case where there is no metadata.

in src/wallet/rpcdump.cpp:682 in 1068d93655 outdated

679                 file << "inactivehdmaster=1";
680             } else {
681                 file << "change=1";
682             }
683-            file << strprintf(" # addr=%s%s\n", strAddr, (pwallet->mapKeyMetadata[keyid].hdKeypath.size() > 0 ? " hdkeypath="+pwallet->mapKeyMetadata[keyid].hdKeypath : ""));
684+            file << strprintf(" # addr=%s%s\n", strAddr, (meta.hdKeypath.empty() ? "" : (" hdkeypath=" + meta.hdKeypath)));

promag commented at 0:42 am on December 1, 2017:

Nit, partial unrelated change to what commit stands for.

promag commented at 0:44 am on December 1, 2017: member

Update test? Squash 2nd and 3rd commits?

in src/wallet/rpcdump.cpp:787 in b24b1c5224 outdated

680             } else {
681                 file << "change=1";
682             }
683-            file << strprintf(" # addr=%s%s\n", strAddr, (pwallet->mapKeyMetadata[keyid].hdKeypath.size() > 0 ? " hdkeypath="+pwallet->mapKeyMetadata[keyid].hdKeypath : ""));
684+            if (!meta.hdKeypath.empty()) {
685+                file << " hdkeypath=" + meta.hdKeypath;

jonasschnelli commented at 6:15 am on December 1, 2017:

What is the reason to move this out of the comment section?

luke-jr commented at 6:29 am on December 1, 2017:

It’s metadata that should be restored. (our importwallet doesn’t support it, but that’s another matter)

jonasschnelli commented at 6:16 am on December 1, 2017: contributor

@luke-jr: can you please elaborate a little bit what situation this PR does improve?

miccomservices approved

laanwj commented at 9:47 am on January 30, 2018: member

@luke-jr Can you please answer @jonasschnelli ’s question? There is no rationale in the OP at all.

luke-jr commented at 3:13 pm on January 30, 2018: member

#11803 (review)

luke-jr force-pushed on Mar 31, 2018

luke-jr commented at 7:38 pm on March 31, 2018: member

Rebased

maflcko added the label Needs rebase on Jun 6, 2018

luke-jr force-pushed on Nov 4, 2018

DrahtBot removed the label Needs rebase on Nov 4, 2018

bitcoin deleted a comment on Nov 5, 2018

meshcollider commented at 12:30 pm on November 10, 2018: contributor

Tested ACK https://github.com/bitcoin/bitcoin/pull/11803/commits/17d609ce26a36e0b52917f277e2b50135e405649

This isn’t really a bugfix because there is no bug in the codebase which is fixed right? But I agree its a sensible change.

Supporting it with importwallet is definitely something that needs to be done, ideally that should be done at the same time because they’re mirror functions. But this doesn’t break compatibility with the current importwallet so it should be ok to leave it for a future PR.

luke-jr commented at 7:59 pm on November 10, 2018: member

I think it’s a bugfix because without it (and another fix to importwallet), dump/import will produce a wallet missing the key metadata. Isn’t the goal of these RPCs to produce the same wallet?

Sjors commented at 6:36 pm on November 20, 2018: member

Concept ACK and lightly tested 17d609c.

Unrelated, I don’t like that the address column depends on the currently active address type. We could replace it with an inferred combo() descriptor after #14477.

DrahtBot commented at 2:46 pm on December 28, 2018: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23127 (tests: Use test framework utils where possible by vincenzopalazzo)
#22805 (refactor: use CWallet const shared pointers in dump{privkey,wallet} by theStack)

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.

DrahtBot added the label Needs rebase on Feb 15, 2019

Sjors commented at 3:06 pm on February 15, 2019: member

This has been fixed as part of #14021 (not just for importmulti):

It could still be useful to switch over to inferred descriptors, so we also get the origin info.

luke-jr closed this on Feb 15, 2019

Sjors commented at 8:35 am on February 16, 2019: member

Just noticed that it’s a seperate field now, but sill in the comment section, so we could move it to the left.

luke-jr reopened this on Feb 16, 2019

luke-jr force-pushed on Apr 6, 2019

luke-jr commented at 6:39 am on April 6, 2019: member

Rebased

Sjors commented at 4:22 pm on August 15, 2019: member

Calling resident key metadata expert @achow101…

in src/wallet/rpcdump.cpp:837 in 479c0cb374 outdated

859             } else {
860                 file << "change=1";
861             }
862-            file << strprintf(" # addr=%s%s\n", strAddr, (pwallet->mapKeyMetadata[keyid].has_key_origin ? " hdkeypath="+WriteHDKeypath(pwallet->mapKeyMetadata[keyid].key_origin.path) : ""));
863+            if (meta.has_key_origin) {
864+                file << " hdkeypath=" + WriteHDKeypath(meta.key_origin.path);

achow101 commented at 4:41 pm on August 15, 2019:

We should also write out the master fingerprint here too (part of key_origin). I think instead of a separate field for that, we could instead write that in the same way descriptors and psbt do key origin information, i.e. fingerprint instead of m in the path.

achow101 commented at 4:45 pm on August 15, 2019: member

Concept ACK

RPC/Wallet: dumpwallet: Lookup key metadata only once 3e7277784d

Bugfix: RPC/Wallet: dumpwallet should include hdkeypath metadata outside of comment 97aabcfb6c

Bugfix: RPC/Wallet: dumpwallet should include hdmasterkeyid metadata e1488eb5f8

RPC/Wallet: dumpwallet: Replace hdmasterkeyid with hdseedid 563dda4b52

luke-jr force-pushed on Mar 29, 2020

DrahtBot removed the label Needs rebase on Mar 29, 2020

ghost commented at 4:25 am on July 22, 2021: none

Concept ACK

Will try this today. Wallet with metadata is obviously better than wallet missing the key metadata.

This PR will complete 1337 days next week

ghost commented at 3:36 pm on July 22, 2021: none

Compiled and tried in POP!_OS with a new wallet (legacy / testnet). Diff for the dump files: https://ghostbin.com/paste/L2SXs. Observed the below things when comparing dump files for Master and PR branch:

hdseedid added for reserve=1 key type

0-cPUphC3iKWVx5HVcYimYKjvL1xn6uSHS2HKZdoLoKM1RzQkihNiD 2021-07-22T14:38:59Z reserve=1 # addr=tb1qp7m7nzhp9c4z7069m9rvrpp6tk6g4hqpj07t9l hdkeypath=m/0'/0'/8'
1
2+cPUphC3iKWVx5HVcYimYKjvL1xn6uSHS2HKZdoLoKM1RzQkihNiD 2021-07-22T14:38:59Z reserve=1 hdkeypath=m/0'/0'/8' hdseedid=b01cf0b04e2799dab0a3658d7b59791a02c5ec86 # addr=tb1qp7m7nzhp9c4z7069m9rvrpp6tk6g4hqpj07t9l

No change in hd master key

0cUEM9SuAS9z5z2bdYJhiwsmtCsq1UAwkP4nEv8i3BGK2SANwP51N 2021-07-22T14:38:59Z hdseed=1 # addr=tb1qsmkv2qs609vhhrt95wcd4xf8f6c0q89sag6fm2

No change in script=1 key type

000143802740190cd3720db8d1f06f4a32f6bafc7ffad 0 script=1 # addr=2MsFRrsqi61GSNEiQKwKUFGNmzbzAevUqE7

NelsonGaldeman commented at 12:01 pm on July 27, 2021: contributor

I think commits should be squashed. Totally agree with prayank23: Wallet with metadata is obviously better than wallet missing the key metadata

This tests failed on my env though. Is it just me?

0feature_backwards_compatibility.py --legacy-wallet | ✖ Failed  | 1 s
1feature_taproot.py --previous_release              | ✖ Failed  | 1 s
2mempool_compatibility.py                           | ✖ Failed  | 1 s
3rpc_createmultisig.py --legacy-wallet              | ✖ Failed  | 103 s
4rpc_signrawtransaction.py --descriptors            | ✖ Failed  | 105 s
5wallet_upgradewallet.py --legacy-wallet            | ✖ Failed  | 1 s

This is exactly how I fetched the branch, compiled the binnaries and ran the tests.

0git fetch core
1git fetch core pull/11803/merge:pr11803
2git switch pr11803
3./autogen.sh
4./configure -q --without-gui
5make clean > /dev/null
6make -j 4
7python3 test/functional/test_runner.py

Checked my HEAD commit within my local PR copy and I’m getting 8642fbd04ce670cb76657580990d4073e41b0fe1 which differs from the last commit I see on the commit tab here, am I doing something wrong?

Rspigler commented at 5:01 am on July 30, 2021: contributor

Tested on master for legacy wallets:

I get an extended private masterkey

Then a list of (private key, address, hdkeypath); although very out of order. Somewhere in there, is the hdseed

Master for descriptor wallets:

This type of wallet does not support this command (code -4)

Tested on 563dda4b52aca970189b757b7a86615adc442608:

Legacy:

Extended private master key.

Then list of (private key, hdkeypath, hdseedid, address). Changes are address is moved to the end of the list, and addition of hdseedid. The keys are still out of order, and the hdseed is also still randomly in the list.

Descriptor:

Unable to create descriptor wallet (either through the GUI or console), so unable to test.

Possible improvements?:

Work with descriptors
Sort addresses/hdkeypaths by address index
Bring the hdseed to top of list, like the extended private master key is
List the hdseedid once at top of list, instead of repeated for every address?

Rspigler commented at 7:03 pm on August 3, 2021: contributor

Work with descriptors

Maybe I was thinking of listdescriptors

DrahtBot commented at 6:16 pm on November 10, 2021: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

DrahtBot added the label Needs rebase on Nov 10, 2021

DrahtBot commented at 12:18 pm on February 22, 2022: contributor

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

achow101 commented at 5:53 pm on October 12, 2022: member

Closing this as it has not had any activity in a while. If you are interested in continuing work on this, please leave a comment so that it can be reopened.

achow101 closed this on Oct 12, 2022

bitcoin locked this on Oct 12, 2023

Bugfix: RPC/Wallet: Include HD key metadata in dumpwallet #11803

Conflicts