test: Extends MEMPOOL msg functional test #28485

sr-gi · 2023-09-14T20:47:30Z

Currently, p2p_filter.py::test_msg_mempool is not testing much. This extends the tests so the interaction between sending MEMPOOL messages with a filter that does not include all transactions in the mempool reacts, plus how it interacts with INV messages, especially after the changes introduced by #27675

DrahtBot · 2023-09-14T20:47:33Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	instagibbs, theStack
Stale ACK	glozow, maflcko

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

maflcko · 2023-09-15T10:06:17Z

test/functional/p2p_filter.py

+        filter_peer.tx_received = False
+        add_txid = self.wallet.send_to(from_node=self.nodes[0], scriptPubKey=getnewdestination()[1], amount=3 * COIN)["txid"]
+        filter_peer.send_and_ping(msg_getdata([CInv(t=MSG_WTX, h=int(self.nodes[0].getmempoolentry(add_txid)["wtxid"], 16))]))
+        self.log.info("We won't be able to request this one given and INV was not sent since it entered the mempool")


I wonder why, given that this test uses immediate tx-relay. Shouldn't this check fail?

Maybe there hasn't been a SendMessages yet?

IIRC send_and_ping should force a SendMessages now (on current master)

I may be missing the point here, but why should it fail? Even if send_and_ping forces SendMessages, aren't invs still delayed?

If the following is added at the end of the test, you can see how the transaction is served:

filter_peer.send_and_ping(msg_inv([CInv(t=MSG_WTX, h=int(self.nodes[0].getmempoolentry(add_txid)["wtxid"], 16))])) filter_peer.send_and_ping(msg_getdata([CInv(t=MSG_WTX, h=int(self.nodes[0].getmempoolentry(add_txid)["wtxid"], 16))])) assert filter_peer.tx_received

You can add a import time;time.sleep(.1) after the wallet.send_to to observe that the test fails.

Should we be using send_message instead so SendMessages is not forced, or just get rid of this third tx request?

FWIW, adding a sleep after send_message will make it fail too

Tests must pass in all environments. If you rely on a race condition for the test to pass, it will fail eventually.

Sure, my point was whether the last check was even worth it. I finally figured out what was going on (h/t to @glozow for being my rubber duckie):

It turns out m_last_inv_sequence is not updated every single time an INV is sent, but every single time an INV may be sent. As long as the node trickles for that peer, m_last_inv_sequence is increased, hence why the transaction was requestable even though no INV was being sent.

Therefore, by using sync_and_ping, which forces SendMessages, we can assert that the transaction is requestable.

c9e31a4 should have fixed this

test/functional/p2p_filter.py

glozow

ACK d10acba, with some nits

test/functional/p2p_filter.py

sr-gi · 2023-10-11T16:04:45Z

Addressed nits

glozow

reACK fb288f3, only diff is the 2 nits on logging

theStack

ACK fb288f3

theStack · 2023-12-05T14:44:18Z

test/functional/p2p_filter.py

+        irr_txid = self.wallet.send_to(from_node=self.nodes[0], scriptPubKey=getnewdestination()[1], amount=2 * COIN)["txid"]
+        irr_wtxid = self.nodes[0].getmempoolentry(irr_txid)["wtxid"]


nit (feel free to ignore): MiniWallet's send_to already returns both txid and wtxid, so one doesn't need the extra getmempoolentry RPC call.

I'm taking this

sr-gi · 2023-12-05T15:30:01Z

Addressed comments. Should be re-review ready

maflcko

lgtm ACK 3f2da64

maflcko · 2023-12-05T15:43:05Z

test/functional/p2p_filter.py

+        filter_peer.wait_for_tx(rel_txid)
+
+        self.log.info("Request the irrelevant transaction even though it was not announced")
+        filter_peer.tx_received = False


nit: Can remove this line, now that you use wait_for_tx ?

maflcko · 2023-12-05T15:43:40Z

test/functional/p2p_filter.py

-        filter_peer.send_message(msg_mempool())
-        filter_peer.wait_for_tx(txid)
+        filter_peer.send_message(filter_peer.watch_filter_init)
+        filter_peer.send_and_ping(msg_mempool())


nit: Is this change from send_message(msg_mempool()) to send_and_ping(msg_mempool()) required?

I don't think it is. Did a bunch of testing locally and it may be a remnant of the old approach taken for this test, which does not longer apply.

I've reversed some of the calls to make the diff as minimal as possible

