test: Extend functional tests for addr relay #21707
Conversation
aacac52 to
11c7924
Compare
There was a problem hiding this comment.
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 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 😛
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.
tACK 11c7924, 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>
11c7924 to
f168187
Compare
Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org>
Co-Authored-By: Martin Zumsande <mzumsande@gmail.com>
f168187 to
a732ee3
Compare
|
@jonatack , @amitiuttarwar Thanks a lot for the reviews! I addressed all comments above.
I changed the commit message and also added a note why mocktime is changed in this commit.
I agree, the 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 a732ee3, small diff based on code review
thanks for that explanation, I'll take a closer look! |
There was a problem hiding this comment.
Concept ACK a732ee3 🌊
Show signature and timestamp
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
Concept ACK a732ee353c1922a1f9ca082775884d190893e0e9 🌊
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUinXAv/fV9qz2YWTlsh45kR8eeKCPJBEjTUqUAad/RtPpxDGxd43H0EilfP8VzG
7SLZlK5N4uRuL9EaRqxwV7d+ByHiKf+ARdicmPd7gjNg+FDgyDs/hZk5g9XsGWUh
vOrEC3MDHdgWtgYMnHAE9bBLYM/8HnxBy23FaAk4GORmIo1DImKXmh/ABFhftZ62
gslGncJYt0m/QqCE85BspFtbwvR5nRrOlng1GkYKsKpOypH2xr4oZHttWQJSWDIE
oK8F6rXBWHX09Yh2uLpZ0reVXl44hKdUeoTzYxIh78zyPzMRFqQ+GWBNH7FvY8B3
YCtDv6/YuHqf+iExqri2Rgm5wVwQu9wBNf1APcoSzxVVIN2lBVMAlxoDlj7Ne3VS
bB4wZauKC4jFUmDGlFwrmX0iY7S4KouFAmM6F2JbW6QDYDIWWaLl4ti+uAtfaLCr
UddKbFtrhb7B5bZM/7o2TBrswflVNFwkTC99IFKAGB5z606NSFYy9dbuGMEFDfZm
JoLhOr8n
=PXgI
-----END PGP SIGNATURE-----
Timestamp of file with hash 799645bbc5af6a827e9e28310a9332d8c563094883569c68faf6c2f08baa24c2 -
| self.log.info('Check that subsequent addr messages sent from an outbound peer are relayed') | ||
| msg2 = self.setup_addr_msg(2) | ||
| self.send_addr_msg(full_outbound_peer, msg2, [inbound_peer]) | ||
| assert_equal(inbound_peer.num_ipv4_received, 2) |
There was a problem hiding this comment.
There was a problem hiding this comment.
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!
Summary: ``` 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. ``` Backport of [[bitcoin/bitcoin#21707 | core#21707]], [[bitcoin/bitcoin#21785 | core#21785]] (fix intermittent failure in the test) and [[bitcoin/bitcoin#22306 | core#22306]] (improvements to p2p_addr_relay.py, also fixes an intermittent issue). Note to reviewers: you can look at the file after #22306 to get the final version of the test. Depends on D10860. Ref T1696. Test Plan: ninja check-functional Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Subscribers: PiRK Maniphest Tasks: T1696 Differential Revision: https://reviews.bitcoinabc.org/D10861
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
GETADDRrequests and tests for-blocksonlymode.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.