Merge bitcoin#29827: test: p2p: add test for rejected tx request logic (m_recent_rejects filter)

60ca5d5 test: p2p: add test for rejected tx request logic (`m_recent_rejects` filter) (Sebastian Falbesoner)
e9dc511 fixup: get all utxos up front in fill_mempool, discourage wallet mixing (glozow)

Pull request description:

  Motivated by the discussion in bitcoin#28970 (bitcoin#28970 (comment)), this PR adds test coverage for the logic around the `m_recent_rejects` filter, in particular that the filter is cleared after a new block comes in:
  https://github.com/bitcoin/bitcoin/blob/f0794cbd405636a7f528a60f2873050b865cf7e8/src/net_processing.cpp#L2199-L2206

  As expected, the second part of the test fails if the following patch is applied:
  ```diff
  diff --git a/src/net_processing.cpp b/src/net_processing.cpp
  index 6996af38cb..5cb1090e70 100644
  --- a/src/net_processing.cpp
  +++ b/src/net_processing.cpp
  @@ -2202,7 +2202,7 @@ bool PeerManagerImpl::AlreadyHaveTx(const GenTxid& gtxid)
           // or a double-spend. Reset the rejects filter and give those
           // txs a second chance.
           hashRecentRejectsChainTip = m_chainman.ActiveChain().Tip()->GetBlockHash();
  -        m_recent_rejects.reset();
  +        //m_recent_rejects.reset();
       }

       const uint256& hash = gtxid.GetHash();
  ```
  I'm still not sure in which file this test fits best, and if there is already test coverage for the first part of the test somewhere. Happy for any suggestions.

ACKs for top commit:
  maflcko:
    ACK 60ca5d5 🍳
  glozow:
    code review ACK 60ca5d5
  instagibbs:
    ACK 60ca5d5

Tree-SHA512: 9cab43858e8f84db04a708151e6775c9cfc68c20ff53096220eac0b2c406f31aaf9223e8e04be345e95bf0a3f6dd15efac50b0ebeb1582a48a4560b3ab0bcba5

Author: glozow

test/functional/p2p_tx_download.py

@@ -5,6 +5,7 @@
 """
 Test transaction download behavior
 """
+from decimal import Decimal
 import time
 
 from test_framework.messages import (
@@ -14,6 +15,7 @@
     MSG_WTX,
     msg_inv,
     msg_notfound,
+    msg_tx,
 )
 from test_framework.p2p import (
     P2PInterface,
@@ -22,6 +24,7 @@
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
     assert_equal,
+    fill_mempool,
 )
 from test_framework.wallet import MiniWallet
 
@@ -54,6 +57,7 @@ def on_getdata(self, message):
 class TxDownloadTest(BitcoinTestFramework):
     def set_test_params(self):
         self.num_nodes = 2
+        self.extra_args= [['-datacarriersize=100000', '-maxmempool=5', '-persistmempool=0']] * self.num_nodes
 
     def test_tx_requests(self):
         self.log.info("Test that we request transactions from all our peers, eventually")
@@ -241,6 +245,29 @@ def test_spurious_notfound(self):
         self.log.info('Check that spurious notfound is ignored')
         self.nodes[0].p2ps[0].send_message(msg_notfound(vec=[CInv(MSG_TX, 1)]))
 
+    def test_rejects_filter_reset(self):
+        self.log.info('Check that rejected tx is not requested again')
+        node = self.nodes[0]
+        fill_mempool(self, node, self.wallet)
+        self.wallet.rescan_utxos()
+        mempoolminfee = node.getmempoolinfo()['mempoolminfee']
+        peer = node.add_p2p_connection(TestP2PConn())
+        low_fee_tx = self.wallet.create_self_transfer(fee_rate=Decimal("0.9")*mempoolminfee)
+        assert_equal(node.testmempoolaccept([low_fee_tx['hex']])[0]["reject-reason"], "mempool min fee not met")
+        peer.send_and_ping(msg_tx(low_fee_tx['tx']))
+        peer.send_and_ping(msg_inv([CInv(t=MSG_WTX, h=int(low_fee_tx['wtxid'], 16))]))
+        node.setmocktime(int(time.time()))
+        node.bumpmocktime(MAX_GETDATA_INBOUND_WAIT)
+        peer.sync_with_ping()
+        assert_equal(peer.tx_getdata_count, 0)
+
+        self.log.info('Check that rejection filter is cleared after new block comes in')
+        self.generate(self.wallet, 1, sync_fun=self.no_op)
+        peer.sync_with_ping()
+        peer.send_and_ping(msg_inv([CInv(t=MSG_WTX, h=int(low_fee_tx['wtxid'], 16))]))
+        node.bumpmocktime(MAX_GETDATA_INBOUND_WAIT)
+        peer.wait_for_getdata([int(low_fee_tx['wtxid'], 16)])
+
     def run_test(self):
         self.wallet = MiniWallet(self.nodes[0])
 
