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

mzumsande commented at 4:18 pm on May 28, 2021: contributor

AddrFetch connections (old name: oneshots) are intended to be short-lived connections on which we ask a peer for addresses via getaddr and disconnect after receiving them.

This is done by disconnecting after receiving the first addr. However, it is no longer working as intended, because nowadays, the first addr a typical bitcoin core node sends is its self-announcement. So we’ll disconnect before the peer gets a chance to answer our getaddr.

I checked that this affects both -seednode peers specified manually, and DNS seeds when AddrFetch is used as a fallback if DNS doesn’t work for us.

The current behavior of getting peers via AddrFetch when starting with an empty addrman would be to connect to the peer, receive its self-announcement and add it to addrman, disconnect, reconnect to the same peer again as a full outbound (no other addresses in addrman) and then receive more addr. This is silly and not in line with AddrFetch peer being intended to be short-lived peers.

Fix this by only disconnecting after receiving an addr message of size > 1.

[Edit] As per review discussion, this PR now also adds a timeout after which we disconnect if we haven’t received any suitable addr, and a functional test.

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

Disconnecting an AddrFetch peer only after receiving an addr
message of size >1 prevents dropping them before
they had a chance to answer the getaddr request.

b6c5d1e450

mzumsande force-pushed on May 28, 2021

sdaftuar commented at 4:50 pm on May 28, 2021: member

I agree this makes sense.

While you’re improving the addrfetch logic, perhaps we could also add a timeout for disconnection so that if an addrfetch peer hasn’t responded to our getaddr for some amount of time, we eventually disconnect regardless? I believe right now we’d just stay connected indefinitely to such a peer.

DrahtBot added the label P2P on May 28, 2021

amitiuttarwar commented at 10:36 pm on May 28, 2021: contributor

good catch! concept ACK

half baked thought: Wondering if it would make sense to combine this value with the one used here. In my understanding, both are trying to use the number of addrs to discern [response to getaddr] vs [probably a self-announcement]. We could introduce a constant that’s used in both places.

I’m also wondering if we can add functional test coverage for AddrFetch connections. I think the groundwork is in place with the outbound-full-relay and block-relay-only connections. We might just be able to add the addr-fetch option to TestNode.add_outbound_p2p_connection, the addconnection RPC, and CConnman::AddConnection. But I only took an initial look & this certainly does not need to happen in this PR.

sipa commented at 3:27 am on May 29, 2021: member

Concept ACK

lsilva01 commented at 3:30 am on May 29, 2021: contributor

Tested ACK https://github.com/bitcoin/bitcoin/pull/22096/commits/b6c5d1e450dde6a54bd785504c923adfb45c7060. I ran the node with the args -debug=net -dnsseed=0 -seednode=<IP> -fixedseeds=0 using the master branch and this PR branch (table below).

The way ADDR_FETCH is used today to fetch new address from a seed IP seems not work as expected, since the first message is only self-announcement and the ADDR message with 1000 addresses is received via OUTBOUND_FULL_RELAY from the same peer.

As shown in the second table, after the proposed change in this PR, the first ADDR_FETCH connection actually fetches for addresses.

Master Branch results:

From IP	Connection Type	`vAddr` size	`AddrMan` size	self-announcement
`seed_ip`	`ADDR_FETCH`	1	0	yes
`seed_ip`	`OUTBOUND_FULL_RELAY`	1	1	yes
`seed_ip`	`OUTBOUND_FULL_RELAY`	1000	1	no
`new_ip`	`OUTBOUND_FULL_RELAY`	1	870	yes

PR Branch results:

From IP	Connection Type	`vAddr` size	`AddrMan` size	self-announcement
`seed_ip`	`ADDR_FETCH`	1	0	yes
`seed_ip`	`ADDR_FETCH`	1000	1	no
`new_ip`	`OUTBOUND_FULL_RELAY`	1	870	yes

bitcoin deleted a comment on May 29, 2021

mzumsande commented at 5:51 pm on May 30, 2021: contributor

While you’re improving the addrfetch logic, perhaps we could also add a timeout for disconnection so that if an addrfetch peer hasn’t responded to our getaddr for some amount of time, we eventually disconnect regardless? I believe right now we’d just stay connected indefinitely to such a peer.

