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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34039.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
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).
inspired by https://github.com/bitcoin/bitcoin/blob/cca113f5b022b131a78579ab233d0d40643c7c2b/test/functional/p2p_addr_relay.py#L130-L133 (using the same exponent of e^-20 here)
nothing for this PR, but since these things are quite common (recently #33121), I wonder if a mode -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)
nit: The whole test relies on mocktime, so is there any value in resetting it between the test cases? Also you can use bumpmocktime to avoid the manual += + setmocktime.
Done! Wasn't aware of 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):
def __init__(self, *, expected, addrv2):
nit: To force named args, which you are already using.
TIL! done.
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.
first_octet = 1 + i
second_octet = i
done!
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.
diff --git a/test/functional/p2p_addr_selfannouncement.py b/test/functional/p2p_addr_selfannouncement.py
index 65527d1e77..47de6156f8 100755
--- a/test/functional/p2p_addr_selfannouncement.py
+++ b/test/functional/p2p_addr_selfannouncement.py
@@ -69,62 +69,28 @@ class AddrSelfAnnouncementTest(BitcoinTestFramework):
a = f"{first_octet}.{second_octet}.1.1"
self.nodes[0].addpeeraddress(a, 8333)
- self.self_announcement_inbound_test(addrv2=False)
- self.self_announcement_inbound_test(addrv2=True)
- self.self_announcement_outbound_test(addrv2=False)
- self.self_announcement_outbound_test(addrv2=True)
-
- def self_announcement_inbound_test(self, addrv2=False):
- addr_version = "addrv2" if addrv2 else "addrv1"
- self.log.info(f"Test that the node does an address self-announcement to inbound connections ({addr_version})")
-
- # We only self-announce after initial block download is done
- assert (not self.nodes[0].getblockchaininfo()["initialblockdownload"])
-
- netinfo = self.nodes[0].getnetworkinfo()
- port = netinfo["localaddresses"][0]["port"]
- self.nodes[0].setmocktime(int(time.time()))
-
- expected = CAddress()
- expected.nServices = int(netinfo["localservices"], 16)
- expected.ip = IP_TO_ANNOUNCE
- expected.port = port
- expected.time = self.nodes[0].mocktime
-
- self.log.info(f"Check that we get an initial self-announcement when connecting to a node and sending a GETADDR (inbound, {addr_version})")
- with self.nodes[0].assert_debug_log([f'Advertising address {IP_TO_ANNOUNCE}:{port}']):
- addr_receiver = self.nodes[0].add_p2p_connection(SelfAnnouncementReceiver(expected=expected, addrv2=addrv2))
- addr_receiver.sync_with_ping()
+ self.self_announcement_test(outbound=False, addrv2=False, assert_on_connection_open=self.inbound_connection_open_assertions)
+ self.self_announcement_test(outbound=False, addrv2=True, assert_on_connection_open=self.inbound_connection_open_assertions)
+ self.self_announcement_test(outbound=True, addrv2=False, assert_on_connection_open=self.outbound_connection_open_assertions)
+ self.self_announcement_test(outbound=True, addrv2=True, assert_on_connection_open=self.outbound_connection_open_assertions)
+ def inbound_connection_open_assertions(self, addr_receiver):
# We expect one self-announcement and multiple other addresses in
# response to a GETADDR in a single addr / addrv2 message.
assert_equal(addr_receiver.self_announcements_received, 1)
assert_equal(addr_receiver.addr_messages_received, 1)
assert_greater_than(addr_receiver.addresses_received, 1)
- self.log.info(f"Check that we get more self-announcements sometime later (inbound, {addr_version})")
- for _ in range(5):
- last_self_announcements_received = addr_receiver.self_announcements_received
- last_addr_messages_received = addr_receiver.addr_messages_received
- last_addresses_received = addr_receiver.addresses_received
- with self.nodes[0].assert_debug_log([f'Advertising address {IP_TO_ANNOUNCE}:{port}']):
- # m_next_local_addr_send and AVG_LOCAL_ADDRESS_BROADCAST_INTERVAL:
- # self-announcements are sent on an exponential distribution with mean interval of 24h.
- # Setting the mocktime 20d forward gives a probability of (1 - e^-(480/24)) that
- # the event will occur (i.e. this fails once in ~500 million repeats).
- self.nodes[0].bumpmocktime(20 * ONE_DAY)
- addr_receiver.expected.time = self.nodes[0].mocktime
- addr_receiver.sync_with_ping()
-
- assert_equal(addr_receiver.self_announcements_received, last_self_announcements_received + 1)
- assert_equal(addr_receiver.addr_messages_received, last_addr_messages_received + 1)
- assert_equal(addr_receiver.addresses_received, last_addresses_received + 1)
-
- self.nodes[0].disconnect_p2ps()
+ def outbound_connection_open_assertions(self, addr_receiver):
+ # We expect only the self-announcement.
+ assert_equal(addr_receiver.self_announcements_received, 1)
+ assert_equal(addr_receiver.addr_messages_received, 1)
+ assert_equal(addr_receiver.addresses_received, 1)
- def self_announcement_outbound_test(self, addrv2=True):
+ def self_announcement_test(self, outbound, addrv2, assert_on_connection_open):
+ connection_type = "outbound" if outbound else "inbound"
addr_version = "addrv2" if addrv2 else "addrv1"
- self.log.info(f"Test that the node does an address self-announcement to outbound connections ({addr_version})")
+ self.log.info(f"Test that the node does an address self-announcement to {connection_type} connections ({addr_version})")
# We only self-announce after initial block download is done
assert (not self.nodes[0].getblockchaininfo()["initialblockdownload"])
@@ -139,21 +105,19 @@ class AddrSelfAnnouncementTest(BitcoinTestFramework):
expected.port = port
expected.time = self.nodes[0].mocktime
- self.log.info(f"Check that we get an initial self-announcement on a outbound connection from the node (outbound, {addr_version})")
+ 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})")
with self.nodes[0].assert_debug_log([f'Advertising address {IP_TO_ANNOUNCE}:{port}']):
- addr_receiver = self.nodes[0].add_outbound_p2p_connection(SelfAnnouncementReceiver(expected=expected, addrv2=addrv2), p2p_idx=0, connection_type="outbound-full-relay")
+ 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))
addr_receiver.sync_with_ping()
- # We expect only the self-announcement.
- assert_equal(addr_receiver.self_announcements_received, 1)
- assert_equal(addr_receiver.addr_messages_received, 1)
- assert_equal(addr_receiver.addresses_received, 1)
+ assert_on_connection_open(addr_receiver)
- # to avoid the node evicting the outbound peer, protect it by announcing the most recent header to it
- tip_header = from_hex(CBlockHeader(), self.nodes[0].getblockheader(self.nodes[0].getbestblockhash(), False))
- addr_receiver.send_and_ping(msg_headers([tip_header]))
+ if outbound:
+ # to avoid the node evicting the outbound peer, protect it by announcing the most recent header to it
+ tip_header = from_hex(CBlockHeader(), self.nodes[0].getblockheader(self.nodes[0].getbestblockhash(), False))
+ addr_receiver.send_and_ping(msg_headers([tip_header]))
- self.log.info(f"Check that we get more self-announcements sometime later (outbound, {addr_version})")
+ self.log.info(f"Check that we get more self-announcements sometime later ({connection_type}, {addr_version})")
for _ in range(5):
last_self_announcements_received = addr_receiver.self_announcements_received
last_addr_messages_received = addr_receiver.addr_messages_received
@@ -173,6 +137,5 @@ class AddrSelfAnnouncementTest(BitcoinTestFramework):
self.nodes[0].disconnect_p2ps()
-
if __name__ == '__main__':
AddrSelfAnnouncementTest(__file__).main()
Not sure if this is the best way to do it but I also had the feeling that some deduplication should be possible here among the inbound and outbound test
Thanks rkrux, included!
Code review fb9c393564ef098267ad2a921b75e0716f38b047
Concept ACK
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.
I would suggest to rework or remove this before merge, first person language and print() don't seem appropriate at that stage anymore, the mismatch print could be an assert if it's not expected to ever happen.
Dropped it.
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.
done.
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})")
nit: an outbound
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)
note: I first thought that there might be a race condition (node receives verack first, and getaddr with some delay, sends out self-advertisment in between) but this isn't actually true because in this case we won't self-advertise due to #21528 (SetupAddressRelay not called yet) . So this is fine for now (whether the node behavior of mixing these is good is another issue)
Concept ACK
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>
Addressed comments by mzumsande
ACK 1841bf9
Thanks for the co-author attribution.
11 | + 12 | +from test_framework.messages import ( 13 | + CAddress, 14 | + from_hex, 15 | + msg_headers, 16 | + CBlockHeader,
nit: Should be below CAddress
80 | + assert_equal(addr_receiver.self_announcements_received, 1) 81 | + assert_equal(addr_receiver.addr_messages_received, 1) 82 | + assert_equal(addr_receiver.addresses_received, 1) 83 | + 84 | + def self_announcement_test(self, *, outbound, addrv2): 85 | + connection_type = "outbound" if outbound else "inbound"
nit: Could have passed the strings as arguments directly instead of interpreting the bools, this would make the call-site a bit easier to read as well I think.
85 | + connection_type = "outbound" if outbound else "inbound" 86 | + addr_version = "addrv2" if addrv2 else "addrv1" 87 | + self.log.info(f"Test that the node does an address self-announcement to {connection_type} connections ({addr_version})") 88 | + 89 | + # We only self-announce after initial block download is done 90 | + assert (not self.nodes[0].getblockchaininfo()["initialblockdownload"])
Is this really needed? Since we are loading the preconfigured chain here I don't think we could ever hit this. I guess this is here rather for documentation purposes? Even if it is a problem then this could prbably move to run_test to just run once.
130 | + # the event will occur (i.e. this fails once in ~500 million repeats). 131 | + self.nodes[0].bumpmocktime(20 * ONE_DAY) 132 | + addr_receiver.expected.time = self.nodes[0].mocktime 133 | + addr_receiver.sync_with_ping() 134 | + 135 | + assert_equal(addr_receiver.self_announcements_received, last_self_announcements_received + 1)
nit: This mostly repeats the checks from inbound_connection_open_assertions and its outbound counterpart with different numbers. I think there could be some further de-duplication where you replace it with a more generic method on the addr_receiver just called connection_open_assertions and then give it the number as args. But it's not a big deal.
117 | + # to avoid the node evicting the outbound peer, protect it by announcing the most recent header to it 118 | + tip_header = from_hex(CBlockHeader(), self.nodes[0].getblockheader(self.nodes[0].getbestblockhash(), False)) 119 | + addr_receiver.send_and_ping(msg_headers([tip_header])) 120 | + 121 | + self.log.info(f"Check that we get more self-announcements sometime later ({connection_type}, {addr_version})") 122 | + for _ in range(5):
Just curious: Why do this 5 times? Is there any special significance to it? Based on the log above I would have expected the loop to exit once a first self-announcement has been observed.
Maybe the info log could be reworded a bit to "Check that we get repeated self-announcements periodically".
111 | + if outbound: 112 | + self.outbound_connection_open_assertions(addr_receiver) 113 | + else: 114 | + self.inbound_connection_open_assertions(addr_receiver) 115 | + 116 | + if outbound:
nit: Could move the code block in this if into the above outbound conditional.
Code review ACK 1841bf9cb67bca99273b3efa38633113422f5882
Left a bunch of nits and minor questions. But I think this is also fine to merge as is.
ACK 1841bf9cb67bca99273b3efa38633113422f5882