Merge bitcoin/bitcoin#32743: refactor: use std::vector<std::byte> for BlockManager::ReadRawBlock()

6ecb9fc chore: use `std::vector<std::byte>` for `BlockManager::ReadRawBlock()` (Roman Zeyde)

Pull request description:

  Following [this comment](bitcoin/bitcoin#32540 (comment)), this PR changes `BlockManager::ReadRawBlock()` to accept a `std::vector<std::byte>` instead of `std::vector<uint8_t>`, in order to avoid casts during its invocations.

  It also adds a new `SpanReader` constructor to allow reading from a span of `std::byte`s (in addition to span of `uint8_t`).

ACKs for top commit:
  l0rinc:
    ACK 6ecb9fc
  maflcko:
    re-ACK 6ecb9fc
  TheCharlatan:
    Re-ACK 6ecb9fc

Tree-SHA512: b0976c34b8da4fa1e6d805a89de2883f48ba431a71069e8c1ae450f48e425cc41aff1a5d479a7d40312a972aaf1f92e9478a985a14a1357c6b3e564e988d03e5

Author: fanquake

src/bench/readwriteblock.cpp

@@ -55,7 +55,7 @@ static void ReadRawBlockBench(benchmark::Bench& bench)
     const auto testing_setup{MakeNoLogFileContext<const TestingSetup>(ChainType::MAIN)};
     auto& blockman{testing_setup->m_node.chainman->m_blockman};
     const auto pos{blockman.WriteBlock(CreateTestBlock(), 413'567)};
-    std::vector<uint8_t> block_data;
+    std::vector<std::byte> block_data;
     blockman.ReadRawBlock(block_data, pos); // warmup
     bench.run([&] {
         const auto success{blockman.ReadRawBlock(block_data, pos)};

src/init.cpp

@@ -1677,7 +1677,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
 
 #ifdef ENABLE_ZMQ
     g_zmq_notification_interface = CZMQNotificationInterface::Create(
-        [&chainman = node.chainman](std::vector<uint8_t>& block, const CBlockIndex& index) {
+        [&chainman = node.chainman](std::vector<std::byte>& block, const CBlockIndex& index) {
             assert(chainman);
             return chainman->m_blockman.ReadRawBlock(block, WITH_LOCK(cs_main, return index.GetBlockPos()));
         });

src/net_processing.cpp

@@ -2303,7 +2303,7 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv&
     } else if (inv.IsMsgWitnessBlk()) {
         // 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;
+        std::vector<std::byte> block_data;
         if (!m_chainman.m_blockman.ReadRawBlock(block_data, block_pos)) {
             if (WITH_LOCK(m_chainman.GetMutex(), return m_chainman.m_blockman.IsBlockPruned(*pindex))) {
                 LogDebug(BCLog::NET, "Block was pruned before it could be read, %s\n", pfrom.DisconnectMsg(fLogIPs));

src/node/blockstorage.cpp

@@ -995,7 +995,7 @@ bool BlockManager::ReadBlock(CBlock& block, const FlatFilePos& pos, const std::o
     block.SetNull();
 
     // Open history file to read
-    std::vector<uint8_t> block_data;
+    std::vector<std::byte> block_data;
     if (!ReadRawBlock(block_data, pos)) {
         return false;
     }
@@ -1037,7 +1037,7 @@ bool BlockManager::ReadBlock(CBlock& block, const CBlockIndex& index) const
     return ReadBlock(block, block_pos, index.GetBlockHash());
 }
 
-bool BlockManager::ReadRawBlock(std::vector<uint8_t>& block, const FlatFilePos& pos) const
+bool BlockManager::ReadRawBlock(std::vector<std::byte>& block, const FlatFilePos& pos) const
 {
     if (pos.nPos < STORAGE_HEADER_BYTES) {
         // If nPos is less than STORAGE_HEADER_BYTES, we can't read the header that precedes the block data
@@ -1071,7 +1071,7 @@ bool BlockManager::ReadRawBlock(std::vector<uint8_t>& block, const FlatFilePos&
         }
 
         block.resize(blk_size); // Zeroing of memory is intentional here
-        filein.read(MakeWritableByteSpan(block));
+        filein.read(block);
     } catch (const std::exception& e) {
         LogError("Read from block file failed: %s for %s while reading raw block", e.what(), pos.ToString());
         return false;

src/node/blockstorage.h

@@ -413,7 +413,7 @@ class BlockManager
     /** Functions for disk access for blocks */
     bool ReadBlock(CBlock& block, const FlatFilePos& pos, const std::optional<uint256>& expected_hash = {}) const;
     bool ReadBlock(CBlock& block, const CBlockIndex& index) const;
-    bool ReadRawBlock(std::vector<uint8_t>& block, const FlatFilePos& pos) const;
+    bool ReadRawBlock(std::vector<std::byte>& block, const FlatFilePos& pos) const;
 
     bool ReadBlockUndo(CBlockUndo& blockundo, const CBlockIndex& index) const;
 

src/rest.cpp

@@ -318,15 +318,15 @@ static bool rest_block(const std::any& context,
         pos = pblockindex->GetBlockPos();
     }
 
-    std::vector<uint8_t> block_data{};
+    std::vector<std::byte> block_data{};
     if (!chainman.m_blockman.ReadRawBlock(block_data, pos)) {
         return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not found");
     }
 
     switch (rf) {
     case RESTResponseFormat::BINARY: {
         req->WriteHeader("Content-Type", "application/octet-stream");
-        req->WriteReply(HTTP_OK, std::as_bytes(std::span{block_data}));
+        req->WriteReply(HTTP_OK, block_data);
         return true;
     }
 

src/rpc/blockchain.cpp

@@ -660,9 +660,9 @@ static CBlock GetBlockChecked(BlockManager& blockman, const CBlockIndex& blockin
     return block;
 }
 
-static std::vector<uint8_t> GetRawBlockChecked(BlockManager& blockman, const CBlockIndex& blockindex)
+static std::vector<std::byte> GetRawBlockChecked(BlockManager& blockman, const CBlockIndex& blockindex)
 {
-    std::vector<uint8_t> data{};
+    std::vector<std::byte> data{};
     FlatFilePos pos{};
     {
         LOCK(cs_main);
@@ -812,7 +812,7 @@ static RPCHelpMan getblock()
         }
     }
 
-    const std::vector<uint8_t> block_data{GetRawBlockChecked(chainman.m_blockman, *pblockindex)};
+    const std::vector<std::byte> block_data{GetRawBlockChecked(chainman.m_blockman, *pblockindex)};
 
     if (verbosity <= 0) {
         return HexStr(block_data);

src/streams.h

@@ -100,13 +100,14 @@ class VectorWriter
 class SpanReader
 {
 private:
-    std::span<const unsigned char> m_data;
+    std::span<const std::byte> m_data;
 
 public:
     /**
      * @param[in]  data Referenced byte vector to overwrite/append
      */
-    explicit SpanReader(std::span<const unsigned char> data) : m_data{data} {}
+    explicit SpanReader(std::span<const unsigned char> data) : m_data{std::as_bytes(data)} {}
+    explicit SpanReader(std::span<const std::byte> data) : m_data{data} {}
 
     template<typename T>
     SpanReader& operator>>(T&& obj)

src/zmq/zmqnotificationinterface.cpp

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

src/zmq/zmqnotificationinterface.h

@@ -26,7 +26,7 @@ class CZMQNotificationInterface final : public CValidationInterface
 
     std::list<const CZMQAbstractNotifier*> GetActiveNotifiers() const;
 
-    static std::unique_ptr<CZMQNotificationInterface> Create(std::function<bool(std::vector<uint8_t>&, const CBlockIndex&)> get_block_by_index);
+    static std::unique_ptr<CZMQNotificationInterface> Create(std::function<bool(std::vector<std::byte>&, const CBlockIndex&)> get_block_by_index);
 
 protected:
     bool Initialize();