-
Notifications
You must be signed in to change notification settings - Fork 38.7k
ParseHDKeypath: support h as hardened marker #28192
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
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process. ConflictsNo conflicts as of last run. |
|
I'm not sure if there's any software that enforces the consistent usage of a hardened marker. |
|
We could always relax the constraint if someone has a good use case for not being consistent. For now this method is only foreseen to be used with |
|
Seems likely someone somewhere will want to combine two derivation paths, which might use different markers. |
c5b287d to
08a8ae9
Compare
|
I made the consistency check optional and off by default. |
src/util/bip32.h
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Why the extra newline after the doxygen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
08a8ae9 to
9bf2c8a
Compare
Optionally check for inconsistent use.
9bf2c8a to
777e5f4
Compare
BIP32 allows both
'andhas hardened derivation marker. Our legacy wallet uses'. Since #26076 our descriptor wallets usehby default.ParseHDKeypathonly supports'. It's currently only used in the legacy wallet context, so this doesn't cause any problems. But it will once #22341 uses it (to parse the RPCpathargument forgetxpub). Might as well fix it now.I added a restriction for not combining
hand'. Afaik this currently isn't enforced anywhere else in the codebase, including for descriptors, but it seems sane. I've occasionally messed that up in the past.