[kernel 2b/n] Add `ChainstateManager::m_adjusted_time

dongcarl commented at 4:48 pm on May 4, 2022: member

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

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

dongcarl moved this from the "WIP PRs" to the "Ready for Review PRs" column in a project

DrahtBot added the label Build system on May 4, 2022

DrahtBot added the label Mining on May 4, 2022

DrahtBot added the label RPC/REST/ZMQ on May 4, 2022

DrahtBot added the label Utils/log/libs on May 4, 2022

DrahtBot added the label Validation on May 4, 2022

DrahtBot commented at 9:46 pm on May 4, 2022: member

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.

MarcoFalke commented at 6:18 am on May 5, 2022: member

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

dongcarl commented at 1:23 pm on May 5, 2022: member

@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.

MarcoFalke removed the label Build system on May 5, 2022

MarcoFalke removed the label RPC/REST/ZMQ on May 5, 2022

MarcoFalke removed the label Mining on May 5, 2022

MarcoFalke removed the label Validation on May 5, 2022

MarcoFalke removed the label Utils/log/libs on May 5, 2022

DrahtBot added the label Refactoring on May 5, 2022

sipa commented at 2:54 pm on May 5, 2022: member

Why does adjusted time depend on asmap…?

dongcarl commented at 3:08 pm on May 5, 2022: member

@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.

DrahtBot added the label Needs rebase on May 13, 2022

dongcarl force-pushed on May 13, 2022

dongcarl commented at 3:53 pm on May 13, 2022: member

Pushed 4b5a8a1a03af47d3cb4a7c9020f2a0bf63a563a7 -> 45e24007e8742d53b5b45ec22ebed962ffffe079

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

DrahtBot removed the label Needs rebase on May 13, 2022

dongcarl force-pushed on May 16, 2022

dongcarl commented at 3:50 pm on May 16, 2022: member

Pushed 45e24007e8742d53b5b45ec22ebed962ffffe079 -> 52804f7ad0da32bc4d7be726e08e0b1e34619acb

Don’t use designated initializers 😢

jamesob approved

jamesob commented at 4:12 pm on May 17, 2022: member

ACK 52804f7ad0da32bc4d7be726e08e0b1e34619acb (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:

010:42 <_aj_> jamesob: [#24531](/bitcoin-bitcoin/24531/) -- VS 2019 (which CI uses) doesn't support them, and figuring out how to fix that isn't trivial
110:42 <@gribble> [#24531](/bitcoin-bitcoin/24531/) | Use designated initializers by MarcoFalke · Pull Request [#24531](/bitcoin-bitcoin/24531/) · bitcoin/bitcoin · GitHub
210: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...
310:47 <_aj_> sipsorcery: doesn't support them with the amount of memory we currently allocate for those CI jobs :-P

0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3ACK 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))
4
5Built, reviewed, unittested locally. Change looks good and is very simple. A few minor points:
6
7- - there's a lingering comment in `validation.cpp` that could use updating: https://github.com/jamesob/bitcoin/blob/52804f7ad0da32bc4d7be726e08e0b1e34619acb/src/validation.cpp#L2008
8- - 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

 0-----BEGIN PGP SIGNATURE-----
 1
 2iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmKDyWUACgkQepNdrbLE
 3TwWRFQ//UtNY+1IMPL/xvHWvqwgWm9O9FzXZIswmIX3OVze3X9GFXk1sEb7RejHR
 4z8jFtQW+g5uh4I4vD85BoRyQPmhD2VSc4TCipRTLOjuzOp67OpgSrTq+my89Myh0
 5ggEQip73F+XPkQl1EjMF/YEH6W1xnUBtIZXLvrXuKm1W03BK5flfxVzsxKYIoj9p
 6Rb6zic0AaVnbFSxKFiz3F5QE/Vw5lfBbZGblHNAricmVneEX9UNt5CVhCn0mZ2Ki
 7Ad2UD3Ugbj/qXoF76QeN44QqMsoi57PxwF8Fs53KsuTjdAJSVnD4Ma3ulaeKAy2C
 86RZsjJAFGXAGXVRIWr8vvneZmvXRSdnhYPOQvw5gGJTIJxcADH/pSuyP5kDP/CLA
 9gfCnq+/eK2zhf6B1u8JzUpEbik6IM0bVYamUShZqVQDOKH4nEf7aoOu7E+j0JbWf
10gywqlOD3rpDeUS2o8c4TjRqRCIqWPXgHxRI53nQE6AXQvOnNzdLzmetRqvWlqFLu
11SgYv4QE25XkFXjbQMDTE+VQ3Qf9/OiHrtRVdYMCpqEHiN2US9RAwGVOF2zqD3wxK
12bcY6j5HtbGP341mhmlD3ERBvpk4qi+L5OQRd1s20LyiTVF4RIodOV9i0P1fLL/qh
13nPCyUeEoxDp9xWDR9l7aMqxxxt8thNnBt1uvEqdElmdEDLZKTbk=
14=9W+m
15-----END PGP SIGNATURE-----

0Tested on Linux-5.10.0-11-amd64-x86_64-with-glibc2.31
1
2Configured 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
3
4Compiled 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
5
6Compiler version: Debian clang version 13.0.1-++20220126092033+75e33f71c2da-1~exp1~20220126212112.63

dongcarl force-pushed on May 17, 2022

dongcarl commented at 4:49 pm on May 17, 2022: member

Pushed 52804f7ad0da32bc4d7be726e08e0b1e34619acb -> f554689988413479ea9bc561f7efd9d56a5c078f

Updated lingering comment in validation.cpp as pointed out here: #25064#pullrequestreview-975757513 (thanks James!)

dongcarl commented at 4:50 pm on May 17, 2022: member

@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 :-/

in src/bitcoin-chainstate.cpp:73 in b4591fce33 outdated

69@@ -70,7 +70,10 @@ int main(int argc, char* argv[])
70 
71 
72     // SETUP: Chainstate
73-    ChainstateManager chainman{chainparams};
74+    const ChainstateManager::Options chainman_opts{

JeremyRubin commented at 5:14 pm on May 17, 2022:

nit: may be more pragmatic to keep name as ‘params’ or ‘settings’ since option made me think std::option.

dongcarl commented at 5:18 pm on May 17, 2022:

I don’t mind either way! I’m following how BlockAssembler::Options and CConnman::Options do it, but if others think we should use Params or something else for these structs in the future, let me know in this thread, I can change it.

JeremyRubin commented at 5:25 pm on May 17, 2022:

fair point – i personally think config/params/settings are better than options, but consistency is probably a overriding interest.

dongcarl commented at 4:07 pm on May 18, 2022:

Going to mark as resolved, feel free to reopen for futher convo!

in src/init.cpp:1428 in b4591fce33 outdated

1423@@ -1424,7 +1424,10 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1424     for (bool fLoaded = false; !fLoaded && !ShutdownRequested();) {
1425         node.mempool = std::make_unique<CTxMemPool>(node.fee_estimator.get(), mempool_check_ratio);
1426 
1427-        node.chainman = std::make_unique<ChainstateManager>(chainparams);
1428+        const ChainstateManager::Options chainman_opts{
1429+            chainparams,
1430+        };
1431+        node.chainman = std::make_unique<ChainstateManager>(chainman_opts);

JeremyRubin commented at 5:20 pm on May 17, 2022:

While this seems to be equivalent to the prior logic for this, looking at the lifetime math makes this seem like an abusable interface…

it should be more clear that chainparams needs some sort of ‘static lifetime’ from the PoV of the ChainstateManager.

dongcarl commented at 5:43 pm on May 17, 2022:

Hmmm, how about having ChainstateManger::m_chainparams not be a reference? That way ChainstateManager will own it wholly. Let me know if there are other easy solutions I’m missing.

JeremyRubin commented at 8:41 pm on May 17, 2022:

that’d be fine to me –i wouldn’t have said anything if it didn’t seem like the kind of thing leading to weird bugs when it’s not clear if the kernel or userspace owns the options passed to it.

dongcarl commented at 4:06 pm on May 18, 2022:

Addressed in 211674ce4fa845e872e2af9bfcdfc47c37729c52

in src/chainstatemanager_opts.h:20 in f554689988 outdated

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

JeremyRubin commented at 5:23 pm on May 17, 2022:

good candidate for a not_null #24423

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

dongcarl commented at 5:37 pm on May 17, 2022:

Totally agree!

in src/chainstatemanager_opts.h:1 in f554689988 outdated

0@@ -0,0 +1,23 @@
1+// Copyright (c) 2022 The Bitcoin Core developers

MarcoFalke commented at 6:00 am on May 18, 2022:

which folder should this file live in and why?

dongcarl commented at 2:55 pm on May 18, 2022:

I don’t really much mind where this file lives, so I’m happy to move the file wherever reviewers deem appropriate!

MarcoFalke commented at 3:00 pm on May 18, 2022:

I would have assumed it will be located in kernel, based on your comment

In the longer run, I think eventually (very far off when libbitcoincarl is very minimal) we could put everything that links into libbitcoincarl into the src/kernel/ folder.

#25065 (review)

edit: However, in commit cb64af9635a9553e335f2dc0b1cca20c6bbd0933 you created src/node/chainstate , so an alternative assumption of mine was that it would be located in node.

dongcarl commented at 3:40 pm on May 18, 2022:

Yeah true it would eventually be in kernel/, so I can just move it there now!

libbitcoincarl

💀

dongcarl commented at 4:06 pm on May 18, 2022:

Addressed in 43f343811cfb22739f5785a00f5a09d7b599fe04

dongcarl force-pushed on May 18, 2022

dongcarl commented at 4:06 pm on May 18, 2022: member

Pushed f554689988413479ea9bc561f7efd9d56a5c078f -> 211674ce4fa845e872e2af9bfcdfc47c37729c52

Addressed: #25064 (review)
Addressed: #25064 (review)

dongcarl force-pushed on May 18, 2022

DrahtBot added the label Needs rebase on May 20, 2022

Add ChainstateManagerOpts, using as ::Options

[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.

dbe45c34f8

Add ChainstateManager::m_adjusted_time_callback

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.

04c31c1295

validation: Have ChainstateManager own m_chainparams

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.

53494bc739

dongcarl force-pushed on May 20, 2022

dongcarl commented at 3:59 pm on May 20, 2022: member

Pushed af8e5468b589ea0e43e7d130033bfec23b616717 -> 53494bc7392591336e09d095f1fc38d63d566abf

Rebased over master to resolve merge conflict

DrahtBot removed the label Needs rebase on May 20, 2022

JeremyRubin commented at 6:34 pm on May 20, 2022: contributor

utack 53494bc

MarcoFalke merged this on May 20, 2022

MarcoFalke closed this on May 20, 2022

MarcoFalke moved this from the "Ready for Review PRs" to the "Done or Closed or Rethinking" column in a project

MarcoFalke deleted a comment on May 20, 2022

MarcoFalke locked this on May 20, 2022

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

Conflicts

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