[refactor] [net] Clean up InactivityCheck() #20927
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
Concept ACK: I find the suggested version easier to reason about. Also thanks for switching to |
jnewbery
force-pushed
the
2021-01-timeout-cleanup
branch
from
9a42bc5 to
075d3c5
Compare
|
I've updated the commit message on the second commit to give a bit more context about why the change to the return type is an improvement. |
There was a problem hiding this comment.
crACK 075d3c5ea25e5e4ec9ee1a18e565aeaed16b2214
I like that you made InactivityChecks a pure function, and agree this is a little cleaner. nit: I didn't find changing the ordering of the if statements to be clearer, but I don't feel strongly about it
|
Thanks for the review @mjdietzx!
Maybe it's just me, but I find that standardizing all time comparisons to |
Also clean up and better comment the function. InactivityChecks() uses a mixture of (non-mockable) system time and mockable time. Make sure that's well documented. Despite being marked as const in CConnman before this commit, the function did mutate the state of the passed in CNode, which is contained in vNodes, which is a member of CConnman. To make the function truly const in CConnman and all its data, instead make InactivityChecks() a pure function, return whether the peer should be disconnected, and let the calling function (SocketHandler()) update the CNode object. Also make the CNode& argument const.
|
I've pushed a small update to make the |
jnewbery
force-pushed
the
2021-01-timeout-cleanup
branch
from
075d3c5 to
bf100f8
Compare
maflcko
left a comment
maflcko
left a comment
There was a problem hiding this comment.
review ACK bf100f8 💫
Show signature and timestamp
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
review ACK bf100f8170770544fb39ae6802175c564cde532f 💫
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUgT8Av/b4FZwKaQtoksDjtnB2DNCLN+Ldh3haCVJR0xcqjm0RoIPbfWVW+542b3
6EoqZIpl1l1DNyZh2ej3ATRGB9FqQF2McIAZqrboXIpUScMPf/yd0f6rJRi2WoCR
Ga/jHA3gX9jzfjOWQEOdQ/CloK8ic/5WKF9B8BNcR7e+kf+MeFazSyFB4RNFUYqP
8oOt4secmfOChQJgBQMAv0RgS1pN7+MJ/uvIF/USeM+Zw8/VLbxUwmKlL6BUqnM+
sa14lX7nFyI6YC7x45mfdvv3tLDZfsOaYNgpN1K8YYzuuE6dIk3FzTkZJqV/5iUz
isP8xRgh9Upv6GZK1EzTbY57YzJ15WAjYu2qmr3CCxU7J2/ZtYPJPYYsve9CQkT9
ej+75BdjF+Q9d1RgJvIFUUfnuUfVyo1zTnbiTQum/CKot6AuGM1VAAJXvqdIGakN
B5hXgACOPBs0HJ6BBKnwARygNigXMzHiCD5Mfa38UrTtvu5uJklNKTQ1NSOvOOlq
n2I2skbQ
=OdKZ
-----END PGP SIGNATURE-----
Timestamp of file with hash 9262a3bf7f8cbef76444cea7a5dd2356354c35fbb3f38dbc8fac626d2cd79705 -
| node.fDisconnect = true; | ||
| } | ||
| // Use non-mockable system time (otherwise these timers will pop when we | ||
| // use setmocktime in the tests). |
There was a problem hiding this comment.
this can be worked around by mocking m_peer_connect_timeout to a high value
bf100f8 [net] Cleanup InactivityChecks() and add commenting about time (John Newbery) 06fa85c [net] InactivityCheck() takes a CNode reference (John Newbery) Pull request description: This is a pure refactor and should not change any behavior. It clarifies and documents the InactivityCheck() function This makes bitcoin#20721 easier to review. In particular, this function uses a mixture of (unmockable) system time and mockable time. It's important to understand where those are being used when reviewing bitcoin#20721. bitcoin#20721 doesn't require this change, so if others don't agree that it's useful and makes review easier, then I'm happy to close this and just do bitcoin#20721 directly. ACKs for top commit: fanquake: ACK bf100f8 MarcoFalke: review ACK bf100f8 💫 Tree-SHA512: 7b001de2a5fbe8a6dc37baeae930db5775290afb2e8a6aecdf13161f1e5b06ef813bc6291d8ee5cefcf1e430c955ea702833a8db84192eebe6e6acf0b9304cb2
Summary: ``` This is a pure refactor and should not change any behavior. It clarifies and documents the InactivityCheck() function This makes #20721 easier to review. In particular, this function uses a mixture of (unmockable) system time and mockable time. It's important to understand where those are being used when reviewing #20721. ``` Backport of [[bitcoin/bitcoin#20927 | core#20927]]. Ref T1696. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Maniphest Tasks: T1696 Differential Revision: https://reviews.bitcoinabc.org/D10874
6 participants
This is a pure refactor and should not change any behavior. It clarifies and documents the InactivityCheck() function
This makes #20721 easier to review. In particular, this function uses a mixture of (unmockable) system time and mockable time. It's important to understand where those are being used when reviewing #20721.
#20721 doesn't require this change, so if others don't agree that it's useful and makes review easier, then I'm happy to close this and just do #20721 directly.