maflcko · 2023-12-05T16:38:19Z

lgtm ACK 59e86af

theStack

re-ACK 59e86af

instagibbs

I haven't done much review of #27675 , but I can't make the test case fail sensibly by commenting out the mempool message, and by the reading of the code.

instagibbs · 2023-12-06T15:07:53Z

test/functional/p2p_filter.py

        filter_peer.send_message(msg_mempool())
-        filter_peer.wait_for_tx(txid)
+        filter_peer.wait_for_tx(rel_txid)


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?

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

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.

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.

test/functional/p2p_filter.py

Currently, p2p_filter.py::test_msg_mempool is not testing much. This extends the tests so the interaction between sending MEMPOOL messages with a filter that does not include all transactions in the mempool reacts, plus how it interacts with INV messages

instagibbs · 2023-12-06T18:23:50Z

ACK 97c0dfa

tests line up with my understanding of the code

theStack

re-ACK 97c0dfa

DrahtBot added the Tests label Sep 14, 2023

sr-gi force-pushed the 2023-09-mempool-msg-test branch from 1e999b4 to eb4b020 Compare September 14, 2023 21:09

DrahtBot added CI failed and removed CI failed labels Sep 14, 2023

maflcko reviewed Sep 15, 2023

View reviewed changes

sr-gi force-pushed the 2023-09-mempool-msg-test branch from eb4b020 to c9e31a4 Compare September 19, 2023 17:41

sr-gi requested review from glozow and maflcko October 4, 2023 17:42

glozow reviewed Oct 5, 2023

View reviewed changes

test/functional/p2p_filter.py Outdated Show resolved Hide resolved

test/functional/p2p_filter.py Outdated Show resolved Hide resolved

test/functional/p2p_filter.py Outdated Show resolved Hide resolved

sr-gi force-pushed the 2023-09-mempool-msg-test branch from c9e31a4 to d10acba Compare October 7, 2023 03:46

DrahtBot added the CI failed label Oct 7, 2023

glozow reviewed Oct 11, 2023

View reviewed changes

test/functional/p2p_filter.py Outdated Show resolved Hide resolved

test/functional/p2p_filter.py Outdated Show resolved Hide resolved

sr-gi force-pushed the 2023-09-mempool-msg-test branch from d10acba to fb288f3 Compare October 11, 2023 16:04

DrahtBot removed the CI failed label Oct 11, 2023

glozow reviewed Oct 13, 2023

View reviewed changes

theStack approved these changes Dec 5, 2023

View reviewed changes

sr-gi force-pushed the 2023-09-mempool-msg-test branch from fb288f3 to 3f2da64 Compare December 5, 2023 15:28

maflcko reviewed Dec 5, 2023

View reviewed changes

DrahtBot requested review from glozow and theStack December 5, 2023 15:44

sr-gi force-pushed the 2023-09-mempool-msg-test branch from 3f2da64 to 59e86af Compare December 5, 2023 16:32

theStack approved these changes Dec 5, 2023

View reviewed changes

fanquake assigned glozow Dec 6, 2023

instagibbs reviewed Dec 6, 2023

View reviewed changes

test/functional/p2p_filter.py Outdated Show resolved Hide resolved

sr-gi force-pushed the 2023-09-mempool-msg-test branch from 59e86af to 97c0dfa Compare December 6, 2023 18:22

DrahtBot requested review from theStack and maflcko December 6, 2023 18:23

theStack approved these changes Dec 8, 2023

View reviewed changes

fanquake merged commit 1f352cf into bitcoin:master Dec 8, 2023

bitcoin locked and limited conversation to collaborators Dec 7, 2024

		irr_txid = self.wallet.send_to(from_node=self.nodes[0], scriptPubKey=getnewdestination()[1], amount=2 * COIN)["txid"]
		irr_wtxid = self.nodes[0].getmempoolentry(irr_txid)["wtxid"]

test: Extends MEMPOOL msg functional test #28485

test: Extends MEMPOOL msg functional test #28485

Uh oh!

Conversation

sr-gi commented Sep 14, 2023

Uh oh!

DrahtBot commented Sep 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sr-gi Sep 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sr-gi Sep 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sr-gi commented Oct 11, 2023

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sr-gi commented Dec 5, 2023

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Dec 5, 2023

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

instagibbs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Sep 14, 2023 •

edited

Loading

sr-gi Sep 15, 2023 •

edited

Loading

sr-gi Sep 18, 2023 •

edited

Loading