Merge bitcoin/bitcoin#23438: refactor: Use spans of std::byte in serialize

fa5d2e6 Remove unused char serialize (MarcoFalke)
fa24493 Use spans of std::byte in serialize (MarcoFalke)
fa65bbf span: Add BytePtr helper (MarcoFalke)

Pull request description:

  This changes the serialize code (`.read()` and `.write()` functions) to take a `Span` instead of a pointer and size. This is a breaking change for the serialize interface, so at no additional cost we can also switch to `std::byte` (instead of using `char`).

  The benefits of using `Span`:
  * Less verbose and less fragile code when passing an already existing `Span`(-like) object to or from serialization

  The benefits of using `std::byte`:
  * `std::byte` can't accidentally be mistaken for an integer

  The goal here is to only change serialize to use spans of `std::byte`. If needed, `AsBytes`,  `MakeUCharSpan`, ... can be used (temporarily) to pass spans of the right type.

  Other changes that are included here:

  * [#22167](bitcoin/bitcoin#22167) (refactor: Remove char serialize by MarcoFalke)
  * [#21906](bitcoin/bitcoin#21906) (Preserve const in cast on CTransactionSignatureSerializer by promag)

ACKs for top commit:
  laanwj:
    Concept and code review ACK fa5d2e6
  sipa:
    re-utACK fa5d2e6

Tree-SHA512: 08ee9eced5fb777cedae593b11e33660bed9a3e1711a7451a87b835089a96c99ce0632918bb4666a4e859c4d020f88fb50f2dd734216b0c3d1a9a704967ece6f

Author: laanwj

src/bench/checkblock.cpp

@@ -17,8 +17,8 @@
 static void DeserializeBlockTest(benchmark::Bench& bench)
 {
     CDataStream stream(benchmark::data::block413567, SER_NETWORK, PROTOCOL_VERSION);
-    char a = '\0';
-    stream.write(&a, 1); // Prevent compaction
+    std::byte a{0};
+    stream.write({&a, 1}); // Prevent compaction
 
     bench.unit("block").run([&] {
         CBlock block;
@@ -31,8 +31,8 @@ static void DeserializeBlockTest(benchmark::Bench& bench)
 static void DeserializeAndCheckBlockTest(benchmark::Bench& bench)
 {
     CDataStream stream(benchmark::data::block413567, SER_NETWORK, PROTOCOL_VERSION);
-    char a = '\0';
-    stream.write(&a, 1); // Prevent compaction
+    std::byte a{0};
+    stream.write({&a, 1}); // Prevent compaction
 
     ArgsManager bench_args;
     const auto chainParams = CreateChainParams(bench_args, CBaseChainParams::MAIN);

src/bench/rpc_blockchain.cpp

@@ -23,8 +23,8 @@ struct TestBlockAndIndex {
     TestBlockAndIndex()
     {
         CDataStream stream(benchmark::data::block413567, SER_NETWORK, PROTOCOL_VERSION);
-        char a = '\0';
-        stream.write(&a, 1); // Prevent compaction
+        std::byte a{0};
+        stream.write({&a, 1}); // Prevent compaction
 
         stream >> block;
 

src/common/bloom.cpp

@@ -62,7 +62,7 @@ void CBloomFilter::insert(const COutPoint& outpoint)
 {
     CDataStream stream(SER_NETWORK, PROTOCOL_VERSION);
     stream << outpoint;
-    insert(stream);
+    insert(MakeUCharSpan(stream));
 }
 
 bool CBloomFilter::contains(Span<const unsigned char> vKey) const
@@ -83,7 +83,7 @@ bool CBloomFilter::contains(const COutPoint& outpoint) const
 {
     CDataStream stream(SER_NETWORK, PROTOCOL_VERSION);
     stream << outpoint;
-    return contains(stream);
+    return contains(MakeUCharSpan(stream));
 }
 
 bool CBloomFilter::IsWithinSizeConstraints() const

src/dbwrapper.h

@@ -147,7 +147,7 @@ class CDBIterator
     template<typename K> bool GetKey(K& key) {
         leveldb::Slice slKey = piter->key();
         try {
-            CDataStream ssKey(MakeUCharSpan(slKey), SER_DISK, CLIENT_VERSION);
+            CDataStream ssKey{MakeByteSpan(slKey), SER_DISK, CLIENT_VERSION};
             ssKey >> key;
         } catch (const std::exception&) {
             return false;
@@ -158,7 +158,7 @@ class CDBIterator
     template<typename V> bool GetValue(V& value) {
         leveldb::Slice slValue = piter->value();
         try {
-            CDataStream ssValue(MakeUCharSpan(slValue), SER_DISK, CLIENT_VERSION);
+            CDataStream ssValue{MakeByteSpan(slValue), SER_DISK, CLIENT_VERSION};
             ssValue.Xor(dbwrapper_private::GetObfuscateKey(parent));
             ssValue >> value;
         } catch (const std::exception&) {
@@ -244,7 +244,7 @@ class CDBWrapper
             dbwrapper_private::HandleError(status);
         }
         try {
-            CDataStream ssValue(MakeUCharSpan(strValue), SER_DISK, CLIENT_VERSION);
+            CDataStream ssValue{MakeByteSpan(strValue), SER_DISK, CLIENT_VERSION};
             ssValue.Xor(obfuscate_key);
             ssValue >> value;
         } catch (const std::exception&) {

src/hash.h

@@ -111,8 +111,9 @@ class CHashWriter
     int GetType() const { return nType; }
     int GetVersion() const { return nVersion; }
 
-    void write(const char *pch, size_t size) {
-        ctx.Write((const unsigned char*)pch, size);
+    void write(Span<const std::byte> src)
+    {
+        ctx.Write(UCharCast(src.data()), src.size());
     }
 
     /** Compute the double-SHA256 hash of all data written to this object.
@@ -162,18 +163,18 @@ class CHashVerifier : public CHashWriter
 public:
     explicit CHashVerifier(Source* source_) : CHashWriter(source_->GetType(), source_->GetVersion()), source(source_) {}
 
-    void read(char* pch, size_t nSize)
+    void read(Span<std::byte> dst)
     {
-        source->read(pch, nSize);
-        this->write(pch, nSize);
+        source->read(dst);
+        this->write(dst);
     }
 
     void ignore(size_t nSize)
     {
-        char data[1024];
+        std::byte data[1024];
         while (nSize > 0) {
             size_t now = std::min<size_t>(nSize, 1024);
-            read(data, now);
+            read({data, now});
             nSize -= now;
         }
     }

src/net.cpp

@@ -3091,11 +3091,11 @@ void CaptureMessage(const CAddress& addr, const std::string& msg_type, const Spa
     CAutoFile f(fsbridge::fopen(path, "ab"), SER_DISK, CLIENT_VERSION);
 
     ser_writedata64(f, now.count());
-    f.write(msg_type.data(), msg_type.length());
+    f.write(MakeByteSpan(msg_type));
     for (auto i = msg_type.length(); i < CMessageHeader::COMMAND_SIZE; ++i) {
         f << uint8_t{'\0'};
     }
     uint32_t size = data.size();
     ser_writedata32(f, size);
-    f.write((const char*)data.data(), data.size());
+    f.write(AsBytes(data));
 }

src/node/blockstorage.cpp

@@ -813,7 +813,7 @@ bool ReadRawBlockFromDisk(std::vector<uint8_t>& block, const FlatFilePos& pos, c
         }
 
         block.resize(blk_size); // Zeroing of memory is intentional here
-        filein.read((char*)block.data(), blk_size);
+        filein.read(MakeWritableByteSpan(block));
     } catch (const std::exception& e) {
         return error("%s: Read from block file failed: %s for %s", __func__, e.what(), pos.ToString());
     }

src/psbt.cpp

@@ -399,7 +399,7 @@ bool DecodeBase64PSBT(PartiallySignedTransaction& psbt, const std::string& base6
 
 bool DecodeRawPSBT(PartiallySignedTransaction& psbt, const std::string& tx_data, std::string& error)
 {
-    CDataStream ss_data(MakeUCharSpan(tx_data), SER_NETWORK, PROTOCOL_VERSION);
+    CDataStream ss_data(MakeByteSpan(tx_data), SER_NETWORK, PROTOCOL_VERSION);
     try {
         ss_data >> psbt;
         if (!ss_data.empty()) {

src/pubkey.h

@@ -142,14 +142,14 @@ class CPubKey
     {
         unsigned int len = size();
         ::WriteCompactSize(s, len);
-        s.write((char*)vch, len);
+        s.write(AsBytes(Span{vch, len}));
     }
     template <typename Stream>
     void Unserialize(Stream& s)
     {
         const unsigned int len(::ReadCompactSize(s));
         if (len <= SIZE) {
-            s.read((char*)vch, len);
+            s.read(AsWritableBytes(Span{vch, len}));
             if (len != size()) {
                 Invalidate();
             }

src/script/bitcoinconsensus.cpp

@@ -22,20 +22,23 @@ class TxInputStream
     m_remaining(txToLen)
     {}
 
-    void read(char* pch, size_t nSize)
+    void read(Span<std::byte> dst)
     {
-        if (nSize > m_remaining)
+        if (dst.size() > m_remaining) {
             throw std::ios_base::failure(std::string(__func__) + ": end of data");
+        }
 
-        if (pch == nullptr)
+        if (dst.data() == nullptr) {
             throw std::ios_base::failure(std::string(__func__) + ": bad destination buffer");
+        }
 
-        if (m_data == nullptr)
+        if (m_data == nullptr) {
             throw std::ios_base::failure(std::string(__func__) + ": bad source buffer");
+        }
 
-        memcpy(pch, m_data, nSize);
-        m_remaining -= nSize;
-        m_data += nSize;
+        memcpy(dst.data(), m_data, dst.size());
+        m_remaining -= dst.size();
+        m_data += dst.size();
     }
 
     template<typename T>