Merge bitcoin/bitcoin#26415: rpc,rest,zmq: faster getblock, NotifyBlock and rest_block by reading raw block

e710cef rest: read raw block in rest_block and deserialize for json (Andrew Toth)
95ce078 rpc: read raw block in getblock and deserialize for verbosity > 0 (Andrew Toth)
0865ab8 test: check more details on zmq raw block response (Andrew Toth)
38265cc zmq: read raw block with ReadRawBlockFromDisk (Andrew Toth)
da338aa blockstorage: check nPos in ReadRawBlockFromDisk before seeking back (Andrew Toth)

Pull request description:

  For the `getblock` endpoint with `verbosity=0`, the  `rest_block` REST endpoint for `bin` and `hex`, and zmq `NotifyBlock` we don't have to deserialize the block since we're just sending the raw data. This PR uses `ReadRawBlockFromDisk` instead of `ReadBlockFromDisk` to serve these requests, and only deserializes for `verbosity > 0` and `json` REST requests. See benchmarks in bitcoin/bitcoin#26684.

  Benchmarked using ApacheBench. Requesting block 750,000 in binary 10k times on a single core (set `-rest=1` in config):
  `ab -n 10000 -c 1 "http://127.0.0.1:8332/rest/block/0000000000000000000592a974b1b9f087cb77628bb4a097d5c2c11b3476a58e.bin"`

  On master, mean time 15ms.
  On this branch, mean time 1ms.

  For RPC
  ```
  echo '{"jsonrpc": "1.0", "id": "curltest", "method": "getblock", "params": ["0000000000000000000592a974b1b9f087cb77628bb4a097d5c2c11b3476a58e", 0]}' > /tmp/data.json
  ab -p /tmp/data.json -n 1000 -c 1 -A user:password "http://127.0.0.1:8332/"
  ```
  On master, mean time 32ms
  On this branch, mean time 13ms

ACKs for top commit:
  achow101:
    re-ACK e710cef

Tree-SHA512: 4cea13c7b563b2139d041b1fdcfdb793c8cc688654ae08db07e7ee6b875c5e582b8185db3ae603abbfb06d2164724f29205774620b48c493726b991999af289e

Author: achow101

src/init.cpp

@@ -1454,9 +1454,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<uint8_t>& 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) {

src/node/blockstorage.cpp

@@ -1088,6 +1088,12 @@ bool BlockManager::ReadBlockFromDisk(CBlock& block, const CBlockIndex& index) co
 bool BlockManager::ReadRawBlockFromDisk(std::vector<uint8_t>& 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()) {

src/rest.cpp

@@ -13,6 +13,7 @@
 #include <chain.h>
 #include <chainparams.h>
 #include <core_io.h>
+#include <flatfile.h>
 #include <httpserver.h>
 #include <index/blockfilterindex.h>
 #include <index/txindex.h>
@@ -34,7 +35,7 @@
 #include <validation.h>
 
 #include <any>
-#include <string>
+#include <vector>
 
 #include <univalue.h>
 
@@ -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<uint8_t> 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");

src/rpc/blockchain.cpp

@@ -17,6 +17,7 @@
 #include <core_io.h>
 #include <deploymentinfo.h>
 #include <deploymentstatus.h>
+#include <flatfile.h>
 #include <hash.h>
 #include <index/blockfilterindex.h>
 #include <index/coinstatsindex.h>
@@ -595,6 +596,28 @@ static CBlock GetBlockChecked(BlockManager& blockman, const CBlockIndex& blockin
     return block;
 }
 
+static std::vector<uint8_t> GetRawBlockChecked(BlockManager& blockman, const CBlockIndex& blockindex)
+{
+    std::vector<uint8_t> 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<uint8_t> 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;

src/zmq/zmqnotificationinterface.cpp

@@ -41,7 +41,7 @@ std::list<const CZMQAbstractNotifier*> CZMQNotificationInterface::GetActiveNotif
     return result;
 }
 
-std::unique_ptr<CZMQNotificationInterface> CZMQNotificationInterface::Create(std::function<bool(CBlock&, const CBlockIndex&)> get_block_by_index)
+std::unique_ptr<CZMQNotificationInterface> CZMQNotificationInterface::Create(std::function<bool(std::vector<uint8_t>&, const CBlockIndex&)> get_block_by_index)
 {
     std::map<std::string, CZMQNotifierFactory> factories;
     factories["pubhashblock"] = CZMQAbstractNotifier::Create<CZMQPublishHashBlockNotifier>;

src/zmq/zmqnotificationinterface.h

@@ -12,6 +12,7 @@
 #include <functional>
 #include <list>
 #include <memory>
+#include <vector>
 
 class CBlock;
 class CBlockIndex;
@@ -25,7 +26,7 @@ class CZMQNotificationInterface final : public CValidationInterface
 
     std::list<const CZMQAbstractNotifier*> GetActiveNotifiers() const;
 
-    static std::unique_ptr<CZMQNotificationInterface> Create(std::function<bool(CBlock&, const CBlockIndex&)> get_block_by_index);
+    static std::unique_ptr<CZMQNotificationInterface> Create(std::function<bool(std::vector<uint8_t>&, const CBlockIndex&)> get_block_by_index);
 
 protected:
     bool Initialize();

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<uint8_t> 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)

src/zmq/zmqpublishnotifier.h

@@ -10,8 +10,8 @@
 #include <cstddef>
 #include <cstdint>
 #include <functional>
+#include <vector>
 
-class CBlock;
 class CBlockIndex;
 class CTransaction;
 
@@ -49,10 +49,10 @@ class CZMQPublishHashTransactionNotifier : public CZMQAbstractPublishNotifier
 class CZMQPublishRawBlockNotifier : public CZMQAbstractPublishNotifier
 {
 private:
-    const std::function<bool(CBlock&, const CBlockIndex&)> m_get_block_by_index;
+    const std::function<bool(std::vector<uint8_t>&, const CBlockIndex&)> m_get_block_by_index;
 
 public:
-    CZMQPublishRawBlockNotifier(std::function<bool(CBlock&, const CBlockIndex&)> get_block_by_index)
+    CZMQPublishRawBlockNotifier(std::function<bool(std::vector<uint8_t>&, const CBlockIndex&)> get_block_by_index)
         : m_get_block_by_index{std::move(get_block_by_index)} {}
     bool NotifyBlock(const CBlockIndex *pindex) override;
 };

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,
 )
@@ -201,8 +203,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()