Fix typo in deprecated type #2693

matthiasdebernardini · 2024-04-17T16:58:19Z

In #2258 we attempted to add back in deprecated BIP-32 types - but we spelled the identifier incorrectly. The patch was then backported to the 0.31.x branch in December but was only just noticed now.

Fix typo in deprecated type from Extendend -> Extended.

coveralls · 2024-04-17T17:00:30Z

Pull Request Test Coverage Report for Build 8904697452

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 83.131%

Totals
Change from base Build 8904174578:	0.0%
Covered Lines:	19195
Relevant Lines:	23090

💛 - Coveralls

sanket1729 · 2024-04-17T17:12:00Z

bitcoin/src/bip32.rs

@@ -31,11 +31,11 @@ const VERSION_BYTES_TESTNETS_PRIVATE: [u8; 4] = [0x04, 0x35, 0x83, 0x94];

 /// The old name for xpub, extended public key.
 #[deprecated(since = "0.31.0", note = "use xpub instead")]
-pub type ExtendendPubKey = Xpub;
+pub type ExtendedPubKey = Xpub;


Interesting, we should just delete this code. The type was named ExtendedPubKey in 0.30 and we removed ExtendedPubKey completely in 0.31 by accidentally mis-spelling it.

Yes, I also agree. Note that with the CI testing, it took a long time for someone to find this typo.

Agreed :(. We messed up the deprecation cycle here. Frustrating that nobody noticed until now -- it would have made the 0.30 to 0.31 transition much easier.

Maybe this PR should instead be a backport to the 0.31 branch.

Also the deprecation method should say Xpub instead of xpub, which will make it clear that there's an actual type called Xpub.

Note that with the CI testing, it took a long time for someone to find this typo.

I'm not sure that tests would have caught it. Tobin and I updated a bunch of crates from 0.30 to 0.31, and I recall seeing that ExtendedPubKey no longer compiled and thinking "hmm don't I remember us doing a deprecation here?" and then deciding that there must've been some reason we didn't.

@tcharding @apoelstra the deprecation warning does not show up unless you explicitly wrap the create a new type. Simply pub type A = B does an alias and would not show warning when user uses type A.

Perhaps we should do as you suggested once in the past @apoelstra and during RC testing I should do each upgrade as two separate patches, the first being required changes and the second fixing all the lints (deprecation). That way we may notice if something is in the first patch that shouldn't have been.

Agreed, though if Sanket is correct, we won't actually get a deprecation warning in this case :(.

I swapped Xpriv for ExtendendPrivKey on master (with the spelling mistake) and got a warning

cargo run --example taproot-psbt --features=rand-std,bitcoinconsensus warning: /home/tobin/build/github.com/tcharding/rust-bitcoin/master/bitcoin/Cargo.toml: version requirement `0.105.0+25.1` for dependency `bitcoinconsensus` includes semver metadata which will be ignored, removing the metadata is recommended to avoid confusion Compiling bitcoin v0.32.0-rc1 (/home/tobin/build/github.com/tcharding/rust-bitcoin/master/bitcoin) warning: use of deprecated type alias `bitcoin::bip32::ExtendendPrivKey`: use xpriv instead --> bitcoin/examples/taproot-psbt.rs:81:77 | 81 | use bitcoin::bip32::{ChildNumber, DerivationPath, Fingerprint, Xpriv, Xpub, ExtendendPrivKey}; | ^^^^^^^^^^^^^^^^ | = note: `#[warn(deprecated)]` on by default warning: use of deprecated type alias `bitcoin::bip32::ExtendendPrivKey`: use xpriv instead --> bitcoin/examples/taproot-psbt.rs:117:9 | 117 | ExtendendPrivKey::from_str(BENEFACTOR_XPRIV_STR)?, | ^^^^^^^^^^^^^^^^

Thanks for checking. For some reason, I recall this not working.

No sweat, there are various deprecation things that definitely do not work as they appear, I can not remember which ones work and which don't either.

tcharding · 2024-04-17T22:07:27Z

Maybe this PR should instead be a backport to the 0.31 branch.

Agreed, added label - I'll handle the backport.

tcharding · 2024-04-17T22:15:57Z

Hi @matthiasdebernardini, thanks for noticing our fail, and thanks for fixing it! This patch will need to be backported to the 0.31.x branch, I took the liberty to improve the PR title and description - could you improve the commit with something similar please then I'll backport the patch exactly as it is merged into master.

Thanks again!

bitcoin/src/bip32.rs

storopoli

ACK 6f0c5c1

tcharding · 2024-04-20T00:53:10Z

Can you squash down to a single commit please.

matthiasdebernardini · 2024-04-29T16:00:24Z

Let me know if there is anything else @tcharding !

tcharding · 2024-04-29T21:34:35Z

Need to fix the commit message please.

matthiasdebernardini · 2024-04-30T18:28:40Z

Got it! Should be good now.

tcharding · 2024-04-30T20:57:05Z

Still needs a brief message followed by the body. You can see this blog for how to write a commit message: https://chris.beams.io/posts/git-commit/

In PR rust-bitcoin#2258, deprecated BIP-32 types were re-added but contained a typo in the identifier: "Extendend" instead of "Extended". This commit fixes that typo. The incorrect patch was backported to the 0.31.x branch in December but only noticed recently.

matthiasdebernardini · 2024-05-01T02:58:43Z

Thank you for your guidance, I think this commit message is good now. Let me know otherwise.

tcharding

ACK 12411fc

tcharding · 2024-05-01T03:13:34Z

Thanks for sticking with it! Grumpy old devs like me like commit messages to be limited to 72 characters wide ... but I let this one through :)

