test: remove race in the user-agent reception check #27986

pull vasild wants to merge 1 commits into bitcoin:master from vasild:test_more_robust_ua_recv_check changing 1 files +7 −4
  1. vasild commented at 8:32 AM on June 28, 2023: contributor

    In add_p2p_connection() we connect to bitcoind from the Python client and check that it has received our version string.

    This check looked up the last/newest entry from getpeerinfo RPC, assuming that it must be the connection we have just opened. But this will not be the case if a new inbound or outbound connection is made to/from bitcoind in the meantime.

    Instead of the last entry in getpeerinfo, check all and find the one which corresponds to our connection using our outgoing address:port tuple which is unique.

  2. DrahtBot commented at 8:32 AM on June 28, 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 jonatack
    Concept ACK RandyMcMillan, MarcoFalke

    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 Jun 28, 2023
  4. vasild commented at 8:34 AM on June 28, 2023: contributor

    I stumbled on this playing with #27509:

    https://cirrus-ci.com/task/6722465808777216?logs=ci#L8761-L8771

    <details> <summary>CI log</summary>

    2023-06-27T09:50:26.796000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 132, in main
    self.run_test()
  File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/p2p_local_tx_relay.py", line 211, in run_test
    observer_inbound = node0.add_p2p_connection(P2PDataStore())
  File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/test_node.py", line 639, in add_p2p_connection
    assert_equal(self.getpeerinfo()[-1]['subver'], P2P_SUBVERSION)
  File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/util.py", line 57, in assert_equal
    raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
AssertionError: not( == /python-p2p-tester:0.0.3/)

    </details>

    The test calls add_p2p_connection() and at the same time bitcoind opens a new outbound connection. The subver field for the new connection in getpeerinfo is empty before the VERSION message is received.

  5. in test/functional/test_framework/test_node.py:639 in 44d71cfd0e outdated 
     638 | -            # our connection, but we don't expect that to happen.
 639 | -            assert_equal(self.getpeerinfo()[-1]['subver'], P2P_SUBVERSION)
 640 | +            # Consistency check that the Bitcoin Core has received our user agent string.
 641 | +            # Find our connection in "getpeerinfo" by our address:port, it is unique.
 642 | +            us_addrport = p2p_conn._transport.get_extra_info("socket").getsockname()
 643 | +            found_our_connection = False
    maflcko commented at 8:39 AM on June 28, 2023:

    Why would this ever be not found, given that the previously the connection was sync_with_ping'd?

    In any case, you can use next() to do the assertion. Roughly:

    p=next([p if p["addr"] == f"{us_addrport[0]}:{us_addrport[1]}" p in self.getpeerinfo()])
