m_block_index ownership of CBlockIndexs
#24050
Part of: #24303 Split off from: #22564
0Instead of having CBlockIndex's live on the heap, which requires manual
1memory management, have them be owned by m_block_index. This means that
2they will live and die with BlockManager.
The second commit demonstrates how this makes calls to Unload() to satisfy the address sanitizer unnecessary.
210
211 // Create new
212- CBlockIndex* pindexNew = new CBlockIndex();
213- mi = m_block_index.insert(std::make_pair(hash, pindexNew)).first;
214+ CBlockIndex new_index{};
215+ mi = m_block_index.insert(std::make_pair(hash, std::move(new_index))).first;
0 mi = m_block_index.insert(std::make_pair(hash, CBlockIndex{})).first;
nit: Could remove the symbol to avoid accidentally using it after the move?
I think you could replace both that and the previous block with try_emplace ?
0 // Return existing or create new
1 auto [mi, inserted] = m_block_index.try_emplace(hash);
2 CBlockIndex* pindex = &(*mi).second;
3 if (inserted) {
4 pindex->phashBlock = &((*mi).first);
5 }
6 return pindex;
4180@@ -4181,8 +4181,8 @@ void CChainState::CheckBlockIndex()
4181
4182 // Build forward-pointing map of the entire block tree.
4183 std::multimap<CBlockIndex*,CBlockIndex*> forward;
4184- for (const std::pair<const uint256, CBlockIndex*>& entry : m_blockman.m_block_index) {
4185- forward.insert(std::make_pair(entry.second->pprev, entry.second));
4186+ for (std::pair<const uint256, CBlockIndex>& entry : m_blockman.m_block_index) {
4187+ forward.insert(std::make_pair(entry.second.pprev, &entry.second));
nit: (assuming it compiles)
0 for (const auto& [_, block] : m_blockman.m_block_index) {
1 forward.emplace(block.pprev, &block));
2964@@ -2965,8 +2965,8 @@ bool CChainState::InvalidateBlock(BlockValidationState& state, CBlockIndex* pind
2965
2966 {
2967 LOCK(cs_main);
2968- for (const auto& entry : m_blockman.m_block_index) {
2969- CBlockIndex *candidate = entry.second;
2970+ for (auto& entry : m_blockman.m_block_index) {
2971+ CBlockIndex *candidate = &entry.second;
49@@ -49,7 +50,11 @@ extern bool fPruneMode;
50 /** Number of MiB of block files that we're trying to stay below. */
51 extern uint64_t nPruneTarget;
52
53-typedef std::unordered_map<uint256, CBlockIndex*, BlockHasher> BlockMap;
54+// Because validation code takes pointers to the map's CBlockIndex objects, if
55+// we ever switch to another associative container, we need to either use a
56+// container that has stable addressing (true of all std associative
57+// containers), or make the key a `std::unique_ptr<CBlockIndex>`
58+typedef std::unordered_map<uint256, CBlockIndex, BlockHasher> BlockMap;
0using BlockMap = std::unordered_map<uint256, CBlockIndex, BlockHasher>;
nit
ACK 8fb03ca47c01907b98461b8d538e2691d7ef65af (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.
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3ACK 8fb03ca47c01907b98461b8d538e2691d7ef65af ([`jamesob/ackr/24050.1.dongcarl.validation_move_cblockin`](https://github.com/jamesob/bitcoin/tree/ackr/24050.1.dongcarl.validation_move_cblockin))
4
5Good change! For what it's worth, my previous benchmarking may be a relevant consideration for reviewers: [#22564 (comment)](/bitcoin-bitcoin/22564/#issuecomment-930377301)
6
7Some of the intermediate commits could probably be squashed, but that doesn't bother me much either way.
8
9I've compiled each commit and run unittests locally.
10-----BEGIN PGP SIGNATURE-----
11
12iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmHnJrIACgkQepNdrbLE
13TwWLJhAAi3IfNFNZIaqTnTfMw8V9W8v2K2wNXT29tsj09JVTaJ5PGpQ9WoBbrkP+
14cTtTvmlJ71X9VvRERPL0oz8A4YF1/QOPj9NJ+ZVE3NLg3z6rTbn5PFCqBNUYUvp/
15a+qLshzpCsluQKs7B+70qpu0L589yYEcO6cKHVRVPLi8jrJxCc7/lEo2GdCjD2xJ
16nxj0Pu+O+erN7zhVZ4evqOr6j+XSufHn6xiqm7DdGXzyafQzpPB3h5FJxVPZbh5p
17PI5plMf5WcxPePd8ynE5v/wnKCZkaMhJm1cNC9D/MjmJUIPZyx8ngJoKPllgztHf
18UzKZ0n7MBPHwFdsMGraKq0VCotD/ZxFGbeNtXaI0BLZfjA2Xyf1vaFFfNRg5uZHe
19d45/Sa8uDQjVApxQtIbfRMGNFVms5pr8SrNuPvJ0EDIXtYXXuEvkVmX5lhkz39Sy
20GO4/IbqBiT7md+dk7J7h5M6j3C5LnyiFWJzqS418//kk24/KSF6/kwGzXp1ASweY
21H85Efa9xq8dr1KBiSw9wZ0U3RFpqOyz+/0ejzrBVxtMjd9x49RF4MRmakNPi/P4D
22+SVBS0e0gr/FV0CW501tc5gZYMC3UK+whGcmaCT0LRjjQlpFv0M1sZZbA6CGdFYQ
23zFR7x0aXcIZjByDECGMi9+T6c3Mi0yGa2ulc8uO7vo3DNLuTJOQ=
24=f/Ec
25-----END PGP SIGNATURE-----
0Tested on Linux-5.10.0-10-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 --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
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 -msha -msse4.1 -msse4.2 i
5
6Compiler version: Debian clang version 13.0.1-++20220112082946+b9a243d1cac2-1~exp1~20220112203027.57
58 // to avoid miners withholding blocks but broadcasting headers, to get a
59 // competitive advantage.
60- pindexNew->nSequenceId = 0;
61- BlockMap::iterator mi = m_block_index.insert(std::make_pair(hash, pindexNew)).first;
62+ new_index.nSequenceId = 0;
63+ BlockMap::iterator mi = m_block_index.insert(std::make_pair(hash, std::move(new_index))).first;
Not really a big deal, I suppose, but this is allocating two CBlockIndex objects – once on the stack for new_index, once as part of the map, copied from the one on the stack. Would have expected something more like:
0 BlockMap::iterator mi = m_block_index.try_emplace(hash, block).first;
1 CBlockIndex* pindexNew = &(*mi).second;
2 pindexNew->nSequenceId = 0;
(or auto [mi, inserted] = ...try_emplace(hash, block); if (!inserted) { return &mi->second; } to avoid the preceding call to find).
3088@@ -3089,17 +3089,17 @@ void CChainState::ResetBlockFailureFlags(CBlockIndex *pindex) {
3089 // Remove the invalidity flag from this block and all its descendants.
3090 BlockMap::iterator it = m_blockman.m_block_index.begin();
3091 while (it != m_blockman.m_block_index.end()) {
for loop if you’re modernising things. Ditto above.
3427@@ -3428,7 +3428,7 @@ bool ChainstateManager::AcceptBlockHeader(const CBlockHeader& block, BlockValida
3428 if (hash != chainparams.GetConsensus().hashGenesisBlock) {
3429 if (miSelf != m_blockman.m_block_index.end()) {
3430 // Block header is already known.
3431- CBlockIndex* pindex = miSelf->second;
3432+ CBlockIndex* pindex = &(miSelf->second);
(&miSelf)->second doesn’t seem that useful to me…
For reference, the stuff is still on the heap. This only changes the layout a bit.
Master:
This:
Pushed 8fb03ca47c01907b98461b8d538e2691d7ef65af -> c68516d662687699b399f8d12e22f2e985edae1a
Anything else this needs before it’s ready for merge?
4@@ -5,6 +5,7 @@
5 #ifndef BITCOIN_NODE_BLOCKSTORAGE_H
6 #define BITCOIN_NODE_BLOCKSTORAGE_H
7
8+#include <chain.h>
3099+ m_blockman.m_dirty_blockindex.insert(&it->second);
3100+ if (it->second.IsValid(BLOCK_VALID_TRANSACTIONS) && it->second.HaveTxsDownloaded() && setBlockIndexCandidates.value_comp()(m_chain.Tip(), &it->second)) {
3101+ setBlockIndexCandidates.insert(&it->second);
3102 }
3103- if (it->second == m_chainman.m_best_invalid) {
3104+ if (&it->second == m_chainman.m_best_invalid) {
first commit self-note: Luckily an almost identically looking diff wouldn’t compile:
0validation.cpp:3133:28: error: invalid operands to binary expression ('CBlockIndex' and 'CBlockIndex')
1 if (it->second == *m_chainman.m_best_invalid) {
2 ~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~
31 error generated.
361@@ -362,10 +362,10 @@ static int64_t AddTx(ChainstateManager& chainman, CWallet& wallet, uint32_t lock
362 CBlockIndex* block = nullptr;
363 if (blockTime > 0) {
364 LOCK(cs_main);
365- auto inserted = chainman.BlockIndex().emplace(GetRandHash(), new CBlockIndex);
366+ 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?
0auto inserted{chainman.BlockIndex().try_emplace(GetRandHash(), CBlockIndex{})};
195@@ -199,18 +196,13 @@ CBlockIndex* BlockManager::InsertBlockIndex(const uint256& hash)
196 return nullptr;
197 }
198
199- // Return existing
200- BlockMap::iterator mi = m_block_index.find(hash);
201- if (mi != m_block_index.end()) {
202- return (*mi).second;
203+ // Return existing or create new
204+ auto [mi, inserted] = m_block_index.try_emplace(hash);
0 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
215@@ -224,8 +216,8 @@ bool BlockManager::LoadBlockIndex(
216 // Calculate nChainWork
217 std::vector<std::pair<int, CBlockIndex*>> vSortedByHeight;
218 vSortedByHeight.reserve(m_block_index.size());
219- for (const std::pair<const uint256, CBlockIndex*>& item : m_block_index) {
220- CBlockIndex* pindex = item.second;
221+ for (auto& [_, block_index] : m_block_index) {
222+ CBlockIndex* pindex = &block_index;
223 vSortedByHeight.push_back(std::make_pair(pindex->nHeight, pindex));
nit in the for-loop commit: Any reason to keep this alias?
0 vSortedByHeight.emplace_back(block_index.nHeight, &block_index));
373@@ -386,8 +374,8 @@ bool BlockManager::LoadBlockIndexDB(ChainstateManager& chainman)
374 // Check presence of blk files
375 LogPrintf("Checking all blk files are present...\n");
376 std::set<int> setBlkDataFiles;
377- for (const std::pair<const uint256, CBlockIndex*>& item : m_block_index) {
378- CBlockIndex* pindex = item.second;
379+ for (const auto& [_, block_index] : m_block_index) {
380+ const CBlockIndex* pindex = &block_index;
left some style nits. Feel fee to ignore any or all. I am happy to re-ACK if you don’t.
review ACK c68516d662687699b399f8d12e22f2e985edae1a 🍏
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3review ACK c68516d662687699b399f8d12e22f2e985edae1a 🍏
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUiTPgv8CbaBOK51qXPuhdeM7/DZJAMYFCAPOYAsAHSD7mrO1PG/7wA5vUW2ffGY
8U2yM/rLCnBBCITrcrOygU61k72HPwgGTQ7jG0zV5+A339cAbnpJBm5+O8ky5/+NJ
9/88BlcGNOmmZwUnuRLOwWwILDvNKPb4SwwBYSEDCWVS3hWt8e26Kw08xwiemHKsG
10jyAIUZy8o789VNw5Wt12UQLbTZ/6p0Ox77le7egFttnC/Bpea0IbOdCTZzWOCiZQ
11PUGcl+YcXMZxEnV1n+3GWWWMQgOYRYLa4BGAPVeauQd3AWEIU1jwfGL3Sy0Mik24
123Ej/Qx9UCMMhv7DB96nBeaYWoY7txJhT6c2ZnmseggFuKdtxcvIpd/nWEsbdEH4S
133Tn0ZZHSFXhPA76ChmS8eHYid7NIxkXPff43bCAz8cERJ68ISm061gqi6RaD0Rxl
14sWOrxIzJpDg0caTkaHo8p2ZPVqm+vaPsoxlBJiYoAGKvQzXz/EjzOazgPbEqk3ZK
155MZUwBRK
16=Ofkd
17-----END PGP SIGNATURE-----
49@@ -49,7 +50,11 @@ extern bool fPruneMode;
50 /** Number of MiB of block files that we're trying to stay below. */
51 extern uint64_t nPruneTarget;
52
53-typedef std::unordered_map<uint256, CBlockIndex*, BlockHasher> BlockMap;
54+// Because validation code takes pointers to the map's CBlockIndex objects, if
55+// we ever switch to another associative container, we need to either use a
56+// container that has stable addressing (true of all std associative
57+// containers), or make the key a `std::unique_ptr<CBlockIndex>`
ACK c68516d662687699b399f8d12e22f2e985edae1a (jamesob/ackr/24050.2.dongcarl.validation_move_cblockin)
Still looks good. Re-reviewed the code, built new revision, ran unittests.
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3ACK c68516d662687699b399f8d12e22f2e985edae1a ([`jamesob/ackr/24050.2.dongcarl.validation_move_cblockin`](https://github.com/jamesob/bitcoin/tree/ackr/24050.2.dongcarl.validation_move_cblockin))
4
5Still looks good. Re-reviewed the code, built new revision, ran unittests.
6
7-----BEGIN PGP SIGNATURE-----
8
9iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmIEB80ACgkQepNdrbLE
10TwX3sQ/+LiR9iKtlAAsH0iGxa5z5pm8BLqTba/1x6IznqZJaC1iztDFRSO+6G6bP
11/q3paKXcBLgTBH8n4UQ/ymhmVPLrn4wTsPopDI9ygTOkXMPk3GUfVTqm64dPNaf6
12CKMYffWzLsXI3qWUAm6HU3weEwaXNoX1O61rWzsIgrJhrJOX+GhQ6ph1JxMImBjO
134QHcRIh+cMoLEv1fajatdLfDKR8JSW9HDfWcB2n3kLqiZ/jbdN8PhagmZaDkS4b1
14lWkTq+m5fpZA1qHQfUknWH+zamuPNorpBnahWmgs8G4bAxEKFeNJMq8a5KZBdy0G
15hn1VNMyshtXnxNS4JkoxS+7hv4NBeIaSspwjChA26B18CvIoeRw/2vEKdyEEIJVH
16A59cPIHL4hYe7ZJRxC756GzaReYGHS0Myz5FBJ4xEp6/lJbBIr8lrWY3oW7porrv
17l50sH4L8ryk2Rhn5qHFhKZ7V1vPSJPLppnzFYyJSzQQlSSqq8NFddJHUv1oz5N2M
18wHsRE0FpGlUzTlsn/iAyQdfML5TG1ZIHZvyBHSvbDFeLrvfawzeMRoUf/aSsE+T7
19q1YNQf2cNHdxYUIFf+NoeYDZRfH81a71krQy2SuKI/WFDgn57EexQB2Qi8lhBSiG
20r3jFuvFvWSihTJQ2Hqt5xbRfLRTgNbjJgNpPjRd+g/yI2rztmEI=
21=GA3x
22-----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 --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
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 -msha -msse4.1 -msse4.2 i
5
6Compiler version: Debian clang version 13.0.1-++20211215043002+d904698b53e4-1~exp1~20211215043040.32
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!
Pushed c68516d662687699b399f8d12e22f2e985edae1a -> 6c23c415613d8b847e6f6a2f872be893da9f4384
const version of BlockManager::LookupBlockIndex to account for changes in fbab43f169924c681ef085639b3e4de6c74a4958CBlockIndex forward declarationNote that I realized parts of libbitcoinkernel is made easier by this work and its followup, so will be adding it to the project.
re-ACK 6c23c415613d8b847e6f6a2f872be893da9f4384 🎨
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3re-ACK 6c23c415613d8b847e6f6a2f872be893da9f4384 🎨
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUjQHQwArZhANo4eHc9fHY/ogxBargWcUs82Jt+5QFFFQFB33PIjOCYNE1fpaPuf
8y7SVbEdp0SvsMs2qOn86LptVA2UJQYaaYB+36/AlPo0f1tXRHmNZbFSnQicRGnYG
98Mf4S/NUS81e9Mg85rqXjTaCxeAkVtALazna7+wSE1xHcPx9VMy20OcBRye+BzkK
10UmzVHIrtjA6/1lVWa7QBWo8S2se6l3GPIT7TARSuH1+tGGH3LRxJ47S5qYbTJciB
11Ec1YBWiepcnq1QywMC9GXYIrYZmKPikmRhrpAIKyL3yQh76eksIFFg1qFLq6yD1d
12QhKWt5PA4peyGYLhQYh6UUV1YDh5cXM1PEOur17tG6BPw3oX5+j8OsdVIfYur3V5
13NM29D2T72nxaBYRrnXH+tIeY6Qz4PN+jKaZSqI3Qa8iaPounJnoryr8FN5SfKIS6
14NYCmRCN/EYFC5TocfoWaEue6F69q4Y9AzWE20ko0UF2zOBtFgkv1L/+NDusi+TTc
15B8Ozf9u4
16=iXAq
17-----END PGP SIGNATURE-----