Merge bitcoin/bitcoin#29325: consensus: Store transaction nVersion as uint32_t

429ec1a refactor: Rename CTransaction::nVersion to version (Ava Chow)
27e70f1 consensus: Store transaction nVersion as uint32_t (Ava Chow)

Pull request description:

  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 everywhere it is used and displayed.

  Since a few alternative implementations have recently been revealed to have made an error with this signedness that would have resulted in consensus failure, I think it makes sense for us to just make this always unsigned to make it clear that the version is treated as unsigned. This would also help us avoid future potential issues with signedness of this value.

  I believe that this is safe and does not actually change what transactions would or would not be considered both standard and consensus valid. Within consensus, the only use of the version in consensus is in BIP68 validation which was already casting it to uint32_t. Within policy, although it is used as a signed int for the transaction version number check, I do not think that this change would change standardness. Standard transactions are limited to the range [1, 2]. Negative numbers would have fallen under the < 1 condition, but by making it unsigned, they are still non-standard under the > 2 condition.

  Unsigned and signed ints are serialized and unserialized the same way so there is no change in serialization.

ACKs for top commit:
  maflcko:
    ACK 429ec1a 🐿
  glozow:
    ACK 429ec1a
  shaavan:
    ACK 429ec1a 💯

Tree-SHA512: 0bcd92a245d7d16c3665d2d4e815a4ef28207ad4a1fb46c6f0203cdafeab1b82c4e95e4bdce7805d80a4f4a46074f6542abad708e970550d38a00d759e3dcef1

Author: fanquake

contrib/signet/miner

@@ -58,14 +58,14 @@ def signet_txs(block, challenge):
     sd += block.nTime.to_bytes(4, "little")
 
     to_spend = CTransaction()
-    to_spend.nVersion = 0
+    to_spend.version = 0
     to_spend.nLockTime = 0
     to_spend.vin = [CTxIn(COutPoint(0, 0xFFFFFFFF), b"\x00" + CScriptOp.encode_op_pushdata(sd), 0)]
     to_spend.vout = [CTxOut(0, challenge)]
     to_spend.rehash()
 
     spend = CTransaction()
-    spend.nVersion = 0
+    spend.version = 0
     spend.nLockTime = 0
     spend.vin = [CTxIn(COutPoint(to_spend.sha256, 0), b"", 0)]
     spend.vout = [CTxOut(0, b"\x6a")]

doc/policy/mempool-replacements.md

@@ -12,7 +12,7 @@ other consensus and policy rules, each of the following conditions are met:
 
 1. The directly conflicting transactions all signal replaceability explicitly. A transaction is
    signaling BIP125 replaceability if any of its inputs have an nSequence number less than (0xffffffff - 1).
-   A transaction also signals replaceability if its nVersion field is set to 3.
+   A transaction also signals replaceability if its version field is set to 3.
 
    *Rationale*: See [BIP125
    explanation](https://github.com/bitcoin/bips/blob/master/bip-0125.mediawiki#motivation).

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.version = newVersion;
 }
 
 static void MutateTxLocktime(CMutableTransaction& tx, const std::string& cmdVal)

src/compressor.h

@@ -55,8 +55,8 @@ struct ScriptCompression
 {
     /**
      * make this static for now (there are only 6 special scripts defined)
-     * this can potentially be extended together with a new nVersion for
-     * transactions, in which case this value becomes dependent on nVersion
+     * this can potentially be extended together with a new version for
+     * transactions, in which case this value becomes dependent on version
      * and nHeight of the enclosing transaction.
      */
     static const unsigned int nSpecialScripts = 6;

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.version >= 2 && flags & LOCKTIME_VERIFY_SEQUENCE;
 
     // Do not enforce sequence numbers as a relative lock time
     // unless we have been instructed to

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.version);
     entry.pushKV("size", tx.GetTotalSize());
     entry.pushKV("vsize", (GetTransactionWeight(tx) + WITNESS_SCALE_FACTOR - 1) / WITNESS_SCALE_FACTOR);
     entry.pushKV("weight", GetTransactionWeight(tx));

