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

mzumsande commented at 5:09 PM on October 10, 2023: contributor

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.

test: make python p2p not send getaddr messages when it's being connected 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.

88c33c6748

DrahtBot commented at 5:09 PM on October 10, 2023: contributor

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.

DrahtBot added the label Tests on Oct 10, 2023

brunoerg commented at 7:44 PM on October 10, 2023: contributor

Concept ACK

pablomartin4btc approved

pablomartin4btc commented at 8:18 PM on October 10, 2023: member

tested ACK

BrandonOdiwuor approved

BrandonOdiwuor commented at 9:32 AM on October 11, 2023: contributor

ACK 88c33c6748da3c4fdadc554ebca43ce7267e9178

DrahtBot requested review from pablomartin4btc on Oct 11, 2023

DrahtBot requested review from brunoerg on Oct 11, 2023

pablomartin4btc commented at 2:50 PM on October 11, 2023: member

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)

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

Co-authored-by: pablomartin4btc <pablomartin4btc@gmail.com>

9cfc1c9440

mzumsande commented at 6:49 PM on October 11, 2023: contributor

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](/bitcoin-bitcoin/5442/))

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

pablomartin4btc commented at 5:31 PM on October 13, 2023: member

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](/bitcoin-bitcoin/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 commented at 8:19 PM on October 13, 2023: contributor

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 commented at 8:35 PM on October 13, 2023: member

Concept ACK

pablomartin4btc commented at 8:40 PM on October 13, 2023: member

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 approved

pablomartin4btc commented at 8:41 PM on October 13, 2023: member

re ACK 9cfc1c94407359379f10906affd2b837851c1b84

DrahtBot requested review from sipa on Oct 13, 2023

DrahtBot requested review from BrandonOdiwuor on Oct 13, 2023

DrahtBot added the label CI failed on Oct 14, 2023

DrahtBot removed the label CI failed on Oct 16, 2023

pinheadmz commented at 7:04 PM on October 18, 2023: member

concept ACK 9cfc1c9440

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)

DrahtBot removed review request from BrandonOdiwuor on Oct 18, 2023

DrahtBot requested review from BrandonOdiwuor on Oct 18, 2023

DrahtBot removed review request from BrandonOdiwuor on Oct 19, 2023

DrahtBot requested review from BrandonOdiwuor on Oct 19, 2023

BrandonOdiwuor approved

BrandonOdiwuor commented at 10:34 AM on October 19, 2023: contributor

re ACK 9cfc1c94407359379f10906affd2b837851c1b84

fanquake merged this on Nov 1, 2023

fanquake closed this on Nov 1, 2023

kwvg referenced this in commit c704012482 on Oct 15, 2024

kwvg referenced this in commit 35253cdd15 on Oct 16, 2024

PastaPastaPasta referenced this in commit dd629cf0eb on Oct 22, 2024

bitcoin locked this on Oct 31, 2024

mzumsande deleted the branch on Jan 2, 2025