This extends the functional test p2p_addr_relay.py.
It adds test coverage for address relay involving outbound peers, tests for both outgoing and incoming GETADDR requests and tests for -blocksonly mode.
The initial refactors and some of the new tests were taken from Amiti Uttarwar’s PR #21528 - they are general test improvements not directly tied to the change proposed there.
157+ def getaddr_tests(self):
158+ self.log.info('Test getaddr behavior')
159+ self.log.info('Check that we send a getaddr message to outbound-full-relay peers')
160+ full_outbound_peer = self.nodes[0].add_outbound_p2p_connection(GetAddrStore(), p2p_idx=0, connection_type="outbound-full-relay")
161+ full_outbound_peer.sync_with_ping()
162+ assert(full_outbound_peer.getaddr_received)
style nit, here and line 202
0 assert full_outbound_peer.getaddr_received
172+
173+ # Add some addresses to addrman
174+ for i in range(1000):
175+ first_octet = i >> 8
176+ second_octet = i % 256
177+ a = "{}.{}.1.1".format(first_octet, second_octet)
We can now use Python f-strings for interpolation (here and e.g. line 70 above).
0 a = f"{first_octet}.{second_octet}.1.1"
121@@ -105,6 +122,42 @@ def relay_tests(self):
122
123 self.nodes[0].disconnect_p2ps()
124
125+ def getaddr_tests(self):
126+ self.log.info('Test getaddr behavior')
127+ self.log.info('Check that we send a getaddr message to outbound-full-relay peers')
0 self.log.info('Check that we send a getaddr message upon connecting to an outbound-full-relay peer')
127+ self.log.info('Check that we send a getaddr message to outbound-full-relay peers')
128+ full_outbound_peer = self.nodes[0].add_outbound_p2p_connection(GetAddrStore(), p2p_idx=0, connection_type="outbound-full-relay")
129+ full_outbound_peer.sync_with_ping()
130+ assert(full_outbound_peer.getaddr_received)
131+
132+ self.log.info('Check that we do not send a getaddr message to block-relay-only peers')
same as previous,
0 self.log.info('Check that we do not send a getaddr message upon connecting to a block-relay-only peer')
140+ # Add some addresses to addrman
141+ for i in range(1000):
142+ first_octet = i >> 8
143+ second_octet = i % 256
144+ a = "{}.{}.1.1".format(first_octet, second_octet)
145+ self.nodes[0].addpeeraddress(a, 8333)
p2p_getaddr_caching.py :smile:
77@@ -78,7 +78,7 @@ def setup_addr_msg(self, num):
78 def send_addr_msg(self, source, msg, receivers):
79 source.send_and_ping(msg)
80 # pop m_next_addr_send timer
81- self.mocktime += 30 * 60
140+ self.send_addr_msg(full_outbound_peer, msg2, [inbound_peer])
141+ assert_equal(inbound_peer.num_ipv4_received, 2)
142+
143+ self.log.info('Check address relay to outbound peers')
144+ block_relay_peer = self.nodes[0].add_outbound_p2p_connection(GetAddrStore(), p2p_idx=1, connection_type="block-relay-only")
145+ block_relay_peer.sync_with_ping()
sync_with_ping() necessary?
send_addr_msg in the next line. Removed.
133+ # from a new outbound peer to be relayed to others. Originally meant to prevent
134+ # large GETADDR responses from being relayed, it now typically affects the self-announcement
135+ # of the outbound peer which is often sent before the GETADDR response.
136+ assert_equal(inbound_peer.num_ipv4_received, 0)
137+
138+ self.log.info('Check that further addr messages sent from an outbound peer are relayed')
s/further/subsequent? later?
my brain keeps thinking “further” relates to how far the message propagates through network nodes.
thank you for working on this! glad to see this increased test coverage for addr handling.
overall looks good to me, some small questions / wording suggestions.
the commit message for 604128088e says “[test] Extract setting up addr message into a helper”, but it really extracts sending addr message, with the previous commit extracting the set up. this was my fault, but I just noticed it now 😛
RE commit 6eb3024f14 adding coverage for the ignore-first-message-from-outbound-peer: since this is the current behavior, it seems reasonable to add coverage (and thanks for adding the explanation), but just want to mention that I find this bitcoin core behavior super weird and think we should consider changing it. Either bitcoin core should be consistent with itself and propagate those initial self announcements, or it shouldn’t self announce at all. From researching the addr behavior of alternative clients for #21528, I saw that a lot of nodes just do one early self-advertisement, so its unfortunate if bitcoin core nodes are just black-holing that as a side effect of getaddr logic. This is all out of scope of this PR but I’m mentioning since the test coverage highlights the behavior.
tACK 11c79242d2, reviewed the code & ran tests on every commit. happy to re-ack if you edit based on review.
Moves setting up the addr message into a repeatable function, and breaks up the
existing tests into separate functions for legibility.
Also reduces mocktime to prevent idle disconnects
Co-Authored-By: Martin Zumsande <mzumsande@gmail.com>
Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org>
Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org>
Co-Authored-By: Martin Zumsande <mzumsande@gmail.com>
@jonatack , @amitiuttarwar Thanks a lot for the reviews! I addressed all comments above.
the commit message for 6041280 says “[test] Extract setting up addr message into a helper”, but it really extracts sending addr message, with the previous commit extracting the set up. this was my fault, but I just noticed it now
I changed the commit message and also added a note why mocktime is changed in this commit.
RE commit 6eb3024 adding coverage for the ignore-first-message-from-outbound-peer: since this is the current behavior, it seems reasonable to add coverage (and thanks for adding the explanation), but just want to mention that I find this bitcoin core behavior super weird and think we should consider changing it. Either bitcoin core should be consistent with itself and propagate those initial self announcements, or it shouldn’t self announce at all. From researching the addr behavior of alternative clients for #21528, I saw that a lot of nodes just do one early self-advertisement, so its unfortunate if bitcoin core nodes are just black-holing that as a side effect of getaddr logic. This is all out of scope of this PR but I’m mentioning since the test coverage highlights the behavior.
I agree, the fGetAddr flag preventing this was initially meant to prevent relaying GETADDR responses - at some time the typical order of messages must have changed, and now it typically affects self-announcements. I discovered this unintended behavior during #19794, which would have removed it.
I closed that PR and didn’t open a follow-up as originally planned because after reading additional BIP155 discussion (especially this post by sipa) I realized that even addrs that are part extraordinarily small GETADDR responses <=10 wouldn’t get relayed due to the 10min recency limit anyway (and if they are recent enough, why not relay them?). The suggestion of keeping the flag around and making sure that it would apply to GETADDR responses again didn’t really convince me anymore after that. (as opposed to just removing the flag and relying on the content of the addr messages themselves instead of timing)
re-ACK a732ee353c, small diff based on code review
I agree, the fGetAddr flag preventing this was …
thanks for that explanation, I’ll take a closer look!
tACK and reviewed the code (seems good).
tested it on MacOS 11.1, ran several times on every commit
Concept ACK a732ee353c1922a1f9ca082775884d190893e0e9 🌊
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3Concept ACK a732ee353c1922a1f9ca082775884d190893e0e9 🌊
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUinXAv/fV9qz2YWTlsh45kR8eeKCPJBEjTUqUAad/RtPpxDGxd43H0EilfP8VzG
87SLZlK5N4uRuL9EaRqxwV7d+ByHiKf+ARdicmPd7gjNg+FDgyDs/hZk5g9XsGWUh
9vOrEC3MDHdgWtgYMnHAE9bBLYM/8HnxBy23FaAk4GORmIo1DImKXmh/ABFhftZ62
10gslGncJYt0m/QqCE85BspFtbwvR5nRrOlng1GkYKsKpOypH2xr4oZHttWQJSWDIE
11oK8F6rXBWHX09Yh2uLpZ0reVXl44hKdUeoTzYxIh78zyPzMRFqQ+GWBNH7FvY8B3
12YCtDv6/YuHqf+iExqri2Rgm5wVwQu9wBNf1APcoSzxVVIN2lBVMAlxoDlj7Ne3VS
13bB4wZauKC4jFUmDGlFwrmX0iY7S4KouFAmM6F2JbW6QDYDIWWaLl4ti+uAtfaLCr
14UddKbFtrhb7B5bZM/7o2TBrswflVNFwkTC99IFKAGB5z606NSFYy9dbuGMEFDfZm
15JoLhOr8n
16=PXgI
17-----END PGP SIGNATURE-----
Timestamp of file with hash 799645bbc5af6a827e9e28310a9332d8c563094883569c68faf6c2f08baa24c2 -
204+ addr_source = self.nodes[0].add_p2p_connection(P2PInterface())
205+ msg = self.setup_addr_msg(2)
206+ addr_source.send_and_ping(msg)
207+ self.mocktime += 5 * 60
208+ self.nodes[0].setmocktime(self.mocktime)
209+ full_outbound_peer.sync_with_ping()
send_addr_msg() helper here?
utACK a732ee353c1922a1f9ca082775884d190893e0e9
Nice additions. Thank you!
137+ assert_equal(inbound_peer.num_ipv4_received, 0)
138+
139+ self.log.info('Check that subsequent addr messages sent from an outbound peer are relayed')
140+ msg2 = self.setup_addr_msg(2)
141+ self.send_addr_msg(full_outbound_peer, msg2, [inbound_peer])
142+ assert_equal(inbound_peer.num_ipv4_received, 2)
The expected addr message is sent directly after the assert has failed, so it looks like a timing issue:
node0 2021-04-27T06:55:10.562357Z (mocktime: 2021-04-27T07:10:08Z) [net.cpp:2967] [PushMessage] sending addr (61 bytes) peer=9 (https://cirrus-ci.com/task/4959397244829696?logs=ci#L6320)
With AVG_ADDRESS_BROADCAST_INTERVAL=30s, I thought that a mocktime of 5 mins should be plenty to prevent Poisson timer fails - I think it’s not that.
Possibly, there needs to be an additional wait after the mocktime to give the node more time to actually send out the addr message?
I’ll look into this more closely later today!