Lazy implementation of semver trick for 0.15

[Kixunil]

Hello, I'm unable to create a PR against non-existing branch, so I'm opening this as an issue.

I managed to implement version 0.15.6 in terms of using secp256k1-sys. This is very useful because it avoids linking the C library twice, creating link errors. Such errors prevented me from building lnp-node. It compiles fine when my change is used (tested via patch in Cargo.toml). CC @dr-orlovsky, you probably want to know about this. :)

All tests (except fuzztarget) for secp256k1 are passing of course. I guess fuzztarget is something special used for fuzzing only, so it doesn't matter that it's not passing, right?

It'd be nice if you created a new branch (maybe without the -lazy-semver-trick suffix) in your repo with my changes and published the 0.15.6 crate to crates.io. I created two commits expecting that it'll make code review easier, but feel free to squash it, if you want. (I wrote the commit message to be easy to modify. :))

See the description in Kixunil@10a33cc to understand what I mean by "lazy" :)

I hope you will find this helpful!

[elichai]

Hmm That's cool,
But sadly it is still a breaking change, but not in a rust way. this removes the libsecp symbols from 0.15.* meaning that if someone used the library for C linkage it will break linking.

An example is rust-bitcoinconsensus at version 0.17.1: https://github.com/rust-bitcoin/rust-bitcoinconsensus/blob/6fa2f9e9d0df0e311432c3b81ee5afae3b82ab81/Cargo.toml

[Kixunil]

Ah, good catch! Didn't notice there are essentially hidden public items. So I guess we would have to reimplement them? Or is there any workaround for it?

[Kixunil]

Ah, I see now where's the problem, it's just that prefixes changed. Forwarding those symbols somehow should work, I think.

[elichai]

You can't fix it without causing the same linking issues you're trying to avoid(multiple definitions of the same symbol)
Although maybe through exposing the functions as weak symbols? hmm

EDIT: It seems like it isn't possible to define weak symbols in stable rust rust-lang/rust#29603

[Kixunil]

Shouldn't be an issue because the library will be included only once. Or is there any other instance of a different library defining those same symbols?

[elichai]

I managed to implement version 0.15.6 in terms of using secp256k1-sys. This is very useful because it avoids linking the C library twice, creating link errors.

I think I might've misunderstood your original problem, are you trying to avoid linking twice to the C library because of binary sizes, or are there actual linking errors? (and rust style errors or C linking errors?)

[Kixunil]

lnp-node depends on two crates, each of them depending on a different version of secp25k1, each of them attempting to link to a C library with the same symbols. By implementing 0.15.6in terms ofsecp256k1-sys`, there will be only one C library to link to, so this error goes away. I already tested that this works.

[Kixunil]

I managed to fix it! 🎉

The latest commit fixes bitcoinconsensus and does not break lnp-node.

Would be nice if someone looked at it to see if the change is sound (no UB), but at least the tests passed.

[apoelstra]

I really like this in principle but am having trouble deciding whether this would constitute a breaking change (and if it did, whether it's one so edge-casey or work-around-able that it's worth improving ergonomics for old versions of the library).

[Kixunil]

Worst case we could just revert all the changes, release a new version and tell all projects that have double-linking problems to search for another solution, right?

[apoelstra]

Heh, yes, in principle we can always "revert if somebody complains" but if we break things for downstream users, that still causes them pain for a day or two, and if they don't think/don't want to complain, maybe we won't ever fix it.

[TheBlueMatt]

I guess the other question is what are the two dependencies that depend on stale libsecps? I presume one of them is RL, which hopefully we can fix soon, but is that enough to resolve the issue a different way (especially given this shouldn’t be an issue again, it only is for past versions?).

…

On Apr 18, 2020, at 09:01, Andrew Poelstra ***@***.***> wrote:  Heh, yes, in principle we can always "revert if somebody complains" but if we break things for downstream users, that still causes them pain for a day or two, and if they don't think/don't want to complain, maybe we won't ever fix it. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[Kixunil]

Yeah, would be better to find out who exactly uses it and how, to know if the trick works reliably enough. Wouldn't want to recklessly break stuff. Just if we do everything we can to avoid breakage without stagnating and it breaks anyway, we have a way out.

Rust lightning is the stale dependency that causes lnp-node to break, so upgrading it there would resolve the linking issue too. There's no other stale dependency, because if it was stale too, it wouldn't have duplicate symbols.

There's also the grin version, but looks like it doesn't conflict for some reason. IDK how it's possible.

If the decision ends up "don't release semver trick version", then maybe mentioning that it exists and is usable via [patch....] somewhere in the docs might still be helpful.