Only build bundled symbols if the user wants to use them

[str4d]

The secp256k1-sys README currently says:

Linking to external symbols

If you want to compile this library without using the bundled symbols (which may
be required for integration into other build systems), you can do so by adding
--cfg=rust_secp_no_symbol_renaming' to your RUSTFLAGS variable.

However, all this does is not use the bundled symbols by not renaming the Rust references to them. It doesn't prevent those symbols from being built, which becomes pointless work in this user-configured context.

Instead, the build.rs could check RUSTFLAGS for the presence of --cfg=rust_secp_no_symbol_renaming', and simply exit if it is present, since that is a clear indication from the user that they do not want these bundled symbols.

As a stretch goal, having this be controlled by a feature flag would enable the cc dependency to be dropped as well if not using the bundled symbols, but cc is a relatively small and self-contained dependency, so that's less of a problem.

[TheBlueMatt]

I think we want this to be a separate cfg flag - users may want to use the built-in library with an external implementation, but, yes, we should support this.

[apoelstra]

This flag exists for rust-secp257k1-zkp, right? Are there other users?

I think for secp-zkp @str4d may be correct that we're better off just not building included libsecp at all.

[elichai]

Do we want to support this though? I remember in the past we made some decisions here due to the fact that we control the exact version of libsecp

EDIT: I'm not saying we shouldn't, just that if we do we need to make sure we don't have anything that assumes implementation-defined details of libsecp

[TheBlueMatt]

IIRC part of the reason for this was to be able to link rust-bitcoin as a part of Bitcoin Core (which of course includes secp256k1). Sadly that work never progressed, so I'm not sure it has a user currently.

[apoelstra]

We could remove it in a RC of the next major version rev, and see if it breaks rust-bitcoinconsensus or rust-secp-zkp.

[str4d]

IIRC part of the reason for this was to be able to link rust-bitcoin as a part of Bitcoin Core (which of course includes secp256k1). Sadly that work never progressed, so I'm not sure it has a user currently.

This is in fact almost precisely my new use case. zcashd (based on Bitcoin Core) has a large Rust component, and until now that's only handled shielded parts of the protocol. But we've just started moving some of the address parsing logic into Rust, and need to validate secp256k1 compressed point encodings. I'm now using the secp256k1 crate with this flag to ensure that we consistently use the libsecp256k1 code vendored in the main repo; this issue was for optimising the resulting build flow.

[apoelstra]

Thanks :) ok we won't remove it. Let's give your "don't build libsecp" proposa a shot.

I wonder why we didn't try using this for secp-zkp rather than doing the crazy symbol-renaming thing that we did do..

[elichai]

I wonder why we didn't try using this for secp-zkp rather than doing the crazy symbol-renaming thing that we did do..

That fixes other problems too, let's say I'm using rust-secp256k1 regularly, but in my dependency tree it appears twice with 2 separate major versions (or once as rust-secp and the other as rust-secp-zkp)

[real-or-random]

I wonder why we didn't try using this for secp-zkp rather than doing the crazy symbol-renaming thing that we did do..

cc @jonasnick, you may know the answer here.

[apoelstra]

IIRC the secp-zkp solution pre-dated the rust-bitcoinconsensus solution ... we had originally intended to use renaming for rust-bitcoinconsensus, but our initial renaming solution was really shitty and didn't work (and this was at the time when Tamas was maintaining rust-bitcoinconsensus but no longer really active). Then Matt came up with this no-symbol-export solution, and then independently Elichai un-shittified the renaming script.

But my memory could be wrong on every one of those points :).

[thomaseizinger]

This flag exists for rust-secp257k1-zkp, right?

Not that I am aware.

In rust-secp256k1-zkp, we actually rely on the fact the contexts of libsecp256k1 and libsecp256k1zkp are the same IF we depend on a version of the libsecp256k1zkp code that was updated to the corresponding upstream version of libsecp256k1. Quite horrendous and results in an entire copy of the library that is effectively unused but it works 😅

Here for example we pass a Context that actually comes from secpk256k1-sys and thus libsecp256k1.

We did this so that rust-secp256k1-zkp essentially becomes a drop-in replacement for rust-secp256k1 and you can use the generated keys and everything with APIs in rust-bitcoin etc.

See discussions here and here.

[thomaseizinger]

entire copy of the library that is effectively unused

Actually not 100% about this. We don't create any bindings for it so I guess it depends on LTO doing its job to figure out that this C code is unused.

[apoelstra]

Unfortunately it is not unused. It is used by rust-secp, and we take context objects created by libsecp256k1 (through rust-secp) then pass them to libsecp256k1-zkp functions, which is questionably-defined behavior.

If we could just avoid complinig and using libsecp256k1 in rust-secp-zkp that would be a huge improvement.

[thomaseizinger]

Unfortunately it is not unused.

I was referring to the copy of libsecp256k1zkp that is mostly unused. (rust-secp256k1-zkp has its own depend directory with a copy of the vendor script, etc)
All the functions defined in there (duplicated from libsecp256k1) plus the static contexts etc.