2023-11-29 edit: the questions and references have been updated slightly to reflect the latest force-push to 5a6a0b1693cf739a5e6cc1161160a1d621d215f9
Notes
The libbitcoinkernel project is an effort to decouple Bitcoin Core’s consensus engine from other non-consensus modules in the codebase. We have previously covered libbitcoinkernel-related PRs #27711, #25527, #24410 and #20158. However, they are not essential to understanding this PR as they focused more on internal code reorganization as opposed to this PR’s focus on the build system.
Suggested reading: Static and dynamic libraries (up until the “Installing and using libraries” section which is not relevant).
Most of the libraries(required reading) that are built are internal and static, and can’t be used outside of Bitcoin Core. Examples of internal libraries are libbitcoin_cli, libbitcoin_common and libbitcoin_node. There are only two external (dynamic, or shared) libraries(required reading): libbitcoinconsensus and libbitcoinkernel, even though the latter is not currently documented as such since the API is not ready.
This PR introduces a new internallibbitcoin_kernel static library, in addition to the existing external libbitoincoinkernel dynamic library.
Internal libraries are solely used to modularize the build system, which helps with build performance as well as maintainability. In the future, the Bitcoin Core build system may start to use the external libbitcoinkernel library, but that would probably require more kernel work (such as having a more complete and stable API) to be completed first.
An explanation of some abbreviations used:
a vs la(as in libbitcoin_kernel_a_SOURCES and libbitcoinkernel_la_SOURCES):
Are the Bitcoin Core internal libraries all statically or dynamically built, or a mix of both? Why?
Why do we need to build external libraries? What’s the purpose of having both an internal libbitcoin_kernel and an external libbitoinkernel? Why do we build external libraries in the first place?
Which of the libraries {libbitcoin_cli, libbitcoin_consensus, libbitcoinconsensus, libbitcoin_kernel, libbitcoinqt} are internal?
How are we using kernel functionality in Bitcoin Core before this PR, if there is no internal library?
Why does the libbitcoinkernel_la_SOURCES source file list specifically include $(libbitcoin_util_a_SOURCES) and $(libbitcoin_consensus_a_SOURCES) but libbitcoin_kernel_a_SOURCES doesn’t seem to?
Commit 41a80de mentions: “Moving util/message.cpp to the common library also breaks an undocumented dependency of the util library on the consensus library’s CPubKey::RecoverCompact symbol.”. What was this dependency?
Commit 5a6a0b1 de-duplicates the source file list. Are there any changes to the source file list used for libbitcoinkernel?
<stickies-v> welcome everyone! Today we're looking at #28690, authored by TheCharlatan. The notes and questions are available on https://bitcoincore.reviews/28690
<hebasto> stickies-v: in CMake it is easy to build any (even internal) library as shared one. the `--no-undefined` option will force a linker to report undefined symbols
<stickies-v> i'm assuming everyone's completed (or already knows about) the required reading from Q2, so we can just skip that and head straight into Q3
<stickies-v> quick reminder that review club is async - so don't hold back on continuing the conversations/questions about previous questions when we've moved on already!
<stickies-v> hebasto: but it's not really "acceptable" for internal static libs either, right? like, the current state is not perfect, but ideally we'd eliminate all such instances?
<abubakarsadiq_> From the list of the libraries in https://github.com/bitcoin/bitcoin/blob/master/doc/design/libraries.md seems like all internal libraries have the file extension .a which means they are all static libs. But I see libsecp256k1 and libbitcoin_crypto_base are dynamic I dont think they are internal libs.
<stickies-v> LarryRuane: shared library/DLL hijacking is indeed something I've come across a few times in my research but I don't know how feasible it is in practice nowadays, hopefully someone can expand on that
<maxedw> feels neater rather than needing to ship a bunch of libraries with the binary. I'm not sure about security as if someone is able to swap out one of the libs then why couldn't they swap out the main binary?
<stickies-v> i think perhaps one way of summarizing is: static libraries is better (i.e. more efficient and maintainable build system) than not having libraries, and shared libraries don't offer any additional benefit that we need here (because we don't want anyone outside of core to use our internal libraries)
<sipa> i feel this is a bit of a false dichotomy too... you can still having static internal libraries as intermediate build products/organization, and then compile them into a shared library
<monlovesmango> it says that libbitcoinkernel API is not ready. if we are making internal libbitcoin_kernel now will that have to keep up with changes from external library?
<stickies-v> I think at some point in the future, we _could_ have a separate libbitcoinkernel project that produces a shared library, and then i think we wouldn't have use for an internal `libbitcoin_kernel` static library in core anymore? correct?
<monlovesmango> stickies-v: can you explain why? seems like libbitcoinkernel already was a shared library but we are still making an internal one to replace it. why would that change if it became a separate project?
<stickies-v> so to answer monlovesmango: as said before: using static libs is better than not using libs (which is the current state), the shared libbitcoinkernel isn't really used anywhere, we just build it
<stickies-v> 7. Why does the `libbitcoinkernel_la_SOURCES` source file list specifically include `$(libbitcoin_util_a_SOURCES)` and `$(libbitcoin_consensus_a_SOURCES)` but `libbitcoin_kernel_a_SOURCES` doesn't seem to?
<monlovesmango> so hebasto said that current state we use a list of object files compiled from source, were these maintained to be in line with libbitcoinkernel? or libbitcoinkernel was built from these?
<hebasto> we want to compile `libbitcoinkernel` with `-DBUILD_BITCOIN_INTERNAL`, that's why we have to compile `libbitcoin_consensus_a_SOURCES` one more time, and not reuse already compiled internal lib, which was compiled without that flag
<TheCharlatan> Besides, next to having to define `-DBUILD_BITCOIN_INTERNAL` during compilation, it is not possible to for our internal libraries and end artifacts (like bitcoind) to eventually link against the external libbitcoin_kernel. Not only does the external libbitcoin_kernel have more symbols (like the translation function pointer), but it is also not possible to statically link a dynamic library
<stickies-v> we're a bit over time already so i'm going to end the meeting here, but we can continue the discussion for a bit longer on this last question for those who want to!
<stickies-v> pablomartin: yeah we skipped that for running out of time, i'll have to run myself but feel free to discuss here though and hopefully people will chime in
<TheCharlatan> After some change requests from reviewers, I decided to leave some of the small utilities in the util library, that previously were not used or part of the kernel, but which don't import further functionality, like `spanparsing`, `bytevectorhash`, and `readwritefile`. So the file list and thus the content of the kernel library does change a bit. They might be moved at later point though if