diff --git a/src/headerssync.cpp b/src/headerssync.cpp index 633ffef53ad..ef605735435 100644 --- a/src/headerssync.cpp +++ b/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(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(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(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()); diff --git a/src/headerssync.h b/src/headerssync.h index 6d720874411..65364bb069f 100644 --- a/src/headerssync.h +++ b/src/headerssync.h @@ -15,6 +15,7 @@ #include #include +#include #include // 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, diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 501b14eaee4..d76f6ffddfb 100644 --- a/src/net_processing.cpp +++ b/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 diff --git a/src/test/headers_sync_chainwork_tests.cpp b/src/test/headers_sync_chainwork_tests.cpp index e18b6f4a027..4385dd7d8ad 100644 --- a/src/test/headers_sync_chainwork_tests.cpp +++ b/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() diff --git a/test/functional/p2p_headers_sync_with_minchainwork.py b/test/functional/p2p_headers_sync_with_minchainwork.py index cd46a82c2a1..b1efe09cd9b 100755 --- a/test/functional/p2p_headers_sync_with_minchainwork.py +++ b/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")