From b115c10755375de459a672e89d854458085e40ee Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Sat, 16 Jan 2016 23:49:42 +0000 Subject: [PATCH 01/21] AreInputsStandard: Return specific reject reasons --- src/policy/policy.cpp | 18 ++++++++++++++++-- src/policy/policy.h | 8 +++++++- src/validation.cpp | 4 ++-- test/functional/p2p_segwit.py | 2 +- 4 files changed, 26 insertions(+), 6 deletions(-) diff --git a/src/policy/policy.cpp b/src/policy/policy.cpp index 68d879b5b8..65db917203 100644 --- a/src/policy/policy.cpp +++ b/src/policy/policy.cpp @@ -133,6 +133,9 @@ bool IsStandardTx(const CTransaction& tx, const std::optional& max_dat for (const CTxOut& txout : tx.vout) { if (!::IsStandard(txout.scriptPubKey, max_datacarrier_bytes, whichType)) { reason = "scriptpubkey"; + if (whichType == TxoutType::WITNESS_UNKNOWN) { + reason += "-unknown-witnessversion"; + } return false; } @@ -174,7 +177,7 @@ bool IsStandardTx(const CTransaction& tx, const std::optional& max_dat * * Note that only the non-witness portion of the transaction is checked here. */ -bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs) +bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs, const std::string& reason_prefix, std::string& out_reason) { if (tx.IsCoinBase()) { return true; // Coinbases don't use vin normally @@ -185,21 +188,32 @@ bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs) std::vector > vSolutions; TxoutType whichType = Solver(prev.scriptPubKey, vSolutions); - if (whichType == TxoutType::NONSTANDARD || whichType == TxoutType::WITNESS_UNKNOWN) { + if (whichType == TxoutType::NONSTANDARD) { + out_reason = reason_prefix + "script-unknown"; + return false; + } else if (whichType == TxoutType::WITNESS_UNKNOWN) { // WITNESS_UNKNOWN failures are typically also caught with a policy // flag in the script interpreter, but it can be helpful to catch // this type of NONSTANDARD transaction earlier in transaction // validation. + out_reason = reason_prefix + "witness-unknown"; return false; } else if (whichType == TxoutType::SCRIPTHASH) { std::vector > stack; // convert the scriptSig into a stack, so we can inspect the redeemScript if (!EvalScript(stack, tx.vin[i].scriptSig, SCRIPT_VERIFY_NONE, BaseSignatureChecker(), SigVersion::BASE)) + { + out_reason = reason_prefix + "scriptsig-failure"; return false; + } if (stack.empty()) + { + out_reason = reason_prefix + "scriptcheck-missing"; return false; + } CScript subscript(stack.back().begin(), stack.back().end()); if (subscript.GetSigOpCount(true) > MAX_P2SH_SIGOPS) { + out_reason = reason_prefix + "scriptcheck-sigops"; return false; } } diff --git a/src/policy/policy.h b/src/policy/policy.h index a82488a28c..95910b0c01 100644 --- a/src/policy/policy.h +++ b/src/policy/policy.h @@ -143,7 +143,13 @@ bool IsStandardTx(const CTransaction& tx, const std::optional& max_dat * @param[in] mapInputs Map of previous transactions that have outputs we're spending * @return True if all inputs (scriptSigs) use only standard transaction forms */ -bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs); +bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs, const std::string& reason_prefix, std::string& out_reason); + +inline bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs) { + std::string reason; + return AreInputsStandard(tx, mapInputs, reason, reason); +} + /** * Check if the transaction is over standard P2WSH resources limit: * 3600bytes witnessScript size, 80bytes per witness stack element, 100 witness stack elements diff --git a/src/validation.cpp b/src/validation.cpp index 8f75b2e30a..df18319f4a 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -901,8 +901,8 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) return false; // state filled in by CheckTxInputs } - if (m_pool.m_opts.require_standard && !AreInputsStandard(tx, m_view)) { - return state.Invalid(TxValidationResult::TX_INPUTS_NOT_STANDARD, "bad-txns-nonstandard-inputs"); + if (m_pool.m_opts.require_standard && !AreInputsStandard(tx, m_view, "bad-txns-input-", reason)) { + return state.Invalid(TxValidationResult::TX_INPUTS_NOT_STANDARD, reason); } // Check for non-standard witnesses. diff --git a/test/functional/p2p_segwit.py b/test/functional/p2p_segwit.py index 9be53d2ab8..73396fd74c 100755 --- a/test/functional/p2p_segwit.py +++ b/test/functional/p2p_segwit.py @@ -1387,7 +1387,7 @@ class SegWitTest(BitcoinTestFramework): # First we test this transaction against std_node # making sure the txid is added to the reject filter self.std_node.announce_tx_and_wait_for_getdata(tx3) - test_transaction_acceptance(self.nodes[1], self.std_node, tx3, with_witness=True, accepted=False, reason="bad-txns-nonstandard-inputs") + test_transaction_acceptance(self.nodes[1], self.std_node, tx3, with_witness=True, accepted=False, reason="bad-txns-input-witness-unknown") # Now the node will no longer ask for getdata of this transaction when advertised by same txid self.std_node.announce_tx_and_wait_for_getdata(tx3, success=False) From 8a18e6513caa6bc482734ec60fd4572adf31c2b9 Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Tue, 1 Aug 2023 18:12:08 +0000 Subject: [PATCH 02/21] AcceptToMemoryPool: Minimally change bool bypass_limits to unordered_set ignore_rejects --- src/kernel/mempool_entry.h | 6 ++-- src/node/mempool_persist.cpp | 2 +- src/policy/fees.cpp | 2 +- src/policy/policy.h | 4 +++ src/test/fuzz/policy_estimator.cpp | 2 +- src/test/policyestimator_tests.cpp | 6 ++-- src/validation.cpp | 44 ++++++++++++++---------------- src/validation.h | 16 ++++++++--- 8 files changed, 46 insertions(+), 36 deletions(-) diff --git a/src/kernel/mempool_entry.h b/src/kernel/mempool_entry.h index 2adeaea652..5db9c2cac0 100644 --- a/src/kernel/mempool_entry.h +++ b/src/kernel/mempool_entry.h @@ -226,7 +226,7 @@ struct NewMempoolTransactionInfo { * This boolean indicates whether the transaction was added * without enforcing mempool fee limits. */ - const bool m_mempool_limit_bypassed; + const ignore_rejects_type m_ignore_rejects; /* This boolean indicates whether the transaction is part of a package. */ const bool m_submitted_in_package; /* @@ -239,11 +239,11 @@ struct NewMempoolTransactionInfo { explicit NewMempoolTransactionInfo(const CTransactionRef& tx, const CAmount& fee, const int64_t vsize, const unsigned int height, - const bool mempool_limit_bypassed, const bool submitted_in_package, + const ignore_rejects_type& ignore_rejects, const bool submitted_in_package, const bool chainstate_is_current, const bool has_no_mempool_parents) : info{tx, fee, vsize, height}, - m_mempool_limit_bypassed{mempool_limit_bypassed}, + m_ignore_rejects{ignore_rejects}, m_submitted_in_package{submitted_in_package}, m_chainstate_is_current{chainstate_is_current}, m_has_no_mempool_parents{has_no_mempool_parents} {} diff --git a/src/node/mempool_persist.cpp b/src/node/mempool_persist.cpp index a265c2e12d..517d59ee0a 100644 --- a/src/node/mempool_persist.cpp +++ b/src/node/mempool_persist.cpp @@ -98,7 +98,7 @@ bool LoadMempool(CTxMemPool& pool, const fs::path& load_path, Chainstate& active } if (nTime > TicksSinceEpoch(now - pool.m_opts.expiry)) { LOCK(cs_main); - const auto& accepted = AcceptToMemoryPool(active_chainstate, tx, nTime, /*bypass_limits=*/false, /*test_accept=*/false); + const auto& accepted = AcceptToMemoryPool(active_chainstate, tx, nTime, empty_ignore_rejects, /*test_accept=*/false); if (accepted.m_result_type == MempoolAcceptResult::ResultType::VALID) { ++count; } else { diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp index 5f1d15c5f2..56571c7c05 100644 --- a/src/policy/fees.cpp +++ b/src/policy/fees.cpp @@ -612,7 +612,7 @@ void CBlockPolicyEstimator::processTransaction(const NewMempoolTransactionInfo& // - the node is not behind // - the transaction is not dependent on any other transactions in the mempool // - it's not part of a package. - const bool validForFeeEstimation = !tx.m_mempool_limit_bypassed && !tx.m_submitted_in_package && tx.m_chainstate_is_current && tx.m_has_no_mempool_parents; + const bool validForFeeEstimation = tx.m_ignore_rejects.empty() && !tx.m_submitted_in_package && tx.m_chainstate_is_current && tx.m_has_no_mempool_parents; // Only want to be updating estimates when our blockchain is synced, // otherwise we'll miscalculate how many blocks its taking to get included. diff --git a/src/policy/policy.h b/src/policy/policy.h index 95910b0c01..e879d576f7 100644 --- a/src/policy/policy.h +++ b/src/policy/policy.h @@ -14,6 +14,7 @@ #include #include +#include class CCoinsViewCache; class CFeeRate; @@ -121,6 +122,9 @@ static constexpr unsigned int STANDARD_NOT_MANDATORY_VERIFY_FLAGS{STANDARD_SCRIP /** Used as the flags parameter to sequence and nLocktime checks in non-consensus code. */ static constexpr unsigned int STANDARD_LOCKTIME_VERIFY_FLAGS{LOCKTIME_VERIFY_SEQUENCE}; +typedef std::unordered_set ignore_rejects_type; +static const ignore_rejects_type empty_ignore_rejects{}; + CAmount GetDustThreshold(const CTxOut& txout, const CFeeRate& dustRelayFee); bool IsDust(const CTxOut& txout, const CFeeRate& dustRelayFee); diff --git a/src/test/fuzz/policy_estimator.cpp b/src/test/fuzz/policy_estimator.cpp index a4e1947b9f..2d25d36402 100644 --- a/src/test/fuzz/policy_estimator.cpp +++ b/src/test/fuzz/policy_estimator.cpp @@ -49,7 +49,7 @@ FUZZ_TARGET(policy_estimator, .init = initialize_policy_estimator) const auto tx_has_mempool_parents = fuzzed_data_provider.ConsumeBool(); const auto tx_info = NewMempoolTransactionInfo(entry.GetSharedTx(), entry.GetFee(), entry.GetTxSize(), entry.GetHeight(), - /*mempool_limit_bypassed=*/false, + empty_ignore_rejects, tx_submitted_in_package, /*chainstate_is_current=*/true, tx_has_mempool_parents); diff --git a/src/test/policyestimator_tests.cpp b/src/test/policyestimator_tests.cpp index 83977c1c89..ceb1cfb010 100644 --- a/src/test/policyestimator_tests.cpp +++ b/src/test/policyestimator_tests.cpp @@ -71,7 +71,7 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) feeV[j], virtual_size, entry.nHeight, - /*mempool_limit_bypassed=*/false, + empty_ignore_rejects, /*submitted_in_package=*/false, /*chainstate_is_current=*/true, /*has_no_mempool_parents=*/true)}; @@ -172,7 +172,7 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) feeV[j], virtual_size, entry.nHeight, - /*mempool_limit_bypassed=*/false, + empty_ignore_rejects, /*submitted_in_package=*/false, /*chainstate_is_current=*/true, /*has_no_mempool_parents=*/true)}; @@ -236,7 +236,7 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) feeV[j], virtual_size, entry.nHeight, - /*mempool_limit_bypassed=*/false, + empty_ignore_rejects, /*submitted_in_package=*/false, /*chainstate_is_current=*/true, /*has_no_mempool_parents=*/true)}; diff --git a/src/validation.cpp b/src/validation.cpp index df18319f4a..266fe60fb8 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -449,7 +449,7 @@ public: struct ATMPArgs { const CChainParams& m_chainparams; const int64_t m_accept_time; - const bool m_bypass_limits; + const ignore_rejects_type& m_ignore_rejects; /* * Return any outpoints which were not previously present in the coins * cache, but were added as a result of validating the tx for mempool @@ -486,11 +486,11 @@ public: /** Parameters for single transaction mempool validation. */ static ATMPArgs SingleAccept(const CChainParams& chainparams, int64_t accept_time, - bool bypass_limits, std::vector& coins_to_uncache, + const ignore_rejects_type& ignore_rejects, std::vector& coins_to_uncache, bool test_accept) { return ATMPArgs{/* m_chainparams */ chainparams, /* m_accept_time */ accept_time, - /* m_bypass_limits */ bypass_limits, + /* m_ignore_rejects */ ignore_rejects, /* m_coins_to_uncache */ coins_to_uncache, /* m_test_accept */ test_accept, /* m_allow_replacement */ true, @@ -507,7 +507,7 @@ public: std::vector& coins_to_uncache) { return ATMPArgs{/* m_chainparams */ chainparams, /* m_accept_time */ accept_time, - /* m_bypass_limits */ false, + empty_ignore_rejects, /* m_coins_to_uncache */ coins_to_uncache, /* m_test_accept */ true, /* m_allow_replacement */ false, @@ -524,7 +524,7 @@ public: std::vector& coins_to_uncache, const std::optional& client_maxfeerate) { return ATMPArgs{/* m_chainparams */ chainparams, /* m_accept_time */ accept_time, - /* m_bypass_limits */ false, + empty_ignore_rejects, /* m_coins_to_uncache */ coins_to_uncache, /* m_test_accept */ false, /* m_allow_replacement */ true, @@ -540,7 +540,7 @@ public: static ATMPArgs SingleInPackageAccept(const ATMPArgs& package_args) { return ATMPArgs{/* m_chainparams */ package_args.m_chainparams, /* m_accept_time */ package_args.m_accept_time, - /* m_bypass_limits */ false, + empty_ignore_rejects, /* m_coins_to_uncache */ package_args.m_coins_to_uncache, /* m_test_accept */ package_args.m_test_accept, /* m_allow_replacement */ true, @@ -557,7 +557,7 @@ public: // mixing up the order of the arguments. Use static functions above instead. ATMPArgs(const CChainParams& chainparams, int64_t accept_time, - bool bypass_limits, + const ignore_rejects_type& ignore_rejects, std::vector& coins_to_uncache, bool test_accept, bool allow_replacement, @@ -568,7 +568,7 @@ public: bool allow_carveouts) : m_chainparams{chainparams}, m_accept_time{accept_time}, - m_bypass_limits{bypass_limits}, + m_ignore_rejects{ignore_rejects}, m_coins_to_uncache{coins_to_uncache}, m_test_accept{test_accept}, m_allow_replacement{allow_replacement}, @@ -702,16 +702,16 @@ private: EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); // Compare a package's feerate against minimum allowed. - bool CheckFeeRate(size_t package_size, CAmount package_fee, TxValidationState& state) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, m_pool.cs) + bool CheckFeeRate(size_t package_size, CAmount package_fee, TxValidationState& state, const ignore_rejects_type& ignore_rejects) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, m_pool.cs) { AssertLockHeld(::cs_main); AssertLockHeld(m_pool.cs); CAmount mempoolRejectFee = m_pool.GetMinFee().GetFee(package_size); - if (mempoolRejectFee > 0 && package_fee < mempoolRejectFee) { + if (mempoolRejectFee > 0 && package_fee < mempoolRejectFee && !ignore_rejects.count(rejectmsg_lowfee_mempool)) { return state.Invalid(TxValidationResult::TX_RECONSIDERABLE, "mempool min fee not met", strprintf("%d < %d", package_fee, mempoolRejectFee)); } - if (package_fee < m_pool.m_opts.min_relay_feerate.GetFee(package_size)) { + if (package_fee < m_pool.m_opts.min_relay_feerate.GetFee(package_size) && !ignore_rejects.count(rejectmsg_lowfee_relay)) { return state.Invalid(TxValidationResult::TX_RECONSIDERABLE, "min relay fee not met", strprintf("%d < %d", package_fee, m_pool.m_opts.min_relay_feerate.GetFee(package_size))); } @@ -776,7 +776,6 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // Copy/alias what we need out of args const int64_t nAcceptTime = args.m_accept_time; - const bool bypass_limits = args.m_bypass_limits; std::vector& coins_to_uncache = args.m_coins_to_uncache; // Alias what we need out of ws @@ -927,9 +926,9 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) } } - // Set entry_sequence to 0 when bypass_limits is used; this allows txs from a block + // Set entry_sequence to 0 when rejectmsg_zero_mempool_entry_seq is used; this allows txs from a block // reorg to be marked earlier than any child txs that were already in the mempool. - const uint64_t entry_sequence = bypass_limits ? 0 : m_pool.GetSequence(); + const uint64_t entry_sequence = args.m_ignore_rejects.count(rejectmsg_zero_mempool_entry_seq) ? 0 : m_pool.GetSequence(); entry.reset(new CTxMemPoolEntry(ptx, ws.m_base_fees, nAcceptTime, m_active_chainstate.m_chain.Height(), entry_sequence, fSpendsCoinbase, nSigOpsCost, lock_points.value())); ws.m_vsize = entry->GetTxSize(); @@ -944,7 +943,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // method of ensuring the tx remains bumped. For example, the fee-bumping child could disappear // due to a replacement. // The only exception is TRUC transactions. - if (!bypass_limits && ws.m_ptx->version != TRUC_VERSION && ws.m_modified_fees < m_pool.m_opts.min_relay_feerate.GetFee(ws.m_vsize)) { + if (ws.m_ptx->version != TRUC_VERSION && ws.m_modified_fees < m_pool.m_opts.min_relay_feerate.GetFee(ws.m_vsize) && !args.m_ignore_rejects.count(rejectmsg_mempoolfull)) { // Even though this is a fee-related failure, this result is TX_MEMPOOL_POLICY, not // TX_RECONSIDERABLE, because it cannot be bypassed using package validation. return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "min relay fee not met", @@ -953,7 +952,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // No individual transactions are allowed below the mempool min feerate except from disconnected // blocks and transactions in a package. Package transactions will be checked using package // feerate later. - if (!bypass_limits && !args.m_package_feerates && !CheckFeeRate(ws.m_vsize, ws.m_modified_fees, state)) return false; + if (!args.m_package_feerates && !CheckFeeRate(ws.m_vsize, ws.m_modified_fees, state, args.m_ignore_rejects)) return false; ws.m_iters_conflicting = m_pool.GetIterSet(ws.m_conflicts); @@ -1295,7 +1294,6 @@ bool MemPoolAccept::Finalize(const ATMPArgs& args, Workspace& ws) const CTransaction& tx = *ws.m_ptx; const uint256& hash = ws.m_hash; TxValidationState& state = ws.m_state; - const bool bypass_limits = args.m_bypass_limits; std::unique_ptr& entry = ws.m_entry; if (!m_subpackage.m_all_conflicts.empty()) Assume(args.m_allow_replacement); @@ -1333,7 +1331,7 @@ bool MemPoolAccept::Finalize(const ATMPArgs& args, Workspace& ws) // If we are validating a package, don't trim here because we could evict a previous transaction // in the package. LimitMempoolSize() should be called at the very end to make sure the mempool // is still within limits and package submission happens atomically. - if (!args.m_package_submission && !bypass_limits) { + if (!args.m_package_submission && !args.m_ignore_rejects.count(rejectmsg_mempoolfull)) { LimitMempoolSize(m_pool, m_active_chainstate.CoinsTip()); if (!m_pool.exists(GenTxid::Txid(hash))) // The tx no longer meets our (new) mempool minimum feerate but could be reconsidered in a package. @@ -1424,7 +1422,7 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector& const CTransaction& tx = *ws.m_ptx; const auto tx_info = NewMempoolTransactionInfo(ws.m_ptx, ws.m_base_fees, ws.m_vsize, ws.m_entry->GetHeight(), - args.m_bypass_limits, args.m_package_submission, + args.m_ignore_rejects, args.m_package_submission, IsCurrentForFeeEstimation(m_active_chainstate), m_pool.HasNoInputsOf(tx)); m_pool.m_opts.signals->TransactionAddedToMempool(tx_info, m_pool.GetAndIncrementSequence()); @@ -1486,7 +1484,7 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef const CTransaction& tx = *ws.m_ptx; const auto tx_info = NewMempoolTransactionInfo(ws.m_ptx, ws.m_base_fees, ws.m_vsize, ws.m_entry->GetHeight(), - args.m_bypass_limits, args.m_package_submission, + args.m_ignore_rejects, args.m_package_submission, IsCurrentForFeeEstimation(m_active_chainstate), m_pool.HasNoInputsOf(tx)); m_pool.m_opts.signals->TransactionAddedToMempool(tx_info, m_pool.GetAndIncrementSequence()); @@ -1580,7 +1578,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: [](const auto& ws) { return ws.m_ptx->GetWitnessHash(); }); TxValidationState placeholder_state; if (args.m_package_feerates && - !CheckFeeRate(m_subpackage.m_total_vsize, m_subpackage.m_total_modified_fees, placeholder_state)) { + !CheckFeeRate(m_subpackage.m_total_vsize, m_subpackage.m_total_modified_fees, placeholder_state, empty_ignore_rejects)) { package_state.Invalid(PackageValidationResult::PCKG_TX, "transaction failed"); return PackageMempoolAcceptResult(package_state, {{workspaces.back().m_ptx->GetWitnessHash(), MempoolAcceptResult::FeeFailure(placeholder_state, CFeeRate(m_subpackage.m_total_modified_fees, m_subpackage.m_total_vsize), all_package_wtxids)}}); @@ -1855,7 +1853,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, } // anon namespace MempoolAcceptResult AcceptToMemoryPool(Chainstate& active_chainstate, const CTransactionRef& tx, - int64_t accept_time, bool bypass_limits, bool test_accept) + int64_t accept_time, const ignore_rejects_type& ignore_rejects, bool test_accept) { AssertLockHeld(::cs_main); const CChainParams& chainparams{active_chainstate.m_chainman.GetParams()}; @@ -1863,7 +1861,7 @@ MempoolAcceptResult AcceptToMemoryPool(Chainstate& active_chainstate, const CTra CTxMemPool& pool{*active_chainstate.GetMempool()}; std::vector coins_to_uncache; - auto args = MemPoolAccept::ATMPArgs::SingleAccept(chainparams, accept_time, bypass_limits, coins_to_uncache, test_accept); + auto args = MemPoolAccept::ATMPArgs::SingleAccept(chainparams, accept_time, ignore_rejects, coins_to_uncache, test_accept); MempoolAcceptResult result = MemPoolAccept(pool, active_chainstate).AcceptSingleTransaction(tx, args); if (result.m_result_type != MempoolAcceptResult::ResultType::VALID) { // Remove coins that were not present in the coins cache before calling diff --git a/src/validation.h b/src/validation.h index f905d6e624..373d505a70 100644 --- a/src/validation.h +++ b/src/validation.h @@ -254,6 +254,11 @@ struct PackageMempoolAcceptResult : m_tx_results{ {wtxid, result} } {} }; +static const std::string rejectmsg_lowfee_mempool = "mempool min fee not met"; +static const std::string rejectmsg_lowfee_relay = "min relay fee not met"; +static const std::string rejectmsg_mempoolfull = "mempool full"; +static const std::string rejectmsg_zero_mempool_entry_seq = "zero mempool entry sequence"; + /** * Try to add a transaction to the mempool. This is an internal function and is exposed only for testing. * Client code should use ChainstateManager::ProcessTransaction() @@ -262,15 +267,18 @@ struct PackageMempoolAcceptResult * @param[in] tx The transaction to submit for mempool acceptance. * @param[in] accept_time The timestamp for adding the transaction to the mempool. * It is also used to determine when the entry expires. - * @param[in] bypass_limits When true, don't enforce mempool fee and capacity limits, - * and set entry_sequence to zero. + * @param[in] ignore_rejects Set of reject reasons to ignore and bypass, if possible. * @param[in] test_accept When true, run validation checks but don't submit to mempool. * * @returns a MempoolAcceptResult indicating whether the transaction was accepted/rejected with reason. */ MempoolAcceptResult AcceptToMemoryPool(Chainstate& active_chainstate, const CTransactionRef& tx, - int64_t accept_time, bool bypass_limits, bool test_accept) - EXCLUSIVE_LOCKS_REQUIRED(cs_main); + int64_t accept_time, const ignore_rejects_type& ignore_rejects, bool test_accept) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + +static inline MempoolAcceptResult AcceptToMemoryPool(Chainstate& active_chainstate, const CTransactionRef& tx, int64_t accept_time, bool bypass_limits, bool test_accept) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { + static const ignore_rejects_type ignore_rejects_legacy{rejectmsg_lowfee_mempool, rejectmsg_lowfee_relay, rejectmsg_mempoolfull, rejectmsg_zero_mempool_entry_seq}; + return AcceptToMemoryPool(active_chainstate, tx, accept_time, (bypass_limits ? ignore_rejects_legacy : empty_ignore_rejects), test_accept); +} /** * Validate (and maybe submit) a package to the mempool. See doc/policy/packages.md for full details From 9ebc373637b5ee114daf4269c90d52fbada269c8 Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Tue, 1 Aug 2023 18:12:22 +0000 Subject: [PATCH 03/21] AcceptToMemoryPool: Support overriding many top-level rejections --- src/policy/rbf.cpp | 6 +++-- src/policy/rbf.h | 4 ++- src/validation.cpp | 67 +++++++++++++++++++++++++++++++++++----------- 3 files changed, 59 insertions(+), 18 deletions(-) diff --git a/src/policy/rbf.cpp b/src/policy/rbf.cpp index 2ad79b6f99..3f019dc2c0 100644 --- a/src/policy/rbf.cpp +++ b/src/policy/rbf.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include #include @@ -59,7 +60,8 @@ RBFTransactionState IsRBFOptInEmptyMempool(const CTransaction& tx) std::optional GetEntriesForConflicts(const CTransaction& tx, CTxMemPool& pool, const CTxMemPool::setEntries& iters_conflicting, - CTxMemPool::setEntries& all_conflicts) + CTxMemPool::setEntries& all_conflicts, + const ignore_rejects_type& ignore_rejects) { AssertLockHeld(pool.cs); const uint256 txid = tx.GetHash(); @@ -70,7 +72,7 @@ std::optional GetEntriesForConflicts(const CTransaction& tx, // entries from the mempool. This potentially overestimates the number of actual // descendants (i.e. if multiple conflicts share a descendant, it will be counted multiple // times), but we just want to be conservative to avoid doing too much work. - if (nConflictingCount > MAX_REPLACEMENT_CANDIDATES) { + if (nConflictingCount > MAX_REPLACEMENT_CANDIDATES && !ignore_rejects.count("too-many-replacements")) { return strprintf("rejecting replacement %s; too many potential replacements (%d > %d)\n", txid.ToString(), nConflictingCount, diff --git a/src/policy/rbf.h b/src/policy/rbf.h index 252fbec8e3..97192e8e13 100644 --- a/src/policy/rbf.h +++ b/src/policy/rbf.h @@ -6,6 +6,7 @@ #define BITCOIN_POLICY_RBF_H #include +#include #include #include #include @@ -68,7 +69,8 @@ RBFTransactionState IsRBFOptInEmptyMempool(const CTransaction& tx); */ std::optional GetEntriesForConflicts(const CTransaction& tx, CTxMemPool& pool, const CTxMemPool::setEntries& iters_conflicting, - CTxMemPool::setEntries& all_conflicts) + CTxMemPool::setEntries& all_conflicts, + const ignore_rejects_type& ignore_rejects=empty_ignore_rejects) EXCLUSIVE_LOCKS_REQUIRED(pool.cs); /** The replacement transaction may only include an unconfirmed input if that input was included in diff --git a/src/validation.cpp b/src/validation.cpp index 266fe60fb8..418b96073c 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -663,6 +663,23 @@ private: PrecomputedTransactionData m_precomputed_txdata; }; + static inline bool MaybeReject_(TxValidationResult reason, const std::string& reason_str, const std::string& debug_msg, const ignore_rejects_type& ignore_rejects, TxValidationState& state) { + if (ignore_rejects.count(reason_str)) { + return false; + } + + state.Invalid(reason, reason_str, debug_msg); + return true; + } + +#define MaybeRejectDbg(reason, reason_str, debug_msg) do { \ + if (MaybeReject_(reason, reason_str, debug_msg, ignore_rejects, state)) { \ + return false; \ + } \ +} while(0) + +#define MaybeReject(reason, reason_str) MaybeRejectDbg(reason, reason_str, "") + // Run the policy checks on a given transaction, excluding any script checks. // Looks up inputs, calculates feerate, considers replacement, evaluates // package limits, etc. As this function can be invoked for "free" by a peer, @@ -670,11 +687,11 @@ private: bool PreChecks(ATMPArgs& args, Workspace& ws) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); // Run checks for mempool replace-by-fee, only used in AcceptSingleTransaction. - bool ReplacementChecks(Workspace& ws) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); + bool ReplacementChecks(ATMPArgs& args, Workspace& ws) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); // Enforce package mempool ancestor/descendant limits (distinct from individual // ancestor/descendant limits done in PreChecks) and run Package RBF checks. - bool PackageMempoolChecks(const std::vector& txns, + bool PackageMempoolChecks(const ATMPArgs& args, const std::vector& txns, std::vector& workspaces, int64_t total_vsize, PackageValidationState& package_state) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); @@ -776,6 +793,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // Copy/alias what we need out of args const int64_t nAcceptTime = args.m_accept_time; + const ignore_rejects_type& ignore_rejects = args.m_ignore_rejects; std::vector& coins_to_uncache = args.m_coins_to_uncache; // Alias what we need out of ws @@ -798,13 +816,13 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // Transactions smaller than 65 non-witness bytes are not relayed to mitigate CVE-2017-12842. if (::GetSerializeSize(TX_NO_WITNESS(tx)) < MIN_STANDARD_TX_NONWITNESS_SIZE) - return state.Invalid(TxValidationResult::TX_NOT_STANDARD, "tx-size-small"); + MaybeReject(TxValidationResult::TX_NOT_STANDARD, "tx-size-small"); // Only accept nLockTime-using transactions that can be mined in the next // block; we don't want our mempool filled up with transactions that can't // be mined yet. if (!CheckFinalTxAtTip(*Assert(m_active_chainstate.m_chain.Tip()), tx)) { - return state.Invalid(TxValidationResult::TX_PREMATURE_SPEND, "non-final"); + MaybeReject(TxValidationResult::TX_PREMATURE_SPEND, "non-final"); } if (m_pool.exists(GenTxid::Wtxid(tx.GetWitnessHash()))) { @@ -839,7 +857,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // // Replaceability signaling of the original transactions may be // ignored due to node setting. - const bool allow_rbf{m_pool.m_opts.full_rbf || SignalsOptInRBF(*ptxConflicting) || ptxConflicting->version == TRUC_VERSION}; + const bool allow_rbf{(m_pool.m_opts.full_rbf || ignore_rejects.count("txn-mempool-conflict")) || SignalsOptInRBF(*ptxConflicting) || ptxConflicting->version == TRUC_VERSION}; if (!allow_rbf) { return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "txn-mempool-conflict"); } @@ -891,6 +909,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // Pass in m_view which has all of the relevant inputs cached. Note that, since m_view's // backend was removed, it no longer pulls coins from the mempool. const std::optional lock_points{CalculateLockPointsAtTip(m_active_chainstate.m_chain.Tip(), m_view, tx)}; + // NOTE: The miner doesn't check this again, so for now it may not be overridden. if (!lock_points.has_value() || !CheckSequenceLocksAtTip(m_active_chainstate.m_chain.Tip(), *lock_points)) { return state.Invalid(TxValidationResult::TX_PREMATURE_SPEND, "non-BIP68-final"); } @@ -906,7 +925,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // Check for non-standard witnesses. if (tx.HasWitness() && m_pool.m_opts.require_standard && !IsWitnessStandard(tx, m_view)) { - return state.Invalid(TxValidationResult::TX_WITNESS_MUTATED, "bad-witness-nonstandard"); + MaybeReject(TxValidationResult::TX_WITNESS_MUTATED, "bad-witness-nonstandard"); } int64_t nSigOpsCost = GetTransactionSigOpCost(tx, m_view, STANDARD_SCRIPT_VERIFY_FLAGS); @@ -934,7 +953,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) ws.m_vsize = entry->GetTxSize(); if (nSigOpsCost > MAX_STANDARD_TX_SIGOPS_COST) - return state.Invalid(TxValidationResult::TX_NOT_STANDARD, "bad-txns-too-many-sigops", + MaybeRejectDbg(TxValidationResult::TX_NOT_STANDARD, "bad-txns-too-many-sigops", strprintf("%d", nSigOpsCost)); // No individual transactions are allowed below the min relay feerate except from disconnected blocks. @@ -996,7 +1015,13 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) maybe_rbf_limits.descendant_size_vbytes += conflict->GetSizeWithDescendants(); } - if (auto ancestors{m_pool.CalculateMemPoolAncestors(*entry, maybe_rbf_limits)}) { + CTxMemPool::Limits limits; + if (ignore_rejects.count("too-long-mempool-chain")) { + limits = CTxMemPool::Limits::NoLimits(); + } else { + limits = maybe_rbf_limits; + } + if (auto ancestors{m_pool.CalculateMemPoolAncestors(*entry, limits)}) { ws.m_ancestors = std::move(*ancestors); } else { // If CalculateMemPoolAncestors fails second time, we want the original error string. @@ -1076,7 +1101,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) return true; } -bool MemPoolAccept::ReplacementChecks(Workspace& ws) +bool MemPoolAccept::ReplacementChecks(ATMPArgs& args, Workspace& ws) { AssertLockHeld(cs_main); AssertLockHeld(m_pool.cs); @@ -1095,25 +1120,29 @@ bool MemPoolAccept::ReplacementChecks(Workspace& ws) // guarantee that this is incentive-compatible for miners, because it is possible for a // descendant transaction of a direct conflict to pay a higher feerate than the transaction that // might replace them, under these rules. + if (!args.m_ignore_rejects.count("insufficient fee")) { if (const auto err_string{PaysMoreThanConflicts(ws.m_iters_conflicting, newFeeRate, hash)}) { // This fee-related failure is TX_RECONSIDERABLE because validating in a package may change // the result. return state.Invalid(TxValidationResult::TX_RECONSIDERABLE, strprintf("insufficient fee%s", ws.m_sibling_eviction ? " (including sibling eviction)" : ""), *err_string); } + } // ignore_rejects // Calculate all conflicting entries and enforce Rule #5. - if (const auto err_string{GetEntriesForConflicts(tx, m_pool, ws.m_iters_conflicting, m_subpackage.m_all_conflicts)}) { + if (const auto err_string{GetEntriesForConflicts(tx, m_pool, ws.m_iters_conflicting, m_subpackage.m_all_conflicts, args.m_ignore_rejects)}) { return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, strprintf("too many potential replacements%s", ws.m_sibling_eviction ? " (including sibling eviction)" : ""), *err_string); } // Enforce Rule #2. + if (!args.m_ignore_rejects.count("replacement-adds-unconfirmed")) { if (const auto err_string{HasNoNewUnconfirmed(tx, m_pool, m_subpackage.m_all_conflicts)}) { // Sibling eviction is only done for TRUC transactions, which cannot have multiple ancestors. Assume(!ws.m_sibling_eviction); return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, strprintf("replacement-adds-unconfirmed%s", ws.m_sibling_eviction ? " (including sibling eviction)" : ""), *err_string); } + } // ignore_rejects // Check if it's economically rational to mine this transaction rather than the ones it // replaces and pays for its own relay fees. Enforce Rules #3 and #4. @@ -1121,16 +1150,18 @@ bool MemPoolAccept::ReplacementChecks(Workspace& ws) m_subpackage.m_conflicting_fees += it->GetModifiedFee(); m_subpackage.m_conflicting_size += it->GetTxSize(); } + if (!args.m_ignore_rejects.count("insufficient fee")) { if (const auto err_string{PaysForRBF(m_subpackage.m_conflicting_fees, ws.m_modified_fees, ws.m_vsize, m_pool.m_opts.incremental_relay_feerate, hash)}) { // Result may change in a package context return state.Invalid(TxValidationResult::TX_RECONSIDERABLE, strprintf("insufficient fee%s", ws.m_sibling_eviction ? " (including sibling eviction)" : ""), *err_string); } + } // ignore_rejects return true; } -bool MemPoolAccept::PackageMempoolChecks(const std::vector& txns, +bool MemPoolAccept::PackageMempoolChecks(const ATMPArgs& args, const std::vector& txns, std::vector& workspaces, const int64_t total_vsize, PackageValidationState& package_state) @@ -1144,7 +1175,13 @@ bool MemPoolAccept::PackageMempoolChecks(const std::vector& txn assert(txns.size() == workspaces.size()); - auto result = m_pool.CheckPackageLimits(txns, total_vsize); + util::Result result = [&]() EXCLUSIVE_LOCKS_REQUIRED(m_pool.cs) { + if (args.m_ignore_rejects.count("package-mempool-limits")) { + return util::Result(); + } else { + return m_pool.CheckPackageLimits(txns, total_vsize); + } + }(); if (!result) { // This is a package-wide error, separate from an individual transaction error. return package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package-mempool-limits", util::ErrorString(result).original); @@ -1370,7 +1407,7 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector& // Re-calculate mempool ancestors to call addUnchecked(). They may have changed since the // last calculation done in PreChecks, since package ancestors have already been submitted. { - auto ancestors{m_pool.CalculateMemPoolAncestors(*ws.m_entry, m_pool.m_opts.limits)}; + auto ancestors{m_pool.CalculateMemPoolAncestors(*ws.m_entry, CTxMemPool::Limits::NoLimits())}; if(!ancestors) { results.emplace(ws.m_ptx->GetWitnessHash(), MempoolAcceptResult::Failure(ws.m_state)); // Since PreChecks() and PackageMempoolChecks() both enforce limits, this should never fail. @@ -1452,7 +1489,7 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef return MempoolAcceptResult::Failure(ws.m_state); } - if (m_subpackage.m_rbf && !ReplacementChecks(ws)) { + if (m_subpackage.m_rbf && !ReplacementChecks(args, ws)) { if (ws.m_state.GetResult() == TxValidationResult::TX_RECONSIDERABLE) { // Failed for incentives-based fee reasons. Provide the effective feerate and which tx was included. return MempoolAcceptResult::FeeFailure(ws.m_state, CFeeRate(ws.m_modified_fees, ws.m_vsize), single_wtxid); @@ -1586,7 +1623,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: // Apply package mempool ancestor/descendant limits. Skip if there is only one transaction, // because it's unnecessary. - if (txns.size() > 1 && !PackageMempoolChecks(txns, workspaces, m_subpackage.m_total_vsize, package_state)) { + if (txns.size() > 1 && !PackageMempoolChecks(args, txns, workspaces, m_subpackage.m_total_vsize, package_state)) { return PackageMempoolAcceptResult(package_state, std::move(results)); } From b716189d95e60dc0328fdb89b9317852be839672 Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Tue, 1 Aug 2023 18:12:32 +0000 Subject: [PATCH 04/21] Ability to ignore IsStandardTx rejection reasons --- src/policy/policy.cpp | 59 ++++++++++++++++++++++++++++--------------- src/policy/policy.h | 2 +- src/validation.cpp | 2 +- 3 files changed, 40 insertions(+), 23 deletions(-) diff --git a/src/policy/policy.cpp b/src/policy/policy.cpp index 65db917203..2bd512f7df 100644 --- a/src/policy/policy.cpp +++ b/src/policy/policy.cpp @@ -67,6 +67,10 @@ bool IsDust(const CTxOut& txout, const CFeeRate& dustRelayFeeIn) return (txout.nValue < GetDustThreshold(txout, dustRelayFeeIn)); } +/** + * Note this must assign whichType even if returning false, in case + * IsStandardTx ignores the "scriptpubkey" rejection. + */ bool IsStandard(const CScript& scriptPubKey, const std::optional& max_datacarrier_bytes, TxoutType& whichType) { std::vector > vSolutions; @@ -91,21 +95,36 @@ bool IsStandard(const CScript& scriptPubKey, const std::optional& max_ return true; } -bool IsStandardTx(const CTransaction& tx, const std::optional& max_datacarrier_bytes, bool permit_bare_multisig, const CFeeRate& dust_relay_fee, std::string& reason) -{ - if (tx.version > TX_MAX_STANDARD_VERSION || tx.version < 1) { - reason = "version"; +static inline bool MaybeReject_(std::string& out_reason, const std::string& reason, const std::string& reason_prefix, const ignore_rejects_type& ignore_rejects) { + if (ignore_rejects.count(reason_prefix + reason)) { return false; } + out_reason = reason_prefix + reason; + return true; +} + +#define MaybeReject(reason) do { \ + if (MaybeReject_(out_reason, reason, reason_prefix, ignore_rejects)) { \ + return false; \ + } \ +} while(0) + +bool IsStandardTx(const CTransaction& tx, const std::optional& max_datacarrier_bytes, bool permit_bare_multisig, const CFeeRate& dust_relay_fee, std::string& out_reason, const ignore_rejects_type& ignore_rejects) +{ + const std::string reason_prefix; + + if (tx.version > TX_MAX_STANDARD_VERSION || tx.version < 1) { + MaybeReject("version"); + } + // Extremely large transactions with lots of inputs can cost the network // almost as much to process as they cost the sender in fees, because // computing signature hashes is O(ninputs*txsize). Limiting transactions // to MAX_STANDARD_TX_WEIGHT mitigates CPU exhaustion attacks. unsigned int sz = GetTransactionWeight(tx); if (sz > MAX_STANDARD_TX_WEIGHT) { - reason = "tx-size"; - return false; + MaybeReject("tx-size"); } for (const CTxIn& txin : tx.vin) @@ -119,12 +138,10 @@ bool IsStandardTx(const CTransaction& tx, const std::optional& max_dat // 20-of-20 CHECKMULTISIG scriptPubKey, though such a scriptPubKey // is not considered standard. if (txin.scriptSig.size() > MAX_STANDARD_SCRIPTSIG_SIZE) { - reason = "scriptsig-size"; - return false; + MaybeReject("scriptsig-size"); } if (!txin.scriptSig.IsPushOnly()) { - reason = "scriptsig-not-pushonly"; - return false; + MaybeReject("scriptsig-not-pushonly"); } } @@ -132,28 +149,28 @@ bool IsStandardTx(const CTransaction& tx, const std::optional& max_dat TxoutType whichType; for (const CTxOut& txout : tx.vout) { if (!::IsStandard(txout.scriptPubKey, max_datacarrier_bytes, whichType)) { - reason = "scriptpubkey"; if (whichType == TxoutType::WITNESS_UNKNOWN) { - reason += "-unknown-witnessversion"; + MaybeReject("scriptpubkey-unknown-witnessversion"); + } else { + MaybeReject("scriptpubkey"); } - return false; } - if (whichType == TxoutType::NULL_DATA) + if (whichType == TxoutType::NULL_DATA) { nDataOut++; + continue; + } else if ((whichType == TxoutType::MULTISIG) && (!permit_bare_multisig)) { - reason = "bare-multisig"; - return false; - } else if (IsDust(txout, dust_relay_fee)) { - reason = "dust"; - return false; + MaybeReject("bare-multisig"); + } + if (IsDust(txout, dust_relay_fee)) { + MaybeReject("dust"); } } // only one OP_RETURN txout is permitted if (nDataOut > 1) { - reason = "multi-op-return"; - return false; + MaybeReject("multi-op-return"); } return true; diff --git a/src/policy/policy.h b/src/policy/policy.h index e879d576f7..dbc78e3cce 100644 --- a/src/policy/policy.h +++ b/src/policy/policy.h @@ -141,7 +141,7 @@ static constexpr decltype(CTransaction::version) TX_MAX_STANDARD_VERSION{3}; * Check for standard transaction types * @return True if all outputs (scriptPubKeys) use only standard transaction forms */ -bool IsStandardTx(const CTransaction& tx, const std::optional& max_datacarrier_bytes, bool permit_bare_multisig, const CFeeRate& dust_relay_fee, std::string& reason); +bool IsStandardTx(const CTransaction& tx, const std::optional& max_datacarrier_bytes, bool permit_bare_multisig, const CFeeRate& dust_relay_fee, std::string& out_reason, const ignore_rejects_type& ignore_rejects=empty_ignore_rejects); /** * Check for standard transaction types * @param[in] mapInputs Map of previous transactions that have outputs we're spending diff --git a/src/validation.cpp b/src/validation.cpp index 418b96073c..7e141c4e46 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -810,7 +810,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // Rather not work on nonstandard transactions (unless -testnet/-regtest) std::string reason; - if (m_pool.m_opts.require_standard && !IsStandardTx(tx, m_pool.m_opts.max_datacarrier_bytes, m_pool.m_opts.permit_bare_multisig, m_pool.m_opts.dust_relay_feerate, reason)) { + if (m_pool.m_opts.require_standard && !IsStandardTx(tx, m_pool.m_opts.max_datacarrier_bytes, m_pool.m_opts.permit_bare_multisig, m_pool.m_opts.dust_relay_feerate, reason, ignore_rejects)) { return state.Invalid(TxValidationResult::TX_NOT_STANDARD, reason); } From d926fd0325ea1d5b47a26009c7f3a5ec70fa29b9 Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Tue, 1 Aug 2023 18:12:43 +0000 Subject: [PATCH 05/21] Ability to ignore AreInputsStandard rejection reasons --- src/policy/policy.cpp | 19 ++++++++++++------- src/policy/policy.h | 2 +- src/validation.cpp | 2 +- 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/src/policy/policy.cpp b/src/policy/policy.cpp index 2bd512f7df..e91a1b289c 100644 --- a/src/policy/policy.cpp +++ b/src/policy/policy.cpp @@ -194,7 +194,7 @@ bool IsStandardTx(const CTransaction& tx, const std::optional& max_dat * * Note that only the non-witness portion of the transaction is checked here. */ -bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs, const std::string& reason_prefix, std::string& out_reason) +bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs, const std::string& reason_prefix, std::string& out_reason, const ignore_rejects_type& ignore_rejects) { if (tx.IsCoinBase()) { return true; // Coinbases don't use vin normally @@ -206,32 +206,37 @@ bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs, std::vector > vSolutions; TxoutType whichType = Solver(prev.scriptPubKey, vSolutions); if (whichType == TxoutType::NONSTANDARD) { - out_reason = reason_prefix + "script-unknown"; - return false; + MaybeReject("script-unknown"); } else if (whichType == TxoutType::WITNESS_UNKNOWN) { // WITNESS_UNKNOWN failures are typically also caught with a policy // flag in the script interpreter, but it can be helpful to catch // this type of NONSTANDARD transaction earlier in transaction // validation. - out_reason = reason_prefix + "witness-unknown"; - return false; + MaybeReject("witness-unknown"); } else if (whichType == TxoutType::SCRIPTHASH) { + if (!tx.vin[i].scriptSig.IsPushOnly()) { + // The only way we got this far, is if the user ignored scriptsig-not-pushonly. + // However, this case is invalid, and will be caught later on. + // But for now, we don't want to run the [possibly expensive] script here. + continue; + } std::vector > stack; // convert the scriptSig into a stack, so we can inspect the redeemScript if (!EvalScript(stack, tx.vin[i].scriptSig, SCRIPT_VERIFY_NONE, BaseSignatureChecker(), SigVersion::BASE)) { + // This case is also invalid or a bug out_reason = reason_prefix + "scriptsig-failure"; return false; } if (stack.empty()) { + // Also invalid out_reason = reason_prefix + "scriptcheck-missing"; return false; } CScript subscript(stack.back().begin(), stack.back().end()); if (subscript.GetSigOpCount(true) > MAX_P2SH_SIGOPS) { - out_reason = reason_prefix + "scriptcheck-sigops"; - return false; + MaybeReject("scriptcheck-sigops"); } } } diff --git a/src/policy/policy.h b/src/policy/policy.h index dbc78e3cce..f4d055c753 100644 --- a/src/policy/policy.h +++ b/src/policy/policy.h @@ -147,7 +147,7 @@ bool IsStandardTx(const CTransaction& tx, const std::optional& max_dat * @param[in] mapInputs Map of previous transactions that have outputs we're spending * @return True if all inputs (scriptSigs) use only standard transaction forms */ -bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs, const std::string& reason_prefix, std::string& out_reason); +bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs, const std::string& reason_prefix, std::string& out_reason, const ignore_rejects_type& ignore_rejects=empty_ignore_rejects); inline bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs) { std::string reason; diff --git a/src/validation.cpp b/src/validation.cpp index 7e141c4e46..71ca2bd700 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -919,7 +919,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) return false; // state filled in by CheckTxInputs } - if (m_pool.m_opts.require_standard && !AreInputsStandard(tx, m_view, "bad-txns-input-", reason)) { + if (m_pool.m_opts.require_standard && !AreInputsStandard(tx, m_view, "bad-txns-input-", reason, ignore_rejects)) { return state.Invalid(TxValidationResult::TX_INPUTS_NOT_STANDARD, reason); } From 9a7339f6726e984591964ff3c711927f5648ab7d Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Tue, 1 Aug 2023 18:12:56 +0000 Subject: [PATCH 06/21] Make bad-witness-nonstandard rejection more specific, and support overriding some --- src/policy/policy.cpp | 35 ++++++++++++++++++++++++++++------- src/policy/policy.h | 2 +- src/test/fuzz/coins_view.cpp | 3 ++- src/test/fuzz/transaction.cpp | 3 ++- src/test/txpackage_tests.cpp | 2 +- src/validation.cpp | 4 ++-- test/functional/p2p_segwit.py | 14 +++++++------- 7 files changed, 43 insertions(+), 20 deletions(-) diff --git a/src/policy/policy.cpp b/src/policy/policy.cpp index e91a1b289c..c4a5c99192 100644 --- a/src/policy/policy.cpp +++ b/src/policy/policy.cpp @@ -244,7 +244,7 @@ bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs, return true; } -bool IsWitnessStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs) +bool IsWitnessStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs, const std::string& reason_prefix, std::string& out_reason, const ignore_rejects_type& ignore_rejects) { if (tx.IsCoinBase()) return true; // Coinbases are skipped @@ -273,9 +273,15 @@ bool IsWitnessStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs) // into a stack. We do not check IsPushOnly nor compare the hash as these will be done later anyway. // If the check fails at this stage, we know that this txid must be a bad one. if (!EvalScript(stack, tx.vin[i].scriptSig, SCRIPT_VERIFY_NONE, BaseSignatureChecker(), SigVersion::BASE)) + { + out_reason = reason_prefix + "scriptsig-failure"; return false; + } if (stack.empty()) + { + out_reason = reason_prefix + "scriptcheck-missing"; return false; + } prevScript = CScript(stack.back().begin(), stack.back().end()); p2sh = true; } @@ -285,18 +291,21 @@ bool IsWitnessStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs) // Non-witness program must not be associated with any witness if (!prevScript.IsWitnessProgram(witnessversion, witnessprogram)) + { + out_reason = reason_prefix + "nonwitness-input"; return false; + } // Check P2WSH standard limits if (witnessversion == 0 && witnessprogram.size() == WITNESS_V0_SCRIPTHASH_SIZE) { if (tx.vin[i].scriptWitness.stack.back().size() > MAX_STANDARD_P2WSH_SCRIPT_SIZE) - return false; + MaybeReject("script-size"); size_t sizeWitnessStack = tx.vin[i].scriptWitness.stack.size() - 1; if (sizeWitnessStack > MAX_STANDARD_P2WSH_STACK_ITEMS) - return false; + MaybeReject("stackitem-count"); for (unsigned int j = 0; j < sizeWitnessStack; j++) { if (tx.vin[i].scriptWitness.stack[j].size() > MAX_STANDARD_P2WSH_STACK_ITEM_SIZE) - return false; + MaybeReject("stackitem-size"); } } @@ -308,17 +317,28 @@ bool IsWitnessStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs) Span stack{tx.vin[i].scriptWitness.stack}; if (stack.size() >= 2 && !stack.back().empty() && stack.back()[0] == ANNEX_TAG) { // Annexes are nonstandard as long as no semantics are defined for them. - return false; + MaybeReject("taproot-annex"); + // If reject reason is ignored, continue as if the annex wasn't there. + SpanPopBack(stack); } if (stack.size() >= 2) { // Script path spend (2 or more stack elements after removing optional annex) const auto& control_block = SpanPopBack(stack); SpanPopBack(stack); // Ignore script - if (control_block.empty()) return false; // Empty control block is invalid + if (control_block.empty()) { + // Empty control block is invalid + out_reason = reason_prefix + "taproot-control-missing"; + return false; + } if ((control_block[0] & TAPROOT_LEAF_MASK) == TAPROOT_LEAF_TAPSCRIPT) { // Leaf version 0xc0 (aka Tapscript, see BIP 342) + if (!ignore_rejects.count(reason_prefix + "taproot-stackitem-size")) { for (const auto& item : stack) { - if (item.size() > MAX_STANDARD_TAPSCRIPT_STACK_ITEM_SIZE) return false; + if (item.size() > MAX_STANDARD_TAPSCRIPT_STACK_ITEM_SIZE) { + out_reason = reason_prefix + "taproot-stackitem-size"; + return false; + } + } } } } else if (stack.size() == 1) { @@ -326,6 +346,7 @@ bool IsWitnessStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs) // (no policy rules apply) } else { // 0 stack elements; this is already invalid by consensus rules + out_reason = reason_prefix + "taproot-witness-missing"; return false; } } diff --git a/src/policy/policy.h b/src/policy/policy.h index f4d055c753..fdd0aa5f59 100644 --- a/src/policy/policy.h +++ b/src/policy/policy.h @@ -161,7 +161,7 @@ inline bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& map * * Also enforce a maximum stack item size limit and no annexes for tapscript spends. */ -bool IsWitnessStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs); +bool IsWitnessStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs, const std::string& reason_prefix, std::string& out_reason, const ignore_rejects_type& ignore_rejects=empty_ignore_rejects); /** Compute the virtual transaction size (weight reinterpreted as bytes). */ int64_t GetVirtualTransactionSize(int64_t nWeight, int64_t nSigOpCost, unsigned int bytes_per_sigop); diff --git a/src/test/fuzz/coins_view.cpp b/src/test/fuzz/coins_view.cpp index 368c69819a..ff842997eb 100644 --- a/src/test/fuzz/coins_view.cpp +++ b/src/test/fuzz/coins_view.cpp @@ -284,7 +284,8 @@ FUZZ_TARGET(coins_view, .init = initialize_coins_view) (void)GetTransactionSigOpCost(transaction, coins_view_cache, flags); }, [&] { - (void)IsWitnessStandard(CTransaction{random_mutable_transaction}, coins_view_cache); + std::string reason; + (void)IsWitnessStandard(CTransaction{random_mutable_transaction}, coins_view_cache, "bad-witness-", reason); }); } } diff --git a/src/test/fuzz/transaction.cpp b/src/test/fuzz/transaction.cpp index 2a043f7458..9c740e8c64 100644 --- a/src/test/fuzz/transaction.cpp +++ b/src/test/fuzz/transaction.cpp @@ -88,7 +88,8 @@ FUZZ_TARGET(transaction, .init = initialize_transaction) CCoinsView coins_view; const CCoinsViewCache coins_view_cache(&coins_view); (void)AreInputsStandard(tx, coins_view_cache); - (void)IsWitnessStandard(tx, coins_view_cache); + std::string reject_reason; + (void)IsWitnessStandard(tx, coins_view_cache, "fuzz", reject_reason); if (tx.GetTotalSize() < 250'000) { // Avoid high memory usage (with msan) due to json encoding { diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp index ca9dc5527d..17588fb7bd 100644 --- a/src/test/txpackage_tests.cpp +++ b/src/test/txpackage_tests.cpp @@ -426,7 +426,7 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup) auto it_parent = result_quit_early.m_tx_results.find(tx_parent_invalid->GetWitnessHash()); auto it_child = result_quit_early.m_tx_results.find(tx_child->GetWitnessHash()); BOOST_CHECK_EQUAL(it_parent->second.m_state.GetResult(), TxValidationResult::TX_WITNESS_MUTATED); - BOOST_CHECK_EQUAL(it_parent->second.m_state.GetRejectReason(), "bad-witness-nonstandard"); + BOOST_CHECK_EQUAL(it_parent->second.m_state.GetRejectReason(), "bad-witness-nonwitness-input"); BOOST_CHECK_EQUAL(it_child->second.m_state.GetResult(), TxValidationResult::TX_MISSING_INPUTS); BOOST_CHECK_EQUAL(it_child->second.m_state.GetRejectReason(), "bad-txns-inputs-missingorspent"); } diff --git a/src/validation.cpp b/src/validation.cpp index 71ca2bd700..f2776761cb 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -924,8 +924,8 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) } // Check for non-standard witnesses. - if (tx.HasWitness() && m_pool.m_opts.require_standard && !IsWitnessStandard(tx, m_view)) { - MaybeReject(TxValidationResult::TX_WITNESS_MUTATED, "bad-witness-nonstandard"); + if (tx.HasWitness() && m_pool.m_opts.require_standard && !IsWitnessStandard(tx, m_view, "bad-witness-", reason, ignore_rejects)) { + return state.Invalid(TxValidationResult::TX_WITNESS_MUTATED, reason); } int64_t nSigOpsCost = GetTransactionSigOpCost(tx, m_view, STANDARD_SCRIPT_VERIFY_FLAGS); diff --git a/test/functional/p2p_segwit.py b/test/functional/p2p_segwit.py index 73396fd74c..e6cb6f4ed0 100755 --- a/test/functional/p2p_segwit.py +++ b/test/functional/p2p_segwit.py @@ -1750,7 +1750,7 @@ class SegWitTest(BitcoinTestFramework): tx2.rehash() # This will be rejected due to a policy check: # No witness is allowed, since it is not a witness program but a p2sh program - test_transaction_acceptance(self.nodes[1], self.std_node, tx2, True, False, 'bad-witness-nonstandard') + test_transaction_acceptance(self.nodes[1], self.std_node, tx2, True, False, 'bad-witness-nonwitness-input') # If we send without witness, it should be accepted. test_transaction_acceptance(self.nodes[1], self.std_node, tx2, False, True) @@ -1819,13 +1819,13 @@ class SegWitTest(BitcoinTestFramework): # Testing native P2WSH # Witness stack size, excluding witnessScript, over 100 is non-standard p2wsh_txs[0].wit.vtxinwit[0].scriptWitness.stack = [pad] * 101 + [scripts[0]] - test_transaction_acceptance(self.nodes[1], self.std_node, p2wsh_txs[0], True, False, 'bad-witness-nonstandard') + test_transaction_acceptance(self.nodes[1], self.std_node, p2wsh_txs[0], True, False, 'bad-witness-stackitem-count') # Non-standard nodes should accept test_transaction_acceptance(self.nodes[0], self.test_node, p2wsh_txs[0], True, True) # Stack element size over 80 bytes is non-standard p2wsh_txs[1].wit.vtxinwit[0].scriptWitness.stack = [pad * 81] * 100 + [scripts[1]] - test_transaction_acceptance(self.nodes[1], self.std_node, p2wsh_txs[1], True, False, 'bad-witness-nonstandard') + test_transaction_acceptance(self.nodes[1], self.std_node, p2wsh_txs[1], True, False, 'bad-witness-stackitem-size') # Non-standard nodes should accept test_transaction_acceptance(self.nodes[0], self.test_node, p2wsh_txs[1], True, True) # Standard nodes should accept if element size is not over 80 bytes @@ -1839,16 +1839,16 @@ class SegWitTest(BitcoinTestFramework): # witnessScript size at 3601 bytes is non-standard p2wsh_txs[3].wit.vtxinwit[0].scriptWitness.stack = [pad, pad, pad, scripts[3]] - test_transaction_acceptance(self.nodes[1], self.std_node, p2wsh_txs[3], True, False, 'bad-witness-nonstandard') + test_transaction_acceptance(self.nodes[1], self.std_node, p2wsh_txs[3], True, False, 'bad-witness-script-size') # Non-standard nodes should accept test_transaction_acceptance(self.nodes[0], self.test_node, p2wsh_txs[3], True, True) # Repeating the same tests with P2SH-P2WSH p2sh_txs[0].wit.vtxinwit[0].scriptWitness.stack = [pad] * 101 + [scripts[0]] - test_transaction_acceptance(self.nodes[1], self.std_node, p2sh_txs[0], True, False, 'bad-witness-nonstandard') + test_transaction_acceptance(self.nodes[1], self.std_node, p2sh_txs[0], True, False, 'bad-witness-stackitem-count') test_transaction_acceptance(self.nodes[0], self.test_node, p2sh_txs[0], True, True) p2sh_txs[1].wit.vtxinwit[0].scriptWitness.stack = [pad * 81] * 100 + [scripts[1]] - test_transaction_acceptance(self.nodes[1], self.std_node, p2sh_txs[1], True, False, 'bad-witness-nonstandard') + test_transaction_acceptance(self.nodes[1], self.std_node, p2sh_txs[1], True, False, 'bad-witness-stackitem-size') test_transaction_acceptance(self.nodes[0], self.test_node, p2sh_txs[1], True, True) p2sh_txs[1].wit.vtxinwit[0].scriptWitness.stack = [pad * 80] * 100 + [scripts[1]] test_transaction_acceptance(self.nodes[1], self.std_node, p2sh_txs[1], True, True) @@ -1856,7 +1856,7 @@ class SegWitTest(BitcoinTestFramework): test_transaction_acceptance(self.nodes[0], self.test_node, p2sh_txs[2], True, True) test_transaction_acceptance(self.nodes[1], self.std_node, p2sh_txs[2], True, True) p2sh_txs[3].wit.vtxinwit[0].scriptWitness.stack = [pad, pad, pad, scripts[3]] - test_transaction_acceptance(self.nodes[1], self.std_node, p2sh_txs[3], True, False, 'bad-witness-nonstandard') + test_transaction_acceptance(self.nodes[1], self.std_node, p2sh_txs[3], True, False, 'bad-witness-script-size') test_transaction_acceptance(self.nodes[0], self.test_node, p2sh_txs[3], True, True) self.generate(self.nodes[0], 1) # Mine and clean up the mempool of non-standard node From cd1b140d5b57fe0382f30cac4160976cddea07f4 Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Wed, 10 Apr 2019 15:45:27 +0000 Subject: [PATCH 07/21] node: Extend BroadcastTransaction to accept ignore_rejects --- src/node/transaction.cpp | 6 +++--- src/node/transaction.h | 3 ++- src/validation.cpp | 4 ++-- src/validation.h | 2 +- 4 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/node/transaction.cpp b/src/node/transaction.cpp index 5014ec61e1..1b15fd7ca4 100644 --- a/src/node/transaction.cpp +++ b/src/node/transaction.cpp @@ -31,7 +31,7 @@ static TransactionError HandleATMPError(const TxValidationState& state, std::str } } -TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef tx, std::string& err_string, const std::variant& max_tx_fee, bool relay, bool wait_callback) +TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef tx, std::string& err_string, const std::variant& max_tx_fee, bool relay, bool wait_callback, const ignore_rejects_type& ignore_rejects) { // BroadcastTransaction can be called by RPC or by the wallet. // chainman, mempool and peerman are initialized before the RPC server and wallet are started @@ -73,7 +73,7 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t if (max_tx_fee_set) { // First, call ATMP with test_accept and check the fee. If ATMP // fails here, return error immediately. - const MempoolAcceptResult result = node.chainman->ProcessTransaction(tx, /*test_accept=*/ true); + const MempoolAcceptResult result = node.chainman->ProcessTransaction(tx, /*test_accept=*/ true, ignore_rejects); if (result.m_result_type != MempoolAcceptResult::ResultType::VALID) { return HandleATMPError(result.m_state, err_string); } else { @@ -89,7 +89,7 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t } } // Try to submit the transaction to the mempool. - const MempoolAcceptResult result = node.chainman->ProcessTransaction(tx, /*test_accept=*/ false); + const MempoolAcceptResult result = node.chainman->ProcessTransaction(tx, /*test_accept=*/ false, ignore_rejects); if (result.m_result_type != MempoolAcceptResult::ResultType::VALID) { return HandleATMPError(result.m_state, err_string); } diff --git a/src/node/transaction.h b/src/node/transaction.h index 8b03c3ba3d..5bc4273484 100644 --- a/src/node/transaction.h +++ b/src/node/transaction.h @@ -8,6 +8,7 @@ #include #include #include +#include #include @@ -51,7 +52,7 @@ static const CAmount DEFAULT_MAX_BURN_AMOUNT{0}; * @param[in] wait_callback wait until callbacks have been processed to avoid stale result due to a sequentially RPC. * return error */ -[[nodiscard]] TransactionError BroadcastTransaction(NodeContext& node, CTransactionRef tx, std::string& err_string, const std::variant& max_tx_fee, bool relay, bool wait_callback); +[[nodiscard]] TransactionError BroadcastTransaction(NodeContext& node, CTransactionRef tx, std::string& err_string, const std::variant& max_tx_fee, bool relay, bool wait_callback, const ignore_rejects_type& ignore_rejects=empty_ignore_rejects); /** * Return transaction with a given hash. diff --git a/src/validation.cpp b/src/validation.cpp index f2776761cb..bfd14f1733 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4626,7 +4626,7 @@ bool ChainstateManager::ProcessNewBlock(const std::shared_ptr& blo return true; } -MempoolAcceptResult ChainstateManager::ProcessTransaction(const CTransactionRef& tx, bool test_accept) +MempoolAcceptResult ChainstateManager::ProcessTransaction(const CTransactionRef& tx, bool test_accept, const ignore_rejects_type& ignore_rejects) { AssertLockHeld(cs_main); Chainstate& active_chainstate = ActiveChainstate(); @@ -4635,7 +4635,7 @@ MempoolAcceptResult ChainstateManager::ProcessTransaction(const CTransactionRef& state.Invalid(TxValidationResult::TX_NO_MEMPOOL, "no-mempool"); return MempoolAcceptResult::Failure(state); } - auto result = AcceptToMemoryPool(active_chainstate, tx, GetTime(), /*bypass_limits=*/ false, test_accept); + auto result = AcceptToMemoryPool(active_chainstate, tx, GetTime(), ignore_rejects, test_accept); active_chainstate.GetMempool()->check(active_chainstate.CoinsTip(), active_chainstate.m_chain.Height() + 1); return result; } diff --git a/src/validation.h b/src/validation.h index 373d505a70..e4250333bf 100644 --- a/src/validation.h +++ b/src/validation.h @@ -1261,7 +1261,7 @@ public: * @param[in] tx The transaction to submit for mempool acceptance. * @param[in] test_accept When true, run validation checks but don't submit to mempool. */ - [[nodiscard]] MempoolAcceptResult ProcessTransaction(const CTransactionRef& tx, bool test_accept=false) + [[nodiscard]] MempoolAcceptResult ProcessTransaction(const CTransactionRef& tx, bool test_accept=false, const ignore_rejects_type& ignore_rejects=empty_ignore_rejects) EXCLUSIVE_LOCKS_REQUIRED(cs_main); //! Load the block tree and coins database from disk, initializing state if we're running with -reindex From f80c8a5fdca902f6b72207f744344fd317a48e59 Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Tue, 1 Aug 2023 18:27:49 +0000 Subject: [PATCH 08/21] RPC: sendrawtransaction: Replace boolean allowhighfees with an Array of rejections to ignore (in a backward compatible manner) --- src/rpc/client.cpp | 1 + src/rpc/mempool.cpp | 17 ++++++++++++++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index b866fa484b..bac5d4634e 100644 --- a/src/rpc/client.cpp +++ b/src/rpc/client.cpp @@ -125,6 +125,7 @@ static const CRPCConvertParam vRPCConvertParams[] = { "signrawtransactionwithwallet", 1, "prevtxs" }, { "sendrawtransaction", 1, "maxfeerate" }, { "sendrawtransaction", 2, "maxburnamount" }, + { "sendrawtransaction", 3, "ignore_rejects" }, { "testmempoolaccept", 0, "rawtxs" }, { "testmempoolaccept", 1, "maxfeerate" }, { "submitpackage", 0, "package" }, diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp index 98e18d121c..e84744e94d 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -54,6 +54,11 @@ static RPCHelpMan sendrawtransaction() "Reject transactions with provably unspendable outputs (e.g. 'datacarrier' outputs that use the OP_RETURN opcode) greater than the specified value, expressed in " + CURRENCY_UNIT + ".\n" "If burning funds through unspendable outputs is desired, increase this value.\n" "This check is based on heuristics and does not guarantee spendability of outputs.\n"}, + {"ignore_rejects", RPCArg::Type::ARR, RPCArg::Default{UniValue::VARR}, "Rejection conditions to ignore, eg 'txn-mempool-conflict'", + { + {"reject_reason", RPCArg::Type::STR, RPCArg::Optional::OMITTED, ""}, + }, + }, }, RPCResult{ RPCResult::Type::STR_HEX, "", "The transaction hash in hex" @@ -85,12 +90,22 @@ static RPCHelpMan sendrawtransaction() CTransactionRef tx(MakeTransactionRef(std::move(mtx))); + const UniValue* json_ign_rejs = &request.params[3]; + const CFeeRate max_raw_tx_fee_rate{ParseFeeRate(self.Arg("maxfeerate"))}; + ignore_rejects_type ignore_rejects; + if (!json_ign_rejs->isNull()) { + for (size_t i = 0; i < json_ign_rejs->size(); ++i) { + const UniValue& json_ign_rej = (*json_ign_rejs)[i]; + ignore_rejects.insert(json_ign_rej.get_str()); + } + } + std::string err_string; AssertLockNotHeld(cs_main); NodeContext& node = EnsureAnyNodeContext(request.context); - const TransactionError err = BroadcastTransaction(node, tx, err_string, max_raw_tx_fee_rate, /*relay=*/true, /*wait_callback=*/true); + const TransactionError err = BroadcastTransaction(node, tx, err_string, max_raw_tx_fee_rate, /*relay=*/true, /*wait_callback=*/true, ignore_rejects); if (TransactionError::OK != err) { throw JSONRPCTransactionError(err, err_string); } From f433dadedd601f752684e015b7f3a3a403c84afb Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Tue, 5 Sep 2023 23:06:28 +0000 Subject: [PATCH 09/21] node: Accept "absurdly-high-fee" and "max-fee-exceeded" reject reasons to ignore max_tx_fee --- src/node/transaction.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/node/transaction.cpp b/src/node/transaction.cpp index 1b15fd7ca4..64933d8a22 100644 --- a/src/node/transaction.cpp +++ b/src/node/transaction.cpp @@ -69,7 +69,10 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t wtxid = mempool_tx->GetWitnessHash(); } else { // Transaction is not already in the mempool. - const bool max_tx_fee_set{(std::holds_alternative(max_tx_fee) ? std::get(max_tx_fee) : std::get(max_tx_fee).GetFeePerK()) > 0}; + bool max_tx_fee_set{(std::holds_alternative(max_tx_fee) ? std::get(max_tx_fee) : std::get(max_tx_fee).GetFeePerK()) > 0}; + if (ignore_rejects.count("absurdly-high-fee") || ignore_rejects.count("max-fee-exceeded")) { + max_tx_fee_set = false; + } if (max_tx_fee_set) { // First, call ATMP with test_accept and check the fee. If ATMP // fails here, return error immediately. From ce3e33cde278da32b38ecb5e9467499da313d71a Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Tue, 1 Aug 2023 18:54:53 +0000 Subject: [PATCH 10/21] Implement ignore_rejects for transaction packages --- src/validation.cpp | 14 +++++++------- src/validation.h | 2 +- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index bfd14f1733..6911afe499 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -504,10 +504,10 @@ public: /** Parameters for test package mempool validation through testmempoolaccept. */ static ATMPArgs PackageTestAccept(const CChainParams& chainparams, int64_t accept_time, - std::vector& coins_to_uncache) { + const ignore_rejects_type& ignore_rejects, std::vector& coins_to_uncache) { return ATMPArgs{/* m_chainparams */ chainparams, /* m_accept_time */ accept_time, - empty_ignore_rejects, + /* m_ignore_rejects */ ignore_rejects, /* m_coins_to_uncache */ coins_to_uncache, /* m_test_accept */ true, /* m_allow_replacement */ false, @@ -521,10 +521,10 @@ public: /** Parameters for child-with-unconfirmed-parents package validation. */ static ATMPArgs PackageChildWithParents(const CChainParams& chainparams, int64_t accept_time, - std::vector& coins_to_uncache, const std::optional& client_maxfeerate) { + std::vector& coins_to_uncache, const std::optional& client_maxfeerate, const ignore_rejects_type& ignore_rejects) { return ATMPArgs{/* m_chainparams */ chainparams, /* m_accept_time */ accept_time, - empty_ignore_rejects, + /* m_ignore_rejects */ ignore_rejects, /* m_coins_to_uncache */ coins_to_uncache, /* m_test_accept */ false, /* m_allow_replacement */ true, @@ -1920,7 +1920,7 @@ MempoolAcceptResult AcceptToMemoryPool(Chainstate& active_chainstate, const CTra } PackageMempoolAcceptResult ProcessNewPackage(Chainstate& active_chainstate, CTxMemPool& pool, - const Package& package, bool test_accept, const std::optional& client_maxfeerate) + const Package& package, bool test_accept, const std::optional& client_maxfeerate, const ignore_rejects_type& ignore_rejects) { AssertLockHeld(cs_main); assert(!package.empty()); @@ -1931,10 +1931,10 @@ PackageMempoolAcceptResult ProcessNewPackage(Chainstate& active_chainstate, CTxM auto result = [&]() EXCLUSIVE_LOCKS_REQUIRED(cs_main) { AssertLockHeld(cs_main); if (test_accept) { - auto args = MemPoolAccept::ATMPArgs::PackageTestAccept(chainparams, GetTime(), coins_to_uncache); + auto args = MemPoolAccept::ATMPArgs::PackageTestAccept(chainparams, GetTime(), ignore_rejects, coins_to_uncache); return MemPoolAccept(pool, active_chainstate).AcceptMultipleTransactions(package, args); } else { - auto args = MemPoolAccept::ATMPArgs::PackageChildWithParents(chainparams, GetTime(), coins_to_uncache, client_maxfeerate); + auto args = MemPoolAccept::ATMPArgs::PackageChildWithParents(chainparams, GetTime(), coins_to_uncache, client_maxfeerate, ignore_rejects); return MemPoolAccept(pool, active_chainstate).AcceptPackage(package, args); } }(); diff --git a/src/validation.h b/src/validation.h index e4250333bf..0b471bb675 100644 --- a/src/validation.h +++ b/src/validation.h @@ -291,7 +291,7 @@ static inline MempoolAcceptResult AcceptToMemoryPool(Chainstate& active_chainsta * possible for the package to be partially submitted. */ PackageMempoolAcceptResult ProcessNewPackage(Chainstate& active_chainstate, CTxMemPool& pool, - const Package& txns, bool test_accept, const std::optional& client_maxfeerate) + const Package& txns, bool test_accept, const std::optional& client_maxfeerate, const ignore_rejects_type& ignore_rejects=empty_ignore_rejects) EXCLUSIVE_LOCKS_REQUIRED(cs_main); /* Mempool validation helper functions */ From fee096b444d39a79caca0a00a375386214fd273c Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Tue, 1 Aug 2023 18:41:06 +0000 Subject: [PATCH 11/21] RPC: Add support for ignore_rejects in testmempoolaccept --- src/rpc/client.cpp | 1 + src/rpc/mempool.cpp | 22 +++++++++++++++++++--- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index bac5d4634e..d464443ecc 100644 --- a/src/rpc/client.cpp +++ b/src/rpc/client.cpp @@ -128,6 +128,7 @@ static const CRPCConvertParam vRPCConvertParams[] = { "sendrawtransaction", 3, "ignore_rejects" }, { "testmempoolaccept", 0, "rawtxs" }, { "testmempoolaccept", 1, "maxfeerate" }, + { "testmempoolaccept", 2, "ignore_rejects" }, { "submitpackage", 0, "package" }, { "submitpackage", 1, "maxfeerate" }, { "submitpackage", 2, "maxburnamount" }, diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp index e84744e94d..fb5dbd7115 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -133,6 +133,11 @@ static RPCHelpMan testmempoolaccept() {"maxfeerate", RPCArg::Type::AMOUNT, RPCArg::Default{FormatMoney(DEFAULT_MAX_RAW_TX_FEE_RATE.GetFeePerK())}, "Reject transactions whose fee rate is higher than the specified value, expressed in " + CURRENCY_UNIT + "/kvB.\nFee rates larger than 1BTC/kvB are rejected.\nSet to 0 to accept any fee rate."}, + {"ignore_rejects", RPCArg::Type::ARR, RPCArg::Default{UniValue::VARR}, "Rejection conditions to ignore, eg 'txn-mempool-conflict'", + { + {"reject_reason", RPCArg::Type::STR, RPCArg::Optional::OMITTED, ""}, + }, + }, }, RPCResult{ RPCResult::Type::ARR, "", "The result of the mempool acceptance test for each raw transaction in the input array.\n" @@ -179,6 +184,16 @@ static RPCHelpMan testmempoolaccept() const CFeeRate max_raw_tx_fee_rate{ParseFeeRate(self.Arg("maxfeerate"))}; + const UniValue* json_ign_rejs = &request.params[2]; + ignore_rejects_type ignore_rejects; + if (!json_ign_rejs->isNull()) { + for (size_t i = 0; i < json_ign_rejs->size(); ++i) { + const UniValue& json_ign_rej = (*json_ign_rejs)[i]; + const std::string& ign_rej = json_ign_rej.get_str(); + ignore_rejects.insert(ign_rej); + } + } + std::vector txns; txns.reserve(raw_transactions.size()); for (const auto& rawtx : raw_transactions.getValues()) { @@ -196,9 +211,9 @@ static RPCHelpMan testmempoolaccept() Chainstate& chainstate = chainman.ActiveChainstate(); const PackageMempoolAcceptResult package_result = [&] { LOCK(::cs_main); - if (txns.size() > 1) return ProcessNewPackage(chainstate, mempool, txns, /*test_accept=*/true, /*client_maxfeerate=*/{}); + if (txns.size() > 1) return ProcessNewPackage(chainstate, mempool, txns, /*test_accept=*/true, /*client_maxfeerate=*/{}, ignore_rejects); return PackageMempoolAcceptResult(txns[0]->GetWitnessHash(), - chainman.ProcessTransaction(txns[0], /*test_accept=*/true)); + chainman.ProcessTransaction(txns[0], /*test_accept=*/true, ignore_rejects)); }(); UniValue rpc_result(UniValue::VARR); @@ -228,7 +243,8 @@ static RPCHelpMan testmempoolaccept() // Check that fee does not exceed maximum fee const int64_t virtual_size = tx_result.m_vsize.value(); const CAmount max_raw_tx_fee = max_raw_tx_fee_rate.GetFee(virtual_size); - if (max_raw_tx_fee && fee > max_raw_tx_fee) { + if (max_raw_tx_fee && fee > max_raw_tx_fee && + 0 == (ignore_rejects.count("absurdly-high-fee") + ignore_rejects.count("max-fee-exceeded"))) { result_inner.pushKV("allowed", false); result_inner.pushKV("reject-reason", "max-fee-exceeded"); exit_early = true; From b812959b510bd2557dbc751e4c33222df2b8671f Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Tue, 1 Aug 2023 18:57:38 +0000 Subject: [PATCH 12/21] QA: rpc_rawtransaction: Test ignore_rejects --- test/functional/rpc_rawtransaction.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/test/functional/rpc_rawtransaction.py b/test/functional/rpc_rawtransaction.py index 18b1fc1896..becd7fb7d8 100755 --- a/test/functional/rpc_rawtransaction.py +++ b/test/functional/rpc_rawtransaction.py @@ -409,12 +409,22 @@ class RawTransactionsTest(BitcoinTestFramework): testres = self.nodes[2].testmempoolaccept([tx['hex']], 0.00001000)[0] assert_equal(testres['allowed'], False) assert_equal(testres['reject-reason'], 'max-fee-exceeded') + testres = self.nodes[2].testmempoolaccept([tx['hex']], 0.00001000, ['foobar'])[0] + assert_equal(testres['allowed'], False) + assert_equal(testres['reject-reason'], 'max-fee-exceeded') + # unless ignored explicitly + testres = self.nodes[2].testmempoolaccept([tx['hex']], 0.00001000, ['max-fee-exceeded'])[0] + assert_equal(testres['allowed'], True) + assert('reject-reason' not in testres) + testres = self.nodes[2].testmempoolaccept([tx['hex']], 0.00001000, ['absurdly-high-fee'])[0] + assert_equal(testres['allowed'], True) + assert('reject-reason' not in testres) # and sendrawtransaction should throw assert_raises_rpc_error(-25, fee_exceeds_max, self.nodes[2].sendrawtransaction, tx['hex'], 0.00001000) # and the following calls should both succeed testres = self.nodes[2].testmempoolaccept(rawtxs=[tx['hex']])[0] assert_equal(testres['allowed'], True) - self.nodes[2].sendrawtransaction(hexstring=tx['hex']) + self.nodes[2].sendrawtransaction(hexstring=tx['hex'], maxfeerate=0.00001000, ignore_rejects=['max-fee-exceeded']) # Test a transaction with a large fee. # Fee rate is 0.20000000 BTC/kvB From adf4ddfac43424f3ed996ea792205dcdfaf65c7d Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Tue, 1 Aug 2023 18:58:30 +0000 Subject: [PATCH 13/21] Bugfix: policy/rbf: Recognise "too many potential replacements" string in ignore_rejects --- src/policy/rbf.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/policy/rbf.cpp b/src/policy/rbf.cpp index 3f019dc2c0..253a1b1959 100644 --- a/src/policy/rbf.cpp +++ b/src/policy/rbf.cpp @@ -72,7 +72,7 @@ std::optional GetEntriesForConflicts(const CTransaction& tx, // entries from the mempool. This potentially overestimates the number of actual // descendants (i.e. if multiple conflicts share a descendant, it will be counted multiple // times), but we just want to be conservative to avoid doing too much work. - if (nConflictingCount > MAX_REPLACEMENT_CANDIDATES && !ignore_rejects.count("too-many-replacements")) { + if (nConflictingCount > MAX_REPLACEMENT_CANDIDATES && !ignore_rejects.count("too-many-replacements") && !ignore_rejects.count("too many potential replacements")) { return strprintf("rejecting replacement %s; too many potential replacements (%d > %d)\n", txid.ToString(), nConflictingCount, From 6d356273b98befc6a8dfbc6edabfa954f05017ae Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Tue, 1 Aug 2023 19:18:15 +0000 Subject: [PATCH 14/21] RPC/mempool: Accept ignore_rejects in sendrawtransaction's 2nd & 3rd params for backward compatibility with Knots <25 --- src/rpc/mempool.cpp | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-) diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp index fb5dbd7115..9a68e676d7 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -49,11 +49,15 @@ static RPCHelpMan sendrawtransaction() {"hexstring", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The hex string of the raw transaction"}, {"maxfeerate", RPCArg::Type::AMOUNT, RPCArg::Default{FormatMoney(DEFAULT_MAX_RAW_TX_FEE_RATE.GetFeePerK())}, "Reject transactions whose fee rate is higher than the specified value, expressed in " + CURRENCY_UNIT + - "/kvB.\nFee rates larger than 1BTC/kvB are rejected.\nSet to 0 to accept any fee rate."}, + "/kvB.\nFee rates larger than 1BTC/kvB are rejected.\nSet to 0 to accept any fee rate.", + RPCArgOptions{.skip_type_check = true} // for ignore_rejects compatibility + }, {"maxburnamount", RPCArg::Type::AMOUNT, RPCArg::Default{FormatMoney(DEFAULT_MAX_BURN_AMOUNT)}, "Reject transactions with provably unspendable outputs (e.g. 'datacarrier' outputs that use the OP_RETURN opcode) greater than the specified value, expressed in " + CURRENCY_UNIT + ".\n" "If burning funds through unspendable outputs is desired, increase this value.\n" - "This check is based on heuristics and does not guarantee spendability of outputs.\n"}, + "This check is based on heuristics and does not guarantee spendability of outputs.\n", + RPCArgOptions{.skip_type_check = true} // for ignore_rejects compatibility + }, {"ignore_rejects", RPCArg::Type::ARR, RPCArg::Default{UniValue::VARR}, "Rejection conditions to ignore, eg 'txn-mempool-conflict'", { {"reject_reason", RPCArg::Type::STR, RPCArg::Optional::OMITTED, ""}, @@ -75,7 +79,24 @@ static RPCHelpMan sendrawtransaction() }, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { - const CAmount max_burn_amount = request.params[2].isNull() ? 0 : AmountFromValue(request.params[2]); + CFeeRate max_raw_tx_fee_rate{DEFAULT_MAX_RAW_TX_FEE_RATE}; + CAmount max_burn_amount{0}; + const UniValue* json_ign_rejs = &request.params[3]; + + if (request.params[1].isArray() && request.params[2].isNull() && request.params[3].isNull()) { + // ignore_rejects used to occupy this position (v0.12.0.knots20160226.rc1-v0.17.1.knots20181229) + json_ign_rejs = &request.params[1]; + } else { + if (!request.params[1].isNull()) { + max_raw_tx_fee_rate = ParseFeeRate(self.Arg("maxfeerate")); + } + if (request.params[2].isArray() && request.params[3].isNull()) { + // ignore_rejects used to occupy this position (v0.18.0.knots20190502-v23.0.knots20220529) + json_ign_rejs = &request.params[2]; + } else if (!request.params[2].isNull()) { + max_burn_amount = AmountFromValue(request.params[2]); + } + } CMutableTransaction mtx; if (!DecodeHexTx(mtx, request.params[0].get_str())) { @@ -90,10 +111,6 @@ static RPCHelpMan sendrawtransaction() CTransactionRef tx(MakeTransactionRef(std::move(mtx))); - const UniValue* json_ign_rejs = &request.params[3]; - - const CFeeRate max_raw_tx_fee_rate{ParseFeeRate(self.Arg("maxfeerate"))}; - ignore_rejects_type ignore_rejects; if (!json_ign_rejs->isNull()) { for (size_t i = 0; i < json_ign_rejs->size(); ++i) { From 416646d1cacd88ff821090c8d76ea3ec2ff79daa Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Tue, 1 Aug 2023 19:25:45 +0000 Subject: [PATCH 15/21] Support ignoring package-fee-too-low rejection --- src/validation.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/validation.cpp b/src/validation.cpp index 6911afe499..e00d567e68 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1615,6 +1615,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: [](const auto& ws) { return ws.m_ptx->GetWitnessHash(); }); TxValidationState placeholder_state; if (args.m_package_feerates && + (!args.m_ignore_rejects.count("package-fee-too-low")) && !CheckFeeRate(m_subpackage.m_total_vsize, m_subpackage.m_total_modified_fees, placeholder_state, empty_ignore_rejects)) { package_state.Invalid(PackageValidationResult::PCKG_TX, "transaction failed"); return PackageMempoolAcceptResult(package_state, {{workspaces.back().m_ptx->GetWitnessHash(), From cf65351293764302a66d72c1d00c28bb61c0382d Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Fri, 31 May 2024 17:11:02 +0000 Subject: [PATCH 16/21] Give separate reject reasons to each TRUC check --- src/policy/truc_policy.cpp | 17 +++++++++++++++++ src/policy/truc_policy.h | 2 ++ src/test/txvalidation_tests.cpp | 12 ++++++++++++ src/validation.cpp | 9 +++++---- test/functional/mempool_truc.py | 34 ++++++++++++++++----------------- 5 files changed, 53 insertions(+), 21 deletions(-) diff --git a/src/policy/truc_policy.cpp b/src/policy/truc_policy.cpp index 69e8d5ed1d..51ead63f61 100644 --- a/src/policy/truc_policy.cpp +++ b/src/policy/truc_policy.cpp @@ -56,6 +56,7 @@ struct ParentInfo { }; std::optional PackageTRUCChecks(const CTransactionRef& ptx, int64_t vsize, + const std::string& reason_prefix, std::string& out_reason, const Package& package, const CTxMemPool::setEntries& mempool_ancestors) { @@ -69,11 +70,13 @@ std::optional PackageTRUCChecks(const CTransactionRef& ptx, int64_t if (ptx->version == TRUC_VERSION) { // SingleTRUCChecks should have checked this already. if (!Assume(vsize <= TRUC_MAX_VSIZE)) { + out_reason = reason_prefix + "vsize-toobig"; return strprintf("version=3 tx %s (wtxid=%s) is too big: %u > %u virtual bytes", ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(), vsize, TRUC_MAX_VSIZE); } if (mempool_ancestors.size() + in_package_parents.size() + 1 > TRUC_ANCESTOR_LIMIT) { + out_reason = reason_prefix + "ancestors-toomany"; return strprintf("tx %s (wtxid=%s) would have too many ancestors", ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString()); } @@ -82,6 +85,7 @@ std::optional PackageTRUCChecks(const CTransactionRef& ptx, int64_t if (has_parent) { // A TRUC child cannot be too large. if (vsize > TRUC_CHILD_MAX_VSIZE) { + out_reason = reason_prefix + "child-toobig"; return strprintf("version=3 child tx %s (wtxid=%s) is too big: %u > %u virtual bytes", ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(), vsize, TRUC_CHILD_MAX_VSIZE); @@ -107,6 +111,7 @@ std::optional PackageTRUCChecks(const CTransactionRef& ptx, int64_t // If there is a parent, it must have the right version. if (parent_info.m_version != TRUC_VERSION) { + out_reason = reason_prefix + "spends-nontruc"; return strprintf("version=3 tx %s (wtxid=%s) cannot spend from non-version=3 tx %s (wtxid=%s)", ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(), parent_info.m_txid.ToString(), parent_info.m_wtxid.ToString()); @@ -121,6 +126,7 @@ std::optional PackageTRUCChecks(const CTransactionRef& ptx, int64_t // sibling is to-be-replaced (done in SingleTRUCChecks) because these transactions // are within the same package. if (input.prevout.hash == parent_info.m_txid) { + out_reason = reason_prefix + "sibling-known"; return strprintf("tx %s (wtxid=%s) would exceed descendant count limit", parent_info.m_txid.ToString(), parent_info.m_wtxid.ToString()); @@ -128,6 +134,7 @@ std::optional PackageTRUCChecks(const CTransactionRef& ptx, int64_t // This tx can't have both a parent and an in-package child. if (input.prevout.hash == ptx->GetHash()) { + out_reason = reason_prefix + "parent-and-child-both"; return strprintf("tx %s (wtxid=%s) would have too many ancestors", package_tx->GetHash().ToString(), package_tx->GetWitnessHash().ToString()); } @@ -135,6 +142,7 @@ std::optional PackageTRUCChecks(const CTransactionRef& ptx, int64_t } if (parent_info.m_has_mempool_descendant) { + out_reason = reason_prefix + "descendant-toomany"; return strprintf("tx %s (wtxid=%s) would exceed descendant count limit", parent_info.m_txid.ToString(), parent_info.m_wtxid.ToString()); } @@ -143,6 +151,7 @@ std::optional PackageTRUCChecks(const CTransactionRef& ptx, int64_t // Non-TRUC transactions cannot have TRUC parents. for (auto it : mempool_ancestors) { if (it->GetTx().version == TRUC_VERSION) { + out_reason = reason_prefix + "spent-by-nontruc"; return strprintf("non-version=3 tx %s (wtxid=%s) cannot spend from version=3 tx %s (wtxid=%s)", ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(), it->GetSharedTx()->GetHash().ToString(), it->GetSharedTx()->GetWitnessHash().ToString()); @@ -150,6 +159,7 @@ std::optional PackageTRUCChecks(const CTransactionRef& ptx, int64_t } for (const auto& index: in_package_parents) { if (package.at(index)->version == TRUC_VERSION) { + out_reason = reason_prefix + "spent-by-nontruc"; return strprintf("non-version=3 tx %s (wtxid=%s) cannot spend from version=3 tx %s (wtxid=%s)", ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(), @@ -162,6 +172,7 @@ std::optional PackageTRUCChecks(const CTransactionRef& ptx, int64_t } std::optional> SingleTRUCChecks(const CTransactionRef& ptx, + const std::string& reason_prefix, std::string& out_reason, const CTxMemPool::setEntries& mempool_ancestors, const std::set& direct_conflicts, int64_t vsize) @@ -169,11 +180,13 @@ std::optional> SingleTRUCChecks(const CT // Check TRUC and non-TRUC inheritance. for (const auto& entry : mempool_ancestors) { if (ptx->version != TRUC_VERSION && entry->GetTx().version == TRUC_VERSION) { + out_reason = reason_prefix + "spent-by-nontruc"; return std::make_pair(strprintf("non-version=3 tx %s (wtxid=%s) cannot spend from version=3 tx %s (wtxid=%s)", ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(), entry->GetSharedTx()->GetHash().ToString(), entry->GetSharedTx()->GetWitnessHash().ToString()), nullptr); } else if (ptx->version == TRUC_VERSION && entry->GetTx().version != TRUC_VERSION) { + out_reason = reason_prefix + "spends-nontruc"; return std::make_pair(strprintf("version=3 tx %s (wtxid=%s) cannot spend from non-version=3 tx %s (wtxid=%s)", ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(), entry->GetSharedTx()->GetHash().ToString(), entry->GetSharedTx()->GetWitnessHash().ToString()), @@ -189,6 +202,7 @@ std::optional> SingleTRUCChecks(const CT if (ptx->version != TRUC_VERSION) return std::nullopt; if (vsize > TRUC_MAX_VSIZE) { + out_reason = reason_prefix + "vsize-toobig"; return std::make_pair(strprintf("version=3 tx %s (wtxid=%s) is too big: %u > %u virtual bytes", ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(), vsize, TRUC_MAX_VSIZE), nullptr); @@ -196,6 +210,7 @@ std::optional> SingleTRUCChecks(const CT // Check that TRUC_ANCESTOR_LIMIT would not be violated. if (mempool_ancestors.size() + 1 > TRUC_ANCESTOR_LIMIT) { + out_reason = reason_prefix + "ancestors-toomany"; return std::make_pair(strprintf("tx %s (wtxid=%s) would have too many ancestors", ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString()), nullptr); @@ -205,6 +220,7 @@ std::optional> SingleTRUCChecks(const CT if (mempool_ancestors.size() > 0) { // If this transaction spends TRUC parents, it cannot be too large. if (vsize > TRUC_CHILD_MAX_VSIZE) { + out_reason = reason_prefix + "child-toobig"; return std::make_pair(strprintf("version=3 child tx %s (wtxid=%s) is too big: %u > %u virtual bytes", ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(), vsize, TRUC_CHILD_MAX_VSIZE), nullptr); @@ -233,6 +249,7 @@ std::optional> SingleTRUCChecks(const CT // Return the sibling if its eviction can be considered. Provide the "descendant count // limit" string either way, as the caller may decide not to do sibling eviction. + out_reason = reason_prefix + "descendants-toomany"; return std::make_pair(strprintf("tx %u (wtxid=%s) would exceed descendant count limit", parent_entry->GetSharedTx()->GetHash().ToString(), parent_entry->GetSharedTx()->GetWitnessHash().ToString()), diff --git a/src/policy/truc_policy.h b/src/policy/truc_policy.h index dbc77696c6..201d4159ed 100644 --- a/src/policy/truc_policy.h +++ b/src/policy/truc_policy.h @@ -62,6 +62,7 @@ static_assert(TRUC_MAX_VSIZE + TRUC_CHILD_MAX_VSIZE <= DEFAULT_DESCENDANT_SIZE_L * applicable. */ std::optional> SingleTRUCChecks(const CTransactionRef& ptx, + const std::string& reason_prefix, std::string& out_reason, const CTxMemPool::setEntries& mempool_ancestors, const std::set& direct_conflicts, int64_t vsize); @@ -88,6 +89,7 @@ std::optional> SingleTRUCChecks(const CT * @returns debug string if an error occurs, std::nullopt otherwise. * */ std::optional PackageTRUCChecks(const CTransactionRef& ptx, int64_t vsize, + const std::string& reason_prefix, std::string& out_reason, const Package& package, const CTxMemPool::setEntries& mempool_ancestors); diff --git a/src/test/txvalidation_tests.cpp b/src/test/txvalidation_tests.cpp index d495a1135f..c559cb1d1f 100644 --- a/src/test/txvalidation_tests.cpp +++ b/src/test/txvalidation_tests.cpp @@ -20,6 +20,18 @@ BOOST_AUTO_TEST_SUITE(txvalidation_tests) +std::optional> SingleTRUCChecks(const CTransactionRef& ptx, const CTxMemPool::setEntries& mempool_ancestors, const std::set& direct_conflicts, int64_t vsize) +{ + std::string dummy; + return SingleTRUCChecks(ptx, dummy, dummy, mempool_ancestors, direct_conflicts, vsize); +} + +std::optional PackageTRUCChecks(const CTransactionRef& ptx, int64_t vsize, const Package& package, const CTxMemPool::setEntries& mempool_ancestors) +{ + std::string dummy; + return PackageTRUCChecks(ptx, vsize, dummy, dummy, package, mempool_ancestors); +} + /** * Ensure that the mempool won't accept coinbase transactions. */ diff --git a/src/validation.cpp b/src/validation.cpp index e00d567e68..0efa8d5486 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1063,7 +1063,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // Even though just checking direct mempool parents for inheritance would be sufficient, we // check using the full ancestor set here because it's more convenient to use what we have // already calculated. - if (const auto err{SingleTRUCChecks(ws.m_ptx, ws.m_ancestors, ws.m_conflicts, ws.m_vsize)}) { + if (const auto err{SingleTRUCChecks(ws.m_ptx, "truc-", reason, ws.m_ancestors, ws.m_conflicts, ws.m_vsize)}) { // Single transaction contexts only. if (args.m_allow_sibling_eviction && err->second != nullptr) { // We should only be considering where replacement is considered valid as well. @@ -1082,7 +1082,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // (which is normally done in PreChecks). However, the only way a TRUC transaction can // have a non-TRUC and non-BIP125 descendant is due to a reorg. } else { - return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "TRUC-violation", err->first); + return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, reason, err->first); } } @@ -1588,9 +1588,10 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: // At this point we have all in-mempool ancestors, and we know every transaction's vsize. // Run the TRUC checks on the package. + std::string reason; for (Workspace& ws : workspaces) { - if (auto err{PackageTRUCChecks(ws.m_ptx, ws.m_vsize, txns, ws.m_ancestors)}) { - package_state.Invalid(PackageValidationResult::PCKG_POLICY, "TRUC-violation", err.value()); + if (auto err{PackageTRUCChecks(ws.m_ptx, ws.m_vsize, "truc-", reason, txns, ws.m_ancestors)}) { + package_state.Invalid(PackageValidationResult::PCKG_POLICY, reason, err.value()); return PackageMempoolAcceptResult(package_state, {}); } } diff --git a/test/functional/mempool_truc.py b/test/functional/mempool_truc.py index 28f3256ef1..6082f87973 100755 --- a/test/functional/mempool_truc.py +++ b/test/functional/mempool_truc.py @@ -57,7 +57,7 @@ class MempoolTRUC(BitcoinTestFramework): self.log.info("Test TRUC-specific maximum transaction vsize") tx_v3_heavy = self.wallet.create_self_transfer(target_weight=(TRUC_MAX_VSIZE + 1) * WITNESS_SCALE_FACTOR, version=3) assert_greater_than_or_equal(tx_v3_heavy["tx"].get_vsize(), TRUC_MAX_VSIZE) - expected_error_heavy = f"TRUC-violation, version=3 tx {tx_v3_heavy['txid']} (wtxid={tx_v3_heavy['wtxid']}) is too big" + expected_error_heavy = f"truc-vsize-toobig, version=3 tx {tx_v3_heavy['txid']} (wtxid={tx_v3_heavy['wtxid']}) is too big" assert_raises_rpc_error(-26, expected_error_heavy, node.sendrawtransaction, tx_v3_heavy["hex"]) self.check_mempool([]) @@ -77,7 +77,7 @@ class MempoolTRUC(BitcoinTestFramework): version=3 ) assert_greater_than_or_equal(tx_v3_child_heavy["tx"].get_vsize(), 1000) - expected_error_child_heavy = f"TRUC-violation, version=3 child tx {tx_v3_child_heavy['txid']} (wtxid={tx_v3_child_heavy['wtxid']}) is too big" + expected_error_child_heavy = f"truc-child-toobig, version=3 child tx {tx_v3_child_heavy['txid']} (wtxid={tx_v3_child_heavy['wtxid']}) is too big" assert_raises_rpc_error(-26, expected_error_child_heavy, node.sendrawtransaction, tx_v3_child_heavy["hex"]) self.check_mempool([tx_v3_parent_normal["txid"]]) # tx has no descendants @@ -157,7 +157,7 @@ class MempoolTRUC(BitcoinTestFramework): utxo_to_spend=tx_v3_parent["new_utxo"], version=2 ) - expected_error_v2_v3 = f"TRUC-violation, non-version=3 tx {tx_v3_child_rbf_v2['txid']} (wtxid={tx_v3_child_rbf_v2['wtxid']}) cannot spend from version=3 tx {tx_v3_parent['txid']} (wtxid={tx_v3_parent['wtxid']})" + expected_error_v2_v3 = f"truc-spent-by-nontruc, non-version=3 tx {tx_v3_child_rbf_v2['txid']} (wtxid={tx_v3_child_rbf_v2['wtxid']}) cannot spend from version=3 tx {tx_v3_parent['txid']} (wtxid={tx_v3_parent['wtxid']})" assert_raises_rpc_error(-26, expected_error_v2_v3, node.sendrawtransaction, tx_v3_child_rbf_v2["hex"]) self.check_mempool([tx_v3_bip125_rbf_v2["txid"], tx_v3_parent["txid"], tx_v3_child["txid"]]) @@ -289,20 +289,20 @@ class MempoolTRUC(BitcoinTestFramework): self.check_mempool([]) result = node.submitpackage([tx_v3_parent_normal["hex"], tx_v3_parent_2_normal["hex"], tx_v3_child_multiparent["hex"]]) - assert_equal(result['package_msg'], f"TRUC-violation, tx {tx_v3_child_multiparent['txid']} (wtxid={tx_v3_child_multiparent['wtxid']}) would have too many ancestors") + assert_equal(result['package_msg'], f"truc-ancestors-toomany, tx {tx_v3_child_multiparent['txid']} (wtxid={tx_v3_child_multiparent['wtxid']}) would have too many ancestors") self.check_mempool([]) self.check_mempool([]) result = node.submitpackage([tx_v3_parent_normal["hex"], tx_v3_child_heavy["hex"]]) # tx_v3_child_heavy is heavy based on weight, not sigops. - assert_equal(result['package_msg'], f"TRUC-violation, version=3 child tx {tx_v3_child_heavy['txid']} (wtxid={tx_v3_child_heavy['wtxid']}) is too big: {tx_v3_child_heavy['tx'].get_vsize()} > 1000 virtual bytes") + assert_equal(result['package_msg'], f"truc-child-toobig, version=3 child tx {tx_v3_child_heavy['txid']} (wtxid={tx_v3_child_heavy['wtxid']}) is too big: {tx_v3_child_heavy['tx'].get_vsize()} > 1000 virtual bytes") self.check_mempool([]) tx_v3_parent = self.wallet.create_self_transfer(version=3) tx_v3_child = self.wallet.create_self_transfer(utxo_to_spend=tx_v3_parent["new_utxo"], version=3) tx_v3_grandchild = self.wallet.create_self_transfer(utxo_to_spend=tx_v3_child["new_utxo"], version=3) result = node.testmempoolaccept([tx_v3_parent["hex"], tx_v3_child["hex"], tx_v3_grandchild["hex"]]) - assert all([txresult["package-error"] == f"TRUC-violation, tx {tx_v3_grandchild['txid']} (wtxid={tx_v3_grandchild['wtxid']}) would have too many ancestors" for txresult in result]) + assert all([txresult["package-error"] == f"truc-parent-and-child-both, tx {tx_v3_grandchild['txid']} (wtxid={tx_v3_grandchild['wtxid']}) would have too many ancestors" for txresult in result]) @cleanup(extra_args=None) def test_truc_ancestors_package_and_mempool(self): @@ -331,7 +331,7 @@ class MempoolTRUC(BitcoinTestFramework): # submitpackage(B, C) should fail result = node.submitpackage([tx_0fee_parent["hex"], tx_child_violator["hex"]]) - assert_equal(result['package_msg'], f"TRUC-violation, tx {tx_child_violator['txid']} (wtxid={tx_child_violator['wtxid']}) would have too many ancestors") + assert_equal(result['package_msg'], f"truc-parent-and-child-both, tx {tx_child_violator['txid']} (wtxid={tx_child_violator['wtxid']}) would have too many ancestors") self.check_mempool([tx_in_mempool["txid"]]) @cleanup(extra_args=None) @@ -384,17 +384,17 @@ class MempoolTRUC(BitcoinTestFramework): # Fails with another non-related transaction via testmempoolaccept tx_unrelated = self.wallet.create_self_transfer(version=3) result_test_unrelated = node.testmempoolaccept([tx_sibling_1["hex"], tx_unrelated["hex"]]) - assert_equal(result_test_unrelated[0]["reject-reason"], "TRUC-violation") + assert_equal(result_test_unrelated[0]["reject-reason"], "truc-descendants-toomany") # Fails in a package via testmempoolaccept result_test_1p1c = node.testmempoolaccept([tx_sibling_1["hex"], tx_has_mempool_uncle["hex"]]) - assert_equal(result_test_1p1c[0]["reject-reason"], "TRUC-violation") + assert_equal(result_test_1p1c[0]["reject-reason"], "truc-descendants-toomany") # Allowed when tx is submitted in a package and evaluated individually. # Note that the child failed since it would be the 3rd generation. result_package_indiv = node.submitpackage([tx_sibling_1["hex"], tx_has_mempool_uncle["hex"]]) self.check_mempool([tx_mempool_parent["txid"], tx_sibling_1["txid"]]) - expected_error_gen3 = f"TRUC-violation, tx {tx_has_mempool_uncle['txid']} (wtxid={tx_has_mempool_uncle['wtxid']}) would have too many ancestors" + expected_error_gen3 = f"truc-ancestors-toomany, tx {tx_has_mempool_uncle['txid']} (wtxid={tx_has_mempool_uncle['wtxid']}) would have too many ancestors" assert_equal(result_package_indiv["tx-results"][tx_has_mempool_uncle['wtxid']]['error'], expected_error_gen3) @@ -405,7 +405,7 @@ class MempoolTRUC(BitcoinTestFramework): # Child cannot pay for sibling eviction for parent, as it violates TRUC topology limits result_package_cpfp = node.submitpackage([tx_sibling_3["hex"], tx_bumps_parent_with_sibling["hex"]]) self.check_mempool([tx_mempool_parent["txid"], tx_sibling_2["txid"]]) - expected_error_cpfp = f"TRUC-violation, tx {tx_mempool_parent['txid']} (wtxid={tx_mempool_parent['wtxid']}) would exceed descendant count limit" + expected_error_cpfp = f"truc-descendants-toomany, tx {tx_mempool_parent['txid']} (wtxid={tx_mempool_parent['wtxid']}) would exceed descendant count limit" assert_equal(result_package_cpfp["tx-results"][tx_sibling_3['wtxid']]['error'], expected_error_cpfp) @@ -426,7 +426,7 @@ class MempoolTRUC(BitcoinTestFramework): ) self.check_mempool([]) result = node.submitpackage([tx_v3_parent["hex"], tx_v2_child["hex"]]) - assert_equal(result['package_msg'], f"TRUC-violation, non-version=3 tx {tx_v2_child['txid']} (wtxid={tx_v2_child['wtxid']}) cannot spend from version=3 tx {tx_v3_parent['txid']} (wtxid={tx_v3_parent['wtxid']})") + assert_equal(result['package_msg'], f"truc-spent-by-nontruc, non-version=3 tx {tx_v2_child['txid']} (wtxid={tx_v2_child['wtxid']}) cannot spend from version=3 tx {tx_v3_parent['txid']} (wtxid={tx_v3_parent['wtxid']})") self.check_mempool([]) @cleanup(extra_args=None) @@ -447,11 +447,11 @@ class MempoolTRUC(BitcoinTestFramework): assert all([result["allowed"] for result in test_accept_v2_and_v3]) test_accept_v3_from_v2 = node.testmempoolaccept([tx_v2["hex"], tx_v3_from_v2["hex"]]) - expected_error_v3_from_v2 = f"TRUC-violation, version=3 tx {tx_v3_from_v2['txid']} (wtxid={tx_v3_from_v2['wtxid']}) cannot spend from non-version=3 tx {tx_v2['txid']} (wtxid={tx_v2['wtxid']})" + expected_error_v3_from_v2 = f"truc-spends-nontruc, version=3 tx {tx_v3_from_v2['txid']} (wtxid={tx_v3_from_v2['wtxid']}) cannot spend from non-version=3 tx {tx_v2['txid']} (wtxid={tx_v2['wtxid']})" assert all([result["package-error"] == expected_error_v3_from_v2 for result in test_accept_v3_from_v2]) test_accept_v2_from_v3 = node.testmempoolaccept([tx_v3["hex"], tx_v2_from_v3["hex"]]) - expected_error_v2_from_v3 = f"TRUC-violation, non-version=3 tx {tx_v2_from_v3['txid']} (wtxid={tx_v2_from_v3['wtxid']}) cannot spend from version=3 tx {tx_v3['txid']} (wtxid={tx_v3['wtxid']})" + expected_error_v2_from_v3 = f"truc-spent-by-nontruc, non-version=3 tx {tx_v2_from_v3['txid']} (wtxid={tx_v2_from_v3['wtxid']}) cannot spend from version=3 tx {tx_v3['txid']} (wtxid={tx_v3['wtxid']})" assert all([result["package-error"] == expected_error_v2_from_v3 for result in test_accept_v2_from_v3]) test_accept_pairs = node.testmempoolaccept([tx_v2["hex"], tx_v3["hex"], tx_v2_from_v2["hex"], tx_v3_from_v3["hex"]]) @@ -463,7 +463,7 @@ class MempoolTRUC(BitcoinTestFramework): tx_v3_child_1 = self.wallet.create_self_transfer(utxo_to_spend=tx_v3_parent["new_utxos"][0], version=3) tx_v3_child_2 = self.wallet.create_self_transfer(utxo_to_spend=tx_v3_parent["new_utxos"][1], version=3) test_accept_2children = node.testmempoolaccept([tx_v3_parent["hex"], tx_v3_child_1["hex"], tx_v3_child_2["hex"]]) - expected_error_2children = f"TRUC-violation, tx {tx_v3_parent['txid']} (wtxid={tx_v3_parent['wtxid']}) would exceed descendant count limit" + expected_error_2children = f"truc-sibling-known, tx {tx_v3_parent['txid']} (wtxid={tx_v3_parent['wtxid']}) would exceed descendant count limit" assert all([result["package-error"] == expected_error_2children for result in test_accept_2children]) # Extra TRUC transaction does not get incorrectly marked as extra descendant @@ -472,7 +472,7 @@ class MempoolTRUC(BitcoinTestFramework): # Extra TRUC transaction does not make us ignore the extra descendant test_accept_2children_with_exra = node.testmempoolaccept([tx_v3_parent["hex"], tx_v3_child_1["hex"], tx_v3_child_2["hex"], tx_v3_independent["hex"]]) - expected_error_extra = f"TRUC-violation, tx {tx_v3_parent['txid']} (wtxid={tx_v3_parent['wtxid']}) would exceed descendant count limit" + expected_error_extra = f"truc-sibling-known, tx {tx_v3_parent['txid']} (wtxid={tx_v3_parent['wtxid']}) would exceed descendant count limit" assert all([result["package-error"] == expected_error_extra for result in test_accept_2children_with_exra]) # Same result if the parent is already in mempool node.sendrawtransaction(tx_v3_parent["hex"]) @@ -609,7 +609,7 @@ class MempoolTRUC(BitcoinTestFramework): utxo_to_spend=tx_with_multi_children["new_utxos"][2], fee_rate=DEFAULT_FEE*50 ) - expected_error_2siblings = f"TRUC-violation, tx {tx_with_multi_children['txid']} (wtxid={tx_with_multi_children['wtxid']}) would exceed descendant count limit" + expected_error_2siblings = f"truc-descendants-toomany, tx {tx_with_multi_children['txid']} (wtxid={tx_with_multi_children['wtxid']}) would exceed descendant count limit" assert_raises_rpc_error(-26, expected_error_2siblings, node.sendrawtransaction, tx_with_sibling3["hex"]) # However, an RBF (with conflicting inputs) is possible even if the resulting cluster size exceeds 2 From a25c4629a8f71646200b91f3c41e901f219ac457 Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Fri, 31 May 2024 17:27:50 +0000 Subject: [PATCH 17/21] Support manually overriding TRUC policy checks --- src/policy/truc_policy.cpp | 30 ++++++++++++++++-------------- src/policy/truc_policy.h | 2 ++ src/test/txvalidation_tests.cpp | 4 ++-- src/validation.cpp | 4 ++-- 4 files changed, 22 insertions(+), 18 deletions(-) diff --git a/src/policy/truc_policy.cpp b/src/policy/truc_policy.cpp index 51ead63f61..8adc9b7200 100644 --- a/src/policy/truc_policy.cpp +++ b/src/policy/truc_policy.cpp @@ -57,6 +57,7 @@ struct ParentInfo { std::optional PackageTRUCChecks(const CTransactionRef& ptx, int64_t vsize, const std::string& reason_prefix, std::string& out_reason, + const ignore_rejects_type& ignore_rejects, const Package& package, const CTxMemPool::setEntries& mempool_ancestors) { @@ -69,13 +70,13 @@ std::optional PackageTRUCChecks(const CTransactionRef& ptx, int64_t // Now we have all ancestors, so we can start checking TRUC rules. if (ptx->version == TRUC_VERSION) { // SingleTRUCChecks should have checked this already. - if (!Assume(vsize <= TRUC_MAX_VSIZE)) { + if (vsize > TRUC_MAX_VSIZE && !ignore_rejects.count(reason_prefix + "vsize-toobig")) { out_reason = reason_prefix + "vsize-toobig"; return strprintf("version=3 tx %s (wtxid=%s) is too big: %u > %u virtual bytes", ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(), vsize, TRUC_MAX_VSIZE); } - if (mempool_ancestors.size() + in_package_parents.size() + 1 > TRUC_ANCESTOR_LIMIT) { + if (mempool_ancestors.size() + in_package_parents.size() + 1 > TRUC_ANCESTOR_LIMIT && !ignore_rejects.count(reason_prefix + "ancestors-toomany")) { out_reason = reason_prefix + "ancestors-toomany"; return strprintf("tx %s (wtxid=%s) would have too many ancestors", ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString()); @@ -84,7 +85,7 @@ std::optional PackageTRUCChecks(const CTransactionRef& ptx, int64_t const bool has_parent{mempool_ancestors.size() + in_package_parents.size() > 0}; if (has_parent) { // A TRUC child cannot be too large. - if (vsize > TRUC_CHILD_MAX_VSIZE) { + if (vsize > TRUC_CHILD_MAX_VSIZE && !ignore_rejects.count(reason_prefix + "child-toobig")) { out_reason = reason_prefix + "child-toobig"; return strprintf("version=3 child tx %s (wtxid=%s) is too big: %u > %u virtual bytes", ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(), @@ -110,7 +111,7 @@ std::optional PackageTRUCChecks(const CTransactionRef& ptx, int64_t }(); // If there is a parent, it must have the right version. - if (parent_info.m_version != TRUC_VERSION) { + if (parent_info.m_version != TRUC_VERSION && !ignore_rejects.count(reason_prefix + "spends-nontruc")) { out_reason = reason_prefix + "spends-nontruc"; return strprintf("version=3 tx %s (wtxid=%s) cannot spend from non-version=3 tx %s (wtxid=%s)", ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(), @@ -125,7 +126,7 @@ std::optional PackageTRUCChecks(const CTransactionRef& ptx, int64_t // Fail if we find another tx with the same parent. We don't check whether the // sibling is to-be-replaced (done in SingleTRUCChecks) because these transactions // are within the same package. - if (input.prevout.hash == parent_info.m_txid) { + if (input.prevout.hash == parent_info.m_txid && !ignore_rejects.count(reason_prefix + "sibling-known")) { out_reason = reason_prefix + "sibling-known"; return strprintf("tx %s (wtxid=%s) would exceed descendant count limit", parent_info.m_txid.ToString(), @@ -133,7 +134,7 @@ std::optional PackageTRUCChecks(const CTransactionRef& ptx, int64_t } // This tx can't have both a parent and an in-package child. - if (input.prevout.hash == ptx->GetHash()) { + if (input.prevout.hash == ptx->GetHash() && !ignore_rejects.count(reason_prefix + "parent-and-child-both")) { out_reason = reason_prefix + "parent-and-child-both"; return strprintf("tx %s (wtxid=%s) would have too many ancestors", package_tx->GetHash().ToString(), package_tx->GetWitnessHash().ToString()); @@ -150,7 +151,7 @@ std::optional PackageTRUCChecks(const CTransactionRef& ptx, int64_t } else { // Non-TRUC transactions cannot have TRUC parents. for (auto it : mempool_ancestors) { - if (it->GetTx().version == TRUC_VERSION) { + if (it->GetTx().version == TRUC_VERSION && !ignore_rejects.count(reason_prefix + "spent-by-nontruc")) { out_reason = reason_prefix + "spent-by-nontruc"; return strprintf("non-version=3 tx %s (wtxid=%s) cannot spend from version=3 tx %s (wtxid=%s)", ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(), @@ -158,7 +159,7 @@ std::optional PackageTRUCChecks(const CTransactionRef& ptx, int64_t } } for (const auto& index: in_package_parents) { - if (package.at(index)->version == TRUC_VERSION) { + if (package.at(index)->version == TRUC_VERSION && !ignore_rejects.count(reason_prefix + "spent-by-nontruc")) { out_reason = reason_prefix + "spent-by-nontruc"; return strprintf("non-version=3 tx %s (wtxid=%s) cannot spend from version=3 tx %s (wtxid=%s)", ptx->GetHash().ToString(), @@ -173,19 +174,20 @@ std::optional PackageTRUCChecks(const CTransactionRef& ptx, int64_t std::optional> SingleTRUCChecks(const CTransactionRef& ptx, const std::string& reason_prefix, std::string& out_reason, + const ignore_rejects_type& ignore_rejects, const CTxMemPool::setEntries& mempool_ancestors, const std::set& direct_conflicts, int64_t vsize) { // Check TRUC and non-TRUC inheritance. for (const auto& entry : mempool_ancestors) { - if (ptx->version != TRUC_VERSION && entry->GetTx().version == TRUC_VERSION) { + if (ptx->version != TRUC_VERSION && entry->GetTx().version == TRUC_VERSION && !ignore_rejects.count(reason_prefix + "spent-by-nontruc")) { out_reason = reason_prefix + "spent-by-nontruc"; return std::make_pair(strprintf("non-version=3 tx %s (wtxid=%s) cannot spend from version=3 tx %s (wtxid=%s)", ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(), entry->GetSharedTx()->GetHash().ToString(), entry->GetSharedTx()->GetWitnessHash().ToString()), nullptr); - } else if (ptx->version == TRUC_VERSION && entry->GetTx().version != TRUC_VERSION) { + } else if (ptx->version == TRUC_VERSION && entry->GetTx().version != TRUC_VERSION && !ignore_rejects.count(reason_prefix + "spends-nontruc")) { out_reason = reason_prefix + "spends-nontruc"; return std::make_pair(strprintf("version=3 tx %s (wtxid=%s) cannot spend from non-version=3 tx %s (wtxid=%s)", ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(), @@ -201,7 +203,7 @@ std::optional> SingleTRUCChecks(const CT // The rest of the rules only apply to transactions with version=3. if (ptx->version != TRUC_VERSION) return std::nullopt; - if (vsize > TRUC_MAX_VSIZE) { + if (vsize > TRUC_MAX_VSIZE && !ignore_rejects.count(reason_prefix + "vsize-toobig")) { out_reason = reason_prefix + "vsize-toobig"; return std::make_pair(strprintf("version=3 tx %s (wtxid=%s) is too big: %u > %u virtual bytes", ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(), vsize, TRUC_MAX_VSIZE), @@ -209,7 +211,7 @@ std::optional> SingleTRUCChecks(const CT } // Check that TRUC_ANCESTOR_LIMIT would not be violated. - if (mempool_ancestors.size() + 1 > TRUC_ANCESTOR_LIMIT) { + if (mempool_ancestors.size() + 1 > TRUC_ANCESTOR_LIMIT && !ignore_rejects.count(reason_prefix + "ancestors-toomany")) { out_reason = reason_prefix + "ancestors-toomany"; return std::make_pair(strprintf("tx %s (wtxid=%s) would have too many ancestors", ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString()), @@ -219,7 +221,7 @@ std::optional> SingleTRUCChecks(const CT // Remaining checks only pertain to transactions with unconfirmed ancestors. if (mempool_ancestors.size() > 0) { // If this transaction spends TRUC parents, it cannot be too large. - if (vsize > TRUC_CHILD_MAX_VSIZE) { + if (vsize > TRUC_CHILD_MAX_VSIZE && !ignore_rejects.count(reason_prefix + "child-toobig")) { out_reason = reason_prefix + "child-toobig"; return std::make_pair(strprintf("version=3 child tx %s (wtxid=%s) is too big: %u > %u virtual bytes", ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(), vsize, TRUC_CHILD_MAX_VSIZE), @@ -238,7 +240,7 @@ std::optional> SingleTRUCChecks(const CT const bool child_will_be_replaced = !children.empty() && std::any_of(children.cbegin(), children.cend(), [&direct_conflicts](const CTxMemPoolEntry& child){return direct_conflicts.count(child.GetTx().GetHash()) > 0;}); - if (parent_entry->GetCountWithDescendants() + 1 > TRUC_DESCENDANT_LIMIT && !child_will_be_replaced) { + if (parent_entry->GetCountWithDescendants() + 1 > TRUC_DESCENDANT_LIMIT && (!child_will_be_replaced) && !ignore_rejects.count(reason_prefix + "descendants-toomany")) { // Allow sibling eviction for TRUC transaction: if another child already exists, even if // we don't conflict inputs with it, consider evicting it under RBF rules. We rely on TRUC rules // only permitting 1 descendant, as otherwise we would need to have logic for deciding diff --git a/src/policy/truc_policy.h b/src/policy/truc_policy.h index 201d4159ed..172e156efd 100644 --- a/src/policy/truc_policy.h +++ b/src/policy/truc_policy.h @@ -63,6 +63,7 @@ static_assert(TRUC_MAX_VSIZE + TRUC_CHILD_MAX_VSIZE <= DEFAULT_DESCENDANT_SIZE_L */ std::optional> SingleTRUCChecks(const CTransactionRef& ptx, const std::string& reason_prefix, std::string& out_reason, + const ignore_rejects_type& ignore_rejects, const CTxMemPool::setEntries& mempool_ancestors, const std::set& direct_conflicts, int64_t vsize); @@ -90,6 +91,7 @@ std::optional> SingleTRUCChecks(const CT * */ std::optional PackageTRUCChecks(const CTransactionRef& ptx, int64_t vsize, const std::string& reason_prefix, std::string& out_reason, + const ignore_rejects_type& ignore_rejects, const Package& package, const CTxMemPool::setEntries& mempool_ancestors); diff --git a/src/test/txvalidation_tests.cpp b/src/test/txvalidation_tests.cpp index c559cb1d1f..45c1275b5f 100644 --- a/src/test/txvalidation_tests.cpp +++ b/src/test/txvalidation_tests.cpp @@ -23,13 +23,13 @@ BOOST_AUTO_TEST_SUITE(txvalidation_tests) std::optional> SingleTRUCChecks(const CTransactionRef& ptx, const CTxMemPool::setEntries& mempool_ancestors, const std::set& direct_conflicts, int64_t vsize) { std::string dummy; - return SingleTRUCChecks(ptx, dummy, dummy, mempool_ancestors, direct_conflicts, vsize); + return SingleTRUCChecks(ptx, dummy, dummy, empty_ignore_rejects, mempool_ancestors, direct_conflicts, vsize); } std::optional PackageTRUCChecks(const CTransactionRef& ptx, int64_t vsize, const Package& package, const CTxMemPool::setEntries& mempool_ancestors) { std::string dummy; - return PackageTRUCChecks(ptx, vsize, dummy, dummy, package, mempool_ancestors); + return PackageTRUCChecks(ptx, vsize, dummy, dummy, empty_ignore_rejects, package, mempool_ancestors); } /** diff --git a/src/validation.cpp b/src/validation.cpp index 0efa8d5486..7c6e4d066c 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1063,7 +1063,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // Even though just checking direct mempool parents for inheritance would be sufficient, we // check using the full ancestor set here because it's more convenient to use what we have // already calculated. - if (const auto err{SingleTRUCChecks(ws.m_ptx, "truc-", reason, ws.m_ancestors, ws.m_conflicts, ws.m_vsize)}) { + if (const auto err{SingleTRUCChecks(ws.m_ptx, "truc-", reason, ignore_rejects, ws.m_ancestors, ws.m_conflicts, ws.m_vsize)}) { // Single transaction contexts only. if (args.m_allow_sibling_eviction && err->second != nullptr) { // We should only be considering where replacement is considered valid as well. @@ -1590,7 +1590,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: // Run the TRUC checks on the package. std::string reason; for (Workspace& ws : workspaces) { - if (auto err{PackageTRUCChecks(ws.m_ptx, ws.m_vsize, "truc-", reason, txns, ws.m_ancestors)}) { + if (auto err{PackageTRUCChecks(ws.m_ptx, ws.m_vsize, "truc-", reason, args.m_ignore_rejects, txns, ws.m_ancestors)}) { package_state.Invalid(PackageValidationResult::PCKG_POLICY, reason, err.value()); return PackageMempoolAcceptResult(package_state, {}); } From a1df69392dd49646b89bf9b708641db1436d84d1 Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Wed, 22 Jan 2025 18:16:09 +0000 Subject: [PATCH 18/21] Label and allow overriding bad-witness-anchor-not-empty rejections --- src/policy/policy.cpp | 2 +- test/functional/mempool_accept.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/policy/policy.cpp b/src/policy/policy.cpp index c4a5c99192..55a3bdd6b1 100644 --- a/src/policy/policy.cpp +++ b/src/policy/policy.cpp @@ -263,7 +263,7 @@ bool IsWitnessStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs, // witness stuffing detected if (prevScript.IsPayToAnchor()) { - return false; + MaybeReject("anchor-not-empty"); } bool p2sh = false; diff --git a/test/functional/mempool_accept.py b/test/functional/mempool_accept.py index 4d08575255..0150150276 100755 --- a/test/functional/mempool_accept.py +++ b/test/functional/mempool_accept.py @@ -405,7 +405,7 @@ class MempoolAcceptanceTest(BitcoinTestFramework): anchor_nonempty_wit_spend.rehash() self.check_mempool_result( - result_expected=[{'txid': anchor_nonempty_wit_spend.rehash(), 'allowed': False, 'reject-reason': 'bad-witness-nonstandard'}], + result_expected=[{'txid': anchor_nonempty_wit_spend.rehash(), 'allowed': False, 'reject-reason': 'bad-witness-anchor-not-empty'}], rawtxs=[anchor_nonempty_wit_spend.serialize().hex()], maxfeerate=0, ) From 1003e8b49f202aafb580669ac2aed3ccc34e28cc Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Wed, 22 Jan 2025 21:46:25 +0000 Subject: [PATCH 19/21] TRUC: Restore Assume for redundant check --- src/policy/truc_policy.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/policy/truc_policy.cpp b/src/policy/truc_policy.cpp index 8adc9b7200..2c637fda04 100644 --- a/src/policy/truc_policy.cpp +++ b/src/policy/truc_policy.cpp @@ -70,7 +70,7 @@ std::optional PackageTRUCChecks(const CTransactionRef& ptx, int64_t // Now we have all ancestors, so we can start checking TRUC rules. if (ptx->version == TRUC_VERSION) { // SingleTRUCChecks should have checked this already. - if (vsize > TRUC_MAX_VSIZE && !ignore_rejects.count(reason_prefix + "vsize-toobig")) { + if (!Assume(vsize <= TRUC_MAX_VSIZE || ignore_rejects.count(reason_prefix + "vsize-toobig"))) { out_reason = reason_prefix + "vsize-toobig"; return strprintf("version=3 tx %s (wtxid=%s) is too big: %u > %u virtual bytes", ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(), vsize, TRUC_MAX_VSIZE); From 8e36e6951d6e0cffcedaf431050f1026599a9214 Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Wed, 22 Jan 2025 22:04:56 +0000 Subject: [PATCH 20/21] Bugfix: Ignore "min relay fee not met" using the correct ignore_rejects string --- src/validation.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/validation.cpp b/src/validation.cpp index 7c6e4d066c..4aa641a224 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -962,7 +962,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // method of ensuring the tx remains bumped. For example, the fee-bumping child could disappear // due to a replacement. // The only exception is TRUC transactions. - if (ws.m_ptx->version != TRUC_VERSION && ws.m_modified_fees < m_pool.m_opts.min_relay_feerate.GetFee(ws.m_vsize) && !args.m_ignore_rejects.count(rejectmsg_mempoolfull)) { + if (ws.m_ptx->version != TRUC_VERSION && ws.m_modified_fees < m_pool.m_opts.min_relay_feerate.GetFee(ws.m_vsize) && !args.m_ignore_rejects.count(rejectmsg_lowfee_relay)) { // Even though this is a fee-related failure, this result is TX_MEMPOOL_POLICY, not // TX_RECONSIDERABLE, because it cannot be bypassed using package validation. return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "min relay fee not met", From b7c5c0c389e6f6a2d19434eae96b55304315960a Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Thu, 23 Jan 2025 00:42:05 +0000 Subject: [PATCH 21/21] Support ignoring "truc-descendant-toomany" rejection --- src/policy/truc_policy.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/policy/truc_policy.cpp b/src/policy/truc_policy.cpp index 2c637fda04..feda0fd83e 100644 --- a/src/policy/truc_policy.cpp +++ b/src/policy/truc_policy.cpp @@ -142,7 +142,7 @@ std::optional PackageTRUCChecks(const CTransactionRef& ptx, int64_t } } - if (parent_info.m_has_mempool_descendant) { + if (parent_info.m_has_mempool_descendant && !ignore_rejects.count(reason_prefix + "descendant-toomany")) { out_reason = reason_prefix + "descendant-toomany"; return strprintf("tx %s (wtxid=%s) would exceed descendant count limit", parent_info.m_txid.ToString(), parent_info.m_wtxid.ToString());