Add secp256k1-sys with prefixed sources #169

stevenroose · 2019-10-21T12:21:17Z

I'm adding a secp256k1-sys crate with just the FFI wrapper of the C libsecp.

And then I added a script (thanks @jonasnick) to prefix the secp sources with a rust-secp specific tag and a version tag, so that we don't suffer from naming conflicts anymore.

elichai · 2019-10-21T20:29:29Z

idk about this.
but first of all you brought back all the malloc functions we removed in #130
And even about the prefix, there are people who do want to compile this code as libsecp (i.e. bitcoin-consensus and stuff @TheBlueMatt is working on). although in theory LTO should take care of the duplicates.

Even if other people do want this i'll split it to ~3 PRs. because right now it's not auditable.
(i.e. one PR creating the *-sys crate, another move only another adds the prefix)

stevenroose · 2019-10-23T10:10:24Z

but first of all you brought back all the malloc functions we removed in #130

That was an accident. Fixed that with patch files.

Even if other people do want this i'll split it to ~3 PRs. because right now it's not auditable.

What's wrong with one PR with 3 commits? The creating the -sys crate and moving the dep is not possible to do in 2 commits without having a giant diff of first adding the entire depend folder a second time in another place and then removing the original in a second commit instead of having git treat them as a move.

I can do that if that is favorable, but that was the reason I did the -sys crate together with migrating secp to -sys in the same commit.

jonasnick

Good catch @elichai, bitcoin-consensus relies on the secp symbol names (because it links libbitcoinsensus with libsecp which has been provided with rust-secp). Not sure what's the best way to solve this.

Fwiw I don't find this PR impossible to review and not sure how 3 PRs would help.

How would LTO take care of this? It doesn't, by default. Also there could be multiple versions of the library in play which can't be solved automatically.

@stevenroose travis doesn't run the -sys tests. It's just a meaningless single test, but would be better to add to travis, so we don't forget it in the future.

secp256k1-sys/vendor-libsecp.sh

jonasnick · 2019-10-24T14:29:30Z

secp256k1-sys/src/lib.rs

@@ -764,3 +785,4 @@ mod tests {
        assert_eq!(orig.len(), unsafe {strlen(test.as_ptr())});
    }
 }
+


is that intentional? you've done that a couple of times.

the ending newline? no idea, I think my vim might not like files without them perhaps? can I consider this a nit? :)

secp256k1-sys/src/macros.rs

elichai · 2019-10-24T15:33:56Z

@jonasnick I forgot about reviewing commit by commit, I'll try that. Sorry.

Isn't the whole point of LTO is to remove code duplication? So if you link against two versions of libsecp but with different symbols then most of the duplication should disappear it you use LTO.

elichai · 2019-10-24T15:58:34Z

secp256k1-sys/depend/secp256k1/Makefile.am

@@ -2,13 +2,13 @@ ACLOCAL_AMFLAGS = -I build-aux/m4



FYI, we don't use the makefile/autotools. so there's no actual reason to change anything there

That is just the auto replace at work.

elichai · 2019-10-24T16:00:11Z

secp256k1-sys/depend/secp256k1/sage/secp256k1.sage

@@ -5,8 +5,8 @@ import sys
 load("group_prover.sage")


This basically checks out libsecp in the depends/secp256k1 folder and then changes the variables and removes the allocation code. Nothing more. So yeah some unused artifacts are there.

secp256k1-sys/depend/secp256k1/src/asm/field_10x26_arm.s

jonasnick · 2019-10-24T16:03:08Z

@elichai just to clarify, are you aware that this PR contains a script secp256k1-sys/vendor-libsecp.sh that just replaces all references of secp256k1 with rustsecp256k1_v0_1_0?

elichai · 2019-10-24T16:07:50Z

@jonasnick well we still need to review that the script did it's job currently and then the person who ran it didn't change anything.

Is it possible in C to have private functions? or all functions are "public" so we need to mangle all their names? even inner functions

jonasnick · 2019-10-24T16:15:22Z

then the person who ran it didn't change anything.

You can verify the script then run it yourself. But okay.

Just renaming may introduce bugs that are not caught by the compiler. But that seems highly unlikely and will also be difficult to notice in manual review.

even inner functions

Are you suggesting to have a more complicated script that only looks at the exported symbols? Why?

stevenroose · 2019-10-25T15:28:08Z

@elichai so you can run the following command to verify the depends folder:

cd ./secp256k1-sys/
./vendor-libsecp.sh depend 0_1_0 143dc6e9ee31852a60321b23eea407d2006171da

The commit has is committed in the ./secp256k1-sys/depend/secp256k1-HEAD-revision.txt file.

real-or-random

Approach ACK

My feeling is that the entire approach needs some README file or similar. What is done here, and the reason why it's done is really not trivial. People not familiar which the project will think we're crazy.

secp256k1-sys/src/lib.rs

secp256k1-sys/vendor-libsecp.sh

real-or-random · 2019-10-29T15:08:11Z

