Eliminate the built-in SNTP support (fixes #4207): (#4628)

Builds/CMake/RippledCore.cmake

@@ -568,9 +568,7 @@ target_sources (rippled PRIVATE
   src/ripple/core/impl/JobQueue.cpp
   src/ripple/core/impl/LoadEvent.cpp
   src/ripple/core/impl/LoadMonitor.cpp
-  src/ripple/core/impl/SNTPClock.cpp
   src/ripple/core/impl/SociDB.cpp
-  src/ripple/core/impl/TimeKeeper.cpp
   src/ripple/core/impl/Workers.cpp
   src/ripple/core/Pg.cpp
   #[===============================[
@@ -929,7 +927,6 @@ if (tests)
     src/test/jtx/impl/AMMTest.cpp
     src/test/jtx/impl/Env.cpp
     src/test/jtx/impl/JSONRPCClient.cpp
-    src/test/jtx/impl/ManualTimeKeeper.cpp
     src/test/jtx/impl/TestHelpers.cpp
     src/test/jtx/impl/WSClient.cpp
     src/test/jtx/impl/acctdelete.cpp

Builds/levelization/results/ordering.txt

@@ -173,7 +173,6 @@ test.nodestore > test.unit_test
 test.overlay > ripple.app
 test.overlay > ripple.basics
 test.overlay > ripple.beast
-test.overlay > ripple.core
 test.overlay > ripple.overlay
 test.overlay > ripple.peerfinder
 test.overlay > ripple.protocol

cfg/rippled-example.cfg

@@ -463,19 +463,6 @@
 #
 #
 #
-# [sntp_servers]
-#
-#   IP address or domain of NTP servers to use for time synchronization.
-#
-#   These NTP servers are suitable for rippled servers located in the United
-#   States:
-#      time.windows.com
-#      time.apple.com
-#      time.nist.gov
-#      pool.ntp.org
-#
-#
-#
 # [max_transactions]
 #
 #   Configure the maximum number of transactions to have in the job queue
@@ -1704,12 +1691,6 @@ advisory_delete=0
 [debug_logfile]
 /var/log/rippled/debug.log
 
-[sntp_servers]
-time.windows.com
-time.apple.com
-time.nist.gov
-pool.ntp.org
-
 # To use the XRP test network
 # (see https://xrpl.org/connect-your-rippled-to-the-xrp-test-net.html),
 # use the following [ips] section:

cfg/rippled-reporting.cfg

@@ -450,19 +450,6 @@
 #
 #
 #
-# [sntp_servers]
-#
-#   IP address or domain of NTP servers to use for time synchronization.
-#
-#   These NTP servers are suitable for rippled servers located in the United
-#   States:
-#      time.windows.com
-#      time.apple.com
-#      time.nist.gov
-#      pool.ntp.org
-#
-#
-#
 # [max_transactions]
 #
 #   Configure the maximum number of transactions to have in the job queue
@@ -1662,12 +1649,6 @@ advisory_delete=0
 [debug_logfile]
 /var/log/rippled-reporting/debug.log
 
-[sntp_servers]
-time.windows.com
-time.apple.com
-time.nist.gov
-pool.ntp.org
-
 # To use the XRP test network
 # (see https://xrpl.org/connect-your-rippled-to-the-xrp-test-net.html),
 # use the following [ips] section:

src/ripple/app/main/Application.cpp

@@ -1175,9 +1175,6 @@ ApplicationImp::setup(boost::program_options::variables_map const& cmdline)
     // Optionally turn off logging to console.
     logs_->silent(config_->silent());
 
-    if (!config_->standalone())
-        timeKeeper_->run(config_->SNTP_SERVERS);
-
     if (!initRelationalDatabase() || !initNodeStore())
         return false;
 

src/ripple/app/main/Main.cpp

@@ -803,10 +803,8 @@ run(int argc, char** argv)
         if (vm.count("debug"))
             setDebugLogSink(logs->makeSink("Debug", beast::severities::kTrace));
 
-        auto timeKeeper = make_TimeKeeper(logs->journal("TimeKeeper"));
-
         auto app = make_Application(
-            std::move(config), std::move(logs), std::move(timeKeeper));
+            std::move(config), std::move(logs), std::make_unique<TimeKeeper>());
 
         if (!app->setup(vm))
             return -1;

src/ripple/app/misc/NetworkOPs.cpp

@@ -739,11 +739,10 @@ class NetworkOPsImp final : public NetworkOPs
         sPeerStatus,      // Peer status changes.
         sConsensusPhase,  // Consensus phase
         sBookChanges,     // Per-ledger order book changes
-
-        sLastEntry = sBookChanges  // as this name implies, any new entry
-                                   // must be ADDED ABOVE this one
+        sLastEntry        // Any new entry must be ADDED ABOVE this one
     };
