From 72d2008901b28d4761cd7543ca60ba441e42c027 Mon Sep 17 00:00:00 2001 From: Alessandro Rezzi Date: Fri, 2 Feb 2024 13:15:13 +0100 Subject: [PATCH 1/6] 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; From ca0fe8c2084271d58794f2dcd551fc10ce1fbf69 Mon Sep 17 00:00:00 2001 From: Alessandro Rezzi Date: Thu, 8 Feb 2024 18:31:39 +0100 Subject: [PATCH 2/6] refactor: introduce HasExtraPayloadField() --- src/bloom.cpp | 2 +- src/evo/specialtxman.cpp | 6 +++--- src/primitives/transaction.h | 14 ++++++++++++-- src/rpc/blockchain.cpp | 2 +- src/script/interpreter.cpp | 2 +- src/wallet/wallet.cpp | 2 +- 6 files changed, 19 insertions(+), 9 deletions(-) diff --git a/src/bloom.cpp b/src/bloom.cpp index aaf0451a5e..7048cd9f58 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.IsSpecialTxVersion() || tx.nType == TRANSACTION_NORMAL) { + if (!tx.HasExtraPayloadField()) { return false; // it is not a special transaction } switch(tx.nType) { diff --git a/src/evo/specialtxman.cpp b/src/evo/specialtxman.cpp index 1f3629b3d5..0bcd52dda1 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.IsSpecialTxVersion() || 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.IsSpecialTxVersion() || 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.IsSpecialTxVersion() || tx.nType == TRANSACTION_NORMAL) { + if (!tx.HasExtraPayloadField()) { return true; } diff --git a/src/primitives/transaction.h b/src/primitives/transaction.h index ae5310c30f..da3c9f6a0d 100644 --- a/src/primitives/transaction.h +++ b/src/primitives/transaction.h @@ -224,7 +224,7 @@ public: s << vin; s << vout; s << nLockTime; - if (this->IsSpecialTxVersion() && this->nType != TRANSACTION_NORMAL) + if (this->HasExtraPayloadField()) s << vExtraPayload; } @@ -270,6 +270,11 @@ public: { return nVersion >= 3; } + + bool HasExtraPayloadField() const noexcept + { + return IsSpecialTxVersion() && nType != TRANSACTION_NORMAL; + } }; /** A mutable version of CTransaction. */ @@ -293,7 +298,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.IsSpecialTxVersion() && obj.nType != TRANSACTION_NORMAL) { + if (obj.HasExtraPayloadField()) { READWRITE(obj.vExtraPayload); } } @@ -314,6 +319,11 @@ struct CMutableTransaction { return nVersion >= 3; } + + bool HasExtraPayloadField() const + { + return IsSpecialTxVersion() && nType != TRANSACTION_NORMAL; + } }; typedef std::shared_ptr CTransactionRef; diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 71fa7ba966..26b6b3746b 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -2564,7 +2564,7 @@ static RPCHelpMan getspecialtxes() for(const auto& tx : block.vtx) { - if (!tx->IsSpecialTxVersion() || 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; } diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp index f349adbadc..07766b84fa 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.IsSpecialTxVersion() && txTo.nType != TRANSACTION_NORMAL) + if (txTo.HasExtraPayloadField()) ::Serialize(s, txTo.vExtraPayload); } }; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 014a6a7cde..53163d5758 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.IsSpecialTxVersion() && tx.nType != TRANSACTION_NORMAL) + if (tx.HasExtraPayloadField()) nExtraPayloadSize = (int)tx.vExtraPayload.size(); CTransactionRef tx_new; From d9f0e93498114b6580977260d114f0fdac89f82d Mon Sep 17 00:00:00 2001 From: Alessandro Rezzi Date: Mon, 4 Mar 2024 21:00:44 +0100 Subject: [PATCH 3/6] refactor: use CTransaction for immutable tx in evo_assetlocks_tests --- src/test/evo_assetlocks_tests.cpp | 34 +++++++++++++++---------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/src/test/evo_assetlocks_tests.cpp b/src/test/evo_assetlocks_tests.cpp index b564ffe056..5d1ac8622c 100644 --- a/src/test/evo_assetlocks_tests.cpp +++ b/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)); @@ -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 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,7 +344,7 @@ 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"); @@ -358,7 +358,7 @@ BOOST_FIXTURE_TEST_CASE(evo_assetunlock, TestChain100Setup) 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) { From b2bb097197e90c471d34c84a0f42564ff66d54cb Mon Sep 17 00:00:00 2001 From: Alessandro Rezzi Date: Mon, 4 Mar 2024 21:01:37 +0100 Subject: [PATCH 4/6] refactor: simplify vExtra using in wallet and add const --- src/wallet/wallet.cpp | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 53163d5758..fe797385f4 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3055,12 +3055,8 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC // CreateTransaction call and LockCoin calls (when lockUnspents is true). LOCK(cs_wallet); - int nExtraPayloadSize = 0; - if (tx.HasExtraPayloadField()) - nExtraPayloadSize = (int)tx.vExtraPayload.size(); - CTransactionRef tx_new; - if (!CreateTransaction(vecSend, tx_new, nFeeRet, nChangePosInOut, error, coinControl, false, nExtraPayloadSize)) { + if (!CreateTransaction(vecSend, tx_new, nFeeRet, nChangePosInOut, error, coinControl, false, tx.vExtraPayload.size())) { return false; } From 9d429f400590f58e1746a1393362ce97ae722264 Mon Sep 17 00:00:00 2001 From: Alessandro Rezzi Date: Mon, 4 Mar 2024 21:07:10 +0100 Subject: [PATCH 5/6] refactor: drop functions from struct which supposed to be simple as possible --- src/primitives/transaction.h | 12 +----------- src/script/interpreter.cpp | 2 +- 2 files changed, 2 insertions(+), 12 deletions(-) diff --git a/src/primitives/transaction.h b/src/primitives/transaction.h index da3c9f6a0d..98c1c43036 100644 --- a/src/primitives/transaction.h +++ b/src/primitives/transaction.h @@ -298,7 +298,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.HasExtraPayloadField()) { + if (obj.nVersion >= 3 && obj.nType != TRANSACTION_NORMAL) { READWRITE(obj.vExtraPayload); } } @@ -314,16 +314,6 @@ struct CMutableTransaction uint256 GetHash() const; std::string ToString() const; - - bool IsSpecialTxVersion() const - { - return nVersion >= 3; - } - - bool HasExtraPayloadField() const - { - return IsSpecialTxVersion() && nType != TRANSACTION_NORMAL; - } }; typedef std::shared_ptr CTransactionRef; diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp index 07766b84fa..84c1d9a0c4 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.HasExtraPayloadField()) + if (txTo.nVersion >= 3 && txTo.nType != TRANSACTION_NORMAL) ::Serialize(s, txTo.vExtraPayload); } }; From 8b6c96d20839ce35f484d762afb7778f62de2a33 Mon Sep 17 00:00:00 2001 From: Alessandro Rezzi Date: Mon, 4 Mar 2024 21:13:06 +0100 Subject: [PATCH 6/6] refactor: a new constant with Tx Version --- src/primitives/transaction.h | 8 +++++--- src/script/interpreter.cpp | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/primitives/transaction.h b/src/primitives/transaction.h index 98c1c43036..73f2e50e3b 100644 --- a/src/primitives/transaction.h +++ b/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 @@ -268,7 +270,7 @@ public: bool IsSpecialTxVersion() const noexcept { - return nVersion >= 3; + return nVersion >= SPECIAL_VERSION; } bool HasExtraPayloadField() const noexcept @@ -298,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); } } diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp index 84c1d9a0c4..d3a994c5ca 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.nVersion >= CTransaction::SPECIAL_VERSION && txTo.nType != TRANSACTION_NORMAL) ::Serialize(s, txTo.vExtraPayload); } };