Test that a node sends a self-announcement with its external IP to in- and outbound peers after connection open and again sometime later.
Since the code for the test is mostly the same for addr and addrv2 messages, I opted to add a new test file instead of having duplicate code in p2p_addr_relay.py and p2p_addrv2_relay.py.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34039.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | rkrux, Bicaru20 |
| Concept ACK | fjahr, mzumsande |
| Stale ACK | danielabrozzoni |
If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
108+ last_addresses_received = addr_receiver.addresses_received
109+ with self.nodes[0].assert_debug_log([f'Advertising address {IP_TO_ANNOUNCE}:{port}']):
110+ # m_next_local_addr_send and AVG_LOCAL_ADDRESS_BROADCAST_INTERVAL:
111+ # self-announcements are sent on an exponential distribution with mean interval of 24h.
112+ # Setting the mocktime 20d forward gives a probability of (1 - e^-(480/24)) that
113+ # the event will occur (i.e. this fails once in ~500 million repeats).
-test=cutoffexponentialtimers or something similar would make sense which would cut off the long tail of all exponential timers (rand_exp_duration) at some percentile, so we don’t need to deal with these probabilistic failures in functional tests.
177+ assert_equal(addr_receiver.addresses_received, last_addresses_received + 1)
178+
179+ self.nodes[0].disconnect_p2ps()
180+ # reset mocktime
181+ self.mocktime = int(time.time())
182+ self.nodes[0].setmocktime(0)
bumpmocktime to avoid the manual += + setmocktime.
bumpmocktime, thanks. Cleaned up use of mocktime a bit.
29+ addr_messages_received = 0
30+
31+ expected = None
32+ addrv2_test = False
33+
34+ def __init__(self, expected, addrv2):
0 def __init__(self, *, expected, addrv2):
nit: To force named args, which you are already using.
63+
64+ def run_test(self):
65+ # populate addrman to have some addresses for a GETADDR response
66+ for i in range(50):
67+ first_octet = 1 + (i >> 8)
68+ second_octet = i % 256
Since increasing the 50 in the loop will not contribute anything to the test, maybe the >> 8 and % 256 could be erased.
0 first_octet = 1 + i
1 second_octet = i
Code review ACK fb9c393564ef098267ad2a921b75e0716f38b047. I have run the test and everything checks out.
I left a small comment in the code, also fine with merging as-is
137+ expected.nServices = int(netinfo["localservices"], 16)
138+ expected.ip = IP_TO_ANNOUNCE
139+ expected.port = port
140+ expected.time = self.nodes[0].mocktime
141+
142+ self.log.info(f"Check that we get an initial self-announcement on a outbound connection from the node (outbound, {addr_version})")
In fb9c393564ef098267ad2a921b75e0716f38b047 “test: address self-announcement”
Is it intentional to mention outbound twice in this log message and deviate from the similar log message of the inbound test?
169+
170+ assert_equal(addr_receiver.self_announcements_received, last_self_announcements_received + 1)
171+ assert_equal(addr_receiver.addr_messages_received, last_addr_messages_received + 1)
172+ assert_equal(addr_receiver.addresses_received, last_addresses_received + 1)
173+
174+ self.nodes[0].disconnect_p2ps()
In fb9c393564ef098267ad2a921b75e0716f38b047 “test: address self-announcement”
I noticed some duplication in the test that could be removed with the following diff. I don’t feel too strongly about this suggestion and if you think such a de-duplicated code might make it more difficult to update the test in the future, then feel free to not implement.
0diff --git a/test/functional/p2p_addr_selfannouncement.py b/test/functional/p2p_addr_selfannouncement.py
1index 65527d1e77..47de6156f8 100755
2--- a/test/functional/p2p_addr_selfannouncement.py
3+++ b/test/functional/p2p_addr_selfannouncement.py
4@@ -69,62 +69,28 @@ class AddrSelfAnnouncementTest(BitcoinTestFramework):
5 a = f"{first_octet}.{second_octet}.1.1"
6 self.nodes[0].addpeeraddress(a, 8333)
7
8- self.self_announcement_inbound_test(addrv2=False)
9- self.self_announcement_inbound_test(addrv2=True)
10- self.self_announcement_outbound_test(addrv2=False)
11- self.self_announcement_outbound_test(addrv2=True)
12-
13- def self_announcement_inbound_test(self, addrv2=False):
14- addr_version = "addrv2" if addrv2 else "addrv1"
15- self.log.info(f"Test that the node does an address self-announcement to inbound connections ({addr_version})")
16-
17- # We only self-announce after initial block download is done
18- assert (not self.nodes[0].getblockchaininfo()["initialblockdownload"])
19-
20- netinfo = self.nodes[0].getnetworkinfo()
21- port = netinfo["localaddresses"][0]["port"]
22- self.nodes[0].setmocktime(int(time.time()))
23-
24- expected = CAddress()
25- expected.nServices = int(netinfo["localservices"], 16)
26- expected.ip = IP_TO_ANNOUNCE
27- expected.port = port
28- expected.time = self.nodes[0].mocktime
29-
30- self.log.info(f"Check that we get an initial self-announcement when connecting to a node and sending a GETADDR (inbound, {addr_version})")
31- with self.nodes[0].assert_debug_log([f'Advertising address {IP_TO_ANNOUNCE}:{port}']):
32- addr_receiver = self.nodes[0].add_p2p_connection(SelfAnnouncementReceiver(expected=expected, addrv2=addrv2))
33- addr_receiver.sync_with_ping()
34+ self.self_announcement_test(outbound=False, addrv2=False, assert_on_connection_open=self.inbound_connection_open_assertions)
35+ self.self_announcement_test(outbound=False, addrv2=True, assert_on_connection_open=self.inbound_connection_open_assertions)
36+ self.self_announcement_test(outbound=True, addrv2=False, assert_on_connection_open=self.outbound_connection_open_assertions)
37+ self.self_announcement_test(outbound=True, addrv2=True, assert_on_connection_open=self.outbound_connection_open_assertions)
38
39+ def inbound_connection_open_assertions(self, addr_receiver):
40 # We expect one self-announcement and multiple other addresses in
41 # response to a GETADDR in a single addr / addrv2 message.
42 assert_equal(addr_receiver.self_announcements_received, 1)
43 assert_equal(addr_receiver.addr_messages_received, 1)
44 assert_greater_than(addr_receiver.addresses_received, 1)
45
46- self.log.info(f"Check that we get more self-announcements sometime later (inbound, {addr_version})")
47- for _ in range(5):
48- last_self_announcements_received = addr_receiver.self_announcements_received
49- last_addr_messages_received = addr_receiver.addr_messages_received
50- last_addresses_received = addr_receiver.addresses_received
51- with self.nodes[0].assert_debug_log([f'Advertising address {IP_TO_ANNOUNCE}:{port}']):
52- # m_next_local_addr_send and AVG_LOCAL_ADDRESS_BROADCAST_INTERVAL:
53- # self-announcements are sent on an exponential distribution with mean interval of 24h.
54- # Setting the mocktime 20d forward gives a probability of (1 - e^-(480/24)) that
55- # the event will occur (i.e. this fails once in ~500 million repeats).
56- self.nodes[0].bumpmocktime(20 * ONE_DAY)
57- addr_receiver.expected.time = self.nodes[0].mocktime
58- addr_receiver.sync_with_ping()
59-
60- assert_equal(addr_receiver.self_announcements_received, last_self_announcements_received + 1)
61- assert_equal(addr_receiver.addr_messages_received, last_addr_messages_received + 1)
62- assert_equal(addr_receiver.addresses_received, last_addresses_received + 1)
63-
64- self.nodes[0].disconnect_p2ps()
65+ def outbound_connection_open_assertions(self, addr_receiver):
66+ # We expect only the self-announcement.
67+ assert_equal(addr_receiver.self_announcements_received, 1)
68+ assert_equal(addr_receiver.addr_messages_received, 1)
69+ assert_equal(addr_receiver.addresses_received, 1)
70
71- def self_announcement_outbound_test(self, addrv2=True):
72+ def self_announcement_test(self, outbound, addrv2, assert_on_connection_open):
73+ connection_type = "outbound" if outbound else "inbound"
74 addr_version = "addrv2" if addrv2 else "addrv1"
75- self.log.info(f"Test that the node does an address self-announcement to outbound connections ({addr_version})")
76+ self.log.info(f"Test that the node does an address self-announcement to {connection_type} connections ({addr_version})")
77
78 # We only self-announce after initial block download is done
79 assert (not self.nodes[0].getblockchaininfo()["initialblockdownload"])
80@@ -139,21 +105,19 @@ class AddrSelfAnnouncementTest(BitcoinTestFramework):
81 expected.port = port
82 expected.time = self.nodes[0].mocktime
83
84- self.log.info(f"Check that we get an initial self-announcement on a outbound connection from the node (outbound, {addr_version})")
85+ self.log.info(f"Check that we get an initial self-announcement when connecting to a node and sending a GETADDR ({connection_type}, {addr_version})")
86 with self.nodes[0].assert_debug_log([f'Advertising address {IP_TO_ANNOUNCE}:{port}']):
87- addr_receiver = self.nodes[0].add_outbound_p2p_connection(SelfAnnouncementReceiver(expected=expected, addrv2=addrv2), p2p_idx=0, connection_type="outbound-full-relay")
88+ addr_receiver = self.nodes[0].add_outbound_p2p_connection(SelfAnnouncementReceiver(expected=expected, addrv2=addrv2), p2p_idx=0, connection_type="outbound-full-relay") if outbound else self.nodes[0].add_p2p_connection(SelfAnnouncementReceiver(expected=expected, addrv2=addrv2))
89 addr_receiver.sync_with_ping()
90
91- # We expect only the self-announcement.
92- assert_equal(addr_receiver.self_announcements_received, 1)
93- assert_equal(addr_receiver.addr_messages_received, 1)
94- assert_equal(addr_receiver.addresses_received, 1)
95+ assert_on_connection_open(addr_receiver)
96
97- # to avoid the node evicting the outbound peer, protect it by announcing the most recent header to it
98- tip_header = from_hex(CBlockHeader(), self.nodes[0].getblockheader(self.nodes[0].getbestblockhash(), False))
99- addr_receiver.send_and_ping(msg_headers([tip_header]))
100+ if outbound:
101+ # to avoid the node evicting the outbound peer, protect it by announcing the most recent header to it
102+ tip_header = from_hex(CBlockHeader(), self.nodes[0].getblockheader(self.nodes[0].getbestblockhash(), False))
103+ addr_receiver.send_and_ping(msg_headers([tip_header]))
104
105- self.log.info(f"Check that we get more self-announcements sometime later (outbound, {addr_version})")
106+ self.log.info(f"Check that we get more self-announcements sometime later ({connection_type}, {addr_version})")
107 for _ in range(5):
108 last_self_announcements_received = addr_receiver.self_announcements_received
109 last_addr_messages_received = addr_receiver.addr_messages_received
110@@ -173,6 +137,5 @@ class AddrSelfAnnouncementTest(BitcoinTestFramework):
111
112 self.nodes[0].disconnect_p2ps()
113
114-
115 if __name__ == '__main__':
116 AddrSelfAnnouncementTest(__file__).main()
tACK fb9c393564ef098267ad2a921b75e0716f38b047
Happy to re-ACK if you decide to pick up #34039 (review) :)
Included both suggestions, thanks!
41+ for addr in message.addrs:
42+ self.addresses_received += 1
43+ if addr == self.expected:
44+ self.self_announcements_received += 1
45+ # The tricky part of this test is the timestamp of the self-announcement.
46+ # The following helped me to debug timestamp mismatches, so I'm leaving it in for the next one.
print() don’t seem appropriate at that stage anymore, the mismatch print could be an assert if it’s not expected to ever happen.
65+ # populate addrman to have some addresses for a GETADDR response
66+ for i in range(50):
67+ a = f"{1 + i}.{i}.1.1"
68+ self.nodes[0].addpeeraddress(a, 8333)
69+
70+ self.self_announcement_test(outbound=False, addrv2=False, assert_on_connection_open=self.inbound_connection_open_assertions)
assert_on_connection_open seems like a redundant arg, could call one or the other based on the existing outbound arg.
103+ expected.port = port
104+ expected.time = self.nodes[0].mocktime
105+
106+ with self.nodes[0].assert_debug_log([f'Advertising address {IP_TO_ANNOUNCE}:{port}']):
107+ if outbound:
108+ self.log.info(f"Check that we get an initial self-announcement on a outbound connection from the node ({connection_type}, {addr_version})")
75+ def inbound_connection_open_assertions(self, addr_receiver):
76+ # We expect one self-announcement and multiple other addresses in
77+ # response to a GETADDR in a single addr / addrv2 message.
78+ assert_equal(addr_receiver.self_announcements_received, 1)
79+ assert_equal(addr_receiver.addr_messages_received, 1)
80+ assert_greater_than(addr_receiver.addresses_received, 1)
SetupAddressRelay not called yet) . So this is fine for now (whether the node behavior of mixing these is good is another issue)
Test that a node sends a self-announcement with its external IP to
in- and outbound peers after connection open and again sometime later.
Since the code for the test is mostly the same for addr and addrv2
messages, I opted to add a new test file instead of having duplicate
code in p2p_addr_relay.py and p2p_addrv2_relay.py.
Co-Authored-By: rkrux <rkrux.connect@gmail.com>
ACK 1841bf9
Thanks for the co-author attribution.