Skip to content

Commit 8038be1

Browse files
MarcoFalkePastaPastaPasta
authored andcommitted
Merge bitcoin#20927: [refactor] [net] Clean up InactivityCheck()
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
1 parent 4bcbb8d commit 8038be1

File tree

2 files changed

+42
-31
lines changed

2 files changed

+42
-31
lines changed

src/net.cpp

Lines changed: 40 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1487,37 +1487,47 @@ void CConnman::CalculateNumConnectionsChangedStats()
14871487
statsClient.gauge("peers.torConnections", torNodes, 1.0f);
14881488
}
14891489

1490-
void CConnman::InactivityCheck(CNode *pnode) const
1490+
bool CConnman::InactivityCheck(const CNode& node) const
14911491
{
1492-
int64_t nTime = GetSystemTimeInSeconds();
1493-
if (nTime - pnode->nTimeConnected > m_peer_connect_timeout)
1494-
{
1495-
if (pnode->nLastRecv == 0 || pnode->nLastSend == 0)
1496-
{
1497-
LogPrint(BCLog::NET, "socket no message in first %i seconds, %d %d from %d\n", m_peer_connect_timeout, pnode->nLastRecv != 0, pnode->nLastSend != 0, pnode->GetId());
1498-
pnode->fDisconnect = true;
1499-
}
1500-
else if (nTime - pnode->nLastSend > TIMEOUT_INTERVAL)
1501-
{
1502-
LogPrintf("socket sending timeout: %is\n", nTime - pnode->nLastSend);
1503-
pnode->fDisconnect = true;
1504-
}
1505-
else if (nTime - pnode->nLastRecv > TIMEOUT_INTERVAL)
1506-
{
1507-
LogPrintf("socket receive timeout: %is\n", nTime - pnode->nLastRecv);
1508-
pnode->fDisconnect = true;
1509-
}
1510-
else if (pnode->nPingNonceSent && pnode->nPingUsecStart + TIMEOUT_INTERVAL * 1000000 < GetTimeMicros())
1511-
{
1512-
LogPrintf("ping timeout: %fs\n", 0.000001 * (GetTimeMicros() - pnode->nPingUsecStart));
1513-
pnode->fDisconnect = true;
1514-
}
1515-
else if (!pnode->fSuccessfullyConnected)
1516-
{
1517-
LogPrint(BCLog::NET, "version handshake timeout from %d\n", pnode->GetId());
1518-
pnode->fDisconnect = true;
1519-
}
1492+
// Use non-mockable system time (otherwise these timers will pop when we
1493+
// use setmocktime in the tests).
1494+
int64_t now = GetSystemTimeInSeconds();
1495+
1496+
if (now <= node.nTimeConnected + m_peer_connect_timeout) {
1497+
// Only run inactivity checks if the peer has been connected longer
1498+
// than m_peer_connect_timeout.
1499+
return false;
15201500
}
1501+
1502+
if (node.nLastRecv == 0 || node.nLastSend == 0) {
1503+
LogPrint(BCLog::NET, "socket no message in first %i seconds, %d %d from %d\n", m_peer_connect_timeout, node.nLastRecv != 0, node.nLastSend != 0, node.GetId());
1504+
return true;
1505+
}
1506+
1507+
if (now > node.nLastSend + TIMEOUT_INTERVAL) {
1508+
LogPrintf("socket sending timeout: %is\n", now - node.nLastSend);
1509+
return true;
1510+
}
1511+
1512+
if (now > node.nLastRecv + TIMEOUT_INTERVAL) {
1513+
LogPrintf("socket receive timeout: %is\n", now - node.nLastRecv);
1514+
return true;
1515+
}
1516+
1517+
if (node.nPingNonceSent && node.nPingUsecStart.load() + TIMEOUT_INTERVAL * 1000000 < GetTimeMicros()) {
1518+
// We use mockable time for ping timeouts. This means that setmocktime
1519+
// may cause pings to time out for peers that have been connected for
1520+
// longer than m_peer_connect_timeout.
1521+
LogPrintf("ping timeout: %fs\n", 0.000001 * (GetTimeMicros() - node.nPingUsecStart));
1522+
return true;
1523+
}
1524+
1525+
if (!node.fSuccessfullyConnected) {
1526+
LogPrint(BCLog::NET, "version handshake timeout from %d\n", node.GetId());
1527+
return true;
1528+
}
1529+
1530+
return false;
15211531
}
15221532

15231533
bool CConnman::GenerateSelectSet(std::set<SOCKET> &recv_set, std::set<SOCKET> &send_set, std::set<SOCKET> &error_set)
@@ -2036,7 +2046,7 @@ void CConnman::ThreadSocketHandler()
20362046
SocketHandler();
20372047
if (GetTimeMillis() - nLastCleanupNodes > 1000) {
20382048
ForEachNode(AllNodes, [&](CNode* pnode) {
2039-
InactivityCheck(pnode);
2049+
if (InactivityCheck(*pnode)) pnode->fDisconnect = true;
20402050
});
20412051
nLastCleanupNodes = GetTimeMillis();
20422052
}

src/net.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -600,7 +600,8 @@ friend class CNode;
600600
void DisconnectNodes();
601601
void NotifyNumConnectionsChanged();
602602
void CalculateNumConnectionsChangedStats();
603-
void InactivityCheck(CNode *pnode) const;
603+
/** Return true if the peer is inactive and should be disconnected. */
604+
bool InactivityCheck(const CNode& node) const;
604605
bool GenerateSelectSet(std::set<SOCKET> &recv_set, std::set<SOCKET> &send_set, std::set<SOCKET> &error_set);
605606
#ifdef USE_KQUEUE
606607
void SocketEventsKqueue(std::set<SOCKET> &recv_set, std::set<SOCKET> &send_set, std::set<SOCKET> &error_set, bool fOnlyPoll);

0 commit comments

Comments
 (0)