assert_equal(p["subver"], P2P_SUBVERSION)
    vasild commented at 9:25 AM on June 28, 2023:

    Why would this ever be not found

    Well, I do not know, maybe some typo in p["addr"] == f"{us_addrport[0]}:{us_addrport[1]}" or some other unexpected thing. That is just another sanity check.

    Taking a step back I wonder if this check as much value - why would bitcoind not have received our user agent string? I wouldn't object if somebody wants to drop the check altogether.

    p=next([p if p["addr"] == f"{us_addrport[0]}:{us_addrport[1]}" p in self.getpeerinfo()])

    I find this completely obscure. It gives:

        p = next([p if p["addr"] == f"{us_addrport[0]}:{us_addrport[1]}" p in self.getpeerinfo()])
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
SyntaxError: expected 'else' after 'if' expression

    As the main language of this project is C++ I try to keep the python tests accessible for C++ readers that are not so good Pyhon readers - me included, I admit :disappointed:

    maflcko commented at 9:31 AM on June 28, 2023:

    That is just another sanity check.

    Might be better to check the length is one (unique), if the check is kept?

    vasild commented at 11:00 AM on June 28, 2023:

    Done!

  6. vasild force-pushed on Jun 28, 2023
  7. vasild commented at 10:59 AM on June 28, 2023: contributor

    44d71cfd0e...9c46f3ba19: do #27986 (review)

  8. in test/functional/test_framework/test_node.py:646 in 9c46f3ba19 outdated 
     645 | +                if p["addr"] == f"{us_addrport[0]}:{us_addrport[1]}":
 646 | +                    assert not found_our_connection
 647 | +                    found_our_connection = True
 648 | +                    assert_equal(p["subver"], P2P_SUBVERSION)
 649 | +                    # Keep checking the rest to ensure not more than one matches.
 650 | +            assert found_our_connection
    maflcko commented at 11:37 AM on June 28, 2023:

    style-nit: Seems odd to have three asserts to check two things. Can be written shorter and easier to read:

                p = [p for p in self.getpeerinfo() if p["addr"] == f"{us_addrport[0]}:{us_addrport[1]}"]
            assert_equal(len(p), 1)
            assert_equal(p[0]["subver"], P2P_SUBVERSION)
    vasild commented at 2:24 PM on June 28, 2023:

    Done.

    I only changed the first p to info because p = [p for p ... seemed too many ps in one place. :exploding_head:

  9. vasild force-pushed on Jun 28, 2023
  10. vasild commented at 2:20 PM on June 28, 2023: contributor

    9c46f3ba19...c1a169082a: take #27986 (review)

  11. DrahtBot added the label CI failed on Jun 28, 2023
  12. in test/functional/test_framework/test_node.py:637 in c1a169082a outdated 
     632 | @@ -633,10 +633,12 @@ def add_p2p_connection(self, p2p_conn, *, wait_for_verack=True, **kwargs):
 633 |              # in comparison to the upside of making tests less fragile and unexpected intermittent errors less likely.
 634 |              p2p_conn.sync_with_ping()
 635 |  
 636 | -            # Consistency check that the Bitcoin Core has received our user agent string. This checks the
 637 | -            # node's newest peer. It could be racy if another Bitcoin Core node has connected since we opened
 638 | -            # our connection, but we don't expect that to happen.
 639 | -            assert_equal(self.getpeerinfo()[-1]['subver'], P2P_SUBVERSION)
 640 | +            # Consistency check that the Bitcoin Core has received our user agent string.
 641 | +            # Find our connection in "getpeerinfo" by our address:port, it is unique.
    jonatack commented at 4:10 PM on June 28, 2023:

    Suggest while touching this, to make it clearer and more coherent with the documentation just above.

    -            # Consistency check that the Bitcoin Core has received our user agent string.
-            # Find our connection in "getpeerinfo" by our address:port, it is unique.
+            # Consistency check that the node received our user agent string.
+            # Find our connection in getpeerinfo by our address:port, as it is unique.
    vasild commented at 4:37 PM on June 28, 2023:

    Done.

  13. jonatack commented at 4:12 PM on June 28, 2023: contributor

    Light tested ACK c1a169082ac1147ec2f3cc329f23bc0bfc28cd6b

  14. vasild force-pushed on Jun 28, 2023
  15. vasild commented at 4:37 PM on June 28, 2023: contributor

    c1a169082a...28d26c4f37: take #27986 (review)

    Invalidates ACK from @jonatack

  16. in test/functional/test_framework/test_node.py:638 in 28d26c4f37 outdated 
     637 | -            # node's newest peer. It could be racy if another Bitcoin Core node has connected since we opened
 638 | -            # our connection, but we don't expect that to happen.
 639 | -            assert_equal(self.getpeerinfo()[-1]['subver'], P2P_SUBVERSION)
 640 | +            # Consistency check that the node received our user agent string.
 641 | +            # Find our connection in getpeerinfo by our address:port, as it is unique.
 642 | +            us_addrport = p2p_conn._transport.get_extra_info("socket").getsockname()
    jonatack commented at 5:01 PM on June 28, 2023:

    Verified per https://docs.python.org/3/whatsnew/3.8.html that asyncio.BaseTransport.get_extra_info() is supported in Python 3.8, the minimum supported version per doc/dependencies.md.

    I'm not sure if import asyncio should be added to this file.

    vasild commented at 10:24 AM on June 29, 2023:

    Thanks for checking the versions!

    I'm not sure if import asyncio should be added to this file.

    test/lint/lint-python.py is happy and CI is green, so I guess no need to add it. If I do, then the linter is upset:

    test/functional/test_framework/test_node.py:7:1: F401 'asyncio' imported but unused
    jonatack commented at 9:36 PM on July 5, 2023:

    test/functional/test_framework/test_node.py:7:1: F401 'asyncio' imported but unused

    Agree, I see that as well with Python 3.11.4.

  17. jonatack commented at 5:02 PM on June 28, 2023: contributor

    ACK 28d26c4f37c433e35a4a05e144d05f0e464f8452

  18. DrahtBot removed the label CI failed on Jun 28, 2023
  19. test: remove race in the user-agent reception check
    In `add_p2p_connection()` we connect to `bitcoind` from the Python
client and check that it has received our version string.

This check looked up the last/newest entry from `getpeerinfo` RPC,
assuming that it must be the connection we have just opened. But this
will not be the case if a new inbound or outbound connection is made
to/from `bitcoind` in the meantime.

Instead of the last entry in `getpeerinfo`, check all and find the one
which corresponds to our connection using our outgoing address:port
tuple which is unique.

Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Co-authored-by: Jon Atack <jon@atack.com>
    20b49460b3
  20. in test/functional/test_framework/test_node.py:639 in 28d26c4f37 outdated 
     638 | -            # our connection, but we don't expect that to happen.
 639 | -            assert_equal(self.getpeerinfo()[-1]['subver'], P2P_SUBVERSION)
 640 | +            # Consistency check that the node received our user agent string.
 641 | +            # Find our connection in getpeerinfo by our address:port, as it is unique.
 642 | +            us_addrport = p2p_conn._transport.get_extra_info("socket").getsockname()
 643 | +            info = [p for p in self.getpeerinfo() if p["addr"] == f"{us_addrport[0]}:{us_addrport[1]}"]
    jonatack commented at 9:47 PM on July 5, 2023:

    Looking at this again, the following would move the string interpolation outside the loop (with perhaps clearer naming). Feel free to ignore.

    -            us_addrport = p2p_conn._transport.get_extra_info("socket").getsockname()
-            info = [p for p in self.getpeerinfo() if p["addr"] == f"{us_addrport[0]}:{us_addrport[1]}"]
+            sockname = p2p_conn._transport.get_extra_info("socket").getsockname()
+            our_addr_and_port = f"{sockname[0]}:{sockname[1]}"
+            info = [peer for peer in self.getpeerinfo() if peer["addr"] == our_addr_and_port]
    vasild commented at 3:49 PM on July 6, 2023:

    Looks better, thanks!

  21. vasild force-pushed on Jul 6, 2023
  22. vasild commented at 3:49 PM on July 6, 2023: contributor

    28d26c4f37...20b49460b3: take #27986 (review)

    Invalidates ACK from @jonatack

  23. RandyMcMillan commented at 4:44 PM on July 6, 2023: contributor

    Concept ACK

  24. jonatack commented at 6:48 PM on July 6, 2023: contributor

    re-ACK 20b49460b35268db59f7efcb02736b0e31191a74

  25. jonatack commented at 11:20 PM on July 17, 2023: contributor

    Anything further needed here?

  26. maflcko commented at 5:52 AM on July 18, 2023: member

    Concept ACK 20b49460b35268db59f7efcb02736b0e31191a74

    Probably an edge case, but it seems useful to have this in case connections are opened at the same time for some reason.

  27. vasild commented at 10:53 AM on July 18, 2023: contributor

    The tests from #27509 failed because of this which prompted me to open this PR.

  28. maflcko added this to the milestone 26.0 on Jul 18, 2023
  29. fanquake merged this on Jul 19, 2023
  30. fanquake closed this on Jul 19, 2023
  31. vasild deleted the branch on Jul 19, 2023
  32. sidhujag referenced this in commit d88252490d on Jul 19, 2023
  33. bitcoin locked this on Jul 18, 2024
Contributors
vasildDrahtBotmaflckojonatackRandyMcMillan
Labels
Tests

Milestone
26.0

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-25 15:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me