Yes, I think that would be useful, and I will add this. I think such an addrfetch peer also wouldn’t count toward the max full outbound limit in spite of effectively behaving like one. As for the timeout, I’d suggest a value 4*AVG_ADDRESS_BROADCAST_INTERVAL = 2min after connection.

half baked thought: Wondering if it would make sense to combine this value with the one used here. In my understanding, both are trying to use the number of addrs to discern [response to getaddr] vs [probably a self-announcement]. We could introduce a constant that’s used in both places.

Do you suggest just using size > 10 instead of 1 in the check? More seems difficult, because the rest of the criteria work on the level of individual addrs.

I’m also wondering if we can add functional test coverage for AddrFetch connections. I think the groundwork is in place with the outbound-full-relay and block-relay-only connections. We might just be able to add the addr-fetch option to TestNode.add_outbound_p2p_connection, the addconnection RPC, and CConnman::AddConnection timeout behavior is tested.

Good idea! I think it would make sense to combine this with @sdaftuar ’s suggestion of a timeout, so that the timout will be tested. Adding AddrFetch to the test framework actually does seem to work exactly as you suggested, just need to figure out some details and will add this in the next days (unless reviewers prefer that to be a separate PR).

bitcoin deleted a comment on May 31, 2021

mzumsande marked this as a draft on May 31, 2021

mzumsande commented at 3:08 pm on June 3, 2021: contributor

I added the timeout and a functional test now. Thanks a lot @amitiuttarwar for help with the functional test!

mzumsande marked this as ready for review on Jun 3, 2021

mzumsande force-pushed on Jun 3, 2021

in src/net_processing.cpp:4369 in d728c6c871 outdated

