Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmake: Build libbitcoinconsensus library #41

Closed
wants to merge 58 commits into from

Conversation

hebasto
Copy link
Owner

@hebasto hebasto commented Oct 29, 2023

New CMake variables that affect the build system configuration:

  • BUILD_BITCOINCONSENSUS_LIB (Bitcoin Core specific)
  • BUILD_SHARED_LIBS (see CMake docs)

This PR uses the same approach as the secp256k1 repo does, which differs from the current one in the master branch.

Also see:

@hebasto
Copy link
Owner Author

hebasto commented Oct 29, 2023

@hebasto
Copy link
Owner Author

hebasto commented Oct 29, 2023

Examples of usage:

  1. Cross-compiling a debug build for Windows:
$ make -j $(nproc) -C depends HOST=x86_64-w64-mingw32 NO_QT=1 DEBUG=1
$ mkdir build
$ cd build
$ cmake -S .. --toolchain depends/x86_64-w64-mingw32/share/toolchain.cmake -DBUILD_SHARED_LIBS=ON -DREDUCE_EXPORTS=ON -DCMAKE_BUILD_TYPE=Debug
$ cmake --build . -j $(nproc)
$ ctest -j $(nproc)
$ file src/script/libbitcoinconsensus.dll
src/script/libbitcoinconsensus.dll: PE32+ executable (DLL) (console) x86-64, for MS Windows
  1. Native compiling on Windows:
>mkdir build
>cd build
>cmake -S .. -A x64 -DCMAKE_TOOLCHAIN_FILE=C:/vcpkg/scripts/buildsystems/vcpkg.cmake -DVCPKG_TARGET_TRIPLET=x64-windows-static -DBUILD_SHARED_LIBS=ON
>cmake --build . -j 8 --config Release
>ctest -C Release -j 8
>dir /B src\script\Release\bitcoinconsensus.dll
bitcoinconsensus.dll
  1. Native compiling on macOS:
% mkdir build
% cd build
% cmake -S .. -DBUILD_SHARED_LIBS=ON -DREDUCE_EXPORTS=ON
% cmake --build . -j 8 -t bitcoinconsensus
% file src/script/libbitcoinconsensus.dylib 
src/script/libbitcoinconsensus.dylib: Mach-O 64-bit dynamically linked shared library arm64

@ajtowns
Copy link

ajtowns commented Oct 30, 2023

Nice. Couple of things that bugged me (not related to this patchset, I think) when doing a quick test:

  • updating from an old cmake-staging state gave me a "Position independent code disabled" error, with no obvious way to fix it. going to a fresh git checkout and starting from scratch fixed it, so that's fine, but maybe something better can be done?
  • cmake --build ../build -j40 seems to build targets in sequence, so repeatedly spends a bunch of time with many cores relatively idle as the last few modules for a target get built; would be nice if everything was being built in parallel all the time as much as possible?

@hebasto
Copy link
Owner Author

hebasto commented Oct 30, 2023

Nice. Couple of things that bugged me (not related to this patchset, I think) when doing a quick test:

  • updating from an old cmake-staging state gave me a "Position independent code disabled" error, with no obvious way to fix it. going to a fresh git checkout and starting from scratch fixed it, so that's fine, but maybe something better can be done?

What are your system, compiler etc?

  • cmake --build ../build -j40 seems to build targets in sequence, so repeatedly spends a bunch of time with many cores relatively idle as the last few modules for a target get built; would be nice if everything was being built in parallel all the time as much as possible?

Could you try the Ninja generator by adding -G "Ninja" option during configuring / generating a build system?

UPD. I think that following the target dependency graph is an inherent behaviour.

@ajtowns
Copy link

ajtowns commented Oct 30, 2023

Nice. Couple of things that bugged me (not related to this patchset, I think) when doing a quick test:

  • updating from an old cmake-staging state gave me a "Position independent code disabled" error, with no obvious way to fix it. going to a fresh git checkout and starting from scratch fixed it, so that's fine, but maybe something better can be done?

What are your system, compiler etc?

Debian, gcc 13.2. Reproducer for me is:

$ git checkout cmake31.03
$ git clean -dxf
$ mkdir build; cd build; cmake -S .. -B . -DWARN_INCOMPATIBLE_BDB=OFF
$ cmake --build .
$ git checkout cmake41.01
$ ccmake .  # configure, WERROR=on, configure -> Position independent code disabled
  • cmake --build ../build -j40 seems to build targets in sequence, so repeatedly spends a bunch of time with many cores relatively idle as the last few modules for a target get built; would be nice if everything was being built in parallel all the time as much as possible?

Could you try the Ninja generator by adding -G "Ninja" option during configuring / generating a build system?

UPD. I think that following the target dependency graph is an inherent behaviour.

