TheCharlatan
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
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
#25665 (refactor: Add util::Result failure values, multiple error and warning messages 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.
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.
TheCharlatan force-pushed
on Oct 30, 2023
TheCharlatan
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.
TheCharlatan
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:925
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
TheCharlatan
commented at 7:56 pm on November 24, 2023:
Could just remove it to be honest.
TheCharlatan
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.
TheCharlatan
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?
TheCharlatan
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
TheCharlatan force-pushed
on Nov 29, 2023
TheCharlatan
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, …)?
TheCharlatan
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?
TheCharlatan
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?
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
TheCharlatan force-pushed
on Dec 1, 2023
TheCharlatan
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.
TheCharlatan
commented at 9:11 am on December 6, 2023:
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
0@@ -0,0 +1,23 @@
1+// Copyright (c) 2009-present The Bitcoin Core developers
2+// Distributed under the MIT software license, see the accompanying
3+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+#ifndef BITCOIN_CRYPTO_HEX_BASE_H
6+#define BITCOIN_CRYPTO_HEX_BASE_H
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.
TheCharlatan
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.
TheCharlatan
commented at 8:16 am on December 7, 2023:
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:
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
TheCharlatan
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.
TheCharlatan 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
TheCharlatan force-pushed
on Jul 2, 2024
TheCharlatan force-pushed
on Jul 2, 2024
TheCharlatan
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
TheCharlatan 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.
TheCharlatan force-pushed
on Aug 29, 2024
TheCharlatan 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
TheCharlatan 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
TheCharlatan 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
TheCharlatan 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
TheCharlatan force-pushed
on May 16, 2025
DrahtBot added the label
CI failed
on May 16, 2025
in
src/util/CMakeLists.txt:33
in
bb3ac254fcoutdated
Should also be easy to get bitcoin_clientversion out of here.
TheCharlatan
commented at 4:38 pm on May 16, 2025:
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.
Yea, we could probably drop that. There’s also CLIENT_VERSION fed into the env RNG, which could also likely be dropped.
TheCharlatan force-pushed
on May 16, 2025
DrahtBot removed the label
CI failed
on May 16, 2025
TheCharlatan marked this as ready for review
on May 17, 2025
TheCharlatan
commented at 8:36 am on May 17, 2025:
contributor
Undrafting this again after some recent discussions on splitting the kernel library out in the future.
DrahtBot added the label
Needs rebase
on May 27, 2025
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.
e4d80d721b
TheCharlatan force-pushed
on May 29, 2025
TheCharlatan force-pushed
on May 29, 2025
TheCharlatan
commented at 2:27 pm on May 29, 2025:
contributor
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: 2025-06-08 09:13 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
