Merge #5864: refactor: don't hardcode the transaction version

8b6c96d refactor: a new constant with Tx Version (Alessandro Rezzi)
9d429f4 refactor: drop functions from struct which supposed to be simple as possible (Alessandro Rezzi)
b2bb097 refactor: simplify vExtra using in wallet and add const (Alessandro Rezzi)
d9f0e93 refactor: use CTransaction for immutable tx in evo_assetlocks_tests (Alessandro Rezzi)
ca0fe8c refactor: introduce HasExtraPayloadField() (Alessandro Rezzi)
72d2008 refactor: introduce IsSpecialTxVersion() (Alessandro Rezzi)

Pull request description:

  ## Issue being fixed or feature implemented
  Refactor extracted from #5860. Even if that PR should not need anymore this commit (since it has been found a better way to activate dip143), I think It's still a worthy refactor

  ## What was done?
    Introduce  `tx.IsSpecialTxVersion()` in place of `tx.nVersion == 3`,  `tx.nVersion >= 3`
    and `tx.HasExtraPayloadField()` in place of  `tx.nVersion == 3 && tx.nType != TRANSACTION_NORMAL`

  ## How Has This Been Tested?

  ## Breaking Changes

  ## Checklist:
    _Go over all the following points, and put an `x` in all the boxes that apply._
  - [X] I have performed a self-review of my own code
  - [X] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  knst:
    utACK 8b6c96d

Tree-SHA512: aa16f9ee570e0fa86561cc440ffba40f6d554caa3e08b630b481732b899cc613eef596c672b5a20dbf3582ad109ffb687f4ad815f712dc16f636f8857d98480a

src/bloom.cpp