That sucks :( The util/cli/wallet/tx tools seem to be build in parallel, at least. Did confirm that ninja didn't make much difference.

CMakeLists.txt Outdated
@@ -255,6 +256,11 @@ if(HARDENING)
try_append_cxx_flags("-fcf-protection=full" TARGET hardening)

if(MINGW)
add_library(link_ssp INTERFACE)
target_link_libraries(link_ssp INTERFACE
$<$<BOOL:${BUILD_SHARED_LIBS}>:ssp>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate on why this is needed (if it still is: bitcoin#28461). I'm suprised this is being added at this point, rather than in the hardening PR, where we actually started using ssp?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only DLL failed to build without linking to libssp when hardening, no?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't tested. Did it? If it doesn't fail on master with the changes in bitcoin#28461, it'd also be good to know why it fails here.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it'd also be good to know why it fails here.

Indeed. FWIW, I have objdump outputs as follow:

 objdump -x src/.libs/libbitcoinconsensus-0.dll | grep -e "DLL Name:"
	DLL Name: ADVAPI32.dll
	DLL Name: KERNEL32.dll
	DLL Name: msvcrt.dll
  • this branch:
$ objdump -x build/src/script/libbitcoinconsensus.dll | grep -e "DLL Name:"
	DLL Name: ADVAPI32.dll
	DLL Name: KERNEL32.dll
	DLL Name: msvcrt.dll

There are no traces of libssp...

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect lssp to be needed everywhere other than bleeding-edge build environments. Is it possible that bitcoind.exe without lssp links but fails to run?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect lssp to be needed everywhere...

FWIW, it was introduced with the commit message:

mingw needs libssp for hardening with dlls

:)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't tested. Did it? If it doesn't fail on master with the changes in bitcoin#28461, it'd also be good to know why it fails here.

Resolved in the recent push.


function(make_bitcoinconsensus_dll_available target)
if(WIN32 AND BUILD_SHARED_LIBS)
# The DLL must reside either in the same folder where the executable is

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The DLL must reside either in the same folder where the executable is
or somewhere in PATH. Using the former option.

Why? Is this some CMake behaviour? Would be good to be more clear in this comment.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a Windows-specific behavior:

... the Windows operating system searches for ... DLL in the following locations in this order:

  1. The application folder

  2. The current folder

  3. The Windows system folder

  4. The Windows folder

@@ -320,6 +330,15 @@ message(" bitcoin-cli ......................... ${BUILD_CLI}")
message(" bitcoin-tx .......................... ${BUILD_TX}")
message(" bitcoin-util ........................ ${BUILD_UTIL}")
message(" bitcoin-wallet ...................... ${BUILD_WALLET_TOOL}")
message("Libraries:")
if(BUILD_SHARED_LIBS)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't this be collapsed down to just:

if(BUILD_SHARED_LIBS)
  message("  library type ........................ Shared")
else()
  message("  library type ........................ Static")
endif()

Then there's no need for an intermediate variable, and "cleaning up" afterwards? We know there's only two types of libs we are building.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Done.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implies that when BUILD_SHARED_LIBS=1 then no static libraries will be built. Is this the case? And no plan to build both in the future? I think it is normal to provide both static and shared libraries, for example:

	/usr/local/lib/libx264.a
	/usr/local/lib/libx264.so
	/usr/local/lib/libx264.so.164
...
	/usr/local/lib/libidn.a
	/usr/local/lib/libidn.so
	/usr/local/lib/libidn.so.12
	/usr/local/lib/libidn.so.12.6.3
...
	/usr/local/lib/libssh2.a
	/usr/local/lib/libssh2.so
	/usr/local/lib/libssh2.so.1
	/usr/local/lib/libssh2.so.1.0.1

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And no plan to build both in the future?

Correct. No plans to build a shared lib and a static lib simultaneously, in a single run. Please see the related discussions:

Also, you might want to look at bitcoin#28779.

@hebasto
Copy link
Owner Author

hebasto commented Oct 30, 2023

@ajtowns

What are your system, compiler etc?

Debian, gcc 13.2. Reproducer for me is:

Clearing the CMake cache, i.e., CMakeCache.txt, or the entire build directory, should help.

@hebasto hebasto force-pushed the 231029-cmake-R branch 2 times, most recently from 861e828 to 6a6a818 Compare October 31, 2023 11:23
@ajtowns
Copy link

ajtowns commented Oct 31, 2023

Clearing the CMake cache, i.e., CMakeCache.txt, or the entire build directory, should help.

Clearing the entire build directory is what I did originally; clearing CMakeCache.txt loses my config settings so doesn't seem much better than clearing the build dir, but does also seem to fix it.

@hebasto
Copy link
Owner Author

hebasto commented Oct 31, 2023

Clearing the CMake cache, i.e., CMakeCache.txt, or the entire build directory, should help.

Clearing the entire build directory is what I did originally; clearing CMakeCache.txt loses my config settings so doesn't seem much better than clearing the build dir, but does also seem to fix it.

You might want to use CMakeUserPresets.json to keep your frequently used settings. See: https://cmake.org/cmake/help/latest/manual/cmake-presets.7.html.

@hebasto
Copy link
Owner Author

hebasto commented Oct 31, 2023

Rebased.

Copy link

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

8521a38 LGTM

diff autotools vs cmake compile bitcoinconsensus
--- /tmp/autotools_compile	2023-11-03 09:40:47.597074000 +0100
+++ /tmp/cmake_compile	2023-11-03 10:57:39.073098000 +0100
@@ -1,52 +1,25 @@
--c consensus/merkle.cpp
+-c /path_to_source/bitcoin/bitcoin/src/consensus/merkle.cpp
 -DABORT_ON_FAILED_ASSUME
--DBUILD_BITCOIN_INTERNAL  # Missing in cmake on the command line but is in BUILD/build.ninja
 -DDEBUG
 -DDEBUG_LOCKCONTENTION
 -DDEBUG_LOCKORDER
--DHAVE_BUILD_INFO
 -DHAVE_CONFIG_H
--DPIC  # Why missing in cmake?
--DPROVIDE_FUZZ_MAIN_FUNCTION
 -DRPC_DOC_CHECK
 -fcf-protection=full
 -fPIC
 -fstack-clash-protection
 -fstack-protector-all
 -ftrapv
 -g3
--I.
--I../src/config
--I./obj
--I./secp256k1/include
--I/usr/local/include
+-I/path_to_source/bitcoin/bitcoin/src
+-I/path_to_source/bitcoin/bitcoin/src/secp256k1/include
+-I/path_to_build/src
 -MD
--MF consensus/.deps/libbitcoinconsensus_la-merkle.Tpo
--MP
--MT consensus/libbitcoinconsensus_la-merkle.lo
--o consensus/libbitcoinconsensus_la-merkle.o
+-MF src/CMakeFiles/bitcoin_consensus.dir/consensus/merkle.cpp.o.d
+-MT src/CMakeFiles/bitcoin_consensus.dir/consensus/merkle.cpp.o
+-o src/CMakeFiles/bitcoin_consensus.dir/consensus/merkle.cpp.o
 -O0
 -std=c++17
--Wall
--Wconditional-uninitialized
--Wdate-time
--Wdocumentation
 -Werror
--Wextra
--Wformat
--Wformat-security
--Wgnu
--Wimplicit-fallthrough
--Wloop-analysis
--Wno-self-assign
--Wno-thread-safety-reference-return
--Wno-unused-parameter
--Woverloaded-virtual
--Wredundant-decls
--Wshadow-field
 -Wstack-protector
--Wsuggest-override
--Wthread-safety
--Wunreachable-code-loop-increment
--Wunused-member-function
--Wvla
diff autotools vs cmake link bitcoinconsensus
--- /tmp/autotools_link	2023-11-03 09:43:06.238869000 +0100
+++ /tmp/cmake_link	2023-11-03 09:58:17.605383000 +0100
@@ -1,57 +1,40 @@
--DPIC
 -fPIC
--fstack-protector-all  # Hmm?
+-ftrapv                # Why this difference?
 -g3
--L/usr/lib
--L/usr/local/lib
--L/usr/local/llvm-devel/lib/clang/18/lib/x86_64-portbld-freebsd14.0
--lc
--lc++
--lgcc
--lgcc
--lgcc_s
--lgcc_s
--lm
--nostdlib
--o .libs/libbitcoinconsensus.so.0.0.0
+-o src/script/libbitcoinconsensus.so.0.0.0
 -O0
 -shared
--std=c++17
--Wl,-soname -Wl,libbitcoinconsensus.so.0
--Wl,-z -Wl,now
--Wl,-z -Wl,relro
--Wl,-z -Wl,separate-code
+-Wl,-soname,libbitcoinconsensus.so.0
+-Wl,-z,now
+-Wl,-z,relro
+-Wl,-z,separate-code
-/usr/lib/crtbeginS.o
-/usr/lib/crtendS.o
-/usr/lib/crti.o
-/usr/lib/crtn.o
-.libs/libbitcoinconsensus_la-arith_uint256.o
+src/CMakeFiles/bitcoin_consensus.dir/arith_uint256.cpp.o
-.libs/libbitcoinconsensus_la-hash.o
+src/CMakeFiles/bitcoin_consensus.dir/hash.cpp.o
-.libs/libbitcoinconsensus_la-pubkey.o
+src/CMakeFiles/bitcoin_consensus.dir/pubkey.cpp.o
-.libs/libbitcoinconsensus_la-uint256.o
+src/CMakeFiles/bitcoin_consensus.dir/uint256.cpp.o
-consensus/.libs/libbitcoinconsensus_la-merkle.o
+src/CMakeFiles/bitcoin_consensus.dir/consensus/merkle.cpp.o
-consensus/.libs/libbitcoinconsensus_la-tx_check.o
+src/CMakeFiles/bitcoin_consensus.dir/consensus/tx_check.cpp.o
-crypto/.libs/libbitcoinconsensus_la-aes.o
+src/crypto/CMakeFiles/bitcoin_crypto_base.dir/aes.cpp.o
-crypto/.libs/libbitcoinconsensus_la-chacha20.o
+src/crypto/CMakeFiles/bitcoin_crypto_base.dir/chacha20.cpp.o
-crypto/.libs/libbitcoinconsensus_la-chacha20poly1305.o
+src/crypto/CMakeFiles/bitcoin_crypto_base.dir/chacha20poly1305.cpp.o
-crypto/.libs/libbitcoinconsensus_la-hkdf_sha256_32.o
+src/crypto/CMakeFiles/bitcoin_crypto_base.dir/hkdf_sha256_32.cpp.o
-crypto/.libs/libbitcoinconsensus_la-hmac_sha256.o
+src/crypto/CMakeFiles/bitcoin_crypto_base.dir/hmac_sha256.cpp.o
-crypto/.libs/libbitcoinconsensus_la-hmac_sha512.o
+src/crypto/CMakeFiles/bitcoin_crypto_base.dir/hmac_sha512.cpp.o
-crypto/.libs/libbitcoinconsensus_la-muhash.o
+src/crypto/CMakeFiles/bitcoin_crypto_base.dir/muhash.cpp.o
-crypto/.libs/libbitcoinconsensus_la-poly1305.o
+src/crypto/CMakeFiles/bitcoin_crypto_base.dir/poly1305.cpp.o
-crypto/.libs/libbitcoinconsensus_la-ripemd160.o
+src/crypto/CMakeFiles/bitcoin_crypto_base.dir/ripemd160.cpp.o
-crypto/.libs/libbitcoinconsensus_la-sha1.o
+src/crypto/CMakeFiles/bitcoin_crypto_base.dir/sha1.cpp.o
-crypto/.libs/libbitcoinconsensus_la-sha256_sse4.o
+src/crypto/CMakeFiles/bitcoin_crypto_base.dir/sha256_sse4.cpp.o
-crypto/.libs/libbitcoinconsensus_la-sha256.o
+src/crypto/CMakeFiles/bitcoin_crypto_base.dir/sha256.cpp.o
-crypto/.libs/libbitcoinconsensus_la-sha3.o
+src/crypto/CMakeFiles/bitcoin_crypto_base.dir/sha3.cpp.o
-crypto/.libs/libbitcoinconsensus_la-sha512.o
+src/crypto/CMakeFiles/bitcoin_crypto_base.dir/sha512.cpp.o
-crypto/.libs/libbitcoinconsensus_la-siphash.o
+src/crypto/CMakeFiles/bitcoin_crypto_base.dir/siphash.cpp.o
-primitives/.libs/libbitcoinconsensus_la-block.o
+src/CMakeFiles/bitcoin_consensus.dir/primitives/block.cpp.o
-primitives/.libs/libbitcoinconsensus_la-transaction.o
+src/CMakeFiles/bitcoin_consensus.dir/primitives/transaction.cpp.o
-script/.libs/libbitcoinconsensus_la-bitcoinconsensus.o
+src/script/CMakeFiles/bitcoinconsensus.dir/bitcoinconsensus.cpp.o
-script/.libs/libbitcoinconsensus_la-interpreter.o
+src/CMakeFiles/bitcoin_consensus.dir/script/interpreter.cpp.o
-script/.libs/libbitcoinconsensus_la-script_error.o
+src/CMakeFiles/bitcoin_consensus.dir/script/script_error.cpp.o
-script/.libs/libbitcoinconsensus_la-script.o
+src/CMakeFiles/bitcoin_consensus.dir/script/script.cpp.o
-secp256k1/.libs/libsecp256k1.a
+libsecp256k1.a
-support/.libs/libbitcoinconsensus_la-cleanse.o
+src/script/CMakeFiles/bitcoinconsensus.dir/__/support/cleanse.cpp.o
-util/.libs/libbitcoinconsensus_la-strencodings.o
+src/CMakeFiles/bitcoin_consensus.dir/util/strencodings.cpp.o

On install: autotools installs both static and shared libbitcoinconsensus whereas cmake just one, depending on BUILD_SHARED_LIBS (IMO that's ok given bitcoin#28779). The installed lib/pkgconfig/libbitcoinconsensus.pc is identical between autotools and cmake.

@hebasto
Copy link
Owner Author

hebasto commented Nov 3, 2023

@vasild

Thank you for your valuable feedback. The way how this branch handles the BUILD_BITCOIN_INTERNAL macro is definitely wrong and must be fixed.

@vasild
Copy link

vasild commented Nov 3, 2023

Why? I noticed that when I added this (forgot to which source code file, edit: now I see it was crypto/sha256.cpp):

#ifdef BUILD_BITCOIN_INTERNAL
#error 123
#else    
#error 456
#endif

it failed with "456"...

@hebasto
Copy link
Owner Author

hebasto commented Nov 3, 2023

@theuni, @fanquake, @TheCharlatan

Fellow Libtool connoisseurs,

In the master branch, I've got:

$ ./configure --disable-ccache --enable-debug
$ cd src
$ make print-DEBUG_CXXFLAGS
DEBUG_CXXFLAGS=-O0 -g3 -ftrapv  # As expected.
$ make print-HARDENED_CXXFLAGS
HARDENED_CXXFLAGS=-Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection  # As expected.
$ make print-libbitcoinconsensus_la_CXXFLAGS
libbitcoinconsensus_la_CXXFLAGS=-O0 -g3 -ftrapv -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -Wall -Wextra -Wformat -Wformat-security -Wvla -Wredundant-decls -Wdate-time -Wduplicated-branches -Wduplicated-cond -Wlogical-op -Woverloaded-virtual -Wsuggest-override -Wimplicit-fallthrough -Wno-unused-parameter     -fno-extended-identifiers -fstack-reuse=none -fPIE
$ make print-libbitcoinconsensus_la_LDFLAGS
libbitcoinconsensus_la_LDFLAGS= -Wl,-z,relro -Wl,-z,now -Wl,-z,separate-code -pie     -no-undefined

When linking libbitcoinconsensus.so.0.0.0:

libtool: link: g++ -std=c++17  -fPIC -DPIC -shared -nostdlib /usr/lib/gcc/x86_64-linux-gnu/13/../../../x86_64-linux-gnu/crti.o /usr/lib/gcc/x86_64-linux-gnu/13/crtbeginS.o  support/.libs/libbitcoinconsensus_la-cleanse.o crypto/.libs/libbitcoinconsensus_la-aes.o crypto/.libs/libbitcoinconsensus_la-chacha20.o crypto/.libs/libbitcoinconsensus_la-chacha20poly1305.o crypto/.libs/libbitcoinconsensus_la-hkdf_sha256_32.o crypto/.libs/libbitcoinconsensus_la-hmac_sha256.o crypto/.libs/libbitcoinconsensus_la-hmac_sha512.o crypto/.libs/libbitcoinconsensus_la-poly1305.o crypto/.libs/libbitcoinconsensus_la-muhash.o crypto/.libs/libbitcoinconsensus_la-ripemd160.o crypto/.libs/libbitcoinconsensus_la-sha1.o crypto/.libs/libbitcoinconsensus_la-sha256.o crypto/.libs/libbitcoinconsensus_la-sha3.o crypto/.libs/libbitcoinconsensus_la-sha512.o crypto/.libs/libbitcoinconsensus_la-siphash.o crypto/.libs/libbitcoinconsensus_la-sha256_sse4.o .libs/libbitcoinconsensus_la-arith_uint256.o consensus/.libs/libbitcoinconsensus_la-merkle.o consensus/.libs/libbitcoinconsensus_la-tx_check.o .libs/libbitcoinconsensus_la-hash.o primitives/.libs/libbitcoinconsensus_la-block.o primitives/.libs/libbitcoinconsensus_la-transaction.o .libs/libbitcoinconsensus_la-pubkey.o script/.libs/libbitcoinconsensus_la-bitcoinconsensus.o script/.libs/libbitcoinconsensus_la-interpreter.o script/.libs/libbitcoinconsensus_la-script.o script/.libs/libbitcoinconsensus_la-script_error.o .libs/libbitcoinconsensus_la-uint256.o util/.libs/libbitcoinconsensus_la-strencodings.o   secp256k1/.libs/libsecp256k1.a -L/usr/lib/gcc/x86_64-linux-gnu/13 -L/usr/lib/gcc/x86_64-linux-gnu/13/../../../x86_64-linux-gnu -L/usr/lib/gcc/x86_64-linux-gnu/13/../../../../lib -L/lib/x86_64-linux-gnu -L/lib/../lib -L/usr/lib/x86_64-linux-gnu -L/usr/lib/../lib -L/usr/lib/gcc/x86_64-linux-gnu/13/../../.. -lstdc++ -lm -lgcc_s -lc -lgcc_s /usr/lib/gcc/x86_64-linux-gnu/13/crtendS.o /usr/lib/gcc/x86_64-linux-gnu/13/../../../x86_64-linux-gnu/crtn.o  -O0 -g3 -fstack-protector-all -Wl,-z -Wl,relro -Wl,-z -Wl,now -Wl,-z -Wl,separate-code   -Wl,-soname -Wl,libbitcoinconsensus.so.0 -o .libs/libbitcoinconsensus.so.0.0.0

I can see -O0 -g3 -fstack-protector-all only. Is this Libtool behaviour known / documented?

Inspired by @vasild's feedback.

@theuni
Copy link

theuni commented Nov 3, 2023

I believe this is the list: https://git.savannah.gnu.org/cgit/libtool.git/tree/build-aux/ltmain.in#n5392

I'm not 100% sure it's what make it through to the linker, it may just be the list that libtool intercepts and does something with.

But yes, that looks like it's been quietly swallowing up a list of arbitrary flags for years :(

Edit: For the most part I think this actually ends up being the correct thing to do though, as the linker doesn't need most of those flags.

@hebasto
Copy link
Owner Author

hebasto commented Nov 3, 2023

The way how this branch handles the BUILD_BITCOIN_INTERNAL macro is definitely wrong and must be fixed.

Fixed.

@hebasto hebasto force-pushed the 231029-cmake-R branch 2 times, most recently from dd0ef98 to 3106952 Compare November 4, 2023 18:00
@hebasto
Copy link
Owner Author

hebasto commented Nov 4, 2023

I've addressed the previous comments and fixed other bugs that I found during extensive testing in different build scenarios.

@fanquake
Copy link

I have a slight preference for its inclusion for easier usability. But thinking it through further... we could always break them apart in the future, but once users start using them separately they'd likely need to stay that way for the sake of back-compat. So... I guess actually have a pretty strong preference for bundling them together.

Yea, please lets not try and prematurely optimize anything here, for uses cases or users that may not even exist. Otherwise we'll end up with a stack of build complexity / options that we don't need, and just have to carry around forever.

@fanquake
Copy link

If the user downloads the release package and untar it, they might need to set the LD_LIBRARY_PATH environment variable to be able to run the test_bitcoin binary.

This doesn't really seem like a good tradeoff, or requirement to introduce.

This is fixed in 9c2fc1e.

Doesn't this just break as soon as the binaries are moved?

@theuni
Copy link

theuni commented Dec 18, 2023

#41 (comment):

Now, ... the tests can correctly link with the external libbitcoinconsensus.

It has a few consequences.

We build release binaries with -DBUILD_SHARED_LIBS=ON (see #67) to ship the libbitcoinconsensus.so as a part of the release package. It makes the test_bitcoin binary depend on libbitcoinconsensus.so.

If the user downloads the release package and untar it, they might need to set the LD_LIBRARY_PATH environment variable to be able to run the test_bitcoin binary.

Mmm, ok, I think that we should probably address this upstream. I hastily added this one-line "test" to test_bitcoin years ago, and I think it was a mistake that we should undo. There's no need to burden test_bitcoin with a link to a shared lib, as that doesn't actually test anything interesting about the use of the shared lib. A static lib would work just as well.

It's useful to demonstrate that linking against the shared lib actually works (no missing symbols, etc), but that can be done in a way that doesn't get in the way of other binaries we ship, similar to bitcoin-chainstate. I'll work on a PR for us to discuss upstream.

Disregard this, either I misremembered how this worked in master or it's changed since. Either way, the above doesn't make much sense. I'm looking at master now and taking another stab at understanding where the problem is :)

@theuni
Copy link

theuni commented Dec 18, 2023

I've been playing around with this. Here is an attempt which adds a new binary with everything built-in: theuni@2831f77

It solves all of our problems, but it's getting a bit messy. It'd be hard to justify a test_bitcoin_consensus alone, but for an eventual test_bitcoin_kernel, it makes more sense.

I can't say I love it, but it works afaics. Thoughts?

@hebasto
Copy link
Owner Author

hebasto commented Dec 19, 2023

If the user downloads the release package and untar it, they might need to set the LD_LIBRARY_PATH environment variable to be able to run the test_bitcoin binary.

This doesn't really seem like a good tradeoff, or requirement to introduce.

I have the same concerns.

This is fixed in 9c2fc1e.

Doesn't this just break as soon as the binaries are moved?

It depends. The answer is "no, it doesn't break" if the user moves/installs the library to a well-known / standard location, or moves the bin and lib directories simultaneously, which keeps the relative path ../lib meaningful from the bin directory.

@theuni
Copy link

theuni commented Dec 20, 2023

How about this: theuni@f30fb14 ?

Linking against an actual shared lib here as we do currently doesn't do us any good because we're not testing anything about the dyloader/dlopener, and we're not testing in C.

How about we just add the file that's missing? It seems overly simple, but it works and removes the paths issue altogether. And as a benefit it means that we can remove the ifdefs in the test and just let them always run.

@TheCharlatan
Copy link

Re #41 (comment)

How about we just add the file that's missing?

Did this last week in the current build system here: TheCharlatan@34d9412 . I think if that consensus file is baked in, there is little point in keeping the HAVE_CONSENSUS_LIB macro. Could PR that, so we don't diverge too much?

@theuni
Copy link

theuni commented Dec 20, 2023

Re #41 (comment)

How about we just add the file that's missing?

Did this last week in the current build system here: TheCharlatan@34d9412 . I think if that consensus file is baked in, there is little point in keeping the HAVE_CONSENSUS_LIB macro. Could PR that, so we don't diverge too much?

Heh, the variable now means the exact opposite of its name.

Concept ACK. Mind PRing that upstream for discussion, and we can bikeshed the names there?

@hebasto
Copy link
Owner Author

hebasto commented Dec 20, 2023

Did this last week in the current build system here: TheCharlatan@34d9412 . I think if that consensus file is baked in, there is little point in keeping the HAVE_CONSENSUS_LIB macro. Could PR that, so we don't diverge too much?

Heh, the variable now means the exact opposite of its name.

Concept ACK. Mind PRing that upstream for discussion, and we can bikeshed the names there?

I'll be happy to join the discussion in that PR.

Just curious about the current Autotools-based build system behaviour when it's configured with --with-libs=no.

@hebasto
Copy link
Owner Author

hebasto commented Dec 20, 2023

I'd like to clarify for myself and posterity our vision of the way how the user of the Bitcoin Core release package can run tests that are specific to the provided shared libraries, i.e., libbitcoinconsensus and upcoming libbitcoinkernel.

  1. Do we provide a testing executable for the shared libraries?
  2. What shared library functionality must be tested exactly?
  3. Are testing facilities provided by the test_bitcoin executable or there are additional dedicated testing executables?
  4. Is it acceptable for the user's experience to define the testing executable runpath to $ORIGIN/../lib?

Please let me know if you think that these questions should be cross-posted to the main repo and discussed there.

@hebasto
Copy link
Owner Author

hebasto commented Dec 20, 2023

How about this: theuni@f30fb14 ?

Linking against an actual shared lib here as we do currently doesn't do us any good because we're not testing anything about the dyloader/dlopener, and we're not testing in C.

Does it mean that libbitcoinkernel should follow the same approach?

theuni added a commit to theuni/bitcoin that referenced this pull request Jan 5, 2024
This library has existed for nearly 10 years with very little known uptake or
impact. It has become a maintenance burden. In several cases it dictates our
code/library structure (for example necessatating LIBBITCOIN_CRYPTO_BASE), as
well as build-system procedures (building multiple copies of object files
especially for the lib).

Several discussions have arisen wrt migrating it to CMake and it has become
difficult to justify adding more complexity for a library that is virtually
unused anyway.

See for example the discussions:
hebasto#41
bitcoin#29123

Instead, we (fanquake, hebasto, TheCharlatan, and I) propose simply not
migrating it to CMake and letting it end with v26. Any remaining use-cases
could be handled in the future by libbitcoinkernel.
theuni added a commit to theuni/bitcoin that referenced this pull request Jan 5, 2024
This library has existed for nearly 10 years with very little known uptake or
impact. It has become a maintenance burden. In several cases it dictates our
code/library structure (for example necessitating LIBBITCOIN_CRYPTO_BASE), as
well as build-system procedures (building multiple copies of object files
especially for the lib).

Several discussions have arisen wrt migrating it to CMake and it has become
difficult to justify adding more complexity for a library that is virtually
unused anyway.

See for example the discussions:
hebasto#41
bitcoin#29123

Instead, we (fanquake, hebasto, TheCharlatan, and I) propose simply not
migrating it to CMake and letting it end with v26. Any remaining use-cases
could be handled in the future by libbitcoinkernel.
theuni added a commit to theuni/bitcoin that referenced this pull request Jan 5, 2024
This library has existed for nearly 10 years with very little known uptake or
impact. It has become a maintenance burden. In several cases it dictates our
code/library structure (for example necessitating LIBBITCOIN_CRYPTO_BASE), as
well as build-system procedures (building multiple copies of object files
especially for the lib).

Several discussions have arisen wrt migrating it to CMake and it has become
difficult to justify adding more complexity for a library that is virtually
unused anyway.

See for example the discussions:
hebasto#41
bitcoin#29123

Instead, we (fanquake, hebasto, TheCharlatan, and I) propose simply not
migrating it to CMake and letting it end with v27. Any remaining use-cases
could be handled in the future by libbitcoinkernel.
@hebasto
Copy link
Owner Author

hebasto commented Jan 8, 2024

Marking this PR as a draft in the light of the bitcoin#29189.

@hebasto hebasto marked this pull request as draft January 8, 2024 10:50
@hebasto hebasto force-pushed the cmake-staging branch 2 times, most recently from 67ceebb to 203a3ab Compare January 16, 2024 13:23
hebasto added a commit that referenced this pull request Jan 25, 2024
9b9f562 cmake: Add fuzzing options (Hennadii Stepanov)
ea7861e cmake: Add `SANITIZERS` option (Hennadii Stepanov)

Pull request description:

  New CMake variables that affect the build system configuration:
  - `SANITIZERS`
  - `BUILD_FUZZ_BINARY`
  - `FUZZ`

  In the light of bitcoin#29189, this PR is no longer based on #41. However, the `test/fuzz/script_bitcoin_consensus.cpp` might be easily added anytime.

  For OSS-Fuzz integration, please refer to hebasto/oss-fuzz#1.

ACKs for top commit:
  theuni:
    Lightly tested ACK 9b9f562.

Tree-SHA512: f762d1218f2f8bfe26bdd43f431cb37a26093a6899db7120b50df3083f81d6ad6aa867937e69930faff6ab4519532cfaca48aaf97831d4c2593b93423cb6d41a
theuni added a commit to theuni/bitcoin that referenced this pull request Jan 30, 2024
This library has existed for nearly 10 years with very little known uptake or
impact. It has become a maintenance burden. In several cases it dictates our
code/library structure (for example necessitating LIBBITCOIN_CRYPTO_BASE), as
well as build-system procedures (building multiple copies of object files
especially for the lib).

Several discussions have arisen wrt migrating it to CMake and it has become
difficult to justify adding more complexity for a library that is virtually
unused anyway.

See for example the discussions:
hebasto#41
bitcoin#29123

Instead, we (fanquake, hebasto, TheCharlatan, and I) propose simply not
migrating it to CMake and letting it end with v27. Any remaining use-cases
could be handled in the future by libbitcoinkernel.
theuni added a commit to theuni/bitcoin that referenced this pull request Jan 30, 2024
This library has existed for nearly 10 years with very little known uptake or
impact. It has become a maintenance burden. In several cases it dictates our
code/library structure (for example necessitating LIBBITCOIN_CRYPTO_BASE), as
well as build-system procedures (building multiple copies of object files
especially for the lib).

Several discussions have arisen wrt migrating it to CMake and it has become
difficult to justify adding more complexity for a library that is virtually
unused anyway.

See for example the discussions:
hebasto#41
bitcoin#29123

Instead, we (fanquake, hebasto, TheCharlatan, and I) propose simply not
migrating it to CMake and letting it end with v27. Any remaining use-cases
could be handled in the future by libbitcoinkernel.
fanquake added a commit to bitcoin/bitcoin that referenced this pull request Feb 1, 2024
25dc87e libconsensus: deprecate (Cory Fields)

Pull request description:

  This library has existed for nearly 10 years with very little known uptake or impact. It has become a maintenance burden. In several cases it dictates our code/library structure (for example necessitating LIBBITCOIN_CRYPTO_BASE), as well as build-system procedures (building multiple copies of object files especially for the lib).

  Several discussions have arisen wrt migrating it to CMake and it has become difficult to justify adding more complexity for a library that is virtually unused anyway.

  See for example the discussions:
  hebasto#41
  #29123

  And here: #29180
  Where it is pointed out that the libbitcoinconsensus functions are slower than those the internal bitcoind equivalents due to the missing sha2 implementations.

  Instead, we (fanquake, hebasto, TheCharlatan, and I) propose simply not migrating it to CMake and letting it end with v27. Any remaining use-cases could be handled in the future by libbitcoinkernel.

  If there are any users currently using libbitcoinconsensus, please chime in with your use-case!

  Edit: Corrected final release to be v27.

ACKs for top commit:
  TheCharlatan:
    ACK 25dc87e
  fanquake:
    ACK 25dc87e - this library has very little, if any impactful real world usage. It has been entirely broken (on various platforms) for long periods of its existence, where nobody even noticed. Pruning this out to save porting, and starting anew with the kernel, is the sane thing to do.

Tree-SHA512: baff2b3c4f76f520c96021035f751fdcb51bedf00e767660249e92a7bc7c5c176786bcf2c4cfe2d2351c200f932b39eb886bcfb22fbec824a41617590d6a1638
@hebasto hebasto closed this Feb 2, 2024
janus pushed a commit to BitgesellOfficial/bitgesell that referenced this pull request Apr 6, 2024
This library has existed for nearly 10 years with very little known uptake or
impact. It has become a maintenance burden. In several cases it dictates our
code/library structure (for example necessitating LIBBITCOIN_CRYPTO_BASE), as
well as build-system procedures (building multiple copies of object files
especially for the lib).

Several discussions have arisen wrt migrating it to CMake and it has become
difficult to justify adding more complexity for a library that is virtually
unused anyway.

See for example the discussions:
hebasto/bitcoin#41
bitcoin/bitcoin#29123

Instead, we (fanquake, hebasto, TheCharlatan, and I) propose simply not
migrating it to CMake and letting it end with v27. Any remaining use-cases
could be handled in the future by libbitcoinkernel.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants