Skip to content
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

Allow m prefix in derivation paths #2684

Merged
merged 1 commit into from
Apr 17, 2024

Conversation

tcharding
Copy link
Member

This PR contains a single patch that was created using cargo cherry-pick 830c1e9c - i.e., it is a backport of #2677.


Recently in #2451 we disallowed bip32 derivation paths with the leading 'm' variable.

There is some confusion as to what exactly the bip specifies however Bitcoin Core RPC call getaddressinfo returns a derivation path with a leading "m/". This means we need to be able to parse it irrespective of what the bip says.

Be more liberal in what we accept as a derivation path, including both with and without the leading 'm/'.

Leave the full investigation of the bip to a later date.

Change back some of the test strings as makes sense and include test strings to showcase the full current behaviour.

Recently in rust-bitcoin#2451 we disallowed bip32 derivation paths with the leading
'm' variable.

There is some confusion as to what exactly the bip specifies however
Bitcoin Core RPC call `getaddressinfo` returns a derivation path with a
leading "m/". This means we need to be able to parse it irrespective of
what the bip says.

Be more liberal in what we accept as a derivation path, including both
with and without the leading 'm/'.

Leave the full investigation of the bip to a later date.

Change back some of the test strings as makes sense and include test
strings to showcase the full current behaviour.
@github-actions github-actions bot added C-bitcoin PRs modifying the bitcoin crate test labels Apr 15, 2024
@coveralls
Copy link

Pull Request Test Coverage Report for Build 8695039328

Details

  • 37 of 37 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.006%) to 83.123%

Totals Coverage Status
Change from base Build 8652460139: 0.006%
Covered Lines: 19193
Relevant Lines: 23090

💛 - Coveralls

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK b7f61e2 confirmed that diff exactly matches 2451. CI failure is unrelated.

@tcharding
Copy link
Member Author

Just confirming, you are ok to merge this with the red kani job, right?

@apoelstra
Copy link
Member

Yep.

I was gonna wait for a second ACK, but I think since we have 2 ACKs on master and I don't anticipate any opposition to the "one ACK carveout for backports" #2627 I think I'll just go for it.

@apoelstra apoelstra merged commit 5134756 into rust-bitcoin:0.32.x Apr 17, 2024
167 of 168 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bitcoin PRs modifying the bitcoin crate test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants