net: Trigger process abort when behind start block MTP

We should not proceed syncing headers from peers when the local system clock is incorrectly set.

A node with a system clock set too far back will typically fail early during startup when the chainstate detects the tip to be too far in the future. This means that in practice we don't expect the failure to ever happen in net_processing.cpp.

An exception is thrown from HeadersSyncState() in order to only compute the error condition once. An alternative would be to compute it a second time in TryLowWorkHeadersSync() to guard against calling HeadersSyncState(), and have an assert inside HeadersSyncState(). We shut down the process so possible resource leaks due to the exception should not be an issue, although none have been spotted. Throwing an exception also keeps the unit test straightforward.

Co-authored-by: Lőrinc <pap.lorinc@gmail.com>

src/headerssync.cpp

@@ -38,8 +38,16 @@ HeadersSyncState::HeadersSyncState(NodeId id,
     // exceeds this bound, because it's not possible for a consensus-valid
     // chain to be longer than this (at the current time -- in the future we
     // could try again, if necessary, to sync a longer chain).
-    const auto max_seconds_since_start{(Ticks<std::chrono::seconds>(NodeClock::now() - NodeSeconds{std::chrono::seconds{chain_start.GetMedianTimePast()}}))
-                                       + MAX_FUTURE_BLOCK_TIME};
+    const auto now{NodeClock::now()};
+    const int64_t max_seconds_since_start{Ticks<std::chrono::seconds>(now - NodeSeconds{std::chrono::seconds{chain_start.GetMedianTimePast()}})
+                                          + MAX_FUTURE_BLOCK_TIME};
+    if (max_seconds_since_start < 0) {
+        throw SystemClockError{strprintf(
+            "System clock is more than %d minutes behind chain start MTP (%s vs %s).",
+            MAX_FUTURE_BLOCK_TIME / 60,
+            FormatISO8601DateTime(TicksSinceEpoch<std::chrono::seconds>(now)),
+            FormatISO8601DateTime(chain_start.GetMedianTimePast()))};
+    }
     m_max_commitments = 6 * max_seconds_since_start / m_params.commitment_period;
 
     LogDebug(BCLog::NET, "Initial headers sync started with peer=%d: height=%i, max_commitments=%i, min_work=%s\n", m_id, m_current_height, m_max_commitments, m_minimum_required_work.ToString());

src/headerssync.h

@@ -15,6 +15,7 @@
 #include <util/hasher.h>
 
 #include <deque>
+#include <stdexcept>
 #include <vector>
 
 // A compressed CBlockHeader, which leaves out the prevhash
@@ -99,8 +100,13 @@ struct CompressedHeader {
  * sync (temporary, per-peer storage).
  */
 
-class HeadersSyncState {
+class HeadersSyncState
+{
 public:
+    struct SystemClockError : std::runtime_error {
+        using std::runtime_error::runtime_error;
+    };
+
     ~HeadersSyncState() = default;
 
     enum class State {
@@ -135,6 +141,8 @@ public:
      * consensus_params: parameters needed for difficulty adjustment validation
      * chain_start: best known fork point that the peer's headers branch from
      * minimum_required_work: amount of chain work required to accept the chain
+     *
+     * @throws SystemClockError if system clock is too far behind chain_start MTP.
      */
     HeadersSyncState(NodeId id, const Consensus::Params& consensus_params,
                      const HeadersSyncParams& params, const CBlockIndex& chain_start,

src/net_processing.cpp

@@ -3030,8 +3030,19 @@ bool PeerManagerImpl::TryLowWorkHeadersSync(Peer& peer, CNode& pfrom, const CBlo
             // of headers is known, some header in this set must be new, so
             // advancing to the first unknown header would be a small effect.
             LOCK(peer.m_headers_sync_mutex);
-            peer.m_headers_sync.reset(new HeadersSyncState(peer.m_id, m_chainparams.GetConsensus(),
-                m_chainparams.HeadersSync(), chain_start_header, minimum_chain_work));
+            try {
+                peer.m_headers_sync.reset(new HeadersSyncState(peer.m_id, m_chainparams.GetConsensus(),
+                    m_chainparams.HeadersSync(), chain_start_header, minimum_chain_work));
+            } catch (const HeadersSyncState::SystemClockError& e) {
+                // Typically we would expect the chain state loading logic to
+                // already have verified that the tip of the locally stored
+                // chain is <= system clock + MAX_FUTURE_BLOCK_TIME. Getting
+                // here is really unexpected.
+                const auto msg{strprintf("Failure when attempting to initiate headers sync: %s", e.what())};
+                std::cerr << msg << std::endl;
+                LogError("%s", msg);
+                std::abort();
+            }
 
             // Now a HeadersSyncState object for tracking this synchronization
             // is created, process the headers using it as normal. Failures are

src/test/headers_sync_chainwork_tests.cpp

@@ -259,10 +259,7 @@ BOOST_AUTO_TEST_CASE(system_clock_lagging_behind_chain_start)
     BOOST_CHECK_NO_THROW(CreateState());
 
     clock -= 1s;
-    // TODO: Fix - Being more than MAX_FUTURE_BLOCK_TIME behind the starting
-    // block leads HeadersSyncState() to compute a negative max_seconds_since_start
-    // which leads to very high HeadersSyncState::m_max_commitments.
-    BOOST_CHECK_NO_THROW(CreateState());
+    BOOST_CHECK_THROW(CreateState(), HeadersSyncState::SystemClockError);
 }
 
 BOOST_AUTO_TEST_SUITE_END()

test/functional/p2p_headers_sync_with_minchainwork.py

@@ -22,6 +22,7 @@ from test_framework.blocktools import (
 
 from test_framework.util import assert_equal
 
+import re
 import time
 
 NODE1_BLOCKS_REQUIRED = 15
@@ -150,7 +151,10 @@ class RejectLowDifficultyHeadersTest(BitcoinTestFramework):
         node.setmocktime(node.getblockheader(node.getblockhash(0))['mediantime'] - MAX_FUTURE_BLOCK_TIME - 1)
         p2p = node.add_p2p_connection(P2PInterface())
         p2p.send_without_ping(headers_message)
-        p2p.wait_for_getheaders(timeout=30, block_hash=hashPrevBlock)  # TODO: A negative elapsed interval should trigger fatal shutdown.
+        node.wait_until_stopped(expect_error=True, expected_ret_code=[-6,          # Unix
+                                                                      3,           # Windows native
+                                                                      0xC0000409], # Windows cross builds
+                                expected_stderr=re.compile("Failure when attempting to initiate headers sync: System clock"))
 
     def test_large_reorgs_can_succeed(self):
         self.log.info("Test that a 2000+ block reorg, starting from a point that is more than 2000 blocks before a locator entry, can succeed")