sedited
commented at 11:01 AM on October 20, 2023:
contributor
This PR introduces a new libbitcoin_kernel internal library. It completes the internal library design as laid out in doc/design/libraries.md. Since the util library contains a bunch of modules that are not required by the kernel library, a new kernel_util library is introduced as well that only consists of the modules required by the kernel library. The external libbitcoinkernel library now re-uses the compiled objects from the internal libraries.
There is a trade-off to this. Since we don't manually export symbols from the kernel library yet, this would make the library unusable if REDUCE_EXPORTS=ON. For now this means that the patch here introduces a separate source file list for the external library that gets populated by the source files of its static dependents.
DrahtBot
commented at 11:01 AM on October 20, 2023:
contributor
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
#25722 (refactor: Use util::Result class for wallet loading by ryanofsky)
#25665 (refactor: Add util::Result failure types and ability to merge result values by ryanofsky)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
DrahtBot added the label Build system on Oct 20, 2023
DrahtBot added the label CI failed on Oct 20, 2023
I've tested lightly this branch. There still exist a circle dependency between libbitcoin_crypto and libbitcoin_util. In particular, the former depends on the memory_cleanse symbol.
sedited force-pushed on Oct 30, 2023
sedited
commented at 1:08 PM on October 30, 2023:
contributor
stickies-v
commented at 1:23 PM on November 16, 2023:
contributor
Concept ACK
It turned out to be a bit less clean than I'd hoped, but I made this branch with a scripted-diff (with a second commit to re-order includes alphabetically) as an alternative to the manual first commit. I'm not sure it makes reviewing easier (and it needs some more manual edits to Makefile, ubsan and an unrelated include), but at least it's a helpful check:
nit: in be0b6233ec6cd21e5730380a3bc7f868189dabc9: is there a reason we move hex_base.{h, cpp} to crypto, and not consensus? It seems unrelated to our crypto library.
sedited
commented at 1:48 PM on November 16, 2023:
contributor
Currently both the consensus and util library share the strencodings files. This is not ideal, because the file will then have to be compiled twice (once for each of them). Further, the util library should not be relying on symbols from the consensus library according to the diagram. So I split the symbols that both the consensus and utill library needs from the strencodings file and put them into a new file in the crypto library, which both the util and consensus library depend on.
theuni
commented at 10:07 PM on November 22, 2023:
member
Concept ACK. Nice.
@stickies-v Thanks! That's super helpful.
I agree that crypto isn't the most ideal home for hex/string functions, but it does actually make it easier to use the lib as a standalone and not have to write your own versions. I don't really see any downside to having them there (no nasty dependencies, nothing stateful, etc) other than being out of place. Curious if @sipa has any specific opposition.
I'd also like to hear what @ryanofsky things about this PR, as he's been involved in many of the layout discussions.
in
src/Makefile.am:945
in
de6d798475outdated
942 | @@ -923,85 +943,7 @@ libbitcoinkernel_la_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS) -fvisibility=default
943 | # TODO: libbitcoinkernel is a work in progress consensus engine library, as more
944 | # and more modules are decoupled from the consensus engine, this list will
945 | # shrink to only those which are absolutely necessary.
stickies-v
commented at 4:54 PM on November 24, 2023:
I think this docstring needs to move as well
sedited
commented at 7:56 PM on November 24, 2023:
Could just remove it to be honest.
sedited
commented at 10:12 PM on November 24, 2023:
Yeah, will do this on the next push.
kashifs
commented at 5:36 PM on November 26, 2023:
contributor
Concept ACK. I just reviewed all the code and the design in preparation for the upcoming PR Review Club
theuni
commented at 7:03 PM on November 26, 2023:
member
Also ping @hebasto for 👀 as this would require some CMake attention :\
ryanofsky
commented at 4:54 PM on November 28, 2023:
In commit "build: Prepare libbitcoin_util for kernel integration" (515c9952829256f4c75c82d90f9ea4994b03baee)
The changes in this commit generally make sense, but IMO bytevectorhash.hreadwritefile.h and sock.h feel more like general purpose utilities that fit better in with the util library than the common library. I don't think there should be a barrier to calling these functions in kernel code even if they aren't called currently.
This came before up in #27989. In general, I think it makes sense to move functions from util to common when those functions shouldn't be called by kernel code, not just when they aren't called by kernel code.
sedited
commented at 9:10 PM on November 28, 2023:
I think for simple utilities like the bytevectorhash module, I kind of agree, since it does not pull in further unwanted deps, nor is used for anything opinionated (and similarly readwritefile.h). sock.h however includes compat.h, which we've previously worked to remove from the kernel header tree, so I think I'll leave that one as is.
in
test/sanitizer_suppressions/ubsan:74
in
515c995282outdated
ryanofsky
commented at 4:58 PM on November 28, 2023:
In commit "build: Prepare libbitcoin_util for kernel integration" (515c9952829256f4c75c82d90f9ea4994b03baee)
Is this an unrelated change? Could the reason for it be explained in the commit description?
In general, I don't really get the commit description. I don't know what "kernel integration" is specifically, or why parts of util need to be moved for it to happen.
ryanofsky
commented at 5:23 PM on November 28, 2023:
In commit "build: Introduce libbitcoin_kernel as an internal compilation unit" (1f98ed5e56230a61b699f669459e39fa101a1ca5)
It's surprising to coins.cpp moving from common library to the util library instead kernel library. Curious if this is necessary.
It might be nice to explain general reasoning behind moves in the commit description and call out any special cases.
I'm guessing that certain of these files that are moved to the util library like coins.cpp and kernel/chainparams.cpp probably could be split up later and mostly moved back to the kernel, since wallet and gui code shouldn't need most of what is in them.
ryanofsky
commented at 5:30 PM on November 28, 2023:
In commit "kernel: De-duplicate source file list" (de6d79847537cd7d0dfc3e2dd7a57b39b4281b5a)
Can code comment above be updated to explain why util and consensus sources are added to kernel library sources, instead of the kernel library just depending on these libraries?
sedited
commented at 10:37 PM on November 28, 2023:
I did not want to change the way libbitcoinkernel_la is compiled in this pull request. The way it is now, it just follows the same pattern of libbitcoinconsensus_la when it comes to defining its sources. So there should be no change in linking behavior of the external libbitcoinkernel arising from this patchset. Besides, linking against the static internal libraries probably won't work anyway.
ryanofsky approved
ryanofsky
commented at 5:33 PM on November 28, 2023:
contributor
Code review ACKde6d79847537cd7d0dfc3e2dd7a57b39b4281b5a. I had a few questions, but most of the changes here seem good, and all look like they should work and be safe.
DrahtBot requested review from theuni on Nov 28, 2023
DrahtBot requested review from hebasto on Nov 28, 2023
DrahtBot requested review from stickies-v on Nov 28, 2023
sedited force-pushed on Nov 29, 2023
sedited
commented at 9:24 AM on November 29, 2023:
contributor
maflcko
commented at 9:45 AM on November 30, 2023:
unrelated: What is the point of having headers in here? Wouldn't it be better to move them all to the CORE_H list, like for all other libraries (common/*.h, util/*.h, node/*.h, ...)?
sedited
commented at 9:48 AM on November 30, 2023:
That point is that you can compile a more minimal libbitcoin_consensus without having to drag in the full BITCOIN_CORE_H and accidentally introduce new stuff into the consensus lib.
maflcko
commented at 9:57 AM on November 30, 2023:
accidentally introduce new stuff into the consensus lib.
Is it true? I don't think the list is enforced in any way. Nothing in C++ is holding anyone back from including a header with header-only code. Moreover, the current code includes headers that are not listed here:
Also, it should be possible to include other headers without running into compile or link issues, as long as no new link symbols are required from other libraries, no?
sedited
commented at 12:01 PM on November 30, 2023:
tx_verify is not part of the consensus library though.
EDIT: To be clear, I still have a bad grasp of how our header accounting actually works and I think your point still stands, e.g. you can move any of the headers between CORE_H and the source definitions.
maflcko
commented at 12:26 PM on November 30, 2023:
Ok, but the following diff on tx_check compiles for me, or am I missing something?
diff --git a/src/consensus/tx_check.cpp b/src/consensus/tx_check.cpp
index b3fee1e8b1..1be8686bea 100644
--- a/src/consensus/tx_check.cpp
+++ b/src/consensus/tx_check.cpp
@@ -7,9 +7,14 @@
#include <consensus/amount.h>
#include <primitives/transaction.h>
#include <consensus/validation.h>
+#include <util/check.h>
+#include <util/fs.h>
bool CheckTransaction(const CTransaction& tx, TxValidationState& state)
{
+Assert(true);
+fs::path foo{"bar"};
+(void)foo;
// Basic checks that don't depend on any context
if (tx.vin.empty())
return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-vin-empty");
sedited
commented at 3:20 PM on November 30, 2023:
DrahtBot removed the label Needs rebase on Nov 30, 2023
ryanofsky
commented at 3:19 PM on December 1, 2023:
contributor
re: #28690 (comment) and earlier discussion about libcrypto starting #28690#pullrequestreview-1734365185:
I agree that crypto isn't the most ideal home for hex/string functions, but it does actually make it easier to use the lib as a standalone and not have to write your own versions.
Just want to note that libbitcoin_crypto and src/crypto/ are not currently mentioned in the libraries document: https://github.com/bitcoin/bitcoin/blob/master/doc/design/libraries.md. It'd be good to mention libbitcoin_crypto there, especially if the intent is for libbitcoin_crypto to be a standalone library that doesn't depend on any other bitcoin libraries, not just a normal internal library using some util functions.
DrahtBot added the label Needs rebase on Dec 1, 2023
sedited force-pushed on Dec 1, 2023
sedited
commented at 10:42 PM on December 1, 2023:
contributor
DrahtBot removed the label Needs rebase on Dec 1, 2023
maflcko added the label DrahtBot Guix build requested on Dec 5, 2023
pablomartin4btc
commented at 10:34 PM on December 5, 2023:
member
Concept ACK
I'd recommend reading Review Club notes from @stickies-v (thanks!) since those are great, well structured and very useful in order to have a wider context of most of the changes involved.
ryanofsky
commented at 11:18 PM on December 5, 2023:
In commit "build: Prepare libbitcoin_util for internal kernel library" (8b3623769e1307c122959fbdc8c4c93ddd81d5c9)
It seems like a bug to have a util header depending on a common header. I think I'd suggest splitting up the spanparsing header and renaming it, since right now it is not very focused. Would suggest moving the spanparsing Split function to src/util/string.h where it is being used, and probably moving the rest of the functions to src/script/parsing.h since they are only used for descriptor and miniscript parsing. Would suggest doing these things in a separate commit to avoid adding complexity to this commit.
Good catch, I think though this file also falls in the category of #28690 (review), and since it does not introduce any further potentially controversial headers, I'd rather drop this change altogether. This can be re-visited once/if we have some accounting of the headers.
DrahtBot
commented at 5:04 AM on December 6, 2023:
contributor
Is this commit needed? I guess there could be issues if the same translation unit is compiled twice?
Not sure if Hex is correct to put into "crypto".
If this isn't needed, maybe leave this for a follow-up?
An alternative would be to just remove it from the util library and keep it only in consensus?
Not sure about the current state, but when this was asked previously #28690#pullrequestreview-1734365185, I think the idea was to have it in crypto since util and consensus libraries depend on the crypto library.
sedited
commented at 10:49 PM on December 6, 2023:
An alternative would be to just remove it from the util library and keep it only in consensus?
These functions are used by components of bitcoin-cli. The bitcoin-cli binary should not have to rely on the consensus library, so I don't think that is an alternative.
Ok, so the reason to put Hex into crypto is because it happens to be convenient at this time? No objection, but it would be good to extend the motivation a bit. I asked if this is needed, and what would happen if the commit was dropped, so it may be good to include a compile/link failure in the motivation, if there is one.
If you drop the commit splitting strencodings and moving the hex function to the crypto library you get a bunch of errors when compiling the last commit:
/usr/bin/ld: /usr/bin/ld: DWARF error: invalid or unhandled FORM value: 0x23
util/.libs/libbitcoinkernel_la-strencodings.o: in function `SanitizeString[abi:cxx11](std::basic_string_view<char, std::char_traits<char> >, int)':
strencodings.cpp:(.text+0x0): multiple definition of `SanitizeString[abi:cxx11](std::basic_string_view<char, std::char_traits<char> >, int)'; /usr/bin/ld: DWARF error: invalid or unhandled FORM value: 0x23
This is because the external libbitcoinkernel library now re-uses the existing source file lists and will find two strencodings files, one from the util sources and one from the consensus sources.
DrahtBot removed the label Needs rebase on Dec 6, 2023
ryanofsky approved
ryanofsky
commented at 10:43 PM on December 6, 2023:
contributor
Main change since last review is no longer moving the spanparsing.h file here, to avoid making util depend on common. I have since opened a separate PR #29015 to remove spanparsing.h functions from util by splitting it up.
DrahtBot requested review from hebasto on Dec 6, 2023
DrahtBot requested review from pablomartin4btc on Dec 6, 2023
DrahtBot requested review from ismaelsadeeq on Dec 6, 2023
hebasto
commented at 1:24 PM on December 10, 2023:
member
The last commit 34970bde23dd87fc0fea5a1970880761f7184774 effectively extends the libbitcoinkernel_la_SOURCES with the following source files:
script/bitcoinconsensus.cpp
util/bytevectorhash.cpp
util/readwritefile.cpp
util/spanparsing.cpp
util/threadinterrupt.cpp
The resulted libbitcoinkernel.a and libbitcoinkernel.so.0.0.0 get bigger, obviously.
This change is not mentioned in the commit message, and it's not clear whether it is done intentionally.
DrahtBot requested review from hebasto on Dec 10, 2023
sedited
commented at 2:28 PM on December 10, 2023:
contributor
Putting this on draft while we figure out the desired contents of the util library in #29015.
sedited marked this as a draft on Dec 10, 2023
DrahtBot added the label Needs rebase on Dec 13, 2023
achow101 referenced this in commit 011a895a82 on Jun 12, 2024
sedited force-pushed on Jul 2, 2024
sedited force-pushed on Jul 2, 2024
sedited
commented at 12:35 PM on July 2, 2024:
contributor
#29015 got merged in the meantime and there are some implications for this pull request. We decided that the util library may contain a superset of the utilities that the kernel strictly requires.
For this reason I decided to implement aj's suggestion (https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-1849208455) of introducing a slimmed-down version of the util library for the kernel containing only the components that the kernel requires. I'd appreciate the opinion of previous reviewers of this PR on this new approach. Having some modules in the util library that are not required is not all bad - they might become useful in the future when working with the kernel. On the other hand they do bloat the library and may make it harder in future to use the kernel library on some platforms.
The external consensus library was also removed while waiting for #29015. I wonder if we should remove the internal consensus library as well and drop its contents into the new internal kernel library.
I will keep this in draft for now - its msvc build is broken and I'm not sure if it is worth going through this exercise again if cmake is going to land soon.
DrahtBot removed the label Needs rebase on Jul 2, 2024
DrahtBot added the label Needs rebase on Jul 3, 2024
sedited force-pushed on Aug 28, 2024
DrahtBot removed the label Needs rebase on Aug 28, 2024
DrahtBot added the label CI failed on Aug 28, 2024
DrahtBot
commented at 1:45 PM on August 28, 2024:
contributor
Make sure to run all tests locally, according to the documentation.
The failure may happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
</details>
sedited force-pushed on Aug 29, 2024
sedited force-pushed on Aug 29, 2024
DrahtBot removed the label CI failed on Aug 29, 2024
DrahtBot added the label Needs rebase on Sep 3, 2024
sedited force-pushed on Sep 3, 2024
DrahtBot removed the label Needs rebase on Sep 3, 2024
DrahtBot added the label CI failed on Sep 3, 2024
DrahtBot added the label Needs rebase on Sep 4, 2024
sedited force-pushed on Sep 4, 2024
DrahtBot removed the label Needs rebase on Sep 4, 2024
DrahtBot removed the label CI failed on Oct 1, 2024
DrahtBot added the label Needs rebase on Oct 11, 2024
sedited force-pushed on May 16, 2025
bitcoin deleted a comment on May 16, 2025
bitcoin deleted a comment on May 16, 2025
DrahtBot removed the label Needs rebase on May 16, 2025
sedited force-pushed on May 16, 2025
DrahtBot added the label CI failed on May 16, 2025
in
src/util/CMakeLists.txt:33
in
bb3ac254fcoutdated
I think it is just used for logging. I could just stub it out, like we do for the translation function, but I am also not as sure how important it really is to be printing the clientversion there for these logs.
DrahtBot removed the label Needs rebase on May 29, 2025
yuvicc
commented at 2:46 PM on July 12, 2025:
contributor
Concept ACK
The external consensus library was also removed while waiting for #29015. I wonder if we should remove the internal consensus library as well and drop its contents into the new internal kernel library
I agree with you, was having a similar thought on the internal library libbitcoin_consensus despite removal of external libbitcoinconsensus library. If we go this way then we can remove the libbitcoin_consensus from the libraries.md as well.
DrahtBot added the label Needs rebase on Jul 18, 2025
sedited force-pushed on Jul 30, 2025
sedited
commented at 7:04 PM on July 30, 2025:
contributor
DrahtBot removed the label Needs rebase on Nov 25, 2025
janb84
commented at 4:01 PM on December 8, 2025:
contributor
ACK4c387e938015de197860215d94b9c9aee9a82da8
Looks good to me, next to several builds & tests, I did a (subsection of a ) Guix build to see if there where any issues with that. It ran fine, see below.
It's not a serious node, e.g. it fetches blocks from mempool.space. It exercises 100% of the API bindings, though not always in a useful way.
Switching from master to this PR in that project makes no difference, so that's good.
BrandonOdiwuor
commented at 8:33 AM on March 24, 2026:
contributor
Approach ACKfdf80892b305eaee17d50a665e821d356b570640 refactoring how the external libbitcoinkernel SHARED library is built, to re-use source files from the new internal libbitcoin_kernel, libbitcoin_kernel_util, and bitcoin_consensus STATIC libraries.
Tested on Ubuntu 24.04 with CMake 3.28.3 and gcc 13.3.0
janb84
commented at 1:24 PM on March 26, 2026:
contributor
re ACKfdf80892b305eaee17d50a665e821d356b570640
changes sinds last ack:
rebase
"There is a trade-off to this. Since we don't manually export symbols from the kernel library yet, this would make the library unusable if REDUCE_EXPORTS=ON. For now this means that the patch here introduces a separate source file list for the external library that gets populated by the source files of its static dependents."
Form the PR description, this reads if there are issues when compiled with REDUCE_EXPORTS=ON. But as per description there is a patch also introduced so it just works (just wanted to clarify this) :
llvm-nm -gU build_dev_mode/lib/libbitcoinkernel.dylib | grep ' _btck_' | head -n 60
000000000000a160 T _btck_block_copy
000000000000a1e0 T _btck_block_count_transactions
0000000000009efc T _btck_block_create
000000000000a640 T _btck_block_destroy
000000000000a5d0 T _btck_block_get_hash
000000000000a2e0 T _btck_block_get_header
000000000000a248 T _btck_block_get_transaction_at
000000000000aaec T _btck_block_hash_copy
000000000000aa80 T _btck_block_hash_create
000000000000ac48 T _btck_block_hash_destroy
000000000000abb8 T _btck_block_hash_equals
000000000000ab58 T _btck_block_hash_to_bytes
000000000000bf04 T _btck_block_header_copy
000000000000bce8 T _btck_block_header_create
000000000000c190 T _btck_block_header_destroy
000000000000c094 T _btck_block_header_get_bits
000000000000bf80 T _btck_block_header_get_hash
000000000000c13c T _btck_block_header_get_nonce
000000000000bfec T _btck_block_header_get_prev_hash
000000000000c040 T _btck_block_header_get_timestamp
000000000000c0e8 T _btck_block_header_get_version
000000000000a708 T _btck_block_read
000000000000af38 T _btck_block_spent_outputs_copy
000000000000afb8 T _btck_block_spent_outputs_count
000000000000b0d8 T _btck_block_spent_outputs_destroy
000000000000b02c T _btck_block_spent_outputs_get_transaction_spent_outputs_at
000000000000accc T _btck_block_spent_outputs_read
000000000000a35c T _btck_block_to_bytes
000000000000aa24 T _btck_block_tree_entry_equals
000000000000a9d0 T _btck_block_tree_entry_get_block_hash
000000000000a894 T _btck_block_tree_entry_get_block_header
000000000000a97c T _btck_block_tree_entry_get_height
0000000000007560 T _btck_block_tree_entry_get_previous
0000000000007674 T _btck_block_validation_state_copy
000000000000760c T _btck_block_validation_state_create
000000000000777c T _btck_block_validation_state_destroy
00000000000078c8 T _btck_block_validation_state_get_block_validation_result
0000000000007860 T _btck_block_validation_state_get_validation_mode
000000000000bc28 T _btck_chain_contains
000000000000bb70 T _btck_chain_get_by_height
000000000000bae4 T _btck_chain_get_height
0000000000006440 T _btck_chain_parameters_copy
0000000000005f8c T _btck_chain_parameters_create
00000000000064d4 T _btck_chain_parameters_destroy
00000000000087e0 T _btck_chainstate_manager_create
00000000000097c8 T _btck_chainstate_manager_destroy
000000000000ba44 T _btck_chainstate_manager_get_active_chain
000000000000974c T _btck_chainstate_manager_get_best_entry
0000000000009644 T _btck_chainstate_manager_get_block_tree_entry_by_hash
0000000000009908 T _btck_chainstate_manager_import_blocks
0000000000007950 T _btck_chainstate_manager_options_create
00000000000084a0 T _btck_chainstate_manager_options_destroy
0000000000008600 T _btck_chainstate_manager_options_set_wipe_dbs
0000000000008430 T _btck_chainstate_manager_options_set_worker_threads_num
00000000000086f0 T _btck_chainstate_manager_options_update_block_tree_db_in_memory
0000000000008768 T _btck_chainstate_manager_options_update_chainstate_db_in_memory
000000000000b8a4 T _btck_chainstate_manager_process_block
000000000000b930 T _btck_chainstate_manager_process_block_header
000000000000b6ec T _btck_coin_confirmation_height
000000000000b50c T _btck_coin_copy
</details>
This PR introduces a new libbitcoin_kernel internal library. It completes the internal library design as laid out in doc/design/libraries.md.
I wanted to see how close we are to the design document after this change, so I generated some graphs with cmake graphviz:
Master:
This PR:
DrahtBot requested review from BrandonOdiwuor on Mar 26, 2026
DrahtBot requested review from Sjors on Mar 26, 2026
DrahtBot added the label Needs rebase on Mar 30, 2026
It would be surprising to me if bitcoin-wallet needs the kernel. It builds without this.
ryanofsky
commented at 1:28 PM on April 8, 2026:
contributor
Since the util library contains a bunch of modules that are not required by the kernel library, a new kernel_util library is introduced as well that only consists of the modules required by the kernel library.
It seems like an alternative to introducing a new library would just be to move parts of util not needed by the kernel to the common library. I think the main reason common exists at this point is to hold code needed by node/wallet/gui code but not needed by the kernel.
sedited force-pushed on Apr 22, 2026
sedited
commented at 10:39 AM on April 22, 2026:
contributor
Addressed @achow101's comments 1, and 2, dropped left over kernel library links where they are no longer needed.
sedited
commented at 10:42 AM on April 22, 2026:
contributor
It seems like an alternative to introducing a new library would just be to move parts of util not needed by the kernel to the common library. I think the main reason common exists at this point is to hold code needed by node/wallet/gui code but not needed by the kernel.
That was what this PR originally did, but it did not seem to get much approval at the time, and I switched approach as mentioned in my comment on this PR a few years ago here:
"https://github.com/bitcoin/bitcoin/pull/29015 got merged in the meantime and there are some implications for this pull request. We decided that the util library may contain a superset of the utilities that the kernel strictly requires. For this reason I decided to implement aj's suggestion (https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-1849208455) of introducing a slimmed-down version of the util library for the kernel containing only the components that the kernel requires. I'd appreciate the opinion of previous reviewers of this PR on this new approach. Having some modules in the util library that are not required is not all bad - they might become useful in the future when working with the kernel. On the other hand they do bloat the library and may make it harder in future to use the kernel library on some platforms."
ryanofsky
commented at 2:22 PM on April 22, 2026:
contributor
Having some modules in the util library that are not required is not all bad
I guess I don't understand what specifically is bad about not using parts of util in the kernel. Things that aren't unused seem unlikely to be included or linked or cause bloat. And if there are things in util that don't belong in the kernel for any reason, they can be moved to common. I can understand wanting to have util and kernel_util libraries(*) and then getting rid of common but I don't understand a reason for wanting 3 libraries.
(*) Though I don't think kernel_util is a great name for the library if it will also be used by the wallet.
switched approach as mentioned in my comment on this PR a few years ago #28690 (comment):
Interestingly that comment says it is implementing a suggestion from #29015 (comment), but is not really doing this. The suggestion there is to get rid of common, put everything in the util namespace and src/util but have separate util and util-core static libraries.
If that's still the long term goal and this PR is a first step, then that seems good, but in this case maybe kernel_util should be called util-core
sedited
commented at 4:14 PM on April 22, 2026:
contributor
I guess I don't understand what specifically is bad about not using parts of util in the kernel. Things that aren't unused seem unlikely to be included or linked or cause bloat. And if there are things in util that don't belong in the kernel for any reason, they can be moved to common. I can understand wanting to have util and kernel_util libraries(*) and then getting rid of common but I don't understand a reason for wanting 3 libraries.
I'm less concerned about bloat, or making it harder to further isolate components for targets that don't have certain OS features. Like previous build system moves and changes motivated by the kernel, this is more about establishing a moat to prevent new entanglements on the build system level. This also was a motivation for the initial introduction of the library and the reason why we have been moving code out of it. For example I never want someone to include the sock header from a kernel source without making it clear something is being moved on the build system level and hopefully further alerting the reviewer.
With this goal in mind, it doesn't really matter to me if this is achieved as it is here, or by moving the non-kernel parts of util to common, or have a kernel_util and core_util. If you have a clear preference and would be willing to move forward and eventually merge one of the approaches, I'd be happy to change to that.
ryanofsky
commented at 5:16 PM on April 22, 2026:
contributor
Hmm, can you explain the sock header example a little bit? I am having some trouble understanding the goal there. It seems like including a socket header would be bad for "targets that don't have certain OS features", but you say you are "less concerned" about that. Is the concern maybe that you want to prevent high level application code from including lower-level code that is supposed to be abstracted?
I'd be happy with any solution where it's reasonably clear which library code belongs in. Having common and util where common is for things like config file parsing that we don't want in the kernel has seemed like a reasonable approach, but dropping common and putting everything in util could also be good.
More specifically, the concern I have about this PR just is the kernel references sneaking into the util and wallet directories. I'd think kernel would be a user of util and util would be oblivious of it, and wallet should not depend on kernel. So I'm wondering what the goal is exactly.
sedited
commented at 7:38 PM on April 22, 2026:
contributor
Hmm, can you explain the sock header example a little bit? I am having some trouble understanding the goal there. It seems like including a socket header would be bad for "targets that don't have certain OS features", but you say you are "less concerned" about that. Is the concern maybe that you want to prevent high level application code from including lower-level code that is supposed to be abstracted?
Mmh, maybe we are talking past each other a bit. The layering argument is confusing. I'm not really distinguishing between an "application specific" and a "lower level utility" here, and I'm not sure that even makes sense, or at least it seems like a question of taste. From my point of view, anything that is not required strictly to get the consensus engine working acts on a different semantic level than the kernel code. This includes stuff like sockets, asmap, exec, etc. Preventing developers from trivially entangling this again with some small build system changes is just another way to communicate this in code in a minimally obtrusive manner. We currently (on master) communicate this through the kernel library cmake sources list. However that is insufficient, because the sources list may deviate (by accident) from what we are actually using on the production path, so we re-use the production path list for the external library in this PR. To keep enforcing the split we also split the utilities in a kernel and non-kernel section.
DrahtBot removed the label Needs rebase on Apr 23, 2026
in
src/util/CMakeLists.txt:5
in
6cb62624cd
1 | @@ -2,30 +2,20 @@
2 | # Distributed under the MIT software license, see the accompanying
3 | # file COPYING or https://opensource.org/license/mit/.
4 |
5 | -add_library(bitcoin_util STATIC EXCLUDE_FROM_ALL 6 | - asmap.cpp 7 | - batchpriority.cpp 8 | - bip32.cpp 9 | - bytevectorhash.cpp 10 | +add_library(bitcoin_kernel_util STATIC EXCLUDE_FROM_ALL
I have a python version of check-deps which I'll PR shortly and I think is a bit easier to follow the output of. EDIT: #35167
ajtowns
commented at 2:48 PM on April 27, 2026:
contributor
Should be updating the design doc, and it would be nicer if this was reducing the complexity of all our libraries rather than adding another one to menagerie.
ajtowns
commented at 3:22 PM on April 27, 2026:
contributor
I couldn't figure out an opinion on splitting things:
Having bitcoin-cli, bitcoin-tx, and bitcoin-wallet all be fairly small is nice;
bitcoin-kernel is already large, but not having extraneous dependencies is definitely nice;
Having "consensus", "common", "util", and "kernel-util" seems a pretty messy way of achieving those goals
Introduce internal kernel library
To facilitate compiling the external kernel library with different
symbol visibility, re-use the same source file list from its dependent
internal counterpart.
a47a701003
sedited force-pushed on Apr 27, 2026
sedited
commented at 9:40 PM on April 27, 2026:
contributor
Addressed @ajtowns' comment, fixed some incorrect module migrations.
Addressed @ajtowns' comment, updated check-deps with kernel library deps.
doc/dependencies.md still has a bunch of internal inconsistencies after this,. We've treated that document as aspirational so far anyway, so I think those are best resolved later.
ajtowns
commented at 3:45 AM on April 28, 2026:
contributor
I think you should also add libbitcoinkernel[libbitcoinkernel]-->libbitcoin_kernel; and class [...],libbitcoinkernel bold (and maybe change the classDef bold to include fill:white as well)
With those changes, and removing transitive deps, the full graph after this PR looks like:
doc/dependencies.md [doc/design/libraries.md] still has a bunch of internal inconsistencies after this,. We've treated that document as aspirational so far anyway, so I think those are best resolved later.
Adding deps from common/util/wallet to the kernel seems backwards to me. I think what we'd like might be something more like this?
consensus is replaced by kernel_lite -- essentially whatever parts of consensus-ish logic needed by bitcoin-wallet or bitcoin-cli
common is dropped, with code moved to util or node
crypto is moved into util_kernel (or util)
With the main property being that each lib is included by a different combination of binaries.
One place where dropping transitive deps is misleading is that the bitcoin-cli, -tx, -util and -wallet utilities currently include the util lib, but not the crypto lib. So rather than merging crypto into util, maybe it would be better to move the modules that depend on crypto out of util and into crypto/kernel/node.
OTOH, I'm not entirely sure how much that matters at all, if the linker just cleans everything up anyway, what are we actually achieving?
Update docs and check-deps for util kernel lib2a77b5fde4
sedited
commented at 10:48 AM on April 28, 2026:
contributor
Adding deps from common/util/wallet to the kernel seems backwards to me. I think what we'd like might be something more like this?
Yeah, there are some confusing deps there. kernel_lite might indeed better describe such a shared component between wallet and kernel. AFAICT many of the components the wallet currently requires from the kernel are policy related, so putting them into something called "consensus" would be very confusing. I've had a branch splitting the mempool from the kernel on the back burner for a while now. Maybe it is time to finally work on that?
OTOH, I'm not entirely sure how much that matters at all, if the linker just cleans everything up anyway, what are we actually achieving?
The impact of all of this is pretty marginal. I laid out my main motivation for it here: #28690 (comment).
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2026-05-02 15:13 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
