consensus: Store transaction nVersion as uint32_t

achow101 · achow101 · commit 27e70f1f5be1 · 2024-06-07T12:40:21.000-04:00
Given that the use of a transaction's nVersion is always as an unsigned
int, it doesn't make sense to store it as signed and then cast it to
unsigned.
diff --git a/src/bitcoin-tx.cpp b/src/bitcoin-tx.cpp
@@ -203,12 +203,12 @@ static CAmount ExtractAndValidateValue(const std::string& strValue)
 
 static void MutateTxVersion(CMutableTransaction& tx, const std::string& cmdVal)
 {
-    int64_t newVersion;
-    if (!ParseInt64(cmdVal, &newVersion) || newVersion < 1 || newVersion > TX_MAX_STANDARD_VERSION) {
+    uint32_t newVersion;
+    if (!ParseUInt32(cmdVal, &newVersion) || newVersion < 1 || newVersion > TX_MAX_STANDARD_VERSION) {
         throw std::runtime_error("Invalid TX version requested: '" + cmdVal + "'");
     }
 
-    tx.nVersion = (int) newVersion;
+    tx.nVersion = newVersion;
 }
 
 static void MutateTxLocktime(CMutableTransaction& tx, const std::string& cmdVal)
diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp
@@ -48,11 +48,7 @@ std::pair<int, int64_t> CalculateSequenceLocks(const CTransaction &tx, int flags
     int nMinHeight = -1;
     int64_t nMinTime = -1;
 
-    // tx.nVersion is signed integer so requires cast to unsigned otherwise
-    // we would be doing a signed comparison and half the range of nVersion
-    // wouldn't support BIP 68.
-    bool fEnforceBIP68 = static_cast<uint32_t>(tx.nVersion) >= 2
-                      && flags & LOCKTIME_VERIFY_SEQUENCE;
+    bool fEnforceBIP68 = tx.nVersion >= 2 && flags & LOCKTIME_VERIFY_SEQUENCE;
 
     // Do not enforce sequence numbers as a relative lock time
     // unless we have been instructed to
diff --git a/src/core_write.cpp b/src/core_write.cpp
@@ -174,9 +174,7 @@ void TxToUniv(const CTransaction& tx, const uint256& block_hash, UniValue& entry
 
     entry.pushKV("txid", tx.GetHash().GetHex());
     entry.pushKV("hash", tx.GetWitnessHash().GetHex());
-    // Transaction version is actually unsigned in consensus checks, just signed in memory,
-    // so cast to unsigned before giving it to the user.
-    entry.pushKV("version", static_cast<int64_t>(static_cast<uint32_t>(tx.nVersion)));
+    entry.pushKV("version", tx.nVersion);
     entry.pushKV("size", tx.GetTotalSize());
     entry.pushKV("vsize", (GetTransactionWeight(tx) + WITNESS_SCALE_FACTOR - 1) / WITNESS_SCALE_FACTOR);
     entry.pushKV("weight", GetTransactionWeight(tx));
diff --git a/src/primitives/transaction.cpp b/src/primitives/transaction.cpp
@@ -63,8 +63,8 @@ std::string CTxOut::ToString() const
     return strprintf("CTxOut(nValue=%d.%08d, scriptPubKey=%s)", nValue / COIN, nValue % COIN, HexStr(scriptPubKey).substr(0, 30));
 }
 
-CMutableTransaction::CMutableTransaction() : nVersion(CTransaction::CURRENT_VERSION), nLockTime(0) {}
-CMutableTransaction::CMutableTransaction(const CTransaction& tx) : vin(tx.vin), vout(tx.vout), nVersion(tx.nVersion), nLockTime(tx.nLockTime) {}
+CMutableTransaction::CMutableTransaction() : nVersion{CTransaction::CURRENT_VERSION}, nLockTime{0} {}
+CMutableTransaction::CMutableTransaction(const CTransaction& tx) : vin(tx.vin), vout(tx.vout), nVersion{tx.nVersion}, nLockTime{tx.nLockTime} {}
 
 Txid CMutableTransaction::GetHash() const
 {
@@ -92,8 +92,8 @@ Wtxid CTransaction::ComputeWitnessHash() const
     return Wtxid::FromUint256((HashWriter{} << TX_WITH_WITNESS(*this)).GetHash());
 }
 
-CTransaction::CTransaction(const CMutableTransaction& tx) : vin(tx.vin), vout(tx.vout), nVersion(tx.nVersion), nLockTime(tx.nLockTime), m_has_witness{ComputeHasWitness()}, hash{ComputeHash()}, m_witness_hash{ComputeWitnessHash()} {}
-CTransaction::CTransaction(CMutableTransaction&& tx) : vin(std::move(tx.vin)), vout(std::move(tx.vout)), nVersion(tx.nVersion), nLockTime(tx.nLockTime), m_has_witness{ComputeHasWitness()}, hash{ComputeHash()}, m_witness_hash{ComputeWitnessHash()} {}
+CTransaction::CTransaction(const CMutableTransaction& tx) : vin(tx.vin), vout(tx.vout), nVersion{tx.nVersion}, nLockTime{tx.nLockTime}, m_has_witness{ComputeHasWitness()}, hash{ComputeHash()}, m_witness_hash{ComputeWitnessHash()} {}
+CTransaction::CTransaction(CMutableTransaction&& tx) : vin(std::move(tx.vin)), vout(std::move(tx.vout)), nVersion{tx.nVersion}, nLockTime{tx.nLockTime}, m_has_witness{ComputeHasWitness()}, hash{ComputeHash()}, m_witness_hash{ComputeWitnessHash()} {}
 
 CAmount CTransaction::GetValueOut() const
 {
@@ -115,7 +115,7 @@ unsigned int CTransaction::GetTotalSize() const
 std::string CTransaction::ToString() const
 {
     std::string str;
-    str += strprintf("CTransaction(hash=%s, ver=%d, vin.size=%u, vout.size=%u, nLockTime=%u)\n",
+    str += strprintf("CTransaction(hash=%s, ver=%u, vin.size=%u, vout.size=%u, nLockTime=%u)\n",
         GetHash().ToString().substr(0,10),
         nVersion,
         vin.size(),
diff --git a/src/primitives/transaction.h b/src/primitives/transaction.h
@@ -197,13 +197,13 @@ static constexpr TransactionSerParams TX_NO_WITNESS{.allow_witness = false};
 
 /**
  * Basic transaction serialization format:
- * - int32_t nVersion
+ * - uint32_t nVersion
  * - std::vector<CTxIn> vin
  * - std::vector<CTxOut> vout
  * - uint32_t nLockTime
  *
  * Extended transaction serialization format:
- * - int32_t nVersion
+ * - uint32_t nVersion
  * - unsigned char dummy = 0x00
  * - unsigned char flags (!= 0)
  * - std::vector<CTxIn> vin
@@ -296,7 +296,7 @@ class CTransaction
 {
 public:
     // Default transaction version.
-    static const int32_t CURRENT_VERSION=2;
+    static const uint32_t CURRENT_VERSION{2};
 
     // The local variables are made const to prevent unintended modification
     // without updating the cached hash value. However, CTransaction is not
@@ -305,7 +305,7 @@ class CTransaction
     // structure, including the hash.
     const std::vector<CTxIn> vin;
     const std::vector<CTxOut> vout;
-    const int32_t nVersion;
+    const uint32_t nVersion;
     const uint32_t nLockTime;
 
 private:
@@ -378,7 +378,7 @@ struct CMutableTransaction
 {
     std::vector<CTxIn> vin;
     std::vector<CTxOut> vout;
-    int32_t nVersion;
+    uint32_t nVersion;
     uint32_t nLockTime;
 
     explicit CMutableTransaction();
diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp
@@ -1748,8 +1748,8 @@ static RPCHelpMan joinpsbts()
         }
         psbtxs.push_back(psbtx);
         // Choose the highest version number
-        if (static_cast<uint32_t>(psbtx.tx->nVersion) > best_version) {
-            best_version = static_cast<uint32_t>(psbtx.tx->nVersion);
+        if (psbtx.tx->nVersion > best_version) {
+            best_version = psbtx.tx->nVersion;
         }
         // Choose the lowest lock time
         if (psbtx.tx->nLockTime < best_locktime) {
@@ -1760,7 +1760,7 @@ static RPCHelpMan joinpsbts()
     // Create a blank psbt where everything will be added
     PartiallySignedTransaction merged_psbt;
     merged_psbt.tx = CMutableTransaction();
-    merged_psbt.tx->nVersion = static_cast<int32_t>(best_version);
+    merged_psbt.tx->nVersion = best_version;
     merged_psbt.tx->nLockTime = best_locktime;
 
     // Merge
diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp
@@ -1743,7 +1743,7 @@ bool GenericTransactionSignatureChecker<T>::CheckSequence(const CScriptNum& nSeq
 
     // Fail if the transaction's version number is not set high
     // enough to trigger BIP 68 rules.
-    if (static_cast<uint32_t>(txTo->nVersion) < 2)
+    if (txTo->nVersion < 2)
         return false;
 
     // Sequence numbers with their most significant bit set are not
diff --git a/src/test/fuzz/util.cpp b/src/test/fuzz/util.cpp
@@ -45,7 +45,7 @@ CMutableTransaction ConsumeTransaction(FuzzedDataProvider& fuzzed_data_provider,
     const auto p2wsh_op_true = fuzzed_data_provider.ConsumeBool();
     tx_mut.nVersion = fuzzed_data_provider.ConsumeBool() ?
                           CTransaction::CURRENT_VERSION :
-                          fuzzed_data_provider.ConsumeIntegral<int32_t>();
+                          fuzzed_data_provider.ConsumeIntegral<uint32_t>();
     tx_mut.nLockTime = fuzzed_data_provider.ConsumeIntegral<uint32_t>();
     const auto num_in = fuzzed_data_provider.ConsumeIntegralInRange<int>(0, max_num_in);
     const auto num_out = fuzzed_data_provider.ConsumeIntegralInRange<int>(0, max_num_out);
diff --git a/src/test/sighash_tests.cpp b/src/test/sighash_tests.cpp
@@ -92,7 +92,7 @@ void static RandomScript(CScript &script) {
 
 void static RandomTransaction(CMutableTransaction& tx, bool fSingle)
 {
-    tx.nVersion = int(InsecureRand32());
+    tx.nVersion = InsecureRand32();
     tx.vin.clear();
     tx.vout.clear();
     tx.nLockTime = (InsecureRandBool()) ? InsecureRand32() : 0;
diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp
@@ -780,7 +780,7 @@ BOOST_AUTO_TEST_CASE(test_IsStandard)
     CheckIsStandard(t);
 
     // Disallowed nVersion
-    t.nVersion = -1;
+    t.nVersion = std::numeric_limits<uint32_t>::max();
     CheckIsNotStandard(t, "version");
 
     t.nVersion = 0;
diff --git a/test/functional/feature_taproot.py b/test/functional/feature_taproot.py
@@ -1410,7 +1410,7 @@ def test_spenders(self, node, spenders, input_counts):
         while left:
             # Construct CTransaction with random nVersion, nLocktime
             tx = CTransaction()
-            tx.nVersion = random.choice([1, 2, random.randint(-0x80000000, 0x7fffffff)])
+            tx.nVersion = random.choice([1, 2, random.getrandbits(32)])
             min_sequence = (tx.nVersion != 1 and tx.nVersion != 0) * 0x80000000  # The minimum sequence number to disable relative locktime
             if random.choice([True, False]):
                 tx.nLockTime = random.randrange(LOCKTIME_THRESHOLD, self.lastblocktime - 7200)  # all absolute locktimes in the past
diff --git a/test/functional/p2p_segwit.py b/test/functional/p2p_segwit.py
@@ -1164,7 +1164,7 @@ def serialize_with_witness(self):
                 if not self.wit.is_null():
                     flags |= 1
                 r = b""
-                r += self.nVersion.to_bytes(4, "little", signed=True)
+                r += self.nVersion.to_bytes(4, "little")
                 if flags:
                     dummy = []
                     r += ser_vector(dummy)
@@ -1975,7 +1975,7 @@ def test_superfluous_witness(self):
         def serialize_with_bogus_witness(tx):
             flags = 3
             r = b""
-            r += tx.nVersion.to_bytes(4, "little", signed=True)
+            r += tx.nVersion.to_bytes(4, "little")
             if flags:
                 dummy = []
                 r += ser_vector(dummy)
diff --git a/test/functional/rpc_packages.py b/test/functional/rpc_packages.py
@@ -394,7 +394,7 @@ def test_submitpackage(self):
         peer = node.add_p2p_connection(P2PTxInvStore())
         txs = self.wallet.create_self_transfer_chain(chain_length=2)
         bad_child = tx_from_hex(txs[1]["hex"])
-        bad_child.nVersion = -1
+        bad_child.nVersion = 0xffffffff
         hex_partial_acceptance = [txs[0]["hex"], bad_child.serialize().hex()]
         res = node.submitpackage(hex_partial_acceptance)
         assert_equal(res["package_msg"], "transaction failed")
diff --git a/test/functional/rpc_rawtransaction.py b/test/functional/rpc_rawtransaction.py
@@ -463,9 +463,9 @@ def transaction_version_number_tests(self):
         self.log.info("Test transaction version numbers")
 
         # Test the minimum transaction version number that fits in a signed 32-bit integer.
-        # As transaction version is unsigned, this should convert to its unsigned equivalent.
+        # As transaction version is serialized unsigned, this should convert to its unsigned equivalent.
         tx = CTransaction()
-        tx.nVersion = -0x80000000
+        tx.nVersion = 0x80000000
         rawtx = tx.serialize().hex()
         decrawtx = self.nodes[0].decoderawtransaction(rawtx)
         assert_equal(decrawtx['version'], 0x80000000)
@@ -477,6 +477,20 @@ def transaction_version_number_tests(self):
         decrawtx = self.nodes[0].decoderawtransaction(rawtx)
         assert_equal(decrawtx['version'], 0x7fffffff)
 
+        # Test the minimum transaction version number that fits in an unsigned 32-bit integer.
+        tx = CTransaction()
+        tx.nVersion = 0
+        rawtx = tx.serialize().hex()
+        decrawtx = self.nodes[0].decoderawtransaction(rawtx)
+        assert_equal(decrawtx['version'], 0)
+
+        # Test the maximum transaction version number that fits in an unsigned 32-bit integer.
+        tx = CTransaction()
+        tx.nVersion = 0xffffffff
+        rawtx = tx.serialize().hex()
+        decrawtx = self.nodes[0].decoderawtransaction(rawtx)
+        assert_equal(decrawtx['version'], 0xffffffff)
+
     def raw_multisig_transaction_legacy_tests(self):
         self.log.info("Test raw multisig transactions (legacy)")
         # The traditional multisig workflow does not work with descriptor wallets so these are legacy only.
diff --git a/test/functional/test_framework/messages.py b/test/functional/test_framework/messages.py
@@ -582,7 +582,7 @@ def __init__(self, tx=None):
             self.wit = copy.deepcopy(tx.wit)
 
     def deserialize(self, f):
-        self.nVersion = int.from_bytes(f.read(4), "little", signed=True)
+        self.nVersion = int.from_bytes(f.read(4), "little")
         self.vin = deser_vector(f, CTxIn)
         flags = 0
         if len(self.vin) == 0:
@@ -605,7 +605,7 @@ def deserialize(self, f):
 
     def serialize_without_witness(self):
         r = b""
-        r += self.nVersion.to_bytes(4, "little", signed=True)
+        r += self.nVersion.to_bytes(4, "little")
         r += ser_vector(self.vin)
         r += ser_vector(self.vout)
         r += self.nLockTime.to_bytes(4, "little")
@@ -617,7 +617,7 @@ def serialize_with_witness(self):
         if not self.wit.is_null():
             flags |= 1
         r = b""
-        r += self.nVersion.to_bytes(4, "little", signed=True)
+        r += self.nVersion.to_bytes(4, "little")
         if flags:
             dummy = []
             r += ser_vector(dummy)
diff --git a/test/functional/test_framework/script.py b/test/functional/test_framework/script.py
@@ -738,7 +738,7 @@ def SegwitV0SignatureMsg(script, txTo, inIdx, hashtype, amount):
         hashOutputs = uint256_from_str(hash256(serialize_outputs))
 
     ss = bytes()
-    ss += txTo.nVersion.to_bytes(4, "little", signed=True)
+    ss += txTo.nVersion.to_bytes(4, "little")
     ss += ser_uint256(hashPrevouts)
     ss += ser_uint256(hashSequence)
     ss += txTo.vin[inIdx].prevout.serialize()
@@ -817,7 +817,7 @@ def TaprootSignatureMsg(txTo, spent_utxos, hash_type, input_index = 0, scriptpat
     in_type = hash_type & SIGHASH_ANYONECANPAY
     spk = spent_utxos[input_index].scriptPubKey
     ss = bytes([0, hash_type]) # epoch, hash_type
-    ss += txTo.nVersion.to_bytes(4, "little", signed=True)
+    ss += txTo.nVersion.to_bytes(4, "little")
     ss += txTo.nLockTime.to_bytes(4, "little")
     if in_type != SIGHASH_ANYONECANPAY:
         ss += BIP341_sha_prevouts(txTo)

Original file line number	Diff line number	Diff line change
`@@ -203,12 +203,12 @@ static CAmount ExtractAndValidateValue(const std::string& strValue)`
`203`	`203`
`204`	`204`	`static void MutateTxVersion(CMutableTransaction& tx, const std::string& cmdVal)`
`205`	`205`	`{`
`206`		`- int64_t newVersion;`
`207`		`- if (!ParseInt64(cmdVal, &newVersion) \|\| newVersion < 1 \|\| newVersion > TX_MAX_STANDARD_VERSION) {`
	`206`	`+ uint32_t newVersion;`
	`207`	`+ if (!ParseUInt32(cmdVal, &newVersion) \|\| newVersion < 1 \|\| newVersion > TX_MAX_STANDARD_VERSION) {`
`208`	`208`	`throw std::runtime_error("Invalid TX version requested: '" + cmdVal + "'");`
`209`	`209`	`}`
`210`	`210`
`211`		`- tx.nVersion = (int) newVersion;`
	`211`	`+ tx.nVersion = newVersion;`
`212`	`212`	`}`
`213`	`213`
`214`	`214`	`static void MutateTxLocktime(CMutableTransaction& tx, const std::string& cmdVal)`
Original file line number	Diff line number	Diff line change
`@@ -63,8 +63,8 @@ std::string CTxOut::ToString() const`
`63`	`63`	`return strprintf("CTxOut(nValue=%d.%08d, scriptPubKey=%s)", nValue / COIN, nValue % COIN, HexStr(scriptPubKey).substr(0, 30));`
`64`	`64`	`}`
`65`	`65`
`66`		`-CMutableTransaction::CMutableTransaction() : nVersion(CTransaction::CURRENT_VERSION), nLockTime(0) {}`
`67`		`-CMutableTransaction::CMutableTransaction(const CTransaction& tx) : vin(tx.vin), vout(tx.vout), nVersion(tx.nVersion), nLockTime(tx.nLockTime) {}`
	`66`	`+CMutableTransaction::CMutableTransaction() : nVersion{CTransaction::CURRENT_VERSION}, nLockTime{0} {}`
	`67`	`+CMutableTransaction::CMutableTransaction(const CTransaction& tx) : vin(tx.vin), vout(tx.vout), nVersion{tx.nVersion}, nLockTime{tx.nLockTime} {}`
`68`	`68`
`69`	`69`	`Txid CMutableTransaction::GetHash() const`
`70`	`70`	`{`
`@@ -92,8 +92,8 @@ Wtxid CTransaction::ComputeWitnessHash() const`
`92`	`92`	`return Wtxid::FromUint256((HashWriter{} << TX_WITH_WITNESS(*this)).GetHash());`
`93`	`93`	`}`
`94`	`94`
`95`		`-CTransaction::CTransaction(const CMutableTransaction& tx) : vin(tx.vin), vout(tx.vout), nVersion(tx.nVersion), nLockTime(tx.nLockTime), m_has_witness{ComputeHasWitness()}, hash{ComputeHash()}, m_witness_hash{ComputeWitnessHash()} {}`
`96`		`-CTransaction::CTransaction(CMutableTransaction&& tx) : vin(std::move(tx.vin)), vout(std::move(tx.vout)), nVersion(tx.nVersion), nLockTime(tx.nLockTime), m_has_witness{ComputeHasWitness()}, hash{ComputeHash()}, m_witness_hash{ComputeWitnessHash()} {}`
	`95`	`+CTransaction::CTransaction(const CMutableTransaction& tx) : vin(tx.vin), vout(tx.vout), nVersion{tx.nVersion}, nLockTime{tx.nLockTime}, m_has_witness{ComputeHasWitness()}, hash{ComputeHash()}, m_witness_hash{ComputeWitnessHash()} {}`
	`96`	`+CTransaction::CTransaction(CMutableTransaction&& tx) : vin(std::move(tx.vin)), vout(std::move(tx.vout)), nVersion{tx.nVersion}, nLockTime{tx.nLockTime}, m_has_witness{ComputeHasWitness()}, hash{ComputeHash()}, m_witness_hash{ComputeWitnessHash()} {}`
`97`	`97`
`98`	`98`	`CAmount CTransaction::GetValueOut() const`
`99`	`99`	`{`
`@@ -115,7 +115,7 @@ unsigned int CTransaction::GetTotalSize() const`
`115`	`115`	`std::string CTransaction::ToString() const`
`116`	`116`	`{`
`117`	`117`	`std::string str;`
`118`		`- str += strprintf("CTransaction(hash=%s, ver=%d, vin.size=%u, vout.size=%u, nLockTime=%u)\n",`
	`118`	`+ str += strprintf("CTransaction(hash=%s, ver=%u, vin.size=%u, vout.size=%u, nLockTime=%u)\n",`
`119`	`119`	`GetHash().ToString().substr(0,10),`
`120`	`120`	`nVersion,`
`121`	`121`	`vin.size(),`
Original file line number	Diff line number	Diff line change
`@@ -92,7 +92,7 @@ void static RandomScript(CScript &script) {`
`92`	`92`
`93`	`93`	`void static RandomTransaction(CMutableTransaction& tx, bool fSingle)`
`94`	`94`	`{`
`95`		`- tx.nVersion = int(InsecureRand32());`
	`95`	`+ tx.nVersion = InsecureRand32();`
`96`	`96`	`tx.vin.clear();`
`97`	`97`	`tx.vout.clear();`
`98`	`98`	`tx.nLockTime = (InsecureRandBool()) ? InsecureRand32() : 0;`