test: make python p2p not send getaddr on incoming connections #28632

mzumsande · 2023-10-10T17:09:24Z

bitcoind nodes send getaddr messages only to outbound nodes (and ignore getaddr received by outgoing connections).
The python p2p node should mirror this behavior by not sending a getaddr message when it is not the initiator of the connection.
This is currently causing several unnecessary messages being sent and then ignored (Ignoring "getaddr" from outbound-full-relay connection.) in tests like p2p_add_connections.py.

…cted to Bitcoind nodes send getaddr msgs only to outbound nodes (and ignore those received by outgoing connections). The python p2p node should mirror this behavior by not sending a getaddr message when it is not the initiator of the connection.

DrahtBot · 2023-10-10T17:09:27Z

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

Code Coverage

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	pablomartin4btc, pinheadmz, BrandonOdiwuor
Concept ACK	brunoerg, sipa

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24748 (test/BIP324: functional tests for v2 P2P encryption by stratospher)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

brunoerg · 2023-10-10T19:44:48Z

Concept ACK

pablomartin4btc

tested ACK

BrandonOdiwuor

ACK 88c33c6

pablomartin4btc

Would it be possible to add these 2 checks (explicitly) at some point (?):

verify that there are no getaddr messages sent to inbound nodes,
verify that filtering out getaddr messages when come from outbound nodes works (eg #5442)

Co-authored-by: pablomartin4btc <[email protected]>

mzumsande · 2023-10-11T18:49:31Z

verify that there are no getaddr messages sent to inbound nodes

Good idea, I added to commit to add this check to p2p_addr_relay.py (it's basically just an added assert)

verify that filtering out getaddr messages when come from outbound nodes works (eg Ignore getaddr messages on Outbound connections. #5442)

That is already verified in the "Check that we answer getaddr messages only from inbound peers" section of p2p_addr_relay.py

pablomartin4btc · 2023-10-13T17:31:10Z

verify that there are no getaddr messages sent to inbound nodes

Good idea, I added to commit to add this check to p2p_addr_relay.py (it's basically just an added assert)

Great, thanks and I appreciate including me as a co-author of that change, many thanks.

verify that filtering out getaddr messages when come from outbound nodes works (eg Ignore getaddr messages on Outbound connections. #5442)

That is already verified in the "Check that we answer getaddr messages only from inbound peers" section of p2p_addr_relay.py

Yeah, perhaps I'm wrong but what I meant is since we are not sending the getaddr anymore (eg to inbound peers) we are not really filtering them out (from outbound peers), so maybe we need to force this situation (assert_equal(full_outbound_peer.getaddr_received(), True)) to validate that we don't reply to them. Perhaps this is already happening at another level and I'm totally missing it, ignore me if that's the case.

mzumsande · 2023-10-13T20:19:25Z

Yeah, perhaps I'm wrong but what I meant is since we are not sending the getaddr anymore (eg to inbound peers) we are not really filtering them out (from outbound peers), so maybe we need to force this situation (assert_equal(full_outbound_peer.getaddr_received(), True)) to validate that we don't reply to them. Perhaps this is already happening at another level and I'm totally missing it, ignore me if that's the case.

Sorry, I don't follow, who is "we"?
After this PR, the python p2p doesn't send getaddr to bitcoind anymore automatically if bitcoind connects to it.
In the existing test that I referred to, we test whether getaddr are ignored or not by instead sending the msg manually from pyhton p2p here and then check here whether bitcoind responded or ignored it.

(assert_equal(full_outbound_peer.getaddr_received(), True))

That is the other direction (python p2p receives getaddr, bitcoind sends it or not) and is checked here.

sipa · 2023-10-13T20:35:00Z

Concept ACK

pablomartin4btc · 2023-10-13T20:40:26Z

Sorry, I don't follow, who is "we"?

I meant with the changes made by this PR.

After this PR, the python p2p doesn't send getaddr to bitcoind anymore automatically if bitcoind connects to it. In the existing test that I referred to, we test whether getaddr are ignored or not by instead sending the msg manually from pyhton p2p here and then check here whether bitcoind responded or ignored it.

Perfect, thank you very much for the clarification, I missed that.

(assert_equal(full_outbound_peer.getaddr_received(), True))

That is the other direction (python p2p receives getaddr, bitcoind sends it or not) and is checked here.

Thanks again for the links, I got it now.

pablomartin4btc

re ACK 9cfc1c9

pinheadmz · 2023-10-18T19:04:36Z

concept ACK 9cfc1c9

Confirmed the in/outbound asymmetry for sending and responding to getaddr messages

I wonder if it'd be cleaner to initialize Class P2PConnection with property self.outbound = True and then you only have to switch it to False in TestNode.add_outbound_p2p_connection() (which is outbound from bitcoin core but the P2PConnection perspective is inbound)

BrandonOdiwuor

re ACK 9cfc1c9

…nections

, bitcoin#28645, bitcoin#28632, bitcoin#28782, bitcoin#28822, bitcoin#29006, bitcoin#29212, merge bitcoin-core/gui#754, partial bitcoin#23443, bitcoin#26448 (BIP324 backports: part 3) aa5311d merge bitcoin#29212: Fix -netinfo backward compat with getpeerinfo pre-v26 (Kittywhiskers Van Gogh) 1a293c7 merge bitcoin#29006: fix v2 transport intermittent test failure (Kittywhiskers Van Gogh) d0804d4 merge bitcoin#28822: Add missing wait for version to be sent in add_outbound_p2p_connection (Kittywhiskers Van Gogh) c0b3062 merge bitcoin#28782: Add missing sync on send_version in peer_connect (Kittywhiskers Van Gogh) 35253cd merge bitcoin#28632: make python p2p not send getaddr on incoming connections (Kittywhiskers Van Gogh) 6a4ca62 merge bitcoin#28645: fix `assert_debug_log` call-site bugs, add type checks (Kittywhiskers Van Gogh) deaee14 merge bitcoin-core/gui#754: Add BIP324-specific labels to peer details (Kittywhiskers Van Gogh) fffe6e7 merge bitcoin#27986: remove race in the user-agent reception check (Kittywhiskers Van Gogh) 1bf135b merge bitcoin#26553: Fix intermittent failure in rpc_net.py (Kittywhiskers Van Gogh) 5bf245b partial bitcoin#26448: fix intermittent failure in p2p_sendtxrcncl.py (Kittywhiskers Van Gogh) b7c0030 partial bitcoin#23443: Erlay support signaling (Kittywhiskers Van Gogh) c709df7 merge bitcoin#20524: Move MIN_VERSION_SUPPORTED to p2p.py (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependent on #6324 * Dependency for #6330 ## How Has This Been Tested? * Changes to Qt client were validated by running the client <details> <summary>Screenshot</summary> ![Transport reporting in Qt](https://github.com/user-attachments/assets/0d551e19-f3a2-4ce7-83d6-5cb3d03b1765) </details> * Changes to `dash-cli` were validated by running it with different node versions **Against a node built on this PR** <details> <summary>Screenshot</summary> ![getinfo with node running the latest build](https://github.com/user-attachments/assets/8cda68cc-727a-4cf3-a4d8-dd6a33331d78) </details> **Against a node built running the last release** <details> <summary>Screenshot</summary> ![getinfo with node running the latest release](https://github.com/user-attachments/assets/0c6ff476-7cc9-4297-bae5-35d423aba480) </details> ## Breaking Changes None expected. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas (note: N/A) - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation (note: N/A) - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: ACK aa5311d PastaPastaPasta: utACK aa5311d Tree-SHA512: 8cca324ac988a73c0590a4e9b318e81ce951ac55fb173cf507fa647cab01ab4981e6a06d4792376b4bfb44ff09d4811de05fadb9ba793dd00b4c7965b4b22654

DrahtBot added the Tests label Oct 10, 2023

pablomartin4btc approved these changes Oct 10, 2023

View reviewed changes

DrahtBot mentioned this pull request Oct 11, 2023

test/BIP324: functional tests for v2 P2P encryption #24748

Merged

BrandonOdiwuor approved these changes Oct 11, 2023

View reviewed changes

DrahtBot requested review from brunoerg and pablomartin4btc October 11, 2023 09:32

pablomartin4btc reviewed Oct 11, 2023

View reviewed changes

test: check that we don't send a getaddr msg to an inbound peer

9cfc1c9

Co-authored-by: pablomartin4btc <[email protected]>

pablomartin4btc approved these changes Oct 13, 2023

View reviewed changes

DrahtBot requested review from BrandonOdiwuor and sipa October 13, 2023 20:41

DrahtBot added CI failed and removed CI failed labels Oct 14, 2023

DrahtBot requested review from BrandonOdiwuor and removed request for BrandonOdiwuor October 18, 2023 19:04

BrandonOdiwuor approved these changes Oct 19, 2023

View reviewed changes

fanquake merged commit eca2e43 into bitcoin:master Nov 1, 2023

kwvg added a commit to kwvg/dash that referenced this pull request Oct 16, 2024

merge bitcoin#28632: make python p2p not send getaddr on incoming con…

35253cd

…nections

bitcoin locked and limited conversation to collaborators Oct 31, 2024

mzumsande deleted the 202310_test_no_getaddr branch January 2, 2025 22:22

test: make python p2p not send getaddr on incoming connections #28632

test: make python p2p not send getaddr on incoming connections #28632

Uh oh!

Conversation

mzumsande commented Oct 10, 2023

Uh oh!

DrahtBot commented Oct 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

brunoerg commented Oct 10, 2023

Uh oh!

pablomartin4btc left a comment

Choose a reason for hiding this comment

Uh oh!

BrandonOdiwuor left a comment

Choose a reason for hiding this comment

Uh oh!

pablomartin4btc left a comment

Choose a reason for hiding this comment

Uh oh!

mzumsande commented Oct 11, 2023

Uh oh!

pablomartin4btc commented Oct 13, 2023

Uh oh!

mzumsande commented Oct 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Oct 13, 2023

Uh oh!

pablomartin4btc commented Oct 13, 2023

Uh oh!

pablomartin4btc left a comment

Choose a reason for hiding this comment

Uh oh!

pinheadmz commented Oct 18, 2023

Uh oh!

BrandonOdiwuor left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

DrahtBot commented Oct 10, 2023 •

edited

Loading

mzumsande commented Oct 13, 2023 •

edited

Loading