[Bundle 4.5/n] Followup fixups to bundle 4 #21525

dongcarl · 2021-03-24T19:39:12Z

Chronological history of this changeset:

Bundle 4 ([Bundle 4/n] Prune g_chainman usage in validation-adjacent modules #21270) got merged
Posthumous reviews were posted
These changes were prepended in bundle 5
More reviews were added in bundle 5
Someone suggested that we split the prepended changes up to another PR
This is that PR

In the future, I will just do posthumous review changes in another PR instead. I apologize for the confusion.

Addresses posthumous reviews on bundle 4:

From jnewbery:
- [Bundle 4/n] Prune g_chainman usage in validation-adjacent modules #21270 (comment)
  - I didn't fix this one, but I added a TODO comment so that we don't lost track of it
- [Bundle 4/n] Prune g_chainman usage in validation-adjacent modules #21270 (comment)
- [Bundle 4/n] Prune g_chainman usage in validation-adjacent modules #21270 (comment)
- [Bundle 4/n] Prune g_chainman usage in validation-adjacent modules #21270 (comment)
- [Bundle 4/n] Prune g_chainman usage in validation-adjacent modules #21270 (comment)
From MarcoFalke:

Addresses reviews on bundle 5:

Checking chainman existence before locking cs_main
- MarcoFalke
  - [Bundle 5/n] Prune g_chainman usage in RPC modules #21391 (comment)
  - [Bundle 5/n] Prune g_chainman usage in RPC modules #21391 (comment)
Appropriate locking, usage of chainman, and control flow in src/node/interfaces.cpp
Style/comment formatting changes
- jnewbery
  - [Bundle 5/n] Prune g_chainman usage in RPC modules #21391 (comment)
  - [Bundle 5/n] Prune g_chainman usage in RPC modules #21391 (comment)
Making LookupBlockIndex const
- jnewbery
  - [Bundle 5/n] Prune g_chainman usage in RPC modules #21391 (comment)

This reverts commit 2afcf24.

This reverts commit 46b7f29.

This reverts commit d0de61b.

dongcarl · 2021-03-24T20:36:14Z

@ryanofsky w/re #21391 (comment)
I've simplified the changes a bit and it may be a bit easier to review now, see commits:

DrahtBot · 2021-03-24T21:38:19Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

[p2p] Introduce node rebroadcast module #21061 ([p2p] Introduce node rebroadcast module by amitiuttarwar)
rpc: getblockfrompeer #20295 (rpc: getblockfrompeer by Sjors)

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.

Also: - Remove extraneous blank line

dongcarl · 2021-03-30T17:55:27Z

Pushed 3b75ec4 -> 693414d

Addressed [Bundle 5/n] Prune g_chainman usage in RPC modules #21391 (review)

ryanofsky

Code review ACK 693414d. I reviewed this previously as part of #21391. I am a fan of the increasingly complicated bundle numbering, and kind of hope there in the next round there is some way we can get bundles 5.333333 and 5.666667!

jamesob

ACK 693414d (jamesob/ackr/21525.1.dongcarl.bundle_4_5_n_followup_f)

Built locally and read through the code. Everything here is pretty non-controversial/rote. The first four commits just revert the chainstate parameterization of BlockAssembler methods in favor of making the chainstate a member and (presumably) saving on args.

Show platform data

Tested on Linux-4.19.0-16-amd64-x86_64-with-glibc2.28

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 --disable-shared --with-pic --enable-benchmark=no --with-bignum=no --enable-module-recovery --enable-module-schnorrsig --enable-experimental --no-create --no-recursion

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

Compiler version: Debian clang version 11.1.0-++20210204120158+1fdec59bffc1-1~exp1~20210203230823.159

Show signature data

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

ACK 693414d27181cf967f787a2ca72344e52c58c7f0 ([`jamesob/ackr/21525.1.dongcarl.bundle_4_5_n_followup_f`](https://github.com/jamesob/bitcoin/tree/ackr/21525.1.dongcarl.bundle_4_5_n_followup_f))

Built locally and read through the code. Everything here is pretty non-controversial/rote. The first four commits just revert the chainstate parameterization of BlockAssembler methods in favor of making the chainstate a member and (presumably) saving on args.

<details><summary>Show platform data</summary>
<p>

Tested on Linux-4.19.0-16-amd64-x86_64-with-glibc2.28

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 --disable-shared --with-pic --enable-benchmark=no --with-bignum=no --enable-module-recovery --enable-module-schnorrsig --enable-experimental --no-create --no-recursion

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

Compiler version: Debian clang version 11.1.0-++20210204120158+1fdec59bffc1-1~~exp1~~20210203230823.159


</p></details>

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmBk+mQACgkQepNdrbLE
TwXnUA/9GSL5MVKKVcWTuPlyLnP6c2MQDjXs7M3D4Bs4vBPAyYdHJlpyF9n4AK7A
ahAm+91r+enBanyHgMAiovGdMfHcsUgZ5rLTAsaeGuT6rOVNw3EI8J+X19qf7AHW
EJqwf7FSQxRi/AFRJ8QiTFDwkrl9TUOLnFdOqiCrMJuOdauplwMihJ5Ry0DkqBaE
TxVQTdE5Pt+KxYJ7bLycfmRmm1SJcBGi/S0CUibx475eIHv67D5u2KfU26L+n5mW
35tH1tIDvZEA2dvl4xxqtbeNdP2f3pG67u2qQiNSZ2leOlK7bi10F2v4cIeIas77
edUjJR9BMkQZFbyuKphXStNt5hYrlx2tOVe+F+YhaB1P+yArOZGaJGOfthK6KScA
9rpWYazHqobC+Bnh4H2VandHOsyPTf49aMcIJ64psP5i8gl+RCtTbmt+/N8kS+8Z
R3eApBtQTMuqeXaoHq5GW4FC1PXKIhiczdnvixBjzkXHaale5eMhiwNVku5D7hbu
QiC9Ck87znK1Tj776WtcMhx/rszMsSRxipdt+CvfnrqemkLqs18SbecxhFknUCQw
76LX/kjwQLmE5rJj3RGe4+46STWcPQ4VvO5C5xrx8n/FZtu/e9o4tvKal324Dj19
tHAtJMOG/VgyAdKWv4iVibR9oa5fNitp3OI0cKa/o0BgMXbyKIQ=
=IU4Y
-----END PGP SIGNATURE-----

maflcko · 2021-04-01T08:58:01Z

Left some suggestions for bundle 4.75

review ACK 693414d 🛐

Show signature and timestamp

Signature:

Timestamp of file with hash 011488e624ba1f6bc94c50a176ab89a375fd25e7786cc3f970aac8152f9c753b -

maflcko · 2021-04-01T08:44:30Z

src/test/util/setup_common.cpp

@@ -244,7 +244,7 @@ CBlock TestChain100Setup::CreateAndProcessBlock(const std::vector<CMutableTransa
 {
    const CChainParams& chainparams = Params();
    CTxMemPool empty_pool;
-    CBlock block = BlockAssembler(empty_pool, chainparams).CreateNewBlock(scriptPubKey)->block;
+    CBlock block = BlockAssembler(::ChainstateActive(), empty_pool, chainparams).CreateNewBlock(scriptPubKey)->block;


Any reason to use the global here and not m_node.chainman->ActiveChainstate()?

maflcko · 2021-04-01T08:44:36Z

src/test/util/mining.cpp

@@ -41,7 +41,7 @@ CTxIn MineBlock(const NodeContext& node, const CScript& coinbase_scriptPubKey)
 std::shared_ptr<CBlock> PrepareBlock(const NodeContext& node, const CScript& coinbase_scriptPubKey)
 {
    auto block = std::make_shared<CBlock>(
-        BlockAssembler{*Assert(node.mempool), Params()}
+        BlockAssembler{::ChainstateActive(), *Assert(node.mempool), Params()}


node.chainman->ActiveChainstate()

maflcko · 2021-04-01T08:44:43Z

src/test/miner_tests.cpp

@@ -44,7 +44,7 @@ BlockAssembler MinerTestingSetup::AssemblerForTest(const CChainParams& params)

    options.nBlockMaxWeight = MAX_BLOCK_WEIGHT;
    options.blockMinFeeRate = blockMinFeeRate;
-    return BlockAssembler(*m_node.mempool, params, options);
+    return BlockAssembler(::ChainstateActive(), *m_node.mempool, params, options);


m_node.chainman->ActiveChainstate()

maflcko · 2021-04-01T08:44:51Z

src/test/blockfilter_index_tests.cpp

@@ -62,7 +62,7 @@ CBlock BuildChainTestingSetup::CreateBlock(const CBlockIndex* prev,
    const CScript& scriptPubKey)
 {
    const CChainParams& chainparams = Params();
-    std::unique_ptr<CBlockTemplate> pblocktemplate = BlockAssembler(*m_node.mempool, chainparams).CreateNewBlock(scriptPubKey);
+    std::unique_ptr<CBlockTemplate> pblocktemplate = BlockAssembler(::ChainstateActive(), *m_node.mempool, chainparams).CreateNewBlock(scriptPubKey);


m_node.chainman->ActiveChainstate()

maflcko · 2021-04-01T08:45:59Z

src/rpc/mining.cpp

@@ -748,7 +748,7 @@ static RPCHelpMan getblocktemplate()

        // Create new block
        CScript scriptDummy = CScript() << OP_TRUE;
-        pblocktemplate = BlockAssembler(mempool, Params()).CreateNewBlock(scriptDummy);
+        pblocktemplate = BlockAssembler(::ChainstateActive(), mempool, Params()).CreateNewBlock(scriptDummy);


Any reason to take mempool from the node and the chainstate from the global?

maflcko · 2021-04-01T08:47:17Z

src/rpc/mining.cpp

@@ -358,7 +358,7 @@ static RPCHelpMan generateblock()
        LOCK(cs_main);

        CTxMemPool empty_mempool;
-        std::unique_ptr<CBlockTemplate> blocktemplate(BlockAssembler(empty_mempool, chainparams).CreateNewBlock(coinbase_script));
+        std::unique_ptr<CBlockTemplate> blocktemplate(BlockAssembler(::ChainstateActive(), empty_mempool, chainparams).CreateNewBlock(coinbase_script));


maflcko · 2021-04-01T08:49:53Z

src/net_processing.cpp

+     * @param[in]   max_height_diff The maximum number of items permitted to request, as specified in BIP 157
+     * @param[out]  stop_index      The CBlockIndex for the stop_hash block, if the request can be serviced.
+     * @param[out]  filter_index    The filter index, if the request can be serviced.
+     * @return                      True if the request can be serviced.


Commit body could mention that this can be reviewed with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space?

maflcko · 2021-04-01T09:04:58Z

src/miner.h

@@ -201,6 +202,7 @@ class BlockAssembler
 void IncrementExtraNonce(CBlock* pblock, const CBlockIndex* pindexPrev, unsigned int& nExtraNonce);
 int64_t UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParams, const CBlockIndex* pindexPrev);

+// TODO just accept a CBlockIndex*


I don't understand this TODO. CBlockIndex requires the block to be in the blockchain (block tree). However the coinbase commitment is regenerated before the block is submitted to the tree.

(Reply #21391 (comment))

586190f rpc/rest: Take and reuse local Chain/ChainState obj (Carl Dong) bc3bd36 rpc: style: Improve BuriedForkDescPushBack signature (Carl Dong) f999139 rpc: Remove unnecessary casting of block height (Carl Dong) 6a3d192 rpc: Tidy up local references (see commit message) (Carl Dong) 038854f rest/rpc: Remove now-unused old Ensure functions (Carl Dong) 6fb65b4 scripted-diff: rest/rpc: Use renamed EnsureAny*() (Carl Dong) 1570c7e rpc: Add renamed EnsureAny*() functions (Carl Dong) 306b1cd rpc: Add alt Ensure* functions acepting NodeContext (Carl Dong) d7824ac rest: Use existing NodeContext (Carl Dong) 3f08934 rest: Pass in NodeContext to rest_block (Carl Dong) 7be0671 rpc/rawtx: Use existing NodeContext (Carl Dong) 60dc05a rpc/mining: Use existing NodeContext (Carl Dong) d485e81 rpc/blockchain: Use existing NodeContext (Carl Dong) d0abf0b rpc/*,rest: Add review-only assertion to EnsureChainman (Carl Dong) cced0f4 miner: Pass in previous CBlockIndex to RegenerateCommitments (Carl Dong) Pull request description: Overall PR: #20158 (tree-wide: De-globalize ChainstateManager) Based on: - [x] #21270 | [Bundle 4/n] Prune g_chainman usage in validation-adjacent modules - [x] #21525 | [Bundle 4.5/n] Followup fixups to bundle 4 Note to reviewers: 1. This bundle may _apparently_ introduce usage of `g_chainman` or `::Chain(state|)Active()` globals, but these are resolved later on in the overall PR. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits) 2. There may be seemingly obvious local references to `ChainstateManager` or other validation objects which are not being used in callers of the current function in question, this is done intentionally to **_keep each commit centered around one function/method_** to ease review and to make the overall change systematic. We don't assume anything about our callers. Rest assured that once we are considering that particular caller in later commits, we will use the obvious local references. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits) 3. When changing a function/method that has many callers (e.g. `LookupBlockIndex` with 55 callers), it is sometimes easier (and less error-prone) to use a scripted-diff. When doing so, there will be 3 commits in sequence so that every commit compiles like so: 1. Add `new_function`, make `old_function` a wrapper of `new_function`, divert all calls to `old_function` to `new_function` **in the local module only** 2. Scripted-diff to divert all calls to `old_function` to `new_function` **in the rest of the codebase** 3. Remove `old_function` ACKs for top commit: ryanofsky: Code review ACK 586190f. Since last review, no changes to existing commits, just some simple new commits added: three new commits renaming std::any Ensure functions (scripted diff commit and manual pre/post commits), and one new commit factoring out a repeated `ActiveChain()` call made in a loop. Thanks for the updates! jnewbery: utACK 586190f MarcoFalke: review ACK 586190f 🍯 Tree-SHA512: 64b677fb50141805b55c3f1afe68fcd298f9a071a359bdcd63256d52e334f83e462f31fb3ebee9b630da8f1d912a03a128cfc38179e7aaec29a055744a98478c

Summary: This is a backport of [[bitcoin/bitcoin#21270 | core#21270]] [6/12] and [[bitcoin/bitcoin#21525 | core#21525]][5/10] bitcoin/bitcoin@106bcd4 bitcoin/bitcoin@07156eb Depends on D11420 Test Plan: `ninja all check-all ` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11422

Summary: This is a backport of [[bitcoin/bitcoin#21270 | core#21270]] [7/12] and [[bitcoin/bitcoin#21525 | core#21525]] [7/10] bitcoin/bitcoin@4cde4a7 bitcoin/bitcoin@88aead2 Depends on D11422 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11423

Summary: This is a backport of [[bitcoin/bitcoin#21270 | core#21270]] [8/12] and [[bitcoin/bitcoin#21525 | core#21525]] [9/10] bitcoin/bitcoin@8a1d580 bitcoin/bitcoin@98c4e25 Depends on D11423 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11424

Summary: This is a backport of [[bitcoin/bitcoin#21270 | core#21270]] [9/12] and [[bitcoin/bitcoin#21525 | core#21525]] [10/10] bitcoin/bitcoin@91c5b68 bitcoin/bitcoin@693414d Depends on D11424 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D11425

Summary: This concludes backport of [[bitcoin/bitcoin#21525 | core#21525]] bitcoin/bitcoin@7e8b5ee Test Plan: `ninja` Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D11507

Summary: This is a backport of [[bitcoin/bitcoin#21525 | core#21525]] bitcoin/bitcoin@1dd8ed7 Test Plan: NA. Only comments moved and blank lines deleted Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D11506

dongcarl added 5 commits March 24, 2021 15:40

Revert "miner: Remove old CreateNewBlock w/o chainstate param"

0c1b2bc

This reverts commit 2afcf24.

Revert "scripted-diff: Invoke CreateNewBlock with chainstate"

eede064

This reverts commit 46b7f29.

Revert "miner: Pass in chainstate to BlockAssembler::CreateNewBlock"

e62067e

This reverts commit d0de61b.

miner: Add chainstate member to BlockAssembler

7b8e976

node/coinstats: Replace #include with fwd-declaration

07156eb

dongcarl force-pushed the 2021-03-kernel-bundle-4.5 branch from 4073b8b to 3b75ec4 Compare March 24, 2021 19:57

DrahtBot added Mining P2P RPC/REST/ZMQ Validation labels Mar 24, 2021

dongcarl mentioned this pull request Mar 24, 2021

[Bundle 5/n] Prune g_chainman usage in RPC modules #21391

Merged

2 tasks

This was referenced Mar 25, 2021

[p2p] Introduce node rebroadcast module #21061

Closed

rpc: getblockfrompeer #20295

Merged

dongcarl added 5 commits March 30, 2021 13:52

net_processing: Move comments to declarations

1dd8ed7

Also: - Remove extraneous blank line

node: Avoid potential UB by asserting assumptions

88aead2

validation: Make BlockManager::LookupBlockIndex const

7e8b5ee

node/ifaces: NodeImpl: Use an accessor for ChainMan

98c4e25

node/ifaces: ChainImpl: Use an accessor for ChainMan

693414d

dongcarl force-pushed the 2021-03-kernel-bundle-4.5 branch from 3b75ec4 to 693414d Compare March 30, 2021 17:52

ryanofsky approved these changes Mar 30, 2021

View reviewed changes

jamesob approved these changes Mar 31, 2021

View reviewed changes

maflcko added Refactoring and removed Mining P2P RPC/REST/ZMQ labels Apr 1, 2021

maflcko approved these changes Apr 1, 2021

View reviewed changes

maflcko merged commit 80a699f into bitcoin:master Apr 1, 2021

maflcko reviewed Apr 1, 2021

View reviewed changes

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 1, 2021

Merge bitcoin#21525: [Bundle 4.5/n] Followup fixups to bundle 4

72fff53

ryanofsky mentioned this pull request Apr 12, 2021

Wallet passive startup #15719

Closed

ryanofsky mentioned this pull request Jul 28, 2022

refactor: make all NodeImpl/ChainImpl/ExternalSignerImpl members public, rm temporaries, simplify #25651

Merged

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Bundle 4.5/n] Followup fixups to bundle 4 #21525

[Bundle 4.5/n] Followup fixups to bundle 4 #21525

Uh oh!

dongcarl commented Mar 24, 2021 •

edited

Loading

Uh oh!

dongcarl commented Mar 24, 2021

Uh oh!

DrahtBot commented Mar 24, 2021

Uh oh!

dongcarl commented Mar 30, 2021

Uh oh!

ryanofsky left a comment

Uh oh!

jamesob left a comment

Uh oh!

maflcko commented Apr 1, 2021

Uh oh!

maflcko Apr 1, 2021

Uh oh!

maflcko Apr 1, 2021

Uh oh!

maflcko Apr 1, 2021

Uh oh!

maflcko Apr 1, 2021

Uh oh!

maflcko Apr 1, 2021

Uh oh!

maflcko Apr 1, 2021

Uh oh!

maflcko Apr 1, 2021

Uh oh!

maflcko Apr 1, 2021

Uh oh!

maflcko Apr 3, 2021

Uh oh!

Uh oh!

[Bundle 4.5/n] Followup fixups to bundle 4 #21525

[Bundle 4.5/n] Followup fixups to bundle 4 #21525

Uh oh!

Conversation

dongcarl commented Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dongcarl commented Mar 24, 2021

Uh oh!

DrahtBot commented Mar 24, 2021

Conflicts

Uh oh!

dongcarl commented Mar 30, 2021

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

jamesob left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Apr 1, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dongcarl commented Mar 24, 2021 •

edited

Loading