Remove m prefix requirement by josibake · Pull Request #2451 · rust-bitcoin/rust-bitcoin

josibake · 2024-02-05T19:34:01Z

m in BIP0032 is a variable, not a constant. Requiring it as a constant here is confusing and can lead to erroneous conclusions if using this library as a means of understanding BIP0032.

Fixes #2449

coveralls · 2024-02-05T19:35:05Z

Pull Request Test Coverage Report for Build 7813971326

-1 of 53 (98.11%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.005%) to 84.155%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
bitcoin/src/bip32.rs	51	52	98.08%

Totals
Change from base Build 7800933584:	-0.005%
Covered Lines:	19343
Relevant Lines:	22985

💛 - Coveralls

Kixunil

ACK except formatting.

Kixunil · 2024-02-05T20:55:47Z

bitcoin/src/bip32.rs

Could you please use write! instead? I know it's less performant but this will mess up formatting so I'd rather not support formatting at all. (I'm not against adding formatting support later but I think it's out of scope of this PR)/

Still getting used to rust so pardon my ignorance, but this shouldn't be changing anything. Previously, the code was writing m directly to the formatter and then calling fmt::Display::fmt(cn, f)? in a loop. My understanding is fmt::Display::fmt(cn, f) is being called so that the ChildNumber types get printed with the hardened notation.

Since we no longer start the strings with m my change is consuming the first element from the iterator (which is a ChildNumber) and calling fmt::Display::fmt so that the first element is properly displayed with the hardened notation before proceeding with the rest of the elements.

I'm probably misunderstanding your point, but AFAICT this is keeping the behavior for displaying derivation paths the same as before.

Formatter can have options that are passed from the format string (e.g. {:42} pads the content to 42 chars). By passing formatter directly you keep the format options so if the user writes {:5} Instead of 1/2 your code will output 1/ 2.

My suggestion removes formatting options so they won't work at all which I think is better than having it broken. We can improve it later.

The old code was also wrong :).

The issue is that if you try to format!("{:20}", path) say, the existing code (and your modification) will apply the :20 to each of the child numbers, instead of applying it to the whole path.

Gotcha! Thanks for the explanation @Kixunil @apoelstra , will update to use @Kixunil 's suggestion.

tcharding · 2024-02-06T00:10:48Z

Any reason not to make the "m/" optional, would save changing all the unit tests and "m/0/1" reads pretty clearly?

Kixunil · 2024-02-06T10:27:27Z

@tcharding it was just wrong originally.

josibake · 2024-02-06T10:48:13Z

Regarding the CI failures, it looks like all of them are due to running an old version of the ecdsa-psbt example. Any recommendations on how to go about fixing this?

apoelstra · 2024-02-06T13:55:15Z

@tcharding it was just wrong originally.

FWIW I think the BIP is pretty confusingly written; you need to find the words "To shorten notation" in the middle of the body in order to realize that the unit tests shouldn't be taken literally? I guess a PR would be welcome.

josibake · 2024-02-06T17:29:54Z

Updated to use write! per @Kixunil 's suggestion. Spent some time looking at the tests to see if there's an obvious way to avoid the failures with the old versions of the ecdsa-psbt example but nothing stood out to me. Would appreciate any suggestions or pointers to documentation on how the CI tests are setup

Kixunil

You need to squash the commits since we require all commits to pass CI.

Kixunil · 2024-02-06T21:22:10Z

Behold, CI failing with a shitty error that nobody knows where it comes from - this is why I'm pushing for adding information to errors.

tcharding · 2024-02-06T22:12:37Z

a shitty error

Its shitty yes because it can come from two code paths in the same function, but I believe our current error strategy, if applied to the bip32 module, would fix this.

that nobody knows where it comes from

It only comes from a single function in the whole codebase, so this is probably a bit hyperbolic :)

tcharding · 2024-02-06T22:15:28Z

Spent some time looking at the tests to see if there's an obvious way to avoid the failures with the old versions of the ecdsa-psbt example

I just removed the "m/" from let path = "m/84h/0h/0h".into_derivation_path()?; and the example runs.

Kixunil · 2024-02-07T07:29:53Z

Its shitty yes because it can come from two code paths in the same function

Actually I have no idea which input it failed on

I believe our current error strategy, if applied to the bip32 module, would fix this.

Yes.

It only comes from a single function in the whole codebase, so this is probably a bit hyperbolic

I couldn't find the place where m/ is used but sure, I didn't look crazy hard.

In BIP0032, m is used as a variable for the root extended key. It is not meant to be used as a constant prefix when serializing paths. Update the DerivationPath parser to no longer require the m prefix. Remove the m prefix from the unit tests and the bip32, ecdsa-psbt, and taproot-psbt examples. close rust-bitcoin#2449

josibake · 2024-02-07T11:34:53Z

