From 1e299c97637c873028751b0587c073bf25411103 Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Sun, 5 Sep 2021 11:32:59 +0200 Subject: [PATCH] Require CBlockIndex::GetBlockPos() to hold mutex cs_main Summary: > consensus: don't call GetBlockPos in ReadBlockFromDisk without lock The `cs_main` lock only covered the first invocation of `CBlockIndex::GetBlockPos()`. Use the `blockPos` local variable instead of calling the function twice, rename it to `block_pos`, and make it const This is a backport of [[https://github.com/bitcoin/bitcoin/pull/22895 | core#22895]] ---- > Require CBlockIndex::GetBlockPos() to hold mutex cs_main This is a partial backport of [[https://github.com/bitcoin/bitcoin/pull/22932 | core#22932]] https://github.com/bitcoin/bitcoin/pull/22932/commits/6fd4341c10b319399c58d71c4ddeae4417e337d7 Test Plan: With clang and DEBUG: `ninja all check-all bitcoin-fuzzers` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D13031 --- src/blockindex.h | 6 +++++- src/index/txindex.cpp | 2 +- src/node/blockstorage.cpp | 11 ++++------- src/test/blockindex_tests.cpp | 1 + src/test/fuzz/chain.cpp | 31 +++++++++++++++++-------------- src/test/util/blockfilter.cpp | 2 ++ src/wallet/test/wallet_tests.cpp | 20 +++++++++++--------- 7 files changed, 41 insertions(+), 32 deletions(-) diff --git a/src/blockindex.h b/src/blockindex.h index 92943c7f69..a774a40375 100644 --- a/src/blockindex.h +++ b/src/blockindex.h @@ -9,11 +9,14 @@ #include #include #include +#include #include #include struct BlockHash; +extern RecursiveMutex cs_main; + /** * The block chain is a tree shaped structure starting with the genesis block at * the root, with each block potentially having multiple candidates to be the @@ -108,7 +111,8 @@ class CBlockIndex { nTime{block.nTime}, nBits{block.nBits}, nNonce{block.nNonce}, nTimeReceived{0} {} - FlatFilePos GetBlockPos() const { + FlatFilePos GetBlockPos() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { + AssertLockHeld(::cs_main); FlatFilePos ret; if (nStatus.hasData()) { ret.nFile = nFile; diff --git a/src/index/txindex.cpp b/src/index/txindex.cpp index b02ba2aec0..39ae59e79e 100644 --- a/src/index/txindex.cpp +++ b/src/index/txindex.cpp @@ -58,7 +58,7 @@ bool TxIndex::WriteBlock(const CBlock &block, const CBlockIndex *pindex) { return true; } - CDiskTxPos pos(pindex->GetBlockPos(), + CDiskTxPos pos(WITH_LOCK(::cs_main, return pindex->GetBlockPos()), GetSizeOfCompactSize(block.vtx.size())); std::vector> vPos; vPos.reserve(block.vtx.size()); diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index 6000660c6d..dd2d75a8b3 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -761,20 +761,17 @@ bool ReadBlockFromDisk(CBlock &block, const FlatFilePos &pos, bool ReadBlockFromDisk(CBlock &block, const CBlockIndex *pindex, const Consensus::Params ¶ms) { - FlatFilePos blockPos; - { - LOCK(cs_main); - blockPos = pindex->GetBlockPos(); - } + const FlatFilePos block_pos{ + WITH_LOCK(cs_main, return pindex->GetBlockPos())}; - if (!ReadBlockFromDisk(block, blockPos, params)) { + if (!ReadBlockFromDisk(block, block_pos, params)) { return false; } if (block.GetHash() != pindex->GetBlockHash()) { return error("ReadBlockFromDisk(CBlock&, CBlockIndex*): GetHash() " "doesn't match index for %s at %s", - pindex->ToString(), pindex->GetBlockPos().ToString()); + pindex->ToString(), block_pos.ToString()); } return true; diff --git a/src/test/blockindex_tests.cpp b/src/test/blockindex_tests.cpp index 8e7bcebbb4..340d5850aa 100644 --- a/src/test/blockindex_tests.cpp +++ b/src/test/blockindex_tests.cpp @@ -68,6 +68,7 @@ BOOST_AUTO_TEST_CASE(get_disk_positions) { } // Data and undo positions should be unmodified + LOCK(::cs_main); FlatFilePos dataPosition = index.GetBlockPos(); if (flags & 0x01) { BOOST_CHECK(dataPosition.nFile == expectedFile); diff --git a/src/test/fuzz/chain.cpp b/src/test/fuzz/chain.cpp index 123cae743f..cd6b421276 100644 --- a/src/test/fuzz/chain.cpp +++ b/src/test/fuzz/chain.cpp @@ -23,20 +23,23 @@ void test_one_input(const std::vector &buffer) { const BlockHash zero{}; disk_block_index->phashBlock = &zero; - (void)disk_block_index->GetBlockHash(); - (void)disk_block_index->GetBlockPos(); - (void)disk_block_index->GetBlockTime(); - (void)disk_block_index->GetBlockTimeMax(); - (void)disk_block_index->GetChainSize(); - (void)disk_block_index->GetChainTxCount(); - (void)disk_block_index->GetHeaderReceivedTime(); - (void)disk_block_index->GetMedianTimePast(); - (void)disk_block_index->GetReceivedTimeDiff(); - (void)disk_block_index->GetUndoPos(); - (void)disk_block_index->HaveTxsDownloaded(); - (void)disk_block_index->IsValid(); - (void)disk_block_index->ToString(); - (void)disk_block_index->UpdateChainStats(); + { + LOCK(::cs_main); + (void)disk_block_index->GetBlockHash(); + (void)disk_block_index->GetBlockPos(); + (void)disk_block_index->GetBlockTime(); + (void)disk_block_index->GetBlockTimeMax(); + (void)disk_block_index->GetChainSize(); + (void)disk_block_index->GetChainTxCount(); + (void)disk_block_index->GetHeaderReceivedTime(); + (void)disk_block_index->GetMedianTimePast(); + (void)disk_block_index->GetReceivedTimeDiff(); + (void)disk_block_index->GetUndoPos(); + (void)disk_block_index->HaveTxsDownloaded(); + (void)disk_block_index->IsValid(); + (void)disk_block_index->ToString(); + (void)disk_block_index->UpdateChainStats(); + } const CBlockHeader block_header = disk_block_index->GetBlockHeader(); (void)CDiskBlockIndex{*disk_block_index}; diff --git a/src/test/util/blockfilter.cpp b/src/test/util/blockfilter.cpp index 73162a200c..eea87c1748 100644 --- a/src/test/util/blockfilter.cpp +++ b/src/test/util/blockfilter.cpp @@ -13,6 +13,8 @@ using node::UndoReadFromDisk; bool ComputeFilter(BlockFilterType filter_type, const CBlockIndex *block_index, BlockFilter &filter) { + LOCK(::cs_main); + CBlock block; if (!ReadBlockFromDisk(block, block_index->GetBlockPos(), Params().GetConsensus())) { diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index a6f443dc59..77be262d89 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -135,12 +135,13 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup) { } // Prune the older block file. + int file_number; { LOCK(cs_main); - Assert(m_node.chainman) - ->m_blockman.PruneOneBlockFile(oldTip->GetBlockPos().nFile); + file_number = oldTip->GetBlockPos().nFile; + Assert(m_node.chainman)->m_blockman.PruneOneBlockFile(file_number); } - UnlinkPrunedFiles({oldTip->GetBlockPos().nFile}); + UnlinkPrunedFiles({file_number}); // Verify ScanForWalletTransactions only picks transactions in the new block // file. @@ -168,10 +169,10 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup) { // Prune the remaining block file. { LOCK(cs_main); - Assert(m_node.chainman) - ->m_blockman.PruneOneBlockFile(newTip->GetBlockPos().nFile); + file_number = newTip->GetBlockPos().nFile; + Assert(m_node.chainman)->m_blockman.PruneOneBlockFile(file_number); } - UnlinkPrunedFiles({newTip->GetBlockPos().nFile}); + UnlinkPrunedFiles({file_number}); // Verify ScanForWalletTransactions scans no blocks. { @@ -206,12 +207,13 @@ BOOST_FIXTURE_TEST_CASE(importmulti_rescan, TestChain100Setup) { CBlockIndex *newTip = m_node.chainman->ActiveTip(); // Prune the older block file. + int file_number; { LOCK(cs_main); - Assert(m_node.chainman) - ->m_blockman.PruneOneBlockFile(oldTip->GetBlockPos().nFile); + file_number = oldTip->GetBlockPos().nFile; + Assert(m_node.chainman)->m_blockman.PruneOneBlockFile(file_number); } - UnlinkPrunedFiles({oldTip->GetBlockPos().nFile}); + UnlinkPrunedFiles({file_number}); // Verify importmulti RPC returns failure for a key whose creation time is // before the missing block, and success for a key whose creation time is