Merge bitcoin/bitcoin#33892: policy: allow <minrelay txns in package context if paid for by cpfp

e44dec027c add release note about supporing non-TRUC <minrelay txns (Greg Sanders)
1488315d76 policy: Allow any transaction version with < minrelay (Greg Sanders)

Pull request description:

  Prior to cluster mempool, a policy was in place that
  disallowed non-TRUC transactions from being
  TX_RECONSIDERABLE in a package setting if it was below
  minrelay. This was meant to simplify reasoning about mempool
  trimming requirements with non-trivial transaction
  topologies in the mempool. This is no longer a concern
  post-cluster mempool, so this is relaxed.

  In effect, this makes 0-value parent transactions relayable
  through the network without the TRUC restrictions and
  thus the anti-pinning protections.

ACKs for top commit:
  ajtowns:
    ACK e44dec027c - lgtm
  ismaelsadeeq:
    ACK e44dec027c

Tree-SHA512: 6fd1a2429c55ca844d9bd669ea797e29eca3f544f0b5d3484743d3c1cdf4364f7c7a058aaf707bcfd94b84c621bea03228cb39487cbc23912b9e0980a1e5b451

doc/policy/packages.md

@@ -99,14 +99,6 @@ submitted as a package.
 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 5sat/vB, a 1sat/vB parent transaction with a high-feerate child will not be accepted, even if
-submitted as a package. Note that this rule does not apply to
-[TRUC transactions](https://github.com/bitcoin/bips/blob/master/bip-0431.mediawiki) as an individual
-TRUC transaction is permitted to be below the mempool min relay feerate, assuming it is considered within
-a package that meets the mempool's feerate requirements.
-
 *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

doc/release-notes-33892.md

@@ -0,0 +1,8 @@
+P2P and network changes
+-----------------------
+
+- Transactions participating in one-parent-one-child package relay can now have the parent
+    with a feerate lower than the `-minrelaytxfee` feerate, even 0 fee. This expands the change
+    from 28.0 to also cover packages of non-TRUC transactions. Note that in general the
+    package child can have additional unconfirmed parents, but they must already be
+    in-mempool for the new package to be relayed. (#33892)

src/test/txpackage_tests.cpp

@@ -908,10 +908,10 @@ BOOST_AUTO_TEST_CASE(package_cpfp_tests)
         } else {
             BOOST_CHECK_EQUAL(submit_cpfp_deprio.m_state.GetResult(), PackageValidationResult::PCKG_TX);
             BOOST_CHECK_EQUAL(submit_cpfp_deprio.m_tx_results.find(tx_parent->GetWitnessHash())->second.m_state.GetResult(),
-                              TxValidationResult::TX_MEMPOOL_POLICY);
+                              TxValidationResult::TX_RECONSIDERABLE);
             BOOST_CHECK_EQUAL(submit_cpfp_deprio.m_tx_results.find(tx_child->GetWitnessHash())->second.m_state.GetResult(),
-                              TxValidationResult::TX_MISSING_INPUTS);
+                              TxValidationResult::TX_RECONSIDERABLE);
-            BOOST_CHECK(submit_cpfp_deprio.m_tx_results.find(tx_parent->GetWitnessHash())->second.m_state.GetRejectReason() == "min relay fee not met");
+            BOOST_CHECK_EQUAL(submit_cpfp_deprio.m_tx_results.find(tx_parent->GetWitnessHash())->second.m_state.GetRejectReason(), "mempool min fee not met");
             BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
         }
     }
@@ -1067,8 +1067,8 @@ BOOST_AUTO_TEST_CASE(package_cpfp_tests)
                     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)));
             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_EQUAL(it_child->second.m_state.GetResult(), TxValidationResult::TX_RECONSIDERABLE);
-            BOOST_CHECK(it_child->second.m_state.GetRejectReason() == "min relay fee not met");
+            BOOST_CHECK_EQUAL(it_child->second.m_state.GetRejectReason(), "mempool min fee not met");
         }
         expected_pool_size += 1;
         BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);

src/validation.cpp