4365@@ -4364,6 +4366,12 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
4366 
4367     MaybeSendPing(*pto, *peer, current_time);
4368 
4369+    if(pto->IsAddrFetchConn() && current_time - std::chrono::seconds(pto->nTimeConnected) > 4 * AVG_ADDRESS_BROADCAST_INTERVAL) {

jonatack commented at 3:28 pm on June 3, 2021:

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

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

mzumsande commented at 4:32 pm on June 4, 2021:

done, thanks

in src/rpc/net.cpp:340 in d728c6c871 outdated

336@@ -337,7 +337,7 @@ static RPCHelpMan addconnection()
337         "\nOpen an outbound connection to a specified node. This RPC is for testing only.\n",
338         {
339             {"address", RPCArg::Type::STR, RPCArg::Optional::NO, "The IP address and port to attempt connecting to."},
340-            {"connection_type", RPCArg::Type::STR, RPCArg::Optional::NO, "Type of connection to open, either \"outbound-full-relay\" or \"block-relay-only\"."},
341+            {"connection_type", RPCArg::Type::STR, RPCArg::Optional::NO, "Type of connection to open, \"outbound-full-relay\", \"block-relay-only\" or \"addr-fetch\"."},

jonatack commented at 3:29 pm on June 3, 2021:

suggest replacing the comma with parentheses or a colon

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

mzumsande commented at 4:33 pm on June 4, 2021:

done

in test/functional/p2p_addrfetch.py:27 in d728c6c871 outdated

22+
23+class AddrFetchPeer(P2PInterface):
24+    def __init__(self):
25+        super().__init__()
26+        self.got_getheaders = False
27+        self.got_getaddr = False

jonatack commented at 3:31 pm on June 3, 2021:

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

in test/functional/p2p_addrfetch.py:47 in d728c6c871 outdated

42+        peer = node.add_outbound_p2p_connection(AddrFetchPeer(), p2p_idx=0, connection_type="addr-fetch")
43+        info = node.getpeerinfo()
44+        assert_equal(len(info), 1)
45+        assert_equal(info[0]['connection_type'], 'addr-fetch')
46+
47+        self.log.info('Check that we send getaddr but don\'t try to sync headers with the addr-fetch peer')

jonatack commented at 3:32 pm on June 3, 2021:

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

in test/functional/p2p_addrfetch.py:48 in d728c6c871 outdated

43+        info = node.getpeerinfo()
44+        assert_equal(len(info), 1)
45+        assert_equal(info[0]['connection_type'], 'addr-fetch')
46+
47+        self.log.info('Check that we send getaddr but don\'t try to sync headers with the addr-fetch peer')
48+        peer.sync_send_with_ping

jonatack commented at 3:32 pm on June 3, 2021:

0        peer.sync_send_with_ping()

mzumsande commented at 4:35 pm on June 4, 2021:

oops, thanks!

in test/functional/p2p_addrfetch.py:60 in d728c6c871 outdated

55+        msg.addrs = [ADDR]
56+        peer.send_and_ping(msg)
57+        assert_equal(len(node.getpeerinfo()), 1)
58+
59+        self.log.info('Check that answering with larger addr messages leads to disconnect')
60+        msg.addrs = [ADDR]*2

jonatack commented at 3:33 pm on June 3, 2021:

PEP8

0        msg.addrs = [ADDR] * 2

in test/functional/p2p_addrfetch.py:64 in d728c6c871 outdated

59+        self.log.info('Check that answering with larger addr messages leads to disconnect')
60+        msg.addrs = [ADDR]*2
61+        peer.send_message(msg)
62+        peer.wait_for_disconnect(timeout=5)
63+
64+        self.log.info('Check timeout for addr-fetch peer that do not send addrs')

jonatack commented at 3:33 pm on June 3, 2021:

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

mzumsande commented at 4:36 pm on June 4, 2021:

fixed

in test/functional/p2p_addrfetch.py:67 in d728c6c871 outdated

62+        peer.wait_for_disconnect(timeout=5)
63+
64+        self.log.info('Check timeout for addr-fetch peer that do not send addrs')
65+        peer = node.add_outbound_p2p_connection(AddrFetchPeer(), p2p_idx=1, connection_type="addr-fetch")
66+        node.setmocktime(int(time.time()) + 130)  # Timeout: 2 min
67+        peer.sync_send_with_ping

jonatack commented at 3:34 pm on June 3, 2021:

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

mzumsande commented at 4:46 pm on June 4, 2021:

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.

in test/functional/test_framework/test_node.py:561 in d728c6c871 outdated

556@@ -557,9 +557,9 @@ def add_p2p_connection(self, p2p_conn, *, wait_for_verack=True, **kwargs):
557         return p2p_conn
558 
559     def add_outbound_p2p_connection(self, p2p_conn, *, p2p_idx, connection_type="outbound-full-relay", **kwargs):
560-        """Add an outbound p2p connection from node. Either
561-        full-relay("outbound-full-relay") or
562-        block-relay-only("block-relay-only") connection.
563+        """Add an outbound p2p connection from node. Must be a
564+        full-relay("outbound-full-relay"), block-relay-only("block-relay-only")

jonatack commented at 3:34 pm on June 3, 2021:

0        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.

mzumsande commented at 4:48 pm on June 4, 2021:

Took the short version, that should be clear enough.

in src/net.cpp:1182 in d728c6c871 outdated

1180+        case ConnectionType::BLOCK_RELAY:
1181+            max_connections = m_max_outbound_block_relay;
1182+        // no limit for ADDR_FETCH because -seednode has no limit either
1183+        case ConnectionType:: ADDR_FETCH:
1184+            max_connections = 0;
1185+    }

jonatack commented at 3:49 pm on June 3, 2021:

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

mzumsande commented at 4:49 pm on June 4, 2021:

done

jonatack commented at 3:54 pm on June 3, 2021: contributor

Approach ACK d728c6c8717960d4cbc5c2ea10833ce997d7ae1b some optional suggestions below, nice find and thanks for adding the test coverage.

mzumsande force-pushed on Jun 4, 2021

mzumsande commented at 4:54 pm on June 4, 2021: contributor

Thanks for the review @jonatack! I took your suggestions and pushed an update.

mzumsande force-pushed on Jun 7, 2021

mzumsande force-pushed on Jun 8, 2021

sipa commented at 5:38 am on June 8, 2021: member

utACK 535c990e16c6c191aa1b484b800a16bf5e4d0765

in test/functional/p2p_addrfetch.py:30 in 535c990e16 outdated

25+        super().__init__()
26+        self.received_getheaders = False
27+        self.received_getaddr = False
28+
29+    def on_getheaders(self, message): self.received_getheaders = True
30+    def on_getaddr(self, message): self.received_getaddr = True

jnewbery commented at 10:17 am on June 9, 2021:

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

 0diff --git a/test/functional/p2p_addrfetch.py b/test/functional/p2p_addrfetch.py
 1index b50a710adb..e50e9e7ee4 100755
 2--- a/test/functional/p2p_addrfetch.py
 3+++ b/test/functional/p2p_addrfetch.py
 4@@ -9,7 +9,7 @@ Test p2p addr-fetch connections
 5 import time
 6 
 7 from test_framework.messages import msg_addr, CAddress, NODE_NETWORK, NODE_WITNESS
 8-from test_framework.p2p import P2PInterface
 9+from test_framework.p2p import P2PInterface, p2p_lock
10 from test_framework.test_framework import BitcoinTestFramework
11 from test_framework.util import assert_equal
12 
13@@ -19,17 +19,6 @@ ADDR.nServices = NODE_NETWORK | NODE_WITNESS
14 ADDR.ip = "192.0.0.8"
15 ADDR.port = 18444
16 
17-
18-class AddrFetchPeer(P2PInterface):
19-    def __init__(self):
20-        super().__init__()
21-        self.received_getheaders = False
22-        self.received_getaddr = False
23-
24-    def on_getheaders(self, message): self.received_getheaders = True
25-    def on_getaddr(self, message): self.received_getaddr = True
26-
27-
28 class P2PAddrFetch(BitcoinTestFramework):
29 
30     def set_test_params(self):
31@@ -39,15 +28,16 @@ class P2PAddrFetch(BitcoinTestFramework):
32     def run_test(self):
33         node = self.nodes[0]
34         self.log.info("Connect to an addr-fetch peer")
35-        peer = node.add_outbound_p2p_connection(AddrFetchPeer(), p2p_idx=0, connection_type="addr-fetch")
36+        peer = node.add_outbound_p2p_connection(P2PInterface(), p2p_idx=0, connection_type="addr-fetch")
37         info = node.getpeerinfo()
38         assert_equal(len(info), 1)
39         assert_equal(info[0]['connection_type'], 'addr-fetch')
40 
41         self.log.info("Check that we send getaddr but don't try to sync headers with the addr-fetch peer")
42         peer.sync_send_with_ping()
43-        assert peer.received_getaddr
44-        assert not peer.received_getheaders
45+        with p2p_lock:
46+            assert peer.message_count['getaddr'] >= 1
47+            assert peer.message_count['getheaders'] == 0
48 
49         self.log.info("Check that answering the getaddr with a single address does not lead to disconnect")
50         # This prevents disconnecting on self-announcements
51@@ -62,7 +52,7 @@ class P2PAddrFetch(BitcoinTestFramework):
52         peer.wait_for_disconnect(timeout=5)
53 
54         self.log.info("Check timeout for addr-fetch peer that does not send addrs")
55-        peer = node.add_outbound_p2p_connection(AddrFetchPeer(), p2p_idx=1, connection_type="addr-fetch")
56+        peer = node.add_outbound_p2p_connection(P2PInterface(), p2p_idx=1, connection_type="addr-fetch")
57         node.setmocktime(int(time.time()) + 121)  # Timeout: 2 minutes
58         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)

