update vendored libsecp to 0.3.2 #645
Conversation
apoelstra
force-pushed
the
2023-08--update-0.3.2
branch
from
7c27788
to
acc16cc
Compare
|
CI failure is because we have a |
upstream libsecp now has a CMakeLists.txt file. Many years ago we added some things to .gitignore which appear to be local developers committing the names of their own stray files, and now this is causing the revendoring script to lose track of vendored files.
This is just a bad test. It constructs a preallocated context object by starting from a non-preallocated context object, in a way that can't be done by users (since it directly constructs a `Secp256k1` struct) and a way that is very difficult to unwind, because you wind up with two pointers to the same underlying context object, one a "preallocated" one and one a normal one. If you then drop the preallocated one, it will call `secp256k1_context_destroy`, forcing you to manually deallocate the other one. If you drop the normally-allocated one, you need to mem::forget the preallocated one to avoid calling `secp256k1_context_destroy` twice. The whole thing is pretty fragile. There is another unit test, `test_raw_ctx`, which gets into the same situation but using the public API, and demonstrates a few ways to get out of it.
I'm not sure how these came to be committed, but they shouldn't be. Running the vendoring script results in them being deleted.
apoelstra
force-pushed
the
2023-08--update-0.3.2
branch
from
acc16cc
to
186b643
Compare
|
Should be good to go. This wound up being more involved than I expected. Upstream changed the semantics of |
There was a problem hiding this comment.
Light review, I didn't verify the changes to depend, assuming they are done mechanically. Also I did not fully grok the ManuallyDrop unit test changes. I don't claim to be an unsafe code reviewer at the moment and did not allocate clock cycles to understanding this particular thing fully.
| | grep "SIGILL\\|\[libsecp256k1] illegal argument. !rustsecp256k1_v0_._._fe_is_zero(&ge->x)" | ||
| | grep "SIGILL\\|\[libsecp256k1] illegal argument. " |
There was a problem hiding this comment.
Why this change? It works still, right? You just don't like the grep statement?
There was a problem hiding this comment.
No, the assertion has changed in the latest version of libsecp.
| // The following drop will have no effect; in fact, they will trigger a compiler | ||
| // error because manually dropping a `ManuallyDrop` is almost certainly incorrect. |
There was a problem hiding this comment.
Did you mean to write "will trigger a clippy warning"?
There was a problem hiding this comment.
No, in 1.72 and higher it will be a compiler warning.
|
We want to get this in before release, right? |
If rust-bitcoin#645 merges we will need a release of `secp256k1-sys` as well.
|
Can you also add how to reproduce this in the commit message? Would help me review. I think the vendoring scripts now have some env variables that I need to set to reproduce. |
|
@sanket1729 put a command in the commit description. |
|
Gentle bump please crew. |
If rust-bitcoin#645 merges we will need a release of `secp256k1-sys` as well.
|
Do we want to jump straight to v0.4.0? |
|
@tcharding yeah, let's do it. Can you open such a PR? You may want to copy the patch files from this one. (If not I can do it, just let me know. Normally it's a straightforward process of just running the vendoring script, but if the patches no longer apply it can be a bit thorny.) |
|
+1 on bumping to v0.4.0. Since this release is the only one with the EllSwift module, needed for #627. |
|
Yep no sweat I can do it. |
|
I just realized that releasing with libsecp 0.4.0 is not trivial because we need to write the wrappers for the new EllSwift stuff, given that (and the fact that I can't satisfy the linker in #652) as well as the fact that we are behind schedule on the rust-bitcoin release can we merge and release using this PR and then do libsecp 0.4.0 upgrade straight after release? |
|
Oh my bad, I did not realise. Will have another go, thanks. |
80b2a8d Update vendored libsecp to v0.4.0 (Davidson Souza) d2285c9 ci: Remove MIPS* from CI (Davidson Souza) 0d58f50 ci: generalize grp in "illegal callback" test (Andrew Poelstra) acf9ac1 delete `test_manual_create_destroy` test (Andrew Poelstra) 04ce508 lib: fix bad unit test (Andrew Poelstra) e4cca90 gitignore: remove things that shouldn't be there (Andrew Poelstra) Pull request description: Replaces #645 and #652. Precedes #627. I'm basically using #652 but resolving the linking problems, My local CI is erring on windows cross-test, but I can compile without issue with `cargo build --target x86_64-pc-windows-gnu`. Some MIPS jobs failed before even installing cross, I think those aren't really related to this PR. Any ideas on what can be happening? ACKs for top commit: apoelstra: ACK 80b2a8d Tree-SHA512: 62c2e04348110e3995111fa666f10dcc403b963770d047361f9209cf45b45db8744a7eb6d9ee3278d18007412dab5131ac3e1dd3e3d704963c6a6f232d57199a
|
We can close this one now. |
Fixes #631.
Also bumps the major version of rust-secp256k1-sys since this bump involves renaming all the symbols, so we might as well do it now. But we won't release after this PR; will wait until we're sure we're ready to do the release.
To reproduce run