From fa604eb6cfa7f70ce11c78c1060f0823884c745b Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Thu, 7 Dec 2023 10:21:14 +0100 Subject: [PATCH 1/2] refactor: Use reference instead of pointer in IsBlockPruned This makes it harder to pass nullptr and cause issues such as https://github.com/bitcoin/bitcoin/commit/dde7ac5c704688c8a9af29bd07e5ae8114824ce7 --- src/node/blockstorage.cpp | 4 ++-- src/node/blockstorage.h | 2 +- src/rest.cpp | 5 ++--- src/rpc/blockchain.cpp | 6 +++--- src/rpc/rawtransaction.cpp | 7 +++---- 5 files changed, 11 insertions(+), 13 deletions(-) diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index ebc08d7567..e6164c2e59 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -582,10 +582,10 @@ const CBlockIndex* BlockManager::GetLastCheckpoint(const CCheckpointData& data) return nullptr; } -bool BlockManager::IsBlockPruned(const CBlockIndex* pblockindex) +bool BlockManager::IsBlockPruned(const CBlockIndex& block) { AssertLockHeld(::cs_main); - return (m_have_pruned && !(pblockindex->nStatus & BLOCK_HAVE_DATA) && pblockindex->nTx > 0); + return m_have_pruned && !(block.nStatus & BLOCK_HAVE_DATA) && (block.nTx > 0); } const CBlockIndex* BlockManager::GetFirstStoredBlock(const CBlockIndex& upper_block, const CBlockIndex* lower_block) diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h index d540406ae5..ce514cc645 100644 --- a/src/node/blockstorage.h +++ b/src/node/blockstorage.h @@ -344,7 +344,7 @@ public: bool m_have_pruned = false; //! Check whether the block associated with this index entry is pruned or not. - bool IsBlockPruned(const CBlockIndex* pblockindex) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + bool IsBlockPruned(const CBlockIndex& block) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); //! Create or update a prune lock identified by its name void UpdatePruneLock(const std::string& name, const PruneLockInfo& lock_info) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); diff --git a/src/rest.cpp b/src/rest.cpp index bb54e780b2..d0d62db728 100644 --- a/src/rest.cpp +++ b/src/rest.cpp @@ -304,10 +304,9 @@ static bool rest_block(const std::any& context, if (!pblockindex) { return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not found"); } - - if (chainman.m_blockman.IsBlockPruned(pblockindex)) + if (chainman.m_blockman.IsBlockPruned(*pblockindex)) { return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not available (pruned data)"); - + } } if (!chainman.m_blockman.ReadBlockFromDisk(block, *pblockindex)) { diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index be6a8c47fd..ea9bf4cbb9 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -181,7 +181,7 @@ UniValue blockToJSON(BlockManager& blockman, const CBlock& block, const CBlockIn case TxVerbosity::SHOW_DETAILS: case TxVerbosity::SHOW_DETAILS_AND_PREVOUT: CBlockUndo blockUndo; - const bool is_not_pruned{WITH_LOCK(::cs_main, return !blockman.IsBlockPruned(blockindex))}; + const bool is_not_pruned{WITH_LOCK(::cs_main, return !blockman.IsBlockPruned(*blockindex))}; const bool have_undo{is_not_pruned && blockman.UndoReadFromDisk(blockUndo, *blockindex)}; for (size_t i = 0; i < block.vtx.size(); ++i) { @@ -581,7 +581,7 @@ static CBlock GetBlockChecked(BlockManager& blockman, const CBlockIndex* pblocki CBlock block; { LOCK(cs_main); - if (blockman.IsBlockPruned(pblockindex)) { + if (blockman.IsBlockPruned(*pblockindex)) { throw JSONRPCError(RPC_MISC_ERROR, "Block not available (pruned data)"); } } @@ -605,7 +605,7 @@ static CBlockUndo GetUndoChecked(BlockManager& blockman, const CBlockIndex* pblo { LOCK(cs_main); - if (blockman.IsBlockPruned(pblockindex)) { + if (blockman.IsBlockPruned(*pblockindex)) { throw JSONRPCError(RPC_MISC_ERROR, "Undo data not available (pruned data)"); } } diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 5f68e7832c..b1e6d677f8 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -394,18 +394,17 @@ static RPCHelpMan getrawtransaction() // If request is verbosity >= 1 but no blockhash was given, then look up the blockindex if (request.params[2].isNull()) { LOCK(cs_main); - blockindex = chainman.m_blockman.LookupBlockIndex(hash_block); + blockindex = chainman.m_blockman.LookupBlockIndex(hash_block); // May be nullptr for mempool transactions } - if (verbosity == 1 || !blockindex) { + if (verbosity == 1) { TxToJSON(*tx, hash_block, result, chainman.ActiveChainstate()); return result; } CBlockUndo blockUndo; CBlock block; - const bool is_block_pruned{WITH_LOCK(cs_main, return chainman.m_blockman.IsBlockPruned(blockindex))}; - if (tx->IsCoinBase() || is_block_pruned || + if (tx->IsCoinBase() || !blockindex || WITH_LOCK(::cs_main, return chainman.m_blockman.IsBlockPruned(*blockindex)) || !(chainman.m_blockman.UndoReadFromDisk(blockUndo, *blockindex) && chainman.m_blockman.ReadBlockFromDisk(block, *blockindex))) { TxToJSON(*tx, hash_block, result, chainman.ActiveChainstate()); return result; From fa5989d514d246e56977c528b2dd2abe6dc8efcc Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Thu, 7 Dec 2023 11:01:06 +0100 Subject: [PATCH 2/2] refactor: rpc: Pass CBlockIndex by reference instead of pointer All functions assume that the pointer is never null, so pass by reference, to avoid accidental segfaults at runtime, or at least make them more obvious. Also, remove unused c-style casts in touched lines. Also, add CHECK_NONFATAL checks, to turn segfault crashes into an recoverable runtime error with debug information. --- src/bench/rpc_blockchain.cpp | 4 +- src/rest.cpp | 4 +- src/rpc/blockchain.cpp | 86 +++++++++++++++++------------------ src/rpc/blockchain.h | 6 +-- src/rpc/mining.cpp | 2 +- src/test/blockchain_tests.cpp | 2 +- 6 files changed, 51 insertions(+), 53 deletions(-) diff --git a/src/bench/rpc_blockchain.cpp b/src/bench/rpc_blockchain.cpp index 2416d40798..713853e8c5 100644 --- a/src/bench/rpc_blockchain.cpp +++ b/src/bench/rpc_blockchain.cpp @@ -41,7 +41,7 @@ static void BlockToJsonVerbose(benchmark::Bench& bench) { TestBlockAndIndex data; bench.run([&] { - auto univalue = blockToJSON(data.testing_setup->m_node.chainman->m_blockman, data.block, &data.blockindex, &data.blockindex, TxVerbosity::SHOW_DETAILS_AND_PREVOUT); + auto univalue = blockToJSON(data.testing_setup->m_node.chainman->m_blockman, data.block, data.blockindex, data.blockindex, TxVerbosity::SHOW_DETAILS_AND_PREVOUT); ankerl::nanobench::doNotOptimizeAway(univalue); }); } @@ -51,7 +51,7 @@ BENCHMARK(BlockToJsonVerbose, benchmark::PriorityLevel::HIGH); static void BlockToJsonVerboseWrite(benchmark::Bench& bench) { TestBlockAndIndex data; - auto univalue = blockToJSON(data.testing_setup->m_node.chainman->m_blockman, data.block, &data.blockindex, &data.blockindex, TxVerbosity::SHOW_DETAILS_AND_PREVOUT); + auto univalue = blockToJSON(data.testing_setup->m_node.chainman->m_blockman, data.block, data.blockindex, data.blockindex, TxVerbosity::SHOW_DETAILS_AND_PREVOUT); bench.run([&] { auto str = univalue.write(); ankerl::nanobench::doNotOptimizeAway(str); diff --git a/src/rest.cpp b/src/rest.cpp index d0d62db728..e47b52fb53 100644 --- a/src/rest.cpp +++ b/src/rest.cpp @@ -264,7 +264,7 @@ static bool rest_headers(const std::any& context, case RESTResponseFormat::JSON: { UniValue jsonHeaders(UniValue::VARR); for (const CBlockIndex *pindex : headers) { - jsonHeaders.push_back(blockheaderToJSON(tip, pindex)); + jsonHeaders.push_back(blockheaderToJSON(*tip, *pindex)); } std::string strJSON = jsonHeaders.write() + "\n"; req->WriteHeader("Content-Type", "application/json"); @@ -333,7 +333,7 @@ static bool rest_block(const std::any& context, } case RESTResponseFormat::JSON: { - UniValue objBlock = blockToJSON(chainman.m_blockman, block, tip, pblockindex, tx_verbosity); + UniValue objBlock = blockToJSON(chainman.m_blockman, block, *tip, *pblockindex, tx_verbosity); std::string strJSON = objBlock.write() + "\n"; req->WriteHeader("Content-Type", "application/json"); req->WriteReply(HTTP_OK, strJSON); diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index ea9bf4cbb9..6f20b1031c 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -73,13 +73,11 @@ static CUpdatedBlock latestblock GUARDED_BY(cs_blockchange); /* Calculate the difficulty for a given block index. */ -double GetDifficulty(const CBlockIndex* blockindex) +double GetDifficulty(const CBlockIndex& blockindex) { - CHECK_NONFATAL(blockindex); - - int nShift = (blockindex->nBits >> 24) & 0xff; + int nShift = (blockindex.nBits >> 24) & 0xff; double dDiff = - (double)0x0000ffff / (double)(blockindex->nBits & 0x00ffffff); + (double)0x0000ffff / (double)(blockindex.nBits & 0x00ffffff); while (nShift < 29) { @@ -95,14 +93,14 @@ double GetDifficulty(const CBlockIndex* blockindex) return dDiff; } -static int ComputeNextBlockAndDepth(const CBlockIndex* tip, const CBlockIndex* blockindex, const CBlockIndex*& next) +static int ComputeNextBlockAndDepth(const CBlockIndex& tip, const CBlockIndex& blockindex, const CBlockIndex*& next) { - next = tip->GetAncestor(blockindex->nHeight + 1); - if (next && next->pprev == blockindex) { - return tip->nHeight - blockindex->nHeight + 1; + next = tip.GetAncestor(blockindex.nHeight + 1); + if (next && next->pprev == &blockindex) { + return tip.nHeight - blockindex.nHeight + 1; } next = nullptr; - return blockindex == tip ? 1 : -1; + return &blockindex == &tip ? 1 : -1; } static const CBlockIndex* ParseHashOrHeight(const UniValue& param, ChainstateManager& chainman) @@ -133,36 +131,36 @@ static const CBlockIndex* ParseHashOrHeight(const UniValue& param, ChainstateMan } } -UniValue blockheaderToJSON(const CBlockIndex* tip, const CBlockIndex* blockindex) +UniValue blockheaderToJSON(const CBlockIndex& tip, const CBlockIndex& blockindex) { // Serialize passed information without accessing chain state of the active chain! AssertLockNotHeld(cs_main); // For performance reasons UniValue result(UniValue::VOBJ); - result.pushKV("hash", blockindex->GetBlockHash().GetHex()); + result.pushKV("hash", blockindex.GetBlockHash().GetHex()); const CBlockIndex* pnext; int confirmations = ComputeNextBlockAndDepth(tip, blockindex, pnext); result.pushKV("confirmations", confirmations); - result.pushKV("height", blockindex->nHeight); - result.pushKV("version", blockindex->nVersion); - result.pushKV("versionHex", strprintf("%08x", blockindex->nVersion)); - result.pushKV("merkleroot", blockindex->hashMerkleRoot.GetHex()); - result.pushKV("time", (int64_t)blockindex->nTime); - result.pushKV("mediantime", (int64_t)blockindex->GetMedianTimePast()); - result.pushKV("nonce", (uint64_t)blockindex->nNonce); - result.pushKV("bits", strprintf("%08x", blockindex->nBits)); + result.pushKV("height", blockindex.nHeight); + result.pushKV("version", blockindex.nVersion); + result.pushKV("versionHex", strprintf("%08x", blockindex.nVersion)); + result.pushKV("merkleroot", blockindex.hashMerkleRoot.GetHex()); + result.pushKV("time", blockindex.nTime); + result.pushKV("mediantime", blockindex.GetMedianTimePast()); + result.pushKV("nonce", blockindex.nNonce); + result.pushKV("bits", strprintf("%08x", blockindex.nBits)); result.pushKV("difficulty", GetDifficulty(blockindex)); - result.pushKV("chainwork", blockindex->nChainWork.GetHex()); - result.pushKV("nTx", (uint64_t)blockindex->nTx); + result.pushKV("chainwork", blockindex.nChainWork.GetHex()); + result.pushKV("nTx", blockindex.nTx); - if (blockindex->pprev) - result.pushKV("previousblockhash", blockindex->pprev->GetBlockHash().GetHex()); + if (blockindex.pprev) + result.pushKV("previousblockhash", blockindex.pprev->GetBlockHash().GetHex()); if (pnext) result.pushKV("nextblockhash", pnext->GetBlockHash().GetHex()); return result; } -UniValue blockToJSON(BlockManager& blockman, const CBlock& block, const CBlockIndex* tip, const CBlockIndex* blockindex, TxVerbosity verbosity) +UniValue blockToJSON(BlockManager& blockman, const CBlock& block, const CBlockIndex& tip, const CBlockIndex& blockindex, TxVerbosity verbosity) { UniValue result = blockheaderToJSON(tip, blockindex); @@ -181,8 +179,8 @@ UniValue blockToJSON(BlockManager& blockman, const CBlock& block, const CBlockIn case TxVerbosity::SHOW_DETAILS: case TxVerbosity::SHOW_DETAILS_AND_PREVOUT: CBlockUndo blockUndo; - const bool is_not_pruned{WITH_LOCK(::cs_main, return !blockman.IsBlockPruned(*blockindex))}; - const bool have_undo{is_not_pruned && blockman.UndoReadFromDisk(blockUndo, *blockindex)}; + const bool is_not_pruned{WITH_LOCK(::cs_main, return !blockman.IsBlockPruned(blockindex))}; + const bool have_undo{is_not_pruned && blockman.UndoReadFromDisk(blockUndo, blockindex)}; for (size_t i = 0; i < block.vtx.size(); ++i) { const CTransactionRef& tx = block.vtx.at(i); @@ -418,7 +416,7 @@ static RPCHelpMan getdifficulty() { ChainstateManager& chainman = EnsureAnyChainman(request.context); LOCK(cs_main); - return GetDifficulty(chainman.ActiveChain().Tip()); + return GetDifficulty(*CHECK_NONFATAL(chainman.ActiveChain().Tip())); }, }; } @@ -571,22 +569,22 @@ static RPCHelpMan getblockheader() return strHex; } - return blockheaderToJSON(tip, pblockindex); + return blockheaderToJSON(*tip, *pblockindex); }, }; } -static CBlock GetBlockChecked(BlockManager& blockman, const CBlockIndex* pblockindex) +static CBlock GetBlockChecked(BlockManager& blockman, const CBlockIndex& blockindex) { CBlock block; { LOCK(cs_main); - if (blockman.IsBlockPruned(*pblockindex)) { + if (blockman.IsBlockPruned(blockindex)) { throw JSONRPCError(RPC_MISC_ERROR, "Block not available (pruned data)"); } } - if (!blockman.ReadBlockFromDisk(block, *pblockindex)) { + if (!blockman.ReadBlockFromDisk(block, blockindex)) { // Block not found on disk. This could be because we have the block // header in our index but not yet have the block or did not accept the // block. Or if the block was pruned right after we released the lock above. @@ -596,21 +594,21 @@ static CBlock GetBlockChecked(BlockManager& blockman, const CBlockIndex* pblocki return block; } -static CBlockUndo GetUndoChecked(BlockManager& blockman, const CBlockIndex* pblockindex) +static CBlockUndo GetUndoChecked(BlockManager& blockman, const CBlockIndex& blockindex) { CBlockUndo blockUndo; // The Genesis block does not have undo data - if (pblockindex->nHeight == 0) return blockUndo; + if (blockindex.nHeight == 0) return blockUndo; { LOCK(cs_main); - if (blockman.IsBlockPruned(*pblockindex)) { + if (blockman.IsBlockPruned(blockindex)) { throw JSONRPCError(RPC_MISC_ERROR, "Undo data not available (pruned data)"); } } - if (!blockman.UndoReadFromDisk(blockUndo, *pblockindex)) { + if (!blockman.UndoReadFromDisk(blockUndo, blockindex)) { throw JSONRPCError(RPC_MISC_ERROR, "Can't read undo data from disk"); } @@ -736,7 +734,7 @@ static RPCHelpMan getblock() } } - const CBlock block{GetBlockChecked(chainman.m_blockman, pblockindex)}; + const CBlock block{GetBlockChecked(chainman.m_blockman, *pblockindex)}; if (verbosity <= 0) { @@ -755,7 +753,7 @@ static RPCHelpMan getblock() tx_verbosity = TxVerbosity::SHOW_DETAILS_AND_PREVOUT; } - return blockToJSON(chainman.m_blockman, block, tip, pblockindex, tx_verbosity); + return blockToJSON(chainman.m_blockman, block, *tip, *pblockindex, tx_verbosity); }, }; } @@ -1257,7 +1255,7 @@ RPCHelpMan getblockchaininfo() obj.pushKV("blocks", height); obj.pushKV("headers", chainman.m_best_header ? chainman.m_best_header->nHeight : -1); obj.pushKV("bestblockhash", tip.GetBlockHash().GetHex()); - obj.pushKV("difficulty", GetDifficulty(&tip)); + obj.pushKV("difficulty", GetDifficulty(tip)); obj.pushKV("time", tip.GetBlockTime()); obj.pushKV("mediantime", tip.GetMedianTimePast()); obj.pushKV("verificationprogress", GuessVerificationProgress(chainman.GetParams().TxData(), &tip)); @@ -1815,8 +1813,8 @@ static RPCHelpMan getblockstats() } } - const CBlock& block = GetBlockChecked(chainman.m_blockman, &pindex); - const CBlockUndo& blockUndo = GetUndoChecked(chainman.m_blockman, &pindex); + const CBlock& block = GetBlockChecked(chainman.m_blockman, pindex); + const CBlockUndo& blockUndo = GetUndoChecked(chainman.m_blockman, pindex); const bool do_all = stats.size() == 0; // Calculate everything if nothing selected (default) const bool do_mediantxsize = do_all || stats.count("mediantxsize") != 0; @@ -2275,8 +2273,8 @@ public: static bool CheckBlockFilterMatches(BlockManager& blockman, const CBlockIndex& blockindex, const GCSFilter::ElementSet& needles) { - const CBlock block{GetBlockChecked(blockman, &blockindex)}; - const CBlockUndo block_undo{GetUndoChecked(blockman, &blockindex)}; + const CBlock block{GetBlockChecked(blockman, blockindex)}; + const CBlockUndo block_undo{GetUndoChecked(blockman, blockindex)}; // Check if any of the outputs match the scriptPubKey for (const auto& tx : block.vtx) { @@ -2845,7 +2843,7 @@ return RPCHelpMan{ data.pushKV("blocks", (int)chain.Height()); data.pushKV("bestblockhash", tip->GetBlockHash().GetHex()); - data.pushKV("difficulty", (double)GetDifficulty(tip)); + data.pushKV("difficulty", GetDifficulty(*tip)); data.pushKV("verificationprogress", GuessVerificationProgress(Params().TxData(), tip)); data.pushKV("coins_db_cache_bytes", cs.m_coinsdb_cache_size_bytes); data.pushKV("coins_tip_cache_bytes", cs.m_coinstip_cache_size_bytes); diff --git a/src/rpc/blockchain.h b/src/rpc/blockchain.h index 0a86085db0..c2021c3608 100644 --- a/src/rpc/blockchain.h +++ b/src/rpc/blockchain.h @@ -32,16 +32,16 @@ static constexpr int NUM_GETBLOCKSTATS_PERCENTILES = 5; * @return A floating point number that is a multiple of the main net minimum * difficulty (4295032833 hashes). */ -double GetDifficulty(const CBlockIndex* blockindex); +double GetDifficulty(const CBlockIndex& blockindex); /** Callback for when block tip changed. */ void RPCNotifyBlockChange(const CBlockIndex*); /** Block description to JSON */ -UniValue blockToJSON(node::BlockManager& blockman, const CBlock& block, const CBlockIndex* tip, const CBlockIndex* blockindex, TxVerbosity verbosity) LOCKS_EXCLUDED(cs_main); +UniValue blockToJSON(node::BlockManager& blockman, const CBlock& block, const CBlockIndex& tip, const CBlockIndex& blockindex, TxVerbosity verbosity) LOCKS_EXCLUDED(cs_main); /** Block header to JSON */ -UniValue blockheaderToJSON(const CBlockIndex* tip, const CBlockIndex* blockindex) LOCKS_EXCLUDED(cs_main); +UniValue blockheaderToJSON(const CBlockIndex& tip, const CBlockIndex& blockindex) LOCKS_EXCLUDED(cs_main); /** Used by getblockstats to get feerates at different percentiles by weight */ void CalculatePercentilesByWeight(CAmount result[NUM_GETBLOCKSTATS_PERCENTILES], std::vector>& scores, int64_t total_weight); diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 3d80656507..b2332c5fdc 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -441,7 +441,7 @@ static RPCHelpMan getmininginfo() obj.pushKV("blocks", active_chain.Height()); if (BlockAssembler::m_last_block_weight) obj.pushKV("currentblockweight", *BlockAssembler::m_last_block_weight); if (BlockAssembler::m_last_block_num_txs) obj.pushKV("currentblocktx", *BlockAssembler::m_last_block_num_txs); - obj.pushKV("difficulty", (double)GetDifficulty(active_chain.Tip())); + obj.pushKV("difficulty", GetDifficulty(*CHECK_NONFATAL(active_chain.Tip()))); obj.pushKV("networkhashps", getnetworkhashps().HandleRequest(request)); obj.pushKV("pooledtx", (uint64_t)mempool.size()); obj.pushKV("chain", chainman.GetParams().GetChainTypeString()); diff --git a/src/test/blockchain_tests.cpp b/src/test/blockchain_tests.cpp index b590467a43..be515a9eac 100644 --- a/src/test/blockchain_tests.cpp +++ b/src/test/blockchain_tests.cpp @@ -41,7 +41,7 @@ static void RejectDifficultyMismatch(double difficulty, double expected_difficul static void TestDifficulty(uint32_t nbits, double expected_difficulty) { CBlockIndex* block_index = CreateBlockIndexWithNbits(nbits); - double difficulty = GetDifficulty(block_index); + double difficulty = GetDifficulty(*block_index); delete block_index; RejectDifficultyMismatch(difficulty, expected_difficulty);