mirrors/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-02-22 06:52:36 +01:00
Code Issues Projects Releases Wiki Activity

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

Browse source 
60ca5d5508 test: p2p: add test for rejected tx request logic (`m_recent_rejects` filter) (Sebastian Falbesoner)
e9dc511a7e 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:
  f0794cbd40/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 60ca5d5508 🍳
  glozow:
    code review ACK 60ca5d5508
  instagibbs:
    ACK 60ca5d5508

Tree-SHA512: 9cab43858e8f84db04a708151e6775c9cfc68c20ff53096220eac0b2c406f31aaf9223e8e04be345e95bf0a3f6dd15efac50b0ebeb1582a48a4560b3ab0bcba5
This commit is contained in:
glozow 2024-04-19 16:13:04 +01:00
commit 67c0d93982
No known key found for this signature in database
GPG key ID: BA03F4DBE0C63FB4
3 changed files with 48 additions and 4 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

30
test/functional/p2p_tx_download.py
View file

@ -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)

8
test/functional/rpc_packages.py
View file

@ -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])

14
test/functional/test_framework/util.py
View file

@ -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