You need to squash the commits since we require all commits to pass CI.

Ah, I see. cargo test was passing locally on the first commit, which is why I deferred the example changes to a separate commit, but the CI was running the examples on the first commit as well. When looking at the CI output, I saw an --edition=2021 flag being passed and incorrectly assumed this was being used to run an old version of the example with a new version of the code, but turns out I was overthinking it 😅

I just removed the "m/" from let path = "m/84h/0h/0h".into_derivation_path()?; and the example runs.

Looks like I also missed a few "m/"! I removed the remaining "m/"'s from the ecdsa-psbt example and also from the taproot-psbt example.

Squashed per @Kixunil 's suggestion, removed the remaining "m/"'s per @tcharding . Thanks for the review!

Kixunil

ACK ccbd09d

Kixunil · 2024-02-07T17:24:49Z

bitcoin/src/bip32.rs

+        for cn in iter {
            f.write_str("/")?;
-            fmt::Display::fmt(cn, f)?;
+            write!(f, "{}", cn)?;


write!(f, "/{}", cn)?; would've been simpler but not a big deal.

apoelstra

ACK ccbd09d

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.

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.

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

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.

b7f61e2 Allow m prefix in derivation paths (Tobin C. Harding) Pull request description: This PR contains a single patch that was created using `cargo cherry-pick 830c1e9` - 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. ACKs for top commit: apoelstra: ACK b7f61e2 confirmed that diff exactly matches 2451. CI failure is unrelated. Tree-SHA512: 075e8a7d368dd216f947954459f67812b248982b5e022ac158e1022d8adcb7ce603f63951385fb88c397a66305e1cb3a25c89fdf4726bddc1fa1e2d190950f41

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 bot added C-bitcoin PRs modifying the bitcoin crate test labels Feb 5, 2024

josibake mentioned this pull request Feb 5, 2024

Don't require "m" when creating derivation path strings #2449

Closed

josibake force-pushed the remove-m-prefix-requirement branch from 671d98c to 8a79457 Compare February 5, 2024 19:55

Kixunil reviewed Feb 5, 2024

View reviewed changes

josibake force-pushed the remove-m-prefix-requirement branch from 8a79457 to 3d5896e Compare February 6, 2024 17:26

Kixunil reviewed Feb 6, 2024

View reviewed changes

josibake force-pushed the remove-m-prefix-requirement branch from 3d5896e to 6446681 Compare February 7, 2024 11:04

josibake force-pushed the remove-m-prefix-requirement branch from 6446681 to ccbd09d Compare February 7, 2024 11:19

Kixunil approved these changes Feb 7, 2024

View reviewed changes

apoelstra approved these changes Feb 7, 2024

View reviewed changes

apoelstra merged commit 94c6526 into rust-bitcoin:master Feb 7, 2024

This was referenced Apr 8, 2024

Usage of "m/" in derivation path #2671

Closed

bip32: Understand and fix the m/ problem #2673

Open

tcharding mentioned this pull request Apr 8, 2024

Revert "Remove unnecessary m/ prefix requirement" #2674

Closed

tcharding mentioned this pull request Apr 10, 2024

Allow m prefix in derivation paths #2677

Merged

tcharding mentioned this pull request Apr 15, 2024

Allow m prefix in derivation paths #2684

Merged

Kixunil mentioned this pull request Jun 9, 2024

chore(deps): upgrade rust-bitcoin to 0.32.0 bitcoindevkit/rust-hwi#99

Merged

3 tasks

Conversation

josibake commented Feb 5, 2024

Uh oh!

coveralls commented Feb 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 7813971326

💛 - Coveralls

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

Kixunil Feb 5, 2024

Choose a reason for hiding this comment

Uh oh!

josibake Feb 6, 2024

Choose a reason for hiding this comment

Uh oh!

Kixunil Feb 6, 2024

Choose a reason for hiding this comment

Uh oh!

apoelstra Feb 6, 2024

Choose a reason for hiding this comment

Uh oh!

josibake Feb 6, 2024

Choose a reason for hiding this comment

Uh oh!

tcharding commented Feb 6, 2024

Uh oh!

Kixunil commented Feb 6, 2024

Uh oh!

josibake commented Feb 6, 2024

Uh oh!

apoelstra commented Feb 6, 2024

Uh oh!

josibake commented Feb 6, 2024

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

Kixunil commented Feb 6, 2024

Uh oh!

tcharding commented Feb 6, 2024

Uh oh!

tcharding commented Feb 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kixunil commented Feb 7, 2024

Uh oh!

josibake commented Feb 7, 2024

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

Kixunil Feb 7, 2024

Choose a reason for hiding this comment

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coveralls commented Feb 5, 2024 •

edited

Loading

tcharding commented Feb 6, 2024 •

edited

Loading