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

pull luke-jr wants to merge 4 commits into bitcoin:master from luke-jr:bugfix_dumpwallet_hdkeypath changing 2 files +27 −14
  1. 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)

  2. fanquake added the label RPC/REST/ZMQ on Nov 30, 2017
  3. fanquake added the label Wallet on Nov 30, 2017
  4. 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 12:40 AM on December 1, 2017:

    In this case how about:

    CKeyMetadata 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.

  5. 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 12:42 AM on December 1, 2017:

    Nit, partial unrelated change to what commit stands for.

  6. promag commented at 12:44 AM on December 1, 2017: member

    Update test? Squash 2nd and 3rd commits?

  7. 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)

  8. 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?

  9. miccomservices approved
  10. 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.

  11. luke-jr commented at 3:13 PM on January 30, 2018: member

    #11803 (review)

  12. luke-jr force-pushed on Mar 31, 2018
  13. luke-jr commented at 7:38 PM on March 31, 2018: member

    Rebased

  14. maflcko added the label Needs rebase on Jun 6, 2018
  15. luke-jr force-pushed on Nov 4, 2018
  16. luke-jr force-pushed on Nov 4, 2018
  17. DrahtBot removed the label Needs rebase on Nov 4, 2018
  18. bitcoin deleted a comment on Nov 5, 2018
  19. 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.

  20. 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?

  21. 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.

  22. DrahtBot commented at 2:46 PM on December 28, 2018: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    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.

  23. DrahtBot added the label Needs rebase on Feb 15, 2019
  24. Sjors commented at 3:06 PM on February 15, 2019: member

    This has been fixed as part of #14021 (not just for importmulti): <img width="1099" alt="schermafbeelding 2019-02-15 om 16 03 23" src="https://user-images.githubusercontent.com/10217/52864874-75298e80-313b-11e9-97f4-8630694c1fa7.png">

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

  25. luke-jr closed this on Feb 15, 2019
  26. 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.

  27. luke-jr reopened this on Feb 16, 2019
  28. luke-jr force-pushed on Apr 6, 2019
  29. luke-jr commented at 6:39 AM on April 6, 2019: member

    Rebased

  30. Sjors commented at 4:22 PM on August 15, 2019: member

    Calling resident key metadata expert @achow101...

  31. 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.

  32. achow101 commented at 4:45 PM on August 15, 2019: member

    Concept ACK

  33. RPC/Wallet: dumpwallet: Lookup key metadata only once 3e7277784d
  34. Bugfix: RPC/Wallet: dumpwallet should include hdkeypath metadata outside of comment 97aabcfb6c
  35. Bugfix: RPC/Wallet: dumpwallet should include hdmasterkeyid metadata e1488eb5f8
  36. RPC/Wallet: dumpwallet: Replace hdmasterkeyid with hdseedid 563dda4b52
  37. luke-jr force-pushed on Mar 29, 2020
  38. DrahtBot removed the label Needs rebase on Mar 29, 2020
  39. 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

  40. 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:

    1. hdseedid added for reserve=1 key type
    -cPUphC3iKWVx5HVcYimYKjvL1xn6uSHS2HKZdoLoKM1RzQkihNiD 2021-07-22T14:38:59Z reserve=1 # addr=tb1qp7m7nzhp9c4z7069m9rvrpp6tk6g4hqpj07t9l hdkeypath=m/0'/0'/8'

+cPUphC3iKWVx5HVcYimYKjvL1xn6uSHS2HKZdoLoKM1RzQkihNiD 2021-07-22T14:38:59Z reserve=1 hdkeypath=m/0'/0'/8' hdseedid=b01cf0b04e2799dab0a3658d7b59791a02c5ec86 # addr=tb1qp7m7nzhp9c4z7069m9rvrpp6tk6g4hqpj07t9l
    1. No change in hd master key
    cUEM9SuAS9z5z2bdYJhiwsmtCsq1UAwkP4nEv8i3BGK2SANwP51N 2021-07-22T14:38:59Z hdseed=1 # addr=tb1qsmkv2qs609vhhrt95wcd4xf8f6c0q89sag6fm2
    1. No change in script=1 key type
    00143802740190cd3720db8d1f06f4a32f6bafc7ffad 0 script=1 # addr=2MsFRrsqi61GSNEiQKwKUFGNmzbzAevUqE7
  41. 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?

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

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

    git fetch core
git fetch core pull/11803/merge:pr11803
git switch pr11803
./autogen.sh
./configure -q --without-gui
make clean > /dev/null
make -j 4
python3 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?

  42. 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?
  43. Rspigler commented at 7:03 PM on August 3, 2021: contributor

    Work with descriptors

    Maybe I was thinking of listdescriptors

  44. DrahtBot commented at 6:16 PM on November 10, 2021: contributor

    <!--cf906140f33d8803c4a75a2196329ecb-->

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

    <sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>

  45. DrahtBot added the label Needs rebase on Nov 10, 2021
  46. DrahtBot commented at 12:18 PM on February 22, 2022: contributor

    <!--13523179cfe9479db18ec6c5d236f789-->There hasn't been much activity lately and the patch still needs rebase. What is the status here?

    • 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.
  47. 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.

  48. achow101 closed this on Oct 12, 2022
  49. bitcoin locked this on Oct 12, 2023
Contributors
luke-jrpromagjonasschnellimiccomserviceslaanwjmeshcolliderSjorsDrahtBotachow101ghostNelsonGaldemanRspigler
Labels
WalletRPC/REST/ZMQNeeds rebase
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-29 09:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me