Link dynamically with system-provided libsecp256k1 instead of bundling

[jirutka]

The secp256k1 readme says:

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.

The mentioned config option just disables symbols renaming, it doesn’t affect the build script. And the links attribute in Cargo.toml defines a non-standard name rustsecp256k1_v0_8_1. Should I build it with the following in .cargo/config.toml or how?

[target.<target>]
rustsecp256k1_v0_8_1 = { rustc-link-lib = ["secp256k1"] }

Related issues and PRs: #380 #189

[apoelstra]

What is a "standard" links attribute value?

And I would strongly strongly discourage anybody from trying to link this library against whatever version of libsecp256k1 happens to exist on their system. libsecp256k1 only had a real release for the first time this year, and prior to that, distributions would just bundle random git commits from the repository with no input or approval from the libsecp maintainers.

At some point we should do a survey and see if all the major distributions have switched to bundling released versions of the library, and then we could investigate this. But we'd still have to handle the case that the library isn't present (or is the wrong version) which cargo can't affect.

[jirutka]

What is a "standard" links attribute value?

It should match the base name of the (native) library (without lib prefix and .so*) under which it’s normally installed on the system; that’s secp256k1. Using the name of the rust binding instead of the linked library quite negates the point of this attribute.

And I would strongly strongly discourage anybody from trying to link this library against whatever version of libsecp256k1 happens to exist on their system.

This is used mainly by package maintainers in Linux distributions. They have control over what version of the library it’s linked with, so it’s not just whatever version.

libsecp256k1 only had a real release for the first time this year, and prior to that, distributions would just bundle random git commits from the repository with no input or approval from the libsecp maintainers.

Well, that’s very inconvenient, but fortunately, it’s in the past; now they do releases and declare SONAME (and hopefully understand ABI compatibility…). BTW, please don’t blame the package maintainers for picking up random commits from the repository when there were no releases for such a long time, so the libsecp maintainers gave them no choice.

At some point we should do a survey and see if all the major distributions have switched to bundling released versions of the library

You can see it here: https://repology.org/project/libsecp256k1/versions.

Alpine Linux already packages released versions of libsecp256k1, so we could link rust-secp256k1 against shared system library instead of bundling it (which is very undesirable, especially in the case of crypto libraries).

[apoelstra]

Ok, thanks for explaining. I'd like to figure this out, but it's difficult because crates.io/cargo don't give us a lot of great options.

Using the name of the rust binding instead of the linked library quite negates the point of this attribute.

