Merge bitcoin/bitcoin#23687: Remove unused (and broken) functionality in SpanReader

31ba1af74a Remove unused (and broken) functionality in SpanReader (Pieter Wuille)

Pull request description:

  This removes the ability to set an offset in the `SpanReader::SpanReader` constructor, as the current code is broken since #23653. All call sites use `pos=0`, so it is actually unused. If future call sites need it, `SpanReader{a, b, c, d}` is equivalent to `SpanReader{a, b, c.subspan(d)}`.

  It also removes the ability to deserialize from `SpanReader` directly from the constructor. This too is unused, and can be more idiomatically simulated using `(SpanReader{a, b, c} >> x >> y >> z)` instead of `SpanReader{a, b, c, x, y, z}`.

  This was pointed out by achow101 in https://github.com/bitcoin/bitcoin/pull/23653#discussion_r763370432.

ACKs for top commit:
  jb55:
    crACK 31ba1af74a
  achow101:
    ACK 31ba1af74a

Tree-SHA512: 700ebcd74147628488c39168dbf3a00f8ed41709a26711695f4bf036250a9b115574923bbf96040ec7b7fee4132d6dbbcb5c6e5a2977c4beb521dc1500e6ed53

src/blockfilter.cpp

@@ -81,7 +81,7 @@ GCSFilter::GCSFilter(const Params& params)
 GCSFilter::GCSFilter(const Params& params, std::vector<unsigned char> encoded_filter)
     : m_params(params), m_encoded(std::move(encoded_filter))
 {
-    SpanReader stream{GCS_SER_TYPE, GCS_SER_VERSION, m_encoded, 0};
+    SpanReader stream{GCS_SER_TYPE, GCS_SER_VERSION, m_encoded};
 
     uint64_t N = ReadCompactSize(stream);
     m_N = static_cast<uint32_t>(N);
@@ -133,7 +133,7 @@ GCSFilter::GCSFilter(const Params& params, const ElementSet& elements)
 
 bool GCSFilter::MatchInternal(const uint64_t* element_hashes, size_t size) const
 {
-    SpanReader stream{GCS_SER_TYPE, GCS_SER_VERSION, m_encoded, 0};
+    SpanReader stream{GCS_SER_TYPE, GCS_SER_VERSION, m_encoded};
 
     // Seek forward by size of N
     uint64_t N = ReadCompactSize(stream);

src/signet.cpp

@@ -98,7 +98,7 @@ std::optional<SignetTxs> SignetTxs::Create(const CBlock& block, const CScript& c
         // no signet solution -- allow this to support OP_TRUE as trivial block challenge
     } else {
         try {
-            SpanReader v{SER_NETWORK, INIT_PROTO_VERSION, signet_solution, 0};
+            SpanReader v{SER_NETWORK, INIT_PROTO_VERSION, signet_solution};
             v >> tx_spending.vin[0].scriptSig;
             v >> tx_spending.vin[0].scriptWitness.stack;
             if (!v.empty()) return std::nullopt; // extraneous data encountered

src/streams.h

@@ -143,28 +143,9 @@ public:
      * @param[in]  type Serialization Type
      * @param[in]  version Serialization Version (including any flags)
      * @param[in]  data Referenced byte vector to overwrite/append
-     * @param[in]  pos Starting position. Vector index where reads should start.
      */
-    SpanReader(int type, int version, Span<const unsigned char> data, size_t pos)
-        : m_type(type), m_version(version), m_data(data)
-    {
-        if (pos > m_data.size()) {
-            throw std::ios_base::failure("SpanReader(...): end of data (pos > m_data.size())");
-        }
-        data = data.subspan(pos);
-    }
-
-    /**
-     * (other params same as above)
-     * @param[in]  args  A list of items to deserialize starting at pos.
-     */
-    template <typename... Args>
-    SpanReader(int type, int version, Span<const unsigned char> data, size_t pos,
-                  Args&&... args)
-        : SpanReader(type, version, data, pos)
-    {
-        ::UnserializeMany(*this, std::forward<Args>(args)...);
-    }
+    SpanReader(int type, int version, Span<const unsigned char> data)
+        : m_type(type), m_version(version), m_data(data) {}
 
     template<typename T>
     SpanReader& operator>>(T&& obj)

src/test/fuzz/golomb_rice.cpp

@@ -82,7 +82,7 @@ FUZZ_TARGET(golomb_rice)
 
     std::vector<uint64_t> decoded_deltas;
     {
-        SpanReader stream{SER_NETWORK, 0, golomb_rice_data, 0};
+        SpanReader stream{SER_NETWORK, 0, golomb_rice_data};
         BitStreamReader<SpanReader> bitreader{stream};
         const uint32_t n = static_cast<uint32_t>(ReadCompactSize(stream));
         for (uint32_t i = 0; i < n; ++i) {
@@ -94,7 +94,7 @@ FUZZ_TARGET(golomb_rice)
 
     {
         const std::vector<uint8_t> random_bytes = ConsumeRandomLengthByteVector(fuzzed_data_provider, 1024);
-        SpanReader stream{SER_NETWORK, 0, random_bytes, 0};
+        SpanReader stream{SER_NETWORK, 0, random_bytes};
         uint32_t n;
         try {
             n = static_cast<uint32_t>(ReadCompactSize(stream));

src/test/fuzz/script_assets_test_minimizer.cpp

@@ -54,7 +54,7 @@ CMutableTransaction TxFromHex(const std::string& str)
 {
     CMutableTransaction tx;
     try {
-        SpanReader{SER_DISK, SERIALIZE_TRANSACTION_NO_WITNESS, CheckedParseHex(str), 0} >> tx;
+        SpanReader{SER_DISK, SERIALIZE_TRANSACTION_NO_WITNESS, CheckedParseHex(str)} >> tx;
     } catch (const std::ios_base::failure&) {
         throw std::runtime_error("Tx deserialization failure");
     }
@@ -68,7 +68,7 @@ std::vector<CTxOut> TxOutsFromJSON(const UniValue& univalue)
     for (size_t i = 0; i < univalue.size(); ++i) {
         CTxOut txout;
         try {
-            SpanReader{SER_DISK, 0, CheckedParseHex(univalue[i].get_str()), 0} >> txout;
+            SpanReader{SER_DISK, 0, CheckedParseHex(univalue[i].get_str())} >> txout;
         } catch (const std::ios_base::failure&) {
             throw std::runtime_error("Prevout invalid format");
         }

src/test/script_tests.cpp

@@ -1473,7 +1473,7 @@ BOOST_AUTO_TEST_CASE(script_HasValidOps)
 static CMutableTransaction TxFromHex(const std::string& str)
 {
     CMutableTransaction tx;
-    SpanReader{SER_DISK, SERIALIZE_TRANSACTION_NO_WITNESS, ParseHex(str), 0} >> tx;
+    SpanReader{SER_DISK, SERIALIZE_TRANSACTION_NO_WITNESS, ParseHex(str)} >> tx;
     return tx;
 }
 
@@ -1483,7 +1483,7 @@ static std::vector<CTxOut> TxOutsFromJSON(const UniValue& univalue)
     std::vector<CTxOut> prevouts;
     for (size_t i = 0; i < univalue.size(); ++i) {
         CTxOut txout;
-        SpanReader{SER_DISK, 0, ParseHex(univalue[i].get_str()), 0} >> txout;
+        SpanReader{SER_DISK, 0, ParseHex(univalue[i].get_str())} >> txout;
         prevouts.push_back(std::move(txout));
     }
     return prevouts;
@@ -1754,7 +1754,7 @@ BOOST_AUTO_TEST_CASE(bip341_keypath_test_vectors)
     for (const auto& vec : vectors.getValues()) {
         auto txhex = ParseHex(vec["given"]["rawUnsignedTx"].get_str());
         CMutableTransaction tx;
-        SpanReader{SER_NETWORK, PROTOCOL_VERSION, txhex, 0} >> tx;
+        SpanReader{SER_NETWORK, PROTOCOL_VERSION, txhex} >> tx;
         std::vector<CTxOut> utxos;
         for (const auto& utxo_spent : vec["given"]["utxosSpent"].getValues()) {
             auto script_bytes = ParseHex(utxo_spent["scriptPubKey"].get_str());

src/test/streams_tests.cpp

@@ -71,7 +71,7 @@ BOOST_AUTO_TEST_CASE(streams_vector_reader)
 {
     std::vector<unsigned char> vch = {1, 255, 3, 4, 5, 6};
 
-    SpanReader reader{SER_NETWORK, INIT_PROTO_VERSION, vch, 0};
+    SpanReader reader{SER_NETWORK, INIT_PROTO_VERSION, vch};
     BOOST_CHECK_EQUAL(reader.size(), 6U);
     BOOST_CHECK(!reader.empty());
 
@@ -101,7 +101,7 @@ BOOST_AUTO_TEST_CASE(streams_vector_reader)
     BOOST_CHECK_THROW(reader >> d, std::ios_base::failure);
 
     // Read a 4 bytes as a signed int from the beginning of the buffer.
-    SpanReader new_reader{SER_NETWORK, INIT_PROTO_VERSION, vch, 0};
+    SpanReader new_reader{SER_NETWORK, INIT_PROTO_VERSION, vch};
     new_reader >> d;
     BOOST_CHECK_EQUAL(d, 67370753); // 1,255,3,4 in little-endian base-256
     BOOST_CHECK_EQUAL(new_reader.size(), 2U);
@@ -115,7 +115,7 @@ BOOST_AUTO_TEST_CASE(streams_vector_reader)
 BOOST_AUTO_TEST_CASE(streams_vector_reader_rvalue)
 {
     std::vector<uint8_t> data{0x82, 0xa7, 0x31};
-    SpanReader reader{SER_NETWORK, INIT_PROTO_VERSION, data, /* pos= */ 0};
+    SpanReader reader{SER_NETWORK, INIT_PROTO_VERSION, data};
     uint32_t varint = 0;
     // Deserialize into r-value
     reader >> VARINT(varint);

src/wallet/test/wallet_tests.cpp

@@ -682,7 +682,7 @@ BOOST_FIXTURE_TEST_CASE(wallet_descriptor_test, BasicTestingSetup)
     vw << (int32_t)0;
     vw << (int32_t)1;
 
-    SpanReader vr{0, 0, malformed_record, 0};
+    SpanReader vr{0, 0, malformed_record};
     WalletDescriptor w_desc;
     BOOST_CHECK_EXCEPTION(vr >> w_desc, std::ios_base::failure, malformed_descriptor);
 }