src/kernel/chainparams.cpp

@@ -29,7 +29,7 @@
 static CBlock CreateGenesisBlock(const char* pszTimestamp, const CScript& genesisOutputScript, uint32_t nTime, uint32_t nNonce, uint32_t nBits, int32_t nVersion, const CAmount& genesisReward)
 {
     CMutableTransaction txNew;
-    txNew.nVersion = 1;
+    txNew.version = 1;
     txNew.vin.resize(1);
     txNew.vout.resize(1);
     txNew.vin[0].scriptSig = CScript() << 486604799 << CScriptNum(4) << std::vector<unsigned char>((const unsigned char*)pszTimestamp, (const unsigned char*)pszTimestamp + strlen(pszTimestamp));

src/policy/policy.cpp

@@ -93,7 +93,7 @@ bool IsStandard(const CScript& scriptPubKey, const std::optional<unsigned>& max_
 
 bool IsStandardTx(const CTransaction& tx, const std::optional<unsigned>& max_datacarrier_bytes, bool permit_bare_multisig, const CFeeRate& dust_relay_fee, std::string& reason)
 {
-    if (tx.nVersion > TX_MAX_STANDARD_VERSION || tx.nVersion < 1) {
+    if (tx.version > TX_MAX_STANDARD_VERSION || tx.version < 1) {
         reason = "version";
         return false;
     }

src/policy/policy.h

@@ -131,7 +131,7 @@ bool IsStandard(const CScript& scriptPubKey, const std::optional<unsigned>& max_
 // Changing the default transaction version requires a two step process: first
 // adapting relay policy by bumping TX_MAX_STANDARD_VERSION, and then later
 // allowing the new transaction version in the wallet/RPC.
-static constexpr decltype(CTransaction::nVersion) TX_MAX_STANDARD_VERSION{3};
+static constexpr decltype(CTransaction::version) TX_MAX_STANDARD_VERSION{3};
 
 /**
 * Check for standard transaction types

src/policy/v3_policy.cpp

@@ -43,13 +43,13 @@ struct ParentInfo {
     const Txid& m_txid;
     /** Wtxid used for debug string */
     const Wtxid& m_wtxid;
-    /** nVersion used to check inheritance of v3 and non-v3 */
-    decltype(CTransaction::nVersion) m_version;
+    /** version used to check inheritance of v3 and non-v3 */
+    decltype(CTransaction::version) m_version;
     /** If parent is in mempool, whether it has any descendants in mempool. */
     bool m_has_mempool_descendant;
 
     ParentInfo() = delete;
-    ParentInfo(const Txid& txid, const Wtxid& wtxid, decltype(CTransaction::nVersion) version, bool has_mempool_descendant) :
+    ParentInfo(const Txid& txid, const Wtxid& wtxid, decltype(CTransaction::version) version, bool has_mempool_descendant) :
         m_txid{txid}, m_wtxid{wtxid}, m_version{version},
         m_has_mempool_descendant{has_mempool_descendant}
     {}
@@ -66,7 +66,7 @@ std::optional<std::string> PackageV3Checks(const CTransactionRef& ptx, int64_t v
     const auto in_package_parents{FindInPackageParents(package, ptx)};
 
     // Now we have all ancestors, so we can start checking v3 rules.
-    if (ptx->nVersion == TRUC_VERSION) {
+    if (ptx->version == TRUC_VERSION) {
         // SingleV3Checks should have checked this already.
         if (!Assume(vsize <= V3_MAX_VSIZE)) {
             return strprintf("v3 tx %s (wtxid=%s) is too big: %u > %u virtual bytes",
@@ -94,14 +94,14 @@ std::optional<std::string> PackageV3Checks(const CTransactionRef& ptx, int64_t v
                     Assume(mempool_parent->GetCountWithDescendants() == 1);
                     return ParentInfo{mempool_parent->GetTx().GetHash(),
                                       mempool_parent->GetTx().GetWitnessHash(),
-                                      mempool_parent->GetTx().nVersion,
+                                      mempool_parent->GetTx().version,
                                       /*has_mempool_descendant=*/mempool_parent->GetCountWithDescendants() > 1};
                 } else {
                     auto& parent_index = in_package_parents.front();
                     auto& package_parent = package.at(parent_index);
                     return ParentInfo{package_parent->GetHash(),
                                       package_parent->GetWitnessHash(),
-                                      package_parent->nVersion,
+                                      package_parent->version,
                                       /*has_mempool_descendant=*/false};
                 }
             }();
@@ -146,14 +146,14 @@ std::optional<std::string> PackageV3Checks(const CTransactionRef& ptx, int64_t v
     } else {
         // Non-v3 transactions cannot have v3 parents.
         for (auto it : mempool_ancestors) {
-            if (it->GetTx().nVersion == TRUC_VERSION) {
+            if (it->GetTx().version == TRUC_VERSION) {
                 return strprintf("non-v3 tx %s (wtxid=%s) cannot spend from v3 tx %s (wtxid=%s)",
                                  ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(),
                                  it->GetSharedTx()->GetHash().ToString(), it->GetSharedTx()->GetWitnessHash().ToString());
             }
         }
         for (const auto& index: in_package_parents) {
-            if (package.at(index)->nVersion == TRUC_VERSION) {
+            if (package.at(index)->version == TRUC_VERSION) {
                 return strprintf("non-v3 tx %s (wtxid=%s) cannot spend from v3 tx %s (wtxid=%s)",
                                  ptx->GetHash().ToString(),
                                  ptx->GetWitnessHash().ToString(),
@@ -172,12 +172,12 @@ std::optional<std::pair<std::string, CTransactionRef>> SingleV3Checks(const CTra
 {
     // Check v3 and non-v3 inheritance.
     for (const auto& entry : mempool_ancestors) {
-        if (ptx->nVersion != TRUC_VERSION && entry->GetTx().nVersion == TRUC_VERSION) {
+        if (ptx->version != TRUC_VERSION && entry->GetTx().version == TRUC_VERSION) {
             return std::make_pair(strprintf("non-v3 tx %s (wtxid=%s) cannot spend from v3 tx %s (wtxid=%s)",
                              ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(),
                              entry->GetSharedTx()->GetHash().ToString(), entry->GetSharedTx()->GetWitnessHash().ToString()),
                 nullptr);
-        } else if (ptx->nVersion == TRUC_VERSION && entry->GetTx().nVersion != TRUC_VERSION) {
+        } else if (ptx->version == TRUC_VERSION && entry->GetTx().version != TRUC_VERSION) {
             return std::make_pair(strprintf("v3 tx %s (wtxid=%s) cannot spend from non-v3 tx %s (wtxid=%s)",
                              ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(),
                              entry->GetSharedTx()->GetHash().ToString(), entry->GetSharedTx()->GetWitnessHash().ToString()),
@@ -189,8 +189,8 @@ std::optional<std::pair<std::string, CTransactionRef>> SingleV3Checks(const CTra
     static_assert(V3_ANCESTOR_LIMIT == 2);
     static_assert(V3_DESCENDANT_LIMIT == 2);
 
-    // The rest of the rules only apply to transactions with nVersion=3.
-    if (ptx->nVersion != TRUC_VERSION) return std::nullopt;
+    // The rest of the rules only apply to transactions with version=3.
+    if (ptx->version != TRUC_VERSION) return std::nullopt;
 
     if (vsize > V3_MAX_VSIZE) {
         return std::make_pair(strprintf("v3 tx %s (wtxid=%s) is too big: %u > %u virtual bytes",