-    std::array<SubMapType, SubTypes::sLastEntry + 1> mStreamMaps;
+
+    std::array<SubMapType, SubTypes::sLastEntry> mStreamMaps;
 
     ServerFeeSummary mLastFeeSummary;
 
@@ -2614,13 +2613,10 @@ NetworkOPsImp::getServerInfo(bool human, bool admin, bool counters)
                 lpClosed->fees().accountReserve(0).decimalXRP();
             l[jss::reserve_inc_xrp] = lpClosed->fees().increment.decimalXRP();
 
-            auto const nowOffset = app_.timeKeeper().nowOffset();
-            if (std::abs(nowOffset.count()) >= 60)
-                l[jss::system_time_offset] = nowOffset.count();
-
-            auto const closeOffset = app_.timeKeeper().closeOffset();
-            if (std::abs(closeOffset.count()) >= 60)
-                l[jss::close_time_offset] = closeOffset.count();
+            if (auto const closeOffset = app_.timeKeeper().closeOffset();
+                std::abs(closeOffset.count()) >= 60)
+                l[jss::close_time_offset] =
+                    static_cast<std::uint32_t>(closeOffset.count());
 
 #if RIPPLED_REPORTING
             std::int64_t const dbAge =

src/ripple/beast/clock/abstract_clock.h

@@ -70,7 +70,7 @@ class abstract_clock
     abstract_clock(abstract_clock const&) = default;
 
     /** Returns the current time. */
-    virtual time_point
+    [[nodiscard]] virtual time_point
     now() const = 0;
 };
 

src/ripple/core/Config.h

@@ -149,9 +149,11 @@ class Config : public BasicConfig
     bool nodeToShard = false;
     bool ELB_SUPPORT = false;
 
-    std::vector<std::string> IPS;           // Peer IPs from rippled.cfg.
-    std::vector<std::string> IPS_FIXED;     // Fixed Peer IPs from rippled.cfg.
-    std::vector<std::string> SNTP_SERVERS;  // SNTP servers from rippled.cfg.
+    // Entries from [ips] config stanza
+    std::vector<std::string> IPS;
+
+    // Entries from [ips_fixed] config stanza
+    std::vector<std::string> IPS_FIXED;
 
     enum StartUpType { FRESH, NORMAL, LOAD, LOAD_FILE, REPLAY, NETWORK };
     StartUpType START_UP = NORMAL;

src/ripple/core/TimeKeeper.h

@@ -22,73 +22,99 @@
 
 #include <ripple/basics/chrono.h>
 #include <ripple/beast/clock/abstract_clock.h>
