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

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

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

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

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.