Skip to content

Update to secp256k1 0.21.2#755

Merged
dr-orlovsky merged 2 commits intorust-bitcoin:masterfrom
sanket1729:secp_update
Jan 7, 2022
Merged

Update to secp256k1 0.21.2#755
dr-orlovsky merged 2 commits intorust-bitcoin:masterfrom
sanket1729:secp_update

Conversation

@sanket1729
Copy link
Copy Markdown
Member

@sanket1729 sanket1729 commented Jan 6, 2022

No description provided.

@sanket1729 sanket1729 changed the title Update to secp256k1 0.21.2 [DO NOT MERGE]Update to secp256k1 0.21.2 Jan 6, 2022
@sanket1729 sanket1729 mentioned this pull request Jan 6, 2022
Merged
@sanket1729 sanket1729 force-pushed the secp_update branch 2 times, most recently from 84c48f7 to 10883eb Compare January 6, 2022 18:35
apoelstra added a commit to rust-bitcoin/rust-secp256k1 that referenced this pull request Jan 6, 2022
@apoelstra
Merge #361: Add basic derives for Parity
6911734 
1671dfc Release 0.21.2 (sanket1729)
837be22 Basic derives for Parity (sanket1729)
7059192 Wildcard export from key module (sanket1729)

Pull request description:

  Sorry for getting another point release. This time I have tested against this branch for rust-bitcoin rust-bitcoin/rust-bitcoin#755. Hopefully, this is the last release.

  Next release, we should have a Release Candidate for a couple of days before publishing a release.

ACKs for top commit:
  apoelstra:
    ACK 1671dfc

Tree-SHA512: 263ad027da3da764bd76f719200382c47ba21a976caefc23ebef45d1c4be35ddfc80ce619b57326310aaab22bbf75ca7f1db80b45e95ec076584805efb791f3f
@sanket1729 sanket1729 marked this pull request as ready for review January 6, 2022 23:00
@sanket1729 sanket1729 changed the title [DO NOT MERGE]Update to secp256k1 0.21.2 Update to secp256k1 0.21.2 Jan 6, 2022
@sanket1729
Update to secp256k1 0.21.2
2178c73
@sanket1729
Uncomment sighash test
91470f5 
We can check tweak add priv key with latest secp
@sanket1729
Copy link
Copy Markdown
Member Author

sanket1729 commented Jan 6, 2022

This is ready for review now and kind of a blocker for the rest of the PRs for the next release. @apoelstra, @dr-orlovsky , @Kixunil