If we anyway clone secp256k1, could we remove it entirely? Probably not because then you need to be online for building and we trust Github more? It's just a wild thought, please ignore me if this is stupid.

elichai · 2019-10-29T15:44:22Z

@stevenroose I tried to link this against upstream and ecdsa_signature_parse_der_lax is missing.
I guess we should build that. because it's not an official part of upstream.

@TheBlueMatt How are you fixing this here? (I tried turning LTO on but I still get link errors)

TheBlueMatt · 2019-10-29T19:02:44Z

Cargo.toml

+
+# Use this feature to not compile the bundled libsecp256k1 C symbols,
+# but use external ones.
+external-symbols = ["secp256k1-sys/external-symbols"]


Can you just not depend on secp256k1-sys at all if you build with external-symbols? pulling in a dummy crate as a dependency sucks.

The crate is still defining the C interface and the FFI objects used in it. In fact it's not doing less than when built without external-symbols, it's just the build.rs construction is no longer utilized.

At least we should add a scarier warning.

Also, as you noted in another channel, this has the potential to cause UB a user is using an old version of rust-secp, as well as this one with external-symbols enabled...so we can't depend on cargo failing if this is enabled on a cargo build.

stevenroose · 2019-10-29T22:15:14Z

If we anyway clone secp256k1, could we remove it entirely? Probably not because then you need to be online for building and we trust Github more? It's just a wild thought, please ignore me if this is stupid.

We're only cloning it when we revendor. That only needs to happen when updated to a new version. The sources are still required for being able to build.

apoelstra · 2019-11-06T19:04:12Z

When we merge this let's be sure to get #162 in as well.

stevenroose · 2019-11-12T17:10:41Z

Any updates on this?

elichai · 2019-11-12T18:04:26Z

secp256k1-sys/src/lib.rs

