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

Relax cc dependency requirements, bump secp256k1 to 0.15 and bump version to 0.17.0 #9

Merged
merged 4 commits into from
Jul 27, 2019

Conversation

jonasnick
Copy link
Contributor

The current latest version of rust-bitcoinconsensus (0.16.4) is not compatible with secp256k1 0.13 since the cc requirements were relaxed (rust-bitcoin/rust-secp256k1#103). The result is that when we update to secp256k1 in rust-bitcoin rust will pick bitcoinconsensus 0.16.3. The problem is that rust 1.22 has a bug where the attempt to resolve dependencies in such a way leads to an infinite loop. My hypothesis is that this PR will fix the problem and allow rust-bitcoin to update its secp256k1 version (see PR rust-bitcoin/rust-bitcoin#278).

@jonasnick
Copy link
Contributor Author

Travis fails because it runs tests with rustc 1.14 which rust-secp256k1 0.13 doesn't support anymore. No idea why the appveyor/windows tests fail.

@jonasnick
Copy link
Contributor Author

Added commit to update the tests in travis and appveyor.

@apoelstra
Copy link
Member

We should go straight to 0.14 - rust-bitcoin/rust-secp256k1#127

@jonasnick
Copy link
Contributor Author

We can't simply update to secp256k1 0.14 because secp256k1_context_create were removed:

rust-bitcoinconsensus/bitcoin/src/pubkey.cpp:288: undefined reference to `secp256k1_context_create'

Looks like rust-bitcoinconsensus only imports rust-secp to link the C++ code against it. It would make sense to make this crate compile let its own libsecp now.
But afaik there's no namespacing and we could run into the same problems that we had in rust-secp256k1-zkp where you don't know against which secp you're linking (which is why we have this script in rust-secp-zkp to rename secp256k1_ to secp256k1_zkp_).

@apoelstra
Copy link
Member

What a mess :(

@elichai
Copy link
Member

elichai commented Jul 8, 2019

Maybe a solution could be to make the compiling of libsecp in rust-secp256k1 optional and then you could link a different version? :/

@apoelstra
Copy link
Member

@elichai letting rust-secp users replace the C bindings without replacing the ffi module is a recipe for horrifying UB

@elichai
Copy link
Member

elichai commented Jul 8, 2019

I hate linking problems.
Had similar problems in rust only too (rust-rocksdb/rust-rocksdb#308)

I don't see an easy solution to it without either side doing something weird. (adding some compilation condition to add/remove functions/libraries)

@elichai
Copy link
Member

elichai commented Jul 12, 2019

I have an idea on how to solve this.
We can write a simple rust shim that provides secp256k1_context_create and secp256k1_context_destroy functions. that way it could link against them.

If you think it's a good idea I can write that and open a PR here

@jonasnick
Copy link
Contributor Author

@elichai Good idea imo!

@jonasnick
Copy link
Contributor Author

Perhaps longer term it should be possible to add prefixes to libsecp symbols, similar to what boringssl does (briansmith/ring#849).

@apoelstra
Copy link
Member

Longer term :) Hopefully we can find a solution that does not require go to build things.

@jonasnick jonasnick changed the title Relax cc dependency requirements, bump secp256k1 to 0.13 and bump version to 0.16.5 Relax cc dependency requirements, bump secp256k1 to 0.14 and bump version to 0.16.5 Jul 14, 2019
@jonasnick
Copy link
Contributor Author

Added commit to bump secp256k1 to 0.14(.1)

@elichai
Copy link
Member

elichai commented Jul 14, 2019

Shouldn't this be a major bump assuming these changes can break stuff if someone has an older rust-secp256k1?

@jonasnick jonasnick changed the title Relax cc dependency requirements, bump secp256k1 to 0.14 and bump version to 0.16.5 Relax cc dependency requirements, bump secp256k1 to 0.14 and bump version to 0.17.0 Jul 14, 2019
@jonasnick
Copy link
Contributor Author

Good point @elichai. Bumped to 0.17.0

@elichai
Copy link
Member

elichai commented Jul 15, 2019

Looks good! any ideas why it doesn't compile on Windows?(appveyor not passing)

Edit: I see now that it's broken since #7, so nvm.

@jonasnick
Copy link
Contributor Author

I see now that it's broken since #7, so nvm.

There was an attempt to fix this in #8. I cherry picked the relevant commit. Let's see...

@jonasnick
Copy link
Contributor Author

Pssh, new errors... I have no idea.

@elichai
Copy link
Member

elichai commented Jul 16, 2019

arghh.
It seems that this commits fix the current errors: rust-lang/cc-rs@9dcf641#diff-1636770f970629d064824915e71ea2cd

Could we relax cc more now that upstream has CI for both MSVC and Linux for 1.16.0?
https://github.com/alexcrichton/cc-rs/blob/master/azure-pipelines.yml
CC @TheBlueMatt

(If anyone knows of a way to test the windows builds on linux I can test different versions of cc before we relax anything)

@jonasnick
Copy link
Contributor Author

As far as I can tell we'd have to

  1. require at least cc 1.0.36 in rust-bitcoinconsensus
  2. relax cc requirement in rust-secp256k1 to include 1.0.36

@apoelstra
Copy link
Member

I think it's fine to support any cc as long as the minimum compiler version there is still 1.16.0

@elichai
Copy link
Member

elichai commented Jul 16, 2019

This seem to fix it: rust-bitcoin/rust-secp256k1#131

Tests: https://ci.appveyor.com/project/elichai/rust-bitcoinconsensus/builds/26027109
code: https://github.com/elichai/rust-bitcoinconsensus/tree/cc

It did also require enabling the recovery module because of missing secp256k1_ecdsa_recoverable_signature_parse_compact symbol (elichai@b15dae9)

@jonasnick
Copy link
Contributor Author

@elichai great, thanks!

@jonasnick jonasnick changed the title Relax cc dependency requirements, bump secp256k1 to 0.14 and bump version to 0.17.0 Relax cc dependency requirements, bump secp256k1 to 0.15 and bump version to 0.17.0 Jul 26, 2019
@tamasblummer
Copy link
Contributor

checked OSX build and link with current rust-bitcoin master and secp256k1 v0.15.0

@tamasblummer
Copy link
Contributor

checked Linux build and link with current rust-bitcoin master and secp256k1 v0.15.0

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.

5 participants