[kernel 2b/n] Add `ChainstateManager::m_adjusted_time_callback` #25064

dongcarl · 2022-05-04T16:48:21Z

This decouples validation.cpp from netaddress.cpp (transitively,
timedata.cpp, and asmap.cpp).

This is important for libbitcoinkernel as:

- There is no reason for the consensus engine to be coupled with
  netaddress, timedata, and asmap
- Users of libbitcoinkernel can now easily supply their own
  std::function that provides the adjusted time.

See the src/Makefile.am changes for some satisfying removals.

DrahtBot · 2022-05-04T21:46:25Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25168 (refactor: Avoid passing params where not needed by MarcoFalke)
#25065 ([kernel 2c/n] Extract only what we use of init/common.cpp by dongcarl)
#24410 ([kernel 2a/n] Split hashing/index GetUTXOStats codepaths, decouple from coinstatsindex by dongcarl)

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.

maflcko · 2022-05-05T06:18:19Z

So even if adjusted time was removed, there'd be a time callback?

dongcarl · 2022-05-05T13:23:09Z

@MarcoFalke

So even if adjusted time was removed, there'd be a time callback?

If usage of adjusted time in validation was removed, then we'd just remove the m_adjusted_time_callback member.

If you're asking about if we'll ever make the normal GetTime into a callback that users of libbitcoinkernel can plug into... Possibly! But it's quite low priority. GetAdjustedTime is much more of a problem since it relies on netaddress.

sipa · 2022-05-05T14:54:14Z

Why does adjusted time depend on asmap...?

dongcarl · 2022-05-05T15:08:37Z

@sipa

Why does adjusted time depend on asmap...?

Oh sorry, I misspoke. More precisely: adjusted time depends on netaddress, and linking in netaddress requires linking in asmap.

dongcarl · 2022-05-13T15:53:48Z

Pushed 4b5a8a1 -> 45e2400

Rebased over merge of deploymentstatus: move g_versionbitscache global to ChainstateManager #24595
Added commit that introduces ChainstateManager::Options (please see commit message + added in-code comments)

dongcarl · 2022-05-16T15:50:49Z

Pushed 45e2400 -> 52804f7

Don't use designated initializers 😢

jamesob

ACK 52804f7 (jamesob/ackr/25064.1.dongcarl.kernel_2b_n_add_chainst)

Built, reviewed, unittested locally. Change looks good and is very simple. A few minor points:

there's a lingering comment in validation.cpp that could use updating: https://github.com/jamesob/bitcoin/blob/52804f7ad0da32bc4d7be726e08e0b1e34619acb/src/validation.cpp#L2008
can't use designated initializers (yet?) because of a VS Code/CI issue:

10:42 <_aj_> jamesob: #24531 -- VS 2019 (which CI uses) doesn't support them, and figuring out how to fix that isn't trivial
10:42 <@gribble> https://github.com/bitcoin/bitcoin/issues/24531 | Use designated initializers by MarcoFalke · Pull Request #24531 · bitcoin/bitcoin · GitHub
10:45 <sipsorcery> _aj_: in fairness VS2019 does seem support them it just seems to want to use a massive amount of memory to compile them...
10:47 <_aj_> sipsorcery: doesn't support them with the amount of memory we currently allocate for those CI jobs :-P