mzumsande commented at 8:45 pm on June 10, 2021:

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

jnewbery commented at 10:32 am on June 9, 2021: contributor

Code review ACK 535c990e16c6c191aa1b484b800a16bf5e4d0765

One suggestion to simplify the test

mzumsande force-pushed on Jun 10, 2021

jnewbery commented at 8:48 am on June 11, 2021: contributor

Code review ACK 8dbf2b210dbdaddc11295b926fefddca127d7e8a

amitiuttarwar commented at 4:12 am on June 16, 2021: contributor

code review ACK 8dbf2b210d

mzumsande commented at 7:04 am on June 16, 2021: contributor

Analyzing #22243 gave me the concern that a timeout of 4*AVG_ADDRESS_BROADCAST_INTERVAL = 2min may be too low, given the distribution of PoissonNextSend(), leading to the node disconnecting early before the peer would send an addr too often. I’ll do some simulations later today to confirm.

p2p: Add timeout for AddrFetch peers

If AddrFetch peers don't send us addresses, disconnect them after
a while.

533500d907

mzumsande force-pushed on Jun 17, 2021

mzumsande commented at 8:11 pm on June 17, 2021: contributor

I changed the timeout to 10 * AVG_ADDRESS_BROADCAST_INTERVAL = 5min. Simulated this, and while the probability of failure (i.e. disconnecting before the peer has scheduled its getaddr answer) was ~1.8% before, it is now of order 10^-5 which I think is fine.

