[refactor] [net] Clean up InactivityCheck()

jnewbery commented at 4:39 pm on January 13, 2021: member

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.

[net] InactivityCheck() takes a CNode reference 06fa85cd50

DrahtBot added the label P2P on Jan 13, 2021

DrahtBot commented at 6:47 pm on January 13, 2021: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20724 (Cleanup of -debug=net log messages by ajtowns)
#20721 (Net: Move ping data to net_processing by jnewbery)
#20685 (Add I2P support using I2P SAM by vasild)

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.

practicalswift commented at 11:41 am on January 14, 2021: contributor

Concept ACK: I find the suggested version easier to reason about.

Also thanks for switching to CNode* to CNode&: it makes the currently implicit precondition explicit. As we all know by know: explicit is much better than implicit :)

jnewbery force-pushed on Jan 14, 2021

jnewbery commented at 1:01 pm on January 14, 2021: member

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.

mjdietzx commented at 8:57 pm on January 15, 2021: contributor

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

jnewbery commented at 10:31 am on January 16, 2021: member

Thanks for the review @mjdietzx!

I didn’t find changing the ordering of the if statements to be clearer, but I don’t feel strongly about it

Maybe it’s just me, but I find that standardizing all time comparisons to now < or now > (or in English - “it is [now] before/after the timeout”) makes it easier to intuitively understand the comparisons. I don’t feel strongly and I’m happy to change them back if the other reviewers agree with you.

[net] Cleanup InactivityChecks() and add commenting about time

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.

bf100f8170

jnewbery commented at 10:51 am on January 19, 2021: member

I’ve pushed a small update to make the CNode& argument const now that the function isn’t mutating it.

jnewbery force-pushed on Jan 19, 2021

fanquake approved

fanquake commented at 11:28 am on January 22, 2021: member

ACK bf100f8170770544fb39ae6802175c564cde532f

in src/net.cpp:1222 in bf100f8170

1246-        {
1247-            LogPrint(BCLog::NET, "version handshake timeout from %d\n", node.GetId());
1248-            node.fDisconnect = true;
1249-        }
1250+    // Use non-mockable system time (otherwise these timers will pop when we
1251+    // use setmocktime in the tests).

MarcoFalke commented at 12:06 pm on January 22, 2021:

this can be worked around by mocking m_peer_connect_timeout to a high value

MarcoFalke approved

MarcoFalke commented at 12:11 pm on January 22, 2021: member

review ACK bf100f8170770544fb39ae6802175c564cde532f 💫

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK bf100f8170770544fb39ae6802175c564cde532f 💫
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgT8Av/b4FZwKaQtoksDjtnB2DNCLN+Ldh3haCVJR0xcqjm0RoIPbfWVW+542b3
 86EoqZIpl1l1DNyZh2ej3ATRGB9FqQF2McIAZqrboXIpUScMPf/yd0f6rJRi2WoCR
 9Ga/jHA3gX9jzfjOWQEOdQ/CloK8ic/5WKF9B8BNcR7e+kf+MeFazSyFB4RNFUYqP
108oOt4secmfOChQJgBQMAv0RgS1pN7+MJ/uvIF/USeM+Zw8/VLbxUwmKlL6BUqnM+
11sa14lX7nFyI6YC7x45mfdvv3tLDZfsOaYNgpN1K8YYzuuE6dIk3FzTkZJqV/5iUz
12isP8xRgh9Upv6GZK1EzTbY57YzJ15WAjYu2qmr3CCxU7J2/ZtYPJPYYsve9CQkT9
13ej+75BdjF+Q9d1RgJvIFUUfnuUfVyo1zTnbiTQum/CKot6AuGM1VAAJXvqdIGakN
14B5hXgACOPBs0HJ6BBKnwARygNigXMzHiCD5Mfa38UrTtvu5uJklNKTQ1NSOvOOlq
15n2I2skbQ
16=OdKZ
17-----END PGP SIGNATURE-----

Timestamp of file with hash 9262a3bf7f8cbef76444cea7a5dd2356354c35fbb3f38dbc8fac626d2cd79705 -

MarcoFalke merged this on Jan 22, 2021

MarcoFalke closed this on Jan 22, 2021

jnewbery deleted the branch on Jan 22, 2021

sidhujag referenced this in commit 86c22c0c27 on Jan 22, 2021

Fabcien referenced this in commit a06ab3bb4c on Jan 25, 2022

DrahtBot locked this on Aug 18, 2022

[refactor] [net] Clean up InactivityCheck() #20927

Conflicts