Skip to content

test: Extends MEMPOOL msg functional test #28485

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
fanquake merged 1 commit into from
Dec 8, 2023
Merged

test: Extends MEMPOOL msg functional test #28485

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions test/functional/p2p_filter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
COIN,
MAX_BLOOM_FILTER_SIZE,
MAX_BLOOM_HASH_FUNCS,
MSG_WTX,
MSG_BLOCK,
MSG_FILTERED_BLOCK,
msg_filteradd,
Expand Down Expand Up @@ -135,14 +136,22 @@ def test_msg_mempool(self):
self.log.info("Check that a node with bloom filters enabled services p2p mempool messages")
filter_peer = P2PBloomFilter()

self.log.debug("Create a tx relevant to the peer before connecting")
txid = self.wallet.send_to(from_node=self.nodes[0], scriptPubKey=filter_peer.watch_script_pubkey, amount=9 * COIN)["txid"]
self.log.info("Create two tx before connecting, one relevant to the node another that is not")
rel_txid = self.wallet.send_to(from_node=self.nodes[0], scriptPubKey=filter_peer.watch_script_pubkey, amount=1 * COIN)["txid"]
irr_result = self.wallet.send_to(from_node=self.nodes[0], scriptPubKey=getnewdestination()[1], amount=2 * COIN)
irr_txid = irr_result["txid"]
irr_wtxid = irr_result["wtxid"]

self.log.debug("Send a mempool msg after connecting and check that the tx is received")
self.log.info("Send a mempool msg after connecting and check that the relevant tx is announced")
self.nodes[0].add_p2p_connection(filter_peer)
filter_peer.send_and_ping(filter_peer.watch_filter_init)
filter_peer.send_message(msg_mempool())
filter_peer.wait_for_tx(txid)
filter_peer.wait_for_tx(rel_txid)
Comment on lines 148 to +149
Copy link
Member

@instagibbs instagibbs Dec 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I comment out these lines the test still passes. Seems wrong based on text below at https://github.com/bitcoin/bitcoin/pull/28485/files#diff-c5c320cd909288d7cf2d82632c6bafcb9085413bfddf5edf361f288dbd76e0cbR153 ?

Based on my look at the code, in SendMessages we set m_last_inv_sequence = m_mempool.GetSequence() immediately on connection, even if we had no INVs to send. This means new connections can ask for anything historical, regardless of mempool message?

Copy link
Member Author

@sr-gi sr-gi Dec 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is related to this? #28485 (comment)

Copy link
Member

@instagibbs instagibbs Dec 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok so we agree, which makes the test a bit odd imo. The MEMPOOL message being sent or not has no relation on whether we respond to that particular getdata from the peer.

Copy link
Member Author

@sr-gi sr-gi Dec 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I don't think this is inherently wrong. The irrelevant transaction is requestable (independently of the mempool message) because it is in the mempool and would have been announced to the peer if it happened to be interested in it (which is not the case, based on the filter).

Sending the mempool message and checking is done to make sure only the relevant transaction is annonced. However, we are still able to request the irrelevant.


self.log.info("Request the irrelevant transaction even though it was not announced")
filter_peer.send_message(msg_getdata([CInv(t=MSG_WTX, h=int(irr_wtxid, 16))]))
self.log.info("We should get it anyway because it was in the mempool on connection to peer")
filter_peer.wait_for_tx(irr_txid)

def test_frelay_false(self, filter_peer):
self.log.info("Check that a node with fRelay set to false does not receive invs until the filter is set")
Expand Down