build: Introduce internal kernel library

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 patch here prohibits compiling the kernel library if REDUCDE_EXPORTS=ON. This should be acceptable, since this option is usually used in the context of releases, and we probably won’t be releasing the kernel library until a header with explicitly exported symbols for external use is defined. However, it does lock us into using the same compiler flags for both the internal and external libraries.

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.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	stickies-v, theuni, kashifs, pablomartin4btc, ismaelsadeeq
Approach ACK	hebasto
Stale ACK	ryanofsky

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31053 (RFC: build: support for pre-compiled headers. by theuni)
#30970 (build: Add missing USDT header dependency to kernel by theuni)
#30911 (build: speed up by flattening the dependency graph by theuni)
#30239 (Ephemeral Dust by instagibbs)
#30079 (Fee Estimation: Ignore all transactions that are CPFP’d by ismaelsadeeq)
#29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)
#28676 ([WIP] Cluster mempool implementation by sdaftuar)

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

TheCharlatan force-pushed on Oct 20, 2023

in src/Makefile.am:661 in a5e49603f8 outdated

643@@ -668,45 +644,45 @@ libbitcoin_common_a_SOURCES = \
644   base58.cpp \
645   bech32.cpp \
646   chainparams.cpp \
647-  coins.cpp \
648+  chainparamsbase.cpp \
649   common/args.cpp \
650+  common/asmap.cpp \
651+  common/bip32.cpp \

hebasto commented at 10:04 am on October 21, 2023:

This change should be accompanied with another one:

0--- a/test/sanitizer_suppressions/ubsan
1+++ b/test/sanitizer_suppressions/ubsan
2@@ -62,5 +62,5 @@ shift-base:arith_uint256.cpp
3 shift-base:crypto/
4 shift-base:hash.cpp
5 shift-base:streams.h
6-shift-base:util/bip32.cpp
7+shift-base:common/bip32.cpp
8 shift-base:xoroshiro128plusplus.h

TheCharlatan force-pushed on Oct 21, 2023

hebasto commented at 10:46 am on October 21, 2023: member

To fix the MSVC build, suggesting to apply the diff as follows:

 0--- a/build_msvc/bitcoin.sln
 1+++ b/build_msvc/bitcoin.sln
 2@@ -82,6 +82,10 @@ Global
 3         {460FEE33-1FE1-483F-B3BF-931FF8E969A5}.Debug|x64.Build.0 = Debug|x64
 4         {460FEE33-1FE1-483F-B3BF-931FF8E969A5}.Release|x64.ActiveCfg = Release|x64
 5         {460FEE33-1FE1-483F-B3BF-931FF8E969A5}.Release|x64.Build.0 = Release|x64
 6+        {A938586F-1016-4C60-9B82-56123264EE14}.Debug|x64.ActiveCfg = Debug|x64
 7+        {A938586F-1016-4C60-9B82-56123264EE14}.Debug|x64.Build.0 = Debug|x64
 8+        {A938586F-1016-4C60-9B82-56123264EE14}.Release|x64.ActiveCfg = Release|x64
 9+        {A938586F-1016-4C60-9B82-56123264EE14}.Release|x64.Build.0 = Release|x64
10         {5724BA7D-A09A-4BA8-800B-C4C1561B3D69}.Debug|x64.ActiveCfg = Debug|x64
11         {5724BA7D-A09A-4BA8-800B-C4C1561B3D69}.Debug|x64.Build.0 = Debug|x64
12         {5724BA7D-A09A-4BA8-800B-C4C1561B3D69}.Release|x64.ActiveCfg = Release|x64

TheCharlatan force-pushed on Oct 21, 2023

TheCharlatan commented at 2:31 pm on October 21, 2023: contributor

Thanks for the fixes @hebasto! Looks like the remaining test failure is a timeout?

hebasto commented at 3:36 pm on October 21, 2023: member

Looks like the remaining test failure is a timeout?

I believe that this PR changes are unrelated to that timeout. See: #28703.

TheCharlatan force-pushed on Oct 23, 2023

TheCharlatan marked this as ready for review on Oct 24, 2023

