Initial packaging

[luke-jr]

This seems sufficient to get it up and building on x86_32 Gentoo. All tests pass (although these build scripts do not at this time build/run the tests).

[jonasschnelli]

No package 'libsecp256k1' found
Would adding libsecp256k1 as subtree make sense?

Or is installing libsecp256k1 be the way to go (is there a stable ABI / release from libsecp256k1?)

[luke-jr]

Subtrees are never proper packaging for libraries.

[voisine]

@luke-jr can you explain that one to me? Using a submodule lets you specify a specific commit to pull from the subtree, and it also lets you make sure it gets built from source. My perspective is that of a mobile app developer, so I'm probably not aware of all the downsides.

[jonasschnelli]

I think with proper packaging he means that he wants the capability to re-use a system wide installed library instead of using a static linked library included over a subtree.

IMO for the current stage where libsecp256k1 is in, a subtree seems to be fine.

utACK on this PR. I think it is very useful and probably save a lot of time for people trying to compile this "library"

[luke-jr]

@voisine If I am running Bitcoin Knots and BreadWallet on the same system, libsecp256k1 should only be loaded into memory once and shared between the two. This is accomplished with a single shared library that both projects link to.

Bitcoin Core currently uses a subtree to bundle libsecp256k1 because it is consensus-critical, but this is more of a necessary-evil than a good practice. BreadWallet-Core is not consensus-critical code, so it has no excuse to do such bundling.

[jonasschnelli]

Tested ACK (together with system wide install of libsecp256k1 with enabled recovery module).

[jonasschnelli]

Although, tests didn't compile with make test.
I had to change a bit:

diff --git a/Makefile.am b/Makefile.am
index 13b26b2..f152be1 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -52,3 +52,10 @@ pkgconfig_DATA = libbreadwallet-core.pc

 dist_noinst_SCRIPTS = autogen.sh
 dist_doc_DATA = LICENSE README.md
+
+noinst_PROGRAMS = test
+test_SOURCES = test.c
+test_CFLAGS = $(libbreadwallet_core_la_CFLAGS) $(libsecp256k1_CFLAGS) $(PTHREAD_CFLAGS)
+test_LDADD = libbreadwallet-core.la
+test_LDFLAGS = -static
+TESTS = test

[luke-jr]

Yes, as I said at the top, this doesn't include the test building. :)

Happy to add that commit if it works - can you do a git format-patch or push it somewhere?

[voisine]

@luke-jr Well, the benefit of dynamically linked libraries is reduced overall system memory usage. Is this the only one?

The disadvantages are less control over the specific version being linked, additional complexity of external dependencies in general, potential security issues since you don't necessarily know the circumstances under which the external library was built, etc...

For really large libraries I can see why you don't want multiple copies in memory, but this one is really small and really security critical, so I'm inclined to think the tradeoff isn't worth it.

My policy has always been to eliminate as many dependencies as possible. I even did a #include of secp256k1.c directly in BRKey.c so you don't have to do anything to build and link it. This also enforces keeping all references to it in one place.

[luke-jr]

No, there are numerous benefits to shared libraries, and generally all distros have a strict policy that mandates libraries must always be distributed in this manner and never bundled (eg, Gentoo, Debian, Fedora). Even the unusual cases (Mac and Windows) do the bundling by making it easy to include a shared library with the program's package.

Bundling doesn't eliminate dependencies, just obfuscates them.

[dgenr8]

I ran into trouble building a shared libsecp256k1 under cygwin, but thought I'd report that after fixing up the includes in BRKey.c and BRKey.h all tests passed after the following

git submodule init
git submodule update
cd secp256k1
./autogen.sh
./configure --enable-module-recovery
make
cd ..
gcc *.c secp256k1/src/libsecp256k1_la-secp256k1.o -lgmp -o test

[voisine]

Ok, I agree that bundling doesn't eliminate the dependency. Having tighter control over the dependencies you can't eliminate does still seem like a good idea.

It occurs to me that one negative issue with static linking is that the end user can't then update the library independently if a security issue is found, but also the converse is true, it's not as easy to enforce a specific patch level.