practicalswift commented at 8:15 pm on June 17, 2021: contributor

Concept ACK

Good catch!

amitiuttarwar commented at 10:16 pm on June 17, 2021: contributor

reACK 96061d3410898d4759488e50d1f78211849ede50

naumenkogs commented at 9:55 am on June 18, 2021: member

Concept ACK

jnewbery commented at 12:34 pm on June 18, 2021: contributor

Code review ACK 96061d3410898d4759488e50d1f78211849ede50

in test/functional/p2p_addrfetch.py:53 in 96061d3410 outdated

48+        assert_equal(len(node.getpeerinfo()), 1)
49+
50+        self.log.info("Check that answering with larger addr messages leads to disconnect")
51+        msg.addrs = [ADDR] * 2
52+        peer.send_message(msg)
53+        peer.wait_for_disconnect(timeout=5)

lsilva01 commented at 7:17 pm on June 21, 2021:

Is the assert_equal() necessary for the validation?

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

amitiuttarwar commented at 0:53 am on June 22, 2021:

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.

mzumsande commented at 9:54 pm on July 1, 2021:

I agree that wait_for_disconnect should be sufficient.

in test/functional/p2p_addrfetch.py:58 in 96061d3410 outdated

53+        peer.wait_for_disconnect(timeout=5)
54+
55+        self.log.info("Check timeout for addr-fetch peer that does not send addrs")
56+        peer = node.add_outbound_p2p_connection(P2PInterface(), p2p_idx=1, connection_type="addr-fetch")
57+        node.setmocktime(int(time.time()) + 301)  # Timeout: 5 minutes
58+        peer.wait_for_disconnect(timeout=5)

lsilva01 commented at 7:18 pm on June 21, 2021:

Is the assert_equal() necessary for the validation?

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

mzumsande commented at 9:56 pm on July 1, 2021:

same as here, wait_for_disconnect should be sufficient.

lsilva01 approved

lsilva01 commented at 7:18 pm on June 21, 2021: contributor

re-ACK https://github.com/bitcoin/bitcoin/pull/22096/commits/96061d3410898d4759488e50d1f78211849ede50

luke-jr referenced this in commit bbadd5609d on Jun 27, 2021

luke-jr referenced this in commit 95630d11d1 on Jun 27, 2021

luke-jr referenced this in commit a7db7eccaa on Jun 27, 2021

luke-jr referenced this in commit 63d49ef5b6 on Jun 27, 2021

in src/net_processing.cpp:4367 in 533500d907 outdated

4363@@ -4364,6 +4364,12 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
4364 
4365     const auto current_time = GetTime<std::chrono::microseconds>();
4366 
4367+    if (pto->IsAddrFetchConn() && current_time - std::chrono::seconds(pto->nTimeConnected) > 10 * AVG_ADDRESS_BROADCAST_INTERVAL) {

naumenkogs commented at 7:51 am on July 9, 2021:

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?

naumenkogs commented at 7:55 am on July 9, 2021:

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?

mzumsande commented at 10:59 am on July 9, 2021:

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.

naumenkogs commented at 7:30 am on July 12, 2021:

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.

naumenkogs commented at 7:45 am on July 12, 2021:

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 :)

amitiuttarwar commented at 11:02 pm on July 12, 2021:

@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.

naumenkogs commented at 8:36 am on July 13, 2021:

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?

amitiuttarwar commented at 5:56 pm on July 13, 2021:

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.

mzumsande commented at 0:47 am on July 14, 2021:

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)”?

in src/net_processing.cpp:2781 in b6c5d1e450 outdated

2778@@ -2779,7 +2779,9 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
2779         }
2780         m_addrman.Add(vAddrOk, pfrom.addr, 2 * 60 * 60);
2781         if (vAddr.size() < 1000) peer->m_getaddr_sent = false;

naumenkogs commented at 8:03 am on July 9, 2021:

Does this work as intended though?

Alice connects to Bob, each other send GETADDR, m_getaddr_sent=true
Alice self-announces, Bob sets m_getaddr_sent=false
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.

naumenkogs commented at 8:16 am on July 9, 2021:

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

mzumsande commented at 11:07 am on July 9, 2021:

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…

in src/net.cpp:1191 in 129b7a8715 outdated

1190     int existing_connections = WITH_LOCK(cs_vNodes,
1191                                          return std::count_if(vNodes.begin(), vNodes.end(), [conn_type](CNode* node) { return node->m_conn_type == conn_type; }););
1192 
1193     // Max connections of specified type already exist
1194-    if (existing_connections >= max_connections) return false;
1195+    if (max_connections && existing_connections >= max_connections) return false;

naumenkogs commented at 8:13 am on July 9, 2021:

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

mzumsande commented at 0:32 am on July 12, 2021:

Done.

net, rpc: Enable AddrFetch connections for functional testing

Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org>

c34ad3309f

test: Add functional test for AddrFetch connections

Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org>

5730a43703

mzumsande force-pushed on Jul 12, 2021

in src/net.cpp:1191 in 5730a43703

1190     int existing_connections = WITH_LOCK(cs_vNodes,
1191                                          return std::count_if(vNodes.begin(), vNodes.end(), [conn_type](CNode* node) { return node->m_conn_type == conn_type; }););
1192 
1193     // Max connections of specified type already exist
1194-    if (existing_connections >= max_connections) return false;
1195+    if (max_connections != std::nullopt && existing_connections >= max_connections) return false;

jnewbery commented at 11:36 am on July 12, 2021:

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

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

mzumsande commented at 0:48 am on July 14, 2021:

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.

maflcko commented at 10:34 am on July 26, 2021:

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.

mzumsande commented at 9:54 pm on July 27, 2021:

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.

maflcko commented at 8:05 am on July 28, 2021:

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

jnewbery commented at 11:37 am on July 12, 2021: contributor

ACK 5730a43703

amitiuttarwar commented at 11:03 pm on July 12, 2021: contributor

reACK 5730a43703f7e5a5ca26245ba3b55fbdd027d0b6

naumenkogs commented at 11:42 am on July 20, 2021: member

ACK 5730a43703f7e5a5ca26245ba3b55fbdd027d0b6

fanquake merged this on Jul 20, 2021

fanquake closed this on Jul 20, 2021

laanwj commented at 12:59 pm on July 20, 2021: member

Posthumous ACK 5730a43703f7e5a5ca26245ba3b55fbdd027d0b6

sidhujag referenced this in commit 8a3dbaa111 on Jul 23, 2021

in test/functional/p2p_addrfetch.py:57 in 5730a43703

52+        peer.send_message(msg)
53+        peer.wait_for_disconnect(timeout=5)
54+
55+        self.log.info("Check timeout for addr-fetch peer that does not send addrs")
56+        peer = node.add_outbound_p2p_connection(P2PInterface(), p2p_idx=1, connection_type="addr-fetch")
57+        node.setmocktime(int(time.time()) + 301)  # Timeout: 5 minutes

jonatack commented at 4:07 pm on July 28, 2021:

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

jonatack commented at 4:08 pm on July 28, 2021: contributor

Post-humous ACK 5730a43703f7e5a5ca26245ba3b55fbdd027d0b6

maflcko referenced this in commit 999f8b24cc on Aug 19, 2021

Fabcien referenced this in commit 7cb864e194 on Jan 28, 2022

gwillen referenced this in commit fc9e070952 on Jun 1, 2022

bitcoin locked this on Aug 18, 2022

mzumsande deleted the branch on Oct 13, 2023

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