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 to secp 0.26 and secp-sys 0.8 #62

Conversation

Tibo-lg
Copy link
Contributor

@Tibo-lg Tibo-lg commented Dec 26, 2022

This PR updates the rust-secp256k1(-sys) dependencies to latest released ones. I tried to mimic what was done upstream. Regarding the trait implementations (Eq, PartialEq,...), since the sys types in this crate don't seem to have serialize functions I felt like it was ok to just derive then but please correct me if that's wrong.

@Tibo-lg Tibo-lg force-pushed the update-upstream-dependencies branch from 6a154b7 to 737bfd1 Compare December 26, 2022 12:30
@Tibo-lg
Copy link
Contributor Author

Tibo-lg commented Dec 26, 2022

I had to implement the traits to make it work with 1.41.1 but not sure what is causing the CI failures. I'll have a look at it again but if someone has a suggestion let me know.

@Tibo-lg
Copy link
Contributor Author

Tibo-lg commented Dec 27, 2022

I'm quite confused, unless I'm missing something it looks like actions on the same commit passed in my forked repo:
https://github.com/p2pderivatives/rust-secp256k1-zkp/actions/runs/3781248443

@Tibo-lg Tibo-lg force-pushed the update-upstream-dependencies branch from 737bfd1 to bd24a81 Compare December 27, 2022 11:25
@Tibo-lg
Copy link
Contributor Author

Tibo-lg commented Jan 18, 2023

Investigating the CI failure I found that for some reason it only happens on recent version of OSX (I've updated recently and before the update I could not reproduce the issue, now I can). The issue also seem to happen on master and also on rust-secp256k1, so I'm pretty confident it's not introduced by this PR.

@Tibo-lg Tibo-lg force-pushed the update-upstream-dependencies branch from bd24a81 to fa5d316 Compare January 30, 2023 05:36
@Tibo-lg Tibo-lg changed the title Update to secp 0.25 and secp-sys 0.7 Update to secp 0.26 and secp-sys 0.8 Jan 30, 2023
Copy link
Contributor

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK fa5d316

@apoelstra
Copy link
Contributor

CI failure seems to be same as rust-pcap/pcap#280 (to take an example from a random project). Apparently "rust 1.41 does not work on macos >= 12". So we will just need to adjust CI.

@Tibo-lg could you add a commit which changes CI to not test 1.41 on Mac?

@Tibo-lg Tibo-lg force-pushed the update-upstream-dependencies branch from 0ce330b to a1cff98 Compare February 1, 2023 00:14
@Tibo-lg
Copy link
Contributor Author

Tibo-lg commented Feb 1, 2023

@Tibo-lg could you add a commit which changes CI to not test 1.41 on Mac?

@apoelstra Done! I've modified a bit the git action file removing the target field which seemed unused, making it easier to exclude 1.41 on macos, but I hope that doesn't break anything.
Thanks for checking the PR!

@apoelstra
Copy link
Contributor

Looks good. I'll ACK and merge this since it fixes CI.

But cc @jonasnick is there any specific reason we had the target field here? Was it to test different endianness or anything?

Copy link
Contributor

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK a1cff98

@apoelstra apoelstra merged commit f2fd733 into BlockstreamResearch:master Feb 1, 2023
@jonasnick
Copy link
Collaborator

I don't remember. It seems redundant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants