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

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

Pull request description:

  Motivated by the discussion in #28970 (https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1553911167), 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 60ca5d55081275a011ccfc9546e0c4a8c4030493 🍳
  glozow:
    code review ACK 60ca5d55081275a011ccfc9546e0c4a8c4030493
  instagibbs:
    ACK 60ca5d55081275a011ccfc9546e0c4a8c4030493

Tree-SHA512: 9cab43858e8f84db04a708151e6775c9cfc68c20ff53096220eac0b2c406f31aaf9223e8e04be345e95bf0a3f6dd15efac50b0ebeb1582a48a4560b3ab0bcba5

3 files changed, 48 insertions, 4 deletions

diff --git a/test/functional/p2p_tx_download.py b/test/functional/p2p_tx_download.py
index 0e463c5072..ae1edec2f1 100755
--- a/test/functional/p2p_tx_download.py
+++ b/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 @@ from test_framework.messages import (
     MSG_WTX,
     msg_inv,
     msg_notfound,
+    msg_tx,
 )
 from test_framework.p2p import (
     P2PInterface,
@@ -22,6 +24,7 @@ from test_framework.p2p import (
 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 @@ MAX_GETDATA_INBOUND_WAIT = GETDATA_TX_INTERVAL + INBOUND_PEER_TX_DELAY + TXID_RE
 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 @@ class TxDownloadTest(BitcoinTestFramework):
         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 @@ class TxDownloadTest(BitcoinTestFramework):
 
         # 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)
diff --git a/test/functional/rpc_packages.py b/test/functional/rpc_packages.py
index e061030da3..37c42f2533 100755
--- a/test/functional/rpc_packages.py
+++ b/test/functional/rpc_packages.py
@@ -386,12 +386,14 @@ class RPCPackagesTest(BitcoinTestFramework):
             "-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 @@ class RPCPackagesTest(BitcoinTestFramework):
 
         # 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 @@ class RPCPackagesTest(BitcoinTestFramework):
         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])
diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py
index dbaf42fdaa..0de09b6440 100644
--- a/test/functional/test_framework/util.py
+++ b/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