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

Upgrade to secp256k1 0.21 #43

Closed
wants to merge 4 commits into from
Closed

Conversation

thomaseizinger
Copy link
Contributor

@thomaseizinger thomaseizinger commented Jan 7, 2022

Resolves #32.

We already re-export every module within `secp256k1` via `*`. There is
no need to selectively re-export some of them as well.

Remove the selected re-exports.
@thomaseizinger thomaseizinger force-pushed the bump-secp256k1-dependency branch from a52763d to 60dc32c Compare January 7, 2022 03:44
@thomaseizinger thomaseizinger force-pushed the bump-secp256k1-dependency branch 3 times, most recently from f247511 to ef3cd6d Compare January 7, 2022 03:51
Release 0.21 includes a module restructure which we can now make use of.
This simplifies some imports.
@thomaseizinger thomaseizinger force-pushed the bump-secp256k1-dependency branch from ef3cd6d to ce66406 Compare January 7, 2022 04:03
@thomaseizinger thomaseizinger marked this pull request as draft January 7, 2022 04:09
@thomaseizinger
Copy link
Contributor Author

@apoelstra Any idea why the tests are failing? I've read something about an endianness issue related to the latest secp256k1 release but can't find the comment now. Perhaps this is also an instance of that.

@apoelstra
Copy link
Contributor

@thomaseizinger I don't think it's about endianness, this looks like a memory issue, and I think it's because upstream's context object is now much smaller than the secp-zkp one, and this library treats them as being the same thing.

I think updating the embedded version of libsecp-zkp to one after BlockstreamResearch/secp256k1-zkp#159 should make things work, but I'm not sure, I'm not totally on top of the relative state of all these libraries.

@thomaseizinger
Copy link
Contributor Author

@thomaseizinger I don't think it's about endianness, this looks like a memory issue, and I think it's because upstream's context object is now much smaller than the secp-zkp one, and this library treats them as being the same thing.

I think updating the embedded version of libsecp-zkp to one after ElementsProject/secp256k1-zkp#159 should make things work, but I'm not sure, I'm not totally on top of the relative state of all these libraries.

Thanks for the pointer, I will try update the embedded lib. Totally forgot that we treating those contexts the same :)

@apoelstra
Copy link
Contributor

Yeah, we are really playing with fire there.

Maybe now that we have fully static contexts, we should stop using the upstream contents entirely and just create dummy contexts inside this lib's functions.

@apoelstra apoelstra closed this Jan 13, 2022
@apoelstra apoelstra reopened this Jan 13, 2022
@apoelstra
Copy link
Contributor

Sorry, for close, trackpad user failure.

@thomaseizinger
Copy link
Contributor Author

Yeah, we are really playing with fire there.

Maybe now that we have fully static contexts, we should stop using the upstream contents entirely and just create dummy contexts inside this lib's functions.

Upstream being rust-secp256k1 here? Is the plan for rust-secp to drop the context arg entirely?

@apoelstra
Copy link
Contributor

@thomaseizinger see discussion in rust-bitcoin/rust-secp256k1#343

The plan isn't totally clear yet.

This hash was chosen because it is a recent commit after the one
that secp256k1-sys depends on.
@thomaseizinger
Copy link
Contributor Author

Closing in favor of #46.

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.

Upgrade at least to rust-secp256k1 0.21
2 participants