Merge bitcoin#30765: refactor: Allow CScript's operator<< to accept spans, not just vectors

achow101 · achow101 · commit 33adc7521cc8 · 2024-09-20T15:16:53.000-04:00
5e190cd Replace CScript _hex_v_u8 appends with _hex (Lőrinc) cac846c Allow CScript's operator<< to accept spans, not just vectors (Lőrinc) c78d8ff prevector: avoid GCC bogus warnings in insert method (Lőrinc) Pull request description: Split out of bitcoin#30377 (comment). Replace `_hex_v_u8` for `CScript` appends to `_hex`, to skip vector conversion before serializing to the `prevector` in `CScript`. To enable both `unsigned char` and `std::byte` values, I've extracted the existing serialization to append the size & data in separate private methods to clarify that it does more than just a simple data insertion. There were also discussion on eliminating the operators here completely to obviate when we're serializing fixed-size collections as raw bytes, and when we're prefixing them with their size - should also be done in a separate PR. ACKs for top commit: achow101: ACK 5e190cd ryanofsky: Code review ACK 5e190cd. Looks good! hodlinator: re-ACK 5e190cd Tree-SHA512: 27a646629e017b2a05416d5eb964dda8b25b900d466221eff7bfa1339ded443e1c5c4cf8ff20cb3bba915a2603787a9fa6f6ec12bc0b9415d9eb07b57289192b
diff --git a/src/kernel/chainparams.cpp b/src/kernel/chainparams.cpp
@@ -73,7 +73,7 @@ static CBlock CreateGenesisBlock(const char* pszTimestamp, const CScript& genesi
 static CBlock CreateGenesisBlock(uint32_t nTime, uint32_t nNonce, uint32_t nBits, int32_t nVersion, const CAmount& genesisReward)
 {
     const char* pszTimestamp = "The Times 03/Jan/2009 Chancellor on brink of second bailout for banks";
-    const CScript genesisOutputScript = CScript() << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f"_hex_v_u8 << OP_CHECKSIG;
+    const CScript genesisOutputScript = CScript() << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f"_hex << OP_CHECKSIG;
     return CreateGenesisBlock(pszTimestamp, genesisOutputScript, nTime, nNonce, nBits, nVersion, genesisReward);
 }
 
@@ -353,7 +353,7 @@ class CTestNet4Params : public CChainParams {
         m_assumed_chain_state_size = 0;
 
         const char* testnet4_genesis_msg = "03/May/2024 000000000000000000001ebd58c244970b3aa9d783bb001011fbe8ea8e98e00e";
-        const CScript testnet4_genesis_script = CScript() << "000000000000000000000000000000000000000000000000000000000000000000"_hex_v_u8 << OP_CHECKSIG;
+        const CScript testnet4_genesis_script = CScript() << "000000000000000000000000000000000000000000000000000000000000000000"_hex << OP_CHECKSIG;
         genesis = CreateGenesisBlock(testnet4_genesis_msg,
                 testnet4_genesis_script,
                 1714777860,
diff --git a/src/prevector.h b/src/prevector.h
@@ -363,7 +363,8 @@ class prevector {
             change_capacity(new_size + (new_size >> 1));
         }
         T* ptr = item_ptr(p);
-        memmove(ptr + 1, ptr, (size() - p) * sizeof(T));
+        T* dst = ptr + 1;
+        memmove(dst, ptr, (size() - p) * sizeof(T));
         _size++;
         new(static_cast<void*>(ptr)) T(value);
         return iterator(ptr);
@@ -376,7 +377,8 @@ class prevector {
             change_capacity(new_size + (new_size >> 1));
         }
         T* ptr = item_ptr(p);
-        memmove(ptr + count, ptr, (size() - p) * sizeof(T));
+        T* dst = ptr + count;
+        memmove(dst, ptr, (size() - p) * sizeof(T));
         _size += count;
         fill(item_ptr(p), count, value);
     }
@@ -390,7 +392,8 @@ class prevector {
             change_capacity(new_size + (new_size >> 1));
         }
         T* ptr = item_ptr(p);
-        memmove(ptr + count, ptr, (size() - p) * sizeof(T));
+        T* dst = ptr + count;
+        memmove(dst, ptr, (size() - p) * sizeof(T));
         _size += count;
         fill(ptr, first, last);
     }
diff --git a/src/script/script.h b/src/script/script.h
@@ -17,6 +17,7 @@
 #include <cstdint>
 #include <cstring>
 #include <limits>
+#include <span>
 #include <stdexcept>
 #include <string>
 #include <type_traits>
@@ -412,6 +413,32 @@ bool GetScriptOp(CScriptBase::const_iterator& pc, CScriptBase::const_iterator en
 /** Serialized script, used inside transaction inputs and outputs */
 class CScript : public CScriptBase
 {
+private:
+    inline void AppendDataSize(const uint32_t size)
+    {
+        if (size < OP_PUSHDATA1) {
+            insert(end(), static_cast<value_type>(size));
+        } else if (size <= 0xff) {
+            insert(end(), OP_PUSHDATA1);
+            insert(end(), static_cast<value_type>(size));
+        } else if (size <= 0xffff) {
+            insert(end(), OP_PUSHDATA2);
+            value_type data[2];
+            WriteLE16(data, size);
+            insert(end(), std::cbegin(data), std::cend(data));
+        } else {
+            insert(end(), OP_PUSHDATA4);
+            value_type data[4];
+            WriteLE32(data, size);
+            insert(end(), std::cbegin(data), std::cend(data));
+        }
+    }
+
+    void AppendData(std::span<const value_type> data)
+    {
+        insert(end(), data.begin(), data.end());
+    }
+
 protected:
     CScript& push_int64(int64_t n)
     {
@@ -463,35 +490,19 @@ class CScript : public CScriptBase
         return *this;
     }
 
-    CScript& operator<<(const std::vector<unsigned char>& b) LIFETIMEBOUND
+    CScript& operator<<(std::span<const std::byte> b) LIFETIMEBOUND
     {
-        if (b.size() < OP_PUSHDATA1)
-        {
-            insert(end(), (unsigned char)b.size());
-        }
-        else if (b.size() <= 0xff)
-        {
-            insert(end(), OP_PUSHDATA1);
-            insert(end(), (unsigned char)b.size());
-        }
-        else if (b.size() <= 0xffff)
-        {
-            insert(end(), OP_PUSHDATA2);
-            uint8_t _data[2];
-            WriteLE16(_data, b.size());
-            insert(end(), _data, _data + sizeof(_data));
-        }
-        else
-        {
-            insert(end(), OP_PUSHDATA4);
-            uint8_t _data[4];
-            WriteLE32(_data, b.size());
-            insert(end(), _data, _data + sizeof(_data));
-        }
-        insert(end(), b.begin(), b.end());
+        AppendDataSize(b.size());
+        AppendData({reinterpret_cast<const value_type*>(b.data()), b.size()});
         return *this;
     }
 
+    // For compatibility reasons. In new code, prefer using std::byte instead of uint8_t.
+    CScript& operator<<(std::span<const value_type> b) LIFETIMEBOUND
+    {
+        return *this << std::as_bytes(b);
+    }
+
     bool GetOp(const_iterator& pc, opcodetype& opcodeRet, std::vector<unsigned char>& vchRet) const
     {
         return GetScriptOp(pc, end(), opcodeRet, &vchRet);
diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp
@@ -608,7 +608,7 @@ void MinerTestingSetup::TestPrioritisedMining(const CScript& scriptPubKey, const
 BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
 {
     // Note that by default, these tests run with size accounting enabled.
-    CScript scriptPubKey = CScript() << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f"_hex_v_u8 << OP_CHECKSIG;
+    CScript scriptPubKey = CScript() << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f"_hex << OP_CHECKSIG;
     std::unique_ptr<CBlockTemplate> pblocktemplate;
 
     CTxMemPool& tx_mempool{*m_node.mempool};
diff --git a/src/test/script_tests.cpp b/src/test/script_tests.cpp
@@ -1368,6 +1368,13 @@ static CScript ScriptFromHex(const std::string& str)
     return ToScript(*Assert(TryParseHex(str)));
 }
 
+BOOST_AUTO_TEST_CASE(script_byte_array_u8_vector_equivalence)
+{
+    const CScript scriptPubKey1 = CScript() << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f"_hex_v_u8 << OP_CHECKSIG;
+    const CScript scriptPubKey2 = CScript() << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f"_hex << OP_CHECKSIG;
+    BOOST_CHECK(scriptPubKey1 == scriptPubKey2);
+}
+
 BOOST_AUTO_TEST_CASE(script_FindAndDelete)
 {
     // Exercise the FindAndDelete functionality
@@ -1421,7 +1428,7 @@ BOOST_AUTO_TEST_CASE(script_FindAndDelete)
     // prefix, leaving 02ff03 which is push-two-bytes:
     s = ToScript("0302ff030302ff03"_hex);
     d = ToScript("03"_hex);
-    expect = CScript() << "ff03"_hex_v_u8 << "ff03"_hex_v_u8;
+    expect = CScript() << "ff03"_hex << "ff03"_hex;
     BOOST_CHECK_EQUAL(FindAndDelete(s, d), 2);
     BOOST_CHECK(s == expect);
 
diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp
@@ -852,24 +852,24 @@ BOOST_AUTO_TEST_CASE(test_IsStandard)
     CheckIsNotStandard(t, "scriptpubkey");
 
     // MAX_OP_RETURN_RELAY-byte TxoutType::NULL_DATA (standard)
-    t.vout[0].scriptPubKey = CScript() << OP_RETURN << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef3804678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38"_hex_v_u8;
+    t.vout[0].scriptPubKey = CScript() << OP_RETURN << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef3804678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38"_hex;
     BOOST_CHECK_EQUAL(MAX_OP_RETURN_RELAY, t.vout[0].scriptPubKey.size());
     CheckIsStandard(t);
 
     // MAX_OP_RETURN_RELAY+1-byte TxoutType::NULL_DATA (non-standard)
-    t.vout[0].scriptPubKey = CScript() << OP_RETURN << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef3804678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef3800"_hex_v_u8;
+    t.vout[0].scriptPubKey = CScript() << OP_RETURN << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef3804678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef3800"_hex;
     BOOST_CHECK_EQUAL(MAX_OP_RETURN_RELAY + 1, t.vout[0].scriptPubKey.size());
     CheckIsNotStandard(t, "scriptpubkey");
 
     // Data payload can be encoded in any way...
-    t.vout[0].scriptPubKey = CScript() << OP_RETURN << ""_hex_v_u8;
+    t.vout[0].scriptPubKey = CScript() << OP_RETURN << ""_hex;
     CheckIsStandard(t);
-    t.vout[0].scriptPubKey = CScript() << OP_RETURN << "00"_hex_v_u8 << "01"_hex_v_u8;
+    t.vout[0].scriptPubKey = CScript() << OP_RETURN << "00"_hex << "01"_hex;
     CheckIsStandard(t);
     // OP_RESERVED *is* considered to be a PUSHDATA type opcode by IsPushOnly()!
-    t.vout[0].scriptPubKey = CScript() << OP_RETURN << OP_RESERVED << -1 << 0 << "01"_hex_v_u8 << 2 << 3 << 4 << 5 << 6 << 7 << 8 << 9 << 10 << 11 << 12 << 13 << 14 << 15 << 16;
+    t.vout[0].scriptPubKey = CScript() << OP_RETURN << OP_RESERVED << -1 << 0 << "01"_hex << 2 << 3 << 4 << 5 << 6 << 7 << 8 << 9 << 10 << 11 << 12 << 13 << 14 << 15 << 16;
     CheckIsStandard(t);
-    t.vout[0].scriptPubKey = CScript() << OP_RETURN << 0 << "01"_hex_v_u8 << 2 << "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff"_hex_v_u8;
+    t.vout[0].scriptPubKey = CScript() << OP_RETURN << 0 << "01"_hex << 2 << "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff"_hex;
     CheckIsStandard(t);
 
     // ...so long as it only contains PUSHDATA's
@@ -883,13 +883,13 @@ BOOST_AUTO_TEST_CASE(test_IsStandard)
 
     // Only one TxoutType::NULL_DATA permitted in all cases
     t.vout.resize(2);
-    t.vout[0].scriptPubKey = CScript() << OP_RETURN << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38"_hex_v_u8;
+    t.vout[0].scriptPubKey = CScript() << OP_RETURN << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38"_hex;
     t.vout[0].nValue = 0;
-    t.vout[1].scriptPubKey = CScript() << OP_RETURN << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38"_hex_v_u8;
+    t.vout[1].scriptPubKey = CScript() << OP_RETURN << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38"_hex;
     t.vout[1].nValue = 0;
     CheckIsNotStandard(t, "multi-op-return");
 
-    t.vout[0].scriptPubKey = CScript() << OP_RETURN << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38"_hex_v_u8;
+    t.vout[0].scriptPubKey = CScript() << OP_RETURN << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38"_hex;
     t.vout[1].scriptPubKey = CScript() << OP_RETURN;
     CheckIsNotStandard(t, "multi-op-return");
 
diff --git a/src/wallet/test/ismine_tests.cpp b/src/wallet/test/ismine_tests.cpp
@@ -684,7 +684,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard)
         BOOST_CHECK(keystore.GetLegacyScriptPubKeyMan()->AddKey(keys[0]));
 
         scriptPubKey.clear();
-        scriptPubKey << OP_0 << "aabb"_hex_v_u8;
+        scriptPubKey << OP_0 << "aabb"_hex;
 
         result = keystore.GetLegacyScriptPubKeyMan()->IsMine(scriptPubKey);
         BOOST_CHECK_EQUAL(result, ISMINE_NO);
@@ -699,7 +699,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard)
         BOOST_CHECK(keystore.GetLegacyScriptPubKeyMan()->AddKey(keys[0]));
 
         scriptPubKey.clear();
-        scriptPubKey << OP_16 << "aabb"_hex_v_u8;
+        scriptPubKey << OP_16 << "aabb"_hex;
 
         result = keystore.GetLegacyScriptPubKeyMan()->IsMine(scriptPubKey);
         BOOST_CHECK_EQUAL(result, ISMINE_NO);

Original file line number	Diff line number	Diff line change
`@@ -363,7 +363,8 @@ class prevector {`
`363`	`363`	`change_capacity(new_size + (new_size >> 1));`
`364`	`364`	`}`
`365`	`365`	`T* ptr = item_ptr(p);`
`366`		`- memmove(ptr + 1, ptr, (size() - p) * sizeof(T));`
	`366`	`+ T* dst = ptr + 1;`
	`367`	`+ memmove(dst, ptr, (size() - p) * sizeof(T));`
`367`	`368`	`_size++;`
`368`	`369`	`new(static_cast<void*>(ptr)) T(value);`
`369`	`370`	`return iterator(ptr);`
`@@ -376,7 +377,8 @@ class prevector {`
`376`	`377`	`change_capacity(new_size + (new_size >> 1));`
`377`	`378`	`}`
`378`	`379`	`T* ptr = item_ptr(p);`
`379`		`- memmove(ptr + count, ptr, (size() - p) * sizeof(T));`
	`380`	`+ T* dst = ptr + count;`
	`381`	`+ memmove(dst, ptr, (size() - p) * sizeof(T));`
`380`	`382`	`_size += count;`
`381`	`383`	`fill(item_ptr(p), count, value);`
`382`	`384`	`}`
`@@ -390,7 +392,8 @@ class prevector {`
`390`	`392`	`change_capacity(new_size + (new_size >> 1));`
`391`	`393`	`}`
`392`	`394`	`T* ptr = item_ptr(p);`
`393`		`- memmove(ptr + count, ptr, (size() - p) * sizeof(T));`
	`395`	`+ T* dst = ptr + count;`
	`396`	`+ memmove(dst, ptr, (size() - p) * sizeof(T));`
`394`	`397`	`_size += count;`
`395`	`398`	`fill(ptr, first, last);`
`396`	`399`	`}`
Original file line number	Diff line number	Diff line change
`@@ -608,7 +608,7 @@ void MinerTestingSetup::TestPrioritisedMining(const CScript& scriptPubKey, const`
`608`	`608`	`BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)`
`609`	`609`	`{`
`610`	`610`	`// Note that by default, these tests run with size accounting enabled.`
`611`		`- CScript scriptPubKey = CScript() << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f"_hex_v_u8 << OP_CHECKSIG;`
	`611`	`+ CScript scriptPubKey = CScript() << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f"_hex << OP_CHECKSIG;`
`612`	`612`	`std::unique_ptr<CBlockTemplate> pblocktemplate;`
`613`	`613`
`614`	`614`	`CTxMemPool& tx_mempool{*m_node.mempool};`