From cd820b37779cf2cf5559f1b6087ca65999822c84 Mon Sep 17 00:00:00 2001
From: Crypto Brad Garlinghouse <cryptobradgarlinghouse@protonmail.com>
Date: Thu, 27 Dec 2018 21:16:06 +0000
Subject: [PATCH] Improve the server's PING/PONG logic

---
 src/ripple/overlay/impl/PeerImp.cpp | 71 ++++++++++++++---------------
 src/ripple/overlay/impl/PeerImp.h   |  4 +-
 src/ripple/overlay/impl/Tuning.h    |  9 ++--
 3 files changed, 41 insertions(+), 43 deletions(-)

diff --git a/src/ripple/overlay/impl/PeerImp.cpp b/src/ripple/overlay/impl/PeerImp.cpp
index c627fa0ff1..e2ecefb72a 100644
--- a/src/ripple/overlay/impl/PeerImp.cpp
+++ b/src/ripple/overlay/impl/PeerImp.cpp
@@ -291,14 +291,9 @@ PeerImp::json()
     }
 
     {
-        std::chrono::milliseconds latency;
-        {
-            std::lock_guard<std::mutex> sl (recentLock_);
-            latency = latency_;
-        }
-
-        if (latency != std::chrono::milliseconds (-1))
-            ret[jss::latency] = static_cast<Json::UInt> (latency.count());
+        std::lock_guard<std::mutex> sl (recentLock_);
+        if (latency_)
+            ret[jss::latency] = static_cast<Json::UInt> (latency_->count());
     }
 
     ret[jss::uptime] = static_cast<Json::UInt>(
@@ -582,25 +577,24 @@ PeerImp::onTimer (error_code const& ec)
         return;
     }
 
-    if (lastPingSeq_ == 0)
+    if (!lastPingSeq_)
     {
-        // Make sequence unpredictable enough that a peer
-        // can't fake their latency
-        lastPingSeq_ = rand_int (65535);
+        // Make the sequence unpredictable enough to prevent guessing
+        lastPingSeq_ = rand_int<std::uint32_t>();
         lastPingTime_ = clock_type::now();
 
         protocol::TMPing message;
         message.set_type (protocol::TMPing::ptPING);
-        message.set_seq (lastPingSeq_);
+        message.set_seq (*lastPingSeq_);
 
-        send (std::make_shared<Message> (
-            message, protocol::mtPING));
+        send (std::make_shared<Message> (message, protocol::mtPING));
     }
     else
     {
         // We have an outstanding ping, raise latency
-        auto minLatency = std::chrono::duration_cast <std::chrono::milliseconds>
-            (clock_type::now() - lastPingTime_);
+        auto const minLatency =
+            std::chrono::duration_cast<std::chrono::milliseconds>
+                (clock_type::now() - lastPingTime_);
 
         std::lock_guard<std::mutex> sl(recentLock_);
 
@@ -908,30 +902,33 @@ PeerImp::onMessage (std::shared_ptr <protocol::TMPing> const& m)
         fee_ = Resource::feeMediumBurdenPeer;
         m->set_type (protocol::TMPing::ptPONG);
         send (std::make_shared<Message> (*m, protocol::mtPING));
-
         return;
     }
 
-    if ((m->type () == protocol::TMPing::ptPONG) && m->has_seq ())
+    if (m->type () == protocol::TMPing::ptPONG)
     {
-        // We have received a pong, update our latency estimate
-        auto unknownLatency = std::chrono::milliseconds (-1);
-
-        std::lock_guard<std::mutex> sl(recentLock_);
-
-        if ((lastPingSeq_ != 0) && (m->seq () == lastPingSeq_))
+        if (m->has_seq() && m->seq() == lastPingSeq_)
         {
             no_ping_ = 0;
-            auto estimate = std::chrono::duration_cast <std::chrono::milliseconds>
-                (clock_type::now() - lastPingTime_);
-            if (latency_ == unknownLatency)
-                latency_ = estimate;
+
+            // Only reset the ping sequence if we actually received a
+            // PONG with the correct cookie. That way, any peers which
+            // respond with incorrect cookies will eventually time out.
+            lastPingSeq_.reset();
+
+            // Update latency estimate
+            std::lock_guard<std::mutex> sl(recentLock_);
+
+            auto const estimate =
+                std::chrono::duration_cast<std::chrono::milliseconds>
+                    (clock_type::now() - lastPingTime_);
+
+            // Calculate the cumulative moving average of the latency:
+            if (latency_)
+                latency_ = (*latency_ * 7 + estimate) / 8;
             else
-                latency_ = (latency_ * 7 + estimate) / 8;
+                latency_ = estimate;
         }
-        else
-            latency_ = unknownLatency;
-        lastPingSeq_ = 0;
 
         return;
     }
@@ -2701,14 +2698,14 @@ PeerImp::getScore (bool haveItem) const
    if (haveItem)
        score += spHaveItem;
 
-   std::chrono::milliseconds latency;
+   boost::optional<std::chrono::milliseconds> latency;
    {
        std::lock_guard<std::mutex> sl (recentLock_);
        latency = latency_;
    }
 
-   if (latency != std::chrono::milliseconds (-1))
-       score -= latency.count() * spLatency;
+   if (latency)
+       score -= latency->count() * spLatency;
    else
        score -= spNoLatency;
 
@@ -2719,7 +2716,7 @@ bool
 PeerImp::isHighLatency() const
 {
     std::lock_guard<std::mutex> sl (recentLock_);
-    return latency_.count() >= Tuning::peerHighLatency;
+    return latency_ >= Tuning::peerHighLatency;
 }
 
 } // ripple
diff --git a/src/ripple/overlay/impl/PeerImp.h b/src/ripple/overlay/impl/PeerImp.h
index a066aeb1e1..f7885cee58 100644
--- a/src/ripple/overlay/impl/PeerImp.h
+++ b/src/ripple/overlay/impl/PeerImp.h
@@ -139,8 +139,8 @@ private:
     std::deque<uint256> recentLedgers_;
     std::deque<uint256> recentTxSets_;
 
-    std::chrono::milliseconds latency_ = std::chrono::milliseconds (-1);
-    std::uint64_t lastPingSeq_ = 0;
+    boost::optional<std::chrono::milliseconds> latency_;
+    boost::optional<std::uint32_t> lastPingSeq_;
     clock_type::time_point lastPingTime_;
     clock_type::time_point creationTime_;
 
diff --git a/src/ripple/overlay/impl/Tuning.h b/src/ripple/overlay/impl/Tuning.h
index 566280fe39..b6d8309325 100644
--- a/src/ripple/overlay/impl/Tuning.h
+++ b/src/ripple/overlay/impl/Tuning.h
@@ -20,6 +20,8 @@
 #ifndef RIPPLE_OVERLAY_TUNING_H_INCLUDED
 #define RIPPLE_OVERLAY_TUNING_H_INCLUDED
 
+#include <chrono>
+
 namespace ripple {
 
 namespace Tuning
@@ -50,10 +52,6 @@ enum
         reply */
     maxReplyNodes       = 8192,
 
-    /** How many milliseconds to consider high latency
-        on a peer connection */
-    peerHighLatency     =  300,
-
     /** How often we check connections (seconds) */
     checkSeconds        =   32,
 
@@ -76,6 +74,9 @@ enum
     sendQueueLogFreq    =    64,
 };
 
+/** The threshold above which we treat a peer connection as high latency */
+std::chrono::milliseconds constexpr peerHighLatency{300};
+
 } // Tuning
 
 } // ripple