Skip to content

Allow m prefix in derivation paths#2684

Merged
apoelstra merged 1 commit intorust-bitcoin:0.32.xfrom
tcharding:04-16-backport-2677
Apr 17, 2024
Merged

Allow m prefix in derivation paths#2684
apoelstra merged 1 commit intorust-bitcoin:0.32.xfrom
tcharding:04-16-backport-2677

Conversation

@tcharding
Copy link
Copy Markdown
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
Copy Markdown

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

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

@apoelstra
Copy link
Copy Markdown
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
@tcharding tcharding deleted the 04-16-backport-2677 branch October 24, 2024 03:59
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.

3 participants