mirrors/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2024-11-19 18:09:47 +01:00
Code Issues Projects Releases Wiki Activity

Merge bitcoin/bitcoin#22932: Add CBlockIndex lock annotations, guard nStatus/nFile/nDataPos/nUndoPos by cs_main

Browse Source 
6ea5682784 Guard CBlockIndex::nStatus/nFile/nDataPos/nUndoPos by cs_main (Jon Atack)
5d59ae0ba8 Remove/inline ReadRawBlockFromDisk(block_data, pindex, message_start) (Hennadii Stepanov)
eaeeb88768 Require IsBlockPruned() to hold mutex cs_main (Jon Atack)
ca47b00577 Require CBlockIndex::IsValid() to hold cs_main (Vasil Dimov)
e9f3aa5f6a Require CBlockIndex::RaiseValidity() to hold cs_main (Vasil Dimov)
8ef457cb83 Require CBlockIndex::IsAssumedValid() to hold cs_main (Vasil Dimov)
572393448b Require CBlockIndex::GetUndoPos() to hold mutex cs_main (Jon Atack)
2e557ced28 Require WriteUndoDataForBlock() to hold mutex cs_main (Jon Atack)
6fd4341c10 Require CBlockIndex::GetBlockPos() to hold mutex cs_main (Jon Atack)

Pull request description:

  Issues:

  - `CBlockIndex` member functions `GetBlockPos()`, `GetUndoPos()`, `IsAssumedValid()`, `RaiseValidity()`, and `IsValid()` and block storage functions `WriteUndoDataForBlock()` and `IsBlockPruned()` are missing thread safety lock annotations to help ensure that they are called with mutex cs_main to avoid bugs like #22895. Doing this also enables the next step:

  - `CBlockIndex::nStatus` may be racy, i.e. potentially accessed by multiple threads, see #17161. A solution is to guard it by cs_main, along with fellow data members `nFile`, `nDataPos` and `nUndoPos`.

  This pull:

  - adds thread safety lock annotations for the functions listed above
  - guards `CBlockIndex::nStatus`, `nFile`, `nDataPos` and `nUndoPos` by cs_main

  How to review and test:
  - debug build with clang and verify there are no `-Wthread-safety-analysis` warnings
  - review the code to verify each annotation or lock is necessary and sensible, or if any are missing
  - look for whether taking a lock can be replaced by a lock annotation instead
  - for more information about Clang thread safety analysis, see
      - https://clang.llvm.org/docs/ThreadSafetyAnalysis.html
      - https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#lockingmutex-usage-notes
      - https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#threads-and-synchronization

  Mitigates/potentially closes #17161.

ACKs for top commit:
  laanwj:
    Code review ACK 6ea5682784

Tree-SHA512: 3ebf429c8623c51f944a7245a2e48d2aa088dec4c4914b40aa6049e89856c1ee8586f6e2e3b65195190566637a33004468b51a781e61a082248748015167569b
This commit is contained in:
laanwj 2022-01-27 10:51:54 +01:00
commit cf5bb048e8
No known key found for this signature in database
GPG Key ID: 1E4AED62986CD25D
13 changed files with 82 additions and 52 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

29
src/chain.h
View File

