Skip to content

net: simplify PONG handler #15613

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

net: simplify PONG handler #15613

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 24 additions & 45 deletions src/net_processing.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2959,58 +2959,37 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
}

if (strCommand == NetMsgType::PONG) {
int64_t pingUsecEnd = nTimeReceived;
// We didn't send a ping, there shouldn't be a pong
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of adding a comment here, we could make the LogPrint more informative (which would self comment the code).

if (pfrom->nPingNonceSent == 0) {
LogPrint(BCLog::NET, "pong peer=%d: unsolicited pong\n", pfrom->GetId());
return true;
}

uint64_t nonce = 0;
size_t nAvail = vRecv.in_avail();
bool bPingFinished = false;
std::string sProblem;
if (nAvail != sizeof(nonce)) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this intentional? sizeof should return 8 (is 8 intentional here) but we might have side effects depending on the OS.
Why use sizeof here and not >0 or just nonce?

LogPrint(BCLog::NET, "pong peer=%d: bad buffer length, expected %u actual %u\n", pfrom->GetId(), sizeof(nonce), nAvail);
pfrom->nPingNonceSent = 0;
return true;
}

if (nAvail >= sizeof(nonce)) {
vRecv >> nonce;
vRecv >> nonce;
if (nonce != pfrom->nPingNonceSent) {
LogPrint(BCLog::NET, "pong peer=%d: nonce mismatch, expected %x actual %x\n", pfrom->GetId(), pfrom->nPingNonceSent, nonce);
pfrom->nPingNonceSent = 0;
return true;
}

// Only process pong message if there is an outstanding ping (old ping without nonce should never pong)
if (pfrom->nPingNonceSent != 0) {
if (nonce == pfrom->nPingNonceSent) {
// Matching pong received, this ping is no longer outstanding
bPingFinished = true;
int64_t pingUsecTime = pingUsecEnd - pfrom->nPingUsecStart;
if (pingUsecTime > 0) {
// Successful ping time measurement, replace previous
pfrom->nPingUsecTime = pingUsecTime;
pfrom->nMinPingUsecTime = std::min(pfrom->nMinPingUsecTime.load(), pingUsecTime);
} else {
// This should never happen
sProblem = "Timing mishap";
}
} else {
// Nonce mismatches are normal when pings are overlapping
sProblem = "Nonce mismatch";
if (nonce == 0) {
// This is most likely a bug in another implementation somewhere; cancel this ping
bPingFinished = true;
sProblem = "Nonce zero";
}
}
} else {
sProblem = "Unsolicited pong without ping";
}
int64_t pingUsecTime = nTimeReceived - pfrom->nPingUsecStart;
if (pingUsecTime >= 0) {
// Successful ping time measurement, replace previous
pfrom->nPingUsecTime = pingUsecTime;
pfrom->nMinPingUsecTime = std::min(pfrom->nMinPingUsecTime.load(), pingUsecTime);
} else {
// This is most likely a bug in another implementation somewhere; cancel this ping
bPingFinished = true;
sProblem = "Short payload";
LogPrint(BCLog::NET, "pong peer=%d: received pong before ping sent, maybe clock changed?\n", pfrom->GetId());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe "received pong before or at time ping sent"

}

if (!(sProblem.empty())) {
LogPrint(BCLog::NET, "pong peer=%d: %s, %x expected, %x received, %u bytes\n",
pfrom->GetId(),
sProblem,
pfrom->nPingNonceSent,
nonce,
nAvail);
}
if (bPingFinished) {
pfrom->nPingNonceSent = 0;
}
pfrom->nPingNonceSent = 0;
return true;
}

Expand Down