Repository: bitcoin/bitcoin
File: test/functional/test_framework/test_node.py (Python)

Code before:
        This method adds the p2p connection to the self.p2ps list and also
        returns the connection to the caller.

        When self.use_v2transport is True, TestNode advertises NODE_P2P_V2 service flag

        An inbound connection is made from TestNode <------ P2PConnection
        - if TestNode doesn't advertise NODE_P2P_V2 service, P2PConnection sends version message and v1 P2P is followed
        - if TestNode advertises NODE_P2P_V2 service, (and if P2PConnections supports v2 P2P)
                P2PConnection sends ellswift bytes and v2 P2P is followed
        """
        if 'dstport' not in kwargs:
            kwargs['dstport'] = p2p_port(self.index)
        if 'dstaddr' not in kwargs:
            kwargs['dstaddr'] = '127.0.0.1'
        if supports_v2_p2p is None:
            supports_v2_p2p = self.use_v2transport


        p2p_conn.p2p_connected_to_node = True
        if self.use_v2transport:
            kwargs['services'] = kwargs.get('services', P2P_SERVICES) | NODE_P2P_V2
        supports_v2_p2p = self.use_v2transport and supports_v2_p2p
        p2p_conn.peer_connect(**kwargs, send_version=send_version, net=self.chain, timeout_factor=self.timeout_factor, supports_v2_p2p=supports_v2_p2p)()

        self.p2ps.append(p2p_conn)
        p2p_conn.wait_until(lambda: p2p_conn.is_connected, check_connected=False)
        if supports_v2_p2p and wait_for_v2_handshake:
            p2p_conn.wait_until(lambda: p2p_conn.v2_state.tried_v2_handshake)
        if send_version:
            p2p_conn.wait_until(lambda: not p2p_conn.on_connection_send_msg)
        if wait_for_verack:
            # Wait for the node to send us the version and verack
            p2p_conn.wait_for_verack()
            # At this point we have sent our version message and received the version and verack, however the full node
            # has not yet received the verack from us (in reply to their version). So, the connection is not yet fully
            # established (fSuccessfullyConnected).
            #
            # This shouldn't lead to any issues when sending messages, since the verack will be in-flight before the
            # message we send. However, it might lead to races where we are expecting to receive a message. E.g. a
            # transaction that will be added to the mempool as soon as we return here.
            #
            # So syncing here is redundant when we only want to send a message, but the cost is low (a few milliseconds)
            # in comparison to the upside of making tests less fragile and unexpected intermittent errors less likely.
            p2p_conn.sync_with_ping()

            # Consistency check that the node received our user agent string.
            # Find our connection in getpeerinfo by our address:port and theirs, as this combination is unique.
            sockname = p2p_conn._transport.get_extra_info("socket").getsockname()
            our_addr_and_port = f"{sockname[0]}:{sockname[1]}"
            dst_addr_and_port = f"{p2p_conn.dstaddr}:{p2p_conn.dstport}"
            info = [peer for peer in self.getpeerinfo() if peer["addr"] == our_addr_and_port and peer["addrbind"] == dst_addr_and_port]

Review comment (suggestion):
It seems not ideal to me that this is done before the line `self.p2ps.append(p2p_conn)`.
I think it could lead to errors in which we expect to get disconnected but actually didn't: If we just haven't established the connected yet (i.e. the `connection_made` callback hasn't been called yet), we could abort here, and only afterwards establish the connection that would then not be part of `self.p2ps`.

A better approach in general might be to have an `expect_success` parameter instead that just skips the waits below and returns, and let the individual test deal with the waiting for disconnection waiting (depending on the test it could ask the TestNode, the test framework, both to wait, or do something else like checking logs for specific disconnect reasons, which the tests you add later already do). 

Code after:

        This method adds the p2p connection to the self.p2ps list and also
        returns the connection to the caller.

        When self.use_v2transport is True, TestNode advertises NODE_P2P_V2 service flag

        An inbound connection is made from TestNode <------ P2PConnection
        - if TestNode doesn't advertise NODE_P2P_V2 service, P2PConnection sends version message and v1 P2P is followed
        - if TestNode advertises NODE_P2P_V2 service, (and if P2PConnections supports v2 P2P)
                P2PConnection sends ellswift bytes and v2 P2P is followed
        """
        if 'dstport' not in kwargs:
            kwargs['dstport'] = p2p_port(self.index)
        if 'dstaddr' not in kwargs:
            kwargs['dstaddr'] = '127.0.0.1'
        if supports_v2_p2p is None:
            supports_v2_p2p = self.use_v2transport

        p2p_conn.p2p_connected_to_node = True
        if self.use_v2transport:
            kwargs['services'] = kwargs.get('services', P2P_SERVICES) | NODE_P2P_V2
        supports_v2_p2p = self.use_v2transport and supports_v2_p2p
        p2p_conn.peer_connect(**kwargs, send_version=send_version, net=self.chain, timeout_factor=self.timeout_factor, supports_v2_p2p=supports_v2_p2p)()

        self.p2ps.append(p2p_conn)
        if not expect_success:
            return p2p_conn
        p2p_conn.wait_until(lambda: p2p_conn.is_connected, check_connected=False)
        if supports_v2_p2p and wait_for_v2_handshake:
            p2p_conn.wait_until(lambda: p2p_conn.v2_state.tried_v2_handshake)
        if send_version:
            p2p_conn.wait_until(lambda: not p2p_conn.on_connection_send_msg)
        if wait_for_verack:
            # Wait for the node to send us the version and verack
            p2p_conn.wait_for_verack()
            # At this point we have sent our version message and received the version and verack, however the full node
            # has not yet received the verack from us (in reply to their version). So, the connection is not yet fully
            # established (fSuccessfullyConnected).
            #
            # This shouldn't lead to any issues when sending messages, since the verack will be in-flight before the
            # message we send. However, it might lead to races where we are expecting to receive a message. E.g. a
            # transaction that will be added to the mempool as soon as we return here.
            #
            # So syncing here is redundant when we only want to send a message, but the cost is low (a few milliseconds)
            # in comparison to the upside of making tests less fragile and unexpected intermittent errors less likely.
            p2p_conn.sync_with_ping()

            # Consistency check that the node received our user agent string.
            # Find our connection in getpeerinfo by our address:port and theirs, as this combination is unique.
            sockname = p2p_conn._transport.get_extra_info("socket").getsockname()
            our_addr_and_port = f"{sockname[0]}:{sockname[1]}"