@@ -943,18 +943,6 @@ 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 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.
-    // The only exception is TRUC transactions.
-    if (!bypass_limits && ws.m_ptx->version != TRUC_VERSION && ws.m_modified_fees < m_pool.m_opts.min_relay_feerate.GetFee(ws.m_vsize)) {
-        // Even though this is a fee-related failure, this result is TX_MEMPOOL_POLICY, not
-        // TX_RECONSIDERABLE, because it cannot be bypassed using package validation.
-        return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "min relay fee not met",
-                             strprintf("%d < %d", ws.m_modified_fees, m_pool.m_opts.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.

test/functional/mempool_ephemeral_dust.py

@@ -216,18 +216,16 @@ class EphemeralDustTest(BitcoinTestFramework):
         self.connect_nodes(0, 1)
         assert_mempool_contents(self, self.nodes[0], expected=[])
 
-    # N.B. If individual minrelay requirement is dropped, this test can be dropped
     def test_non_truc(self):
-        self.log.info("Test that v2 dust-having transaction is rejected even if spent, because of min relay requirement")
+        self.log.info("Test that v2 dust-having transaction is also accepted if spent")
 
         assert_equal(self.nodes[0].getrawmempool(), [])
         dusty_tx, sweep_tx = self.create_ephemeral_dust_package(tx_version=2)
 
         res = self.nodes[0].submitpackage([dusty_tx["hex"], sweep_tx["hex"]])
-        assert_equal(res["package_msg"], "transaction failed")
+        assert_equal(res["package_msg"], "success")
-        assert_equal(res["tx-results"][dusty_tx["wtxid"]]["error"], "min relay fee not met, 0 < 15")
+        assert_mempool_contents(self, self.nodes[0], expected=[dusty_tx["tx"], sweep_tx["tx"]])
-
+        self.generate(self.nodes[0], 1)
-        assert_equal(self.nodes[0].getrawmempool(), [])
 
     def test_unspent_ephemeral(self):
         self.log.info("Test that spending from a tx with ephemeral outputs is only allowed if dust is spent as well")

test/functional/mempool_truc.py

@@ -612,7 +612,7 @@ class MempoolTRUC(BitcoinTestFramework):
     @cleanup(extra_args=None)
     def test_minrelay_in_package_combos(self):
         node = self.nodes[0]
-        self.log.info("Test that only TRUC transactions can be under minrelaytxfee for various settings...")
+        self.log.info("Test that all transaction versions can be under minrelaytxfee for various settings...")
 
         for minrelay_setting in (0, 5, 10, 100, 500, 1000, 5000, 333333, 2500000):
             self.log.info(f"-> Test -minrelaytxfee={minrelay_setting}sat/kvB...")
@@ -649,14 +649,8 @@ class MempoolTRUC(BitcoinTestFramework):
             assert_equal(result_truc["package_msg"], "success")
 
             result_non_truc = node.submitpackage([tx_v2_0fee_parent["hex"], tx_v2_child["hex"]], maxfeerate=0)
-            if minrelayfeerate > 0:
+            assert_equal(result_non_truc["package_msg"], "success")
-                assert_equal(result_non_truc["package_msg"], "transaction failed")
+            self.check_mempool([tx_v2_0fee_parent["txid"], tx_v2_child["txid"], tx_v3_0fee_parent["txid"], tx_v3_child["txid"]])
-                min_fee_parent = int(get_fee(tx_v2_0fee_parent["tx"].get_vsize(), minrelayfeerate) * COIN)
-                assert_equal(result_non_truc["tx-results"][tx_v2_0fee_parent["wtxid"]]["error"], f"min relay fee not met, 0 < {min_fee_parent}")
-                self.check_mempool([tx_v3_0fee_parent["txid"], tx_v3_child["txid"]])
-            else:
-                assert_equal(result_non_truc["package_msg"], "success")
-                self.check_mempool([tx_v2_0fee_parent["txid"], tx_v2_child["txid"], tx_v3_0fee_parent["txid"], tx_v3_child["txid"]])
 
 
     def run_test(self):

test/functional/p2p_orphan_handling.py

@@ -5,6 +5,7 @@
 
 import time
 
+from test_framework.blocktools import MAX_STANDARD_TX_WEIGHT
 from test_framework.mempool_util import (
     create_large_orphan,
     tx_in_orphanage,
@@ -188,15 +189,15 @@ class OrphanHandlingTest(BitcoinTestFramework):
         peer2 = node.add_p2p_connection(PeerTxRelayer())
 
         self.log.info("Test orphan handling when a nonsegwit parent is known to be invalid")
-        parent_low_fee_nonsegwit = self.wallet_nonsegwit.create_self_transfer(fee_rate=0)
+        parent_overly_large_nonsegwit = self.wallet_nonsegwit.create_self_transfer(target_vsize=int(MAX_STANDARD_TX_WEIGHT / 4) + 1)
-        assert_equal(parent_low_fee_nonsegwit["txid"], parent_low_fee_nonsegwit["tx"].wtxid_hex)
+        assert_equal(parent_overly_large_nonsegwit["txid"], parent_overly_large_nonsegwit["tx"].wtxid_hex)
         parent_other = self.wallet_nonsegwit.create_self_transfer()
         child_nonsegwit = self.wallet_nonsegwit.create_self_transfer_multi(
-            utxos_to_spend=[parent_other["new_utxo"], parent_low_fee_nonsegwit["new_utxo"]])
+            utxos_to_spend=[parent_other["new_utxo"], parent_overly_large_nonsegwit["new_utxo"]])
 
-        # Relay the parent. It should be rejected because it pays 0 fees.
+        # Relay the parent. It should be rejected (and not reconsiderable) because it violated size limitations.
-        self.relay_transaction(peer1, parent_low_fee_nonsegwit["tx"])
+        self.relay_transaction(peer1, parent_overly_large_nonsegwit["tx"])
-        assert parent_low_fee_nonsegwit["txid"] not in node.getrawmempool()
+        assert parent_overly_large_nonsegwit["txid"] not in node.getrawmempool()
 
         # Relay the child. It should not be accepted because it has missing inputs.
         # Its parent should not be requested because its hash (txid == wtxid) has been added to the rejection filter.
@@ -208,7 +209,7 @@ class OrphanHandlingTest(BitcoinTestFramework):
         self.nodes[0].bumpmocktime(GETDATA_TX_INTERVAL)
         peer1.assert_never_requested(int(parent_other["txid"], 16))
         peer2.assert_never_requested(int(parent_other["txid"], 16))
-        peer2.assert_never_requested(int(parent_low_fee_nonsegwit["txid"], 16))
+        peer2.assert_never_requested(int(parent_overly_large_nonsegwit["txid"], 16))
 
         self.log.info("Test orphan handling when a segwit parent was invalid but may be retried with another witness")
         parent_low_fee = self.wallet.create_self_transfer(fee_rate=0)
@@ -391,23 +392,23 @@ class OrphanHandlingTest(BitcoinTestFramework):
         peer3 = node.add_p2p_connection(PeerTxRelayer(wtxidrelay=False))
 
         self.log.info("Test that an orphan with rejected parents, along with any descendants, cannot be retried with an alternate witness")
-        parent_low_fee_nonsegwit = self.wallet_nonsegwit.create_self_transfer(fee_rate=0)
+        parent_overly_large_nonsegwit = self.wallet_nonsegwit.create_self_transfer(target_vsize=int(MAX_STANDARD_TX_WEIGHT / 4) + 1)
-        assert_equal(parent_low_fee_nonsegwit["txid"], parent_low_fee_nonsegwit["tx"].wtxid_hex)
+        assert_equal(parent_overly_large_nonsegwit["txid"], parent_overly_large_nonsegwit["tx"].wtxid_hex)
-        child = self.wallet.create_self_transfer(utxo_to_spend=parent_low_fee_nonsegwit["new_utxo"])
+        child = self.wallet.create_self_transfer(utxo_to_spend=parent_overly_large_nonsegwit["new_utxo"])
         grandchild = self.wallet.create_self_transfer(utxo_to_spend=child["new_utxo"])
         assert_not_equal(child["txid"], child["tx"].wtxid_hex)
         assert_not_equal(grandchild["txid"], grandchild["tx"].wtxid_hex)
 
         # Relay the parent. It should be rejected because it pays 0 fees.
-        self.relay_transaction(peer1, parent_low_fee_nonsegwit["tx"])
+        self.relay_transaction(peer1, parent_overly_large_nonsegwit["tx"])
-        assert parent_low_fee_nonsegwit["txid"] not in node.getrawmempool()
+        assert parent_overly_large_nonsegwit["txid"] not in node.getrawmempool()
 
         # Relay the child. It should be rejected for having missing parents, and this rejection is
         # cached by txid and wtxid.
         self.relay_transaction(peer1, child["tx"])
         assert_equal(0, len(node.getrawmempool()))
         assert not tx_in_orphanage(node, child["tx"])
-        peer1.assert_never_requested(parent_low_fee_nonsegwit["txid"])
+        peer1.assert_never_requested(parent_overly_large_nonsegwit["txid"])
 
         # Grandchild should also not be kept in orphanage because its parent has been rejected.
         self.relay_transaction(peer2, grandchild["tx"])