sanket1729
sanket1729 commented Jan 6, 2022
View reviewed changes
src/util/taproot.rs
pub fn encode<Write: io::Write>(&self, mut writer: Write) -> io::Result<usize> {
let first_byte: u8 =
(if self.output_key_parity { 1 } else { 0 }) | self.leaf_version.as_u8();
let first_byte: u8 = i32::from(self.output_key_parity) as u8 | self.leaf_version.as_u8();
Copy link
Copy Markdown
Member Author

@sanket1729 sanket1729 Jan 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, we rely on Parity being only one bit. But converting to u8 should be enough.

@sanket1729 sanket1729 mentioned this pull request Jan 6, 2022
Merged
Kixunil
Kixunil approved these changes Jan 7, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 91470f5

However, I'm highly surprised Parity is not an enum (Even, Odd) and even more strangely, impl From<i32> for Parity exists. I'd expect it to be TryFrom. I understand this is something from secp256k1, that's why I'm ACKing this even though I find it suspicious, maybe even wrong.

If someone could explain why it's not an enum, it'd be appreciated.

@sanket1729
Copy link
Copy Markdown
Member Author

sanket1729 commented Jan 7, 2022

If someone could explain why it's not an enum, it'd be appreciated.

This was a mistake in rust-secp and I the maintainers agree with it. I think we have to live with this now till the next release :(.

For what it's worth, rust-bitcoin only has states Parity(1)/Parity(0) and there is no function that takes input Parity from the user.

@Kixunil
Copy link
Copy Markdown
Collaborator

Kixunil commented Jan 7, 2022

As far as I can see, the consumer of rust-bitcoin could construct invalid ControlBlock. I personally wouldn't mind if fixing breaking release of secp256k1 was made (soonish) but if people feel it's not a good idea I can accept the sad reality.

@sanket1729 sanket1729 mentioned this pull request Jan 7, 2022
Closed
20 tasks
dr-orlovsky
dr-orlovsky approved these changes Jan 7, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@dr-orlovsky dr-orlovsky left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 91470f5 in order to unlock the rest of PRs required for Taproot, which depend on this.

However I feel that we need another major secp256k1 release fixing Parity (making it enum) before releasing new rust-bitcoin version.

@dr-orlovsky dr-orlovsky added the P-high High priority label Jan 7, 2022
@dr-orlovsky dr-orlovsky added this to the 0.28.0 milestone Jan 7, 2022
@dr-orlovsky
Copy link
Copy Markdown
Collaborator

dr-orlovsky commented Jan 7, 2022

One use of deprecated function:

warning: use of deprecated associated function `secp256k1::ecdsa::recovery::<impl secp256k1::Secp256k1<C>>::sign_recoverable`: Use sign_ecdsa_recoverable instead.
   --> src/util/misc.rs:323:29

Since this is a non-critical warning I will merge this anyway, but it may be nice to fix it in some other PR

@dr-orlovsky dr-orlovsky merged commit 7010672 into rust-bitcoin:master Jan 7, 2022
@tcharding tcharding mentioned this pull request Jan 10, 2022
Closed
@dr-orlovsky dr-orlovsky mentioned this pull request Jan 15, 2022
Closed
chain-forgexcr45 added a commit to chain-forgexcr45/rust-secp256k1 that referenced this pull request Sep 28, 2025
@chain-forgexcr45
Merge rust-bitcoin/rust-secp256k1#361: Add basic derives for Parity
0b2e600 
1671dfc2ed7f35fc71ee256392a34196b081744d Release 0.21.2 (sanket1729)
837be22e09b9e1a51eea26a67ec4112fcdf83d6e Basic derives for Parity (sanket1729)
7059192de9e1fb6cf24cb03e8be31a4d6b4f6587 Wildcard export from key module (sanket1729)

Pull request description:

  Sorry for getting another point release. This time I have tested against this branch for rust-bitcoin rust-bitcoin/rust-bitcoin#755. Hopefully, this is the last release.

  Next release, we should have a Release Candidate for a couple of days before publishing a release.

ACKs for top commit:
  apoelstra:
    ACK 1671dfc2ed7f35fc71ee256392a34196b081744d

Tree-SHA512: 263ad027da3da764bd76f719200382c47ba21a976caefc23ebef45d1c4be35ddfc80ce619b57326310aaab22bbf75ca7f1db80b45e95ec076584805efb791f3f
william2332-limf added a commit to william2332-limf/rust-secp256k1 that referenced this pull request Oct 2, 2025
@william2332-limf
Merge rust-bitcoin/rust-secp256k1#361: Add basic derives for Parity
8c8ee3d 
1671dfc2ed7f35fc71ee256392a34196b081744d Release 0.21.2 (sanket1729)
837be22e09b9e1a51eea26a67ec4112fcdf83d6e Basic derives for Parity (sanket1729)
7059192de9e1fb6cf24cb03e8be31a4d6b4f6587 Wildcard export from key module (sanket1729)

Pull request description:

  Sorry for getting another point release. This time I have tested against this branch for rust-bitcoin rust-bitcoin/rust-bitcoin#755. Hopefully, this is the last release.

  Next release, we should have a Release Candidate for a couple of days before publishing a release.

ACKs for top commit:
  apoelstra:
    ACK 1671dfc2ed7f35fc71ee256392a34196b081744d

Tree-SHA512: 263ad027da3da764bd76f719200382c47ba21a976caefc23ebef45d1c4be35ddfc80ce619b57326310aaab22bbf75ca7f1db80b45e95ec076584805efb791f3f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Kixunil Kixunil Kixunil approved these changes

+1 more reviewer

@dr-orlovsky dr-orlovsky dr-orlovsky approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

P-high High priority

Projects

None yet

Milestone

0.28.0

Development

Successfully merging this pull request may close these issues.

3 participants

@sanket1729 @Kixunil @dr-orlovsky