validation: Give `m_block_index` ownership of `CBlockIndex`s #24050

dongcarl · 2022-01-12T19:45:52Z

Part of: #24303
Split off from: #22564

Instead of having CBlockIndex's live on the heap, which requires manual
memory management, have them be owned by m_block_index. This means that
they will live and die with BlockManager.

The second commit demonstrates how this makes calls to Unload() to satisfy the address sanitizer unnecessary.

maflcko

Left some nits. Feel free to ignore any or all of them.

src/node/blockstorage.cpp

src/validation.cpp

src/node/blockstorage.h

dongcarl · 2022-01-13T17:49:53Z

Okay I'm still reserving my right to ignore future nits, but these one struck a soft spot in my heart so I incorporated them 😁

jamesob

ACK 8fb03ca (jamesob/ackr/24050.1.dongcarl.validation_move_cblockin)

Good change! For what it's worth, my previous benchmarking may be a relevant consideration for reviewers: #22564 (comment)

Some of the intermediate commits could probably be squashed, but that doesn't bother me much either way.

I've compiled each commit and run unittests locally.

Show signature data

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

ACK 8fb03ca47c01907b98461b8d538e2691d7ef65af ([`jamesob/ackr/24050.1.dongcarl.validation_move_cblockin`](https://github.com/jamesob/bitcoin/tree/ackr/24050.1.dongcarl.validation_move_cblockin))

Good change! For what it's worth, my previous benchmarking may be a relevant consideration for reviewers: https://github.com/bitcoin/bitcoin/pull/22564#issuecomment-930377301

Some of the intermediate commits could probably be squashed, but that doesn't bother me much either way.

I've compiled each commit and run unittests locally.
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmHnJrIACgkQepNdrbLE
TwWLJhAAi3IfNFNZIaqTnTfMw8V9W8v2K2wNXT29tsj09JVTaJ5PGpQ9WoBbrkP+
cTtTvmlJ71X9VvRERPL0oz8A4YF1/QOPj9NJ+ZVE3NLg3z6rTbn5PFCqBNUYUvp/
a+qLshzpCsluQKs7B+70qpu0L589yYEcO6cKHVRVPLi8jrJxCc7/lEo2GdCjD2xJ
nxj0Pu+O+erN7zhVZ4evqOr6j+XSufHn6xiqm7DdGXzyafQzpPB3h5FJxVPZbh5p
PI5plMf5WcxPePd8ynE5v/wnKCZkaMhJm1cNC9D/MjmJUIPZyx8ngJoKPllgztHf
UzKZ0n7MBPHwFdsMGraKq0VCotD/ZxFGbeNtXaI0BLZfjA2Xyf1vaFFfNRg5uZHe
d45/Sa8uDQjVApxQtIbfRMGNFVms5pr8SrNuPvJ0EDIXtYXXuEvkVmX5lhkz39Sy
GO4/IbqBiT7md+dk7J7h5M6j3C5LnyiFWJzqS418//kk24/KSF6/kwGzXp1ASweY
H85Efa9xq8dr1KBiSw9wZ0U3RFpqOyz+/0ejzrBVxtMjd9x49RF4MRmakNPi/P4D
+SVBS0e0gr/FV0CW501tc5gZYMC3UK+whGcmaCT0LRjjQlpFv0M1sZZbA6CGdFYQ
zFR7x0aXcIZjByDECGMi9+T6c3Mi0yGa2ulc8uO7vo3DNLuTJOQ=
=f/Ec
-----END PGP SIGNATURE-----

Show platform data

Tested on Linux-5.10.0-10-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 --disable-shared --with-pic --enable-benchmark=no --enable-module-recovery --enable-module-schnorrsig --enable-experimental PYTHONPATH= --disable-shared --with-pic --enable-benchmark=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 -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 -msha -msse4.1 -msse4.2  i

Compiler version: Debian clang version 13.0.1-++20220112082946+b9a243d1cac2-1~exp1~20220112203027.57

ajtowns

ACK 8fb03ca ; code review only. some more nits you may want to consider

src/node/blockstorage.cpp

src/validation.cpp

ajtowns · 2022-01-19T01:13:32Z

src/validation.cpp

@@ -3428,7 +3428,7 @@ bool ChainstateManager::AcceptBlockHeader(const CBlockHeader& block, BlockValida
    if (hash != chainparams.GetConsensus().hashGenesisBlock) {
        if (miSelf != m_blockman.m_block_index.end()) {
            // Block header is already known.
-            CBlockIndex* pindex = miSelf->second;
+            CBlockIndex* pindex = &(miSelf->second);


Adding brackets to distinguish from (&miSelf)->second doesn't seem that useful to me...

maflcko · 2022-01-19T17:32:13Z

For reference, the stuff is still on the heap. This only changes the layout a bit.

Master:

This:

dongcarl · 2022-01-21T21:37:34Z

Pushed 8fb03ca -> c68516d

Some more modernizations

Anything else this needs before it's ready for merge?

maflcko

left some style nits. Feel fee to ignore any or all. I am happy to re-ACK if you don't.

review ACK c68516d 🍏

Show signature

Signature:

src/node/blockstorage.h

maflcko · 2022-01-26T16:55:49Z

src/validation.cpp

            }
-            if (it->second == m_chainman.m_best_invalid) {
+            if (&it->second == m_chainman.m_best_invalid) {


first commit self-note: Luckily an almost identically looking diff wouldn't compile:

validation.cpp:3133:28: error: invalid operands to binary expression ('CBlockIndex' and 'CBlockIndex') if (it->second == *m_chainman.m_best_invalid) { ~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 error generated.

maflcko · 2022-01-26T17:08:17Z

src/wallet/test/wallet_tests.cpp

@@ -362,10 +362,10 @@ static int64_t AddTx(ChainstateManager& chainman, CWallet& wallet, uint32_t lock
    CBlockIndex* block = nullptr;
    if (blockTime > 0) {
        LOCK(cs_main);
-        auto inserted = chainman.BlockIndex().emplace(GetRandHash(), new CBlockIndex);
+        auto inserted = chainman.BlockIndex().emplace(std::piecewise_construct, std::make_tuple(GetRandHash()), std::make_tuple());


first commit: Is there any reason/advantage to change this to std::piecewise_construct and not try_emplace, which automatically enables that. Or would that not work because the CBlockIndex{} rvalue can't be elided?

auto inserted{chainman.BlockIndex().try_emplace(GetRandHash(), CBlockIndex{})};

maflcko · 2022-01-26T17:19:13Z

src/node/blockstorage.cpp

+    // Return existing or create new
+    auto [mi, inserted] = m_block_index.try_emplace(hash);


Suggested change

// Return existing or create new

auto [mi, inserted] = m_block_index.try_emplace(hash);

const auto [mi, inserted]{m_block_index.try_emplace(hash)};

In the third commit: I think try_emplace is self-explanatory and the comment can be removed

maflcko · 2022-01-26T17:29:59Z

src/node/blockstorage.cpp

+        CBlockIndex* pindex = &block_index;
        vSortedByHeight.push_back(std::make_pair(pindex->nHeight, pindex));


nit in the for-loop commit: Any reason to keep this alias?

Suggested change

CBlockIndex* pindex = &block_index;

vSortedByHeight.push_back(std::make_pair(pindex->nHeight, pindex));

vSortedByHeight.emplace_back(block_index.nHeight, &block_index));

maflcko · 2022-01-26T17:33:05Z

src/node/blockstorage.cpp

-    for (const std::pair<const uint256, CBlockIndex*>& item : m_block_index) {
-        CBlockIndex* pindex = item.second;
+    for (const auto& [_, block_index] : m_block_index) {
+        const CBlockIndex* pindex = &block_index;


dongcarl · 2022-02-04T19:17:37Z

Sorry, not going to make any more style/modernization-related changes, they can be followups.

jamesob

ACK c68516d (jamesob/ackr/24050.2.dongcarl.validation_move_cblockin)

Still looks good. Re-reviewed the code, built new revision, ran unittests.

Show signature data

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

ACK c68516d662687699b399f8d12e22f2e985edae1a ([`jamesob/ackr/24050.2.dongcarl.validation_move_cblockin`](https://github.com/jamesob/bitcoin/tree/ackr/24050.2.dongcarl.validation_move_cblockin))

Still looks good. Re-reviewed the code, built new revision, ran unittests.

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

iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmIEB80ACgkQepNdrbLE
TwX3sQ/+LiR9iKtlAAsH0iGxa5z5pm8BLqTba/1x6IznqZJaC1iztDFRSO+6G6bP
/q3paKXcBLgTBH8n4UQ/ymhmVPLrn4wTsPopDI9ygTOkXMPk3GUfVTqm64dPNaf6
CKMYffWzLsXI3qWUAm6HU3weEwaXNoX1O61rWzsIgrJhrJOX+GhQ6ph1JxMImBjO
4QHcRIh+cMoLEv1fajatdLfDKR8JSW9HDfWcB2n3kLqiZ/jbdN8PhagmZaDkS4b1
lWkTq+m5fpZA1qHQfUknWH+zamuPNorpBnahWmgs8G4bAxEKFeNJMq8a5KZBdy0G
hn1VNMyshtXnxNS4JkoxS+7hv4NBeIaSspwjChA26B18CvIoeRw/2vEKdyEEIJVH
A59cPIHL4hYe7ZJRxC756GzaReYGHS0Myz5FBJ4xEp6/lJbBIr8lrWY3oW7porrv
l50sH4L8ryk2Rhn5qHFhKZ7V1vPSJPLppnzFYyJSzQQlSSqq8NFddJHUv1oz5N2M
wHsRE0FpGlUzTlsn/iAyQdfML5TG1ZIHZvyBHSvbDFeLrvfawzeMRoUf/aSsE+T7
q1YNQf2cNHdxYUIFf+NoeYDZRfH81a71krQy2SuKI/WFDgn57EexQB2Qi8lhBSiG
r3jFuvFvWSihTJQ2Hqt5xbRfLRTgNbjJgNpPjRd+g/yI2rztmEI=
=GA3x
-----END PGP SIGNATURE-----

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 --enable-experimental --disable-openssl-tests --disable-shared --with-pic --enable-benchmark=no --enable-module-recovery --enable-module-schnorrsig --enable-experimental --disable-openssl-tests --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 -msha -msse4.1 -msse4.2  i

Compiler version: Debian clang version 13.0.1-++20211215043002+d904698b53e4-1~exp1~20211215043040.32

jamesob · 2022-02-09T18:25:09Z

src/node/blockstorage.h

+// Because validation code takes pointers to the map's CBlockIndex objects, if
+// we ever switch to another associative container, we need to either use a
+// container that has stable addressing (true of all std associative
+// containers), or make the key a `std::unique_ptr<CBlockIndex>`


maflcko · 2022-02-14T09:06:03Z

Assigned 24.0, as this missed the 0.23 feature freeze

Instead of having CBlockIndex's live on the heap, which requires manual memory management, have them be owned by m_block_index. This means that they will live and die with BlockManager. A change to BlockManager::LookupBlockIndex: - Previously, it was a const member function returning a non-const CBlockIndex* - Now, there's are const and non-const versions of BlockManager::LookupBlockIndex returning a CBlockIndex with the same const-ness as the member function: (e.g. const CBlockIndex* LookupBlockIndex(...) const) See next commit for some weirdness that this eliminates. The range based for-loops are modernize (using auto + destructuring) in a future commit.

These manual calls to Unload() are no longer necessary because CBlockIndex's no longer live in the heap as of the previous commit.

Credit to ajtowns for this suggestion, thanks!

dongcarl · 2022-02-22T18:07:47Z

Pushed c68516d -> 6c23c41

Added const version of BlockManager::LookupBlockIndex to account for changes in fbab43f
Removed a now-unnecessary CBlockIndex forward declaration

Note that I realized parts of libbitcoinkernel is made easier by this work and its followup, so will be adding it to the project.

ajtowns · 2022-03-02T05:47:11Z

ACK 6c23c41

Commenting out the non-const version of LookupBlockIndex and adding const annotations at call sites everywhere that's feasible has a lot of hits. Might be worth a followup post-merge.

maflcko

re-ACK 6c23c41 🎨

Show signature

Signature:

…ockIn… …dex`s

Summary: These manual calls to Unload() are no longer necessary because CBlockIndex's no longer live in the heap as of the previous commit. This is a partial backport of [[bitcoin/bitcoin#24050 | core#24050]] bitcoin/bitcoin@531dce0 Depends on D12991 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D12992

Summary: Credit to ajtowns for this suggestion, thanks! This is a partial backport of [[bitcoin/bitcoin#24050 | core#24050]] bitcoin/bitcoin@dd79dad bitcoin/bitcoin@6c23c41 Depends on D12992 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D12993

Summary: This concludes backport of [[bitcoin/bitcoin#24050 | core#24050]] bitcoin/bitcoin@c05cf7a Depends on D12993 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D12994

DrahtBot added Block storage RPC/REST/ZMQ Validation Wallet labels Jan 12, 2022

maflcko removed Wallet RPC/REST/ZMQ labels Jan 12, 2022

maflcko reviewed Jan 12, 2022

View reviewed changes

src/node/blockstorage.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

src/node/blockstorage.h Outdated Show resolved Hide resolved

dongcarl force-pushed the 2022-01-kirby-p2 branch from 9823f38 to 8fb03ca Compare January 13, 2022 17:47

jamesob approved these changes Jan 18, 2022

View reviewed changes

ajtowns approved these changes Jan 19, 2022

View reviewed changes

dongcarl changed the title ~~validation: Move CBlockIndexs from heap to m_block_index~~ validation: Give m_block_index ownership of CBlockIndexs Jan 19, 2022

dongcarl force-pushed the 2022-01-kirby-p2 branch from 8fb03ca to c68516d Compare January 19, 2022 19:24

maflcko approved these changes Jan 26, 2022

View reviewed changes

jamesob approved these changes Feb 9, 2022

View reviewed changes

maflcko added this to the 24.0 milestone Feb 14, 2022

dongcarl added 6 commits February 22, 2022 11:52

tests: Remove now-unnecessary manual Unload's

531dce0

These manual calls to Unload() are no longer necessary because CBlockIndex's no longer live in the heap as of the previous commit.

refactor: Rewrite InsertBlockIndex with try_emplace

dd79dad

Credit to ajtowns for this suggestion, thanks!

style-only: Use using instead of typedef for BlockMap

c2a1655

style: Modernize range-based loops over m_block_index

c05cf7a

refactor: Rewrite AddToBlockIndex with try_emplace

6c23c41

dongcarl force-pushed the 2022-01-kirby-p2 branch from c68516d to 6c23c41 Compare February 22, 2022 18:05

This was referenced Feb 22, 2022

The libbitcoinkernel Project #24303

Closed

refactor: Move mutable globals cleared in ::UnloadBlockIndex to BlockManager #22564

Merged

maflcko approved these changes Mar 7, 2022

View reviewed changes

maflcko merged commit 5e49b2a into bitcoin:master Mar 7, 2022

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 7, 2022

Merge bitcoin#24050: validation: Give m_block_index ownership of `CBl…

7d64503

…ockIn… …dex`s

maflcko mentioned this pull request Mar 21, 2022

Switch BlockMap to use an unordered_set under the hood #19677

Closed

maflcko mentioned this pull request Apr 4, 2022

The BlockIndex/BlockMap should not live in memory *all the time* #24760

Closed

bitcoin locked and limited conversation to collaborators Mar 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

validation: Give `m_block_index` ownership of `CBlockIndex`s #24050

validation: Give `m_block_index` ownership of `CBlockIndex`s #24050

dongcarl commented Jan 12, 2022 •

edited

Loading

maflcko left a comment

dongcarl commented Jan 13, 2022

jamesob left a comment

ajtowns left a comment

ajtowns Jan 19, 2022

maflcko commented Jan 19, 2022

dongcarl commented Jan 21, 2022

maflcko left a comment

maflcko Jan 26, 2022

maflcko Jan 26, 2022

maflcko Jan 26, 2022

maflcko Jan 26, 2022

maflcko Jan 26, 2022

dongcarl commented Feb 4, 2022

jamesob left a comment

jamesob Feb 9, 2022

maflcko commented Feb 14, 2022

dongcarl commented Feb 22, 2022 •

edited

Loading

ajtowns commented Mar 2, 2022

maflcko left a comment

		// Return existing or create new
		auto [mi, inserted] = m_block_index.try_emplace(hash);

	// Return existing or create new
	auto [mi, inserted] = m_block_index.try_emplace(hash);
	const auto [mi, inserted]{m_block_index.try_emplace(hash)};

		CBlockIndex* pindex = &block_index;
		vSortedByHeight.push_back(std::make_pair(pindex->nHeight, pindex));

	CBlockIndex* pindex = &block_index;
	vSortedByHeight.push_back(std::make_pair(pindex->nHeight, pindex));
	vSortedByHeight.emplace_back(block_index.nHeight, &block_index));

validation: Give m_block_index ownership of CBlockIndexs #24050

validation: Give m_block_index ownership of CBlockIndexs #24050

Conversation

dongcarl commented Jan 12, 2022 • edited Loading

maflcko left a comment

Choose a reason for hiding this comment

dongcarl commented Jan 13, 2022

jamesob left a comment

Choose a reason for hiding this comment

ajtowns left a comment

Choose a reason for hiding this comment

ajtowns Jan 19, 2022

Choose a reason for hiding this comment

maflcko commented Jan 19, 2022

dongcarl commented Jan 21, 2022

maflcko left a comment

Choose a reason for hiding this comment

maflcko Jan 26, 2022

Choose a reason for hiding this comment

maflcko Jan 26, 2022

Choose a reason for hiding this comment

maflcko Jan 26, 2022

Choose a reason for hiding this comment

maflcko Jan 26, 2022

Choose a reason for hiding this comment

maflcko Jan 26, 2022

Choose a reason for hiding this comment

dongcarl commented Feb 4, 2022

jamesob left a comment

Choose a reason for hiding this comment

jamesob Feb 9, 2022

Choose a reason for hiding this comment

maflcko commented Feb 14, 2022

dongcarl commented Feb 22, 2022 • edited Loading

ajtowns commented Mar 2, 2022

maflcko left a comment

Choose a reason for hiding this comment

validation: Give `m_block_index` ownership of `CBlockIndex`s #24050

validation: Give `m_block_index` ownership of `CBlockIndex`s #24050

dongcarl commented Jan 12, 2022 •

edited

Loading

dongcarl commented Feb 22, 2022 •

edited

Loading