@@ -121,7 +121,7 @@ bool CBloomFilter::CheckScript(const CScript &script) const
 // wallets, etc.)
 bool CBloomFilter::CheckSpecialTransactionMatchesAndUpdate(const CTransaction &tx)
 {
-    if(tx.nVersion != 3 || tx.nType == TRANSACTION_NORMAL) {
+    if (!tx.HasExtraPayloadField()) {
         return false; // it is not a special transaction
     }
     switch(tx.nType) {

src/consensus/tx_verify.cpp

@@ -161,8 +161,7 @@ unsigned int GetTransactionSigOpCount(const CTransaction& tx, const CCoinsViewCa
 
 bool Consensus::CheckTxInputs(const CTransaction& tx, TxValidationState& state, const CCoinsViewCache& inputs, int nSpendHeight, CAmount& txfee)
 {
-
-    if (bool isAssetUnlockTx = (tx.nVersion == 3 && tx.nType ==  TRANSACTION_ASSET_UNLOCK); isAssetUnlockTx) {
+    if (bool isAssetUnlockTx = (tx.IsSpecialTxVersion() && tx.nType == TRANSACTION_ASSET_UNLOCK); isAssetUnlockTx) {
         return GetAssetUnlockFee(tx, txfee, state);
     }
 

src/evo/cbtx.cpp

@@ -261,7 +261,7 @@ bool CalcCbTxMerkleRootQuorums(const CBlock& block, const CBlockIndex* pindexPre
     for (size_t i = 1; i < block.vtx.size(); i++) {
         const auto& tx = block.vtx[i];
 
-        if (tx->nVersion == 3 && tx->nType == TRANSACTION_QUORUM_COMMITMENT) {
+        if (tx->IsSpecialTxVersion() && tx->nType == TRANSACTION_QUORUM_COMMITMENT) {
             const auto opt_qc = GetTxPayload<llmq::CFinalCommitmentTxPayload>(*tx);
             if (!opt_qc) {
                 return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-qc-payload-calc-cbtx-quorummerkleroot");

src/evo/creditpool.cpp

@@ -57,7 +57,7 @@ static UnlockDataPerBlock GetDataFromUnlockTxes(const std::vector<CTransactionRe
     UnlockDataPerBlock blockData;
 
     for (CTransactionRef tx : vtx) {
-        if (tx->nVersion != 3 || tx->nType != TRANSACTION_ASSET_UNLOCK) continue;
+        if (!tx->IsSpecialTxVersion() || tx->nType != TRANSACTION_ASSET_UNLOCK) continue;
 
         CAmount unlocked{0};
         TxValidationState tx_state;
@@ -120,7 +120,7 @@ static std::optional<CBlock> GetBlockForCreditPool(const CBlockIndex* const bloc
     assert(!block.vtx.empty());
 
     // Should not fail if V20 (DIP0027) is active but it happens for RegChain (unit tests)
-    if (block.vtx[0]->nVersion != 3) return std::nullopt;
+    if (!block.vtx[0]->IsSpecialTxVersion()) return std::nullopt;
 
     assert(!block.vtx[0]->vExtraPayload.empty());
 
@@ -269,7 +269,7 @@ bool CCreditPoolDiff::Unlock(const CTransaction& tx, TxValidationState& state)
 
 bool CCreditPoolDiff::ProcessLockUnlockTransaction(const CTransaction& tx, TxValidationState& state)
 {
-    if (tx.nVersion != 3) return true;
+    if (!tx.IsSpecialTxVersion()) return true;
     if (tx.nType != TRANSACTION_ASSET_LOCK && tx.nType != TRANSACTION_ASSET_UNLOCK) return true;
 
     if (!CheckAssetLockUnlockTx(tx, pindexPrev, pool.indexes, state)) {

src/evo/deterministicmns.cpp

@@ -739,7 +739,7 @@ bool CDeterministicMNManager::BuildNewListFromBlock(const CBlock& block, gsl::no
     for (int i = 1; i < (int)block.vtx.size(); i++) {
         const CTransaction& tx = *block.vtx[i];
 
-        if (tx.nVersion != 3) {
+        if (!tx.IsSpecialTxVersion()) {
             // only interested in special TXs
             continue;
         }
@@ -1096,7 +1096,7 @@ CDeterministicMNList CDeterministicMNManager::GetListAtChainTip()
 
 bool CDeterministicMNManager::IsProTxWithCollateral(const CTransactionRef& tx, uint32_t n)
 {
-    if (tx->nVersion != 3 || tx->nType != TRANSACTION_PROVIDER_REGISTER) {
+    if (!tx->IsSpecialTxVersion() || tx->nType != TRANSACTION_PROVIDER_REGISTER) {
         return false;
     }
     const auto opt_proTx = GetTxPayload<CProRegTx>(*tx);

src/evo/mnhftx.cpp

@@ -102,7 +102,7 @@ bool MNHFTx::Verify(const uint256& quorumHash, const uint256& requestId, const u
 
 bool CheckMNHFTx(const CTransaction& tx, const CBlockIndex* pindexPrev, TxValidationState& state)
 {
-    if (tx.nVersion != 3 || tx.nType != TRANSACTION_MNHF_SIGNAL) {
+    if (!tx.IsSpecialTxVersion() || tx.nType != TRANSACTION_MNHF_SIGNAL) {
         return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-mnhf-type");
     }
 
@@ -147,7 +147,7 @@ bool CheckMNHFTx(const CTransaction& tx, const CBlockIndex* pindexPrev, TxValida
 
 std::optional<uint8_t> extractEHFSignal(const CTransaction& tx)
 {
-    if (tx.nVersion != 3 || tx.nType != TRANSACTION_MNHF_SIGNAL) {
+    if (!tx.IsSpecialTxVersion() || tx.nType != TRANSACTION_MNHF_SIGNAL) {
         // only interested in special TXs 'TRANSACTION_MNHF_SIGNAL'
         return std::nullopt;
     }
@@ -165,7 +165,7 @@ static bool extractSignals(const CBlock& block, const CBlockIndex* const pindex,
     for (size_t i = 1; i < block.vtx.size(); ++i) {
         const CTransaction& tx = *block.vtx[i];
 
-        if (tx.nVersion != 3 || tx.nType != TRANSACTION_MNHF_SIGNAL) {
+        if (!tx.IsSpecialTxVersion() || tx.nType != TRANSACTION_MNHF_SIGNAL) {
             // only interested in special TXs 'TRANSACTION_MNHF_SIGNAL'
             continue;
         }

src/evo/specialtxman.cpp

@@ -22,7 +22,7 @@ static bool CheckSpecialTxInner(CDeterministicMNManager& dmnman, const CTransact
 {
     AssertLockHeld(cs_main);
 
-    if (tx.nVersion != 3 || tx.nType == TRANSACTION_NORMAL)
+    if (!tx.HasExtraPayloadField())
         return true;
 
     const auto& consensusParams = Params().GetConsensus();
@@ -80,7 +80,7 @@ bool CheckSpecialTx(CDeterministicMNManager& dmnman, const CTransaction& tx, con
 
 [[nodiscard]] bool CSpecialTxProcessor::ProcessSpecialTx(const CTransaction& tx, const CBlockIndex* pindex, TxValidationState& state)
 {
-    if (tx.nVersion != 3 || tx.nType == TRANSACTION_NORMAL) {
+    if (!tx.HasExtraPayloadField()) {
         return true;
     }
 
@@ -106,7 +106,7 @@ bool CheckSpecialTx(CDeterministicMNManager& dmnman, const CTransaction& tx, con
 
 [[nodiscard]] bool CSpecialTxProcessor::UndoSpecialTx(const CTransaction& tx, const CBlockIndex* pindex)
 {
-    if (tx.nVersion != 3 || tx.nType == TRANSACTION_NORMAL) {
+    if (!tx.HasExtraPayloadField()) {
         return true;
     }
 
@@ -281,7 +281,7 @@ bool CSpecialTxProcessor::CheckCreditPoolDiffForBlock(const CBlock& block, const
         // If we get there we have v20 activated and credit pool amount must be included in block CbTx
         const auto& tx = *block.vtx[0];
         assert(tx.IsCoinBase());
-        assert(tx.nVersion == 3);
+        assert(tx.IsSpecialTxVersion());
         assert(tx.nType == TRANSACTION_COINBASE);
 
         const auto opt_cbTx = GetTxPayload<CCbTx>(tx);

src/merkleblock.cpp

@@ -52,7 +52,7 @@ CMerkleBlock::CMerkleBlock(const CBlock& block, CBloomFilter* filter, const std:
     {
         const auto& tx = *block.vtx[i];
         const uint256& hash = tx.GetHash();
-        bool isAllowedType = tx.nVersion != 3 || allowedTxTypes.count(tx.nType) != 0;
+        bool isAllowedType = !tx.IsSpecialTxVersion() || allowedTxTypes.count(tx.nType) != 0;
 
         if (txids && txids->count(hash)) {
             vMatch.push_back(true);

src/primitives/transaction.h

@@ -184,12 +184,14 @@ class CTransaction
 public:
     // Default transaction version.
     static const int32_t CURRENT_VERSION=2;
+    // Special transaction version
+    static const int32_t SPECIAL_VERSION = 3;
 
     // Changing the default transaction version requires a two step process: first
     // adapting relay policy by bumping MAX_STANDARD_VERSION, and then later date
     // bumping the default CURRENT_VERSION at which point both CURRENT_VERSION and
     // MAX_STANDARD_VERSION will be equal.
-    static const int32_t MAX_STANDARD_VERSION=3;
+    static const int32_t MAX_STANDARD_VERSION = SPECIAL_VERSION;
 
     // The local variables are made const to prevent unintended modification
     // without updating the cached hash value. However, CTransaction is not
@@ -224,7 +226,7 @@ class CTransaction
         s << vin;
         s << vout;
         s << nLockTime;
-        if (this->nVersion == 3 && this->nType != TRANSACTION_NORMAL)
+        if (this->HasExtraPayloadField())
             s << vExtraPayload;
     }
 
@@ -265,6 +267,16 @@ class CTransaction
     }
 
     std::string ToString() const;
+
+    bool IsSpecialTxVersion() const noexcept
+    {
+        return nVersion >= SPECIAL_VERSION;
+    }
+
+    bool HasExtraPayloadField() const noexcept
+    {
+        return IsSpecialTxVersion() && nType != TRANSACTION_NORMAL;
+    }
 };
 
 /** A mutable version of CTransaction. */
@@ -288,7 +300,7 @@ struct CMutableTransaction
         SER_READ(obj, obj.nVersion = (int16_t) (n32bitVersion & 0xffff));
         SER_READ(obj, obj.nType = (uint16_t) ((n32bitVersion >> 16) & 0xffff));
         READWRITE(obj.vin, obj.vout, obj.nLockTime);
-        if (obj.nVersion == 3 && obj.nType != TRANSACTION_NORMAL) {
+        if (obj.nVersion >= CTransaction::SPECIAL_VERSION && obj.nType != TRANSACTION_NORMAL) {
             READWRITE(obj.vExtraPayload);
         }
     }

src/rpc/blockchain.cpp

@@ -2564,9 +2564,9 @@ static RPCHelpMan getspecialtxes()
 
     for(const auto& tx : block.vtx)
     {
-        if (tx->nVersion != 3 || tx->nType == TRANSACTION_NORMAL // ensure it's in fact a special tx
+        if (!tx->HasExtraPayloadField()                   // ensure it's in fact a special tx
             || (nTxType != -1 && tx->nType != nTxType)) { // ensure special tx type matches filter, if given
-                continue;
+            continue;
         }
 
         nTxNum++;

src/script/interpreter.cpp

@@ -1476,7 +1476,7 @@ class CTransactionSignatureSerializer
              SerializeOutput(s, nOutput);
         // Serialize nLockTime
         ::Serialize(s, txTo.nLockTime);
-        if (txTo.nVersion == 3 && txTo.nType != TRANSACTION_NORMAL)
+        if (txTo.nVersion >= CTransaction::SPECIAL_VERSION && txTo.nType != TRANSACTION_NORMAL)
             ::Serialize(s, txTo.vExtraPayload);
     }
 };

src/test/evo_assetlocks_tests.cpp

@@ -134,7 +134,7 @@ BOOST_FIXTURE_TEST_CASE(evo_assetlock, TestChain100Setup)
     CKey key;
     key.MakeNewKey(true);
 
-    const CMutableTransaction tx = CreateAssetLockTx(keystore, coins, key);
+    const CTransaction tx = CreateAssetLockTx(keystore, coins, key);
     std::string reason;
     BOOST_CHECK(IsStandardTx(CTransaction(tx), reason));
 
@@ -149,7 +149,7 @@ BOOST_FIXTURE_TEST_CASE(evo_assetlock, TestChain100Setup)
 
     // Check version
     {
-        BOOST_CHECK(tx.nVersion == 3);
+        BOOST_CHECK(tx.IsSpecialTxVersion());
 
         const auto opt_payload = GetTxPayload<CAssetLockPayload>(tx);
 
@@ -159,7 +159,7 @@ BOOST_FIXTURE_TEST_CASE(evo_assetlock, TestChain100Setup)
 
     {
         // Wrong type "Asset Unlock TX" instead "Asset Lock TX"
-        CMutableTransaction txWrongType = tx;
+        CMutableTransaction txWrongType(tx);
         txWrongType.nType = TRANSACTION_ASSET_UNLOCK;
         BOOST_CHECK(!CheckAssetLockTx(CTransaction(txWrongType), tx_state));
         BOOST_CHECK(tx_state.GetRejectReason() == "bad-assetlocktx-type");
@@ -175,13 +175,13 @@ BOOST_FIXTURE_TEST_CASE(evo_assetlock, TestChain100Setup)
         BOOST_CHECK(inSum == outSum);
 
         // Outputs should not be bigger than inputs
-        CMutableTransaction txBigOutput = tx;
+        CMutableTransaction txBigOutput(tx);
         txBigOutput.vout[0].nValue += 1;
         BOOST_CHECK(!CheckAssetLockTx(CTransaction(txBigOutput), tx_state));
         BOOST_CHECK(tx_state.GetRejectReason() == "bad-assetlocktx-creditamount");
 
         // Smaller outputs are allown
-        CMutableTransaction txSmallOutput = tx;
+        CMutableTransaction txSmallOutput(tx);
         txSmallOutput.vout[1].nValue -= 1;
         BOOST_CHECK(CheckAssetLockTx(CTransaction(txSmallOutput), tx_state));
     }
@@ -195,7 +195,7 @@ BOOST_FIXTURE_TEST_CASE(evo_assetlock, TestChain100Setup)
         wrongOutput[0].nValue += CENT;
         CAssetLockPayload greaterCreditsPayload(wrongOutput);
 
-        CMutableTransaction txGreaterCredits = tx;
+        CMutableTransaction txGreaterCredits(tx);
         SetTxPayload(txGreaterCredits, greaterCreditsPayload);
 
         BOOST_CHECK(!CheckAssetLockTx(CTransaction(txGreaterCredits), tx_state));
@@ -205,7 +205,7 @@ BOOST_FIXTURE_TEST_CASE(evo_assetlock, TestChain100Setup)
         wrongOutput[1].nValue -= 2 * CENT;
         CAssetLockPayload lessCreditsPayload(wrongOutput);
 
-        CMutableTransaction txLessCredits = tx;
+        CMutableTransaction txLessCredits(tx);
         SetTxPayload(txLessCredits, lessCreditsPayload);
 
         BOOST_CHECK(!CheckAssetLockTx(CTransaction(txLessCredits), tx_state));
@@ -218,7 +218,7 @@ BOOST_FIXTURE_TEST_CASE(evo_assetlock, TestChain100Setup)
         creditOutputsOutOfRange[0].nValue = 0;
         CAssetLockPayload invalidOutputsPayload(creditOutputsOutOfRange);
 
-        CMutableTransaction txInvalidOutputs = tx;
+        CMutableTransaction txInvalidOutputs(tx);
         SetTxPayload(txInvalidOutputs, invalidOutputsPayload);
 
         BOOST_CHECK(!CheckAssetLockTx(CTransaction(txInvalidOutputs), tx_state));
@@ -243,7 +243,7 @@ BOOST_FIXTURE_TEST_CASE(evo_assetlock, TestChain100Setup)
         creditOutputsNotPubkey[0].scriptPubKey = CScript() << OP_1;
         CAssetLockPayload notPubkeyPayload(creditOutputsNotPubkey);
 
-        CMutableTransaction txNotPubkey = tx;
+        CMutableTransaction txNotPubkey(tx);
         SetTxPayload(txNotPubkey, notPubkeyPayload);
 
         BOOST_CHECK(!CheckAssetLockTx(CTransaction(txNotPubkey), tx_state));
@@ -253,7 +253,7 @@ BOOST_FIXTURE_TEST_CASE(evo_assetlock, TestChain100Setup)
 
     {
         // OP_RETURN must be only one, not more
-        CMutableTransaction txMultipleReturn = tx;
+        CMutableTransaction txMultipleReturn(tx);
         txMultipleReturn.vout[1].scriptPubKey = CScript() << OP_RETURN << ParseHex("");
 
         BOOST_CHECK(!CheckAssetLockTx(CTransaction(txMultipleReturn), tx_state));
@@ -263,7 +263,7 @@ BOOST_FIXTURE_TEST_CASE(evo_assetlock, TestChain100Setup)
 
     {
         // zero/negative OP_RETURN
-        CMutableTransaction txReturnOutOfRange = tx;
+        CMutableTransaction txReturnOutOfRange(tx);
         txReturnOutOfRange.vout[0].nValue = 0;
 
         BOOST_CHECK(!CheckAssetLockTx(CTransaction(txReturnOutOfRange), tx_state));
@@ -278,7 +278,7 @@ BOOST_FIXTURE_TEST_CASE(evo_assetlock, TestChain100Setup)
 
     {
         // OP_RETURN is missing
-        CMutableTransaction txNoReturn = tx;
+        CMutableTransaction txNoReturn(tx);
         txNoReturn.vout[0].scriptPubKey = GetScriptForDestination(PKHash(key.GetPubKey()));
 
         BOOST_CHECK(!CheckAssetLockTx(CTransaction(txNoReturn), tx_state));
@@ -287,7 +287,7 @@ BOOST_FIXTURE_TEST_CASE(evo_assetlock, TestChain100Setup)
 
     {
         // OP_RETURN should not have any data
-        CMutableTransaction txReturnData = tx;
+        CMutableTransaction txReturnData(tx);
         txReturnData.vout[0].scriptPubKey = CScript() << OP_RETURN << ParseHex("abc");
 
         BOOST_CHECK(!CheckAssetLockTx(CTransaction(txReturnData), tx_state));
@@ -303,7 +303,7 @@ BOOST_FIXTURE_TEST_CASE(evo_assetunlock, TestChain100Setup)
     CKey key;
     key.MakeNewKey(true);
 
-    const CMutableTransaction tx = CreateAssetUnlockTx(keystore, key);
+    const CTransaction tx = CreateAssetUnlockTx(keystore, key);
     std::string reason;
     BOOST_CHECK(IsStandardTx(CTransaction(tx), reason));
 
@@ -322,7 +322,7 @@ BOOST_FIXTURE_TEST_CASE(evo_assetunlock, TestChain100Setup)
         CCoinsViewCache coins(&coinsDummy);
         std::vector<CMutableTransaction> dummyTransactions = SetupDummyInputs(keystore, coins);
 
-        CMutableTransaction txNonemptyInput = tx;
+        CMutableTransaction txNonemptyInput(tx);
         txNonemptyInput.vin.resize(1);
         txNonemptyInput.vin[0].prevout.hash = dummyTransactions[0].GetHash();
         txNonemptyInput.vin[0].prevout.n = 1;
@@ -344,21 +344,21 @@ BOOST_FIXTURE_TEST_CASE(evo_assetunlock, TestChain100Setup)
         BOOST_CHECK(unlockPayload->getIndex() == 0x001122334455667788L);
 
         // Wrong type "Asset Lock TX" instead "Asset Unlock TX"
-        CMutableTransaction txWrongType = tx;
+        CMutableTransaction txWrongType(tx);
         txWrongType.nType = TRANSACTION_ASSET_LOCK;
         BOOST_CHECK(!CheckAssetUnlockTx(CTransaction(txWrongType), block_index, std::nullopt, tx_state));
         BOOST_CHECK(tx_state.GetRejectReason() == "bad-assetunlocktx-type");
 
         // Check version of tx and payload
-        BOOST_CHECK(tx.nVersion == 3);
+        BOOST_CHECK(tx.IsSpecialTxVersion());
         for (uint8_t payload_version : {0, 1, 2, 255}) {
             CAssetUnlockPayload unlockPayload_tmp{payload_version,
                 unlockPayload->getIndex(),
                 unlockPayload->getFee(),
                 unlockPayload->getRequestedHeight(),
                 unlockPayload->getQuorumHash(),
                 unlockPayload->getQuorumSig()};
-            CMutableTransaction txWrongVersion = tx;
+            CMutableTransaction txWrongVersion(tx);
             SetTxPayload(txWrongVersion, unlockPayload_tmp);
             if (payload_version != 1) {
                 BOOST_CHECK(!CheckAssetUnlockTx(CTransaction(txWrongVersion), block_index, std::nullopt, tx_state));
@@ -372,7 +372,7 @@ BOOST_FIXTURE_TEST_CASE(evo_assetunlock, TestChain100Setup)
 
     {
         // Exactly 32 withdrawal is fine
-        CMutableTransaction txManyOutputs = tx;
+        CMutableTransaction txManyOutputs(tx);
         int outputsLimit = 32;
         txManyOutputs.vout.resize(outputsLimit);
         for (auto& out : txManyOutputs.vout) {

src/test/util/setup_common.cpp

@@ -388,7 +388,7 @@ CBlock TestChainSetup::CreateBlock(const std::vector<CMutableTransaction>& txns,
 
     std::vector<CTransactionRef> llmqCommitments;
     for (const auto& tx : block.vtx) {
-        if (tx->nVersion == 3 && tx->nType == TRANSACTION_QUORUM_COMMITMENT) {
+        if (tx->IsSpecialTxVersion() && tx->nType == TRANSACTION_QUORUM_COMMITMENT) {
             llmqCommitments.emplace_back(tx);
         }
     }