Show signature data

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK 52804f7ad0da32bc4d7be726e08e0b1e34619acb ([`jamesob/ackr/25064.1.dongcarl.kernel_2b_n_add_chainst`](https://github.com/jamesob/bitcoin/tree/ackr/25064.1.dongcarl.kernel_2b_n_add_chainst))

Built, reviewed, unittested locally. Change looks good and is very simple. A few minor points:

- - there's a lingering comment in `validation.cpp` that could use updating: https://github.com/jamesob/bitcoin/blob/52804f7ad0da32bc4d7be726e08e0b1e34619acb/src/validation.cpp#L2008
- - can't use designated initializers (yet?) because of a VS Code/CI issue:

10:42 <aj> jamesob: #24531 -- VS 2019 (which CI uses) doesn't support them, and figuring out how to fix that isn't trivial
10:42 <@gribble> #24531 | Use designated initializers by MarcoFalke · Pull Request #24531 · bitcoin/bitcoin · GitHub
10:45 aj: in fairness VS2019 does seem support them it just seems to want to use a massive amount of memory to compile them...
10:47 <aj> sipsorcery: doesn't support them with the amount of memory we currently allocate for those CI jobs :-P

Show platform data

Tested on Linux-5.10.0-11-amd64-x86_64-with-glibc2.31

Configured with ./configure LDFLAGS=-L/home/james/src/bitcoin/db4/lib/ CPPFLAGS=-I/home/james/src/bitcoin/db4/include/ CXXFLAGS=-fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis --enable-wallet --enable-debug --with-daemon --enable-natpmp-default CXX=/usr/local/bin/clang++ CC=/usr/local/bin/clang PYTHONPATH= --disable-shared --with-pic --enable-benchmark=no --enable-module-recovery --enable-module-schnorrsig --disable-shared --with-pic --enable-benchmark=no --enable-module-recovery --enable-module-schnorrsig --no-create --no-recursion

Compiled with /usr/bin/ccache /usr/local/bin/clang++ -std=c++17 -mavx -mavx2 -mpclmul -fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis -O0 -g3 -ftrapv -fdebug-prefix-map=$(abs_top_srcdir)=.  -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -msse4.1 -msse4.2 -msse4 -msha  i

Compiler version: Debian clang version 13.0.1-++20220126092033+75e33f71c2da-1~exp1~20220126212112.63

dongcarl · 2022-05-17T16:49:41Z

Pushed 52804f7 -> f554689

Updated lingering comment in validation.cpp as pointed out here: [kernel 2b/n] Add ChainstateManager::m_adjusted_time_callback #25064 (review) (thanks James!)

dongcarl · 2022-05-17T16:50:45Z

@jamesob

can't use designated initializers (yet?) because of a VS Code/CI issue:

Yeah that's exactly right. We need to resolve #24531 first before we can use them :-/

src/bitcoin-chainstate.cpp

src/init.cpp

JeremyRubin · 2022-05-17T17:23:59Z

src/chainstatemanager_opts.h

@@ -17,6 +17,7 @@ class CChainParams;
 */
 struct ChainstateManagerOpts {
    const CChainParams& chainparams;
+    const std::function<int64_t()> adjusted_time_callback{nullptr};


good candidate for a not_null #24423

given that we assert not null where it is actually used.

Totally agree!

src/chainstatemanager_opts.h

dongcarl · 2022-05-18T16:06:04Z

Pushed f554689 -> 211674c

[META] Although it seems like we don't need it for just one option, we're going to introduce another member to this struct *in the next commit*. In future patchsets for libbitcoinkernel decoupling it from ArgsManager, even more members will be added here.

This decouples validation.cpp from netaddress.cpp (transitively, timedata.cpp, and asmap.cpp). This is important for libbitcoinkernel as: - There is no reason for the consensus engine to be coupled with netaddress, timedata, and asmap - Users of libbitcoinkernel can now easily supply their own std::function that provides the adjusted time. See the src/Makefile.am changes for some satisfying removals.

We want m_chainparams to be alive for the duration of ChainstateManager's lifetime since ChainstateManager's behaviour depends on m_chainparams. We could allow for a std::shared_ptr to be passed in as m_chainparams, but that complicates things further. Given that CChainParams is not an entity class or struct, we can just copy it and have ChainstateManager own it.

dongcarl · 2022-05-20T15:59:47Z

Pushed af8e546 -> 53494bc

Rebased over master to resolve merge conflict

JeremyRubin · 2022-05-20T18:34:16Z

utack 53494bc

DrahtBot added Build system Mining RPC/REST/ZMQ Utils/log/libs Validation labels May 4, 2022

dongcarl mentioned this pull request May 4, 2022

The libbitcoinkernel Project #24303

Closed

14 tasks

DrahtBot mentioned this pull request May 5, 2022

deploymentstatus: move g_versionbitscache global to ChainstateManager #24595

Merged

maflcko removed Build system RPC/REST/ZMQ Mining Validation Utils/log/libs labels May 5, 2022

DrahtBot added the Refactoring label May 5, 2022

DrahtBot added the Needs rebase label May 13, 2022

dongcarl force-pushed the 2022-05-libbitcoinkernel-adjtime branch from 4b5a8a1 to 45e2400 Compare May 13, 2022 15:52

DrahtBot removed the Needs rebase label May 13, 2022

dongcarl force-pushed the 2022-05-libbitcoinkernel-adjtime branch from 45e2400 to 52804f7 Compare May 16, 2022 15:49

jamesob approved these changes May 17, 2022

View reviewed changes

dongcarl force-pushed the 2022-05-libbitcoinkernel-adjtime branch from 52804f7 to f554689 Compare May 17, 2022 16:48

JeremyRubin reviewed May 17, 2022

View reviewed changes

src/bitcoin-chainstate.cpp Show resolved Hide resolved

JeremyRubin reviewed May 17, 2022

View reviewed changes

src/init.cpp Show resolved Hide resolved

JeremyRubin reviewed May 17, 2022

View reviewed changes

maflcko reviewed May 18, 2022

View reviewed changes

src/chainstatemanager_opts.h Outdated Show resolved Hide resolved

dongcarl force-pushed the 2022-05-libbitcoinkernel-adjtime branch from f554689 to a4cb66a Compare May 18, 2022 15:59

dongcarl force-pushed the 2022-05-libbitcoinkernel-adjtime branch from 211674c to af8e546 Compare May 18, 2022 17:55

This was referenced May 19, 2022

refactor: Avoid passing params where not needed #25168

Merged

[kernel 2c/n] Introduce kernel::Context, encapsulate global init/teardown #25065

Merged

[kernel 2a/n] Split hashing/index GetUTXOStats codepaths, decouple from coinstatsindex #24410

Merged

DrahtBot added the Needs rebase label May 20, 2022

dongcarl added 3 commits May 20, 2022 11:54

dongcarl force-pushed the 2022-05-libbitcoinkernel-adjtime branch from af8e546 to 53494bc Compare May 20, 2022 15:59

DrahtBot removed the Needs rebase label May 20, 2022

maflcko merged commit 640eb77 into bitcoin:master May 20, 2022

bitcoin deleted a comment May 20, 2022

bitcoin locked as spam and limited conversation to collaborators May 20, 2022

[kernel 2b/n] Add ChainstateManager::m_adjusted_time_callback #25064

[kernel 2b/n] Add ChainstateManager::m_adjusted_time_callback #25064

Uh oh!

Conversation

dongcarl commented May 4, 2022

Uh oh!

DrahtBot commented May 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

maflcko commented May 5, 2022

Uh oh!

dongcarl commented May 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented May 5, 2022

Uh oh!

dongcarl commented May 5, 2022

Uh oh!

dongcarl commented May 13, 2022

Uh oh!

dongcarl commented May 16, 2022

Uh oh!

jamesob left a comment

Choose a reason for hiding this comment

Uh oh!

dongcarl commented May 17, 2022

Uh oh!

dongcarl commented May 17, 2022

Uh oh!

Uh oh!

Uh oh!

JeremyRubin May 17, 2022

Choose a reason for hiding this comment

Uh oh!

dongcarl May 17, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dongcarl commented May 18, 2022

Uh oh!

dongcarl commented May 20, 2022

Uh oh!

JeremyRubin commented May 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

[kernel 2b/n] Add `ChainstateManager::m_adjusted_time_callback` #25064

[kernel 2b/n] Add `ChainstateManager::m_adjusted_time_callback` #25064

DrahtBot commented May 4, 2022 •

edited

Loading

dongcarl commented May 5, 2022 •

edited

Loading

JeremyRubin commented May 20, 2022 •

edited

Loading