DrahtBot removed the label CI failed on Oct 25, 2023

DrahtBot added the label Needs rebase on Oct 26, 2023

hebasto commented at 3:44 pm on October 27, 2023: member

Concept ACK on improving internal library boundaries.

Should also address #28548.

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

Thank you for taking a look at @hebasto,

Rebased 04d7a985b4c58a37dc3f99c05d16810720d60957 -> 84a8cae915308dcdea6c579e7e24168d3d3eb030 (kernelInternalLib_0 -> kernelInternalLib_1, compare)

Resolved conflicts

Updated 84a8cae915308dcdea6c579e7e24168d3d3eb030 -> 815eaef0e1a30b746aef2a1112bb9b7895d119de (kernelInternalLib_1 -> kernelInternalLib_2, compare)

Addressed @hebasto’s comment, moved cleanse files to crypto library.

DrahtBot removed the label Needs rebase on Oct 30, 2023

DrahtBot added the label CI failed on Oct 30, 2023

TheCharlatan force-pushed on Oct 31, 2023

TheCharlatan commented at 11:27 am on October 31, 2023: contributor

Updated 815eaef0e1a30b746aef2a1112bb9b7895d119de -> 4052c2d5b9f84289752093981f475ea2ca5b64e7 (kernelInternalLib_2 -> kernelInternalLib_3, compare)

Fixed silent merge conflict.

DrahtBot removed the label CI failed on Oct 31, 2023

laanwj requested review from laanwj on Oct 31, 2023

DrahtBot added the label Needs rebase on Nov 2, 2023

TheCharlatan force-pushed on Nov 2, 2023

TheCharlatan commented at 1:30 pm on November 2, 2023: contributor

Rebased 4052c2d5b9f84289752093981f475ea2ca5b64e7 -> 1a589335617944b755235597aafc254e28e4acf9 (kernelInternalLib_3 -> kernelInternalLib_4, compare)

Fixed merge conflict.

DrahtBot removed the label Needs rebase on Nov 2, 2023

DrahtBot added the label Needs rebase on Nov 7, 2023

TheCharlatan force-pushed on Nov 9, 2023

TheCharlatan commented at 11:27 am on November 9, 2023: contributor

Rebased 1a589335617944b755235597aafc254e28e4acf9 -> 40e151697d3d1ed594364498d9e6220219d9b545 (kernelInternalLib_4 -> kernelInternalLib_5, compare)

Fixed merge conflict.

DrahtBot removed the label Needs rebase on Nov 9, 2023

DrahtBot added the label Needs rebase on Nov 15, 2023

TheCharlatan force-pushed on Nov 15, 2023

DrahtBot removed the label Needs rebase on Nov 15, 2023

TheCharlatan commented at 4:07 pm on November 15, 2023: contributor

Rebased 40e151697d3d1ed594364498d9e6220219d9b545 -> de6d79847537cd7d0dfc3e2dd7a57b39b4281b5a (kernelInternalLib_5 -> kernelInternalLib_6, compare)

Fixed merge conflict.

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:

https://github.com/bitcoin/bitcoin/compare/master...stickies-v:bitcoin:182d54aed91bfb1c161033642c322a542c8f2e04

0% git diff 515c9952829256f4c75c82d90f9ea4994b03baee 182d54aed91bfb1c161033642c322a542c8f2e04 --stat
1 src/Makefile.am                   | 48 ++++++++++++++++++++++++------------------------
2 src/common/sock.h                 |  1 -
3 test/sanitizer_suppressions/ubsan |  1 -

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

Re #28690#pullrequestreview-1734365185

nit: in https://github.com/bitcoin/bitcoin/commit/be0b6233ec6cd21e5730380a3bc7f868189dabc9: is there a reason we move hex_base.{h, cpp} to crypto, and not consensus? It seems unrelated to our crypto library.

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 de6d798475 outdated

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 :\

in src/Makefile.am:142 in 515c995282 outdated

