Skip to content

Commit

Permalink
Merge bitcoin/bitcoin#30765: refactor: Allow CScript's operator<<…
Browse files Browse the repository at this point in the history 
… to accept spans, not just vectors

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/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
  • Loading branch information
@achow101
achow101 committed Sep 20, 2024
2 parents 0894748 + 5e190cd commit 33adc75
Show file tree
Hide file tree
Showing 7 changed files with 64 additions and 43 deletions.
4 changes: 2 additions & 2 deletions src/kernel/chainparams.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down Expand Up @@ -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,
Expand Down
9 changes: 6 additions & 3 deletions src/prevector.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
}
Expand All @@ -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);
}
Expand Down
61 changes: 36 additions & 25 deletions src/script/script.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include <cstdint>
#include <cstring>
#include <limits>
#include <span>
#include <stdexcept>
#include <string>
#include <type_traits>
Expand Down Expand Up @@ -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)
{
Expand Down Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion src/test/miner_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down
9 changes: 8 additions & 1 deletion src/test/script_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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);

Expand Down
18 changes: 9 additions & 9 deletions src/test/transaction_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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");

Expand Down
4 changes: 2 additions & 2 deletions src/wallet/test/ismine_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand Down

0 comments on commit 33adc75

Please sign in to comment.