Currently the sync in connect_nodes mentions the version and verack message types, but only checks the verack. Neither check is required, as the pong check implies both. In case of failure, the debug log will have to be consulted anyway, so the redundant check doesn’t add value.
Also clarify in the comments that the goal is to check the flag fSuccessfullyConnected indirectly.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
653-
654- # The message bytes are counted before processing the message, so make
655- # sure it was fully processed by waiting for a ping.
656+ # Poll until version handshake (fSuccessfullyConnected) is complete to
657+ # avoid race conditions. As the flag fSuccessfullyConnected is not
658+ # exposed, check it by waiting for a ping, which can only happen after
check it by waiting for a ping
The code waits for successful pongs rather than pings.
ACK fa4f453281e1b38ec4bc87eadbfcfee11d6ea530 Makes sense. Version handshake occurs before ping/pong.
As a sanity check, ran functional tests to see the tests continue to function properly (all passed). Also viewed node traffic between two regtest nodes, and saw ping/pong occur after version/verack exchange.
652- self.wait_until(lambda: check_bytesrecv(find_conn(to_connection, from_connection_subver, inbound=True), 'verack', 21))
653-
654- # The message bytes are counted before processing the message, so make
655- # sure it was fully processed by waiting for a ping.
656+ # Poll until version handshake (fSuccessfullyConnected) is complete to
657+ # avoid race conditions. As the flag fSuccessfullyConnected is not
I think the name fSuccessfullyConnected==false implies that the connection may not be successfully used (yet), so I think I’ll leave it as-is for now.
Also, I think “Will ignore all messges received” isn’t true, because the node will accept some message types.
What about: “Some message types are blocked from being sent or received before fSuccessfullyConnected.”? (Edit: Pushed this)
