test: Extends MEMPOOL msg functional test #28485

pull sr-gi wants to merge 1 commits into bitcoin:master from sr-gi:2023-09-mempool-msg-test changing 1 files +13 −4
  1. sr-gi commented at 8:47 PM on September 14, 2023: member

    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

  2. DrahtBot commented at 8:47 PM on September 14, 2023: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage

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

    <!--021abf342d371248e50ceaed478a90ca-->

    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.

  3. DrahtBot added the label Tests on Sep 14, 2023
  4. sr-gi force-pushed on Sep 14, 2023
  5. DrahtBot added the label CI failed on Sep 14, 2023
  6. DrahtBot removed the label CI failed on Sep 15, 2023
  7. in test/functional/p2p_filter.py:160 in eb4b020e3d outdated 
     161 | +
 162 | +        self.log.info("Create a third transaction (that was therefore not part of the mempool message) and try to retrieve it")
 163 | +        filter_peer.tx_received = False
 164 | +        add_txid = self.wallet.send_to(from_node=self.nodes[0], scriptPubKey=getnewdestination()[1], amount=3 * COIN)["txid"]
 165 | +        filter_peer.send_and_ping(msg_getdata([CInv(t=MSG_WTX, h=int(self.nodes[0].getmempoolentry(add_txid)["wtxid"], 16))]))
 166 | +        self.log.info("We won't be able to request this one given and INV was not sent since it entered the mempool")
    maflcko commented at 10:06 AM on September 15, 2023:

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

    glozow commented at 10:22 AM on September 15, 2023:

    Maybe there hasn't been a SendMessages yet?

    maflcko commented at 10:26 AM on September 15, 2023:

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

    sr-gi commented at 2:06 PM on September 15, 2023:

    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
    maflcko commented at 2:23 PM on September 15, 2023:

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

    sr-gi commented at 2:48 PM on September 15, 2023:

    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

    maflcko commented at 3:17 PM on September 15, 2023:

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

    sr-gi commented at 4:45 PM on September 18, 2023:

    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.

    sr-gi commented at 5:42 PM on September 19, 2023:

    c9e31a4 should have fixed this

  8. sr-gi force-pushed on Sep 19, 2023
  9. sr-gi requested review from glozow on Oct 4, 2023
  10. sr-gi requested review from maflcko on Oct 4, 2023
  11. in test/functional/p2p_filter.py:153 in c9e31a40d2 outdated 
     154 | +        filter_peer.wait_for_tx(rel_txid)
 155 | +
 156 | +        self.log.info("Request the irrelevant transaction even though it has not being offered to us")
 157 | +        filter_peer.tx_received = False
 158 | +        filter_peer.send_and_ping(msg_getdata([CInv(t=MSG_WTX, h=int(irr_wtxid, 16))]))
 159 | +        self.log.info("We should get it anyway given it was on the mempool when the MEMPOOL message was sent")
    glozow commented at 11:49 AM on October 5, 2023: 
            self.log.info("We should get it anyway because it was in the mempool when the MEMPOOL message was sent")
  12. in test/functional/p2p_filter.py:154 in c9e31a40d2 outdated 
     155 | +
 156 | +        self.log.info("Request the irrelevant transaction even though it has not being offered to us")
 157 | +        filter_peer.tx_received = False
 158 | +        filter_peer.send_and_ping(msg_getdata([CInv(t=MSG_WTX, h=int(irr_wtxid, 16))]))
 159 | +        self.log.info("We should get it anyway given it was on the mempool when the MEMPOOL message was sent")
 160 | +        assert filter_peer.tx_received
    glozow commented at 11:54 AM on October 5, 2023:

    instead of using tx_received, wait_for_tx will also assert that you're receiving the right tx:

            filter_peer.wait_for_tx(irr_txid)
  13. in test/functional/p2p_filter.py:163 in c9e31a40d2 outdated 
     164 | +        add_txid = self.wallet.send_to(from_node=self.nodes[0], scriptPubKey=getnewdestination()[1], amount=3 * COIN)["txid"]
 165 | +        # sync_with_ping forces the node to trickle, which increases m_last_inv_sequence
 166 | +        filter_peer.sync_with_ping()
 167 | +        filter_peer.send_and_ping(msg_getdata([CInv(t=MSG_WTX, h=int(self.nodes[0].getmempoolentry(add_txid)["wtxid"], 16))]))
 168 | +        self.log.info("We should be able to request this one given too since we force the node to trickle")
 169 | +        assert filter_peer.tx_received
    glozow commented at 11:56 AM on October 5, 2023:

    What's the purpose of this test? It seems like we're just testing that we don't delay announcements to peers with noban permissions. That doesn't have anything to do with bloom filters or the mempool message.

    Assuming the original intent was to check that they won't be able to request the tx before announcement, you could remove noban permissions and assert the opposite, but I think that's covered in other tests already.

    sr-gi commented at 7:19 PM on October 5, 2023:

    I guess we could get rid of this bit.

    The purpose was really to test how the transaction won't be available until the node has trickled for the requesting peer, but that is not something we can check without introducing a time-variant to the test that will make it fail sparsely.

  14. sr-gi force-pushed on Oct 7, 2023
  15. DrahtBot added the label CI failed on Oct 7, 2023
  16. in test/functional/p2p_filter.py:144 in d10acba3d7 outdated 
     142 | +        rel_txid = self.wallet.send_to(from_node=self.nodes[0], scriptPubKey=filter_peer.watch_script_pubkey, amount=1 * COIN)["txid"]
 143 | +        irr_txid = self.wallet.send_to(from_node=self.nodes[0], scriptPubKey=getnewdestination()[1], amount=2 * COIN)["txid"]
 144 | +        irr_wtxid = self.nodes[0].getmempoolentry(irr_txid)["wtxid"]
 145 |  
 146 | -        self.log.debug("Send a mempool msg after connecting and check that the tx is received")
 147 | +        self.log.info("Send a mempool msg after connecting and check that the relevant tx is received")
    glozow commented at 8:57 AM on October 11, 2023:

    nit: in the context of tx relay I typically prefer to use more specific words to disambiguate between getting invs and transaction data

            self.log.info("Send a mempool msg after connecting and check that the relevant tx is announced")
  17. in test/functional/p2p_filter.py:150 in d10acba3d7 outdated 
     151 | -        filter_peer.wait_for_tx(txid)
 152 | +        filter_peer.send_message(filter_peer.watch_filter_init)
 153 | +        filter_peer.send_and_ping(msg_mempool())
 154 | +        filter_peer.wait_for_tx(rel_txid)
 155 | +
 156 | +        self.log.info("Request the irrelevant transaction even though it has not being offered to us")
    glozow commented at 8:58 AM on October 11, 2023:

    nit: grammar and I think "announced" is more fitting than "offered"

            self.log.info("Request the irrelevant transaction even though it was not announced")
  18. glozow commented at 8:59 AM on October 11, 2023: member

    ACK d10acba3d730397484f10ccb2f1116544161507d, with some nits

  19. sr-gi force-pushed on Oct 11, 2023
  20. sr-gi commented at 4:04 PM on October 11, 2023: member

    Addressed nits

  21. DrahtBot removed the label CI failed on Oct 11, 2023
  22. glozow commented at 1:38 PM on October 13, 2023: member

    reACK fb288f331277536c8b528c0eae44d81617fcbee6, only diff is the 2 nits on logging

  23. in test/functional/p2p_filter.py:142 in fb288f3312 outdated 
     135 | @@ -135,14 +136,22 @@ def test_msg_mempool(self):
 136 |          self.log.info("Check that a node with bloom filters enabled services p2p mempool messages")
 137 |          filter_peer = P2PBloomFilter()
 138 |  
 139 | -        self.log.debug("Create a tx relevant to the peer before connecting")
 140 | -        txid = self.wallet.send_to(from_node=self.nodes[0], scriptPubKey=filter_peer.watch_script_pubkey, amount=9 * COIN)["txid"]
 141 | +        self.log.info("Create two tx before connecting, one relevant to the node another that is not")
 142 | +        rel_txid = self.wallet.send_to(from_node=self.nodes[0], scriptPubKey=filter_peer.watch_script_pubkey, amount=1 * COIN)["txid"]
 143 | +        irr_txid = self.wallet.send_to(from_node=self.nodes[0], scriptPubKey=getnewdestination()[1], amount=2 * COIN)["txid"]
 144 | +        irr_wtxid = self.nodes[0].getmempoolentry(irr_txid)["wtxid"]
    theStack commented at 2:44 PM on December 5, 2023:

    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.

    sr-gi commented at 3:28 PM on December 5, 2023:

    I'm taking this

  24. theStack approved
  25. theStack commented at 2:44 PM on December 5, 2023: contributor

    ACK fb288f331277536c8b528c0eae44d81617fcbee6

  26. sr-gi force-pushed on Dec 5, 2023
  27. sr-gi commented at 3:30 PM on December 5, 2023: member

    Addressed comments. Should be re-review ready

  28. in test/functional/p2p_filter.py:152 in 3f2da64a45 outdated 
     153 | +        filter_peer.send_message(filter_peer.watch_filter_init)
 154 | +        filter_peer.send_and_ping(msg_mempool())
 155 | +        filter_peer.wait_for_tx(rel_txid)
 156 | +
 157 | +        self.log.info("Request the irrelevant transaction even though it was not announced")
 158 | +        filter_peer.tx_received = False
    maflcko commented at 3:43 PM on December 5, 2023:

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

  29. in test/functional/p2p_filter.py:148 in 3f2da64a45 outdated 
     149 |          self.nodes[0].add_p2p_connection(filter_peer)
 150 | -        filter_peer.send_and_ping(filter_peer.watch_filter_init)
 151 | -        filter_peer.send_message(msg_mempool())
 152 | -        filter_peer.wait_for_tx(txid)
 153 | +        filter_peer.send_message(filter_peer.watch_filter_init)
 154 | +        filter_peer.send_and_ping(msg_mempool())
    maflcko commented at 3:43 PM on December 5, 2023:

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

    sr-gi commented at 4:34 PM on December 5, 2023:

    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

  30. maflcko commented at 3:44 PM on December 5, 2023: member

    lgtm ACK 3f2da64a45f79033c29d08ee24147ca124a26c22

  31. DrahtBot requested review from glozow on Dec 5, 2023
  32. DrahtBot requested review from theStack on Dec 5, 2023
  33. sr-gi force-pushed on Dec 5, 2023
  34. maflcko commented at 4:38 PM on December 5, 2023: member

    lgtm ACK 59e86afbcdfb9dbf52a6580246233ee501c51475

  35. theStack approved
  36. theStack commented at 5:26 PM on December 5, 2023: contributor

    re-ACK 59e86afbcdfb9dbf52a6580246233ee501c51475

  37. fanquake assigned glozow on Dec 6, 2023
  38. in test/functional/p2p_filter.py:149 in 59e86afbcd outdated 
     148 | +        self.log.info("Send a mempool msg after connecting and check that the relevant tx is announced")
 149 |          self.nodes[0].add_p2p_connection(filter_peer)
 150 |          filter_peer.send_and_ping(filter_peer.watch_filter_init)
 151 |          filter_peer.send_message(msg_mempool())
 152 | -        filter_peer.wait_for_tx(txid)
 153 | +        filter_peer.wait_for_tx(rel_txid)
    instagibbs commented at 3:07 PM on December 6, 2023:

    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?

    sr-gi commented at 5:19 PM on December 6, 2023:

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

    instagibbs commented at 5:23 PM on December 6, 2023:

    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.

    sr-gi commented at 5:23 PM on December 6, 2023:

    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.

  39. instagibbs commented at 3:32 PM on December 6, 2023: member

    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.

  40. test: Extends MEMPOOL msg functional test
    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
    97c0dfa894
  41. in test/functional/p2p_filter.py:153 in 59e86afbcd outdated 
     152 | -        filter_peer.wait_for_tx(txid)
 153 | +        filter_peer.wait_for_tx(rel_txid)
 154 | +
 155 | +        self.log.info("Request the irrelevant transaction even though it was not announced")
 156 | +        filter_peer.send_message(msg_getdata([CInv(t=MSG_WTX, h=int(irr_wtxid, 16))]))
 157 | +        self.log.info("We should get it anyway because it was in the mempool when the MEMPOOL message was sent")
    instagibbs commented at 5:27 PM on December 6, 2023: 
            self.log.info("We should get it anyway because it was in the mempool on connection to peer")
  42. sr-gi force-pushed on Dec 6, 2023
  43. instagibbs commented at 6:23 PM on December 6, 2023: member

    ACK https://github.com/bitcoin/bitcoin/pull/28485/commits/97c0dfa8942c7fd62c920deee03b4d0c59aba641

    tests line up with my understanding of the code

  44. DrahtBot requested review from theStack on Dec 6, 2023
  45. DrahtBot requested review from maflcko on Dec 6, 2023
  46. theStack approved
  47. theStack commented at 9:52 AM on December 8, 2023: contributor

    re-ACK 97c0dfa8942c7fd62c920deee03b4d0c59aba641

  48. fanquake merged this on Dec 8, 2023
  49. fanquake closed this on Dec 8, 2023
  50. bitcoin locked this on Dec 7, 2024
Contributors
sr-giDrahtBotmaflckoglozowtheStackinstagibbs


maflckoglozow

Labels
Tests
Linked (view graph)
#27675 p2p: Drop m_recently_announced_invs bloom filter
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: 2026-04-22 09:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me