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.
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;
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 ?
// Return existing or create new
auto [mi, inserted] = m_block_index.try_emplace(hash);
CBlockIndex* pindex = &(*mi).second;
if (inserted) {
pindex->phashBlock = &((*mi).first);
}
return pindex;
Much nicer! Committed!
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)
for (const auto& [_, block] : m_blockman.m_block_index) {
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;
nit: clang-format touched code
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;
using BlockMap = std::unordered_map<uint256, CBlockIndex, BlockHasher>;
nit
Left some nits. Feel free to ignore any or all of them.
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 😁
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.
<details><summary>Show signature data</summary> <p>
-----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: [#22564 (comment)](/bitcoin-bitcoin/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-----
</p></details>
<details><summary>Show platform data</summary> <p>
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
</p></details>
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:
BlockMap::iterator mi = m_block_index.try_emplace(hash, block).first;
CBlockIndex* pindexNew = &(*mi).second;
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()) {
Could be a 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);
Adding brackets to distinguish from (&miSelf)->second doesn't seem that useful to me...
ACK 8fb03ca47c01907b98461b8d538e2691d7ef65af ; code review only. some more nits you may want to consider
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>
you can remove CBlockIndex forward decl when adding this in the first commit?
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:
validation.cpp:3133:28: error: invalid operands to binary expression ('CBlockIndex' and 'CBlockIndex')
if (it->second == *m_chainman.m_best_invalid) {
~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~
1 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?
auto 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);
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?
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;
same
left some style nits. Feel fee to ignore any or all. I am happy to re-ACK if you don't.
review ACK c68516d662687699b399f8d12e22f2e985edae1a 🍏
<details><summary>Show signature</summary>
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
review ACK c68516d662687699b399f8d12e22f2e985edae1a 🍏
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUiTPgv8CbaBOK51qXPuhdeM7/DZJAMYFCAPOYAsAHSD7mrO1PG/7wA5vUW2ffGY
U2yM/rLCnBBCITrcrOygU61k72HPwgGTQ7jG0zV5+A339cAbnpJBm5+O8ky5/+NJ
/88BlcGNOmmZwUnuRLOwWwILDvNKPb4SwwBYSEDCWVS3hWt8e26Kw08xwiemHKsG
jyAIUZy8o789VNw5Wt12UQLbTZ/6p0Ox77le7egFttnC/Bpea0IbOdCTZzWOCiZQ
PUGcl+YcXMZxEnV1n+3GWWWMQgOYRYLa4BGAPVeauQd3AWEIU1jwfGL3Sy0Mik24
3Ej/Qx9UCMMhv7DB96nBeaYWoY7txJhT6c2ZnmseggFuKdtxcvIpd/nWEsbdEH4S
3Tn0ZZHSFXhPA76ChmS8eHYid7NIxkXPff43bCAz8cERJ68ISm061gqi6RaD0Rxl
sWOrxIzJpDg0caTkaHo8p2ZPVqm+vaPsoxlBJiYoAGKvQzXz/EjzOazgPbEqk3ZK
5MZUwBRK
=Ofkd
-----END PGP SIGNATURE-----
</details>
Sorry, not going to make any more style/modernization-related changes, they can be followups.
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>`
Nice doc.
ACK c68516d662687699b399f8d12e22f2e985edae1a (jamesob/ackr/24050.2.dongcarl.validation_move_cblockin)
Still looks good. Re-reviewed the code, built new revision, ran unittests.
<details><summary>Show signature data</summary> <p>
-----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-----
</p></details>
<details><summary>Show platform data</summary> <p>
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
</p></details>
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!
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 🎨
<details><summary>Show signature</summary>
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
re-ACK 6c23c415613d8b847e6f6a2f872be893da9f4384 🎨
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjQHQwArZhANo4eHc9fHY/ogxBargWcUs82Jt+5QFFFQFB33PIjOCYNE1fpaPuf
y7SVbEdp0SvsMs2qOn86LptVA2UJQYaaYB+36/AlPo0f1tXRHmNZbFSnQicRGnYG
8Mf4S/NUS81e9Mg85rqXjTaCxeAkVtALazna7+wSE1xHcPx9VMy20OcBRye+BzkK
UmzVHIrtjA6/1lVWa7QBWo8S2se6l3GPIT7TARSuH1+tGGH3LRxJ47S5qYbTJciB
Ec1YBWiepcnq1QywMC9GXYIrYZmKPikmRhrpAIKyL3yQh76eksIFFg1qFLq6yD1d
QhKWt5PA4peyGYLhQYh6UUV1YDh5cXM1PEOur17tG6BPw3oX5+j8OsdVIfYur3V5
NM29D2T72nxaBYRrnXH+tIeY6Qz4PN+jKaZSqI3Qa8iaPounJnoryr8FN5SfKIS6
NYCmRCN/EYFC5TocfoWaEue6F69q4Y9AzWE20ko0UF2zOBtFgkv1L/+NDusi+TTc
B8Ozf9u4
=iXAq
-----END PGP SIGNATURE-----
</details>