Skip to content
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

update libsecp to latest ersion; bump major version number #384

Merged
merged 5 commits into from Mar 9, 2022

Conversation

apoelstra
Copy link
Member

Should wait on merging until we get a minor release out with #382 and #376.

May also want to bundle #380 with this?

@apoelstra
Copy link
Member Author

cc @real-or-random

real-or-random
real-or-random previously approved these changes Jan 24, 2022
Copy link
Collaborator

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

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

utACK 6950253 I inspected the rust changes but I haven't reproduced the C subtree

Comment on lines 61 to +63
base_config.file("depend/secp256k1/contrib/lax_der_parsing.c")
.file("depend/secp256k1/src/precomputed_ecmult_gen.c")
.file("depend/secp256k1/src/precomputed_ecmult.c")
Copy link
Collaborator

Choose a reason for hiding this comment

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

note: In upstream, we compile these to a separate object which is then linked to the actual library code. This saves compilation time in the common case that the code has changed but tables haven't. But it's probably not worth the hassle here because we never touch the C code here, not even during development.

@dr-orlovsky
Copy link
Contributor

Is this ready to be merged to unblock miniscript progress towards taproot? (see rust-bitcoin/rust-miniscript#292 for details)

@dr-orlovsky dr-orlovsky added this to Ready for Review in Taproot Feb 21, 2022
Taproot automation moved this from Ready for Review to In review Feb 21, 2022
@apoelstra
Copy link
Member Author

Rebased. But we still don't have #390 in.

@apoelstra
Copy link
Member Author

Let's get #396 #401 #402 #403 and maybe #406 in.

@Kixunil
Copy link
Collaborator

Kixunil commented Feb 23, 2022

Do we plan to change MSRV already or is it intended for later release?

@apoelstra
Copy link
Member Author

@Kixunil later release. I'd like to get this major release into the upcoming rust-bitcoin major release, so I'm trying to keep it small (just new version of upstream and fixed Parity API)

@Kixunil
Copy link
Collaborator

Kixunil commented Feb 24, 2022

OK, will change #403 to not require MSRV bump.

@apoelstra
Copy link
Member Author

Rebased. Remaining:

@tcharding
Copy link
Member

tcharding commented Mar 1, 2022

Let's leave it out, thanks for asking.

@Kixunil
Copy link
Collaborator

Kixunil commented Mar 1, 2022

changes the serde serialization which is a controversial thing to do

Is it? One of my PRs did just that and nobody complained. :)

@apoelstra
Copy link
Member Author

@Kixunil just in Parity, you mean? I'm ok with that since it's a new and fairly minor type.

@Kixunil
Copy link
Collaborator

Kixunil commented Mar 1, 2022

Yes, OK.

@apoelstra
Copy link
Member Author

Would like to get #410 in as well

Two API changes needed to be reflected: schnorrsig_sign and schnorrsig_verify.

Also bump both Cargo.toml files
…CHECK

We can no longer produce non-verification context objects, so instead produce
an invalid public key.
apoelstra added a commit that referenced this pull request Mar 8, 2022
6bcf3ea Add bitcoin-hashes-std features (Tobin Harding)
555833b Disable bitcoin_hashes default features (Tobin Harding)
b6f169f Improve manifest whitespace (Tobin Harding)

Pull request description:

  Currently we use default features for the `bitcoin_hashes` dependency, doing so breaks the `no-std` feature in `rust-bitcoin` because `std` is part of `bitcoin_hashes` default feature set.

  Disable `bitcoin_hashes` default features, no changes to `rust-bitcoin` are require after this change since we manually turn on `std` and `alloc` as part of the `std`/`no-std` features of `rust-bitcoin`.

  For other users of `rust-secp256k1` this is a breaking change but is unlikely to cause too much bother because `std` is so commonly used.

  This PR resolves an open [issue](#384) in `rust-bitcoin`, see issue for discussion.

ACKs for top commit:
  apoelstra:
    ACK 6bcf3ea

Tree-SHA512: 3cb83b67ba73b096f05cb5c98e1057c34cbf75208c626830a9c5050d3927c7dc6c13109e43c01701b1dfa7adfcfb6745bae6501f903be5976f6d1534fa9b3598
@apoelstra
Copy link
Member Author

Rebased; added changelog; this is ready to go.

@elichai and/or @real-or-random can you ACK this?

Copy link
Collaborator

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

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

ACK aa51638 I can't judge if the feature set is meaningful but this release PR is fine

Taproot automation moved this from In review to Ready for merge Mar 9, 2022
@sanket1729
Copy link
Member

Thanks for this @real-or-random. This release would give us static contexts that help a lot in terms of code hygiene for downstream projects.

@apoelstra apoelstra merged commit 50b7c25 into rust-bitcoin:master Mar 9, 2022
Taproot automation moved this from Ready for merge to Done Mar 9, 2022
@apoelstra apoelstra deleted the 2022-01--secp-update branch March 9, 2022 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Update to latest master to get support for static verification contexts
6 participants