diff --git a/src/bitcoin-tx.cpp b/src/bitcoin-tx.cpp index cfac50e090..c16428a0f4 100644 --- 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 index 154146f08d..1181cce680 100644 --- a/src/consensus/tx_verify.cpp +++ b/src/consensus/tx_verify.cpp @@ -48,11 +48,7 @@ std::pair 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(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 index 3a2bf865fc..c7dc3a955c 100644 --- 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(static_cast(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 index b4a860dd9e..0143aec4a5 100644 --- 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 index 976542cfae..f68be00e4e 100644 --- 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 vin * - std::vector vout * - uint32_t nLockTime * * Extended transaction serialization format: - * - int32_t nVersion + * - uint32_t nVersion * - unsigned char dummy = 0x00 * - unsigned char flags (!= 0) * - std::vector 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 @@ public: // structure, including the hash. const std::vector vin; const std::vector vout; - const int32_t nVersion; + const uint32_t nVersion; const uint32_t nLockTime; private: @@ -378,7 +378,7 @@ struct CMutableTransaction { std::vector vin; std::vector vout; - int32_t nVersion; + uint32_t nVersion; uint32_t nLockTime; explicit CMutableTransaction(); diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 2ecaeeaf40..012da77f54 100644 --- 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(psbtx.tx->nVersion) > best_version) { - best_version = static_cast(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(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 index c969ce45f1..1ebf5425a5 100644 --- a/src/script/interpreter.cpp +++ b/src/script/interpreter.cpp @@ -1743,7 +1743,7 @@ bool GenericTransactionSignatureChecker::CheckSequence(const CScriptNum& nSeq // Fail if the transaction's version number is not set high // enough to trigger BIP 68 rules. - if (static_cast(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 index 259b00fcae..a1119297f4 100644 --- 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(); + fuzzed_data_provider.ConsumeIntegral(); tx_mut.nLockTime = fuzzed_data_provider.ConsumeIntegral(); const auto num_in = fuzzed_data_provider.ConsumeIntegralInRange(0, max_num_in); const auto num_out = fuzzed_data_provider.ConsumeIntegralInRange(0, max_num_out); diff --git a/src/test/sighash_tests.cpp b/src/test/sighash_tests.cpp index d08d5519a1..d43c474ae1 100644 --- 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 index e6cf64611e..384babbde1 100644 --- 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::max(); CheckIsNotStandard(t, "version"); t.nVersion = 0; diff --git a/test/functional/feature_taproot.py b/test/functional/feature_taproot.py index e7d65b4539..210ec54329 100755 --- a/test/functional/feature_taproot.py +++ b/test/functional/feature_taproot.py @@ -1410,7 +1410,7 @@ class TaprootTest(BitcoinTestFramework): 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 index b63c2c7a8d..31f7f35cc3 100755 --- a/test/functional/p2p_segwit.py +++ b/test/functional/p2p_segwit.py @@ -1164,7 +1164,7 @@ class SegWitTest(BitcoinTestFramework): 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 @@ class SegWitTest(BitcoinTestFramework): 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 index 113424c0a6..16c038746f 100755 --- a/test/functional/rpc_packages.py +++ b/test/functional/rpc_packages.py @@ -394,7 +394,7 @@ class RPCPackagesTest(BitcoinTestFramework): 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 index 3978c80dde..e18a735fbc 100755 --- a/test/functional/rpc_rawtransaction.py +++ b/test/functional/rpc_rawtransaction.py @@ -463,9 +463,9 @@ class RawTransactionsTest(BitcoinTestFramework): 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 @@ class RawTransactionsTest(BitcoinTestFramework): 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 index 4e496a9275..db793233b8 100755 --- a/test/functional/test_framework/messages.py +++ b/test/functional/test_framework/messages.py @@ -582,7 +582,7 @@ class CTransaction: 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 @@ class CTransaction: 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 @@ class CTransaction: 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 index ab3dc2ffb1..8489e2937d 100644 --- 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)