-
Notifications
You must be signed in to change notification settings - Fork 621
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Minimally-invasive separation of bitcoin keys from ECDSA signature types #757
Conversation
Concept ACK. Given how trivial the Was also thinking of moving things outside of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
#[deprecated(since = "0.26.1", note = "Please use `ecdsa::PrivateKey` instead")] | ||
pub use util::ecdsa::PrivateKey; | ||
#[deprecated(since = "0.26.1", note = "Please use `ecdsa::PublicKey` instead")] | ||
pub use util::ecdsa::PublicKey; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
I've redone this PR trying to follow your suggestions. Now it effectively moves keys from |
ACK 6ecc880 except for Kixunil's comment about deprecation. |
@apoelstra updated, needs re-ACK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 38fdb15
… 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 38fdb15
Had to re-base after the latest merges; @sanket1729 @Kixunil @apoelstra pls re-ACK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 0d5d0f3
Now this one also has conflicts |
Rebased, another re-ACK required, sorry, @Kixunil @apoelstra @sanket1729 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-ACK 406f4c4
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.
@apoelstra which is good, isn't it? :D |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 406f4c4
Rebased after latest merges. @apoelstra @Kixunil @sanket1729 please re-ACK. This will complete Taproot todo list and open a way to get first RC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 05f4d07
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 05f4d07
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.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 8a993e8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 8a993e8
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 exposingXonlyPublicKey
type, since now all key types may co-exist in a single module under different namesThe PR goal is achieved through
ec
mod in it;key
module, removing previous depreciation message for bitcoin keys.