Minimally-invasive separation of bitcoin keys from ECDSA signature types by dr-orlovsky · Pull Request #757 · rust-bitcoin/rust-bitcoin

dr-orlovsky · 2022-01-08T13:10:18Z

This PR tries to do a minimally-invazive separation of signature- and key-related types, previously mixed in a single util::ecdsa module.

Rationale: bitcoin key types are not specific for signature algorithm. See discussion at #588.

This PR became possible after we moved on new secp256k1 version exposing XonlyPublicKey type, since now all key types may co-exist in a single module under different names

The PR goal is achieved through

Renaming ecdsa mod into private ec module such that the code is not copied and diff size is small;
Introducing dummy ecdsa mod back in the next commit and re-exporiting only signature types from internal ec mod in it;
Re-exporting all key types under key module, removing previous depreciation message for bitcoin keys.

Kixunil · 2022-01-08T14:12:32Z

Concept ACK. Given how trivial the ecdsa mod is It think this would be better without eedab33955840527cee1f05b4e9cc7e4ec674c6c

Was also thinking of moving things outside of util while you're at it but maybe it's too unrelated.

src/util/schnorr.rs

src/util/ec.rs

sanket1729

As mentioned in the review comment, we did not really deprecate bitcoin::PublicKey and bitcoin::PrivateKey. I think we can get away with nuking ecdsa::PublicKey and ecdsa::SecretKey.

And I think renaming and moving things can be left to #751 (which I have not reviewed, but might be a better place to organize things in the library).

I think we can limit the PR to removing the ecdsa::PublicKey and ecdsa::PrivateKey and leave the organization and signatures and keys to a longer-term project.

We probably want key stuff in util::key and leave only the signature stuff in ecdsa.rs. I am also okay with punting on this till 0.29 as I don't think this is critical for taproot release.

sanket1729 · 2022-01-09T01:11:38Z

src/lib.rs

FYI, for the future, this warning does not really work. If you want to deprecate a type, you would have to have to

Rename the existing type

Create a new type pub type OldName = NewName

Add deprecated above it

dr-orlovsky · 2022-01-09T06:52:29Z

I've redone this PR trying to follow your suggestions. Now it effectively moves keys from ecdsa mod to key mod - and it is done in a way that you can check that the code has not changed by reviewing on a per-commit basis.

src/util/schnorr.rs

apoelstra · 2022-01-09T15:05:12Z

ACK 6ecc8809112ef17c428c24eefaf300f7f84d86b1 except for Kixunil's comment about deprecation.

dr-orlovsky · 2022-01-09T19:41:20Z

@apoelstra updated, needs re-ACK

sanket1729

ACK 38fdb159b458d4ccc344f19eb64d0613670380d8

… of bitcoin ECDSA cf0c48c Improve Debug for PrivateKey (Dr Maxim Orlovsky) b65a6ae Test for extended private key keypair generation f5875a (Dr Maxim Orlovsky) e6a3d60 BIP32 extended key `to_ecdsa()` and `to_schnorr()` methods (Dr Maxim Orlovsky) b72f56c BIP32 extended keys are using Scep256k1 keys instead of bitcoin ECDSA (Dr Maxim Orlovsky) Pull request description: This is third step required to introduce Schnorr key support according to #588. This PR starts API-breaking changes and is follow-up to non-API breaking #589, which is already merged. PR rationale: BIP32 does not support uncompressed keys and using type with compression flag was a mistake ACKs for top commit: apoelstra: ACK cf0c48c sanket1729: ACK cf0c48c. #757 might need rework after this Tree-SHA512: 6356a65004e7517256bacbf9aaeb69a22fd8536b341e567c5c4e819288e1105d083fe12ac0641404c407c97acf039bdc525f8e02b1b594a6cdda90106f3b1bdc

Kixunil

ACK 38fdb159b458d4ccc344f19eb64d0613670380d8

dr-orlovsky · 2022-01-10T08:17:10Z

Had to re-base after the latest merges; @sanket1729 @Kixunil @apoelstra pls re-ACK

apoelstra

ACK 0d5d0f349c3c80d364aa3908b4e7c22b52a355a6

sanket1729 · 2022-01-11T07:20:29Z

Now this one also has conflicts

dr-orlovsky · 2022-01-11T12:15:59Z

Rebased, another re-ACK required, sorry, @Kixunil @apoelstra @sanket1729

apoelstra

re-ACK 406f4c403a04d54daee2448ba7e1de3421c1782a

Took me a moment to figure out why you removed the PSBT changes, but it is because PSBT now uses rust-secp key types and is immune to API changes for rust-bitcoin key types.

dr-orlovsky · 2022-01-11T15:56:08Z

@apoelstra which is good, isn't it? :D

Kixunil

ACK 406f4c403a04d54daee2448ba7e1de3421c1782a

