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

pull theStack wants to merge 2 commits into bitcoin:master from theStack:202501-test-p2p-fix_inv_block_subtest changing 1 files +33 −2
  1. 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))

  2. 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
  3. 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
    ACK danielabrozzoni, mzumsande, maflcko
    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.

  4. DrahtBot added the label Tests on Jan 15, 2025
  5. i-am-yuvi commented at 4:50 am on January 15, 2025: none

    Concept ACK

    Good catch @theStack!

  6. 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.

  7. 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
  8. i-am-yuvi approved
  9. i-am-yuvi commented at 6:22 am on January 15, 2025: none
    Tested ACK e0b333682222927d64217b07bb8cfd7ff3139660
  10. DrahtBot requested review from mzumsande on Jan 15, 2025
  11. 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?

  12. 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”)?

  13. 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?

  14. 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).

  15. 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.

  16. 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 🙏

  17. test: p2p: check that INV messages not matching wtxidrelay are ignored 8996fef8ae
  18. theStack force-pushed on Jan 17, 2025
  19. danielabrozzoni approved
  20. danielabrozzoni commented at 11:05 pm on January 22, 2025: contributor

    ACK 8996fef8aebd99aa9e1dec12b0087d79c0993a84

    I manually verified that before this change the nodes would ignore the inv and wouldn’t send a getdata by adding this line:

     0diff --git a/test/functional/p2p_tx_download.py b/test/functional/p2p_tx_download.py
 1index b1dad1d850..e737ea7320 100755
 2--- a/test/functional/p2p_tx_download.py
 3+++ b/test/functional/p2p_tx_download.py
 4@@ -100,6 +100,7 @@ class TxDownloadTest(BitcoinTestFramework):
 5         msg = msg_inv([CInv(t=MSG_WTX, h=wtxid)])
 6         for p in self.peers:
 7             p.send_and_ping(msg)
 8+        self.wait_until(lambda: any(p.tx_getdata_count > 0 for p in self.peers))
 9 
10         self.log.info("Put the tx in node 0's mempool")
11         self.nodes[0].sendrawtransaction(tx['hex'])

    This makes the test fail on master, while it passes on this branch.

  21. DrahtBot requested review from i-am-yuvi on Jan 22, 2025
  22. maflcko commented at 8:43 am on January 23, 2025: member

    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.

    Would there be a use-case for this? My understanding is that any type mismatch in “normal” tests (tests that expect a getdata after inv) would be equivalent to having not sent the inv at all, so they’d already be failing, similar to how they’d fail if the inv sending was removed completely, no?

    So this leaves the “non-normal” tests (tests that check for the absence of a getdata after inv), which seem small enough in the number to add the ensure_for/mocktime on a case-by-case basis to catch future regressions?

  23. theStack commented at 2:00 pm on January 23, 2025: contributor
    @maflcko: For both of the INV mismatch cases identified so far (i.e. the one fixed in this PR and the one in test_orphan_handling.py, fixed in #31666 / https://github.com/bitcoin/bitcoin/pull/31666/commits/3cd6c46a71e162343048ddeca648ea8ed7fe8a83), I don’t see what we could add to reliably catch future regressions. I could add the check #31658#pullrequestreview-2568472848 if wanted, but it’s probably confusing test readers, considering that not all peers get a GETDATA answer (note that we have 10 peers here)? Happy to add suggested patches, maybe I was thinking in the wrong direction so far.
  24. mzumsande commented at 10:55 pm on January 23, 2025: contributor
    Code Review ACK 8996fef8aebd99aa9e1dec12b0087d79c0993a84
  25. maflcko commented at 10:10 am on January 24, 2025: member

    but it’s probably confusing test readers, considering that not all peers get a GETDATA answer (note that we have 10 peers here)? Happy to add suggested patches, maybe I was thinking in the wrong direction so far.

    Correct, only one inbound should receive a getdata, because after the expiry the preferred peer (outbound) will be preferred. So I think the patch by Daniela is fine and not confusing. Also, I think checking that the expiry delay isn’t zero doesn’t seem confusing either. Example diff, which reliably fails on master and passes on this pull:

     0diff --git a/test/functional/p2p_tx_download.py b/test/functional/p2p_tx_download.py
 1index c69d6ff405..e85714d031 100755
 2--- a/test/functional/p2p_tx_download.py
 3+++ b/test/functional/p2p_tx_download.py
 4@@ -27,6 +27,7 @@ from test_framework.p2p import (
 5 from test_framework.test_framework import BitcoinTestFramework
 6 from test_framework.util import (
 7     assert_equal,
 8+    ensure_for,
 9 )
10 from test_framework.wallet import MiniWallet
11 
12@@ -102,7 +103,11 @@ class TxDownloadTest(BitcoinTestFramework):
13             p.send_and_ping(msg)
14 
15         self.log.info("Put the tx in node 0's mempool")
16-        self.nodes[0].sendrawtransaction(tx['hex'])
17+        txid = self.nodes[0].sendrawtransaction(tx['hex'])
18+
19+        self.log.info("Check that the request does not expire immediately")
20+        # Use half the expiry to avoid intermittent test failures on extremely slow machines
21+        ensure_for(duration=GETDATA_TX_INTERVAL / 2, f=lambda: txid not in self.nodes[1].getrawmempool())
22 
23         # Since node 1 is connected outbound to an honest peer (node 0), it
24         # should get the tx within a timeout. (Assuming that node 0
25@@ -110,12 +115,13 @@ class TxDownloadTest(BitcoinTestFramework):
26         # The timeout is the sum of
27         # * the worst case until the tx is first requested from an inbound
28         #   peer, plus
29-        # * the first time it is re-requested from the outbound peer, plus
30+        # * the first time it is re-requested from the outbound peer (after expiry), plus
31         # * 2 seconds to avoid races
32         assert self.nodes[1].getpeerinfo()[0]['inbound'] == False
33         timeout = 2 + INBOUND_PEER_TX_DELAY + GETDATA_TX_INTERVAL
34         self.log.info("Tx should be received at node 1 after {} seconds".format(timeout))
35         self.sync_mempools(timeout=timeout)
36+        assert txid in self.nodes[1].getrawmempool()
37 
38     def test_in_flight_max(self):
39         self.log.info("Test that we don't load peers with more than {} transaction requests immediately".format(MAX_GETDATA_IN_FLIGHT))

    Happy to re-write it in mocktime, just let me know.

    But not a blocker, the changes here are fine either way.

  26. maflcko commented at 10:10 am on January 24, 2025: member

    ACK 8996fef8aebd99aa9e1dec12b0087d79c0993a84 😸

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: ACK 8996fef8aebd99aa9e1dec12b0087d79c0993a84 😸
39g6SgXJls/NOHpo4qo6vhFou1WtpZ6LGXBk7SvT8WaDju/w5rCnBvCwYWikLdlaD2aaTUwaqz+DD5DjyCw1CAw==
  27. fanquake merged this on Jan 24, 2025
  28. fanquake closed this on Jan 24, 2025
  29. theStack deleted the branch on Jan 24, 2025
  30. theStack commented at 12:41 pm on January 24, 2025: contributor

    @maflcko: Thanks for elaborating!

    Happy to re-write it in mocktime, just let me know.

    That would be nice, also considering that the test_inv_block sub-test currently takes a significant amount of the total p2p_tx_download.py run-time (~70% on my machine, 65s/94s).


theStack DrahtBot i-am-yuvi mzumsande maflcko glozow danielabrozzoni


i-am-yuvi

Labels
Tests

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-02-22 06:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me