[kernel 2/n] Decouple from `netaddress`+`asmap`, parts of `timedata`+`init/common`

dongcarl commented at 8:32 pm on February 13, 2022: member

Part of: #24303 Depends on: #24322

This PR:

Removes netaddress.cpp and asmap.cpp from the list of _SOURCES necessary to be linked into libbitcoinkernel. It does so by splitting off libbitcoinkernel-used parts of timedata.cpp off to kernel/timedata.cpp. Although not absolutely necessary at this step (but eventually necessary in stage 1 step 3 of #24303 (text fragment link), we also split off the corresponding declarations off to kernel/timedata.h, and adjust existing dependencies on timedata.h to kernel/timedata.h wherever appropriate.
Similarly splits off libbitcoinkernel-used parts of init/common.cpp (namely init::SetGlobals() and init::UnsetGlobals()) off to kernel/init/common.cpp. Meaning that the rest of init/common.cpp (which deals mostly with setting up the logger, which we abstract away in a future PR) will not be linked into libbitcoinkernel.

Note for libbitcoinkernel reviewers: Changes like these are representative of the changes being done in stage 1 step 2 of #24303 (text fragment link)

Please read the commit messages for more details.

dongcarl force-pushed on Feb 13, 2022

DrahtBot added the label Build system on Feb 13, 2022

DrahtBot added the label P2P on Feb 13, 2022

DrahtBot added the label RPC/REST/ZMQ on Feb 13, 2022

DrahtBot added the label Utils/log/libs on Feb 13, 2022

DrahtBot added the label Validation on Feb 13, 2022

DrahtBot commented at 10:13 am on February 14, 2022: member

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

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

DrahtBot added the label Needs rebase on Feb 14, 2022

build: Add example bitcoin-chainstate executable

The bitcoin-chainstate executable serves to surface the dependencies
required by a program wishing to use Bitcoin Core's consensus engine as
it is right now.

More broadly, the _SOURCES list serves as a guiding "North Star" for the
libbitcoinkernel project: as we decouple more and more modules of the
codebase from our consensus engine, this _SOURCES list will grow shorter
and shorter. One day, only what is critical to our consensus engine will
remain. Right now, it's "the minimal list of files to link in to even
use our consensus engine".

[META] In a future commit the libbitcoinkernel library will be extracted
       from bitcoin-chainstate, and the libbitcoinkernel library's
       _SOURCES list will be the list that we aim to shrink.

095aa6ca37

ci: Build bitcoin-chainstate

...to make sure that the linker errors that arise from coupling
regressions are caught by CI.

Adding to the "no wallet" ci job as suggested by MarcoFalke.

2c03cec2ff

COMMITS AFTER ARE NON-BASE 667e7c3b38

build: Create .la library for leveldb

Libtool will yell at you if you try to link a shared library against
static ones.

This change creates a libtool archive library for leveldb and allows a
shared library to be linked against it portably.

[META] This change is done in preparation for a future commit where we
       link the libbitcoinkernel library against this one.

1c86eef369

build: Create .la library for crc32c

Libtool will yell at you if you try to link a shared library against
static ones.

This change creates a libtool archive library for crc32c and allows a
shared library to be linked against it portably.

[META] This change is done in preparation for a future commit where we
       link the libbitcoinkernel library against this one.

23985a91e7

build: Create .la library for bitcoincrypto

Libtool will yell at you if you try to link a shared library against
static ones.

This change creates a libtool archive library for bitcoincrypto and
allows a shared library to be linked against it portably.

[META] This change is done in preparation for a future commit where we
       link the libbitcoinkernel library against this one.

aee0288e43

build: Separate lib_LTLIBRARIES initialization

Set lib_LTLIBRARIES with '=' to an empty value at the top of the
Makefile.am and append to it from the library-local block for
readability.

Here's the error you get if you don't set lib_LTLIBRARIES to be empty:

    error: lib_LTLIBRARIES must be set with '=' before using '+='

[META] In a subsequent commit, we're going to introduce a library and
       append it to lib_LTLIBRARIES in its local block, this makes
       things more readable.

87ddac9a6f

build: Extract the libbitcoinkernel library

I strongly recommend reviewing with the following git-diff flags:
  --patience --color-moved=dimmed-zebra

Extract out a libbitcoinkernel library linking in all files necessary
for using our consensus engine as-is. Link bitcoin-chainstate against
it.

See previous commit "build: Add example bitcoin-chainstate executable"
for more context.

[META] This _SOURCES list of files may seem daunting, and some of them
       may seem like they don't belong (e.g. index/*). However, they are
       not here to stay, they will be removed incrementally as we
       decouple them from our consensus engine one-by-one.

-----

Note to users: You need to either specify:

  --enable-experimental-util-chainstate

or,

  --with-experimental-kernel-lib

To build the libbitcionkernel library. See the configure help for more
details.

6c038b1faa

build: Absorb RELDFLAGS into AM_LDFLAGS

...also print RELDFLAGS in configure epilogue

14d60c4080

build: Rename RELDFLAGS to RE_LDFLAGS 38267af769

build: Put reduced exports flags in RE_CXXFLAGS

Also:
- Print RE_CXXFLAGS in configure epilogue
- Set -fvisibility=default in libbitcoinkernel_la_CXXXFLAGS since
  libbitcoinkernel requires default symbol visibility

Note: We really shouldn't be setting CXXFLAGS at all in configure.ac
      except for flags that are meant to be overridden by the user.

e5d96cdc68

b-cs: Explicitly mark G_TRANSLATION_FUN as visible

G_TRANSLATION_FUN needs to have default visibility in order to satisfy
the extern declaration in 'util/translation.h'. Otherwise the linker
will fail with:

    /usr/bin/ld: .libs/bitcoin-chainstate: hidden symbol `G_TRANSLATION_FUN' in bitcoin_chainstate-bitcoin-chainstate.o is referenced by DSO
    /usr/bin/ld: final link failed: Bad value

655518b74a

ci: Build libbitcoinkernel 32f49d613e

COMMITS AFTER ARE NON-BASE 9df8991356

kernel: Decouple from netaddr+asmap

...by splitting the libbitcoinkernel-used portion of timedata.cpp off to
   kernel/timedata.cpp

Notes for reviewers:
    - I strongly recommend reviewing with the following git-diff flags:
          --patience --color-moved=dimmed-zebra
    - See the diff for src/Makefile.am to see the overall effect.

ab6d2ae061

kernel: Split timedata header as well

...according to how we split the .cpp file in the commit before.

This allows us to keep the header tree that we need to ship with
libbitcoinkernel more minimal. For example, if we need to ship the full
timedata.h, we'd need to ship with netaddress.h and all its dependent
headers.

We don't absolutely need to do this right now, but it might be clearer
for reviewers since we just did the .cpp splitting in the previous commit.

Notes for reviewers:
    - I strongly recommend reviewing with the following git-diff flags:
          --patience --color-moved=dimmed-zebra

[META] In the next commit, we will use the more stripped-down
       kernel/timedata.h wherever we can.

ae4c1d4021

kernel: Use kernel/timedata.h where possible

A lot of files do not need the full timedata.h, just kernel/timedata.h.

Without this change, we would still need to ship the full timedata.h
with libbitcoinkernel since validation.h includes timedata.h.

28688392f4

kernel: Decouple from full init/common.cpp

...by splitting the libbitcoinkernel-used portion of init/common.cpp off
   to kernel/init/common.cpp

Notes for reviewers:
    - I strongly recommend reviewing with the following git-diff flags:
          --patience --color-moved=dimmed-zebra
    - See the diff for src/Makefile.am to see the overall effect.

9211d91b22

kernel: Split init/common header as well

...according to how we split the .cpp file in the commit before.

This allows us to keep the header tree that we need to ship with
libbitcoinkernel more minimal.

We don't absolutely need to do this right now, but it might be clearer
for reviewers since we just did the .cpp splitting in the previous
commit.

Notes for reviewers:
    - I strongly recommend reviewing with the following git-diff flags:
          --patience --color-moved=dimmed-zebra

[META] In the next commit, we will use the more stripped-down
       kernel/init/common.h wherever we can.

52e3d53ac3

kernel: Use kernel/init/common.h where possible 02e143cfe7

dongcarl force-pushed on Feb 14, 2022

dongcarl commented at 8:12 pm on February 14, 2022: member

Rebased

ryanofsky commented at 9:10 pm on February 14, 2022: member

Concept ACK. I think the code changes here basically look good, but I do have some feedback.

I think new code in src/kernel/ should try not rely on global variables. Or at least, any API exposed by libbitcoin_kernel should not be forced to rely on global variables, because it it is missing some pointer to the appropriate context. I made a small change in b6072da940b1af4eadf8c1bc6192ae86d634e714 which adds a little infrastructure fixing this problem for timedata in a way that should let us clean up globals later, while not substantially increasing the size of the diff now. Could squash it into this PR.
I think it would be good to put all code in src/kernel/ in a kernel:: namespace, similar to how src/node/ code is in node::, src/wallet/ is in wallet::, newer src/util code is in util::. (Suggested changes above in b6072da940b1af4eadf8c1bc6192ae86d634e714 do use kernel:: namespace FWIW)
I don’t love how there are duplicative src/timedata.h, src/timedata.cpp, src/kernel/timedata.h, and src/kernel/timedata.cpp files. Would be good to name the kernel files to something like src/kernel/adjusted_time or something more specific to make them more distinct.
kernel/init/common naming doesn’t make much sense. The src/init/common.cpp file has that name because every other file in the src/init/ directory contains init code for one of the bitcoin-core executables (bitcoind.cpp, bitcoin-qt.cpp, bitcoin-wallet.cpp) and common.cpp was supposed to hold whatever common boilerplate was used by them all. Probably the src/kernel/init/ directory should not exist and the code should just be moved into the kernel::Context constructors / destructors.
I guess we are actively discussing this in #24303, but I still don’t understand where the boundary between libbitcoin_kernel and libbitcoin_consensus is or why these are two different libraries. I guess depending on where that discussion goes, maybe some of this code may go in libbitcoin_consensus not libbitcoin_kernel.

in src/Makefile.am:596 in 9211d91b22 outdated

592@@ -593,6 +593,7 @@ libbitcoin_common_a_SOURCES = \
593   deploymentinfo.cpp \
594   external_signer.cpp \
595   init/common.cpp \
596+  kernel/init/common.cpp \

ryanofsky commented at 9:01 pm on February 15, 2022:

In commit “kernel: Decouple from full init/common.cpp” (9211d91b226ff45c1eeaa4942a7b5c79f170a78d)

Adding src/kernel/ code to libbitcoin_common is a red flag here because it means wallet and gui code will now be depending on libbitcoinkernel source code. But I already wrote yesterday #24332 (comment) that I don’t think these SetGlobals/UnsetGlobals functions were moved to the right place anyway, so this is just additional confirmation.

A simple fix should be to just move kernel/init/common.cpp to src/util/globals.cpp and kernel/util/common.h to src/util/globals.h and list them in libbitcoin_util_a_SOURCES not libbitcoin_common_a_SOURCES. I hope reasoning makes sense that:

It’s ok for wallet code to depend on util but not for it to depend on kernel
Generally when we create new source files and add them to a libbitcoin_whatever_a_SOURCES we should try to locate those files in src/whatever/
It would also be reasonable to add these SetGlobals/UnsetGlobals functions to common instead of util. But I think functionally they make a little more sense in util since they are lower level, and in general I think it is probably nicer if libbitcoin_kernel doesn’t depend on anything in common at all. (util seems like a better home for essential stuff, while common seems like more of a home for miscellaneous stuff)

dongcarl commented at 8:05 pm on February 16, 2022: member

Concept ACK. I think the code changes here basically look good, but I do have some feedback.

I think new code in src/kernel/ should try not rely on global variables. Or at least, any API exposed by libbitcoin_kernel should not be forced to rely on global variables, because it it is missing some pointer to the appropriate context. I made a small change in b6072da940b1af4eadf8c1bc6192ae86d634e714 which adds a little infrastructure fixing this problem for timedata in a way that should let us clean up globals later, while not substantially increasing the size of the diff now. Could squash it into this PR.

I think it would be good to put all code in src/kernel/ in a kernel:: namespace, similar to how src/node/ code is in node::, src/wallet/ is in wallet::, newer src/util code is in util::. (Suggested changes above in b6072da940b1af4eadf8c1bc6192ae86d634e714 do use kernel:: namespace FWIW)

I don’t love how there are duplicative src/timedata.h, src/timedata.cpp, src/kernel/timedata.h, and src/kernel/timedata.cpp files. Would be good to name the kernel files to something like src/kernel/adjusted_time or something more specific to make them more distinct.

kernel/init/common naming doesn’t make much sense. The src/init/common.cpp file has that name because every other file in the src/init/ directory contains init code for one of the bitcoin-core executables (bitcoind.cpp, bitcoin-qt.cpp, bitcoin-wallet.cpp) and common.cpp was supposed to hold whatever common boilerplate was used by them all. Probably the src/kernel/init/ directory should not exist and the code should just be moved into the kernel::Context constructors / destructors.

I guess we are actively discussing this in #24303, but I still don’t understand where the boundary between libbitcoin_kernel and libbitcoin_consensus is or why these are two different libraries. I guess depending on where that discussion goes, maybe some of this code may go in libbitcoin_consensus not libbitcoin_kernel.

As mentioned here and here, I had a fruitful talk offline with Ryan and hashed out a few of these points. I will incorporate the changes to introduce a kernel::Context struct, and tweak the code organization such that it aligns with our existing code organization style more.

Closing temporarily, will re-open (or open another PR)!

dongcarl closed this on Feb 16, 2022

dongcarl added this to the "WIP PRs" column in a project

dongcarl moved this from the "WIP PRs" to the "Closed (Potentially Rethinking)" column in a project

dongcarl moved this from the "Closed (Potentially Rethinking)" to the "Done or Closed or Rethinking" column in a project

DrahtBot locked this on Feb 16, 2023

[kernel 2/n] Decouple from netaddress+asmap, parts of timedata+init/common #24332

[kernel 2/n] Decouple from `netaddress`+`asmap`, parts of `timedata`+`init/common` #24332