From 72d2008901b28d4761cd7543ca60ba441e42c027 Mon Sep 17 00:00:00 2001 From: Alessandro Rezzi Date: Fri, 2 Feb 2024 13:15:13 +0100 Subject: [PATCH] refactor: introduce IsSpecialTxVersion() Instead of hardcoding the tx version --- src/bloom.cpp | 2 +- src/consensus/tx_verify.cpp | 3 +-- src/evo/cbtx.cpp | 2 +- src/evo/creditpool.cpp | 6 +++--- src/evo/deterministicmns.cpp | 4 ++-- src/evo/mnhftx.cpp | 6 +++--- src/evo/specialtxman.cpp | 8 ++++---- src/merkleblock.cpp | 2 +- src/primitives/transaction.h | 14 ++++++++++++-- src/rpc/blockchain.cpp | 4 ++-- src/script/interpreter.cpp | 2 +- src/test/evo_assetlocks_tests.cpp | 4 ++-- src/test/util/setup_common.cpp | 2 +- src/validation.cpp | 6 +++--- src/wallet/wallet.cpp | 2 +- 15 files changed, 38 insertions(+), 29 deletions(-) diff --git a/src/bloom.cpp b/src/bloom.cpp index 73adaab8ca..aaf0451a5e 100644 --- a/src/bloom.cpp +++ b/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.IsSpecialTxVersion() || tx.nType == TRANSACTION_NORMAL) { return false; // it is not a special transaction } switch(tx.nType) { diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp index d3940d2202..05708c11f0 100644 --- a/src/consensus/tx_verify.cpp +++ b/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); } diff --git a/src/evo/cbtx.cpp b/src/evo/cbtx.cpp index 846381b3c7..7c4201bfbd 100644 --- a/src/evo/cbtx.cpp +++ b/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(*tx); if (!opt_qc) { return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-qc-payload-calc-cbtx-quorummerkleroot"); diff --git a/src/evo/creditpool.cpp b/src/evo/creditpool.cpp index 1b7b83054e..2e2648df49 100644 --- a/src/evo/creditpool.cpp +++ b/src/evo/creditpool.cpp @@ -57,7 +57,7 @@ static UnlockDataPerBlock GetDataFromUnlockTxes(const std::vectornVersion != 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 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)) { diff --git a/src/evo/deterministicmns.cpp b/src/evo/deterministicmns.cpp index 3d02770382..dff76f4fb5 100644 --- a/src/evo/deterministicmns.cpp +++ b/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(*tx); diff --git a/src/evo/mnhftx.cpp b/src/evo/mnhftx.cpp index a42456772e..8919456360 100644 --- a/src/evo/mnhftx.cpp +++ b/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 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; } diff --git a/src/evo/specialtxman.cpp b/src/evo/specialtxman.cpp index 16171c8dcf..1f3629b3d5 100644 --- a/src/evo/specialtxman.cpp +++ b/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.IsSpecialTxVersion() || tx.nType == TRANSACTION_NORMAL) 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.IsSpecialTxVersion() || tx.nType == TRANSACTION_NORMAL) { 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.IsSpecialTxVersion() || tx.nType == TRANSACTION_NORMAL) { 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(tx); diff --git a/src/merkleblock.cpp b/src/merkleblock.cpp index 2495d707c0..9f3cf15c4a 100644 --- a/src/merkleblock.cpp +++ b/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); diff --git a/src/primitives/transaction.h b/src/primitives/transaction.h index 3603ba8431..ae5310c30f 100644 --- a/src/primitives/transaction.h +++ b/src/primitives/transaction.h @@ -224,7 +224,7 @@ public: s << vin; s << vout; s << nLockTime; - if (this->nVersion == 3 && this->nType != TRANSACTION_NORMAL) + if (this->IsSpecialTxVersion() && this->nType != TRANSACTION_NORMAL) s << vExtraPayload; } @@ -265,6 +265,11 @@ public: } std::string ToString() const; + + bool IsSpecialTxVersion() const noexcept + { + return nVersion >= 3; + } }; /** A mutable version of CTransaction. */ @@ -288,7 +293,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.IsSpecialTxVersion() && obj.nType != TRANSACTION_NORMAL) { READWRITE(obj.vExtraPayload); } } @@ -304,6 +309,11 @@ struct CMutableTransaction uint256 GetHash() const; std::string ToString() const; + + bool IsSpecialTxVersion() const + { + return nVersion >= 3; + } }; typedef std::shared_ptr CTransactionRef; diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 1c0e61d3a0..71fa7ba966 100644 --- a/src/rpc/blockchain.cpp +++ b/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->IsSpecialTxVersion() || tx->nType == TRANSACTION_NORMAL // ensure it's in fact a special tx || (nTxType != -1 && tx->nType != nTxType)) { // ensure special tx type matches filter, if given - continue; + continue; } nTxNum++; diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp index 63f34a09cd..f349adbadc 100644 --- a/src/script/interpreter.cpp +++ b/src/script/interpreter.cpp @@ -1476,7 +1476,7 @@ public: SerializeOutput(s, nOutput); // Serialize nLockTime ::Serialize(s, txTo.nLockTime); - if (txTo.nVersion == 3 && txTo.nType != TRANSACTION_NORMAL) + if (txTo.IsSpecialTxVersion() && txTo.nType != TRANSACTION_NORMAL) ::Serialize(s, txTo.vExtraPayload); } }; diff --git a/src/test/evo_assetlocks_tests.cpp b/src/test/evo_assetlocks_tests.cpp index 97d354acf3..b564ffe056 100644 --- a/src/test/evo_assetlocks_tests.cpp +++ b/src/test/evo_assetlocks_tests.cpp @@ -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(tx); @@ -350,7 +350,7 @@ BOOST_FIXTURE_TEST_CASE(evo_assetunlock, TestChain100Setup) 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(), diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 1801022ed9..cf6f3a4144 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -387,7 +387,7 @@ CBlock TestChainSetup::CreateBlock(const std::vector& txns, std::vector 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); } } diff --git a/src/validation.cpp b/src/validation.cpp index 3387f746b9..11696deec4 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -376,7 +376,7 @@ static bool ContextualCheckTransaction(const CTransaction& tx, TxValidationState if (fDIP0003Active_context) { // check version 3 transaction types - if (tx.nVersion >= 3) { + if (tx.IsSpecialTxVersion()) { if (tx.nType != TRANSACTION_NORMAL && tx.nType != TRANSACTION_PROVIDER_REGISTER && tx.nType != TRANSACTION_PROVIDER_UPDATE_SERVICE && @@ -657,7 +657,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) if (!ContextualCheckTransaction(tx, state, chainparams.GetConsensus(), m_active_chainstate.m_chain.Tip())) return error("%s: ContextualCheckTransaction: %s, %s", __func__, hash.ToString(), state.ToString()); - if (tx.nVersion == 3 && tx.nType == TRANSACTION_QUORUM_COMMITMENT) { + if (tx.IsSpecialTxVersion() && tx.nType == TRANSACTION_QUORUM_COMMITMENT) { // quorum commitment is not allowed outside of blocks return state.Invalid(TxValidationResult::TX_CONSENSUS, "qc-not-allowed"); } @@ -800,7 +800,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // blocks // Checking of fee for MNHF_SIGNAL should be skipped: mnhf does not have // inputs, outputs, or fee - if (tx.nVersion != 3 || tx.nType != TRANSACTION_MNHF_SIGNAL) { + if (!tx.IsSpecialTxVersion() || tx.nType != TRANSACTION_MNHF_SIGNAL) { if (!bypass_limits && !CheckFeeRate(nSize, nModifiedFees, state)) return false; } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index e106d5ef72..014a6a7cde 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3056,7 +3056,7 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC LOCK(cs_wallet); int nExtraPayloadSize = 0; - if (tx.nVersion == 3 && tx.nType != TRANSACTION_NORMAL) + if (tx.IsSpecialTxVersion() && tx.nType != TRANSACTION_NORMAL) nExtraPayloadSize = (int)tx.vExtraPayload.size(); CTransactionRef tx_new;