Please expose compile time and runtime libwally versions

[rustyrussell]

See ElementsProject/lightning#6581

Can go for either major (ABI) and minor (non-ABI fixes), or a single version, but we need both a compile-time constant and a runtime check.

See https://www.sqlite.org/c3ref/libversion.html for inspiration.

[wtogami]

You mean compile time would end up within the binary?

I suppose that would be compatible with reproducible builds because they have frozen artificial build times.

[jgriffiths]

@rustyrussell Note that the version number alone is not a sufficient check, you also need to verify that wally was/wasn't built with elements support via wally_is_elements_build since that changes the ABI.

[rustyrussell]

Good point! Please make sure you document the correct way to match this, too

[jgriffiths]

Good point! Please make sure you document the correct way to match this, too

@rustyrussell There's nothing to document further I don't think. If you include wally includes with BUILD_ELEMENTS defined, then wally_is_elements_build must return non-zero at run time. The converse also holds true.

At this stage I'm thinking of providing major/minor/version as compile time constants and at runtime through a call. Along with the existing wally_is_elements_build that will allow you to match released versions against what you expect.

Additionally I will release v1.0.0 and from that point adhere to "no ABI changes without a major version bump" and remove the readme advice about ABI changes.

However, if you are using non-released versions (i.e. master) then things get more complicated, particularly since you may be using a wally that was built with a non-standard libsecp256k1 even if wallys major/minor/patch versions match. Or it may have ABI changes that aren't yet reflected in the version number being increased (because it hasn't been released).

Currently BUILD_MINIMAL for embedded targets disables e.g. support for non-English languages for mnemonics/bip85. I expect once error handling is improved that the error messages (but not the codes) will be compiled out for minimal builds as well. There is currently no way to determine this at runtime except to try to call some functionality that BUILD_MINIMAL excludes and note the failure.

I'm unsure of the context of the error originally reported since I'm not on Telegram. But wally was not initially intended to be installed as a system wide (C) library. For languages where it is used as such (e.g. Python wheels and Javascript packages), we always build non-minimal, with elements support. Any C shared library global install should IMO do the same, but I do not know if e.g. Gentoo (which has a globally installable wally C library package) adheres to this.

[whitslack]

we always build non-minimal, with elements support. Any C shared library global install should IMO do the same, but I do not know if e.g. Gentoo (which has a globally installable wally C library package) adheres to this.

net-libs/libwally-core has a user-configurable elements USE flag that controls whether --disable-elements or --enable-elements is passed to configure. Dependent packages that need Elements support must depend on net-libs/libwally-core[elements], and dependent packages that need no Elements support must depend on net-libs/libwally-core[-elements]. The unfortunate upshot of this situation has been that it is not possible to have Elements-requiring libwally-core dependents installed on the same system as no-Elements-requiring libwally-core dependents since the ABI has historically been incompatible in both directions. The recent PR should address this by enabling dependent packages that do not need Elements support to work with an installed libwally-core that has Elements support enabled. Thus, there will no longer be a need for packages to depend explicitly on net-libs/libwally-core[-elements], and the elements USE flag of net-libs/libwally-core will be enabled by default to reflect the new configure default of --enable-elements.

I see no need to expose the esoteric new --disable-elements-abi option as a USE flag, as it's a foot-gun with no appreciable value on a Gentoo system.

[jgriffiths]

Thanks for the further explanation @whitslack. Agreed that the ABI flag is not required for the distro use case.

edit: Its possibly worth clarifying that:

dependent packages that need no Elements support

Is not really a thing (not for technical reasons; perhaps for adhering to a sense of maximalist purity). Wally with Elements support is strictly additive to wally without it: all non-Elements/BTC functionality works exactly the same regardless of whether Elements support is enabled or not. If there is any app that works when compiled against wally-without-Elements, but does not when compiled against wally-with-Elements, that is a bug either in the app or wally itself and would ideally be reported to be fixed.

[whitslack]

Wally with Elements support is strictly additive to wally without it: all non-Elements/BTC functionality works exactly the same regardless of whether Elements support is enabled or not.

