Update rust-secp-zkp to latest upstream and latest rust-secp

[sanket1729]

The upstream impl_array_newtype now implements a core::Hash. This
caused a breaking changed here :( despite it being a minor release.

[sanket1729]

Now that we have a rust-secp release, I have attempted to take update upstream. However, I am facing errors in WASM build like

 = note: rust-lld: error: duplicate symbol: WASM32_INT_SIZE
          >>> defined in /home/sanket1729/rust-secp256k1/target/wasm32-unknown-unknown/release/deps/libsecp256k1_sys-4d81bcb6fb4889ca.rlib(precomputed_ecmult_gen.o)
          >>> defined in /home/sanket1729/rust-secp256k1/target/wasm32-unknown-unknown/release/deps/libsecp256k1_sys-4d81bcb6fb4889ca.rlib(precomputed_ecmult.o)

See also the CI failure here: https://github.com/ElementsProject/rust-secp256k1-zkp/runs/5543678672?check_suite_focus=true

This issue is not faced by upstream rust-secp256k1. I tried a bit to find the differences and to make it work, but at this point I am trying random things basically to make things work. So, it's probably better for someone who knows this to pick this up.

Any inputs on trying to fix this appreciated @thomaseizinger, @apoelstra ?

[thomaseizinger]

It seems our hack of reusing the same context is haunting us down now :D

From thinking about how things work, my analysis would be:

• upstream libsecp256k1 defines some global symbol
• upstream libsecp256k1-zkp being a fork of that has the same symbol
• in this library, we link against both and puff, we want to redefine the same symbol

We should be able to work around this by including a patch in this repository, that removes the offending global symbol from the libsecp256k1-zkp code.

[thomaseizinger]

Hmm, that might have been a too quick analysis. I just realized that the error points to the same rlib.

I am not very proficient in C but shouldn't this header have an IFDEF guard to avoid duplicate definitions in case it is included multiple times?

[sanket1729]

I am not very proficient in C but shouldn't this header have an IFDEF guard to avoid duplicate definitions in case it is included multiple times?

I tried that but still failed. The interesting thing is that duplicate symbols all point to things that are upstream. And the upstream CI does not fail 😕 🤷

[thomaseizinger]

I am not very proficient in C but shouldn't this header have an IFDEF guard to avoid duplicate definitions in case it is included multiple times?

I tried that but still failed. The interesting thing is that duplicate symbols all point to things that are upstream. And the upstream CI does not fail confused shrug

Another theory: We fail at link time right? Do we test that we can actually link rust-secp256k1 version 0.5 against something? That change was made only 6 days ago: rust-bitcoin/rust-secp256k1@8294ea3.

I did some more digging and the CI for that upstream PR "passed" but it is actually broken: https://github.com/rust-bitcoin/rust-secp256k1/runs/5470259221?check_suite_focus=true#step:5:518

I am almost certain that if that script would actually run, the linking would also fail there.

[thomaseizinger]

I know people feel strongly about not "vendor-locking" into GitHub actions and thus packing all the testing into test.sh. However, this is not the first time that the bash code in that script is buggy and we thus have seemingly successful CI, yet it is actually broken.

Personally, I would buy into the GitHub actions ecosystem, using matrix etc to their full advantage and "simply" try to organise all testing such that cargo test --all-targets --all-features is sufficient locally. Every layer between the CI server and the actual command for running the tests unnecessarily obfuscates things which leads to situations like this, esp. when that layer is written in bash which I think we all agree is a horribly broken language? Just my 2c :)

[sanket1729]

Personally, I would buy into the GitHub actions ecosystem, using matrix etc to their full advantage and "simply" try to organise all testing such that cargo test --all-targets --all-features is sufficient locally. Every layer between the CI server and the actual command for running the tests unnecessarily obfuscates things which leads to situations like this, esp. when that layer is written in bash which I think we all agree is horribly broken language? Just my 2c :)

+1. It's really sad that the list slipped past in 0.5.0 upstream release. I don't have any experience with HG actions apart from copying some templates and making them work, but if it is as simple as cargo test --all-targets, it's a huge advantage. I think it will take a lot of convincing to other participants to make this happen, but incidents like this make a good case.

Speaking of WASM here issue, are you and @apoelstra okay with disabling it in CI so that we can progress on Musig2 PRs like (#29) ? We can re-enable it once upstream has some solution for it.

[thomaseizinger]

I am okay with disabling it for now, we are not relying on the wasm build to work!

In regards to GH actions, I may find time to draft something up to show what it would look like.

There are also docker-based solutions for running GitHub workflows locally.

[sanket1729]

Marked this as ready for review, and opened an issue to keep track fo WASM issue #47

[thomaseizinger]

LGTM overall.

Should we move the "Comment out Wasm build on CI" commit before we bump the secp256k1 version so that every commit builds on CI? Also, is the patch of the header file necessary for the upgrade? If yes then I think we might want to squash these changes so that the commits are still atomic.

[sanket1729]

Should we move the "Comment out Wasm build on CI" commit before we bump the secp256k1 version so that every commit builds on CI? Also, is the patch of the header file necessary for the upgrade? If yes then I think we might want to squash these changes so that the commits are still atomic.

Agreed on all counts. The patch was in separate commit to highlight the changes.
Pushed to c61a982. The diff between the previous ACK and this one is null.

sanket1729@sanket-pc:~/rust-secp256k1-zkp$ git diff c61a9820bd57162914d24642beb2405c1870bf6d c1efef9

[thomaseizinger]

ACK c61a982

The tests don't pass for commit b1a3048 because the underlying C libraries differ. I think that is okay, at least everything compiles for each commit.

[apoelstra]

utACK c61a982

Reviewed all the code (and redid the all the automated steps). I think we're good to go here.

[sanket1729]

@apoelstra , @thomaseizinger can any of you merge this?

[apoelstra]

Yep, I will

[apoelstra]

Merged. Should I publish the new versions of secp-zkp and secp-zkp-sys?

[sanket1729]

@apoelstra Yes, please. That would be helpful, the latest published version does not even build...

[apoelstra]

Ok, done.