@@ -365,7 +386,7 @@ unsafe fn strlen(mut str_ptr: *const c_char) -> usize {
 /// Rust doesn't promise what pointers does it give to ZST (https://doc.rust-lang.org/nomicon/exotic-sizes.html#zero-sized-types-zsts)
 /// In case the type is empty this trait will give a NULL pointer, which should be handled in C.
 /// 
-pub(crate) trait CPtr {
+pub trait CPtr {


In a late PR I'll try move this to secp256k1 and not -sys and make it pub(crate) again.

Is this not used in -sys? In that case I can move it.. The idea is to try and reduce the times we need to bump a -sys version. So I think better make breaking changes now.

Hmm, seems to be used in macros as well. Yeah I think I'd prefer to merge this as is :)

Merge != release.
let's not change this PR any more so it can get merged at some point :)

elichai

ACK code review.
ACK reproduced the same changes by the script.
0093a5d

I'm still not so sure about this script but I guess that's the best we can do right now.

secp256k1-sys/src/lib.rs

secp256k1-sys/vendor-libsecp.sh

elichai

ACK on the diff

elichai

ACK on latest diff

apoelstra

ACK. I was suspicious about this script but it's actually pretty great.

real-or-random · 2019-11-27T20:20:52Z

needs rebase :/

This feature disables using the bundles sources and will link into existing libsecp256k1 symbols.

elichai

ACK
Reviewed the diff from my last ACK

stevenroose changed the title ~~Add secp256k1-sys~~ WIP: Add secp256k1-sys Oct 21, 2019

stevenroose force-pushed the sys branch 3 times, most recently from f5b52d1 to 6aac0f4 Compare October 21, 2019 19:08

stevenroose force-pushed the sys branch 2 times, most recently from cc5b3e9 to 267ba31 Compare October 23, 2019 10:07

stevenroose force-pushed the sys branch from 267ba31 to b9d3617 Compare October 23, 2019 10:14

stevenroose changed the title ~~WIP: Add secp256k1-sys~~ Add secp256k1-sys Oct 23, 2019

stevenroose changed the title ~~Add secp256k1-sys~~ Add secp256k1-sys with prefixed sources Oct 23, 2019

stevenroose force-pushed the sys branch from b9d3617 to d9c474f Compare October 24, 2019 09:20

jonasnick reviewed Oct 24, 2019

View reviewed changes

elichai reviewed Oct 24, 2019

View reviewed changes

secp256k1-sys/depend/secp256k1/src/asm/field_10x26_arm.s Show resolved Hide resolved

stevenroose mentioned this pull request Oct 29, 2019

Add a feature to disable replacing C symbols with rust #177

Merged

stevenroose force-pushed the sys branch 3 times, most recently from 151acda to f1bbc2e Compare October 29, 2019 11:41

real-or-random reviewed Oct 29, 2019

View reviewed changes

secp256k1-sys/src/lib.rs Outdated Show resolved Hide resolved

secp256k1-sys/vendor-libsecp.sh Outdated Show resolved Hide resolved

secp256k1-sys/vendor-libsecp.sh Outdated Show resolved Hide resolved

secp256k1-sys/vendor-libsecp.sh Show resolved Hide resolved

TheBlueMatt reviewed Oct 29, 2019

View reviewed changes

stevenroose mentioned this pull request Nov 6, 2019

FR: Have a method to construct a Secp256k1 context from a raw context pointer #178

Closed

stevenroose force-pushed the sys branch 2 times, most recently from d2cd901 to 0093a5d Compare November 6, 2019 14:04

elichai reviewed Nov 12, 2019

View reviewed changes

elichai previously approved these changes Nov 12, 2019

View reviewed changes

apoelstra reviewed Nov 12, 2019

View reviewed changes

secp256k1-sys/src/lib.rs Show resolved Hide resolved

apoelstra reviewed Nov 13, 2019

View reviewed changes

secp256k1-sys/vendor-libsecp.sh Outdated Show resolved Hide resolved

stevenroose dismissed elichai’s stale review via f0d2b5e November 13, 2019 10:55

elichai previously approved these changes Nov 13, 2019

View reviewed changes

stevenroose dismissed elichai’s stale review via a0a0a0b November 13, 2019 15:19

stevenroose force-pushed the sys branch from a0a0a0b to fdaf15a Compare November 27, 2019 17:53

elichai previously approved these changes Nov 27, 2019

View reviewed changes

apoelstra previously approved these changes Nov 27, 2019

View reviewed changes

stevenroose added 6 commits November 27, 2019 21:02

Add forgotten mention of upstream update

9bd0882

Add secp256k1-sys

724c49f

Add secp256k1-sys README and LICENSE files

80e1935

Add vendor script to prefix secp256k1 sources

55fab77

Add external-symbols feature to support external libsecp

bf3fba7

This feature disables using the bundles sources and will link into existing libsecp256k1 symbols.

Bump version to v0.17.0

18c511f

stevenroose dismissed stale reviews from apoelstra and elichai via 18c511f November 27, 2019 21:04

stevenroose force-pushed the sys branch from fdaf15a to 18c511f Compare November 27, 2019 21:04

elichai approved these changes Dec 2, 2019

View reviewed changes

apoelstra approved these changes Dec 5, 2019

View reviewed changes

apoelstra merged commit e3aaf00 into rust-bitcoin:master Dec 5, 2019

apoelstra mentioned this pull request Dec 10, 2019

add c_int return type to EcdhHashFn signature, pub for types mod #161

Closed

elichai mentioned this pull request Feb 9, 2020

Fix typos in the external-symbols feature(bug) #199

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add secp256k1-sys with prefixed sources #169

Add secp256k1-sys with prefixed sources #169

stevenroose commented Oct 21, 2019

elichai commented Oct 21, 2019

stevenroose commented Oct 23, 2019

jonasnick left a comment

jonasnick Oct 24, 2019

stevenroose Oct 30, 2019

elichai commented Oct 24, 2019

elichai Oct 24, 2019 •

edited

Loading

stevenroose Oct 25, 2019

elichai Oct 24, 2019

stevenroose Oct 25, 2019

jonasnick commented Oct 24, 2019 •

edited

Loading

elichai commented Oct 24, 2019

jonasnick commented Oct 24, 2019

stevenroose commented Oct 25, 2019

real-or-random left a comment

real-or-random commented Oct 29, 2019

elichai commented Oct 29, 2019 •

edited

Loading

TheBlueMatt Oct 29, 2019

stevenroose Oct 29, 2019

apoelstra Nov 5, 2019

stevenroose commented Oct 29, 2019

apoelstra commented Nov 6, 2019

stevenroose commented Nov 12, 2019

elichai Nov 12, 2019

stevenroose Nov 12, 2019

stevenroose Nov 12, 2019

elichai Nov 12, 2019

elichai left a comment

elichai left a comment

elichai left a comment

apoelstra left a comment

real-or-random commented Nov 27, 2019

elichai left a comment

Add secp256k1-sys with prefixed sources #169

Add secp256k1-sys with prefixed sources #169

Conversation

stevenroose commented Oct 21, 2019

elichai commented Oct 21, 2019

stevenroose commented Oct 23, 2019

jonasnick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elichai commented Oct 24, 2019

elichai Oct 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonasnick commented Oct 24, 2019 • edited Loading

elichai commented Oct 24, 2019

jonasnick commented Oct 24, 2019

stevenroose commented Oct 25, 2019

real-or-random left a comment

Choose a reason for hiding this comment

real-or-random commented Oct 29, 2019

elichai commented Oct 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenroose commented Oct 29, 2019

apoelstra commented Nov 6, 2019

stevenroose commented Nov 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elichai left a comment

Choose a reason for hiding this comment

elichai left a comment

Choose a reason for hiding this comment

elichai left a comment

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

real-or-random commented Nov 27, 2019

elichai left a comment

Choose a reason for hiding this comment

elichai Oct 24, 2019 •

edited

Loading

jonasnick commented Oct 24, 2019 •

edited

Loading

elichai commented Oct 29, 2019 •

edited

Loading