If we then had two major versions of the bindings, which both linked to the same underlying C code (or at least, two versions with the same soname), then it would be impossible for both to exist in the same cargo tree, breaking compilation for people with certain dependency combinations. (You can see this in the discussion of #189, which you linked, and related issues.)

AFAICT preventing this is the point of the link name, and us sticking the Rust binding version number into there accomplishes it.

This is used mainly by package maintainers in Linux distributions. They have control over what version of the library it’s linked with, so it’s not just whatever version.

Ah, ok, and I guess here is where it gets frustrating that we have an ad-hoc-looking name in link.

So, unfortunately the crates.io ecosystem, which we are more-or-less forced to support, makes it difficult for use to simultaneously support Debian maintainers. In particular:

• If we use a standard link field then this limits our ability to update the bindings, unless we ensure that we only make major API changes alongside soname-changing changes to upstream. This library is pretty mature, so this wouldn't be the end of the world.... though e.g. Context randomization tracking issue #388/Wasm, Contexts, and Entropy #346 is likely to be a breaking change that doesn't affect upstream.
• If we make it "too easy" to let people link to their system libsecp256k1 rather than the bundled one, I fear that people will do this by accident and then encounter UB when the APIs mismatch. I feel that --all-features doing this would be "too easy" so I don't want to use a cargo feature. And selfishly, I want to be able to use --all-features locally while testing, and for this to be a "normal" configuration.
• Having said this, if we were to just add a panic-on-startup check which checks the SONAME against what we expect, maybe things would be manageable. We could even plausibly support multiple versions of upstream at once.
• We could add a feature to use the bundled library, which would be on by default, but I think this wouldn't help you, because basically everybody depending on rust-secp would leave it on.
• I'm happy to add a cfg flag or something to help you, would you would need to set directly with RUSTFLAGS=--cfg=whatever, which I'd assume is already part of your workflow. But you'd need to actively set this flag for every dependent of rust-secp, or else it would use the bundled libsecp, which is a silent (and potentially dangerous) failure mode.

BTW, please don’t blame the package maintainers for picking up random commits from the repository when there were no releases for such a long time, so the libsecp maintainers gave them no choice.

From the libsecp maintainers' perspective, libsecp was an internal component of Bitcoin Core (which did, and always will, bundle its own vendored copy), which we discouraged people to use outside of Core. I appreciate that when people did do this nonetheless, package maintainers were put in a difficult position, but it was also an unfair burden on us to need to commit to a well-defined ABI when we were still iterating on major parts of the library internals.

[apoelstra]

@jirutka can you describe what you would like to happen? e.g. if we changed the build script to just disable building the bundled library when rust_secp_no_symbol_renaming is set, would that be sufficient?

Knowing what you need would help me figure out how to do it in the least-disruptive way to cargo users.

[Kixunil]

Since I recently asked a similar question (#657 ) I went into the rabbit hole of understanding linking in Rust.

According to the documentation overriding build scripts should be the right way to link to system libraries. However, cargo/rustc does not provide any protection mechanism to prevent accidental linkage against a wrong version. I'm not sure how bad this is.

Also I think links key should really have value secp256k1 when renaming is not used. We can't change it dynamically though.

I suspect the correct way of doing things would be to compile and statically link the code inside the non-sys crate and only when secp256k1-sys feature is on skip this and use that one. This at least keeps existing use case of linking multiple versions of the library working but also support globally move over to dynamically linked version.

The -sys build script can have a compile-time check of library version (by soname or whatever) but if anyone tediously overrides it with .cargo/config they're out of luck.

Just one thing that annoys me is the inability to prevent cc from compiling if dynamic linking is used.

Side note: it might be nicer to use bindgen but that would completely disable overriding of build scripts.

[apoelstra]

bindgen would produce code that was less readable and yet would require much more careful review because no human was in the loop.

As for linking, I'm not quite sure what you're proposing. It sounds like:

• If the secp256k1-sys feature is on, use the static library.
• Otherwise, if the correct version of secp256k1 is available to link against dynamically, do that.
• Otherwise, fall back to the static library.

We should also do some sort of startup check in the library to check that the correct version of secp256k1 is available at runtime, though (I think) there aren't any non-malicious colliding SONAMEs out there anymore. And of course there's nothing we can do about malicious ones.

Just one thing that annoys me is the inability to prevent cc from compiling if dynamic linking is used.

In theory we could make the cc dep depend on the secp256k1-sys feaature and feature-gate all our calls to it? But I think that, even if the feature is turned off, we should fall back to static linking so that the library continues to work.

[Kixunil]

much more careful review because no human was in the loop.

It'd require less review because there's no human. Humans make more mistakes than machines.

Otherwise, fall back to the static library.

No such fallback, -sys means "please link to system library".

But I got confused and used "static library" to mean vendored and "dynamic" meaning system library. There's .a in libsecp256k1-dev on Debian.

We can't feature gate cc because it needs to be on by default to enable building and it's impossible to turn off dependencies based on features. But there's another way: have secp256k1-vendored which we will depend on (can still be optional, on-by-default) but will be easy to patch-away using a dummy (empty) crate if users want to avoid dealing with it without going around and asking all maintainers to modify features.

[apoelstra]

No such fallback, -sys means "please link to system library".

Oh, I see. This sounds good, though it's confusing because the static library is called -sys.

But there's another way: have secp256k1-vendored which we will depend on (can still be optional, on-by-default) but will be easy to patch-away using a dummy (empty) crate if users want to avoid dealing with it without going around and asking all maintainers to modify

This sounds good, though I'm not sure exactly what it would look like.