Skip to content

build: Introduce internal kernel library #28690

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

build: Introduce internal kernel library #28690

wants to merge 1 commit into from
+99 −118

Conversation

TheCharlatan
Copy link
Contributor

@TheCharlatan TheCharlatan commented Oct 20, 2023 •
edited
Loading

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
Copy link
Contributor

DrahtBot commented Oct 20, 2023 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/28690.

Reviews

See the guideline for information on the review process.

Type Reviewers
Concept ACK stickies-v, theuni, kashifs, pablomartin4btc, ismaelsadeeq, yuvicc
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:

  • #33218 (refactor: rename fees.{h,cpp} to fees/block_policy_estimator{h,cpp} by ismaelsadeeq)
  • #32652 (wallet: add codex32 argument to addhdkey by roconnor-blockstream)
  • #32543 (kernel: Remove dependency on clientversion by TheCharlatan)
  • #32427 ((RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store by TheCharlatan)
  • #31664 (Fees: add Fee rate Forecaster Manager by ismaelsadeeq)
  • #30277 ([DO NOT MERGE] Erlay: bandwidth-efficient transaction relay protocol (Full implementation) by sr-gi)
  • #30157 (Fee Estimation via Fee rate Forecasters by ismaelsadeeq)
  • #30116 (p2p: Fill reconciliation sets (Erlay) attempt 2 by sr-gi)
  • #29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)
  • #28676 (Cluster mempool implementation by sdaftuar)
  • #26022 (Add util::ResultPtr class by ryanofsky)
  • #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.

This was referenced Oct 20, 2023
Closed
Open
Merged
Closed
Merged
This was referenced Oct 20, 2023
Closed
Merged
Merged
This was referenced Oct 20, 2023
Merged
Closed
Closed
Open
Closed
Open
This was referenced Oct 20, 2023
Closed
Merged
hebasto
hebasto reviewed Oct 21, 2023
View reviewed changes
@hebasto
Copy link
Member

hebasto commented Oct 21, 2023

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
Copy link
Contributor Author

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

@hebasto
Copy link
Member

hebasto commented Oct 21, 2023

Looks like the remaining test failure is a timeout?

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

@DrahtBot DrahtBot mentioned this pull request Oct 23, 2023
Closed
This was referenced May 19, 2025
Merged
Open
Draft
@TheCharlatan TheCharlatan force-pushed the kernelInternalLib branch 2 times, most recently from 01a43b2 to e4d80d7 Compare May 29, 2025 14:27
@TheCharlatan
Copy link
Contributor Author

Rebased 7e6e43a -> e4d80d7 (kernelInternalLib_17 -> kernelInternalLib_18, compare)

Fixed confilict with #31375

@DrahtBot DrahtBot mentioned this pull request May 31, 2025
Draft
@DrahtBot DrahtBot mentioned this pull request Jun 22, 2025
Draft
@DrahtBot DrahtBot mentioned this pull request Jul 10, 2025
Merged
@yuvicc
Copy link
Contributor

yuvicc commented Jul 12, 2025

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.

@yuvicc yuvicc mentioned this pull request Jul 30, 2025
Open
@TheCharlatan
Copy link
Contributor Author

Rebased e4d80d7 -> 8138742 (kernelInternalLib_18 -> kernelInternalLib_19, compare)

This was referenced Jul 31, 2025
Merged
Draft
Draft
Draft
Open
Draft
@TheCharlatan
Introduce internal kernel library
78ed83b 
To facilitate compiling the external kernel library with different
symbol visibility, re-use the same source file list from its dependent
internal counterpart.
@TheCharlatan
Copy link
Contributor Author

Rebased 8138742 -> 78ed83b (kernelInternalLib_19 -> kernelInternalLib_20, compare)

Fixed conflict with #33077

@DrahtBot DrahtBot mentioned this pull request Aug 8, 2025
Draft
@DrahtBot DrahtBot mentioned this pull request Aug 20, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fanquake fanquake fanquake left review comments

@ryanofsky ryanofsky ryanofsky approved these changes

@laanwj laanwj Awaiting requested review from laanwj

@theuni theuni Awaiting requested review from theuni

@stickies-v stickies-v Awaiting requested review from stickies-v

@pablomartin4btc pablomartin4btc Awaiting requested review from pablomartin4btc

@ismaelsadeeq ismaelsadeeq Awaiting requested review from ismaelsadeeq

@hebasto hebasto Awaiting requested review from hebasto

+1 more reviewer

@maflcko maflcko maflcko left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Build system
Projects
The libbitcoinkernel Project
Status: Ready for Review PRs
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.