Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from

Conversation

luke-jr
Copy link
Member

@luke-jr luke-jr commented Nov 30, 2017

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)

Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update test? Squash 2nd and 3rd commits?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, partial unrelated change to what commit stands for.

@jonasschnelli
Copy link
Contributor

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

@laanwj
Copy link
Member

laanwj commented Jan 30, 2018

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

@luke-jr
Copy link
Member Author

luke-jr commented Jan 30, 2018

#11803 (comment)

@luke-jr luke-jr force-pushed the bugfix_dumpwallet_hdkeypath branch from b24b1c5 to 393511c Compare March 31, 2018 19:38
@luke-jr
Copy link
Member Author

luke-jr commented Mar 31, 2018

Rebased

@luke-jr luke-jr force-pushed the bugfix_dumpwallet_hdkeypath branch 2 times, most recently from 73d8c21 to 17d609c Compare November 4, 2018 03:24
@bitcoin bitcoin deleted a comment from ismail120572 Nov 5, 2018
@meshcollider
Copy link
Contributor

Tested ACK 17d609c

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
Copy link
Member Author

luke-jr commented Nov 10, 2018

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
Copy link
Member

Sjors commented Nov 20, 2018

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
Copy link
Contributor

DrahtBot commented Dec 28, 2018

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.

@Sjors
Copy link
Member

Sjors commented Feb 15, 2019

This has been fixed as part of #14021 (not just for importmulti):
schermafbeelding 2019-02-15 om 16 03 23

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

@luke-jr luke-jr closed this Feb 15, 2019
@Sjors
Copy link
Member

Sjors commented Feb 16, 2019

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 luke-jr reopened this Feb 16, 2019
@luke-jr luke-jr force-pushed the bugfix_dumpwallet_hdkeypath branch from 17d609c to 479c0cb Compare April 6, 2019 06:37
@luke-jr
Copy link
Member Author

luke-jr commented Apr 6, 2019

Rebased

@Sjors
Copy link
Member

Sjors commented Aug 15, 2019

Calling resident key metadata expert @achow101...

Copy link
Member

@achow101 achow101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

file << "inactivehdseed=1";
} else {
file << "change=1";
}
file << strprintf(" # addr=%s%s\n", strAddr, (pwallet->mapKeyMetadata[keyid].has_key_origin ? " hdkeypath="+WriteHDKeypath(pwallet->mapKeyMetadata[keyid].key_origin.path) : ""));
if (meta.has_key_origin) {
file << " hdkeypath=" + WriteHDKeypath(meta.key_origin.path);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ghost
Copy link

ghost commented Jul 22, 2021

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
Copy link

ghost commented Jul 22, 2021

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

@NelsonGaldeman
Copy link
Contributor

NelsonGaldeman commented Jul 27, 2021

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 8642fbd which differs from the last commit I see on the commit tab here, am I doing something wrong?

@Rspigler
Copy link
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 563dda4:

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
Copy link
Contributor

Rspigler commented Aug 3, 2021

Work with descriptors

Maybe I was thinking of listdescriptors

@DrahtBot
Copy link
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
Copy link
Contributor

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.

@achow101
Copy link
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 achow101 closed this Oct 12, 2022
@bitcoin bitcoin locked and limited conversation to collaborators Oct 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.