135@@ -136,9 +136,18 @@ BITCOIN_CORE_H = \
136   clientversion.h \
137   coins.h \
138   common/args.h \
139+  common/asmap.h \
140+  common/bip32.h \
141   common/bloom.h \
142+  common/bytevectorhash.h \

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.h readwritefile.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 515c995282 outdated

70@@ -71,4 +71,5 @@ shift-base:crypto/
71 shift-base:ROTL32
72 shift-base:streams.h
73 shift-base:FormatHDKeypath
74+shift-base:common/bip32.cpp

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.

in src/Makefile.am:325 in 1f98ed5e56 outdated

320@@ -319,6 +321,7 @@ BITCOIN_CORE_H = \
321   util/result.h \
322   util/serfloat.h \
323   util/signalinterrupt.h \
324+  util/strencodings.h \

ryanofsky commented at 5:06 pm on November 28, 2023:

In commit “build: Introduce libbitcoin_kernel as an internal compilation unit” (1f98ed5e56230a61b699f669459e39fa101a1ca5)

Reason for adding strencodings.h this commit? Can it be explained in the commit description, or does it maybe belong the previous commit?

in src/Makefile.am:744 in 1f98ed5e56 outdated

742 # util #
743 libbitcoin_util_a_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES)
744 libbitcoin_util_a_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS)
745 libbitcoin_util_a_SOURCES = \
746   clientversion.cpp \
747+  coins.cpp \

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.

in src/Makefile.am:943 in de6d798475 outdated

1019-  util/tokenpipe.cpp \
1020-  validation.cpp \
1021-  validationinterface.cpp \
1022-  versionbits.cpp \
1023-  warnings.cpp
1024+libbitcoinkernel_la_SOURCES = kernel/bitcoinkernel.cpp $(libbitcoin_kernel_a_SOURCES) $(libbitcoin_util_a_SOURCES) $(libbitcoin_consensus_a_SOURCES)

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 ACK de6d79847537cd7d0dfc3e2dd7a57b39b4281b5a. 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

Thank you for the review @ryanofsky!

Rebased de6d79847537cd7d0dfc3e2dd7a57b39b4281b5a -> 35ad530fd951ebdc3cb4b36aaa5ca2af77c527e4 (kernelInternalLib_6 -> kernelInternalLib_7, compare)

Updated 35ad530fd951ebdc3cb4b36aaa5ca2af77c527e4 -> 511e3fc74cdd03c36764eaf3bb637e7bbe98b335 (kernelInternalLib_7 -> kernelInternalLib_8, compare)

Addressed @ryanofsky’s comment, moving bytevectorhash and readwritefile modules back to the util library.
Addressed @ryanofsky’s comment, dropped change in the sanitizer suppression that became redundant after #28865 was merged.
Addressed @ryanofsky’s comment, moved strencodings change to the correct commit.
Addressed @ryanofsky’s comment, explaining in the commit message why some files are moved from common to util.

in src/Makefile.am:645 in 5a6a0b1693 outdated

642@@ -657,8 +643,6 @@ libbitcoin_consensus_a_SOURCES = \
643   tinyformat.h \
644   uint256.cpp \
645   uint256.h \

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:

0src/consensus/tx_verify.cpp:#include <util/check.h>
1src/consensus/tx_verify.cpp:#include <util/moneystr.h>

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?

 0diff --git a/src/consensus/tx_check.cpp b/src/consensus/tx_check.cpp
 1index b3fee1e8b1..1be8686bea 100644
 2--- a/src/consensus/tx_check.cpp
 3+++ b/src/consensus/tx_check.cpp
 4@@ -7,9 +7,14 @@
 5 #include <consensus/amount.h>
 6 #include <primitives/transaction.h>
 7 #include <consensus/validation.h>
 8+#include <util/check.h>
 9+#include <util/fs.h>
