From da338aada7943c392013c36c542af621fbc6edd1 Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Tue, 12 Mar 2024 12:46:07 -0400 Subject: [PATCH 1/5] blockstorage: check nPos in ReadRawBlockFromDisk before seeking back ReadRawBlockFromDisk assumes a non-null pos that has an nPos >= 8. This simple check makes the function safer to call in the future, so callers don't need to worry about causing UB if the pos is null. --- src/node/blockstorage.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index 211d557826367..d0ddfeb0f1e47 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -1088,6 +1088,12 @@ bool BlockManager::ReadBlockFromDisk(CBlock& block, const CBlockIndex& index) co bool BlockManager::ReadRawBlockFromDisk(std::vector& block, const FlatFilePos& pos) const { FlatFilePos hpos = pos; + // If nPos is less than 8 the pos is null and we don't have the block data + // Return early to prevent undefined behavior of unsigned int underflow + if (hpos.nPos < 8) { + LogError("%s: OpenBlockFile failed for %s\n", __func__, pos.ToString()); + return false; + } hpos.nPos -= 8; // Seek back 8 bytes for meta header AutoFile filein{OpenBlockFile(hpos, true)}; if (filein.IsNull()) { From 38265cc14e7d646bf27882329d374d42167eb49f Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Tue, 12 Mar 2024 12:46:46 -0400 Subject: [PATCH 2/5] zmq: read raw block with ReadRawBlockFromDisk --- src/init.cpp | 4 ++-- src/zmq/zmqnotificationinterface.cpp | 2 +- src/zmq/zmqnotificationinterface.h | 3 ++- src/zmq/zmqpublishnotifier.cpp | 7 ++----- src/zmq/zmqpublishnotifier.h | 6 +++--- 5 files changed, 10 insertions(+), 12 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index b9a0bb732aa04..635fbd7759886 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1452,9 +1452,9 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) #if ENABLE_ZMQ g_zmq_notification_interface = CZMQNotificationInterface::Create( - [&chainman = node.chainman](CBlock& block, const CBlockIndex& index) { + [&chainman = node.chainman](std::vector& block, const CBlockIndex& index) { assert(chainman); - return chainman->m_blockman.ReadBlockFromDisk(block, index); + return chainman->m_blockman.ReadRawBlockFromDisk(block, WITH_LOCK(cs_main, return index.GetBlockPos())); }); if (g_zmq_notification_interface) { diff --git a/src/zmq/zmqnotificationinterface.cpp b/src/zmq/zmqnotificationinterface.cpp index 63c27377061eb..d10db046f5274 100644 --- a/src/zmq/zmqnotificationinterface.cpp +++ b/src/zmq/zmqnotificationinterface.cpp @@ -41,7 +41,7 @@ std::list CZMQNotificationInterface::GetActiveNotif return result; } -std::unique_ptr CZMQNotificationInterface::Create(std::function get_block_by_index) +std::unique_ptr CZMQNotificationInterface::Create(std::function&, const CBlockIndex&)> get_block_by_index) { std::map factories; factories["pubhashblock"] = CZMQAbstractNotifier::Create; diff --git a/src/zmq/zmqnotificationinterface.h b/src/zmq/zmqnotificationinterface.h index 45d0982bd3e1f..c879fdd0ddfed 100644 --- a/src/zmq/zmqnotificationinterface.h +++ b/src/zmq/zmqnotificationinterface.h @@ -12,6 +12,7 @@ #include #include #include +#include class CBlock; class CBlockIndex; @@ -25,7 +26,7 @@ class CZMQNotificationInterface final : public CValidationInterface std::list GetActiveNotifiers() const; - static std::unique_ptr Create(std::function get_block_by_index); + static std::unique_ptr Create(std::function&, const CBlockIndex&)> get_block_by_index); protected: bool Initialize(); diff --git a/src/zmq/zmqpublishnotifier.cpp b/src/zmq/zmqpublishnotifier.cpp index 0f20706364d2b..608870c48985f 100644 --- a/src/zmq/zmqpublishnotifier.cpp +++ b/src/zmq/zmqpublishnotifier.cpp @@ -243,16 +243,13 @@ bool CZMQPublishRawBlockNotifier::NotifyBlock(const CBlockIndex *pindex) { LogPrint(BCLog::ZMQ, "Publish rawblock %s to %s\n", pindex->GetBlockHash().GetHex(), this->address); - DataStream ss; - CBlock block; + std::vector block{}; if (!m_get_block_by_index(block, *pindex)) { zmqError("Can't read block from disk"); return false; } - ss << TX_WITH_WITNESS(block); - - return SendZmqMessage(MSG_RAWBLOCK, &(*ss.begin()), ss.size()); + return SendZmqMessage(MSG_RAWBLOCK, block.data(), block.size()); } bool CZMQPublishRawTransactionNotifier::NotifyTransaction(const CTransaction &transaction) diff --git a/src/zmq/zmqpublishnotifier.h b/src/zmq/zmqpublishnotifier.h index a5cd4337615f3..cc941a899c5ea 100644 --- a/src/zmq/zmqpublishnotifier.h +++ b/src/zmq/zmqpublishnotifier.h @@ -10,8 +10,8 @@ #include #include #include +#include -class CBlock; class CBlockIndex; class CTransaction; @@ -49,10 +49,10 @@ class CZMQPublishHashTransactionNotifier : public CZMQAbstractPublishNotifier class CZMQPublishRawBlockNotifier : public CZMQAbstractPublishNotifier { private: - const std::function m_get_block_by_index; + const std::function&, const CBlockIndex&)> m_get_block_by_index; public: - CZMQPublishRawBlockNotifier(std::function get_block_by_index) + CZMQPublishRawBlockNotifier(std::function&, const CBlockIndex&)> get_block_by_index) : m_get_block_by_index{std::move(get_block_by_index)} {} bool NotifyBlock(const CBlockIndex *pindex) override; }; From 0865ab8712429761bc69f09d93760f8c6081c99c Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Tue, 12 Mar 2024 12:47:01 -0400 Subject: [PATCH 3/5] test: check more details on zmq raw block response --- test/functional/interface_zmq.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/test/functional/interface_zmq.py b/test/functional/interface_zmq.py index 2358dd4387358..3297f6a8823cd 100755 --- a/test/functional/interface_zmq.py +++ b/test/functional/interface_zmq.py @@ -5,6 +5,7 @@ """Test the ZMQ notification interface.""" import struct from time import sleep +from io import BytesIO from test_framework.address import ( ADDRESS_BCRT1_P2WSH_OP_TRUE, @@ -17,6 +18,7 @@ ) from test_framework.test_framework import BitcoinTestFramework from test_framework.messages import ( + CBlock, hash256, tx_from_hex, ) @@ -203,8 +205,13 @@ def test_basic(self): assert_equal(tx.hash, txid.hex()) # Should receive the generated raw block. - block = rawblock.receive() - assert_equal(genhashes[x], hash256_reversed(block[:80]).hex()) + hex = rawblock.receive() + block = CBlock() + block.deserialize(BytesIO(hex)) + assert block.is_valid() + assert_equal(block.vtx[0].hash, tx.hash) + assert_equal(len(block.vtx), 1) + assert_equal(genhashes[x], hash256_reversed(hex[:80]).hex()) # Should receive the generated block hash. hash = hashblock.receive().hex() From 95ce0783a6dab325038a64d6c529c9e7816e3072 Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Tue, 12 Mar 2024 12:47:17 -0400 Subject: [PATCH 4/5] rpc: read raw block in getblock and deserialize for verbosity > 0 Note that for speed this commit also removes the proof of work and signet signature checks before returning the block in getblock. It is assumed if a block is stored it will be valid. --- src/rpc/blockchain.cpp | 34 +++++++++++++++++++++++++++++----- 1 file changed, 29 insertions(+), 5 deletions(-) diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index dfdddeacead5f..a1135c27d4a48 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include @@ -595,6 +596,28 @@ static CBlock GetBlockChecked(BlockManager& blockman, const CBlockIndex& blockin return block; } +static std::vector GetRawBlockChecked(BlockManager& blockman, const CBlockIndex& blockindex) +{ + std::vector data{}; + FlatFilePos pos{}; + { + LOCK(cs_main); + if (blockman.IsBlockPruned(blockindex)) { + throw JSONRPCError(RPC_MISC_ERROR, "Block not available (pruned data)"); + } + pos = blockindex.GetBlockPos(); + } + + if (!blockman.ReadRawBlockFromDisk(data, pos)) { + // 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. + throw JSONRPCError(RPC_MISC_ERROR, "Block not found on disk"); + } + + return data; +} + static CBlockUndo GetUndoChecked(BlockManager& blockman, const CBlockIndex& blockindex) { CBlockUndo blockUndo; @@ -735,15 +758,16 @@ static RPCHelpMan getblock() } } - const CBlock block{GetBlockChecked(chainman.m_blockman, *pblockindex)}; + const std::vector block_data{GetRawBlockChecked(chainman.m_blockman, *pblockindex)}; if (verbosity <= 0) { - DataStream ssBlock; - ssBlock << TX_WITH_WITNESS(block); - std::string strHex = HexStr(ssBlock); - return strHex; + return HexStr(block_data); } + DataStream block_stream{block_data}; + CBlock block{}; + block_stream >> TX_WITH_WITNESS(block); + TxVerbosity tx_verbosity; if (verbosity == 1) { tx_verbosity = TxVerbosity::SHOW_TXID; From e710cefd5701cd33d1e55034b3e37cea78582733 Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Tue, 12 Mar 2024 12:48:04 -0400 Subject: [PATCH 5/5] rest: read raw block in rest_block and deserialize for json Note that for speed this commit also removes the proof of work and signet signature checks before returning the block in getblock. It is assumed if a block is stored it will be valid. --- src/rest.cpp | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/src/rest.cpp b/src/rest.cpp index 91184745c888d..89c033b8a31d4 100644 --- a/src/rest.cpp +++ b/src/rest.cpp @@ -13,6 +13,7 @@ #include #include #include +#include #include #include #include @@ -34,7 +35,7 @@ #include #include -#include +#include #include @@ -295,7 +296,7 @@ static bool rest_block(const std::any& context, if (!ParseHashStr(hashStr, hash)) return RESTERR(req, HTTP_BAD_REQUEST, "Invalid hash: " + hashStr); - CBlock block; + FlatFilePos pos{}; const CBlockIndex* pblockindex = nullptr; const CBlockIndex* tip = nullptr; ChainstateManager* maybe_chainman = GetChainman(context, req); @@ -311,32 +312,33 @@ static bool rest_block(const std::any& context, if (chainman.m_blockman.IsBlockPruned(*pblockindex)) { return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not available (pruned data)"); } + pos = pblockindex->GetBlockPos(); } - if (!chainman.m_blockman.ReadBlockFromDisk(block, *pblockindex)) { + std::vector block_data{}; + if (!chainman.m_blockman.ReadRawBlockFromDisk(block_data, pos)) { return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not found"); } switch (rf) { case RESTResponseFormat::BINARY: { - DataStream ssBlock; - ssBlock << TX_WITH_WITNESS(block); - std::string binaryBlock = ssBlock.str(); + const std::string binaryBlock{block_data.begin(), block_data.end()}; req->WriteHeader("Content-Type", "application/octet-stream"); req->WriteReply(HTTP_OK, binaryBlock); return true; } case RESTResponseFormat::HEX: { - DataStream ssBlock; - ssBlock << TX_WITH_WITNESS(block); - std::string strHex = HexStr(ssBlock) + "\n"; + const std::string strHex{HexStr(block_data) + "\n"}; req->WriteHeader("Content-Type", "text/plain"); req->WriteReply(HTTP_OK, strHex); return true; } case RESTResponseFormat::JSON: { + CBlock block{}; + DataStream block_stream{block_data}; + block_stream >> TX_WITH_WITNESS(block); UniValue objBlock = blockToJSON(chainman.m_blockman, block, *tip, *pblockindex, tx_verbosity); std::string strJSON = objBlock.write() + "\n"; req->WriteHeader("Content-Type", "application/json");