test: p2p: fix sending of manual INVs in tx download test

theStack commented at 3:06 am on January 15, 2025: contributor

The test_inv_block sub-test in p2p_tx_download.py has a subtle bug: the manual msg_inv announcements from peers currently have no effect, since they don’t match the wtxidrelay setting (=true by default for P2PInterface instances) and are hence ignored by the nodes (since 2d282e0c / PR #18044):

https://github.com/bitcoin/bitcoin/blob/e7c479495509c068215b73f6df070af2d406ae15/src/net_processing.cpp#L3904-L3911

Though the test still passes on master, it does so without the intended scenario of asking an additional peer (triggering the GETDATA_TX_INTERVAL delay). Fix this by sending the INV message with MSG_WTX instead of MSG_TX. This increases the test run time by about one minute intentionally.

It might be good to avoid issues like this in the future, happy to add test framework improvements if someone has a concrete idea.

(Got into the topic of tx/wtx announcements via the discussion #31397 (review))

test: p2p: fix sending of manual INVs in tx download test

The `test_inv_block` sub-test in p2p_tx_download.py has a subtle bug:
the manual msg_inv announcements from peers currently have no effect,
since they don't match the wtxidrelay setting (=true by default for
`P2PInterface` instances) and are hence ignored by the nodes (since
2d282e0c / PR #18044).  Though the test still passes, it does so without
the intended scenario of asking an additional peer (triggering the
GETDATA_TX_INTERVAL delay). Fix this by sending the INV message with
MSG_WTX instead of MSG_TX. This increases the test run time by about one
minute.

e0b3336822

DrahtBot commented at 3:06 am on January 15, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31658.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	mzumsande
Stale ACK	i-am-yuvi

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

DrahtBot added the label Tests on Jan 15, 2025

i-am-yuvi commented at 4:50 am on January 15, 2025: none

Concept ACK

Good catch @theStack!

mzumsande commented at 5:57 am on January 15, 2025: contributor

Concept ACK

Good find! I think that p2p_orphan_handling.py has a similar spot.

i-am-yuvi commented at 6:21 am on January 15, 2025: none

It could be good followup! Can you also change that as well @theStack

i-am-yuvi approved

i-am-yuvi commented at 6:22 am on January 15, 2025: none

Tested ACK e0b333682222927d64217b07bb8cfd7ff3139660

DrahtBot requested review from mzumsande on Jan 15, 2025

maflcko commented at 8:11 am on January 15, 2025: member

It might be good to avoid issues like this in the future, happy to add test framework improvements if someone has a concrete idea.

Would it not be possible to use mocktime and sync_with_ping?

theStack commented at 0:37 am on January 16, 2025: contributor

Good find! I think that p2p_orphan_handling.py has a similar spot.

Ah right, missed that one. If I interpret the test_orphan_inherit_rejection sub-test correctly, we want to send the INV with MSG_TX there to “announce again with potentially different witness”, so the solution in this case to avoid ignoring the INV would be to add peer3 with wtxidrelay=False (needs slight changes in the P2PTxInvStore and PeerTxRelayer constructors to make that possible though). Will address that tomorrow.

It might be good to avoid issues like this in the future, happy to add test framework improvements if someone has a concrete idea.

Would it not be possible to use mocktime and sync_with_ping?

Hm, that speeds up the test and can be done, but doesn’t prevent inv-tx-type/wtxidrelay mismatch and unintended INV message ignoring in the future (that’s what I meant with “issues like this”)?

maflcko commented at 9:33 am on January 16, 2025: member

Hm, that speeds up the test and can be done, but doesn’t prevent inv-tx-type/wtxidrelay mismatch and unintended INV message ignoring in the future (that’s what I meant with “issues like this”)?

mocktime should allow to check for the absence of the tx for the timeout period. An alternative would be to use ensure_for with a duration less than the timeout, if you want to keep the wall clock.

My understanding is that a change like this would make the test fail on master, and pass with the patch in the pull request as it is now. But I haven’t tried it, so I may be missing something?

mzumsande commented at 4:22 pm on January 16, 2025: contributor

My understanding is that a change like this would make the test fail on master, and pass with the patch in the pull request as it is now. But I haven’t tried it, so I may be missing something?

I suspect that theStack was looking for something that would prevent the underlying issue (sending wrong inv type) in general:

For example we could add an assert in send_message of p2p.py if we try to send an Inv of type MSG_TX and have self.wtxidrelay set (and vice versa). But it seems a bit crude to check that with every message, so I’m not seriously suggesting that.

I’m not sure if this can be done in an effective way - but I think it would be nice to add a simple test that actually sends the wrong inv type on purpose and checks that it gets ignored (no GETDATA).

glozow commented at 7:14 pm on January 16, 2025: member

Good find! I think that p2p_orphan_handling.py has a similar spot.

Ah right, missed that one. If I interpret the test_orphan_inherit_rejection sub-test correctly, we want to send the INV with MSG_TX there to “announce again with potentially different witness”, so the solution in this case to avoid ignoring the INV would be to add peer3 with wtxidrelay=False (needs slight changes in the P2PTxInvStore and PeerTxRelayer constructors to make that possible though). Will address that tomorrow.

Nice, @theStack @mzumsande! I went ahead and added this to #31666.

theStack commented at 0:16 am on January 17, 2025: contributor

My understanding is that a change like this would make the test fail on master, and pass with the patch in the pull request as it is now. But I haven’t tried it, so I may be missing something?

I suspect that theStack was looking for something that would prevent the underlying issue (sending wrong inv type) in general:

For example we could add an assert in send_message of p2p.py if we try to send an Inv of type MSG_TX and have self.wtxidrelay set (and vice versa). But it seems a bit crude to check that with every message, so I’m not seriously suggesting that.

Yes, the idea was a general prevention of inv-type/wtxidrelay mismatch. I also thought of introducing a special send_inv method to the P2PConnection class that would accept a CTransaction object and automatically pick MSG_TX or MSG_WTX (with txid or wtxid sent, accordingly) depending on how wtxidrelay was set. But then, there is no way to guarantee that test writers use that helper and looking at existing tests, it seems that very often we want to be specific about the INV message type.

I’m not sure if this can be done in an effective way - but I think it would be nice to add a simple test that actually sends the wrong inv type on purpose and checks that it gets ignored (no GETDATA).

Good idea, added a test in the second commit.

Nice, @theStack @mzumsande! I went ahead and added this to #31666.

Thanks 🙏

test: p2p: check that INV messages not matching wtxidrelay are ignored 8996fef8ae

theStack force-pushed on Jan 17, 2025

test: p2p: fix sending of manual INVs in tx download test #31658

Code Coverage & Benchmarks

Reviews