dr-orlovsky · 2022-01-13T16:26:24Z

Rebased after latest merges. @apoelstra @Kixunil @sanket1729 please re-ACK.

This will complete Taproot todo list and open a way to get first RC

apoelstra

ACK 05f4d0792e5a5fc52cfb191e801b2fcec192c811

Kixunil

ACK 05f4d0792e5a5fc52cfb191e801b2fcec192c811

This commit tries to achieve separation of signature- and key-related types, previously mixed in a single ECDSA module. Rationale: bitcoin key types are not specific for signature algorithm. This is achieved through - Remove key mod with its content moved to ecdsa mod - Re-export keys under key module in util mod - to make git generate diff for the rename of ecdsa mod in the next commit correctly.

dr-orlovsky · 2022-01-14T08:46:12Z

Sorry guys, I know you are tired of this - but while we were-reACKing it it had to be rebased once again :( Hopefully for the last time.

apoelstra

ACK 8a993e8

sanket1729

utACK 8a993e8

dr-orlovsky added API break This PR requires a version bump for the next release code quality Makes code easier to understand and less likely to lead to problems trivial Obvious, easy and quick to review (few lines or doc-only...) labels Jan 8, 2022

dr-orlovsky added this to the 0.28.0 milestone Jan 8, 2022

This was referenced Jan 8, 2022

Taproot keys implementation strategy #588

Closed

Tracking issue for Taproot #503

Closed

apoelstra reviewed Jan 8, 2022

View reviewed changes

src/util/schnorr.rs Outdated Show resolved Hide resolved

apoelstra reviewed Jan 8, 2022

View reviewed changes

src/util/ec.rs Outdated Show resolved Hide resolved

sanket1729 reviewed Jan 9, 2022

View reviewed changes

Kixunil reviewed Jan 9, 2022

View reviewed changes

src/util/schnorr.rs Outdated Show resolved Hide resolved

sanket1729 previously approved these changes Jan 9, 2022

View reviewed changes

sanket1729 mentioned this pull request Jan 9, 2022

Taproot: BIP32 extended keys using Scep256k1 keys instead of bitcoin ECDSA #590

Merged

Kixunil previously approved these changes Jan 9, 2022

View reviewed changes

dr-orlovsky dismissed stale reviews from Kixunil and sanket1729 via 0d5d0f3 January 10, 2022 08:16

apoelstra previously approved these changes Jan 10, 2022

View reviewed changes

dr-orlovsky mentioned this pull request Jan 11, 2022

Rename inner key field in PrivateKey and PublicKey #762

Merged

dr-orlovsky dismissed apoelstra’s stale review via 406f4c4 January 11, 2022 12:15

dr-orlovsky requested review from Kixunil, apoelstra and sanket1729 January 11, 2022 12:16

apoelstra previously approved these changes Jan 11, 2022

View reviewed changes

Kixunil previously approved these changes Jan 13, 2022

View reviewed changes

dr-orlovsky dismissed stale reviews from Kixunil and apoelstra via 560b8ea January 13, 2022 16:22

apoelstra previously approved these changes Jan 13, 2022

View reviewed changes

Kixunil previously approved these changes Jan 13, 2022

View reviewed changes

dr-orlovsky added 5 commits January 14, 2022 01:35

Re-export all key types under util::key. Deprecate other exports.

2d9de78

Re-org keys and ecdsa mods - pt.2

d1c2213

Re-org keys and ecdsa mods - pt.3

bcb8932

Properly deprecate util::ecdsa key re-exports

8a993e8

dr-orlovsky dismissed stale reviews from Kixunil and apoelstra via 8a993e8 January 14, 2022 08:45

dr-orlovsky requested review from Kixunil and apoelstra January 14, 2022 08:46

apoelstra approved these changes Jan 14, 2022

View reviewed changes

sanket1729 approved these changes Jan 15, 2022

View reviewed changes

sanket1729 merged commit d1f051c into rust-bitcoin:master Jan 15, 2022

dr-orlovsky mentioned this pull request Jan 15, 2022

Bump version to v0.28 #785

Closed

Conversation

dr-orlovsky commented Jan 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kixunil commented Jan 8, 2022

Uh oh!

Uh oh!

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

sanket1729 Jan 9, 2022

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky commented Jan 9, 2022

Uh oh!

Uh oh!

apoelstra commented Jan 9, 2022

Uh oh!

dr-orlovsky commented Jan 9, 2022

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky commented Jan 10, 2022

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

sanket1729 commented Jan 11, 2022

Uh oh!

dr-orlovsky commented Jan 11, 2022

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky commented Jan 11, 2022

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky commented Jan 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky commented Jan 14, 2022

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dr-orlovsky commented Jan 8, 2022 •

edited

Loading

dr-orlovsky commented Jan 13, 2022 •

edited

Loading