p2p: AddrFetch - don't disconnect on self-announcements #22096

nit, don't hesitate to run clang format on the changes

    if (pto->IsAddrFetchConn() && current_time - std::chrono::seconds(pto->nTimeConnected) > 4 * AVG_ADDRESS_BROADCAST_INTERVAL) {

done, thanks

suggest replacing the comma with parentheses or a colon

            {"connection_type", RPCArg::Type::STR, RPCArg::Optional::NO,
             "Type of connection to open (\"outbound-full-relay\", \"block-relay-only\" or \"addr-fetch\")."},

done

New variables naming nit, s/got_*/received_*/

        self.log.info("Check that we send getaddr but don't try to sync headers with the addr-fetch peer")

        peer.sync_send_with_ping()

oops, thanks!

PEP8

        msg.addrs = [ADDR] * 2

        self.log.info('Check timeout for addr-fetch peer that does not send addrs')

fixed

-        node.setmocktime(int(time.time()) + 130)  # Timeout: 2 min
-        peer.sync_send_with_ping
+        node.setmocktime(int(time.time()) + 130)  # Timeout: 2 minutes (with 10 seconds of leeway)
+        peer.sync_send_with_ping()

Changed to 121 (more leeway is not really needed) and removed the sync_send_with_ping() which is incorrect here because it conflicts with the disconnect.

        full-relay ("outbound-full-relay"), block-relay-only ("block-relay-only")

or: Must be an "outbound-full-relay", "block-relay-only" or "addr-fetch" connection.

Took the short version, that should be clear enough.

    } // no default case, so the compiler can warn about missing cases

done

No need for this specialized subclass. The base P2PInterface counts messages received by type, so you can just use that:

diff --git a/test/functional/p2p_addrfetch.py b/test/functional/p2p_addrfetch.py
index b50a710adb..e50e9e7ee4 100755
--- a/test/functional/p2p_addrfetch.py
+++ b/test/functional/p2p_addrfetch.py
@@ -9,7 +9,7 @@ Test p2p addr-fetch connections
 import time
 
 from test_framework.messages import msg_addr, CAddress, NODE_NETWORK, NODE_WITNESS
-from test_framework.p2p import P2PInterface
+from test_framework.p2p import P2PInterface, p2p_lock
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import assert_equal
 
@@ -19,17 +19,6 @@ ADDR.nServices = NODE_NETWORK | NODE_WITNESS
 ADDR.ip = "192.0.0.8"
 ADDR.port = 18444
 
-
-class AddrFetchPeer(P2PInterface):
-    def __init__(self):
-        super().__init__()
-        self.received_getheaders = False
-        self.received_getaddr = False
-
-    def on_getheaders(self, message): self.received_getheaders = True
-    def on_getaddr(self, message): self.received_getaddr = True
-
-
 class P2PAddrFetch(BitcoinTestFramework):
 
     def set_test_params(self):
@@ -39,15 +28,16 @@ class P2PAddrFetch(BitcoinTestFramework):
     def run_test(self):
         node = self.nodes[0]
         self.log.info("Connect to an addr-fetch peer")
-        peer = node.add_outbound_p2p_connection(AddrFetchPeer(), p2p_idx=0, connection_type="addr-fetch")
+        peer = node.add_outbound_p2p_connection(P2PInterface(), p2p_idx=0, connection_type="addr-fetch")
         info = node.getpeerinfo()
         assert_equal(len(info), 1)
         assert_equal(info[0]['connection_type'], 'addr-fetch')
 
         self.log.info("Check that we send getaddr but don't try to sync headers with the addr-fetch peer")
         peer.sync_send_with_ping()
-        assert peer.received_getaddr
-        assert not peer.received_getheaders
+        with p2p_lock:
+            assert peer.message_count['getaddr'] >= 1
+            assert peer.message_count['getheaders'] == 0
 
         self.log.info("Check that answering the getaddr with a single address does not lead to disconnect")
         # This prevents disconnecting on self-announcements
@@ -62,7 +52,7 @@ class P2PAddrFetch(BitcoinTestFramework):
         peer.wait_for_disconnect(timeout=5)
 
         self.log.info("Check timeout for addr-fetch peer that does not send addrs")
-        peer = node.add_outbound_p2p_connection(AddrFetchPeer(), p2p_idx=1, connection_type="addr-fetch")
+        peer = node.add_outbound_p2p_connection(P2PInterface(), p2p_idx=1, connection_type="addr-fetch")
         node.setmocktime(int(time.time()) + 121)  # Timeout: 2 minutes
         peer.wait_for_disconnect(timeout=5)

(this also fixes a potential race where data in the P2PInterface was being read without locking the p2p_lock)

Thanks - I simplified the test as suggested. (with the slightly stricter assert peer.message_count['getaddr'] == 1)

Is the assert_equal() necessary for the validation?

        peer.wait_for_disconnect(timeout=5)
        assert_equal(len(node.getpeerinfo()), 0)

checking the node's connections doesn't seem harmful, but it also doesn't seem necessary because of the way the wait_for_disconnect function works- it keeps checking if the P2PConnection object is connected, and would fail if that was still true after 5 seconds.

I agree that wait_for_disconnect should be sufficient.

Is the assert_equal() necessary for the validation?

        peer.wait_for_disconnect(timeout=5)
        assert_equal(len(node.getpeerinfo()), 0)

same as here, wait_for_disconnect should be sufficient.

Why linking this timing to AVG_ADDRESS_BROADCAST_INTERVAL? It seems irrelevant. Also, 5 minutes feels like a lot, if we thinking of a bootstrapping node hitting buggy/sybil nodes again and again. Maybe 1 minute?