10 
11 bool CheckTransaction(const CTransaction& tx, TxValidationState& state)
12 {
13+Assert(true);
14+fs::path foo{"bar"};
15+(void)foo;
16     // Basic checks that don't depend on any context
17     if (tx.vin.empty())
18         return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-vin-empty");

TheCharlatan commented at 3:20 pm on November 30, 2023:

Mmh, looking at the history, similar things are said in the commit message introducing the headers in libbitcoin_crypto: https://github.com/bitcoin/bitcoin/commit/4791b99e2dea0593775a8c75f62c8406d340191e. Maybe a regression was introduced at some point?

maflcko commented at 3:23 pm on November 30, 2023:

Or it is a typo? “headers”/“modules”? cc @theuni

DrahtBot added the label Needs rebase on Nov 30, 2023

TheCharlatan force-pushed on Nov 30, 2023

TheCharlatan commented at 8:01 pm on November 30, 2023: contributor

Rebased 511e3fc74cdd03c36764eaf3bb637e7bbe98b335 -> 0af5d4d2d6c4201c7c6f1ba8b497cce23dc9f3ad (kernelInternalLib_8 -> kernelInternalLib_9, compare)

Fixed conflict with https://github.com/bitcoin/bitcoin/pull/28451

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

Rebased 0af5d4d2d6c4201c7c6f1ba8b497cce23dc9f3ad -> 5a235048500a38fae691396cb59f6697032b4deb (kernelInternalLib_9 -> kernelInternalLib_10, compare)

Fixed conflict with https://github.com/bitcoin/bitcoin/pull/28368

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.

in src/util/string.h:8 in 8b3623769e outdated

4@@ -5,7 +5,7 @@
5 #ifndef BITCOIN_UTIL_STRING_H
6 #define BITCOIN_UTIL_STRING_H
7 
8-#include <util/spanparsing.h>
9+#include <common/spanparsing.h>

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

Guix builds (on x86_64)

File	commit 6d5790956f45e3de5c6c4ee6fda21878b0d1287b(master)	commit d5b4ca0a29b5de1ca7d6a42aae71d22f7d606f15(master and this pull)
SHA256SUMS.part	`070f2d87d6e3f214...`	`19a6fb1710c51e6a...`
*-aarch64-linux-gnu-debug.tar.gz	`e50e6cf360642fd3...`	`2af0b043bf9b07cf...`
*-aarch64-linux-gnu.tar.gz	`d1994e5d2e586f1c...`	`e019631b69fa2e31...`
*-arm-linux-gnueabihf-debug.tar.gz	`cc20ddb16663d398...`	`a93994efdf7e0f2a...`
*-arm-linux-gnueabihf.tar.gz	`908022604cd339e1...`	`927f97d5ab73a208...`
*-arm64-apple-darwin-unsigned.tar.gz	`f3a3d8672e487573...`	`77dd77541a24a84f...`
*-arm64-apple-darwin-unsigned.zip	`e696a59082f4df02...`	`8aaf698673af3c8f...`
*-arm64-apple-darwin.tar.gz	`76f681aeb99ce7a2...`	`173cdc19e0e0d801...`
*-powerpc64-linux-gnu-debug.tar.gz	`afa1b69ff9326a5c...`	`ba90f1389a9baf85...`
*-powerpc64-linux-gnu.tar.gz	`64d0ab4b97a56739...`	`2dc8c59666e7ddb2...`
*-powerpc64le-linux-gnu-debug.tar.gz	`19e1304c77570866...`	`263b55c0fedf493a...`
*-powerpc64le-linux-gnu.tar.gz	`c1b8a29c208746c7...`	`74e087c831f84e3b...`
*-riscv64-linux-gnu-debug.tar.gz	`7641821f6aa71788...`	`09a24ac522ca8565...`
*-riscv64-linux-gnu.tar.gz	`87cf896dd53b4ee9...`	`6dc6c75525aa52ba...`
*-x86_64-apple-darwin-unsigned.tar.gz	`264bb8d7474ea91a...`	`1d9cede0c41f8b6a...`
*-x86_64-apple-darwin-unsigned.zip	`888c6e62d5eba0eb...`	`fd6a1047be61c33a...`
*-x86_64-apple-darwin.tar.gz	`056d123f3f51d08a...`	`0365849a12370358...`
*-x86_64-linux-gnu-debug.tar.gz	`f14054fdbe8c170e...`	`98c2d2ebdd07f837...`
*-x86_64-linux-gnu.tar.gz	`da8fdd90d2ff7e72...`	`ccb3035d61661db9...`
*.tar.gz	`f8989c183ea48bef...`	`79e1c877aa931cd0...`
guix_build.log	`291ae30442da67cc...`	`c3d56b67abbf2db5...`
guix_build.log.diff		`a68430b6b8712fee...`

DrahtBot removed the label DrahtBot Guix build requested on Dec 6, 2023

TheCharlatan force-pushed on Dec 6, 2023

TheCharlatan commented at 10:10 am on December 6, 2023: contributor

Thank you for the review @ryanofsky,

Rebased 5a235048500a38fae691396cb59f6697032b4deb -> 2086d1d4c3f40cce34647995ead4bff22967ffd8 (kernelInternalLib_10 -> kernelInternalLib_11, compare)

Addressed @ryanofsky’s comment, undo changes to the spanparsing module.

hebasto commented at 3:07 pm on December 6, 2023: member

I’ve reviewed the first commit (7651c93749557839f7db3b73eb6df6357649e2d3).

Not counting the symbols from the libbitcoin_crypto library, the libbitcoin_util has undefined symbols as follows:

on the master branch:

0- `ArgsManager::AddArg`
1- `ArgsManager::SelectConfigNetwork`
2- `CKey::SignCompact`
3- `CPubKey::RecoverCompact`
4- `DecodeDestination`
5- `gArgs`
6- `G_TRANSLATION_FUN`
7- `IsValidDestination`
8- `PKHash::PKHash`

for 7651c93749557839f7db3b73eb6df6357649e2d3:

0- `G_TRANSLATION_FUN`

It looks good.

hebasto commented at 3:57 pm on December 6, 2023: member

Approach ACK 2086d1d4c3f40cce34647995ead4bff22967ffd8.

Still reviewing the 4th commit (983d0978a973a12c3128b2c8e13b73ed08155e67).

ismaelsadeeq commented at 4:58 pm on December 6, 2023: member

Concept ACK Did a light review this looks good to me

DrahtBot added the label Needs rebase on Dec 6, 2023

TheCharlatan force-pushed on Dec 6, 2023

TheCharlatan commented at 8:44 pm on December 6, 2023: contributor

Rebased 2086d1d4c3f40cce34647995ead4bff22967ffd8 -> 34970bde23dd87fc0fea5a1970880761f7184774 (kernelInternalLib_11 -> kernelInternalLib_12, compare)

Fixed conflict with #27581

in src/crypto/hex_base.h:6 in 36f95a360e outdated

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

maflcko commented at 9:05 pm on December 6, 2023:

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?

ryanofsky commented at 10:37 pm on December 6, 2023:

re: #28690 (review)

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:

Re #28690 (review)

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.

maflcko commented at 7:39 am on December 7, 2023:

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:

0/usr/bin/ld: /usr/bin/ld: DWARF error: invalid or unhandled FORM value: 0x23
1util/.libs/libbitcoinkernel_la-strencodings.o: in function `SanitizeString[abi:cxx11](std::basic_string_view<char, std::char_traits<char> >, int)':
2strencodings.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

Code review ACK 34970bde23dd87fc0fea5a1970880761f7184774.

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 commented at 12:35 pm on July 2, 2024: contributor

Rebased and updated 34970bde23dd87fc0fea5a1970880761f7184774 -> 1a1e2dac0c657c3ee96c7faf9594aa0bed770702 (kernelInternalLib_12 -> kernelInternalLib_13)

#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

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/29358890178

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

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

Introduce internal kernel library

Do not compile the kernel library if reduced exports are switched on.
Symbols from the kernel library won't be reduced anyway, so there is no
point in doing so in combination. Leave the option there will break
compilation currently, since no symbols will be exported from the
dependent libraries of the kernel library and thus make no symbols
available for bitcoin-chainstate.

d91296b19a

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 commented at 2:42 pm on October 11, 2024: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot added the label Needs rebase on Oct 11, 2024

build: Introduce internal kernel library #28690

Code Coverage

Reviews

Conflicts

Guix builds (on x86_64)