test: fix intermittent failure in p2p_getaddr_caching.py #28144

1. 
   mzumsande commented at 1:01 AM on July 25, 2023: contributor

   Fixes #28133

   In the consistency check, it's not enough to check that our address/port is unique, only the combination of source and target must be unique. Otherwise, the OS may reuse ports for connections to different -addrbind, which was happening in the failed runs.

   While at it, the second commit cleans up duplicate getaddr messages in p2p_getaddr_caching.py that do nothing but generate Ignoring repeated "getaddr" log messages (and cleans up some whitespace the python linter complains about).

2. 
   DrahtBot commented at 1:02 AM on July 25, 2023: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->

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

   <!--021abf342d371248e50ceaed478a90ca-->

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	vasild
   Concept ACK	jonatack

   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 Jul 25, 2023
4. mzumsande renamed this:
   test: fix intermittent timeout in p2p_getaddr_caching.py
   test: fix intermittent failure in p2p_getaddr_caching.py
   on Jul 25, 2023
5. mzumsande force-pushed on Jul 25, 2023
6. 
   jonatack commented at 1:32 AM on July 25, 2023: member

   Concept ACK, thank you for looking into it.

7. DrahtBot added the label CI failed on Jul 25, 2023
8. DrahtBot removed the label CI failed on Jul 25, 2023
9. maflcko requested review from vasild on Jul 25, 2023
10. maflcko added this to the milestone 26.0 on Jul 25, 2023
11. in test/functional/p2p_getaddr_caching.py:73 in e4d37875e6 outdated

      70 | @@ -70,11 +71,8 @@ def run_test(self):
      71 |          cur_mock_time = int(time.time())
      72 |          for i in range(N):
      73 |              addr_receiver_local = self.nodes[0].add_p2p_connection(AddrReceiver())
      74 | -            addr_receiver_local.send_and_ping(msg_getaddr())
    

    ---

    ---

    

    vasild commented at 8:47 AM on July 25, 2023:

    There is a similar pattern below on lines 108,110,112. Should be possible to remove those too? If yes, then also from test_framework.messages import msg_getaddr.

    To other reviewers: the GETADDR message is sent at the end of P2PInterface::on_version().

    ---

    

    mzumsande commented at 2:47 PM on July 25, 2023:

    yes, I missed those, now I removed them all!

12. in test/functional/test_framework/test_node.py:651 in e4d37875e6 outdated

     644 | @@ -645,10 +645,11 @@ def add_p2p_connection(self, p2p_conn, *, wait_for_verack=True, **kwargs):
     645 |              p2p_conn.sync_with_ping()
     646 |  
     647 |              # Consistency check that the node received our user agent string.
     648 | -            # Find our connection in getpeerinfo by our address:port, as it is unique.
     649 | +            # Find our connection in getpeerinfo by our address:port and theirs, as this combination is unique.
     650 |              sockname = p2p_conn._transport.get_extra_info("socket").getsockname()
     651 |              our_addr_and_port = f"{sockname[0]}:{sockname[1]}"
     652 | -            info = [peer for peer in self.getpeerinfo() if peer["addr"] == our_addr_and_port]
     653 | +            dst_addr_and_port =f"{p2p_conn.dstaddr}:{p2p_conn.dstport}"
    

    ---

    ---

    

    vasild commented at 8:49 AM on July 25, 2023:

    nit, missing white space after =:

                dst_addr_and_port = f"{p2p_conn.dstaddr}:{p2p_conn.dstport}"
    

    ---

    

    mzumsande commented at 2:47 PM on July 25, 2023:

    fixed

13. vasild approved
14. 
    vasild commented at 8:52 AM on July 25, 2023: contributor

    ACK e4d37875e6a440748623250d77ec3be523a1d34d

    Today I learned something new: it is possible to have duplicate (srcaddr, srcport) for TCP connections if the destination is different. Thank you!

15. test: fix intermittent failure in p2p_getaddr_caching

    Only the combined addr:port of source and destination
    must be unique. If the destination is different, the same addr:port
    for the source may be used by the OS.

    feb0096139
16. test: drop duplicate getaddrs from p2p_getaddr_caching

    python p2p instances will automatically send a getaddr msg after
    connecting, the explicit message was a duplicate that was being ignored.

    8a20f765cc
17. mzumsande force-pushed on Jul 25, 2023
18. 
    mzumsande commented at 2:50 PM on July 25, 2023: contributor

    e4d3787 to 8a20f76: Addressed feedback, thanks!

    Today I learned something new: it is possible to have duplicate (srcaddr, srcport) for TCP connections if the destination is different. Thank you!

    I was also surprised by this!

19. vasild approved
20. 
    vasild commented at 4:45 PM on July 25, 2023: contributor

    ACK 8a20f765cce2fc0fadf1a2b66b843b67f2d2ae12

21. fanquake merged this on Aug 1, 2023
22. fanquake closed this on Aug 1, 2023

    ---

23. sidhujag referenced this in commit c744ab6024 on Aug 9, 2023
24. bitcoin locked this on Jul 31, 2024

Contributors

mzumsandeDrahtBotjonatackvasild

Milestone
26.0