-#include <ripple/beast/utility/Journal.h>
-#include <string>
-#include <vector>
+#include <atomic>
 
 namespace ripple {
 
 /** Manages various times used by the server. */
 class TimeKeeper : public beast::abstract_clock<NetClock>
 {
-public:
-    virtual ~TimeKeeper() = default;
-
-    /** Launch the internal thread.
+private:
+    std::atomic<std::chrono::seconds> closeOffset_{};
 
-        The internal thread synchronizes local network time
-        using the provided list of SNTP servers.
-    */
-    virtual void
-    run(std::vector<std::string> const& servers) = 0;
-
-    /** Returns the estimate of wall time, in network time.
+    // Adjust system_clock::time_point for NetClock epoch
+    static constexpr time_point
+    adjust(std::chrono::system_clock::time_point when)
+    {
+        return time_point(std::chrono::duration_cast<duration>(
+            when.time_since_epoch() - days(10957)));
+    }
 
-        The network time is wall time adjusted for the Ripple
-        epoch, the beginning of January 1st, 2000 UTC. Each server
-        can compute a different value for network time. Other
-        servers value for network time is not directly observable,
-        but good guesses can be made by looking at validators'
-        positions on close times.
+public:
+    virtual ~TimeKeeper() = default;
 
-        Servers compute network time by adjusting a local wall
-        clock using SNTP and then adjusting for the epoch.
-    */
-    virtual time_point
-    now() const override = 0;
+    /** Returns the current time, using the server's clock.
 
-    /** Returns the close time, in network time.
+        It's possible for servers to have a different value for network
+        time, especially if they do not use some external mechanism for
+        time synchronization (e.g. NTP or SNTP). This is fine.
 
-        Close time is the time the network would agree that
-        a ledger closed, if a ledger closed right now.
+        This estimate is not directly visible to other servers over the
+        protocol, but it is possible for them to make an educated guess
+        if this server publishes proposals or validations.
 
-        The close time represents the notional "center"
-        of the network. Each server assumes its clock
-        is correct, and tries to pull the close time towards
-        its measure of network time.
+        @note The network time is adjusted for the "Ripple epoch" which
+              was arbitrarily defined as 2000-01-01T00:00:00Z by Arthur
+              Britto and David Schwartz during early development of the
+              code. No rationale has been provided for this curious and
+              annoying, but otherwise unimportant, choice.
     */
-    virtual time_point
-    closeTime() const = 0;
+    [[nodiscard]] time_point
+    now() const override
+    {
+        return adjust(std::chrono::system_clock::now());
+    }
 
-    /** Adjust the close time.
+    /** Returns the predicted close time, in network time.
 
-        This is called in response to received validations.
+        The predicted close time represents the notional "center" of the
+        network. Each server assumes that its clock is correct and tries
+        to pull the close time towards its measure of network time.
     */
-    virtual void
-    adjustCloseTime(std::chrono::duration<std::int32_t> amount) = 0;
+    [[nodiscard]] time_point
+    closeTime() const
+    {
+        return now() + closeOffset_.load();
+    }
 
     // This may return a negative value
-    virtual std::chrono::duration<std::int32_t>
-    nowOffset() const = 0;
-
-    // This may return a negative value
-    virtual std::chrono::duration<std::int32_t>
-    closeOffset() const = 0;
+    [[nodiscard]] std::chrono::seconds
+    closeOffset() const
+    {
+        return closeOffset_.load();
+    }
+
+    /** Adjust the close time, based on the network's view of time. */
+    std::chrono::seconds
+    adjustCloseTime(std::chrono::seconds by)
+    {
+        using namespace std::chrono_literals;
+
+        auto offset = closeOffset_.load();
+
+        if (by == 0s && offset == 0s)
+            return offset;
+
+        // The close time adjustment is serialized externally to this
+        // code. The compare/exchange only serves as a weak check and
+        // should not fail. Even if it does, it's safe to simply just
+        // skip the adjustment.
+        closeOffset_.compare_exchange_strong(offset, [by, offset]() {
+            // Ignore small offsets and push the close time
+            // towards our wall time.
+            if (by > 1s)
+                return offset + ((by + 3s) / 4);
+
+            if (by < -1s)
+                return offset + ((by - 3s) / 4);
+
+            return (offset * 3) / 4;
+        }());
+
+        return closeOffset_.load();
+    }
 };
 
-extern std::unique_ptr<TimeKeeper>
-make_TimeKeeper(beast::Journal j);
-
 }  // namespace ripple
 
 #endif

src/ripple/core/impl/Config.cpp

@@ -206,14 +206,10 @@ parseIniFile(std::string const& strInput, const bool bTrim)
 IniFileSections::mapped_type*
 getIniFileSection(IniFileSections& secSource, std::string const& strSection)
 {
-    IniFileSections::iterator it;
-    IniFileSections::mapped_type* smtResult;
-    it = secSource.find(strSection);
-    if (it == secSource.end())
-        smtResult = nullptr;
-    else
-        smtResult = &(it->second);
-    return smtResult;
+    if (auto it = secSource.find(strSection); it != secSource.end())
+        return &(it->second);
+
+    return nullptr;
 }
 
 bool
@@ -223,22 +219,21 @@ getSingleSection(
     std::string& strValue,
     beast::Journal j)
 {
-    IniFileSections::mapped_type* pmtEntries =
-        getIniFileSection(secSource, strSection);
-    bool bSingle = pmtEntries && 1 == pmtEntries->size();
+    auto const pmtEntries = getIniFileSection(secSource, strSection);
 
-    if (bSingle)
+    if (pmtEntries && pmtEntries->size() == 1)
     {
         strValue = (*pmtEntries)[0];
+        return true;
     }
-    else if (pmtEntries)
+
+    if (pmtEntries)
     {
-        JLOG(j.warn()) << boost::str(
-            boost::format("Section [%s]: requires 1 line not %d lines.") %
-            strSection % pmtEntries->size());
+        JLOG(j.warn()) << "Section '" << strSection << "': requires 1 line not "
+                       << pmtEntries->size() << " lines.";
     }
 
-    return bSingle;
+    return false;
 }
 
 //------------------------------------------------------------------------------
@@ -461,9 +456,6 @@ Config::loadFromString(std::string const& fileContents)
     if (auto s = getIniFileSection(secConfig, SECTION_IPS_FIXED))
         IPS_FIXED = *s;
 
-    if (auto s = getIniFileSection(secConfig, SECTION_SNTP))
-        SNTP_SERVERS = *s;
-
     // if the user has specified ip:port then replace : with a space.
     {
         auto replaceColons = [](std::vector<std::string>& strVec) {