@@ -257,7 +284,8 @@ def run_test(self):
 
         # Run each test against new bitcoind instances, as setting mocktimes has long-term effects on when
         # the next trickle relay event happens.
-        for test in [self.test_in_flight_max, self.test_inv_block, self.test_tx_requests]:
+        for test in [self.test_in_flight_max, self.test_inv_block, self.test_tx_requests,
+                     self.test_rejects_filter_reset]:
             self.stop_nodes()
             self.start_nodes()
             self.connect_nodes(1, 0)

test/functional/rpc_packages.py

@@ -386,12 +386,14 @@ def test_maxfeerate_submitpackage(self):
             "-maxmempool=5",
             "-persistmempool=0",
         ])
+        self.wallet.rescan_utxos()
 
         fill_mempool(self, node, self.wallet)
 
         minrelay = node.getmempoolinfo()["minrelaytxfee"]
         parent = self.wallet.create_self_transfer(
             fee_rate=minrelay,
+            confirmed_only=True,
         )
 
         child = self.wallet.create_self_transfer(
@@ -411,6 +413,7 @@ def test_maxfeerate_submitpackage(self):
 
         # Reset maxmempool, datacarriersize, reset dynamic mempool minimum feerate, and empty mempool.
         self.restart_node(0)
+        self.wallet.rescan_utxos()
 
         assert_equal(node.getrawmempool(), [])
 
@@ -420,7 +423,10 @@ def test_maxburn_submitpackage(self):
         assert_equal(node.getrawmempool(), [])
 
         self.log.info("Submitpackage maxburnamount arg testing")
-        chained_txns_burn = self.wallet.create_self_transfer_chain(chain_length=2)
+        chained_txns_burn = self.wallet.create_self_transfer_chain(
+            chain_length=2,
+            utxo_to_spend=self.wallet.get_utxo(confirmed_only=True),
+        )
         chained_burn_hex = [t["hex"] for t in chained_txns_burn]
 
         tx = tx_from_hex(chained_burn_hex[1])

test/functional/test_framework/util.py

@@ -505,6 +505,8 @@ def fill_mempool(test_framework, node, miniwallet):
     It will not ensure mempools become synced as it
     is based on a single node and assumes -minrelaytxfee
     is 1 sat/vbyte.
+    To avoid unintentional tx dependencies, it is recommended to use separate miniwallets for
+    mempool filling vs transactions in tests.
     """
     test_framework.log.info("Fill the mempool until eviction is triggered and the mempoolminfee rises")
     txouts = gen_return_txouts()
@@ -522,8 +524,14 @@ def fill_mempool(test_framework, node, miniwallet):
     # Mine COINBASE_MATURITY - 1 blocks so that the UTXOs are allowed to be spent
     test_framework.generate(node, 100 - 1)
 
+    # Get all UTXOs up front to ensure none of the transactions spend from each other, as that may
+    # change their effective feerate and thus the order in which they are selected for eviction.
+    confirmed_utxos = [miniwallet.get_utxo(confirmed_only=True) for _ in range(num_of_batches * tx_batch_size + 1)]
+    assert_equal(len(confirmed_utxos), num_of_batches * tx_batch_size + 1)
+
     test_framework.log.debug("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_id = miniwallet.send_self_transfer(from_node=node, utxo_to_spend=confirmed_utxos[0], fee_rate=relayfee)["txid"]
+    del confirmed_utxos[0]
 
     # 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)
@@ -534,7 +542,9 @@ def fill_mempool(test_framework, node, miniwallet):
     with node.assert_debug_log(["rolling minimum fee bumped"]):
         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)
+            utxos = confirmed_utxos[:tx_batch_size]
+            create_lots_of_big_transactions(miniwallet, node, fee, tx_batch_size, txouts, utxos)
+            del confirmed_utxos[:tx_batch_size]
 
     test_framework.log.debug("The tx should be evicted by now")
     # The number of transactions created should be greater than the ones present in the mempool