Use type-safe time for better code readability/maintainability and mockable time for better testability. This speeds up the p2p_timeout test.
This is also a bugfix for intermittent test issues like: https://cirrus-ci.com/task/4769904156999680?command=ci#L2836
Fixes #20654
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, 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. There’s a lot of refactor/tidyup in this PR. I wonder if that can be split out to make this more focused on the functional changes.
Good idea. Split up a scripted-diff as the first commit.
Doesn’t this defeat the purpose of the test?
This is a bugfix for the test. Currently it intermittently fails, see OP.
-BEGIN VERIFY SCRIPT-
ren() { sed -i "s/\<$1\>/$2/g" $(git grep -l "$1" ./src) ; }
ren nLastSend m_last_send
ren nLastRecv m_last_recv
-END VERIFY SCRIPT-1337
1338 if (!ShouldRunInactivityChecks(node, now)) return false;
1339
1340- if (node.m_last_recv == 0 || node.m_last_send == 0) {
1341- LogPrint(BCLog::NET, "socket no message in first %i seconds, %d %d peer=%d\n", m_peer_connect_timeout, node.m_last_recv != 0, node.m_last_send != 0, node.GetId());
1342+ if (last_recv.count() == 0 || last_send.count() == 0) {
.count() and sometimes count_seconds()?
last_recv is a bit like std::optional. I use .count(), when .has_value() would be used and I use count_seconds(), when .value() would be used.
4311@@ -4312,9 +4312,10 @@ void PeerManagerImpl::CheckForStaleTipAndEvictPeers()
4312
4313 void PeerManagerImpl::MaybeSendPing(CNode& node_to, Peer& peer, std::chrono::microseconds now)
4314 {
4315- if (m_connman.ShouldRunInactivityChecks(node_to, std::chrono::duration_cast<std::chrono::seconds>(now).count()) &&
4316+ if (m_connman.ShouldRunInactivityChecks(node_to, std::chrono::duration_cast<std::chrono::seconds>(now)) &&
4317 peer.m_ping_nonce_sent &&
4318- now > peer.m_ping_start.load() + std::chrono::seconds{TIMEOUT_INTERVAL}) {
4319+ now > peer.m_ping_start.load() + std::chrono::seconds{TIMEOUT_INTERVAL})
0 now > peer.m_ping_start.load() + TIMEOUT_INTERVAL)
TIMEOUT_INTERVAL is of type std::chrono::minutes now.
1327 bool CConnman::InactivityCheck(const CNode& node) const
1328 {
1329- // Use non-mockable system time (otherwise these timers will pop when we
1330- // use setmocktime in the tests).
1331- int64_t now = GetTimeSeconds();
1332+ // Test that see disconnects after using mocktime can start with a large
Concept ACK
Reviewed the non-GUI-related parts so far and played with p2p_timeouts.py - looks good to me, just some nits.
4311@@ -4312,9 +4312,10 @@ void PeerManagerImpl::CheckForStaleTipAndEvictPeers()
4312
4313 void PeerManagerImpl::MaybeSendPing(CNode& node_to, Peer& peer, std::chrono::microseconds now)
4314 {
4315- if (m_connman.ShouldRunInactivityChecks(node_to, std::chrono::duration_cast<std::chrono::seconds>(now).count()) &&
4316+ if (m_connman.ShouldRunInactivityChecks(node_to, std::chrono::duration_cast<std::chrono::seconds>(now)) &&
4317 peer.m_ping_nonce_sent &&
4318- now > peer.m_ping_start.load() + std::chrono::seconds{TIMEOUT_INTERVAL}) {
4319+ now > peer.m_ping_start.load() + TIMEOUT_INTERVAL)
4320+ {
{ on the same line?
I think (and other seem to agree, see #21735) that multiline if conditions are hard to read because there is no break between condition and body, so changed it here.
Happy to revert, though.
if cases have the { on the same line), if you have a good reason for it it’s fine with me.