Ah yeah, I see the Poisson justification.

nit: Shouldn't it be linked to AVG_LOCAL_ADDRESS_BROADCAST_INTERVAL then? This is the relevant constant for the first Poisson delay, no?

I don't think so: AVG_LOCAL_ADDRESS_BROADCAST_INTERVAL=24h only deals with our self-announcements, which are then added to m_addrs_to_send. The frequency with which we actually send out the content of m_addrs_to_send is controlled by AVG_ADDRESS_BROADCAST_INTERVAL.

I initially chose a timeout of 2min instead of 5min, but that would have led to a premature disconnect in 1.8% of attempts, which seemed too high. I think it makes sense to be a bit more conservative here, because we use AddrFetch only when we have specified the peer with -seednode or when we can't do DNS and thus connect manually to DNS seeds. We currently don't make AddrFetch connections to random nodes, and don't just choose another AddrFetch peer if we don't get an answer from the current one.

ACK the second part justification.

Re first part: You are right, my confusion was because of 2 variables we have: m_next_addr_send and m_next_local_addr_send.

So I'm just pointing out here that this timeout also handles the case when they send us 1-addr messages (in a malicious case, this can be anything, not just self-announcement).

Currently I can't come up with a way why this is bad, but if we want to disable this unintended behaviour, we could disconnect addrfetch peers on the second 1-addr message too.

Ideally, this gets a comment at least :)

@naumenkogs do I understand correctly that you're concerned about the case where an AddrFetch connection could repeatedly send addr messages with 1 address in them, until this timeout is hit?

The malicious case seems very unlikely- we only make AddrFetch connections to nodes submitted via -seednode, or to the DNS seeds in certain circumstances.

In a more ordinary case, I could imagine how the gossiping of self-announcements could lead to > 1 addr message with a size of 1. If the AddrFetch connection runs a Bitcoin Core node, it seems possible since this line waits approximately 10 intervals before timing out. But also I believe the seeders often run custom software and it doesn't make sense to me that they should be less effective if they were to participate in network gossip.

Conceptually, AddrFetch connections are trying to get a getaddr to be fulfilled, and a getaddr is a request for many addresses. To me, it makes sense for the requesting node to wait a certain amount of time for an acceptable response, and disconnect if that condition is not met. I'm not seeing why we would want to disconnect even earlier in this circumstance.

The malicious case seems very unlikely- we only make AddrFetch connections to nodes submitted via -seednode, or to the DNS seeds in certain circumstances.

Yes indeed. For some reason I thought we do this to random nodes periodically. Maybe that's a feature I wanted to see, in some form :) I agree that for current master it's much less of a concern. A comment might help for future when master changes.

If the AddrFetch connection runs a Bitcoin Core node, it seems possible since this line waits approximately 10 intervals before timing out.

I don't understand this part. Yeah it's 10 intervals of 30s, but self-announcement happens once every 24h. Very-very unlikely a second self-announcement appears within 300s.

But also I believe the seeders often run custom software and it doesn't make sense to me that they should be less effective if they were to participate in network gossip.

I also don't understand this part, what does it have to do with them participating in gossip?

I was entertaining the idea of when a node might send multiple addr messages of size 1 before fulfilling the getaddr message. Although bitcoin core nodes will only initiate a self announcement once every ~24 hours, they will relay self announcements from other nodes.

I think it's also not really more malicious than when they send us nothing at all: The worst thing I could think of that they might do is to painstakingly send us more than 1000 addresses this way.

I'm not sure what kind of comment would help, I'd rather not describe the AddrFetch success-case logic in detail here at the failure case, because if we adjust that logic, the comment will most likely be forgotten.
Maybe something non-specific like "Timeout for AddrFetch peers for when they don't sent us a suitable getaddr response (in which case we'd immediately disconnect)"?

Does this work as intended though?

1. Alice connects to Bob, each other send GETADDR, m_getaddr_sent=true
2. Alice self-announces, Bob sets m_getaddr_sent=false
3. Alice responds to GETADDR, m_getaddr_sent is already false, so the check relying on it is not really correct.

The check being incorrect seems to be not critical because vAddr.size() <= 10 is very likely to be false anyway (assuming sane addrman), but what's the point then?

What this check actually does is don't relay the first self-announcement. I don't think this is intended? Or if it is, it's very unclear from the codebase esp with that variable name. We could really benefit from a comment.

Note: what i'm asking here is not really related to the change of this PR.

I agree, it doesn't work as intended! There was some earlier discussion about this (the flag was recently renamed from fGetAddr), see #21707 (comment) and you may recall #19794. Maybe it would be good to revisit this topic...

nit: I would probably slightly prefer std::optional over relying on bool(0) == false, because the former is more readable.

Done.

nit: it may be slightly more idiomatic to use std::optional::has_value(), or even just use the bool operator:

    if (max_connections && existing_connections >= max_connections) return false;

Will change to std::optional::has_value() when I push next time - I don't like the bool operator for optional<int> because it being true in the case max_connections == 0 might be a bit confusing for a cursory reader.

I find it slightly confusing to check for std::nullopt, but then use the >= operator on std::optional. That operator will again check for std::nullopt. Funnily that operator will evaluate to true if max_connections was std::nullopt, which it can't be here. It might be more clear to just use the >= operator on int.

I see your point - do I understand it correctly that you suggest to use existing_connections >= max_connections.value() in the comparison instead? I could do that as a follow-up.

*max_connections should be enough because an exception can't be thrown

The state just before the timeout could be checked to ensure the disconnection only occurs after the 5 minute threshold.