From 43fd360e50eeab2820e030ddaa1f5b389eebe591 Mon Sep 17 00:00:00 2001 From: glozow Date: Mon, 20 Feb 2023 10:54:38 +0000 Subject: [PATCH 01/57] -- Part 0: Mempool Logic for Arbitrary Packages-- From f154c3487bc63b868175a6395256866856240c71 Mon Sep 17 00:00:00 2001 From: glozow Date: Tue, 17 Jan 2023 11:00:49 +0000 Subject: [PATCH 02/57] [test util] mock mempool minimum feerate --- src/test/util/setup_common.cpp | 26 ++++++++++++++++++++++++++ src/test/util/setup_common.h | 12 ++++++++++++ 2 files changed, 38 insertions(+) diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 58593c9d5bb9a..8ce63a30cbdf7 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -432,6 +432,32 @@ 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; + 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 948ebc097c2b5..7c39718049242 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 }; From 5d09d97328fe56425b53d858b895a577658893a8 Mon Sep 17 00:00:00 2001 From: glozow Date: Tue, 17 Jan 2023 15:37:25 +0000 Subject: [PATCH 03/57] [test] package cpfp bumps parents =minrelaytxfee --- src/test/txpackage_tests.cpp | 49 ++++++++++++++++++-------------- test/functional/mempool_limit.py | 49 ++++++++++++++++++++++++++++++-- test/functional/rpc_packages.py | 40 -------------------------- 3 files changed, 74 insertions(+), 64 deletions(-) diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp index 024526497c4a5..55a01790baca0 100644 --- a/src/test/txpackage_tests.cpp +++ b/src/test/txpackage_tests.cpp @@ -373,6 +373,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 +561,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 - 200), /*submit=*/false); CTransactionRef ptx_parent3 = MakeTransactionRef(mtx_parent3); package_mixed.push_back(ptx_parent3); + BOOST_CHECK(m_node.mempool->GetMinFee().GetFee(GetVirtualTransactionSize(*ptx_parent3)) > 200); + BOOST_CHECK(m_node.mempool->m_min_relay_feerate.GetFee(GetVirtualTransactionSize(*ptx_parent3)) <= 200); // child spends parent1, parent2, and parent3 CKey mixed_grandchild_key; @@ -627,6 +630,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 +640,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 - 200}; const CAmount child_value{parent_value - COIN}; Package package_cpfp; @@ -657,9 +661,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 +679,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 +693,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 +713,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 +745,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 +757,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 + 800, 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() == 200); 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() == 600); 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 d38a37f952ab2..f8e86cafabcfc 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 @@ def run_test(self): # 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 @@ def run_test(self): 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 6cb9760b3df21..ae1a498e28abd 100755 --- a/test/functional/rpc_packages.py +++ b/test/functional/rpc_packages.py @@ -20,7 +20,6 @@ assert_raises_rpc_error, ) from test_framework.wallet import ( - COIN, DEFAULT_FEE, MiniWallet, ) @@ -325,42 +324,6 @@ def test_submit_child_with_parents(self, num_parents, partial_submit): 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 @@ def test_submitpackage(self): 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)] From 64fc0dae1716c9368926fdeed1979decd084f226 Mon Sep 17 00:00:00 2001 From: glozow Date: Tue, 17 Jan 2023 11:01:13 +0000 Subject: [PATCH 04/57] [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 274854ddf93de..2a5758318a9b6 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 55a01790baca0..bf09ff0b4a436 100644 --- a/src/test/txpackage_tests.cpp +++ b/src/test/txpackage_tests.cpp @@ -668,10 +668,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)); } @@ -805,8 +808,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()); @@ -815,6 +818,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 1fd8f0e326877..eb2b1f2da770c 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -845,9 +845,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); From 1f40f8cc7e4ac167b6295ad29363ca9259482bfd Mon Sep 17 00:00:00 2001 From: glozow Date: Tue, 24 Jan 2023 15:31:28 +0000 Subject: [PATCH 05/57] [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 eb2b1f2da770c..713ba0da3c2e8 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1200,6 +1200,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)); } } From 3639ad00923ee28eac8af735fb5d19465063b3bb Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 15 Dec 2022 18:00:04 +0000 Subject: [PATCH 06/57] [validation] call AcceptSingleTransaction when only 1 package tx left Avoid calling PackageMempoolChecks() when there is only 1 transaction. Note to reviewers: there is a slight change in the error type returned, as shown in the txpackage_tests change. When a transaction is the last one left in the package and its fee is too low, this returns a PCKG_TX instead of PCKG_POLICY. This interface is clearer; "package-fee-too-low" for 1 transaction would be a bit misleading. --- src/test/txpackage_tests.cpp | 6 ++++-- src/validation.cpp | 19 +++++++++++++++++-- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp index bf09ff0b4a436..f6b08be822ec2 100644 --- a/src/test/txpackage_tests.cpp +++ b/src/test/txpackage_tests.cpp @@ -807,18 +807,20 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup) expected_pool_size += 1; 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. + // The child would have been validated on its own and failed. 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()); + auto it_child = submit_rich_parent.m_tx_results.find(tx_child_poor->GetWitnessHash()); BOOST_CHECK(it_parent != submit_rich_parent.m_tx_results.end()); + BOOST_CHECK(it_child != submit_rich_parent.m_tx_results.end()); BOOST_CHECK(it_parent->second.m_result_type == MempoolAcceptResult::ResultType::VALID); + BOOST_CHECK(it_child->second.m_result_type == MempoolAcceptResult::ResultType::INVALID); BOOST_CHECK(it_parent->second.m_state.GetRejectReason() == ""); 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); diff --git a/src/validation.cpp b/src/validation.cpp index 713ba0da3c2e8..4d01fb98b1af3 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1396,6 +1396,21 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, // the new transactions. This ensures we don't double-count transaction counts and sizes when // checking ancestor/descendant limits, or double-count transaction fees for fee-related policy. ATMPArgs single_args = ATMPArgs::SingleInPackageAccept(args); + const auto AcceptPackageWrappingSingle = [&](const std::vector& subpackage) + EXCLUSIVE_LOCKS_REQUIRED(::cs_main, m_pool.cs) { + AssertLockHeld(::cs_main); + AssertLockHeld(m_pool.cs); + if (subpackage.size() > 1) { + return AcceptMultipleTransactions(subpackage, args); + } + const auto& tx = subpackage.front(); + const auto single_res = AcceptSingleTransaction(tx, single_args); + PackageValidationState package_state_wrapped; + if (single_res.m_result_type != MempoolAcceptResult::ResultType::VALID) { + package_state_wrapped.Invalid(PackageValidationResult::PCKG_TX, "transaction failed"); + } + return PackageMempoolAcceptResult(package_state_wrapped, {{tx->GetWitnessHash(), single_res}}); + }; // Results from individual validation. "Nonfinal" because if a transaction fails by itself but // succeeds later (i.e. when evaluated with a fee-bumping child), the result changes (though not // reflected in this map). If a transaction fails more than once, we want to return the first @@ -1467,7 +1482,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, } // Validate the (deduplicated) transactions as a package. Note that submission_result has its // own PackageValidationState; package_state_quit_early is unused past this point. - auto submission_result = AcceptMultipleTransactions(txns_package_eval, args); + auto submission_result = AcceptPackageWrappingSingle(txns_package_eval); // Include already-in-mempool transaction results in the final result. for (const auto& [wtxid, mempoolaccept_res] : results_final) { Assume(submission_result.m_tx_results.emplace(wtxid, mempoolaccept_res).second); @@ -1475,7 +1490,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, } if (submission_result.m_state.GetResult() == PackageValidationResult::PCKG_TX) { // Package validation failed because one or more transactions failed. Provide a result for - // each transaction; if AcceptMultipleTransactions() didn't return a result for a tx, + // each transaction; if a transaction doesn't have an entry in submission_result, // include the previous individual failure reason. submission_result.m_tx_results.insert(individual_results_nonfinal.cbegin(), individual_results_nonfinal.cend()); From a781945518b8c4b423393c72242499b84668272c Mon Sep 17 00:00:00 2001 From: glozow Date: Tue, 17 Jan 2023 13:43:27 +0000 Subject: [PATCH 07/57] [mempool] evict everything below min relay fee in TrimToSize() At this point it's not expected that there are any such transactions, except from reorgs. --- src/txmempool.cpp | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 032dfee3ea64e..d34b41bf0df08 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -1057,17 +1057,23 @@ void CTxMemPool::TrimToSize(size_t sizelimit, std::vector* pvNoSpends unsigned nTxnRemoved = 0; CFeeRate maxFeeRateRemoved(0); - while (!mapTx.empty() && DynamicMemoryUsage() > sizelimit) { + while (!mapTx.empty()) { indexed_transaction_set::index::type::iterator it = mapTx.get().begin(); + CFeeRate removed(it->GetModFeesWithDescendants(), it->GetSizeWithDescendants()); + // Skim away everything paying effectively zero fees, regardless of mempool size. + // Above that feerate, just trim until memory is within limits. + if (removed >= m_min_relay_feerate && DynamicMemoryUsage() <= sizelimit) break; + // We set the new mempool min fee to the feerate of the removed set, plus the // "minimum reasonable fee rate" (ie some value under which we consider txn // to have 0 fee). This way, we don't allow txn to enter mempool with feerate // equal to txn which were removed with no block in between. - CFeeRate removed(it->GetModFeesWithDescendants(), it->GetSizeWithDescendants()); - removed += m_incremental_relay_feerate; - trackPackageRemoved(removed); - maxFeeRateRemoved = std::max(maxFeeRateRemoved, removed); + if (removed >= m_min_relay_feerate) { + removed += m_incremental_relay_feerate; + trackPackageRemoved(removed); + maxFeeRateRemoved = std::max(maxFeeRateRemoved, removed); + } setEntries stage; CalculateDescendants(mapTx.project<0>(it), stage); From daf6b810e1504bb8c6a2bb1e6c07e961985accfa Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 19 Jan 2023 10:33:54 +0000 Subject: [PATCH 08/57] [test framework] return txhex from create_lots_of_big_transactions --- test/functional/mining_prioritisetransaction.py | 2 +- test/functional/test_framework/util.py | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/test/functional/mining_prioritisetransaction.py b/test/functional/mining_prioritisetransaction.py index a4481c15a0d0b..54e0f281b0fbc 100755 --- a/test/functional/mining_prioritisetransaction.py +++ b/test/functional/mining_prioritisetransaction.py @@ -152,7 +152,7 @@ def run_test(self): (i+1) * base_fee, end_range - start_range, self.txouts, - utxos[start_range:end_range]) + utxos[start_range:end_range])[0] # Make sure that the size of each group of transactions exceeds # MAX_BLOCK_WEIGHT // 4 -- otherwise the test needs to be revised to diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py index a1b90860f67bc..cf61908baff6e 100644 --- a/test/functional/test_framework/util.py +++ b/test/functional/test_framework/util.py @@ -505,6 +505,7 @@ def gen_return_txouts(): # transaction to make it large. See gen_return_txouts() above. def create_lots_of_big_transactions(mini_wallet, node, fee, tx_batch_size, txouts, utxos=None): txids = [] + txhex = [] use_internal_utxos = utxos is None for _ in range(tx_batch_size): tx = mini_wallet.create_self_transfer( @@ -512,10 +513,11 @@ def create_lots_of_big_transactions(mini_wallet, node, fee, tx_batch_size, txout fee=fee, )["tx"] tx.vout.extend(txouts) + txhex.append(tx.serialize().hex()) res = node.testmempoolaccept([tx.serialize().hex()])[0] assert_equal(res['fees']['base'], fee) txids.append(node.sendrawtransaction(tx.serialize().hex())) - return txids + return txids, txhex def mine_large_block(test_framework, mini_wallet, node): From 569c284dee2482fb4ab319a45471282bdd2dcfae Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 19 Jan 2023 15:12:26 +0000 Subject: [PATCH 09/57] [test] raise wallet_abandonconflict -minrelaytxfee settings The intention of the test is to set a high -minrelaytxfee such that these transactions are rejected in LoadMempool() and in CWallet::ResumbitWalletTransactions(). However, while the parent transactions are below minrelaytxfee, they each have descendants high enough feerate to bump them past the minrelaytxfee (observe that the `assert_greater_than` checks fail if given the original amount of 0.0001). These transactions will be kept after the mempool persists packages, which is an improvement, but will cause the original test to fail. --- test/functional/wallet_abandonconflict.py | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/test/functional/wallet_abandonconflict.py b/test/functional/wallet_abandonconflict.py index 934f44588df44..60b566c162cb5 100755 --- a/test/functional/wallet_abandonconflict.py +++ b/test/functional/wallet_abandonconflict.py @@ -16,6 +16,7 @@ from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_equal, + assert_greater_than, assert_raises_rpc_error, ) @@ -100,10 +101,15 @@ def run_test(self): newbalance = alice.getbalance() assert_equal(newbalance, balance - Decimal("30") + signed3_change) balance = newbalance + for txid in self.nodes[0].getrawmempool(): + entry = self.nodes[0].getmempoolentry(txid) + # Multiply by 1000 because minrelaytxfee is specified in BTC per KvB. + assert_greater_than(Decimal("0.006"), entry["fees"]["modified"] / entry["vsize"] * 1000) + assert_greater_than(Decimal("0.006"), entry["fees"]["descendant"] / entry["descendantsize"] * 1000) # Restart the node with a higher min relay fee so the parent tx is no longer in mempool # TODO: redo with eviction - self.restart_node(0, extra_args=["-minrelaytxfee=0.0001"]) + self.restart_node(0, extra_args=["-minrelaytxfee=0.006"]) alice = self.nodes[0].get_wallet_rpc(self.default_wallet_name) assert self.nodes[0].getmempoolinfo()['loaded'] @@ -160,8 +166,14 @@ def run_test(self): assert_equal(newbalance, balance - Decimal("10") - Decimal("14.99998") + Decimal("24.9996")) balance = newbalance + for txid in self.nodes[0].getrawmempool(): + entry = self.nodes[0].getmempoolentry(txid) + # Multiply by 1000 because minrelaytxfee is specified in BTC per KvB. + assert_greater_than(Decimal("0.006"), entry["fees"]["modified"] / entry["vsize"] * 1000) + assert_greater_than(Decimal("0.006"), entry["fees"]["descendant"] / entry["descendantsize"] * 1000) + # Remove using high relay fee again - self.restart_node(0, extra_args=["-minrelaytxfee=0.0001"]) + self.restart_node(0, extra_args=["-minrelaytxfee=0.006"]) alice = self.nodes[0].get_wallet_rpc(self.default_wallet_name) assert self.nodes[0].getmempoolinfo()['loaded'] assert_equal(len(self.nodes[0].getrawmempool()), 0) From faf8b239d424a8150dd0158475bd7fb3f7c51f66 Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 19 Jan 2023 10:34:18 +0000 Subject: [PATCH 10/57] [mempool] persist packages across restart Hold pool.cs the entire time otherwise wallet resubmissions may call TrimtoSize() in between loading transactions from disk. --- src/kernel/mempool_persist.cpp | 16 +++++++-- test/functional/mempool_limit.py | 59 +++++++++++++++++++++++++++----- 2 files changed, 65 insertions(+), 10 deletions(-) diff --git a/src/kernel/mempool_persist.cpp b/src/kernel/mempool_persist.cpp index 71f3aac3664ff..95592e207bf62 100644 --- a/src/kernel/mempool_persist.cpp +++ b/src/kernel/mempool_persist.cpp @@ -63,6 +63,7 @@ bool LoadMempool(CTxMemPool& pool, const fs::path& load_path, Chainstate& active } uint64_t num; file >> num; + LOCK2(cs_main, pool.cs); while (num) { --num; CTransactionRef tx; @@ -77,8 +78,12 @@ bool LoadMempool(CTxMemPool& pool, const fs::path& load_path, Chainstate& active pool.PrioritiseTransaction(tx->GetHash(), amountdelta); } if (nTime > TicksSinceEpoch(now - pool.m_expiry)) { - LOCK(cs_main); - const auto& accepted = AcceptToMemoryPool(active_chainstate, tx, nTime, /*bypass_limits=*/false, /*test_accept=*/false); + // Use bypass_limits=true to skip feerate checks, and call TrimToSize() at the very + // end. This means the mempool may temporarily exceed its maximum capacity. However, + // this means fee-bumped transactions are persisted, and the resulting mempool + // minimum feerate is not dependent on the order in which transactions are loaded + // from disk. + const auto& accepted = AcceptToMemoryPool(active_chainstate, tx, nTime, /*bypass_limits=*/true, /*test_accept=*/false); if (accepted.m_result_type == MempoolAcceptResult::ResultType::VALID) { ++count; } else { @@ -104,6 +109,13 @@ bool LoadMempool(CTxMemPool& pool, const fs::path& load_path, Chainstate& active for (const auto& i : mapDeltas) { pool.PrioritiseTransaction(i.first, i.second); } + const auto size_before_trim{pool.size()}; + // Ensure the maximum memory limits are ultimately enforced and any transactions below + // minimum feerates are evicted, since bypass_limits was set to true during ATMP calls. + pool.TrimToSize(pool.m_max_size_bytes); + const auto num_evicted{size_before_trim - pool.size()}; + count -= num_evicted; + failed += num_evicted; std::set unbroadcast_txids; file >> unbroadcast_txids; diff --git a/test/functional/mempool_limit.py b/test/functional/mempool_limit.py index f8e86cafabcfc..71121904f08aa 100755 --- a/test/functional/mempool_limit.py +++ b/test/functional/mempool_limit.py @@ -13,6 +13,7 @@ assert_equal, assert_fee_amount, assert_greater_than, + assert_greater_than_or_equal, assert_raises_rpc_error, create_lots_of_big_transactions, gen_return_txouts, @@ -27,10 +28,13 @@ class MempoolLimitTest(BitcoinTestFramework): def set_test_params(self): self.setup_clean_chain = True - self.num_nodes = 1 + self.num_nodes = 2 self.extra_args = [[ "-datacarriersize=100000", "-maxmempool=5", + ], + [ + "-datacarriersize=100000", ]] self.supports_cli = False @@ -39,25 +43,31 @@ def run_test(self): node = self.nodes[0] miniwallet = MiniWallet(node) relayfee = node.getnetworkinfo()['relayfee'] + all_transactions = [] self.log.info('Check that mempoolminfee is minrelaytxfee') assert_equal(node.getmempoolinfo()['minrelaytxfee'], Decimal('0.00001000')) assert_equal(node.getmempoolinfo()['mempoolminfee'], Decimal('0.00001000')) - tx_batch_size = 25 - num_of_batches = 3 + # Batch size 1 is required to ensure every tx has a different feerate, which means the + # selection of transaction(s) for eviction is identical across nodes. + 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 + # 75 transactions, each with a fee rate much higher than the previous one # And 1 more to verify that this tx does not get added to the mempool with a fee rate less than the mempoolminfee # 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) + self.disconnect_nodes(0, 1) self.log.info('Create a mempool tx that will be evicted') - tx_to_be_evicted_id = miniwallet.send_self_transfer(from_node=node, fee_rate=relayfee)["txid"] + tx_to_be_evicted = miniwallet.send_self_transfer(from_node=node, fee_rate=relayfee) + tx_to_be_evicted_id = tx_to_be_evicted["txid"] + all_transactions.append(tx_to_be_evicted["hex"]) # Increase the tx fee rate to give the subsequent transactions a higher priority in the mempool # The tx has an approx. vsize of 65k, i.e. multiplying the previous fee rate (in sats/kvB) @@ -67,7 +77,7 @@ def run_test(self): self.log.info("Fill up the mempool with txs with higher fee rate") for batch_of_txid in range(num_of_batches): fee = (batch_of_txid + 1) * base_fee - create_lots_of_big_transactions(miniwallet, node, fee, tx_batch_size, txouts) + all_transactions.extend(create_lots_of_big_transactions(miniwallet, node, fee, tx_batch_size, txouts)[1]) self.log.info('The tx should be evicted by now') # The number of transactions created should be greater than the ones present in the mempool @@ -81,7 +91,9 @@ def run_test(self): # Deliberately try to create a tx with a fee less than the minimum mempool fee to assert that it does not get added to the mempool 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) + tx_below_mempoolmin = miniwallet.create_self_transfer(fee_rate=relayfee) + assert_raises_rpc_error(-26, "mempool min fee not met", node.sendrawtransaction, tx_below_mempoolmin["hex"]) + all_transactions.append(tx_below_mempoolmin["hex"]) self.log.info("Check that submitpackage allows cpfp of a parent below mempool min feerate") node = self.nodes[0] @@ -116,10 +128,41 @@ def run_test(self): 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"]) + assert_greater_than_or_equal(poor_parent_result["fees"]["effective-feerate"], Decimal("0.0002")) # 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) + kept_txids = set(node.getrawmempool()) + + self.log.info('Test restarting with smaller maxmempool persists the correct transactions.') + # All transactions would make it in to a default size mempool: + self.nodes[1].prioritisetransaction(tx_rich["txid"], 0, int(DEFAULT_FEE * COIN)) + for txhex in all_transactions: + self.nodes[1].sendrawtransaction(txhex) + self.nodes[1].submitpackage([tx["hex"] for tx in package_txns]) + node1_info = self.nodes[1].getmempoolinfo() + assert_equal(node1_info["mempoolminfee"], node1_info["minrelaytxfee"]) + self.stop_node(1) + # Upon restart, the mempool min fee will rise above 1sat/vB due to the limited capacity. + self.restart_node(1, extra_args=["-maxmempool=5","-datacarriersize=100000"]) + node1_info_after_restart = self.nodes[1].getmempoolinfo() + assert_greater_than(node1_info_after_restart["mempoolminfee"], node1_info_after_restart["minrelaytxfee"]) + assert_equal(set(self.nodes[1].getrawmempool()), kept_txids) + + txids_above_20 = set() + for txid in kept_txids: + entry = self.nodes[1].getmempoolentry(txid) + descendant_feerate = Decimal(entry["fees"]["descendant"] / entry["descendantsize"]) + assert_greater_than_or_equal(descendant_feerate * 1000, node1_info_after_restart["mempoolminfee"]) + if (descendant_feerate * 1000 >= Decimal("0.0002")): + txids_above_20.add(txid) + assert all([tx["txid"] in txids_above_20 for tx in package_txns]) + + self.log.info('Test restarting with higher -minrelaytxfee persists the correct transactions.') + self.stop_node(1) + self.restart_node(1, extra_args=["-minrelaytxfee=0.0002","-datacarriersize=100000"]) + assert_equal(set(self.nodes[1].getrawmempool()), txids_above_20) + self.log.info('Test passing a value below the minimum (5 MB) to -maxmempool throws an error') self.stop_node(0) From f9b1993ae850fd4f1fa75dc5384e023d35dbe586 Mon Sep 17 00:00:00 2001 From: glozow Date: Fri, 16 Sep 2022 16:49:24 +0100 Subject: [PATCH 11/57] MOVEONLY: move package checks into helper functions --- src/policy/packages.cpp | 51 +++++++++++++++++++++++++---------------- src/policy/packages.h | 14 +++++++++++ 2 files changed, 45 insertions(+), 20 deletions(-) diff --git a/src/policy/packages.cpp b/src/policy/packages.cpp index 6e70a94088a2d..4a2befe2e8099 100644 --- a/src/policy/packages.cpp +++ b/src/policy/packages.cpp @@ -15,25 +15,8 @@ #include #include -bool CheckPackage(const Package& txns, PackageValidationState& state) +bool IsSorted(const Package& txns) { - const unsigned int package_count = txns.size(); - - if (package_count > MAX_PACKAGE_COUNT) { - return state.Invalid(PackageValidationResult::PCKG_POLICY, "package-too-many-transactions"); - } - - const int64_t total_size = std::accumulate(txns.cbegin(), txns.cend(), 0, - [](int64_t sum, const auto& tx) { return sum + GetVirtualTransactionSize(*tx); }); - // If the package only contains 1 tx, it's better to report the policy violation on individual tx size. - if (package_count > 1 && total_size > MAX_PACKAGE_SIZE * 1000) { - return state.Invalid(PackageValidationResult::PCKG_POLICY, "package-too-large"); - } - - // Require the package to be sorted in order of dependency, i.e. parents appear before children. - // An unsorted package will fail anyway on missing-inputs, but it's better to quit earlier and - // fail on something less ambiguous (missing-inputs could also be an orphan or trying to - // spend nonexistent coins). std::unordered_set later_txids; std::transform(txns.cbegin(), txns.cend(), std::inserter(later_txids, later_txids.end()), [](const auto& tx) { return tx->GetHash(); }); @@ -41,19 +24,23 @@ bool CheckPackage(const Package& txns, PackageValidationState& state) for (const auto& input : tx->vin) { if (later_txids.find(input.prevout.hash) != later_txids.end()) { // The parent is a subsequent transaction in the package. - return state.Invalid(PackageValidationResult::PCKG_POLICY, "package-not-sorted"); + return false; } } later_txids.erase(tx->GetHash()); } + return true; +} +bool IsConsistent(const Package& txns) +{ // Don't allow any conflicting transactions, i.e. spending the same inputs, in a package. std::unordered_set inputs_seen; for (const auto& tx : txns) { for (const auto& input : tx->vin) { if (inputs_seen.find(input.prevout) != inputs_seen.end()) { // This input is also present in another tx in the package. - return state.Invalid(PackageValidationResult::PCKG_POLICY, "conflict-in-package"); + return false; } } // Batch-add all the inputs for a tx at a time. If we added them 1 at a time, we could @@ -65,6 +52,30 @@ bool CheckPackage(const Package& txns, PackageValidationState& state) return true; } +bool CheckPackage(const Package& txns, PackageValidationState& state) +{ + const unsigned int package_count = txns.size(); + + if (package_count > MAX_PACKAGE_COUNT) { + return state.Invalid(PackageValidationResult::PCKG_POLICY, "package-too-many-transactions"); + } + + const int64_t total_size = std::accumulate(txns.cbegin(), txns.cend(), 0, + [](int64_t sum, const auto& tx) { return sum + GetVirtualTransactionSize(*tx); }); + // If the package only contains 1 tx, it's better to report the policy violation on individual tx size. + if (package_count > 1 && total_size > MAX_PACKAGE_SIZE * 1000) { + return state.Invalid(PackageValidationResult::PCKG_POLICY, "package-too-large"); + } + + // Require the package to be sorted in order of dependency, i.e. parents appear before children. + // An unsorted package will fail anyway on missing-inputs, but it's better to quit earlier and + // fail on something less ambiguous (missing-inputs could also be an orphan or trying to + // spend nonexistent coins). + if (!IsSorted(txns)) return state.Invalid(PackageValidationResult::PCKG_POLICY, "package-not-sorted"); + if (!IsConsistent(txns)) return state.Invalid(PackageValidationResult::PCKG_POLICY, "conflict-in-package"); + return true; +} + bool IsChildWithParents(const Package& package) { assert(std::all_of(package.cbegin(), package.cend(), [](const auto& tx){return tx != nullptr;})); diff --git a/src/policy/packages.h b/src/policy/packages.h index 0a0e7cf6bb85e..a773134c08833 100644 --- a/src/policy/packages.h +++ b/src/policy/packages.h @@ -45,6 +45,20 @@ using Package = std::vector; class PackageValidationState : public ValidationState {}; +/** If any direct dependencies exist between transactions (i.e. a child spending the output of a + * parent), checks that all parents appear somewhere in the list before their respective children. + * This function cannot detect indirect dependencies (e.g. a transaction's grandparent if its parent + * is not present). + * @returns true if sorted. False if any tx spends the output of a tx that appears later in txns. + */ +bool IsSorted(const Package& txns); + +/** Checks that none of the transactions conflict, i.e., spend the same prevout. Consequently also + * checks that there are no duplicate transactions. + * @returns true if there are no conflicts. False if any two transactions spend the same prevout. + * */ +bool IsConsistent(const Package& txns); + /** Context-free package policy checks: * 1. The number of transactions cannot exceed MAX_PACKAGE_COUNT. * 2. The total virtual size cannot exceed MAX_PACKAGE_SIZE. From 2d0d4c0e02c7b57e4a5d07f582f9b033199b0e46 Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 19 Jan 2023 17:31:58 +0000 Subject: [PATCH 12/57] scripted-diff: rename CheckPackage to IsPackageWellFormed -BEGIN VERIFY SCRIPT- sed -i 's/CheckPackage(/IsPackageWellFormed(/g' $(git grep -l CheckPackage) -END VERIFY SCRIPT- --- src/policy/packages.cpp | 2 +- src/policy/packages.h | 2 +- src/test/txpackage_tests.cpp | 14 +++++++------- src/validation.cpp | 4 ++-- 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/policy/packages.cpp b/src/policy/packages.cpp index 4a2befe2e8099..a8584024a8d84 100644 --- a/src/policy/packages.cpp +++ b/src/policy/packages.cpp @@ -52,7 +52,7 @@ bool IsConsistent(const Package& txns) return true; } -bool CheckPackage(const Package& txns, PackageValidationState& state) +bool IsPackageWellFormed(const Package& txns, PackageValidationState& state) { const unsigned int package_count = txns.size(); diff --git a/src/policy/packages.h b/src/policy/packages.h index a773134c08833..a6d0d58715fa3 100644 --- a/src/policy/packages.h +++ b/src/policy/packages.h @@ -65,7 +65,7 @@ bool IsConsistent(const Package& txns); * 3. If any dependencies exist between transactions, parents must appear before children. * 4. Transactions cannot conflict, i.e., spend the same inputs. */ -bool CheckPackage(const Package& txns, PackageValidationState& state); +bool IsPackageWellFormed(const Package& txns, PackageValidationState& state); /** Context-free check that a package is exactly one child and its parents; not all parents need to * be present, but the package must not contain any transactions that are not the child's parents. diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp index f6b08be822ec2..35cbf955a8c98 100644 --- a/src/test/txpackage_tests.cpp +++ b/src/test/txpackage_tests.cpp @@ -45,7 +45,7 @@ BOOST_FIXTURE_TEST_CASE(package_sanitization_tests, TestChain100Setup) package_too_many.emplace_back(create_placeholder_tx(1, 1)); } PackageValidationState state_too_many; - BOOST_CHECK(!CheckPackage(package_too_many, state_too_many)); + BOOST_CHECK(!IsPackageWellFormed(package_too_many, state_too_many)); BOOST_CHECK_EQUAL(state_too_many.GetResult(), PackageValidationResult::PCKG_POLICY); BOOST_CHECK_EQUAL(state_too_many.GetRejectReason(), "package-too-many-transactions"); @@ -60,7 +60,7 @@ BOOST_FIXTURE_TEST_CASE(package_sanitization_tests, TestChain100Setup) } BOOST_CHECK(package_too_large.size() <= MAX_PACKAGE_COUNT); PackageValidationState state_too_large; - BOOST_CHECK(!CheckPackage(package_too_large, state_too_large)); + BOOST_CHECK(!IsPackageWellFormed(package_too_large, state_too_large)); BOOST_CHECK_EQUAL(state_too_large.GetResult(), PackageValidationResult::PCKG_POLICY); BOOST_CHECK_EQUAL(state_too_large.GetRejectReason(), "package-too-large"); } @@ -144,8 +144,8 @@ BOOST_FIXTURE_TEST_CASE(noncontextual_package_tests, TestChain100Setup) CTransactionRef tx_child = MakeTransactionRef(mtx_child); PackageValidationState state; - BOOST_CHECK(CheckPackage({tx_parent, tx_child}, state)); - BOOST_CHECK(!CheckPackage({tx_child, tx_parent}, state)); + BOOST_CHECK(IsPackageWellFormed({tx_parent, tx_child}, state)); + BOOST_CHECK(!IsPackageWellFormed({tx_child, tx_parent}, state)); BOOST_CHECK_EQUAL(state.GetResult(), PackageValidationResult::PCKG_POLICY); BOOST_CHECK_EQUAL(state.GetRejectReason(), "package-not-sorted"); BOOST_CHECK(IsChildWithParents({tx_parent, tx_child})); @@ -172,7 +172,7 @@ BOOST_FIXTURE_TEST_CASE(noncontextual_package_tests, TestChain100Setup) package.push_back(MakeTransactionRef(child)); PackageValidationState state; - BOOST_CHECK(CheckPackage(package, state)); + BOOST_CHECK(IsPackageWellFormed(package, state)); BOOST_CHECK(IsChildWithParents(package)); package.erase(package.begin()); @@ -208,8 +208,8 @@ BOOST_FIXTURE_TEST_CASE(noncontextual_package_tests, TestChain100Setup) BOOST_CHECK(IsChildWithParents({tx_parent, tx_parent_also_child, tx_child})); // IsChildWithParents does not detect unsorted parents. BOOST_CHECK(IsChildWithParents({tx_parent_also_child, tx_parent, tx_child})); - BOOST_CHECK(CheckPackage({tx_parent, tx_parent_also_child, tx_child}, state)); - BOOST_CHECK(!CheckPackage({tx_parent_also_child, tx_parent, tx_child}, state)); + BOOST_CHECK(IsPackageWellFormed({tx_parent, tx_parent_also_child, tx_child}, state)); + BOOST_CHECK(!IsPackageWellFormed({tx_parent_also_child, tx_parent, tx_child}, state)); BOOST_CHECK_EQUAL(state.GetResult(), PackageValidationResult::PCKG_POLICY); BOOST_CHECK_EQUAL(state.GetRejectReason(), "package-not-sorted"); } diff --git a/src/validation.cpp b/src/validation.cpp index 4d01fb98b1af3..a70cd3850b42b 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1246,7 +1246,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: // These context-free package limits can be done before taking the mempool lock. PackageValidationState package_state; - if (!CheckPackage(txns, package_state)) return PackageMempoolAcceptResult(package_state, {}); + if (!IsPackageWellFormed(txns, package_state)) return PackageMempoolAcceptResult(package_state, {}); std::vector workspaces{}; workspaces.reserve(txns.size()); @@ -1339,7 +1339,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, // transactions and thus won't return any MempoolAcceptResults, just a package-wide error. // Context-free package checks. - if (!CheckPackage(package, package_state_quit_early)) return PackageMempoolAcceptResult(package_state_quit_early, {}); + if (!IsPackageWellFormed(package, package_state_quit_early)) return PackageMempoolAcceptResult(package_state_quit_early, {}); // All transactions in the package must be a parent of the last transaction. This is just an // opportunity for us to fail fast on a context-free check without taking the mempool lock. From 9bfb4a18ffc19b46c67ad253d790d8e506e15374 Mon Sep 17 00:00:00 2001 From: glozow Date: Fri, 16 Sep 2022 16:51:36 +0100 Subject: [PATCH 13/57] [packages] Packageifier for arbitrary transaction lists We cannot require that peers send topologically sorted lists, because we cannot check for this property without ensuring we have the same chain tip and ensuring we have the full ancestor set. Instead, add the ability to handle arbitrarily ordered transaction lists. --- src/policy/packages.cpp | 86 +++++++++++++++++++++- src/policy/packages.h | 40 +++++++++++ src/test/txpackage_tests.cpp | 136 +++++++++++++++++++++++++++++++++++ 3 files changed, 260 insertions(+), 2 deletions(-) diff --git a/src/policy/packages.cpp b/src/policy/packages.cpp index a8584024a8d84..ddb175b3670ed 100644 --- a/src/policy/packages.cpp +++ b/src/policy/packages.cpp @@ -5,7 +5,7 @@ #include #include #include -#include +#include #include #include @@ -13,7 +13,6 @@ #include #include #include -#include bool IsSorted(const Package& txns) { @@ -92,3 +91,86 @@ bool IsChildWithParents(const Package& package) return std::all_of(package.cbegin(), package.cend() - 1, [&input_txids](const auto& ptx) { return input_txids.count(ptx->GetHash()) > 0; }); } + +// Calculates curr_tx's in-package ancestor set. If the tx spends another tx in the package, calls +// visit() for that transaction first, since any transaction's ancestor set includes its parents' +// ancestor sets. Transaction dependency cycles are not possible without breaking sha256 and +// duplicate transactions were checked in the Packageifier() ctor, so this won't recurse infinitely. +// After this function returns, curr_tx is guaranteed to be in the ancestor_subsets map. +void Packageifier::visit(const CTransactionRef& curr_tx) +{ + const uint256& curr_txid = curr_tx->GetHash(); + if (ancestor_subsets.count(curr_txid) > 0) return; + std::set my_ancestors; + my_ancestors.insert(curr_txid); + for (const auto& input : curr_tx->vin) { + auto parent_tx = txid_to_tx.find(input.prevout.hash); + if (parent_tx == txid_to_tx.end()) continue; + if (ancestor_subsets.count(parent_tx->first) == 0) { + visit(parent_tx->second); + } + auto parent_ancestor_set = ancestor_subsets.find(parent_tx->first); + my_ancestors.insert(parent_ancestor_set->second.cbegin(), parent_ancestor_set->second.cend()); + } + ancestor_subsets.insert(std::make_pair(curr_txid, my_ancestors)); +} + +Packageifier::Packageifier(const Package& txns_in) +{ + // Duplicate transactions are not allowed, as they will result in infinite visit() recusion. + Assume(IsConsistent(txns_in)); + // Populate txid_to_tx for quick lookup + std::transform(txns_in.cbegin(), txns_in.cend(), std::inserter(txid_to_tx, txid_to_tx.end()), + [](const auto& tx) { return std::make_pair(tx->GetHash(), tx); }); + // DFS-based algorithm to sort transactions by ancestor count and populate ancestor_subsets cache. + // Best case runtime is if the package is already sorted and no recursive calls happen. + // Exclusion from ancestor_subsets is equivalent to not yet being fully processed. + size_t i{0}; + while (ancestor_subsets.size() < txns_in.size() && i < txns_in.size()) { + const auto& tx = txns_in[i]; + if (ancestor_subsets.count(tx->GetHash()) == 0) visit(tx); + Assume(ancestor_subsets.count(tx->GetHash()) == 1); + ++i; + } + txns = txns_in; + // Sort by the number of in-package ancestors. + std::sort(txns.begin(), txns.end(), [&](const CTransactionRef& a, const CTransactionRef& b) -> bool { + auto a_ancestors = ancestor_subsets.find(a->GetHash()); + auto b_ancestors = ancestor_subsets.find(b->GetHash()); + return a_ancestors->second.size() < b_ancestors->second.size(); + }); + Assume(IsSorted(txns)); +} + +std::optional> Packageifier::GetAncestorSet(const CTransactionRef& tx) +{ + auto ancestor_set = ancestor_subsets.find(tx->GetHash()); + std::vector result; + for (const auto& txid : ancestor_set->second) { + if (banned_txns.find(txid) != banned_txns.end()) { + return std::nullopt; + } + } + result.reserve(ancestor_set->second.size()); + for (const auto& txid : ancestor_set->second) { + auto it = txid_to_tx.find(txid); + if (excluded_txns.find(txid) == excluded_txns.end()) { + result.push_back(it->second); + } + } + std::sort(result.begin(), result.end(), [&](const CTransactionRef& a, const CTransactionRef& b) -> bool { + auto a_ancestors = ancestor_subsets.find(a->GetHash()); + auto b_ancestors = ancestor_subsets.find(b->GetHash()); + return a_ancestors->second.size() < b_ancestors->second.size(); + }); + return result; +} + +void Packageifier::Exclude(const CTransactionRef& transaction) +{ + excluded_txns.insert(transaction->GetHash()); +} +void Packageifier::Ban(const CTransactionRef& transaction) +{ + banned_txns.insert(transaction->GetHash()); +} diff --git a/src/policy/packages.h b/src/policy/packages.h index a6d0d58715fa3..d3bd0cf696df1 100644 --- a/src/policy/packages.h +++ b/src/policy/packages.h @@ -9,8 +9,11 @@ #include #include #include +#include +#include #include +#include #include /** Default maximum number of transactions in a package. */ @@ -73,4 +76,41 @@ bool IsPackageWellFormed(const Package& txns, PackageValidationState& state); */ bool IsChildWithParents(const Package& package); +class Packageifier +{ + /** Transactions sorted topologically (see IsSorted()). */ + Package txns; + /** Map from txid to transaction for quick lookup. */ + std::map txid_to_tx; + /** Cache of the in-package ancestors for each transaction, indexed by txid. */ + std::map> ancestor_subsets; + /** Txids of transactions to exclude when returning ancestor subsets.*/ + std::unordered_set excluded_txns; + /** Txids of transactions that are banned. Return nullopt from GetAncestorSet() if it contains + * any of these transactions.*/ + std::unordered_set banned_txns; + + /** Helper function for recursively constructing ancestor caches in ctor. */ + void visit(const CTransactionRef&); +public: + /** Constructs ancestor package, sorting the transactions topologically and constructing the + * txid_to_tx and ancestor_subsets maps. It is ok if the input txns is not sorted. + * Expects: + * - No duplicate transactions. + * - No conflicts between transactions. + * - txns is of reasonable size (e.g. below MAX_PACKAGE_COUNT) to limit recursion depth + */ + Packageifier(const Package& txns); + /** Returns the transactions, in ascending order of number of in-package ancestors. */ + Package Txns() const { return txns; } + /** Get the ancestor subpackage for a tx, sorted so that ancestors appear before descendants. + * The list includes the tx. If this transaction depends on a Banned tx, returns std::nullopt. + * If one or more ancestors have been Excluded, they will not appear in the result. */ + std::optional> GetAncestorSet(const CTransactionRef& tx); + /** From now on, exclude this tx from any result in GetAncestorSet(). Does not affect Txns(). */ + void Exclude(const CTransactionRef& transaction); + /** Mark a transaction as "banned." From now on, if this transaction is present in the ancestor + * set, GetAncestorSet() should return std::nullopt for that tx. Does not affect Txns(). */ + void Ban(const CTransactionRef& transaction); +}; #endif // BITCOIN_POLICY_PACKAGES_H diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp index 35cbf955a8c98..f81c9807e2dca 100644 --- a/src/test/txpackage_tests.cpp +++ b/src/test/txpackage_tests.cpp @@ -36,6 +36,30 @@ inline CTransactionRef create_placeholder_tx(size_t num_inputs, size_t num_outpu return MakeTransactionRef(mtx); } +// Context-free check that a package only contains a tx (the last tx in the package) with its +// ancestors. Not all of the tx's ancestors need to be present. +bool IsAncestorPackage(const Package& package) +{ + if (!IsSorted(package)) return false; + if (!IsConsistent(package)) return false; + const auto& dependent = package.back(); + std::unordered_set dependency_txids; + for (auto it = package.rbegin(); it != package.rend(); ++it) { + const auto& tx = *it; + // Each transaction must be a dependency of the last transaction. + if (tx->GetWitnessHash() != dependent->GetWitnessHash() && + dependency_txids.count(tx->GetHash()) == 0) { + return false; + } + // Add each transaction's dependencies to allow transactions which are ancestors but not + // necessarily direct parents of the last transaction. + std::transform(tx->vin.cbegin(), tx->vin.cend(), + std::inserter(dependency_txids, dependency_txids.end()), + [](const auto& input) { return input.prevout.hash; }); + } + return true; +} + BOOST_FIXTURE_TEST_CASE(package_sanitization_tests, TestChain100Setup) { // Packages can't have more than 25 transactions. @@ -64,6 +88,117 @@ BOOST_FIXTURE_TEST_CASE(package_sanitization_tests, TestChain100Setup) BOOST_CHECK_EQUAL(state_too_large.GetResult(), PackageValidationResult::PCKG_POLICY); BOOST_CHECK_EQUAL(state_too_large.GetRejectReason(), "package-too-large"); } +BOOST_FIXTURE_TEST_CASE(packageifier_tests, TestChain100Setup) +{ + CKey placeholder_key; + placeholder_key.MakeNewKey(true); + CScript spk = GetScriptForDestination(PKHash(placeholder_key.GetPubKey())); + FastRandomContext det_rand{true}; + // Basic chain of 25 transactions + { + Package package; + CTransactionRef last_tx = m_coinbase_txns[0]; + CKey signing_key = coinbaseKey; + for (int i{0}; i < 24; ++i) { + auto tx = MakeTransactionRef(CreateValidMempoolTransaction(last_tx, 0, 0, signing_key, spk, CAmount((49-i) * COIN), false)); + package.emplace_back(tx); + last_tx = tx; + if (i == 0) signing_key = placeholder_key; + } + BOOST_CHECK(!IsChildWithParents(package)); + BOOST_CHECK(IsAncestorPackage(package)); + + Package package_copy = package; + Shuffle(package_copy.begin(), package_copy.end(), det_rand); + Packageifier packageified(package_copy); + BOOST_CHECK(IsAncestorPackage(packageified.Txns())); + for (auto i{0}; i < 24; ++i) { + BOOST_CHECK_EQUAL(packageified.GetAncestorSet(package[i])->size(), i + 1); + BOOST_CHECK(IsAncestorPackage(*packageified.GetAncestorSet(package[i]))); + } + for (auto i{0}; i < 10; ++i) packageified.Exclude(package[i]); + packageified.Ban(package[20]); + for (auto i{11}; i < 20; ++i) { + const auto& tx = package[i]; + BOOST_CHECK_EQUAL(packageified.GetAncestorSet(tx)->size(), i - 9); + BOOST_CHECK(IsAncestorPackage(*packageified.GetAncestorSet(tx))); + } + for (auto i{20}; i < 24; ++i) { + BOOST_CHECK(!packageified.GetAncestorSet(package[i])); + } + } + // 99 Parents and 1 Child + { + Package package; + CMutableTransaction child; + for (int i{0}; i < 99; ++i) { + auto parent = MakeTransactionRef(CreateValidMempoolTransaction(m_coinbase_txns[i + 1], + 0, 0, coinbaseKey, spk, CAmount(49 * COIN), false)); + package.emplace_back(parent); + child.vin.push_back(CTxIn(COutPoint(parent->GetHash(), 0))); + } + child.vout.push_back(CTxOut(4900 * COIN, spk)); + package.push_back(MakeTransactionRef(child)); + + Package package_copy(package); + Shuffle(package_copy.begin(), package_copy.end(), det_rand); + Packageifier packageified(package_copy); + for (auto i{0}; i < 99; ++i) { + BOOST_CHECK_EQUAL(packageified.GetAncestorSet(package[i])->size(), 1); + BOOST_CHECK(IsAncestorPackage(*packageified.GetAncestorSet(packageified.Txns()[i]))); + if (i < 50) packageified.Exclude(package[i]); + } + BOOST_CHECK_EQUAL(packageified.GetAncestorSet(package.back())->size(), 50); + BOOST_CHECK(IsAncestorPackage(*packageified.GetAncestorSet(package.back()))); + packageified.Ban(package[75]); + for (auto i{50}; i < 99; ++i) { + if (i == 75) { + BOOST_CHECK(!packageified.GetAncestorSet(package[i])); + } else { + BOOST_CHECK_EQUAL(packageified.GetAncestorSet(package[i])->size(), 1); + } + } + BOOST_CHECK(!packageified.GetAncestorSet(package.back())); + } + + // Heavily inter-connected set of 500 transactions + LOCK2(cs_main, m_node.mempool->cs); + auto transactions{PopulateMempool(det_rand, /*num_transactions=*/500, /*submit=*/true)}; + Shuffle(transactions.begin(), transactions.end(), det_rand); + Packageifier packageified{transactions}; + const auto sorted_transactions{packageified.Txns()}; + BOOST_CHECK(IsSorted(sorted_transactions)); + for (const auto& tx : sorted_transactions) { + const auto packageified_ancestors{packageified.GetAncestorSet(tx)}; + BOOST_CHECK(IsAncestorPackage(*packageified_ancestors)); + auto mempool_ancestors{m_node.mempool->CalculateMemPoolAncestors(*m_node.mempool->GetIter(tx->GetHash()).value(), + CTxMemPool::Limits::NoLimits(), /*fSearchForParents=*/false)}; + // Add 1 because CMPA doesn't include the tx itself in its ancestor set. + BOOST_CHECK_EQUAL(mempool_ancestors->size() + 1, packageified_ancestors->size()); + std::set packageified_ancestors_wtxids; + for (const auto& tx : packageified_ancestors.value()) packageified_ancestors_wtxids.insert(tx->GetWitnessHash()); + for (const auto& mempool_iter : *mempool_ancestors) { + BOOST_CHECK(packageified_ancestors_wtxids.count(mempool_iter->GetTx().GetWitnessHash()) > 0); + } + } + // Exclude the 200th transaction. All of its descendants should have 1 fewer tx in their ancestor sets. + const auto& tx_200{sorted_transactions[200]}; + CTxMemPool::setEntries descendants_200; + m_node.mempool->CalculateDescendants(m_node.mempool->GetIter(tx_200->GetHash()).value(), descendants_200); + packageified.Exclude(tx_200); + for (const auto& desc_iter : descendants_200) { + BOOST_CHECK_EQUAL(packageified.GetAncestorSet(m_node.mempool->info(GenTxid::Txid(desc_iter->GetTx().GetHash())).tx)->size(), + desc_iter->GetCountWithAncestors() - 1); + } + // Ban the 400th transaction. GetAncestorSet() for all of its descendants should return std::nullopt. + const auto& tx_400{sorted_transactions[400]}; + CTxMemPool::setEntries descendants_400; + m_node.mempool->CalculateDescendants(m_node.mempool->GetIter(tx_400->GetHash()).value(), descendants_400); + packageified.Ban(tx_400); + for (const auto& desc_iter : descendants_400) { + BOOST_CHECK(!packageified.GetAncestorSet(m_node.mempool->info(GenTxid::Txid(desc_iter->GetTx().GetHash())).tx)); + } +} BOOST_FIXTURE_TEST_CASE(package_validation_tests, TestChain100Setup) { @@ -208,6 +343,7 @@ BOOST_FIXTURE_TEST_CASE(noncontextual_package_tests, TestChain100Setup) BOOST_CHECK(IsChildWithParents({tx_parent, tx_parent_also_child, tx_child})); // IsChildWithParents does not detect unsorted parents. BOOST_CHECK(IsChildWithParents({tx_parent_also_child, tx_parent, tx_child})); + BOOST_CHECK(!IsSorted({tx_parent_also_child, tx_parent, tx_child})); BOOST_CHECK(IsPackageWellFormed({tx_parent, tx_parent_also_child, tx_child}, state)); BOOST_CHECK(!IsPackageWellFormed({tx_parent_also_child, tx_parent, tx_child}, state)); BOOST_CHECK_EQUAL(state.GetResult(), PackageValidationResult::PCKG_POLICY); From 87e43a5932ab2cc40517d3c51629f0ee7bd3daf6 Mon Sep 17 00:00:00 2001 From: glozow Date: Mon, 3 Oct 2022 10:56:00 +0100 Subject: [PATCH 14/57] [test util] multi-input multi-output CreateValidMempoolTransaction --- src/test/util/setup_common.cpp | 63 ++++++++++++++++++++++------------ src/test/util/setup_common.h | 19 +++++++++- 2 files changed, 59 insertions(+), 23 deletions(-) diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 8ce63a30cbdf7..8f3f70b356839 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -335,40 +335,41 @@ CBlock TestChain100Setup::CreateAndProcessBlock( } -CMutableTransaction TestChain100Setup::CreateValidMempoolTransaction(CTransactionRef input_transaction, - int input_vout, +CMutableTransaction TestChain100Setup::CreateValidMempoolTransaction(const std::vector& input_transactions, + const std::vector& inputs, int input_height, - CKey input_signing_key, - CScript output_destination, - CAmount output_amount, + const std::vector& input_signing_keys, + const std::vector& outputs, bool submit) { - // Transaction we will submit to the mempool CMutableTransaction mempool_txn; + mempool_txn.vin.reserve(inputs.size()); + mempool_txn.vout.reserve(outputs.size()); - // Create an input - COutPoint outpoint_to_spend(input_transaction->GetHash(), input_vout); - CTxIn input(outpoint_to_spend); - mempool_txn.vin.push_back(input); - - // Create an output - CTxOut output(output_amount, output_destination); - mempool_txn.vout.push_back(output); + for (const auto& outpoint : inputs) { + mempool_txn.vin.emplace_back(CTxIn{outpoint}); + } + mempool_txn.vout = outputs; - // Sign the transaction // - Add the signing key to a keystore FillableSigningProvider keystore; - keystore.AddKey(input_signing_key); + for (const auto& input_signing_key : input_signing_keys) { + keystore.AddKey(input_signing_key); + } // - Populate a CoinsViewCache with the unspent output CCoinsView coins_view; CCoinsViewCache coins_cache(&coins_view); - AddCoins(coins_cache, *input_transaction.get(), input_height); - // - Use GetCoin to properly populate utxo_to_spend, - Coin utxo_to_spend; - assert(coins_cache.GetCoin(outpoint_to_spend, utxo_to_spend)); - // - Then add it to a map to pass in to SignTransaction + for (const auto& input_transaction : input_transactions) { + AddCoins(coins_cache, *input_transaction.get(), input_height); + } + // Build Outpoint to Coin map for SignTransaction std::map input_coins; - input_coins.insert({outpoint_to_spend, utxo_to_spend}); + for (const auto& outpoint_to_spend : inputs) { + // - Use GetCoin to properly populate utxo_to_spend, + Coin utxo_to_spend; + assert(coins_cache.GetCoin(outpoint_to_spend, utxo_to_spend)); + input_coins.insert({outpoint_to_spend, utxo_to_spend}); + } // - Default signature hashing type int nHashType = SIGHASH_ALL; std::map input_errors; @@ -384,6 +385,24 @@ CMutableTransaction TestChain100Setup::CreateValidMempoolTransaction(CTransactio return mempool_txn; } +CMutableTransaction TestChain100Setup::CreateValidMempoolTransaction(CTransactionRef input_transaction, + uint32_t input_vout, + int input_height, + CKey input_signing_key, + CScript output_destination, + CAmount output_amount, + bool submit) +{ + COutPoint input{input_transaction->GetHash(), input_vout}; + CTxOut output{output_amount, output_destination}; + return CreateValidMempoolTransaction(/*input_transactions=*/{input_transaction}, + /*inputs=*/{input}, + /*input_height=*/input_height, + /*input_signing_keys=*/{input_signing_key}, + /*outputs=*/{output}, + /*submit=*/submit); +} + std::vector TestChain100Setup::PopulateMempool(FastRandomContext& det_rand, size_t num_transactions, bool submit) { std::vector mempool_transactions; diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h index 7c39718049242..baab1fa60bd99 100644 --- a/src/test/util/setup_common.h +++ b/src/test/util/setup_common.h @@ -154,6 +154,23 @@ struct TestChain100Setup : public TestingSetup { //! Mine a series of new blocks on the active chain. void mineBlocks(int num_blocks); + /** + * Create a transaction and submit to the mempool. + * + * @param input_transactions The transactions to spend + * @param input_height The height of the block that included the input transactions. + * @param inputs Outpoints with which to construct transaction vin. + * @param input_signing_keys The keys to spend the input transactions. + * @param outputs Transaction vout. + * @param submit Whether or not to submit to mempool + */ + CMutableTransaction CreateValidMempoolTransaction(const std::vector& input_transactions, + const std::vector& inputs, + int input_height, + const std::vector& input_signing_keys, + const std::vector& outputs, + bool submit = true); + /** * Create a transaction and submit to the mempool. * @@ -166,7 +183,7 @@ struct TestChain100Setup : public TestingSetup { * @param submit Whether or not to submit to mempool */ CMutableTransaction CreateValidMempoolTransaction(CTransactionRef input_transaction, - int input_vout, + uint32_t input_vout, int input_height, CKey input_signing_key, CScript output_destination, From 2109d36731efb78b2a5fb7a04178b2a8c2c83d20 Mon Sep 17 00:00:00 2001 From: glozow Date: Wed, 14 Dec 2022 13:38:32 +0000 Subject: [PATCH 15/57] [validation] pre-fill missing inputs for txns depending on invalid Packageifier calculates the in-package ancestors. We already know this tx will fail because it depends on something invalid (specifically for a non-policy reason). We also know that it is missing at least one input (the tx that did not and will not make it into our mempool). Just return missing inputs directly. Note to reviewers: slight behavior change. If this tx has multiple errors, there may be a difference in which one is returned. For example, if the tx has a dust output in addition to relying on the invalid tx, we will return TX_MISSING_INPUTS when we previously would have returned TX_NOT_STANDARD. This is simply because dust is checked earlier within PreChecks(). Add a test that we don't quit *too* early and reject transactions we should keep. --- src/test/txpackage_tests.cpp | 51 ++++++++++++++++++++++++++++++++++++ src/validation.cpp | 30 ++++++++++++++++----- 2 files changed, 74 insertions(+), 7 deletions(-) diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp index f81c9807e2dca..c4a498d21ae9f 100644 --- a/src/test/txpackage_tests.cpp +++ b/src/test/txpackage_tests.cpp @@ -353,6 +353,8 @@ BOOST_FIXTURE_TEST_CASE(noncontextual_package_tests, TestChain100Setup) BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup) { + // Make coinbases 0 and 1 mature/spendable. + mineBlocks(1); LOCK(cs_main); unsigned int expected_pool_size = m_node.mempool->size(); CKey parent_key; @@ -441,6 +443,55 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup) BOOST_CHECK_EQUAL(it_child->second.m_state.GetRejectReason(), "bad-txns-inputs-missingorspent"); } + // Check that validation isn't quitting *too* early. + // This package has a child with 2 parents. Parent1 has a consensus issue, while Parent2 is + // valid and has a high feerate (can be accepted by itself). Parent1 and the child would be + // rejected, and Parent2 should be accepted. Otherwise, anybody could censor a transaction like + // Parent2 by broadcasting it in a package similar to this one. + { + // premature coinbase spend (consensus error) + auto tx_parent1_bad{MakeTransactionRef(CreateValidMempoolTransaction(/*input_transaction=*/m_coinbase_txns[99], + /*input_vout=*/0, + /*input_height=*/102, + /*input_signing_key=*/coinbaseKey, + /*output_destination=*/parent_locking_script, + /*output_amount=*/ 24 * COIN, + /*submit=*/false))}; + auto tx_parent2_good{MakeTransactionRef(CreateValidMempoolTransaction(/*input_transaction=*/m_coinbase_txns[1], + /*input_vout=*/0, + /*input_height=*/102, + /*input_signing_key=*/coinbaseKey, + /*output_destination=*/parent_locking_script, + /*output_amount=*/ 24 * COIN, + /*submit=*/false))}; + auto tx_child_quit_early{MakeTransactionRef(CreateValidMempoolTransaction(/*input_transactions=*/{tx_parent1_bad, tx_parent2_good}, + /*inputs=*/{COutPoint{tx_parent1_bad->GetHash(), 0}, + COutPoint{tx_parent2_good->GetHash(), 0}}, + /*input_height=*/102, + /*input_signing_keys=*/{parent_key}, + /*outputs=*/{CTxOut{23*COIN, child_locking_script}}, + /*submit=*/false))}; + Package package_quit_too_early{tx_parent1_bad, tx_parent2_good, tx_child_quit_early}; + auto result_quit_too_early = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, + package_quit_too_early, /*test_accept=*/ false); + expected_pool_size += 1; + BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); + BOOST_CHECK(result_quit_too_early.m_state.IsInvalid()); + BOOST_CHECK_EQUAL(result_quit_too_early.m_state.GetResult(), PackageValidationResult::PCKG_TX); + BOOST_CHECK_EQUAL(result_quit_too_early.m_tx_results.size(), 3); + BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_parent2_good->GetHash()))); + BOOST_CHECK(!m_node.mempool->exists(GenTxid::Txid(tx_parent1_bad->GetHash()))); + BOOST_CHECK(!m_node.mempool->exists(GenTxid::Txid(tx_child_quit_early->GetHash()))); + auto it_parent1 = result_quit_too_early.m_tx_results.find(tx_parent1_bad->GetWitnessHash()); + auto it_parent2 = result_quit_too_early.m_tx_results.find(tx_parent2_good->GetWitnessHash()); + auto it_child = result_quit_too_early.m_tx_results.find(tx_child_quit_early->GetWitnessHash()); + BOOST_CHECK(it_parent1->second.m_state.IsInvalid()); + BOOST_CHECK_EQUAL(it_parent1->second.m_state.GetResult(), TxValidationResult::TX_PREMATURE_SPEND); + BOOST_CHECK_MESSAGE(it_parent2->second.m_state.IsValid(), "parent2 error: " << it_parent2->second.m_state.GetRejectReason()); + BOOST_CHECK(it_child->second.m_state.IsInvalid()); + BOOST_CHECK_EQUAL(it_child->second.m_state.GetResult(), TxValidationResult::TX_MISSING_INPUTS); + } + // Child with missing parent. mtx_child.vin.push_back(CTxIn(COutPoint(package_unrelated[0]->GetHash(), 0))); Package package_missing_parent; diff --git a/src/validation.cpp b/src/validation.cpp index a70cd3850b42b..ddccd02f9bcd6 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1411,6 +1411,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, } return PackageMempoolAcceptResult(package_state_wrapped, {{tx->GetWitnessHash(), single_res}}); }; + Packageifier packageified(package); // Results from individual validation. "Nonfinal" because if a transaction fails by itself but // succeeds later (i.e. when evaluated with a fee-bumping child), the result changes (though not // reflected in this map). If a transaction fails more than once, we want to return the first @@ -1418,7 +1419,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, std::map individual_results_nonfinal; bool quit_early{false}; std::vector txns_package_eval; - for (const auto& tx : package) { + for (const auto& tx : packageified.Txns()) { const auto& wtxid = tx->GetWitnessHash(); const auto& txid = tx->GetHash(); // There are 3 possibilities: already in mempool, same-txid-diff-wtxid already in mempool, @@ -1429,6 +1430,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, auto iter = m_pool.GetIter(txid); assert(iter != std::nullopt); results_final.emplace(wtxid, MempoolAcceptResult::MempoolTx(iter.value()->GetTxSize(), iter.value()->GetFee())); + packageified.Exclude(tx); } else if (m_pool.exists(GenTxid::Txid(txid))) { // Transaction with the same non-witness data but different witness (same txid, // different wtxid) already exists in the mempool. @@ -1441,8 +1443,23 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, assert(iter != std::nullopt); // Provide the wtxid of the mempool tx so that the caller can look it up in the mempool. results_final.emplace(wtxid, MempoolAcceptResult::MempoolTxDifferentWitness(iter.value()->GetTx().GetWitnessHash())); + packageified.Exclude(tx); } else { // Transaction does not already exist in the mempool. + const auto subpackage = packageified.GetAncestorSet(tx); + if (!subpackage) { + Assume(quit_early); + // This transaction depends on a tx we will definitely not accept (failed for a + // non-policy and non-missing-inputs reason). We already know that this transaction + // will be invalid for at least one reason, i.e. a missing input. To minimize the + // amount of repeated work, don't validate this tx. Just return missing inputs. + TxValidationState tx_state_quit_early; + tx_state_quit_early.Invalid(TxValidationResult::TX_MISSING_INPUTS, "bad-txns-inputs-missingorspent"); + individual_results_nonfinal.emplace(wtxid, MempoolAcceptResult::Failure(tx_state_quit_early)); + // Don't quit too early. Other transactions may not necessarily depend on the same + // parent, and should still be considered. + continue; + } // Try submitting the transaction on its own. const auto single_res = AcceptSingleTransaction(tx, single_args); if (single_res.m_result_type == MempoolAcceptResult::ResultType::VALID) { @@ -1450,20 +1467,18 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, // in package validation, because its fees should only be "used" once. assert(m_pool.exists(GenTxid::Wtxid(wtxid))); results_final.emplace(wtxid, single_res); + packageified.Exclude(tx); } else if (single_res.m_state.GetResult() != TxValidationResult::TX_MEMPOOL_POLICY && single_res.m_state.GetResult() != TxValidationResult::TX_MISSING_INPUTS) { // Package validation policy only differs from individual policy in its evaluation // of feerate. For example, if a transaction fails here due to violation of a // consensus rule, the result will not change when it is submitted as part of a - // package. To minimize the amount of repeated work, unless the transaction fails - // due to feerate or missing inputs (its parent is a previous transaction in the - // package that failed due to feerate), don't run package validation. Note that this - // decision might not make sense if different types of packages are allowed in the - // future. Continue individually validating the rest of the transactions, because - // some of them may still be valid. + // package. Tell the Packageifier that subsequent transactions depending on this one + // should be skipped. quit_early = true; package_state_quit_early.Invalid(PackageValidationResult::PCKG_TX, "transaction failed"); individual_results_nonfinal.emplace(wtxid, single_res); + packageified.Ban(tx); } else { individual_results_nonfinal.emplace(wtxid, single_res); txns_package_eval.push_back(tx); @@ -1473,6 +1488,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, // Quit early because package validation won't change the result or the entire package has // already been submitted. + // If txns_package_eval is empty, all transactions have already passed. if (quit_early || txns_package_eval.empty()) { for (const auto& [wtxid, mempoolaccept_res] : individual_results_nonfinal) { Assume(results_final.emplace(wtxid, mempoolaccept_res).second); From 9bcefd6f2da5c6f8be2100f8cfcfd407cedbb063 Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 15 Dec 2022 18:17:47 +0000 Subject: [PATCH 16/57] [validation] validate packages by submitting each tx's ancestor sub-packages This results in the incentive-compatible transactions ending up in our mempool. Prior to this commit, if parents within the package relied on each other, we could end up (1) accepting a low-feerate child or (2) rejecting high-feerate parents. Instead of validating each transaction *individually* in turn, validate each one with their in-package ancestor set. This means parents with inter-dependencies are validated correctly, while we continue using aggregate totals for package feerate. --- src/validation.cpp | 81 ++++++++++++++++++++++++++++++---------------- 1 file changed, 53 insertions(+), 28 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index ddccd02f9bcd6..8a92c783d5e92 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1319,6 +1319,14 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: } } + if (args.m_package_feerates) { + // The only valid fee bump between transactions is CPFP (or more generally, a tx should only + // be able to bump ancestors). The use of aggregate package feerate is assuming + // transactions would not be submitted as a package if they would have passed the feerate + // checks on their own. Check this assumption; the aggregate package feerate should not be + // higher than the feerate of the last transaction, which is the "child" or "sponsor." + Assume(CFeeRate(workspaces.back().m_modified_fees, workspaces.back().m_vsize) >= package_feerate); + } if (args.m_test_accept) return PackageMempoolAcceptResult(package_state, std::move(results)); if (!SubmitPackage(args, workspaces, package_state, results)) { @@ -1417,8 +1425,6 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, // reflected in this map). If a transaction fails more than once, we want to return the first // result, when it was considered on its own. So changes will only be from invalid -> valid. std::map individual_results_nonfinal; - bool quit_early{false}; - std::vector txns_package_eval; for (const auto& tx : packageified.Txns()) { const auto& wtxid = tx->GetWitnessHash(); const auto& txid = tx->GetHash(); @@ -1448,7 +1454,6 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, // Transaction does not already exist in the mempool. const auto subpackage = packageified.GetAncestorSet(tx); if (!subpackage) { - Assume(quit_early); // This transaction depends on a tx we will definitely not accept (failed for a // non-policy and non-missing-inputs reason). We already know that this transaction // will be invalid for at least one reason, i.e. a missing input. To minimize the @@ -1460,36 +1465,56 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, // parent, and should still be considered. continue; } - // Try submitting the transaction on its own. - const auto single_res = AcceptSingleTransaction(tx, single_args); - if (single_res.m_result_type == MempoolAcceptResult::ResultType::VALID) { - // The transaction succeeded on its own and is now in the mempool. Don't include it - // in package validation, because its fees should only be "used" once. - assert(m_pool.exists(GenTxid::Wtxid(wtxid))); - results_final.emplace(wtxid, single_res); - packageified.Exclude(tx); - } else if (single_res.m_state.GetResult() != TxValidationResult::TX_MEMPOOL_POLICY && - single_res.m_state.GetResult() != TxValidationResult::TX_MISSING_INPUTS) { - // Package validation policy only differs from individual policy in its evaluation - // of feerate. For example, if a transaction fails here due to violation of a - // consensus rule, the result will not change when it is submitted as part of a - // package. Tell the Packageifier that subsequent transactions depending on this one - // should be skipped. - quit_early = true; - package_state_quit_early.Invalid(PackageValidationResult::PCKG_TX, "transaction failed"); - individual_results_nonfinal.emplace(wtxid, single_res); - packageified.Ban(tx); - } else { + // This transaction does not already exist in the mempool and is not the child. + // Try submitting the transaction with its in-package ancestor set. + const auto subpackage_result = AcceptPackageWrappingSingle(subpackage.value()); + // Look for "final" answers: once a tx is successfully submitted, we can add its + // MempoolAcceptResult to the results map. Note that it's possible for transactions to + // have been submitted to the mempool even if subpackage_result.m_state.IsInvalid(). + for (const auto& subpackage_tx : subpackage.value()) { + const auto subpackage_wtxid{subpackage_tx->GetWitnessHash()}; + if (m_pool.exists(GenTxid::Wtxid(subpackage_wtxid))) { + const auto subpackage_it{subpackage_result.m_tx_results.find(subpackage_wtxid)}; + results_final.emplace(subpackage_wtxid, subpackage_it->second); + // Erase any previous invalid results for this transaction. For example, this + // could be a low-feerate tx that has just been bumped. + individual_results_nonfinal.erase(subpackage_wtxid); + packageified.Exclude(subpackage_tx); + } + } + // If m_state is valid, we already processed each tx in the loop above. + if (subpackage_result.m_state.IsValid()) continue; + + const auto single_res_it = subpackage_result.m_tx_results.find(wtxid); + if (single_res_it != subpackage_result.m_tx_results.end()) { + const auto single_res = single_res_it->second; + if (single_res.m_state.GetResult() != TxValidationResult::TX_MEMPOOL_POLICY && + single_res.m_state.GetResult() != TxValidationResult::TX_MISSING_INPUTS) { + // Package validation policy only differs from individual policy in its evaluation + // of feerate. For example, if a transaction fails here due to violation of a + // consensus rule, the result will not change when it is submitted as part of a + // package. Tell the Packageifier that subsequent transactions depending on this one + // should be skipped. + package_state_quit_early.Invalid(PackageValidationResult::PCKG_TX, "transaction failed"); + individual_results_nonfinal.emplace(wtxid, single_res); + packageified.Ban(tx); + } individual_results_nonfinal.emplace(wtxid, single_res); - txns_package_eval.push_back(tx); + } else if (subpackage->size() > 1) { + // This transaction didn't get a result because it depends on another transaction + // that failed (PCKG_POLICY or AcceptMultipleTransactions() quit early on a previous + // transaction). Its error is missing inputs. + TxValidationState tx_state_dependent; + tx_state_dependent.Invalid(TxValidationResult::TX_MISSING_INPUTS, "bad-txns-inputs-missingorspent"); + individual_results_nonfinal.emplace(wtxid, MempoolAcceptResult::Failure(tx_state_dependent)); } } } - // Quit early because package validation won't change the result or the entire package has - // already been submitted. + const auto txns_package_eval{packageified.GetAncestorSet(child)}; + // If txns_package_eval is std::nullopt, the last tx's result was pre-filled. // If txns_package_eval is empty, all transactions have already passed. - if (quit_early || txns_package_eval.empty()) { + if (!txns_package_eval || txns_package_eval->empty()) { for (const auto& [wtxid, mempoolaccept_res] : individual_results_nonfinal) { Assume(results_final.emplace(wtxid, mempoolaccept_res).second); Assume(mempoolaccept_res.m_result_type == MempoolAcceptResult::ResultType::INVALID); @@ -1498,7 +1523,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, } // Validate the (deduplicated) transactions as a package. Note that submission_result has its // own PackageValidationState; package_state_quit_early is unused past this point. - auto submission_result = AcceptPackageWrappingSingle(txns_package_eval); + auto submission_result = AcceptPackageWrappingSingle(txns_package_eval.value()); // Include already-in-mempool transaction results in the final result. for (const auto& [wtxid, mempoolaccept_res] : results_final) { Assume(submission_result.m_tx_results.emplace(wtxid, mempoolaccept_res).second); From 0c011bc62c3778451e669a4eca3ed5e95244b8a1 Mon Sep 17 00:00:00 2001 From: glozow Date: Fri, 17 Feb 2023 13:42:52 +0000 Subject: [PATCH 17/57] [policy] allow any ancestor package, not just child-with-unconfirmed-parents We can safely allow any ancestor package since Packageifier can handle things that are out of order. Remove the check that "all unconfirmed parents are present" because, even if a tx is missing inputs, the other transactions may be worth validating. --- src/policy/packages.cpp | 4 +- src/policy/packages.h | 2 +- src/test/txpackage_tests.cpp | 69 +++++++++++++-------------------- src/validation.cpp | 51 +++++------------------- test/functional/rpc_packages.py | 7 ---- 5 files changed, 38 insertions(+), 95 deletions(-) diff --git a/src/policy/packages.cpp b/src/policy/packages.cpp index ddb175b3670ed..d1009b8376d7c 100644 --- a/src/policy/packages.cpp +++ b/src/policy/packages.cpp @@ -51,7 +51,7 @@ bool IsConsistent(const Package& txns) return true; } -bool IsPackageWellFormed(const Package& txns, PackageValidationState& state) +bool IsPackageWellFormed(const Package& txns, PackageValidationState& state, bool require_sorted) { const unsigned int package_count = txns.size(); @@ -70,7 +70,7 @@ bool IsPackageWellFormed(const Package& txns, PackageValidationState& state) // An unsorted package will fail anyway on missing-inputs, but it's better to quit earlier and // fail on something less ambiguous (missing-inputs could also be an orphan or trying to // spend nonexistent coins). - if (!IsSorted(txns)) return state.Invalid(PackageValidationResult::PCKG_POLICY, "package-not-sorted"); + if (require_sorted && !IsSorted(txns)) return state.Invalid(PackageValidationResult::PCKG_POLICY, "package-not-sorted"); if (!IsConsistent(txns)) return state.Invalid(PackageValidationResult::PCKG_POLICY, "conflict-in-package"); return true; } diff --git a/src/policy/packages.h b/src/policy/packages.h index d3bd0cf696df1..3fc2c4e469871 100644 --- a/src/policy/packages.h +++ b/src/policy/packages.h @@ -68,7 +68,7 @@ bool IsConsistent(const Package& txns); * 3. If any dependencies exist between transactions, parents must appear before children. * 4. Transactions cannot conflict, i.e., spend the same inputs. */ -bool IsPackageWellFormed(const Package& txns, PackageValidationState& state); +bool IsPackageWellFormed(const Package& txns, PackageValidationState& state, bool require_sorted); /** Context-free check that a package is exactly one child and its parents; not all parents need to * be present, but the package must not contain any transactions that are not the child's parents. diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp index c4a498d21ae9f..6ec0a8cf786f4 100644 --- a/src/test/txpackage_tests.cpp +++ b/src/test/txpackage_tests.cpp @@ -69,7 +69,7 @@ BOOST_FIXTURE_TEST_CASE(package_sanitization_tests, TestChain100Setup) package_too_many.emplace_back(create_placeholder_tx(1, 1)); } PackageValidationState state_too_many; - BOOST_CHECK(!IsPackageWellFormed(package_too_many, state_too_many)); + BOOST_CHECK(!IsPackageWellFormed(package_too_many, state_too_many, /*require_sorted=*/true)); BOOST_CHECK_EQUAL(state_too_many.GetResult(), PackageValidationResult::PCKG_POLICY); BOOST_CHECK_EQUAL(state_too_many.GetRejectReason(), "package-too-many-transactions"); @@ -84,7 +84,7 @@ BOOST_FIXTURE_TEST_CASE(package_sanitization_tests, TestChain100Setup) } BOOST_CHECK(package_too_large.size() <= MAX_PACKAGE_COUNT); PackageValidationState state_too_large; - BOOST_CHECK(!IsPackageWellFormed(package_too_large, state_too_large)); + BOOST_CHECK(!IsPackageWellFormed(package_too_large, state_too_large, /*require_sorted=*/true)); BOOST_CHECK_EQUAL(state_too_large.GetResult(), PackageValidationResult::PCKG_POLICY); BOOST_CHECK_EQUAL(state_too_large.GetRejectReason(), "package-too-large"); } @@ -279,8 +279,8 @@ BOOST_FIXTURE_TEST_CASE(noncontextual_package_tests, TestChain100Setup) CTransactionRef tx_child = MakeTransactionRef(mtx_child); PackageValidationState state; - BOOST_CHECK(IsPackageWellFormed({tx_parent, tx_child}, state)); - BOOST_CHECK(!IsPackageWellFormed({tx_child, tx_parent}, state)); + BOOST_CHECK(IsPackageWellFormed({tx_parent, tx_child}, state, /*require_sorted=*/true)); + BOOST_CHECK(!IsPackageWellFormed({tx_child, tx_parent}, state, /*require_sorted=*/true)); BOOST_CHECK_EQUAL(state.GetResult(), PackageValidationResult::PCKG_POLICY); BOOST_CHECK_EQUAL(state.GetRejectReason(), "package-not-sorted"); BOOST_CHECK(IsChildWithParents({tx_parent, tx_child})); @@ -307,7 +307,7 @@ BOOST_FIXTURE_TEST_CASE(noncontextual_package_tests, TestChain100Setup) package.push_back(MakeTransactionRef(child)); PackageValidationState state; - BOOST_CHECK(IsPackageWellFormed(package, state)); + BOOST_CHECK(IsPackageWellFormed(package, state, /*require_sorted=*/true)); BOOST_CHECK(IsChildWithParents(package)); package.erase(package.begin()); @@ -344,8 +344,8 @@ BOOST_FIXTURE_TEST_CASE(noncontextual_package_tests, TestChain100Setup) // IsChildWithParents does not detect unsorted parents. BOOST_CHECK(IsChildWithParents({tx_parent_also_child, tx_parent, tx_child})); BOOST_CHECK(!IsSorted({tx_parent_also_child, tx_parent, tx_child})); - BOOST_CHECK(IsPackageWellFormed({tx_parent, tx_parent_also_child, tx_child}, state)); - BOOST_CHECK(!IsPackageWellFormed({tx_parent_also_child, tx_parent, tx_child}, state)); + BOOST_CHECK(IsPackageWellFormed({tx_parent, tx_parent_also_child, tx_child}, state, /*require_sorted=*/true)); + BOOST_CHECK(!IsPackageWellFormed({tx_parent_also_child, tx_parent, tx_child}, state, /*require_sorted=*/true)); BOOST_CHECK_EQUAL(state.GetResult(), PackageValidationResult::PCKG_POLICY); BOOST_CHECK_EQUAL(state.GetRejectReason(), "package-not-sorted"); } @@ -374,7 +374,7 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup) package_unrelated, /*test_accept=*/false); BOOST_CHECK(result_unrelated_submit.m_state.IsInvalid()); BOOST_CHECK_EQUAL(result_unrelated_submit.m_state.GetResult(), PackageValidationResult::PCKG_POLICY); - BOOST_CHECK_EQUAL(result_unrelated_submit.m_state.GetRejectReason(), "package-not-child-with-parents"); + BOOST_CHECK_EQUAL(result_unrelated_submit.m_state.GetRejectReason(), "not-ancestor-package"); BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); // Parent and Child (and Grandchild) Package @@ -409,16 +409,6 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup) CTransactionRef tx_grandchild = MakeTransactionRef(mtx_grandchild); package_3gen.push_back(tx_grandchild); - // 3 Generations is not allowed. - { - auto result_3gen_submit = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, - package_3gen, /*test_accept=*/false); - BOOST_CHECK(result_3gen_submit.m_state.IsInvalid()); - BOOST_CHECK_EQUAL(result_3gen_submit.m_state.GetResult(), PackageValidationResult::PCKG_POLICY); - BOOST_CHECK_EQUAL(result_3gen_submit.m_state.GetRejectReason(), "package-not-child-with-parents"); - BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); - } - // Parent and child package where transactions are invalid for reasons other than fee and // missing inputs, so the package validation isn't expected to happen. { @@ -492,44 +482,37 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup) BOOST_CHECK_EQUAL(it_child->second.m_state.GetResult(), TxValidationResult::TX_MISSING_INPUTS); } - // Child with missing parent. - mtx_child.vin.push_back(CTxIn(COutPoint(package_unrelated[0]->GetHash(), 0))); - Package package_missing_parent; - package_missing_parent.push_back(tx_parent); - package_missing_parent.push_back(MakeTransactionRef(mtx_child)); + // Submit package parent + child + grandchild. { - const auto result_missing_parent = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, - package_missing_parent, /*test_accept=*/false); - BOOST_CHECK(result_missing_parent.m_state.IsInvalid()); - BOOST_CHECK_EQUAL(result_missing_parent.m_state.GetResult(), PackageValidationResult::PCKG_POLICY); - BOOST_CHECK_EQUAL(result_missing_parent.m_state.GetRejectReason(), "package-not-child-with-unconfirmed-parents"); - BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); - } - - // Submit package with parent + child. - { - const auto submit_parent_child = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, - package_parent_child, /*test_accept=*/false); - expected_pool_size += 2; - BOOST_CHECK_MESSAGE(submit_parent_child.m_state.IsValid(), - "Package validation unexpectedly failed: " << submit_parent_child.m_state.GetRejectReason()); - BOOST_CHECK_EQUAL(submit_parent_child.m_tx_results.size(), package_parent_child.size()); - auto it_parent = submit_parent_child.m_tx_results.find(tx_parent->GetWitnessHash()); - auto it_child = submit_parent_child.m_tx_results.find(tx_child->GetWitnessHash()); - BOOST_CHECK(it_parent != submit_parent_child.m_tx_results.end()); + auto result_3gen_submit = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, + package_3gen, /*test_accept=*/false); + expected_pool_size += 3; + BOOST_CHECK_MESSAGE(result_3gen_submit.m_state.IsValid(), + "Package validation unexpectedly failed: " << result_3gen_submit.m_state.GetRejectReason()); + BOOST_CHECK_EQUAL(result_3gen_submit.m_tx_results.size(), package_3gen.size()); + auto it_parent = result_3gen_submit.m_tx_results.find(tx_parent->GetWitnessHash()); + auto it_child = result_3gen_submit.m_tx_results.find(tx_child->GetWitnessHash()); + auto it_grandchild = result_3gen_submit.m_tx_results.find(tx_grandchild->GetWitnessHash()); + BOOST_CHECK(it_parent != result_3gen_submit.m_tx_results.end()); BOOST_CHECK(it_parent->second.m_state.IsValid()); BOOST_CHECK(it_parent->second.m_effective_feerate == CFeeRate(1 * COIN, GetVirtualTransactionSize(*tx_parent))); BOOST_CHECK_EQUAL(it_parent->second.m_wtxids_fee_calculations.value().size(), 1); BOOST_CHECK_EQUAL(it_parent->second.m_wtxids_fee_calculations.value().front(), tx_parent->GetWitnessHash()); - BOOST_CHECK(it_child != submit_parent_child.m_tx_results.end()); + BOOST_CHECK(it_child != result_3gen_submit.m_tx_results.end()); BOOST_CHECK(it_child->second.m_state.IsValid()); BOOST_CHECK(it_child->second.m_effective_feerate == CFeeRate(1 * COIN, GetVirtualTransactionSize(*tx_child))); BOOST_CHECK_EQUAL(it_child->second.m_wtxids_fee_calculations.value().size(), 1); BOOST_CHECK_EQUAL(it_child->second.m_wtxids_fee_calculations.value().front(), tx_child->GetWitnessHash()); + BOOST_CHECK(it_grandchild != result_3gen_submit.m_tx_results.end()); + BOOST_CHECK(it_grandchild->second.m_state.IsValid()); + BOOST_CHECK(it_grandchild->second.m_effective_feerate == CFeeRate(1 * COIN, GetVirtualTransactionSize(*tx_grandchild))); + BOOST_CHECK_EQUAL(it_grandchild->second.m_wtxids_fee_calculations.value().size(), 1); + BOOST_CHECK_EQUAL(it_grandchild->second.m_wtxids_fee_calculations.value().front(), tx_grandchild->GetWitnessHash()); BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_parent->GetHash()))); BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_child->GetHash()))); + BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_grandchild->GetHash()))); } // Already-in-mempool transactions should be detected and de-duplicated. diff --git a/src/validation.cpp b/src/validation.cpp index 8a92c783d5e92..91cbb89e0b927 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1246,7 +1246,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: // These context-free package limits can be done before taking the mempool lock. PackageValidationState package_state; - if (!IsPackageWellFormed(txns, package_state)) return PackageMempoolAcceptResult(package_state, {}); + if (!IsPackageWellFormed(txns, package_state, /*require_sorted=*/true)) return PackageMempoolAcceptResult(package_state, {}); std::vector workspaces{}; workspaces.reserve(txns.size()); @@ -1345,52 +1345,20 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, // Check that the package is well-formed. If it isn't, we won't try to validate any of the // transactions and thus won't return any MempoolAcceptResults, just a package-wide error. - // Context-free package checks. - if (!IsPackageWellFormed(package, package_state_quit_early)) return PackageMempoolAcceptResult(package_state_quit_early, {}); - - // All transactions in the package must be a parent of the last transaction. This is just an - // opportunity for us to fail fast on a context-free check without taking the mempool lock. - if (!IsChildWithParents(package)) { - package_state_quit_early.Invalid(PackageValidationResult::PCKG_POLICY, "package-not-child-with-parents"); + if (!package.empty() && !IsPackageWellFormed(package, package_state_quit_early, /*require_sorted=*/false)) { return PackageMempoolAcceptResult(package_state_quit_early, {}); } - - // IsChildWithParents() guarantees the package is > 1 transactions. - assert(package.size() > 1); - // The package must be 1 child with all of its unconfirmed parents. The package is expected to - // be sorted, so the last transaction is the child. + // Sorts the package and calculate ancestor subpackages. + Packageifier packageified(package); const auto& child = package.back(); - std::unordered_set unconfirmed_parent_txids; - std::transform(package.cbegin(), package.cend() - 1, - std::inserter(unconfirmed_parent_txids, unconfirmed_parent_txids.end()), - [](const auto& tx) { return tx->GetHash(); }); - - // All child inputs must refer to a preceding package transaction or a confirmed UTXO. The only - // way to verify this is to look up the child's inputs in our current coins view (not including - // mempool), and enforce that all parents not present in the package be available at chain tip. - // Since this check can bring new coins into the coins cache, keep track of these coins and - // uncache them if we don't end up submitting this package to the mempool. - const CCoinsViewCache& coins_tip_cache = m_active_chainstate.CoinsTip(); - for (const auto& input : child->vin) { - if (!coins_tip_cache.HaveCoinInCache(input.prevout)) { - args.m_coins_to_uncache.push_back(input.prevout); - } - } - // Using the MemPoolAccept m_view cache allows us to look up these same coins faster later. - // This should be connecting directly to CoinsTip, not to m_viewmempool, because we specifically - // require inputs to be confirmed if they aren't in the package. - m_view.SetBackend(m_active_chainstate.CoinsTip()); - const auto package_or_confirmed = [this, &unconfirmed_parent_txids](const auto& input) { - return unconfirmed_parent_txids.count(input.prevout.hash) > 0 || m_view.HaveCoin(input.prevout); - }; - if (!std::all_of(child->vin.cbegin(), child->vin.cend(), package_or_confirmed)) { - package_state_quit_early.Invalid(PackageValidationResult::PCKG_POLICY, "package-not-child-with-unconfirmed-parents"); + const auto child_subpackage = packageified.GetAncestorSet(child); + // If this is an ancestor package, the last transaction's ancestor set should be the + // entire package. + if (child_subpackage == std::nullopt || child_subpackage.value().size() != package.size()) { + package_state_quit_early.Invalid(PackageValidationResult::PCKG_POLICY, "not-ancestor-package"); return PackageMempoolAcceptResult(package_state_quit_early, {}); } - // Protect against bugs where we pull more inputs from disk that miss being added to - // coins_to_uncache. The backend will be connected again when needed in PreChecks. - m_view.SetBackend(m_dummy); LOCK(m_pool.cs); // Stores final results that won't change @@ -1419,7 +1387,6 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, } return PackageMempoolAcceptResult(package_state_wrapped, {{tx->GetWitnessHash(), single_res}}); }; - Packageifier packageified(package); // Results from individual validation. "Nonfinal" because if a transaction fails by itself but // succeeds later (i.e. when evaluated with a fee-bumping child), the result changes (though not // reflected in this map). If a transaction fails more than once, we want to return the first diff --git a/test/functional/rpc_packages.py b/test/functional/rpc_packages.py index ae1a498e28abd..5305c2497a037 100755 --- a/test/functional/rpc_packages.py +++ b/test/functional/rpc_packages.py @@ -325,18 +325,11 @@ def test_submit_child_with_parents(self, num_parents, partial_submit): self.generate(node, 1) def test_submitpackage(self): - node = self.nodes[0] - self.log.info("Submitpackage valid packages with 1 child and some number of parents") for num_parents in [1, 2, 24]: self.test_submit_child_with_parents(num_parents, False) self.test_submit_child_with_parents(num_parents, True) - 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)] - assert_raises_rpc_error(-25, "not-child-with-parents", node.submitpackage, chain_hex) - if __name__ == "__main__": RPCPackagesTest().main() From e42d8d998b1ed4cff7d7194d04d13f40e0bf46b2 Mon Sep 17 00:00:00 2001 From: glozow Date: Mon, 20 Feb 2023 10:45:56 +0000 Subject: [PATCH 18/57] -- Part 1: Orphan Resolution Module -- Create TxPackageTracker module, responsible for handling orphan resolution. Enable node to retry orphan parent downloading from multiple peers who announced the tx. Use preferred peers and avoid overloading peers with too many requests. Making the orphanage more reliable is also necessary because it becomes part of the "critical path" for some transactions. A 0-fee parent bumped by a child *must* be relayed via package relay; if the orphanage overflows or is churned by a malicious peer, the package would be censored from the network. From 08212361d6f62fc80e233ff2e9764e1d2445ecb8 Mon Sep 17 00:00:00 2001 From: glozow Date: Sun, 1 May 2022 10:41:53 -0700 Subject: [PATCH 19/57] [p2p] add tx package tracker --- src/Makefile.am | 2 ++ src/net_processing.cpp | 3 +++ src/node/txpackagetracker.cpp | 14 ++++++++++++++ src/node/txpackagetracker.h | 26 ++++++++++++++++++++++++++ 4 files changed, 45 insertions(+) create mode 100644 src/node/txpackagetracker.cpp create mode 100644 src/node/txpackagetracker.h diff --git a/src/Makefile.am b/src/Makefile.am index 1d7004ac863bf..234d90732334f 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -219,6 +219,7 @@ BITCOIN_CORE_H = \ node/minisketchwrapper.h \ node/psbt.h \ node/transaction.h \ + node/txpackagetracker.h \ node/txreconciliation.h \ node/utxo_snapshot.h \ node/validation_cache_args.h \ @@ -410,6 +411,7 @@ libbitcoin_node_a_SOURCES = \ node/minisketchwrapper.cpp \ node/psbt.cpp \ node/transaction.cpp \ + node/txpackagetracker.cpp \ node/txreconciliation.cpp \ node/utxo_snapshot.cpp \ node/validation_cache_args.cpp \ diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 68bd91297c1fe..2c9191b661204 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include @@ -716,6 +717,7 @@ class PeerManagerImpl final : public PeerManager CTxMemPool& m_mempool; TxRequestTracker m_txrequest GUARDED_BY(::cs_main); std::unique_ptr m_txreconciliation; + std::unique_ptr m_txpackagetracker; /** The height of the best chain */ std::atomic m_best_height{-1}; @@ -1801,6 +1803,7 @@ PeerManagerImpl::PeerManagerImpl(CConnman& connman, AddrMan& addrman, if (gArgs.GetBoolArg("-txreconciliation", DEFAULT_TXRECONCILIATION_ENABLE)) { m_txreconciliation = std::make_unique(TXRECONCILIATION_VERSION); } + m_txpackagetracker = std::make_unique(); } void PeerManagerImpl::StartScheduledTasks(CScheduler& scheduler) diff --git a/src/node/txpackagetracker.cpp b/src/node/txpackagetracker.cpp new file mode 100644 index 0000000000000..8d327b0530643 --- /dev/null +++ b/src/node/txpackagetracker.cpp @@ -0,0 +1,14 @@ +// Copyright (c) 2022 +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include + +namespace node { +class TxPackageTracker::Impl { +}; + +TxPackageTracker::TxPackageTracker() : m_impl{std::make_unique()} {} +TxPackageTracker::~TxPackageTracker() = default; + +} // namespace node diff --git a/src/node/txpackagetracker.h b/src/node/txpackagetracker.h new file mode 100644 index 0000000000000..1795d18de5223 --- /dev/null +++ b/src/node/txpackagetracker.h @@ -0,0 +1,26 @@ +// Copyright (c) 2022 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#ifndef BITCOIN_NODE_TXPACKAGETRACKER_H +#define BITCOIN_NODE_TXPACKAGETRACKER_H + +#include + +#include +#include +#include + +namespace node { +static constexpr bool DEFAULT_ENABLE_PACKAGE_RELAY{false}; + +class TxPackageTracker { + class Impl; + const std::unique_ptr m_impl; + +public: + explicit TxPackageTracker(); + ~TxPackageTracker(); +}; +} // namespace node +#endif // BITCOIN_NODE_TXPACKAGETRACKER_H From bcb33bb0a91cbf53954bc7e34f5f0cbe8fd216e3 Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 6 Oct 2022 12:02:29 +0100 Subject: [PATCH 20/57] [refactor] wrap TxOrphanage inside TxPackageTracker --- src/net_processing.cpp | 28 ++++++++++++-------------- src/node/txpackagetracker.cpp | 30 ++++++++++++++++++++++++++++ src/node/txpackagetracker.h | 37 +++++++++++++++++++++++++++++++++++ 3 files changed, 79 insertions(+), 16 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 2c9191b661204..270747738bb70 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -36,7 +36,6 @@ #include #include #include -#include #include #include // For NDEBUG compile time check #include @@ -936,9 +935,6 @@ class PeerManagerImpl final : public PeerManager /** Number of peers from which we're downloading blocks. */ int m_peers_downloading_from GUARDED_BY(cs_main) = 0; - /** Storage for orphan information */ - TxOrphanage m_orphanage; - void AddToCompactExtraTransactions(const CTransactionRef& tx) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex); /** Orphan/conflicted/etc transactions that are kept for compact block reconstruction. @@ -1510,7 +1506,7 @@ void PeerManagerImpl::FinalizeNode(const CNode& node) for (const QueuedBlock& entry : state->vBlocksInFlight) { mapBlocksInFlight.erase(entry.pindex->GetBlockHash()); } - m_orphanage.EraseForPeer(nodeid); + m_txpackagetracker->DisconnectedPeer(nodeid); m_txrequest.DisconnectedPeer(nodeid); if (m_txreconciliation) m_txreconciliation->ForgetPeer(nodeid); m_num_preferred_download_peers -= state->fPreferredDownload; @@ -1529,7 +1525,7 @@ void PeerManagerImpl::FinalizeNode(const CNode& node) assert(m_outbound_peers_with_protect_from_disconnect == 0); assert(m_wtxid_relay_peers == 0); assert(m_txrequest.Size() == 0); - assert(m_orphanage.Size() == 0); + assert(m_txpackagetracker->OrphanageSize() == 0); } } // cs_main if (node.fSuccessfullyConnected && misbehavior == 0 && @@ -1828,7 +1824,7 @@ void PeerManagerImpl::StartScheduledTasks(CScheduler& scheduler) */ void PeerManagerImpl::BlockConnected(const std::shared_ptr& pblock, const CBlockIndex* pindex) { - m_orphanage.EraseForBlock(*pblock); + m_txpackagetracker->BlockConnected(*pblock); m_last_tip_update = GetTime(); { @@ -2015,7 +2011,7 @@ bool PeerManagerImpl::AlreadyHaveTx(const GenTxid& gtxid) const uint256& hash = gtxid.GetHash(); - if (m_orphanage.HaveTx(gtxid)) return true; + if (m_txpackagetracker->OrphanageHaveTx(gtxid)) return true; { LOCK(m_recent_confirmed_transactions_mutex); @@ -2920,7 +2916,7 @@ bool PeerManagerImpl::ProcessOrphanTx(Peer& peer) CTransactionRef porphanTx = nullptr; - while (CTransactionRef porphanTx = m_orphanage.GetTxToReconsider(peer.m_id)) { + while (CTransactionRef porphanTx = m_txpackagetracker->GetTxToReconsider(peer.m_id)) { const MempoolAcceptResult result = m_chainman.ProcessTransaction(porphanTx); const TxValidationState& state = result.m_state; const uint256& orphanHash = porphanTx->GetHash(); @@ -2928,8 +2924,8 @@ bool PeerManagerImpl::ProcessOrphanTx(Peer& peer) if (result.m_result_type == MempoolAcceptResult::ResultType::VALID) { LogPrint(BCLog::MEMPOOL, " accepted orphan tx %s\n", orphanHash.ToString()); RelayTransaction(orphanHash, porphanTx->GetWitnessHash()); - m_orphanage.AddChildrenToWorkSet(*porphanTx); - m_orphanage.EraseTx(orphanHash); + m_txpackagetracker->AddChildrenToWorkSet(*porphanTx); + m_txpackagetracker->EraseOrphanTx(orphanHash); for (const CTransactionRef& removedTx : result.m_replaced_transactions.value()) { AddToCompactExtraTransactions(removedTx); } @@ -2975,7 +2971,7 @@ bool PeerManagerImpl::ProcessOrphanTx(Peer& peer) m_recent_rejects.insert(porphanTx->GetHash()); } } - m_orphanage.EraseTx(orphanHash); + m_txpackagetracker->EraseOrphanTx(orphanHash); return true; } } @@ -4051,7 +4047,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, m_txrequest.ForgetTxHash(tx.GetHash()); m_txrequest.ForgetTxHash(tx.GetWitnessHash()); RelayTransaction(tx.GetHash(), tx.GetWitnessHash()); - m_orphanage.AddChildrenToWorkSet(tx); + m_txpackagetracker->AddChildrenToWorkSet(tx); pfrom.m_last_tx_time = GetTime(); @@ -4098,7 +4094,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, if (!AlreadyHaveTx(gtxid)) AddTxAnnouncement(pfrom, gtxid, current_time); } - if (m_orphanage.AddTx(ptx, pfrom.GetId())) { + if (m_txpackagetracker->OrphanageAddTx(ptx, pfrom.GetId())) { AddToCompactExtraTransactions(ptx); } @@ -4108,7 +4104,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // DoS prevention: do not allow m_orphanage to grow unbounded (see CVE-2012-3789) unsigned int nMaxOrphanTx = (unsigned int)std::max((int64_t)0, gArgs.GetIntArg("-maxorphantx", DEFAULT_MAX_ORPHAN_TRANSACTIONS)); - m_orphanage.LimitOrphans(nMaxOrphanTx); + m_txpackagetracker->LimitOrphans(nMaxOrphanTx); } else { LogPrint(BCLog::MEMPOOL, "not keeping orphan with rejected parents %s\n",tx.GetHash().ToString()); // We will continue to reject this tx since it has rejected @@ -4928,7 +4924,7 @@ bool PeerManagerImpl::ProcessMessages(CNode* pfrom, std::atomic& interrupt // by another peer that was already processed; in that case, // the extra work may not be noticed, possibly resulting in an // unnecessary 100ms delay) - if (m_orphanage.HaveTxToReconsider(peer->m_id)) fMoreWork = true; + if (m_txpackagetracker->HaveTxToReconsider(peer->m_id)) fMoreWork = true; } catch (const std::exception& e) { LogPrint(BCLog::NET, "%s(%s, %u bytes): Exception '%s' (%s) caught\n", __func__, SanitizeString(msg.m_type), msg.m_message_size, e.what(), typeid(e).name()); } catch (...) { diff --git a/src/node/txpackagetracker.cpp b/src/node/txpackagetracker.cpp index 8d327b0530643..d0f0c3fc56bda 100644 --- a/src/node/txpackagetracker.cpp +++ b/src/node/txpackagetracker.cpp @@ -4,11 +4,41 @@ #include +#include + namespace node { class TxPackageTracker::Impl { + /** Manages unvalidated tx data (orphan transactions for which we are downloading ancestors). */ + TxOrphanage m_orphanage; +public: + Impl() = default; + + // Orphanage Wrapper Functions + bool OrphanageAddTx(const CTransactionRef& tx, NodeId peer) { return m_orphanage.AddTx(tx, peer); } + bool OrphanageHaveTx(const GenTxid& gtxid) { return m_orphanage.HaveTx(gtxid); } + CTransactionRef GetTxToReconsider(NodeId peer) { return m_orphanage.GetTxToReconsider(peer); } + int EraseOrphanTx(const uint256& txid) { return m_orphanage.EraseTx(txid); } + void DisconnectedPeer(NodeId peer) { + m_orphanage.EraseForPeer(peer); + } + void BlockConnected(const CBlock& block) { m_orphanage.EraseForBlock(block); } + void LimitOrphans(unsigned int max_orphans) { m_orphanage.LimitOrphans(max_orphans); } + void AddChildrenToWorkSet(const CTransaction& tx) { m_orphanage.AddChildrenToWorkSet(tx); } + bool HaveTxToReconsider(NodeId peer) { return m_orphanage.HaveTxToReconsider(peer); } + size_t OrphanageSize() { return m_orphanage.Size(); } }; TxPackageTracker::TxPackageTracker() : m_impl{std::make_unique()} {} TxPackageTracker::~TxPackageTracker() = default; +bool TxPackageTracker::OrphanageAddTx(const CTransactionRef& tx, NodeId peer) { return m_impl->OrphanageAddTx(tx, peer); } +bool TxPackageTracker::OrphanageHaveTx(const GenTxid& gtxid) { return m_impl->OrphanageHaveTx(gtxid); } +CTransactionRef TxPackageTracker::GetTxToReconsider(NodeId peer) { return m_impl->GetTxToReconsider(peer); } +int TxPackageTracker::EraseOrphanTx(const uint256& txid) { return m_impl->EraseOrphanTx(txid); } +void TxPackageTracker::DisconnectedPeer(NodeId peer) { m_impl->DisconnectedPeer(peer); } +void TxPackageTracker::BlockConnected(const CBlock& block) { m_impl->BlockConnected(block); } +void TxPackageTracker::LimitOrphans(unsigned int max_orphans) { m_impl->LimitOrphans(max_orphans); } +void TxPackageTracker::AddChildrenToWorkSet(const CTransaction& tx) { m_impl->AddChildrenToWorkSet(tx); } +bool TxPackageTracker::HaveTxToReconsider(NodeId peer) { return m_impl->HaveTxToReconsider(peer); } +size_t TxPackageTracker::OrphanageSize() { return m_impl->OrphanageSize(); } } // namespace node diff --git a/src/node/txpackagetracker.h b/src/node/txpackagetracker.h index 1795d18de5223..6738f824b822b 100644 --- a/src/node/txpackagetracker.h +++ b/src/node/txpackagetracker.h @@ -11,6 +11,7 @@ #include #include +class TxOrphanage; namespace node { static constexpr bool DEFAULT_ENABLE_PACKAGE_RELAY{false}; @@ -21,6 +22,42 @@ class TxPackageTracker { public: explicit TxPackageTracker(); ~TxPackageTracker(); + + // Orphanage wrapper functions + /** Add new tx to orphanage if it isn't already there. Returns whether the tx was added. */ + bool OrphanageAddTx(const CTransactionRef& tx, NodeId peer); + + /** Check if we already have an orphan transaction (by txid or wtxid) */ + bool OrphanageHaveTx(const GenTxid& gtxid); + + /** Extract a transaction from a peer's work set + * Returns nullptr if there are no transactions to work on. + * Otherwise returns the transaction reference, and removes + * it from the work set. + */ + CTransactionRef GetTxToReconsider(NodeId peer); + + /** Erase an orphan by txid */ + int EraseOrphanTx(const uint256& txid); + + /** Erase all orphans announced by a peer (eg, after that peer disconnects) */ + void DisconnectedPeer(NodeId peer); + + /** Erase all orphans included in or invalidated by a new block */ + void BlockConnected(const CBlock& block); + + /** Limit the orphanage to the given maximum */ + void LimitOrphans(unsigned int max_orphans); + + /** Add any orphans that list a particular tx as a parent into the from peer's work set */ + void AddChildrenToWorkSet(const CTransaction& tx); + + /** Does this peer have any orphans to validate? */ + bool HaveTxToReconsider(NodeId peer); + + /** Return how many entries exist in the orphange */ + size_t OrphanageSize(); + }; } // namespace node #endif // BITCOIN_NODE_TXPACKAGETRACKER_H From 718ce4aa62e121dea8f94134e5db281a91c45267 Mon Sep 17 00:00:00 2001 From: glozow Date: Tue, 24 Jan 2023 13:14:31 +0000 Subject: [PATCH 21/57] [txorphange] GetTx by wtxid --- src/txorphanage.cpp | 7 +++++++ src/txorphanage.h | 3 +++ 2 files changed, 10 insertions(+) diff --git a/src/txorphanage.cpp b/src/txorphanage.cpp index 19f9fae99835d..70e4f1f699ff5 100644 --- a/src/txorphanage.cpp +++ b/src/txorphanage.cpp @@ -52,6 +52,13 @@ bool TxOrphanage::AddTx(const CTransactionRef& tx, NodeId peer) return true; } +CTransactionRef TxOrphanage::GetTx(const uint256& wtxid) +{ + LOCK(m_mutex); + const auto it = m_wtxid_to_orphan_it.find(wtxid); + return it == m_wtxid_to_orphan_it.end() ? nullptr : it->second->second.tx; +} + int TxOrphanage::EraseTx(const uint256& txid) { LOCK(m_mutex); diff --git a/src/txorphanage.h b/src/txorphanage.h index 45276c6c983b5..5539bc0ec7df5 100644 --- a/src/txorphanage.h +++ b/src/txorphanage.h @@ -23,6 +23,9 @@ class TxOrphanage { /** Add a new orphan transaction */ bool AddTx(const CTransactionRef& tx, NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); + /** Get orphan transaction by wtxid. Returns nullptr if we don't have it anymore. */ + CTransactionRef GetTx(const uint256& wtxid) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); + /** Check if we already have an orphan transaction (by txid or wtxid) */ bool HaveTx(const GenTxid& gtxid) const EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); From 60566f41c4df63f7b7871c0214a35216b8e22770 Mon Sep 17 00:00:00 2001 From: glozow Date: Mon, 20 Feb 2023 09:43:41 +0000 Subject: [PATCH 22/57] [txorphanage] EraseTx by wtxid --- src/net_processing.cpp | 4 ++-- src/txorphanage.cpp | 20 ++++++++++---------- src/txorphanage.h | 4 ++-- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 270747738bb70..6c23bc76ececc 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2925,7 +2925,7 @@ bool PeerManagerImpl::ProcessOrphanTx(Peer& peer) LogPrint(BCLog::MEMPOOL, " accepted orphan tx %s\n", orphanHash.ToString()); RelayTransaction(orphanHash, porphanTx->GetWitnessHash()); m_txpackagetracker->AddChildrenToWorkSet(*porphanTx); - m_txpackagetracker->EraseOrphanTx(orphanHash); + m_txpackagetracker->EraseOrphanTx(porphanTx->GetWitnessHash()); for (const CTransactionRef& removedTx : result.m_replaced_transactions.value()) { AddToCompactExtraTransactions(removedTx); } @@ -2971,7 +2971,7 @@ bool PeerManagerImpl::ProcessOrphanTx(Peer& peer) m_recent_rejects.insert(porphanTx->GetHash()); } } - m_txpackagetracker->EraseOrphanTx(orphanHash); + m_txpackagetracker->EraseOrphanTx(porphanTx->GetWitnessHash()); return true; } } diff --git a/src/txorphanage.cpp b/src/txorphanage.cpp index 70e4f1f699ff5..70aaa35156ccd 100644 --- a/src/txorphanage.cpp +++ b/src/txorphanage.cpp @@ -59,18 +59,18 @@ CTransactionRef TxOrphanage::GetTx(const uint256& wtxid) return it == m_wtxid_to_orphan_it.end() ? nullptr : it->second->second.tx; } -int TxOrphanage::EraseTx(const uint256& txid) +int TxOrphanage::EraseTx(const uint256& wtxid) { LOCK(m_mutex); - return _EraseTx(txid); + return _EraseTx(wtxid); } -int TxOrphanage::_EraseTx(const uint256& txid) +int TxOrphanage::_EraseTx(const uint256& wtxid) { AssertLockHeld(m_mutex); - std::map::iterator it = m_orphans.find(txid); - if (it == m_orphans.end()) - return 0; + const auto wtxid_it = m_wtxid_to_orphan_it.find(wtxid); + if (wtxid_it == m_wtxid_to_orphan_it.end()) return 0; + std::map::iterator it = wtxid_it->second; for (const CTxIn& txin : it->second.tx->vin) { auto itPrev = m_outpoint_to_orphan_it.find(txin.prevout); @@ -110,7 +110,7 @@ void TxOrphanage::EraseForPeer(NodeId peer) std::map::iterator maybeErase = iter++; // increment to avoid iterator becoming invalid if (maybeErase->second.fromPeer == peer) { - nErased += _EraseTx(maybeErase->second.tx->GetHash()); + nErased += _EraseTx(maybeErase->second.tx->GetWitnessHash()); } } if (nErased > 0) LogPrint(BCLog::MEMPOOL, "Erased %d orphan tx from peer=%d\n", nErased, peer); @@ -132,7 +132,7 @@ void TxOrphanage::LimitOrphans(unsigned int max_orphans) { std::map::iterator maybeErase = iter++; if (maybeErase->second.nTimeExpire <= nNow) { - nErased += _EraseTx(maybeErase->second.tx->GetHash()); + nErased += _EraseTx(maybeErase->second.tx->GetWitnessHash()); } else { nMinExpTime = std::min(maybeErase->second.nTimeExpire, nMinExpTime); } @@ -146,7 +146,7 @@ void TxOrphanage::LimitOrphans(unsigned int max_orphans) { // Evict a random orphan: size_t randompos = rng.randrange(m_orphan_list.size()); - _EraseTx(m_orphan_list[randompos]->first); + _EraseTx(m_orphan_list[randompos]->second.tx->GetWitnessHash()); ++nEvicted; } if (nEvicted > 0) LogPrint(BCLog::MEMPOOL, "orphanage overflow, removed %u tx\n", nEvicted); @@ -228,7 +228,7 @@ void TxOrphanage::EraseForBlock(const CBlock& block) if (itByPrev == m_outpoint_to_orphan_it.end()) continue; for (auto mi = itByPrev->second.begin(); mi != itByPrev->second.end(); ++mi) { const CTransaction& orphanTx = *(*mi)->second.tx; - const uint256& orphanHash = orphanTx.GetHash(); + const uint256& orphanHash = orphanTx.GetWitnessHash(); vOrphanErase.push_back(orphanHash); } } diff --git a/src/txorphanage.h b/src/txorphanage.h index 5539bc0ec7df5..72b6f34e0cf8c 100644 --- a/src/txorphanage.h +++ b/src/txorphanage.h @@ -36,8 +36,8 @@ class TxOrphanage { */ CTransactionRef GetTxToReconsider(NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); - /** Erase an orphan by txid */ - int EraseTx(const uint256& txid) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); + /** Erase an orphan by wtxid */ + int EraseTx(const uint256& wtxid) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); /** Erase all orphans announced by a peer (eg, after that peer disconnects) */ void EraseForPeer(NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); From dce6d8b9fa6e1c4457d778c2e413fc759523fc26 Mon Sep 17 00:00:00 2001 From: glozow Date: Mon, 20 Feb 2023 09:59:14 +0000 Subject: [PATCH 23/57] [txorphanage] return erased wtxids from EraseForBlock --- src/txorphanage.cpp | 3 ++- src/txorphanage.h | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/txorphanage.cpp b/src/txorphanage.cpp index 70aaa35156ccd..6c7ab0d0dee39 100644 --- a/src/txorphanage.cpp +++ b/src/txorphanage.cpp @@ -213,7 +213,7 @@ bool TxOrphanage::HaveTxToReconsider(NodeId peer) return false; } -void TxOrphanage::EraseForBlock(const CBlock& block) +std::vector TxOrphanage::EraseForBlock(const CBlock& block) { LOCK(m_mutex); @@ -242,4 +242,5 @@ void TxOrphanage::EraseForBlock(const CBlock& block) } LogPrint(BCLog::MEMPOOL, "Erased %d orphan tx included or conflicted by block\n", nErased); } + return vOrphanErase; } diff --git a/src/txorphanage.h b/src/txorphanage.h index 72b6f34e0cf8c..6876b6b6e0d2b 100644 --- a/src/txorphanage.h +++ b/src/txorphanage.h @@ -42,8 +42,8 @@ class TxOrphanage { /** Erase all orphans announced by a peer (eg, after that peer disconnects) */ void EraseForPeer(NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); - /** Erase all orphans included in or invalidated by a new block */ - void EraseForBlock(const CBlock& block) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); + /** Erase all orphans included in or invalidated by a new block. Returns wtxids of erased txns. */ + std::vector EraseForBlock(const CBlock& block) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); /** Limit the orphanage to the given maximum */ void LimitOrphans(unsigned int max_orphans) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); From 2e72630aeb9b0e1381e9d1188a314c9a3f6cd208 Mon Sep 17 00:00:00 2001 From: glozow Date: Mon, 6 Mar 2023 16:46:18 +0000 Subject: [PATCH 24/57] [log] add category TXPACKAGES for orphanage and package relay --- src/logging.cpp | 3 +++ src/logging.h | 1 + src/txorphanage.cpp | 12 ++++++------ 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/src/logging.cpp b/src/logging.cpp index 7725fefeb7de3..7bd4479654f3f 100644 --- a/src/logging.cpp +++ b/src/logging.cpp @@ -182,6 +182,7 @@ const CLogCategoryDesc LogCategories[] = {BCLog::BLOCKSTORE, "blockstorage"}, {BCLog::TXRECONCILIATION, "txreconciliation"}, {BCLog::SCAN, "scan"}, + {BCLog::TXPACKAGES, "txpackages"}, {BCLog::ALL, "1"}, {BCLog::ALL, "all"}, }; @@ -286,6 +287,8 @@ std::string LogCategoryToStr(BCLog::LogFlags category) return "txreconciliation"; case BCLog::LogFlags::SCAN: return "scan"; + case BCLog::LogFlags::TXPACKAGES: + return "txpackages"; case BCLog::LogFlags::ALL: return "all"; } diff --git a/src/logging.h b/src/logging.h index e7c554e79f83a..16f1d4665840f 100644 --- a/src/logging.h +++ b/src/logging.h @@ -68,6 +68,7 @@ namespace BCLog { BLOCKSTORE = (1 << 26), TXRECONCILIATION = (1 << 27), SCAN = (1 << 28), + TXPACKAGES = (1 << 29), ALL = ~(uint32_t)0, }; enum class Level { diff --git a/src/txorphanage.cpp b/src/txorphanage.cpp index 6c7ab0d0dee39..6861c445ba53e 100644 --- a/src/txorphanage.cpp +++ b/src/txorphanage.cpp @@ -34,7 +34,7 @@ bool TxOrphanage::AddTx(const CTransactionRef& tx, NodeId peer) unsigned int sz = GetTransactionWeight(*tx); if (sz > MAX_STANDARD_TX_WEIGHT) { - LogPrint(BCLog::MEMPOOL, "ignoring large orphan tx (size: %u, hash: %s)\n", sz, hash.ToString()); + LogPrint(BCLog::TXPACKAGES, "ignoring large orphan tx (size: %u, hash: %s)\n", sz, hash.ToString()); return false; } @@ -47,7 +47,7 @@ bool TxOrphanage::AddTx(const CTransactionRef& tx, NodeId peer) m_outpoint_to_orphan_it[txin.prevout].insert(ret.first); } - LogPrint(BCLog::MEMPOOL, "stored orphan tx %s (mapsz %u outsz %u)\n", hash.ToString(), + LogPrint(BCLog::TXPACKAGES, "stored orphan tx %s (mapsz %u outsz %u)\n", hash.ToString(), m_orphans.size(), m_outpoint_to_orphan_it.size()); return true; } @@ -113,7 +113,7 @@ void TxOrphanage::EraseForPeer(NodeId peer) nErased += _EraseTx(maybeErase->second.tx->GetWitnessHash()); } } - if (nErased > 0) LogPrint(BCLog::MEMPOOL, "Erased %d orphan tx from peer=%d\n", nErased, peer); + if (nErased > 0) LogPrint(BCLog::TXPACKAGES, "Erased %d orphan tx from peer=%d\n", nErased, peer); } void TxOrphanage::LimitOrphans(unsigned int max_orphans) @@ -139,7 +139,7 @@ void TxOrphanage::LimitOrphans(unsigned int max_orphans) } // Sweep again 5 minutes after the next entry that expires in order to batch the linear scan. nNextSweep = nMinExpTime + ORPHAN_TX_EXPIRE_INTERVAL; - if (nErased > 0) LogPrint(BCLog::MEMPOOL, "Erased %d orphan tx due to expiration\n", nErased); + if (nErased > 0) LogPrint(BCLog::TXPACKAGES, "Erased %d orphan tx due to expiration\n", nErased); } FastRandomContext rng; while (m_orphans.size() > max_orphans) @@ -149,7 +149,7 @@ void TxOrphanage::LimitOrphans(unsigned int max_orphans) _EraseTx(m_orphan_list[randompos]->second.tx->GetWitnessHash()); ++nEvicted; } - if (nEvicted > 0) LogPrint(BCLog::MEMPOOL, "orphanage overflow, removed %u tx\n", nEvicted); + if (nEvicted > 0) LogPrint(BCLog::TXPACKAGES, "orphanage overflow, removed %u tx\n", nEvicted); } void TxOrphanage::AddChildrenToWorkSet(const CTransaction& tx) @@ -240,7 +240,7 @@ std::vector TxOrphanage::EraseForBlock(const CBlock& block) for (const uint256& orphanHash : vOrphanErase) { nErased += _EraseTx(orphanHash); } - LogPrint(BCLog::MEMPOOL, "Erased %d orphan tx included or conflicted by block\n", nErased); + LogPrint(BCLog::TXPACKAGES, "Erased %d orphan tx included or conflicted by block\n", nErased); } return vOrphanErase; } From 49aa3862cfc1427581603a56eae2410d3d204b1e Mon Sep 17 00:00:00 2001 From: glozow Date: Fri, 31 Mar 2023 10:58:09 +0100 Subject: [PATCH 25/57] [txorphanage] impose a maximum total size of orphans No effect for now. TODO: add per-peer limits on orphan memory usage --- src/test/orphanage_tests.cpp | 61 ++++++++++++++++++++++++++++++++++-- src/txorphanage.cpp | 5 +-- src/txorphanage.h | 13 ++++++++ 3 files changed, 75 insertions(+), 4 deletions(-) diff --git a/src/test/orphanage_tests.cpp b/src/test/orphanage_tests.cpp index a2c477433847e..dfb90fe1e3c42 100644 --- a/src/test/orphanage_tests.cpp +++ b/src/test/orphanage_tests.cpp @@ -3,6 +3,7 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include +#include #include #include