Revert "Remove unnecessary m/ prefix requirement"#2674
Closed
tcharding wants to merge 2 commits intorust-bitcoin:masterfrom
Closed
Revert "Remove unnecessary m/ prefix requirement"#2674tcharding wants to merge 2 commits intorust-bitcoin:masterfrom
tcharding wants to merge 2 commits intorust-bitcoin:masterfrom
Conversation
This reverts commit ccbd09d. There were merits to the change in rust-bitcoin#2451, and the bip is somewhat unclear on the matter, however the change breaks our ability to parse output from Bitcoin Core RPC call `getaddressinfo`. For the release to progress we need to revert this change.
Pull Request Test Coverage Report for Build 8608077559Details
💛 - Coveralls |
Member
|
CI failure is real. |
In the integration test of the new PSBT signing API the derivation path strings do not have a leading 'm/'. The PR that introduced the new API must have overlapped with the one that removed the prefix from the `bip32` modules `DerivationPath` string parsing because when we reverted that change these paths were not included.
Member
Author
|
Ha, your too quick today. Fixed now. |
Member
Author
|
I had a go at explaining why patch 2 is needed, its a guess though. |
Member
|
I don't think it's correct that the first part must be |
Member
Author
I agree but was just going to revert for this release and fix it in the next one (hence #2673). Would you prefer to see it fixed this release? |
Member
I would prefer that we be able to parse with or without a |
Member
Author
|
Replaced by #2677 |
apoelstra
added a commit
that referenced
this pull request
Apr 15, 2024
830c1e9 Allow m prefix in derivation paths (Tobin C. Harding) Pull request description: 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. This PR replaces #2674. ACKs for top commit: apoelstra: ACK 830c1e9 sanket1729: ACK 830c1e9 junderw: ACK 830c1e9 Tree-SHA512: 7a4fccd49cb8cd91a6c8db51d758ae116d9d2e98fead7b87520ca302022b37ddbcf3f85453941c5f336f8e934ad224beba99527dc29ce8368fbb1f25508c1615
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This reverts commit ccbd09d.
There were merits to the change in #2451, and the bip is somewhat unclear on the matter, however the change breaks our ability to parse output from Bitcoin Core RPC call
getaddressinfo.For the release to progress we need to revert this change.