@ -10,6 +10,7 @@
#include <consensus/params.h>
#include <flatfile.h>
#include <primitives/block.h>
#include <sync.h>
#include <tinyformat.h>
#include <uint256.h>
@ -37,6 +38,8 @@ static constexpr int64_t TIMESTAMP_WINDOW = MAX_FUTURE_BLOCK_TIME;
*/
static constexpr int64_t MAX_BLOCK_TIME_GAP = 90 * 60;
extern RecursiveMutex cs_main;
class CBlockFileInfo
{
public:
@ -161,13 +164,13 @@ public:
int nHeight{0};
//! Which # file this block is stored in (blk?????.dat)
int nFile{0};
int nFile GUARDED_BY(::cs_main){0};
//! Byte offset within blk?????.dat where this block's data is stored
unsigned int nDataPos{0};
unsigned int nDataPos GUARDED_BY(::cs_main){0};
//! Byte offset within rev?????.dat where this block's undo data is stored
unsigned int nUndoPos{0};
unsigned int nUndoPos GUARDED_BY(::cs_main){0};
//! (memory only) Total amount of work (expected number of hashes) in the chain up to and including this block
arith_uint256 nChainWork{};
@ -195,7 +198,7 @@ public:
//! load to avoid the block index being spuriously rewound.
//! @sa NeedsRedownload
//! @sa ActivateSnapshot
uint32_t nStatus{0};
uint32_t nStatus GUARDED_BY(::cs_main){0};
//! block header
int32_t nVersion{0};
@ -223,8 +226,9 @@ public:
{
}
FlatFilePos GetBlockPos() const
FlatFilePos GetBlockPos() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
{
AssertLockHeld(::cs_main);
FlatFilePos ret;
if (nStatus & BLOCK_HAVE_DATA) {
ret.nFile = nFile;
@ -233,8 +237,9 @@ public:
return ret;
}
FlatFilePos GetUndoPos() const
FlatFilePos GetUndoPos() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
{
AssertLockHeld(::cs_main);
FlatFilePos ret;
if (nStatus & BLOCK_HAVE_UNDO) {
ret.nFile = nFile;
@ -306,7 +311,9 @@ public:
//! Check whether this block index entry is valid up to the passed validity level.
bool IsValid(enum BlockStatus nUpTo = BLOCK_VALID_TRANSACTIONS) const
EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
{
AssertLockHeld(::cs_main);
assert(!(nUpTo & ~BLOCK_VALID_MASK)); // Only validity flags allowed.
if (nStatus & BLOCK_FAILED_MASK)
return false;
@ -315,12 +322,17 @@ public:
//! @returns true if the block is assumed-valid; this means it is queued to be
//! validated by a background chainstate.
bool IsAssumedValid() const { return nStatus & BLOCK_ASSUMED_VALID; }
bool IsAssumedValid() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
{
AssertLockHeld(::cs_main);
return nStatus & BLOCK_ASSUMED_VALID;
}
//! Raise the validity level of this block index entry.
//! Returns true if the validity was changed.
bool RaiseValidity(enum BlockStatus nUpTo)
bool RaiseValidity(enum BlockStatus nUpTo) EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
{
AssertLockHeld(::cs_main);
assert(!(nUpTo & ~BLOCK_VALID_MASK)); // Only validity flags allowed.
if (nStatus & BLOCK_FAILED_MASK) return false;
@ -370,6 +382,7 @@ public:
SERIALIZE_METHODS(CDiskBlockIndex, obj)
{
LOCK(::cs_main);
int _nVersion = s.GetVersion();
if (!(s.GetType() & SER_GETHASH)) READWRITE(VARINT_MODE(_nVersion, VarIntMode::NONNEGATIVE_SIGNED));

4
src/index/txindex.cpp
View File

@ -59,7 +59,9 @@ bool TxIndex::WriteBlock(const CBlock& block, const CBlockIndex* pindex)
// Exclude genesis block transaction because outputs are not spendable.
if (pindex->nHeight == 0) return true;
CDiskTxPos pos(pindex->GetBlockPos(), GetSizeOfCompactSize(block.vtx.size()));
CDiskTxPos pos{
WITH_LOCK(::cs_main, return pindex->GetBlockPos()),
GetSizeOfCompactSize(block.vtx.size())};
std::vector<std::pair<uint256, CDiskTxPos>> vPos;
vPos.reserve(block.vtx.size());
for (const auto& tx : block.vtx) {

2
src/net_processing.cpp
View File

@ -1880,7 +1880,7 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv&
// Fast-path: in this case it is possible to serve the block directly from disk,
// as the network format matches the format on disk
std::vector<uint8_t> block_data;
if (!ReadRawBlockFromDisk(block_data, pindex, m_chainparams.MessageStart())) {
if (!ReadRawBlockFromDisk(block_data, pindex->GetBlockPos(), m_chainparams.MessageStart())) {
assert(!"cannot load block from disk");
}
m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::BLOCK, Span{block_data}));

16
src/node/blockstorage.cpp
View File

@ -429,6 +429,7 @@ CBlockIndex* BlockManager::GetLastCheckpoint(const CCheckpointData& data)
bool IsBlockPruned(const CBlockIndex* pblockindex)
{
AssertLockHeld(::cs_main);
return (fHavePruned && !(pblockindex->nStatus & BLOCK_HAVE_DATA) && pblockindex->nTx > 0);
}
@ -513,7 +514,8 @@ static bool UndoWriteToDisk(const CBlockUndo& blockundo, FlatFilePos& pos, const
bool UndoReadFromDisk(CBlockUndo& blockundo, const CBlockIndex* pindex)
{
FlatFilePos pos = pindex->GetUndoPos();
const FlatFilePos pos{WITH_LOCK(::cs_main, return pindex->GetUndoPos())};
if (pos.IsNull()) {
return error("%s: no undo data available", __func__);
}
@ -712,6 +714,7 @@ static bool WriteBlockToDisk(const CBlock& block, FlatFilePos& pos, const CMessa
bool BlockManager::WriteUndoDataForBlock(const CBlockUndo& blockundo, BlockValidationState& state, CBlockIndex* pindex, const CChainParams& chainparams)
{
AssertLockHeld(::cs_main);
// Write undo information to disk
if (pindex->GetUndoPos().IsNull()) {
FlatFilePos _pos;
@ -818,17 +821,6 @@ bool ReadRawBlockFromDisk(std::vector<uint8_t>& block, const FlatFilePos& pos, c
return true;
}
bool ReadRawBlockFromDisk(std::vector<uint8_t>& block, const CBlockIndex* pindex, const CMessageHeader::MessageStartChars& message_start)
{
FlatFilePos block_pos;
{
LOCK(cs_main);
block_pos = pindex->GetBlockPos();
}
return ReadRawBlockFromDisk(block, block_pos, message_start);
}
/** Store block on disk. If dbp is non-nullptr, the file is known to already reside on disk */
FlatFilePos BlockManager::SaveBlockToDisk(const CBlock& block, int nHeight, CChain& active_chain, const CChainParams& chainparams, const FlatFilePos* dbp)
{

9
src/node/blockstorage.h
View File

@ -7,12 +7,15 @@
#include <fs.h>
#include <protocol.h> // For CMessageHeader::MessageStartChars
#include <sync.h>
#include <txdb.h>
#include <atomic>
#include <cstdint>
#include <vector>
extern RecursiveMutex cs_main;
class ArgsManager;
class BlockValidationState;
class CBlock;
@ -146,7 +149,8 @@ public:
/** Get block file info entry for one block file */
CBlockFileInfo* GetBlockFileInfo(size_t n);
bool WriteUndoDataForBlock(const CBlockUndo& blockundo, BlockValidationState& state, CBlockIndex* pindex, const CChainParams& chainparams);
bool WriteUndoDataForBlock(const CBlockUndo& blockundo, BlockValidationState& state, CBlockIndex* pindex, const CChainParams& chainparams)
EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
FlatFilePos SaveBlockToDisk(const CBlock& block, int nHeight, CChain& active_chain, const CChainParams& chainparams, const FlatFilePos* dbp);
@ -163,7 +167,7 @@ public:
};
//! Check whether the block associated with this index entry is pruned or not.
bool IsBlockPruned(const CBlockIndex* pblockindex);
bool IsBlockPruned(const CBlockIndex* pblockindex) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
void CleanupBlockRevFiles();
@ -181,7 +185,6 @@ void UnlinkPrunedFiles(const std::set<int>& setFilesToPrune);
bool ReadBlockFromDisk(CBlock& block, const FlatFilePos& pos, const Consensus::Params& consensusParams);
bool ReadBlockFromDisk(CBlock& block, const CBlockIndex* pindex, const Consensus::Params& consensusParams);
bool ReadRawBlockFromDisk(std::vector<uint8_t>& block, const FlatFilePos& pos, const CMessageHeader::MessageStartChars& message_start);
bool ReadRawBlockFromDisk(std::vector<uint8_t>& block, const CBlockIndex* pindex, const CMessageHeader::MessageStartChars& message_start);
bool UndoReadFromDisk(CBlockUndo& blockundo, const CBlockIndex* pindex);

11
src/rpc/blockchain.cpp
View File

@ -185,7 +185,7 @@ UniValue blockToJSON(const CBlock& block, const CBlockIndex* tip, const CBlockIn
case TxVerbosity::SHOW_DETAILS:
case TxVerbosity::SHOW_DETAILS_AND_PREVOUT:
CBlockUndo blockUndo;
const bool have_undo = !IsBlockPruned(blockindex) && UndoReadFromDisk(blockUndo, blockindex);
const bool have_undo{WITH_LOCK(::cs_main, return !IsBlockPruned(blockindex) && UndoReadFromDisk(blockUndo, blockindex))};
for (size_t i = 0; i < block.vtx.size(); ++i) {
const CTransactionRef& tx = block.vtx.at(i);
@ -818,7 +818,8 @@ static RPCHelpMan getblockfrompeer()
throw JSONRPCError(RPC_MISC_ERROR, "Block header missing");
}
if (index->nStatus & BLOCK_HAVE_DATA) {
const bool block_has_data = WITH_LOCK(::cs_main, return index->nStatus & BLOCK_HAVE_DATA);
if (block_has_data) {
throw JSONRPCError(RPC_MISC_ERROR, "Block already downloaded");
}
@ -929,8 +930,9 @@ static RPCHelpMan getblockheader()
};
}
static CBlock GetBlockChecked(const CBlockIndex* pblockindex)
static CBlock GetBlockChecked(const CBlockIndex* pblockindex) EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
{
AssertLockHeld(::cs_main);
CBlock block;
if (IsBlockPruned(pblockindex)) {
throw JSONRPCError(RPC_MISC_ERROR, "Block not available (pruned data)");
@ -946,8 +948,9 @@ static CBlock GetBlockChecked(const CBlockIndex* pblockindex)
return block;
}
static CBlockUndo GetUndoChecked(const CBlockIndex* pblockindex)
static CBlockUndo GetUndoChecked(const CBlockIndex* pblockindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
{
AssertLockHeld(::cs_main);
CBlockUndo blockUndo;
if (IsBlockPruned(pblockindex)) {
throw JSONRPCError(RPC_MISC_ERROR, "Undo data not available (pruned data)");

3
src/rpc/rawtransaction.cpp
View File

@ -241,7 +241,8 @@ static RPCHelpMan getrawtransaction()
if (!tx) {
std::string errmsg;
if (blockindex) {
if (!(blockindex->nStatus & BLOCK_HAVE_DATA)) {
const bool block_has_data = WITH_LOCK(::cs_main, return blockindex->nStatus & BLOCK_HAVE_DATA);
if (!block_has_data) {
throw JSONRPCError(RPC_MISC_ERROR, "Block not available");
}
errmsg = "No such transaction found in the provided block";

23
src/test/fuzz/chain.cpp
View File

@ -21,15 +21,18 @@ FUZZ_TARGET(chain)
const uint256 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->GetMedianTimePast();
(void)disk_block_index->GetUndoPos();
(void)disk_block_index->HaveTxsDownloaded();
(void)disk_block_index->IsValid();
(void)disk_block_index->ToString();
{
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->GetMedianTimePast();
(void)disk_block_index->GetUndoPos();
(void)disk_block_index->HaveTxsDownloaded();
(void)disk_block_index->IsValid();
(void)disk_block_index->ToString();
}
const CBlockHeader block_header = disk_block_index->GetBlockHeader();
(void)CDiskBlockIndex{*disk_block_index};
@ -55,7 +58,7 @@ FUZZ_TARGET(chain)
if (block_status & ~BLOCK_VALID_MASK) {
continue;
}
(void)disk_block_index->RaiseValidity(block_status);
WITH_LOCK(::cs_main, (void)disk_block_index->RaiseValidity(block_status));
}
CBlockIndex block_index{block_header};

1
src/test/interfaces_tests.cpp
View File

@ -123,6 +123,7 @@ BOOST_AUTO_TEST_CASE(findCommonAncestor)
BOOST_AUTO_TEST_CASE(hasBlocks)
{
LOCK(::cs_main);
auto& chain = m_node.chain;
const CChain& active = Assert(m_node.chainman)->ActiveChain();

2
src/test/util/blockfilter.cpp
View File

@ -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())) {
return false;

3
src/test/validation_chainstatemanager_tests.cpp
View File

@ -235,7 +235,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_activate_snapshot, TestChain100Setup)
*chainman.SnapshotBlockhash());
// Ensure that the genesis block was not marked assumed-valid.
BOOST_CHECK(!chainman.ActiveChain().Genesis()->IsAssumedValid());
BOOST_CHECK(WITH_LOCK(::cs_main, return !chainman.ActiveChain().Genesis()->IsAssumedValid()));
const AssumeutxoData& au_data = *ExpectedAssumeutxo(snapshot_height, ::Params());
const CBlockIndex* tip = chainman.ActiveTip();
@ -356,6 +356,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_loadblockindex, TestChain100Setup)
// Mark some region of the chain assumed-valid.
for (int i = 0; i <= cs1.m_chain.Height(); ++i) {
LOCK(::cs_main);
auto index = cs1.m_chain[i];
if (i < last_assumed_valid_idx && i >= assumed_valid_start_idx) {

14
src/txdb.cpp
View File

@ -311,19 +311,23 @@ bool CBlockTreeDB::LoadBlockIndexGuts(const Consensus::Params& consensusParams,
CBlockIndex* pindexNew = insertBlockIndex(diskindex.GetBlockHash());
pindexNew->pprev = insertBlockIndex(diskindex.hashPrev);
pindexNew->nHeight = diskindex.nHeight;
pindexNew->nFile = diskindex.nFile;
pindexNew->nDataPos = diskindex.nDataPos;
pindexNew->nUndoPos = diskindex.nUndoPos;
pindexNew->nVersion = diskindex.nVersion;
pindexNew->hashMerkleRoot = diskindex.hashMerkleRoot;
pindexNew->nTime = diskindex.nTime;
pindexNew->nBits = diskindex.nBits;
pindexNew->nNonce = diskindex.nNonce;
pindexNew->nStatus = diskindex.nStatus;
pindexNew->nTx = diskindex.nTx;
{
LOCK(::cs_main);
pindexNew->nFile = diskindex.nFile;
pindexNew->nDataPos = diskindex.nDataPos;
pindexNew->nUndoPos = diskindex.nUndoPos;
pindexNew->nStatus = diskindex.nStatus;
}
if (!CheckProofOfWork(pindexNew->GetBlockHash(), pindexNew->nBits, consensusParams))
if (!CheckProofOfWork(pindexNew->GetBlockHash(), pindexNew->nBits, consensusParams)) {
return error("%s: CheckProofOfWork failed: %s", __func__, pindexNew->ToString());
}
pcursor->Next();
} else {

17
src/wallet/test/wallet_tests.cpp
View File

@ -140,11 +140,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.
@ -169,9 +171,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.
{
@ -202,11 +205,13 @@ BOOST_FIXTURE_TEST_CASE(importmulti_rescan, TestChain100Setup)
CBlockIndex* newTip = m_node.chainman->ActiveChain().Tip();
// 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