I can think of a (contrived) example where this is not quite so. Suppose an application is linked against a library that in turn links against libsecp256k1.so. The application also links against libwallycore.so. If libwally-core is built with Elements support, thenlibwallycore.so must be linked against libsecp256k1_zkp.so. So now the application transitively links against both libsecp256k1.so and libsecp256k1_zkp.so. Depending on the order in which the dynamic linker loads the libraries, symbols that are common to both versions of libsecp256k1 may be resolved to either one library or the other. If there are incompatible structure definitions between the two libraries or some functions differ in their ABI or behavior, then this could lead to undefined behavior at runtime. Conversely, if libwally-core is built without Elements support and also with --enable-standard-secp, then libwallycore.so can/will be linked against libsecp256k1.so, and there will be no chance for conflict in the application. Of course, this example is contrived, but it demonstrates the possibility of a scenario wherein a no-Elements application could be potentially harmed by an Elements-enabled libwally-core.

It sure would be nice if we didn't have multiple forks of libsecp256k1 in active use. 😕

[jgriffiths]

If libwally-core is built with Elements support, then libwallycore.so must be linked against libsecp256k1_zkp.so

As it stands wally expects to link with secp statically and include its symbols in its own .so file. You can't currently build an Elements-supporting wally with a non-zkp secp. Its expected that apps linking wally will take their secp symbols from linking with wally and not link to secp directly (see e.g. c-lightning).

So the case you describe can only happen with your proposed change to allow un-bundling of libsecp I think. The zkp secp variant is strictly additive to the non-zkp branch in the same way that Elements-wally is to non-Elements.

I'd like to see the patch(es) you make to c-lightning for linking it explicitly against secp I think (particularly the proposed 1.0.0 branch with your external secp changes applied.

[whitslack]

As it stands wally expects to link with secp statically and include its symbols in its own .so file.

Right, and that stands in contrast to the Gentoo philosophy, as then applications linking against libwally-core are stuck with whatever version of libsecp256k1 was embedded into libwally-core and cannot benefit from bug fixes and security patches coming from newer libsecp256k1 releases. This is the biggest reason why static linking is evil and why I strive so hard to eradicate this anti-pattern from the packages that I maintain.

The zkp secp variant is strictly additive to the non-zkp branch in the same way that Elements-wally is to non-Elements.

That's the intent, yes, but the reality diverges since libsecp256k1 and libsecp256k1_zkp are separate projects with independent release schedules, and the latter usually lags the former w.r.t. bug fixes and security patches. Their diverging release version numbering makes it impossible to tell at a glance whether any particular feature of the parent project has been incorporated into the fork.

I'd like to see the patch(es) you make to c-lightning for linking it explicitly against secp I think (particularly the proposed 1.0.0 branch with your external secp changes applied.

There aren't any patches. Core Lightning gets built with these two Make variables overridden:

EXTERNAL_INCLUDE_FLAGS="-I external/jsmn/ -I external/gheap/ $("$(tc-getPKG_CONFIG)" --cflags libsodium wallycore libsecp256k1_zkp)"
EXTERNAL_LDLIBS="external/build-${CHOST}/libjsmn.a $("$(tc-getPKG_CONFIG)" --libs libsodium wallycore libsecp256k1_zkp) -lbacktrace"

As I recall, this was done because it wasn't always the case that C-Lightning needed an Elements-enabled libwally-core, but it did always need some zkp-specific functionality from libsecp256k1. I have not tested building CLN without explicitly linking against libsecp256k1_zkp (and relying on the transitive link through libwally-core). Maybe it would be worth revisiting.

For what it's worth, libwally-core on Gentoo has always dynamically linked with libsecp256k1[_zkp]. I simply submitted what we've been doing all along (with some major improvements) as a PR.

Also, for what it's worth, libwally-core on Gentoo, if the python USE flag is enabled, patches setup.py so as to dynamically link the Python module's native shared library to libwallycore.so (and, transitively, to libsecp256k1_zkp.so) rather than duplicating all that code.

[jgriffiths]

contrast to the Gentoo philosophy,

Don't get me wrong, I understand the rationale and appreciate it, I just want to be clear about the existing behaviour of master vs proposed/upcoming changes. I'm happy to accommodate building in this way and would prefer to have wally support it directly rather than you having to carry a ton of patches to build it.

[jgriffiths]

OK master has the version changes so I'm closing this as they will be available in the v1.0 release along with a bunch of other ABI changes.

@whitslack If there are any further build changes you'd like reviewed/considered (other than bumping the DSO version to 1.0.0 for the 1.0 release) please PR them against master, thanks.