net: simplify PONG handler #15613

pull pstratem wants to merge 1 commits into bitcoin:master from pstratem:2019-03-17-net-processing-pong changing 1 files +24 −45

pstratem commented at 4:39 PM on March 17, 2019: contributor

We treat a buffer size mismatch as a protocol violation. We treat a zero microsecond pong response as spurious (likely the local clock is broken).
pstratem force-pushed on Mar 17, 2019
pstratem renamed this:
~~net: Simplify PONG handler, improve readability of the processing logic.~~
net: simplify PONG handler and disconnect for protocol violations
on Mar 17, 2019
practicalswift commented at 5:05 PM on March 17, 2019: contributor

Is this intended as a pure refactoring or does the change come with any intentional change in behaviour?

Is the change from nAvail >= sizeof(nonce) to nAvail != sizeof(nonce) intentional?
pstratem commented at 5:13 PM on March 17, 2019: contributor

@practicalswift the change is intentional, the buffer should exactly match the expected length (yes i know this means we cant extend the pong message in the future)
practicalswift commented at 5:17 PM on March 17, 2019: contributor

@pstratem Thanks for clarifying! Any other intentional deviations from a pure refactoring? :-)
pstratem force-pushed on Mar 17, 2019
pstratem force-pushed on Mar 17, 2019
pstratem force-pushed on Mar 17, 2019
Simplify PONG handler, improve readability of the processing logic. a3fc70f084
pstratem force-pushed on Mar 17, 2019
pstratem commented at 5:39 PM on March 17, 2019: contributor

@practicalswift the timing constraint is changed from > to >= as it's measured in microseconds. a ping time of zero seconds is almost certainly in error.
pstratem commented at 5:39 PM on March 17, 2019: contributor

and obviously the error messages are changed to be more specific
pstratem force-pushed on Mar 17, 2019
pstratem renamed this:
~~net: simplify PONG handler and disconnect for protocol violations~~
net: simplify PONG handler
on Mar 17, 2019
fanquake added the label P2P on Mar 17, 2019

in src/net_processing.cpp:2989 in a3fc70f084

3018 | +            pfrom->nMinPingUsecTime = std::min(pfrom->nMinPingUsecTime.load(), pingUsecTime);
3019 |          } else {
3020 | -            // This is most likely a bug in another implementation somewhere; cancel this ping
3021 | -            bPingFinished = true;
3022 | -            sProblem = "Short payload";
3023 | +            LogPrint(BCLog::NET, "pong peer=%d: received pong before ping sent, maybe clock changed?\n", pfrom->GetId());

laanwj commented at 11:05 AM on June 6, 2019:

maybe "received pong before or at time ping sent"

in src/net_processing.cpp:2962 in a3fc70f084

2958 | @@ -2959,58 +2959,37 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
2959 |      }
2960 |  
2961 |      if (strCommand == NetMsgType::PONG) {
2962 | -        int64_t pingUsecEnd = nTimeReceived;
2963 | +        // We didn't send a ping, there shouldn't be a pong

fqlx commented at 4:10 AM on July 4, 2019:

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

in src/net_processing.cpp:2970 in a3fc70f084
```
2968 | +
2969 |          uint64_t nonce = 0;
2970 |          size_t nAvail = vRecv.in_avail();
2971 | -        bool bPingFinished = false;
2972 | -        std::string sProblem;
2973 | +        if (nAvail != sizeof(nonce)) {
```
fqlx commented at 4:13 AM on July 4, 2019:

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?
fqlx changes_requested
fanquake commented at 12:25 PM on August 23, 2019: member

@pstratem did you want to follow up and address any comments here?
fanquake added the label Waiting for author on Aug 23, 2019
laanwj commented at 12:36 PM on September 30, 2019: member

Closing, as this has been waiting for author for more than a month. Let me know if you want to reopen this.
laanwj closed this on Sep 30, 2019
laanwj removed the label Waiting for author on Oct 24, 2019
DrahtBot locked this on Dec 16, 2021

Contributors

Labels