From ac463e87df728689701810c3961155c49fdc5b31 Mon Sep 17 00:00:00 2001 From: glozow Date: Tue, 17 Jan 2023 11:00:49 +0000 Subject: [test util] mock mempool minimum feerate --- src/test/util/setup_common.cpp | 27 +++++++++++++++++++++++++++ src/test/util/setup_common.h | 12 ++++++++++++ 2 files changed, 39 insertions(+) diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 58593c9d5b..4992dbd956 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -432,6 +432,33 @@ std::vector TestChain100Setup::PopulateMempool(FastRandomContex return mempool_transactions; } +void TestChain100Setup::MockMempoolMinFee(const CFeeRate& target_feerate) +{ + LOCK2(cs_main, m_node.mempool->cs); + // Transactions in the mempool will affect the new minimum feerate. + assert(m_node.mempool->size() == 0); + // The target feerate cannot be too low... + // ...otherwise the transaction's feerate will need to be negative. + assert(target_feerate > m_node.mempool->m_incremental_relay_feerate); + // ...otherwise this is not meaningful. The feerate policy uses the maximum of both feerates. + assert(target_feerate > m_node.mempool->m_min_relay_feerate); + + // Manually create an invalid transaction. Manually set the fee in the CTxMemPoolEntry to + // achieve the exact target feerate. + CMutableTransaction mtx = CMutableTransaction(); + mtx.vin.push_back(CTxIn{COutPoint{g_insecure_rand_ctx.rand256(), 0}}); + mtx.vout.push_back(CTxOut(1 * COIN, GetScriptForDestination(WitnessV0ScriptHash(CScript() << OP_TRUE)))); + const auto tx{MakeTransactionRef(mtx)}; + LockPoints lp; + // The new mempool min feerate is equal to the removed package's feerate + incremental feerate. + const auto tx_fee = target_feerate.GetFee(GetVirtualTransactionSize(*tx)) - + m_node.mempool->m_incremental_relay_feerate.GetFee(GetVirtualTransactionSize(*tx)); + m_node.mempool->addUnchecked(CTxMemPoolEntry(tx, /*fee=*/tx_fee, + /*time=*/0, /*entry_height=*/1, + /*spends_coinbase=*/true, /*sigops_cost=*/1, lp)); + m_node.mempool->TrimToSize(0); + assert(m_node.mempool->GetMinFee() == target_feerate); +} /** * @returns a real block (0000000000013b8ab2cd513b0261a14096412195a72a0c4827d229dcc7e0f7af) * with 9 txs. diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h index 8874db7e75..10a6b19d03 100644 --- a/src/test/util/setup_common.h +++ b/src/test/util/setup_common.h @@ -23,6 +23,7 @@ #include #include +class CFeeRate; class Chainstate; /** This is connected to the logger. Can be used to redirect logs to any other log */ @@ -185,6 +186,17 @@ struct TestChain100Setup : public TestingSetup { */ std::vector PopulateMempool(FastRandomContext& det_rand, size_t num_transactions, bool submit); + /** Mock the mempool minimum feerate by adding a transaction and calling TrimToSize(0), + * simulating the mempool "reaching capacity" and evicting by descendant feerate. Note that + * this clears the mempool, and the new minimum feerate will depend on the maximum feerate of + * transactions removed, so this must be called while the mempool is empty. + * + * @param target_feerate The new mempool minimum feerate after this function returns. + * Must be above max(incremental feerate, min relay feerate), + * or 1sat/vB with default settings. + */ + void MockMempoolMinFee(const CFeeRate& target_feerate); + std::vector m_coinbase_txns; // For convenience, coinbase transactions CKey coinbaseKey; // private/public key needed to spend coinbase transactions }; -- cgit v1.2.3 From c4554fe894d7af8e666f5d424deccddf516713ef Mon Sep 17 00:00:00 2001 From: glozow Date: Tue, 17 Jan 2023 15:37:25 +0000 Subject: [test] package cpfp bumps parents =minrelaytxfee --- src/test/txpackage_tests.cpp | 52 +++++++++++++++++++++++----------------- test/functional/mempool_limit.py | 49 +++++++++++++++++++++++++++++++++++-- test/functional/rpc_packages.py | 40 ------------------------------- 3 files changed, 77 insertions(+), 64 deletions(-) diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp index 024526497c..af29c955bc 100644 --- a/src/test/txpackage_tests.cpp +++ b/src/test/txpackage_tests.cpp @@ -16,6 +16,9 @@ #include BOOST_AUTO_TEST_SUITE(txpackage_tests) +// A fee amount that is above 1sat/vB but below 5sat/vB for most transactions created within these +// unit tests. +static const CAmount low_fee_amt{200}; // Create placeholder transactions that have no meaning. inline CTransactionRef create_placeholder_tx(size_t num_inputs, size_t num_outputs) @@ -373,6 +376,7 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup) { // Mine blocks to mature coinbases. mineBlocks(5); + MockMempoolMinFee(CFeeRate(5000)); LOCK(cs_main); // Transactions with a same-txid-different-witness transaction in the mempool should be ignored, @@ -560,13 +564,15 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup) BOOST_CHECK(parent2_v2_result.m_result_type == MempoolAcceptResult::ResultType::VALID); package_mixed.push_back(ptx_parent2_v1); - // parent3 will be a new transaction. Put 0 fees on it to make it invalid on its own. + // parent3 will be a new transaction. Put a low feerate to make it invalid on its own. auto mtx_parent3 = CreateValidMempoolTransaction(/*input_transaction=*/m_coinbase_txns[3], /*input_vout=*/0, /*input_height=*/0, /*input_signing_key=*/coinbaseKey, /*output_destination=*/acs_spk, - /*output_amount=*/CAmount(50 * COIN), /*submit=*/false); + /*output_amount=*/CAmount(50 * COIN - low_fee_amt), /*submit=*/false); CTransactionRef ptx_parent3 = MakeTransactionRef(mtx_parent3); package_mixed.push_back(ptx_parent3); + BOOST_CHECK(m_node.mempool->GetMinFee().GetFee(GetVirtualTransactionSize(*ptx_parent3)) > low_fee_amt); + BOOST_CHECK(m_node.mempool->m_min_relay_feerate.GetFee(GetVirtualTransactionSize(*ptx_parent3)) <= low_fee_amt); // child spends parent1, parent2, and parent3 CKey mixed_grandchild_key; @@ -627,6 +633,7 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup) BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup) { mineBlocks(5); + MockMempoolMinFee(CFeeRate(5000)); LOCK(::cs_main); size_t expected_pool_size = m_node.mempool->size(); CKey child_key; @@ -636,9 +643,9 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup) grandchild_key.MakeNewKey(true); CScript child_spk = GetScriptForDestination(WitnessV0KeyHash(grandchild_key.GetPubKey())); - // zero-fee parent and high-fee child package + // low-fee parent and high-fee child package const CAmount coinbase_value{50 * COIN}; - const CAmount parent_value{coinbase_value - 0}; + const CAmount parent_value{coinbase_value - low_fee_amt}; const CAmount child_value{parent_value - COIN}; Package package_cpfp; @@ -657,9 +664,9 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup) package_cpfp.push_back(tx_child); // Package feerate is calculated using modified fees, and prioritisetransaction accepts negative - // fee deltas. This should be taken into account. De-prioritise the parent transaction by -1BTC, - // bringing the package feerate to 0. - m_node.mempool->PrioritiseTransaction(tx_parent->GetHash(), -1 * COIN); + // fee deltas. This should be taken into account. De-prioritise the parent transaction + // to bring the package feerate to 0. + m_node.mempool->PrioritiseTransaction(tx_parent->GetHash(), child_value - coinbase_value); { BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); const auto submit_cpfp_deprio = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, @@ -675,8 +682,8 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup) // Clear the prioritisation of the parent transaction. WITH_LOCK(m_node.mempool->cs, m_node.mempool->ClearPrioritisation(tx_parent->GetHash())); - // Package CPFP: Even though the parent pays 0 absolute fees, the child pays 1 BTC which is - // enough for the package feerate to meet the threshold. + // Package CPFP: Even though the parent's feerate is below the mempool minimum feerate, the + // child pays enough for the package feerate to meet the threshold. { BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); const auto submit_cpfp = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, @@ -689,7 +696,7 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup) auto it_child = submit_cpfp.m_tx_results.find(tx_child->GetWitnessHash()); BOOST_CHECK(it_parent != submit_cpfp.m_tx_results.end()); BOOST_CHECK(it_parent->second.m_result_type == MempoolAcceptResult::ResultType::VALID); - BOOST_CHECK(it_parent->second.m_base_fees.value() == 0); + BOOST_CHECK(it_parent->second.m_base_fees.value() == coinbase_value - parent_value); BOOST_CHECK(it_child != submit_cpfp.m_tx_results.end()); BOOST_CHECK(it_child->second.m_result_type == MempoolAcceptResult::ResultType::VALID); BOOST_CHECK(it_child->second.m_base_fees.value() == COIN); @@ -709,22 +716,28 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup) } // Just because we allow low-fee parents doesn't mean we allow low-feerate packages. - // This package just pays 200 satoshis total. This would be enough to pay for the child alone, - // but isn't enough for the entire package to meet the 1sat/vbyte minimum. + // The mempool minimum feerate is 5sat/vB, but this package just pays 800 satoshis total. + // The child fees would be able to pay for itself, but isn't enough for the entire package. Package package_still_too_low; + const CAmount parent_fee{200}; + const CAmount child_fee{600}; auto mtx_parent_cheap = CreateValidMempoolTransaction(/*input_transaction=*/m_coinbase_txns[1], /*input_vout=*/0, /*input_height=*/0, /*input_signing_key=*/coinbaseKey, /*output_destination=*/parent_spk, - /*output_amount=*/coinbase_value, /*submit=*/false); + /*output_amount=*/coinbase_value - parent_fee, /*submit=*/false); CTransactionRef tx_parent_cheap = MakeTransactionRef(mtx_parent_cheap); package_still_too_low.push_back(tx_parent_cheap); + BOOST_CHECK(m_node.mempool->GetMinFee().GetFee(GetVirtualTransactionSize(*tx_parent_cheap)) > parent_fee); + BOOST_CHECK(m_node.mempool->m_min_relay_feerate.GetFee(GetVirtualTransactionSize(*tx_parent_cheap)) <= parent_fee); auto mtx_child_cheap = CreateValidMempoolTransaction(/*input_transaction=*/tx_parent_cheap, /*input_vout=*/0, /*input_height=*/101, /*input_signing_key=*/child_key, /*output_destination=*/child_spk, - /*output_amount=*/coinbase_value - 200, /*submit=*/false); + /*output_amount=*/coinbase_value - parent_fee - child_fee, /*submit=*/false); CTransactionRef tx_child_cheap = MakeTransactionRef(mtx_child_cheap); package_still_too_low.push_back(tx_child_cheap); + BOOST_CHECK(m_node.mempool->GetMinFee().GetFee(GetVirtualTransactionSize(*tx_child_cheap)) <= child_fee); + BOOST_CHECK(m_node.mempool->GetMinFee().GetFee(GetVirtualTransactionSize(*tx_parent_cheap) + GetVirtualTransactionSize(*tx_child_cheap)) > parent_fee + child_fee); // Cheap package should fail with package-fee-too-low. { @@ -735,11 +748,6 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup) BOOST_CHECK_EQUAL(submit_package_too_low.m_state.GetResult(), PackageValidationResult::PCKG_POLICY); BOOST_CHECK_EQUAL(submit_package_too_low.m_state.GetRejectReason(), "package-fee-too-low"); BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); - const CFeeRate child_feerate(200, GetVirtualTransactionSize(*tx_child_cheap)); - BOOST_CHECK(child_feerate.GetFeePerK() > 1000); - const CFeeRate expected_feerate(200, - GetVirtualTransactionSize(*tx_parent_cheap) + GetVirtualTransactionSize(*tx_child_cheap)); - BOOST_CHECK(expected_feerate.GetFeePerK() < 1000); } // Package feerate includes the modified fees of the transactions. @@ -752,18 +760,18 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup) expected_pool_size += 2; BOOST_CHECK_MESSAGE(submit_prioritised_package.m_state.IsValid(), "Package validation unexpectedly failed" << submit_prioritised_package.m_state.GetRejectReason()); - const CFeeRate expected_feerate(1 * COIN + 200, + const CFeeRate expected_feerate(1 * COIN + parent_fee + child_fee, GetVirtualTransactionSize(*tx_parent_cheap) + GetVirtualTransactionSize(*tx_child_cheap)); BOOST_CHECK_EQUAL(submit_prioritised_package.m_tx_results.size(), package_still_too_low.size()); auto it_parent = submit_prioritised_package.m_tx_results.find(tx_parent_cheap->GetWitnessHash()); auto it_child = submit_prioritised_package.m_tx_results.find(tx_child_cheap->GetWitnessHash()); BOOST_CHECK(it_parent != submit_prioritised_package.m_tx_results.end()); BOOST_CHECK(it_parent->second.m_result_type == MempoolAcceptResult::ResultType::VALID); - BOOST_CHECK(it_parent->second.m_base_fees.value() == 0); + BOOST_CHECK(it_parent->second.m_base_fees.value() == parent_fee); BOOST_CHECK(it_parent->second.m_effective_feerate.value() == expected_feerate); BOOST_CHECK(it_child != submit_prioritised_package.m_tx_results.end()); BOOST_CHECK(it_child->second.m_result_type == MempoolAcceptResult::ResultType::VALID); - BOOST_CHECK(it_child->second.m_base_fees.value() == 200); + BOOST_CHECK(it_child->second.m_base_fees.value() == child_fee); BOOST_CHECK(it_child->second.m_effective_feerate.value() == expected_feerate); std::vector expected_wtxids({tx_parent_cheap->GetWitnessHash(), tx_child_cheap->GetWitnessHash()}); BOOST_CHECK(it_parent->second.m_wtxids_fee_calculations.value() == expected_wtxids); diff --git a/test/functional/mempool_limit.py b/test/functional/mempool_limit.py index d38a37f952..f8e86cafab 100755 --- a/test/functional/mempool_limit.py +++ b/test/functional/mempool_limit.py @@ -7,15 +7,21 @@ from decimal import Decimal from test_framework.blocktools import COINBASE_MATURITY +from test_framework.p2p import P2PTxInvStore from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_equal, + assert_fee_amount, assert_greater_than, assert_raises_rpc_error, create_lots_of_big_transactions, gen_return_txouts, ) -from test_framework.wallet import MiniWallet +from test_framework.wallet import ( + COIN, + DEFAULT_FEE, + MiniWallet, +) class MempoolLimitTest(BitcoinTestFramework): @@ -44,7 +50,8 @@ class MempoolLimitTest(BitcoinTestFramework): # 1 to create a tx initially that will be evicted from the mempool later # 3 batches of multiple transactions with a fee rate much higher than the previous UTXO # And 1 more to verify that this tx does not get added to the mempool with a fee rate less than the mempoolminfee - self.generate(miniwallet, 1 + (num_of_batches * tx_batch_size) + 1) + # And 2 more for the package cpfp test + self.generate(miniwallet, 1 + (num_of_batches * tx_batch_size) + 1 + 2) # Mine 99 blocks so that the UTXOs are allowed to be spent self.generate(node, COINBASE_MATURITY - 1) @@ -76,6 +83,44 @@ class MempoolLimitTest(BitcoinTestFramework): self.log.info('Create a mempool tx that will not pass mempoolminfee') assert_raises_rpc_error(-26, "mempool min fee not met", miniwallet.send_self_transfer, from_node=node, fee_rate=relayfee) + self.log.info("Check that submitpackage allows cpfp of a parent below mempool min feerate") + node = self.nodes[0] + peer = node.add_p2p_connection(P2PTxInvStore()) + + # Package with 2 parents and 1 child. One parent has a high feerate due to modified fees, + # another is below the mempool minimum feerate but bumped by the child. + tx_poor = miniwallet.create_self_transfer(fee_rate=relayfee) + tx_rich = miniwallet.create_self_transfer(fee=0, fee_rate=0) + node.prioritisetransaction(tx_rich["txid"], 0, int(DEFAULT_FEE * COIN)) + package_txns = [tx_rich, tx_poor] + coins = [tx["new_utxo"] for tx in package_txns] + tx_child = miniwallet.create_self_transfer_multi(utxos_to_spend=coins, fee_per_output=10000) #DEFAULT_FEE + package_txns.append(tx_child) + + submitpackage_result = node.submitpackage([tx["hex"] for tx in package_txns]) + + rich_parent_result = submitpackage_result["tx-results"][tx_rich["wtxid"]] + poor_parent_result = submitpackage_result["tx-results"][tx_poor["wtxid"]] + child_result = submitpackage_result["tx-results"][tx_child["tx"].getwtxid()] + assert_fee_amount(poor_parent_result["fees"]["base"], tx_poor["tx"].get_vsize(), relayfee) + assert_equal(rich_parent_result["fees"]["base"], 0) + assert_equal(child_result["fees"]["base"], DEFAULT_FEE) + # The "rich" parent does not require CPFP so its effective feerate is just its individual feerate. + assert_fee_amount(DEFAULT_FEE, tx_rich["tx"].get_vsize(), rich_parent_result["fees"]["effective-feerate"]) + assert_equal(rich_parent_result["fees"]["effective-includes"], [tx_rich["wtxid"]]) + # The "poor" parent and child's effective feerates are the same, composed of their total + # fees divided by their combined vsize. + package_fees = poor_parent_result["fees"]["base"] + child_result["fees"]["base"] + package_vsize = tx_poor["tx"].get_vsize() + tx_child["tx"].get_vsize() + assert_fee_amount(package_fees, package_vsize, poor_parent_result["fees"]["effective-feerate"]) + assert_fee_amount(package_fees, package_vsize, child_result["fees"]["effective-feerate"]) + assert_equal([tx_poor["wtxid"], tx_child["tx"].getwtxid()], poor_parent_result["fees"]["effective-includes"]) + assert_equal([tx_poor["wtxid"], tx_child["tx"].getwtxid()], child_result["fees"]["effective-includes"]) + + # The node will broadcast each transaction, still abiding by its peer's fee filter + peer.wait_for_broadcast([tx["tx"].getwtxid() for tx in package_txns]) + self.generate(node, 1) + self.log.info('Test passing a value below the minimum (5 MB) to -maxmempool throws an error') self.stop_node(0) self.nodes[0].assert_start_raises_init_error(["-maxmempool=4"], "Error: -maxmempool must be at least 5 MB") diff --git a/test/functional/rpc_packages.py b/test/functional/rpc_packages.py index 6cb9760b3d..ae1a498e28 100755 --- a/test/functional/rpc_packages.py +++ b/test/functional/rpc_packages.py @@ -20,7 +20,6 @@ from test_framework.util import ( assert_raises_rpc_error, ) from test_framework.wallet import ( - COIN, DEFAULT_FEE, MiniWallet, ) @@ -325,42 +324,6 @@ class RPCPackagesTest(BitcoinTestFramework): peer.wait_for_broadcast([tx["tx"].getwtxid() for tx in package_txns]) self.generate(node, 1) - def test_submit_cpfp(self): - node = self.nodes[0] - peer = node.add_p2p_connection(P2PTxInvStore()) - - # Package with 2 parents and 1 child. One parent pays for itself using modified fees, and - # another has 0 fees but is bumped by child. - tx_poor = self.wallet.create_self_transfer(fee=0, fee_rate=0) - tx_rich = self.wallet.create_self_transfer(fee=0, fee_rate=0) - node.prioritisetransaction(tx_rich["txid"], 0, int(DEFAULT_FEE * COIN)) - package_txns = [tx_rich, tx_poor] - coins = [tx["new_utxo"] for tx in package_txns] - tx_child = self.wallet.create_self_transfer_multi(utxos_to_spend=coins, fee_per_output=10000) #DEFAULT_FEE - package_txns.append(tx_child) - - submitpackage_result = node.submitpackage([tx["hex"] for tx in package_txns]) - - rich_parent_result = submitpackage_result["tx-results"][tx_rich["wtxid"]] - poor_parent_result = submitpackage_result["tx-results"][tx_poor["wtxid"]] - child_result = submitpackage_result["tx-results"][tx_child["tx"].getwtxid()] - assert_equal(rich_parent_result["fees"]["base"], 0) - assert_equal(poor_parent_result["fees"]["base"], 0) - assert_equal(child_result["fees"]["base"], DEFAULT_FEE) - # The "rich" parent does not require CPFP so its effective feerate. - assert_fee_amount(DEFAULT_FEE, tx_rich["tx"].get_vsize(), rich_parent_result["fees"]["effective-feerate"]) - assert_equal(rich_parent_result["fees"]["effective-includes"], [tx_rich["wtxid"]]) - # The "poor" parent and child's effective feerates are the same, composed of the child's fee - # divided by their combined vsize. - assert_fee_amount(DEFAULT_FEE, tx_poor["tx"].get_vsize() + tx_child["tx"].get_vsize(), poor_parent_result["fees"]["effective-feerate"]) - assert_fee_amount(DEFAULT_FEE, tx_poor["tx"].get_vsize() + tx_child["tx"].get_vsize(), child_result["fees"]["effective-feerate"]) - assert_equal([tx_poor["wtxid"], tx_child["tx"].getwtxid()], poor_parent_result["fees"]["effective-includes"]) - assert_equal([tx_poor["wtxid"], tx_child["tx"].getwtxid()], child_result["fees"]["effective-includes"]) - - # The node will broadcast each transaction, still abiding by its peer's fee filter - peer.wait_for_broadcast([tx["tx"].getwtxid() for tx in package_txns]) - self.generate(node, 1) - def test_submitpackage(self): node = self.nodes[0] @@ -369,9 +332,6 @@ class RPCPackagesTest(BitcoinTestFramework): self.test_submit_child_with_parents(num_parents, False) self.test_submit_child_with_parents(num_parents, True) - self.log.info("Submitpackage valid packages with CPFP") - self.test_submit_cpfp() - self.log.info("Submitpackage only allows packages of 1 child with its parents") # Chain of 3 transactions has too many generations chain_hex = [t["hex"] for t in self.wallet.create_self_transfer_chain(chain_length=25)] -- cgit v1.2.3 From 563a2ee4f564c8ea5f8313d711b196e260568c04 Mon Sep 17 00:00:00 2001 From: glozow Date: Tue, 17 Jan 2023 11:01:13 +0000 Subject: [policy] disallow transactions under min relay fee, even in packages Avoid adding transactions below min relay feerate because, even if they were bumped through CPFP when entering the mempool, we do not have a DoS-resistant way of ensuring they always remain bumped. In the future, this rule can be relaxed (e.g. to allow packages to bump 0-fee transactions) if we find a way to do so. --- doc/policy/packages.md | 37 +++++++++++++++++++++++++------------ src/test/txpackage_tests.cpp | 20 ++++++++++++++------ src/validation.cpp | 15 ++++++++++++--- 3 files changed, 51 insertions(+), 21 deletions(-) diff --git a/doc/policy/packages.md b/doc/policy/packages.md index 274854ddf9..2a5758318a 100644 --- a/doc/policy/packages.md +++ b/doc/policy/packages.md @@ -80,24 +80,37 @@ test accepts): If any transactions in the package are already in the mempool, they are not submitted again ("deduplicated") and are thus excluded from this calculation. -To meet the two feerate requirements of a mempool, i.e., the pre-configured minimum relay feerate -(`-minrelaytxfee`) and the dynamic mempool minimum feerate, the total package feerate is used instead -of the individual feerate. The individual transactions are allowed to be below the feerate -requirements if the package meets the feerate requirements. For example, the parent(s) in the -package can pay no fees but be paid for by the child. - -*Rationale*: This can be thought of as "CPFP within a package," solving the issue of a parent not -meeting minimum fees on its own. This would allow contracting applications to adjust their fees at -broadcast time instead of overshooting or risking becoming stuck or pinned. - -*Rationale*: It would be incorrect to use the fees of transactions that are already in the mempool, as -we do not want a transaction's fees to be double-counted. +To meet the dynamic mempool minimum feerate, i.e., the feerate determined by the transactions +evicted when the mempool reaches capacity (not the static minimum relay feerate), the total package +feerate instead of individual feerate can be used. For example, if the mempool minimum feerate is +5sat/vB and a 1sat/vB parent transaction has a high-feerate child, it may be accepted if +submitted as a package. + +*Rationale*: This can be thought of as "CPFP within a package," solving the issue of a presigned +transaction (i.e. in which a replacement transaction with a higher fee cannot be signed) being +rejected from the mempool when transaction volume is high and the mempool minimum feerate rises. + +Note: Package feerate cannot be used to meet the minimum relay feerate (`-minrelaytxfee`) +requirement. For example, if the mempool minimum feerate is 5sat/vB and the minimum relay feerate is +set to 5satvB, a 1sat/vB parent transaction with a high-feerate child will not be accepted, even if +submitted as a package. + +*Rationale*: Avoid situations in which the mempool contains non-bumped transactions below min relay +feerate (which we consider to have pay 0 fees and thus receiving free relay). While package +submission would ensure these transactions are bumped at the time of entry, it is not guaranteed +that the transaction will always be bumped. For example, a later transaction could replace the +fee-bumping child without still bumping the parent. These no-longer-bumped transactions should be +removed during a replacement, but we do not have a DoS-resistant way of removing them or enforcing a +limit on their quantity. Instead, prevent their entry into the mempool. Implementation Note: Transactions within a package are always validated individually first, and package validation is used for the transactions that failed. Since package feerate is only calculated using transactions that are not in the mempool, this implementation detail affects the outcome of package validation. +*Rationale*: It would be incorrect to use the fees of transactions that are already in the mempool, as +we do not want a transaction's fees to be double-counted. + *Rationale*: Packages are intended for incentive-compatible fee-bumping: transaction B is a "legitimate" fee-bump for transaction A only if B is a descendant of A and has a *higher* feerate than A. We want to prevent "parents pay for children" behavior; fees of parents should not help diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp index af29c955bc..c08d2748a6 100644 --- a/src/test/txpackage_tests.cpp +++ b/src/test/txpackage_tests.cpp @@ -671,10 +671,13 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup) BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); const auto submit_cpfp_deprio = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package_cpfp, /*test_accept=*/ false); - BOOST_CHECK_EQUAL(submit_cpfp_deprio.m_state.GetResult(), PackageValidationResult::PCKG_POLICY); - BOOST_CHECK_MESSAGE(submit_cpfp_deprio.m_state.IsInvalid(), - "Package validation unexpectedly succeeded: " << submit_cpfp_deprio.m_state.GetRejectReason()); - BOOST_CHECK(submit_cpfp_deprio.m_tx_results.empty()); + BOOST_CHECK_EQUAL(submit_cpfp_deprio.m_state.GetResult(), PackageValidationResult::PCKG_TX); + BOOST_CHECK(submit_cpfp_deprio.m_state.IsInvalid()); + BOOST_CHECK_EQUAL(submit_cpfp_deprio.m_tx_results.find(tx_parent->GetWitnessHash())->second.m_state.GetResult(), + TxValidationResult::TX_MEMPOOL_POLICY); + BOOST_CHECK_EQUAL(submit_cpfp_deprio.m_tx_results.find(tx_child->GetWitnessHash())->second.m_state.GetResult(), + TxValidationResult::TX_MISSING_INPUTS); + BOOST_CHECK(submit_cpfp_deprio.m_tx_results.find(tx_parent->GetWitnessHash())->second.m_state.GetRejectReason() == "min relay fee not met"); BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); const CFeeRate expected_feerate(0, GetVirtualTransactionSize(*tx_parent) + GetVirtualTransactionSize(*tx_child)); } @@ -808,8 +811,8 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup) BOOST_CHECK_MESSAGE(submit_rich_parent.m_state.IsInvalid(), "Package validation unexpectedly succeeded"); // The child would have been validated on its own and failed, then submitted as a "package" of 1. - BOOST_CHECK_EQUAL(submit_rich_parent.m_state.GetResult(), PackageValidationResult::PCKG_POLICY); - BOOST_CHECK_EQUAL(submit_rich_parent.m_state.GetRejectReason(), "package-fee-too-low"); + BOOST_CHECK_EQUAL(submit_rich_parent.m_state.GetResult(), PackageValidationResult::PCKG_TX); + BOOST_CHECK_EQUAL(submit_rich_parent.m_state.GetRejectReason(), "transaction failed"); auto it_parent = submit_rich_parent.m_tx_results.find(tx_parent_rich->GetWitnessHash()); BOOST_CHECK(it_parent != submit_rich_parent.m_tx_results.end()); @@ -818,6 +821,11 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup) BOOST_CHECK_MESSAGE(it_parent->second.m_base_fees.value() == high_parent_fee, strprintf("rich parent: expected fee %s, got %s", high_parent_fee, it_parent->second.m_base_fees.value())); BOOST_CHECK(it_parent->second.m_effective_feerate == CFeeRate(high_parent_fee, GetVirtualTransactionSize(*tx_parent_rich))); + auto it_child = submit_rich_parent.m_tx_results.find(tx_child_poor->GetWitnessHash()); + BOOST_CHECK(it_child != submit_rich_parent.m_tx_results.end()); + BOOST_CHECK_EQUAL(it_child->second.m_result_type, MempoolAcceptResult::ResultType::INVALID); + BOOST_CHECK_EQUAL(it_child->second.m_state.GetResult(), TxValidationResult::TX_MEMPOOL_POLICY); + BOOST_CHECK(it_child->second.m_state.GetRejectReason() == "min relay fee not met"); BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_parent_rich->GetHash()))); diff --git a/src/validation.cpp b/src/validation.cpp index e82fead89e..0fa7cbbabd 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -844,9 +844,18 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) return state.Invalid(TxValidationResult::TX_NOT_STANDARD, "bad-txns-too-many-sigops", strprintf("%d", nSigOpsCost)); - // No individual transactions are allowed below the min relay feerate and mempool min feerate except from - // disconnected blocks and transactions in a package. Package transactions will be checked using - // package feerate later. + // No individual transactions are allowed below the min relay feerate except from disconnected blocks. + // This requirement, unlike CheckFeeRate, cannot be bypassed using m_package_feerates because, + // while a tx could be package CPFP'd when entering the mempool, we do not have a DoS-resistant + // method of ensuring the tx remains bumped. For example, the fee-bumping child could disappear + // due to a replacement. + if (!bypass_limits && ws.m_modified_fees < m_pool.m_min_relay_feerate.GetFee(ws.m_vsize)) { + return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "min relay fee not met", + strprintf("%d < %d", ws.m_modified_fees, m_pool.m_min_relay_feerate.GetFee(ws.m_vsize))); + } + // 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; ws.m_iters_conflicting = m_pool.GetIterSet(ws.m_conflicts); -- cgit v1.2.3 From b51ebccc28e66c1822ab22d2d178be55c6618196 Mon Sep 17 00:00:00 2001 From: glozow Date: Tue, 24 Jan 2023 15:31:28 +0000 Subject: [validation] set PackageValidationState when mempool full --- src/validation.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/validation.cpp b/src/validation.cpp index 0fa7cbbabd..978f0d07c8 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1199,6 +1199,7 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector& } else { all_submitted = false; ws.m_state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "mempool full"); + package_state.Invalid(PackageValidationResult::PCKG_TX, "transaction failed"); results.emplace(ws.m_ptx->GetWitnessHash(), MempoolAcceptResult::Failure(ws.m_state)); } } -- cgit v1.2.3 From bf77fc9cb45209b9c560208c65abc94209cd7919 Mon Sep 17 00:00:00 2001 From: glozow Date: Mon, 17 Apr 2023 12:25:04 +0100 Subject: [test] mempool full in package accept --- test/functional/mempool_limit.py | 30 +++++++++++++++++++++++++++--- 1 file changed, 27 insertions(+), 3 deletions(-) diff --git a/test/functional/mempool_limit.py b/test/functional/mempool_limit.py index f8e86cafab..f3f4b42ad0 100755 --- a/test/functional/mempool_limit.py +++ b/test/functional/mempool_limit.py @@ -44,8 +44,8 @@ class MempoolLimitTest(BitcoinTestFramework): assert_equal(node.getmempoolinfo()['minrelaytxfee'], Decimal('0.00001000')) assert_equal(node.getmempoolinfo()['mempoolminfee'], Decimal('0.00001000')) - tx_batch_size = 25 - num_of_batches = 3 + tx_batch_size = 1 + num_of_batches = 75 # Generate UTXOs to flood the mempool # 1 to create a tx initially that will be evicted from the mempool later # 3 batches of multiple transactions with a fee rate much higher than the previous UTXO @@ -119,7 +119,31 @@ class MempoolLimitTest(BitcoinTestFramework): # The node will broadcast each transaction, still abiding by its peer's fee filter peer.wait_for_broadcast([tx["tx"].getwtxid() for tx in package_txns]) - self.generate(node, 1) + + self.log.info("Check a package that passes mempoolminfee but is evicted immediately after submission") + mempoolmin_feerate = node.getmempoolinfo()["mempoolminfee"] + current_mempool = node.getrawmempool(verbose=False) + worst_feerate_btcvb = Decimal("21000000") + for txid in current_mempool: + entry = node.getmempoolentry(txid) + worst_feerate_btcvb = min(worst_feerate_btcvb, entry["fees"]["descendant"] / entry["descendantsize"]) + # Needs to be large enough to trigger eviction + target_weight_each = 200000 + assert_greater_than(target_weight_each * 2, node.getmempoolinfo()["maxmempool"] - node.getmempoolinfo()["bytes"]) + # Should be a true CPFP: parent's feerate is just below mempool min feerate + parent_fee = (mempoolmin_feerate / 1000) * (target_weight_each // 4) - Decimal("0.00001") + # Parent + child is above mempool minimum feerate + child_fee = (worst_feerate_btcvb) * (target_weight_each // 4) - Decimal("0.00001") + # However, when eviction is triggered, these transactions should be at the bottom. + # This assertion assumes parent and child are the same size. + miniwallet.rescan_utxos() + tx_parent_just_below = miniwallet.create_self_transfer(fee=parent_fee, target_weight=target_weight_each) + tx_child_just_above = miniwallet.create_self_transfer(utxo_to_spend=tx_parent_just_below["new_utxo"], fee=child_fee, target_weight=target_weight_each) + # This package ranks below the lowest descendant package in the mempool + assert_greater_than(worst_feerate_btcvb, (parent_fee + child_fee) / (tx_parent_just_below["tx"].get_vsize() + tx_child_just_above["tx"].get_vsize())) + assert_greater_than(mempoolmin_feerate, (parent_fee) / (tx_parent_just_below["tx"].get_vsize())) + assert_greater_than((parent_fee + child_fee) / (tx_parent_just_below["tx"].get_vsize() + tx_child_just_above["tx"].get_vsize()), mempoolmin_feerate / 1000) + assert_raises_rpc_error(-26, "mempool full", node.submitpackage, [tx_parent_just_below["hex"], tx_child_just_above["hex"]]) self.log.info('Test passing a value below the minimum (5 MB) to -maxmempool throws an error') self.stop_node(0) -- cgit v1.2.3