Thanks for the contribution man! Sorry to be such a fussy reviewer.

storopoli

ACK 12411fc

apoelstra

ACK 12411fc

matthiasdebernardini · 2024-05-01T14:51:29Z

@tcharding I think that's a positive - I look forward to learning more and making better contributions in the future. Thanks again!

tcharding · 2024-05-01T22:06:46Z

Awesome, glad to have your contributions! We have a ton of work that needs doing, some easy so not so much :)

github-actions bot added the C-bitcoin PRs modifying the bitcoin crate label Apr 17, 2024

sanket1729 reviewed Apr 17, 2024

View reviewed changes

tcharding added the port-0.31.x Needs porting to 0.31.x branch label Apr 17, 2024

tcharding changed the title ~~fixed typo~~ Fix typo in deprecated type Apr 17, 2024

matthiasdebernardini force-pushed the master branch from fda3690 to d78afe1 Compare April 18, 2024 00:30

storopoli suggested changes Apr 18, 2024

View reviewed changes

bitcoin/src/bip32.rs Outdated Show resolved Hide resolved

bitcoin/src/bip32.rs Outdated Show resolved Hide resolved

storopoli approved these changes Apr 19, 2024

View reviewed changes

matthiasdebernardini force-pushed the master branch from 6f0c5c1 to 1ce08ac Compare April 29, 2024 15:59

matthiasdebernardini force-pushed the master branch from 1ce08ac to adbbc9b Compare April 30, 2024 18:27

matthiasdebernardini force-pushed the master branch from adbbc9b to 12411fc Compare May 1, 2024 02:57

tcharding approved these changes May 1, 2024

View reviewed changes

storopoli approved these changes May 1, 2024

View reviewed changes

apoelstra approved these changes May 1, 2024

View reviewed changes

apoelstra merged commit ad212da into rust-bitcoin:master May 1, 2024
21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix typo in deprecated type #2693

Fix typo in deprecated type #2693

matthiasdebernardini commented Apr 17, 2024 •

edited by tcharding

coveralls commented Apr 17, 2024 •

edited

sanket1729 Apr 17, 2024

storopoli Apr 17, 2024 •

edited

apoelstra Apr 17, 2024

apoelstra Apr 17, 2024

sanket1729 Apr 17, 2024

tcharding Apr 17, 2024

apoelstra Apr 18, 2024

tcharding Apr 18, 2024

sanket1729 Apr 18, 2024

tcharding Apr 18, 2024

tcharding commented Apr 17, 2024

tcharding commented Apr 17, 2024

storopoli left a comment

tcharding commented Apr 20, 2024

matthiasdebernardini commented Apr 29, 2024

tcharding commented Apr 29, 2024

matthiasdebernardini commented Apr 30, 2024

tcharding commented Apr 30, 2024

matthiasdebernardini commented May 1, 2024

tcharding left a comment

tcharding commented May 1, 2024

storopoli left a comment

apoelstra left a comment

matthiasdebernardini commented May 1, 2024

tcharding commented May 1, 2024

Fix typo in deprecated type #2693

Fix typo in deprecated type #2693

Conversation

matthiasdebernardini commented Apr 17, 2024 • edited by tcharding

coveralls commented Apr 17, 2024 • edited

Pull Request Test Coverage Report for Build 8904697452

Details

💛 - Coveralls

Choose a reason for hiding this comment

storopoli Apr 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding commented Apr 17, 2024

tcharding commented Apr 17, 2024

storopoli left a comment

Choose a reason for hiding this comment

tcharding commented Apr 20, 2024

matthiasdebernardini commented Apr 29, 2024

tcharding commented Apr 29, 2024

matthiasdebernardini commented Apr 30, 2024

tcharding commented Apr 30, 2024

matthiasdebernardini commented May 1, 2024

tcharding left a comment

Choose a reason for hiding this comment

tcharding commented May 1, 2024

storopoli left a comment

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

matthiasdebernardini commented May 1, 2024

tcharding commented May 1, 2024

matthiasdebernardini commented Apr 17, 2024 •

edited by tcharding

coveralls commented Apr 17, 2024 •

edited

storopoli Apr 17, 2024 •

edited