From 90b60a16731ac756dce44c29f57d111f4dffba75 Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 3 Nov 2022 12:36:12 +0000 Subject: [PATCH] cpfp carveout is excluded in packages The behavior is not new, but this rule exits earlier than before. Previously, a carve out could have been granted in PreChecks() but then nullified in PackageMempoolChecks() when CheckPackageLimits() is called with the default limits. --- doc/policy/packages.md | 9 +++++-- src/validation.cpp | 30 +++++++++++++++------- test/functional/mempool_package_onemore.py | 19 ++++++++++---- 3 files changed, 42 insertions(+), 16 deletions(-) diff --git a/doc/policy/packages.md b/doc/policy/packages.md index dba270e4940675..7e983221c5f39c 100644 --- a/doc/policy/packages.md +++ b/doc/policy/packages.md @@ -48,8 +48,13 @@ The following rules are enforced for all packages: heavily connected, i.e. some transaction in the package is the ancestor or descendant of all the other transactions. -The following rules are only enforced for packages to be submitted to the mempool (not enforced for -test accepts): +* [CPFP Carve Out](./mempool-limits.md#CPFP-Carve-Out) is disabled in packaged contexts. (#21800) + + - *Rationale*: This carve out cannot be accurately applied when there are multiple transactions' + ancestors and descendants being considered at the same time. + +The following rules are only enforced for packages to be submitted to the mempool (not +enforced for test accepts): * Packages must be child-with-unconfirmed-parents packages. This also means packages must contain at least 2 transactions. (#22674) diff --git a/src/validation.cpp b/src/validation.cpp index 90f5897b5f30e9..eaf6f3213bc6f7 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -476,6 +476,9 @@ class MemPoolAccept */ const std::optional m_client_maxfeerate; + /** Whether CPFP carveout and RBF carveout are granted. */ + const bool m_allow_carveouts; + /** Parameters for single transaction mempool validation. */ static ATMPArgs SingleAccept(const CChainParams& chainparams, int64_t accept_time, bool bypass_limits, std::vector& coins_to_uncache, @@ -489,6 +492,7 @@ class MemPoolAccept /* m_package_submission */ false, /* m_package_feerates */ false, /* m_client_maxfeerate */ {}, // checked by caller + /* m_allow_carveouts */ true, }; } @@ -504,6 +508,7 @@ class MemPoolAccept /* m_package_submission */ false, // not submitting to mempool /* m_package_feerates */ false, /* m_client_maxfeerate */ {}, // checked by caller + /* m_allow_carveouts */ false, }; } @@ -519,6 +524,7 @@ class MemPoolAccept /* m_package_submission */ true, /* m_package_feerates */ true, /* m_client_maxfeerate */ client_maxfeerate, + /* m_allow_carveouts */ false, }; } @@ -533,6 +539,7 @@ class MemPoolAccept /* m_package_submission */ true, // do not LimitMempoolSize in Finalize() /* m_package_feerates */ false, // only 1 transaction /* m_client_maxfeerate */ package_args.m_client_maxfeerate, + /* m_allow_carveouts */ false, }; } @@ -547,7 +554,8 @@ class MemPoolAccept bool allow_replacement, bool package_submission, bool package_feerates, - std::optional client_maxfeerate) + std::optional client_maxfeerate, + bool allow_carveouts) : m_chainparams{chainparams}, m_accept_time{accept_time}, m_bypass_limits{bypass_limits}, @@ -556,7 +564,8 @@ class MemPoolAccept m_allow_replacement{allow_replacement}, m_package_submission{package_submission}, m_package_feerates{package_feerates}, - m_client_maxfeerate{client_maxfeerate} + m_client_maxfeerate{client_maxfeerate}, + m_allow_carveouts{allow_carveouts} { } }; @@ -909,7 +918,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) CTxMemPool::Limits maybe_rbf_limits = m_pool.m_opts.limits; // Calculate in-mempool ancestors, up to a limit. - if (ws.m_conflicts.size() == 1) { + if (ws.m_conflicts.size() == 1 && args.m_allow_carveouts) { // In general, when we receive an RBF transaction with mempool conflicts, we want to know whether we // would meet the chain limits after the conflicts have been removed. However, there isn't a practical // way to do this short of calculating the ancestor and descendant sets with an overlay cache of @@ -948,6 +957,13 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) ws.m_ancestors = std::move(*ancestors); } else { // If CalculateMemPoolAncestors fails second time, we want the original error string. + const auto error_message{util::ErrorString(ancestors).original}; + + // Carve-out is not allowed in this context; fail + if (!args.m_allow_carveouts) { + return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too-long-mempool-chain", error_message); + } + // Contracting/payment channels CPFP carve-out: // If the new transaction is relatively small (up to 40k weight) // and has at most one ancestor (ie ancestor limit of 2, including @@ -965,7 +981,6 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) .descendant_count = maybe_rbf_limits.descendant_count + 1, .descendant_size_vbytes = maybe_rbf_limits.descendant_size_vbytes + EXTRA_DESCENDANT_TX_SIZE_LIMIT, }; - const auto error_message{util::ErrorString(ancestors).original}; if (ws.m_vsize > EXTRA_DESCENDANT_TX_SIZE_LIMIT) { return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too-long-mempool-chain", error_message); } @@ -1418,11 +1433,8 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: MempoolAcceptResult::FeeFailure(placeholder_state, CFeeRate(m_total_modified_fees, m_total_vsize), all_package_wtxids)}}); } - // Apply package mempool ancestor/descendant limits. Skip if there is only one transaction, - // because it's unnecessary. Also, CPFP carve out can increase the limit for individual - // transactions, but this exemption is not extended to packages in CheckPackageLimits(). - std::string err_string; - if (txns.size() > 1 && !PackageMempoolChecks(txns, m_total_vsize, package_state)) { + // Apply package mempool ancestor/descendant limits. + if (!PackageMempoolChecks(txns, m_total_vsize, package_state)) { return PackageMempoolAcceptResult(package_state, std::move(results)); } diff --git a/test/functional/mempool_package_onemore.py b/test/functional/mempool_package_onemore.py index 921c190668e77e..76e5ad2ff8555a 100755 --- a/test/functional/mempool_package_onemore.py +++ b/test/functional/mempool_package_onemore.py @@ -48,21 +48,30 @@ def run_test(self): # Adding one more transaction on to the chain should fail. assert_raises_rpc_error(-26, "too-long-mempool-chain, too many unconfirmed ancestors [limit: 25]", self.chain_tx, [utxo]) - # ...even if it chains on from some point in the middle of the chain. + # ... or if it chains on from some point in the middle of the chain. assert_raises_rpc_error(-26, "too-long-mempool-chain, too many descendants", self.chain_tx, [chain[2]]) assert_raises_rpc_error(-26, "too-long-mempool-chain, too many descendants", self.chain_tx, [chain[1]]) # ...even if it chains on to two parent transactions with one in the chain. assert_raises_rpc_error(-26, "too-long-mempool-chain, too many descendants", self.chain_tx, [chain[0], second_chain]) # ...especially if its > 40k weight assert_raises_rpc_error(-26, "too-long-mempool-chain, too many descendants", self.chain_tx, [chain[0]], num_outputs=350) + # ...even if it's submitted with other transactions + replaceable_tx = self.wallet.create_self_transfer_multi(utxos_to_spend=[chain[0]]) + txns = [replaceable_tx["tx"], self.wallet.create_self_transfer_multi(utxos_to_spend=replaceable_tx["new_utxos"])["tx"]] + txns_hex = [tx.serialize().hex() for tx in txns] + assert_equal(self.nodes[0].testmempoolaccept(txns_hex)[0]["reject-reason"], "too-long-mempool-chain") + pkg_result = self.nodes[0].submitpackage(txns_hex) + assert "too-long-mempool-chain" in pkg_result["tx-results"][txns[0].getwtxid()]["error"] + assert_equal(pkg_result["tx-results"][txns[1].getwtxid()]["error"], "bad-txns-inputs-missingorspent") # But not if it chains directly off the first transaction - replacable_tx = self.wallet.send_self_transfer_multi(from_node=self.nodes[0], utxos_to_spend=[chain[0]])['tx'] + self.nodes[0].sendrawtransaction(replaceable_tx["hex"]) # and the second chain should work just fine self.chain_tx([second_chain]) - # Make sure we can RBF the chain which used our carve-out rule - replacable_tx.vout[0].nValue -= 1000000 - self.nodes[0].sendrawtransaction(replacable_tx.serialize().hex()) + # Ensure an individual transaction with single direct conflict can RBF the chain which used our carve-out rule + replacement_tx = replaceable_tx["tx"] + replacement_tx.vout[0].nValue -= 1000000 + self.nodes[0].sendrawtransaction(replacement_tx.serialize().hex()) # Finally, check that we added two transactions assert_equal(len(self.nodes[0].getrawmempool()), DEFAULT_ANCESTOR_LIMIT + 3)