build: Introduce internal kernel library #28690
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
TheCharlatan
force-pushed
the
kernelInternalLib
branch
from
3e169c7
to
e37fbee
Compare
TheCharlatan
force-pushed
the
kernelInternalLib
branch
from
e37fbee
to
a5e4960
Compare
TheCharlatan
force-pushed
the
kernelInternalLib
branch
from
a5e4960
to
d6032c5
Compare
|
To fix the MSVC build, suggesting to apply the diff as follows: --- a/build_msvc/bitcoin.sln
+++ b/build_msvc/bitcoin.sln
@@ -82,6 +82,10 @@ Global
{460FEE33-1FE1-483F-B3BF-931FF8E969A5}.Debug|x64.Build.0 = Debug|x64
{460FEE33-1FE1-483F-B3BF-931FF8E969A5}.Release|x64.ActiveCfg = Release|x64
{460FEE33-1FE1-483F-B3BF-931FF8E969A5}.Release|x64.Build.0 = Release|x64
+ {A938586F-1016-4C60-9B82-56123264EE14}.Debug|x64.ActiveCfg = Debug|x64
+ {A938586F-1016-4C60-9B82-56123264EE14}.Debug|x64.Build.0 = Debug|x64
+ {A938586F-1016-4C60-9B82-56123264EE14}.Release|x64.ActiveCfg = Release|x64
+ {A938586F-1016-4C60-9B82-56123264EE14}.Release|x64.Build.0 = Release|x64
{5724BA7D-A09A-4BA8-800B-C4C1561B3D69}.Debug|x64.ActiveCfg = Debug|x64
{5724BA7D-A09A-4BA8-800B-C4C1561B3D69}.Debug|x64.Build.0 = Debug|x64
{5724BA7D-A09A-4BA8-800B-C4C1561B3D69}.Release|x64.ActiveCfg = Release|x64 |
TheCharlatan
force-pushed
the
kernelInternalLib
branch
from
d6032c5
to
f1b1456
Compare
|
Thanks for the fixes @hebasto! 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
the
kernelInternalLib
branch
from
f1b1456
to
1239bdf
Compare
TheCharlatan
force-pushed
the
kernelInternalLib
branch
from
1239bdf
to
04d7a98
Compare
TheCharlatan
force-pushed
the
kernelInternalLib
branch
from
5a23504
to
2086d1d
Compare
|
Thank you for the review @ryanofsky, Rebased 5a23504 -> 2086d1d (kernelInternalLib_10 -> kernelInternalLib_11, compare)
|
There was a problem hiding this comment.
I've reviewed the first commit (7651c93).
Not counting the symbols from the libbitcoin_crypto library, the libbitcoin_util has undefined symbols as follows:
- on the master branch:
- `ArgsManager::AddArg`
- `ArgsManager::SelectConfigNetwork`
- `CKey::SignCompact`
- `CPubKey::RecoverCompact`
- `DecodeDestination`
- `gArgs`
- `G_TRANSLATION_FUN`
- `IsValidDestination`
- `PKHash::PKHash`
- for 7651c93:
- `G_TRANSLATION_FUN`
It looks good.
|
Concept ACK |
Move util library files that are not used by the kernel library and are not unopinionated utilities to the common library. This is consistent with the library structure laid out in doc/design/libraries.md. 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.
The crypto library should not have any dependencies on the util library. With this move the cleanse file can be removed from the explicit libbitcoinconsensus dependency list, since it is now linked in through the crypto library.
The files should not be duplicated between the consensus and util libraries. Instead split off the part that is required by the consensus library into a new file that is compiled as part of the crypto library.
See doc/design/libraries.md for an overview of the internal libraries. Some of the internal libraries and bitcoin binaries do not link with the libbitcoin kernel (e.g. libbitcoin_wallet, bitcoin-wallet, libbitcoin_cli, libbitcoinqt). However they still rely on some of the files that are also required by the kernel library. Move this class of files into the util library, which all libraries and binaries can link with. Files moved for this reason from the common to the util library are: coins.cpp core_read.cpp kernel/chainparams.cpp key.cpp policy/feerate.cpp policy/policy.cpp scheduler.cpp script/solver.cpp
The external kernel library should use the same file list that the internal kernel library uses.
TheCharlatan
force-pushed
the
kernelInternalLib
branch
from
2086d1d
to
34970bd
Compare
|
Rebased 2086d1d -> 34970bd (kernelInternalLib_11 -> kernelInternalLib_12, compare)
|
| // file COPYING or http://www.opensource.org/licenses/mit-license.php. | ||
|
|
||
| #ifndef BITCOIN_CRYPTO_HEX_BASE_H | ||
| #define BITCOIN_CRYPTO_HEX_BASE_H |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
re: #28690 (comment)
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 (review), I think the idea was to have it in crypto since util and consensus libraries depend on the crypto library.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The last commit 34970bd effectively extends the libbitcoinkernel_la_SOURCES with the following source files:
script/bitcoinconsensus.cpputil/bytevectorhash.cpputil/readwritefile.cpputil/spanparsing.cpputil/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.
|
Putting this on draft while we figure out the desired contents of the util library in #29015. |
|
🐙 This pull request conflicts with the target branch and needs rebase. |
|
⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
This PR introduces a new
libbitcoin_kernelinternal library. It completes the internal library design as laid out in doc/design/libraries.md. The externallibbitcoinkernellibrary now uses the source lists of the internal library for its compilation.Should also address #28548.