From 1883cecb4d05788a02d673b1c7541d93fb9af2c7 Mon Sep 17 00:00:00 2001 From: Hodlinator <172445034+hodlinator@users.noreply.github.com> Date: Wed, 19 Aug 2026 09:41:04 +0200 Subject: [PATCH 1/2] test: Characterize lagging-clock headers presync MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The node currently continues low-work headers presync and requests more headers when its clock is more than `MAX_FUTURE_BLOCK_TIME` behind the chain-start MTP. Record this behavior before the follow-up rejects the invalid elapsed-time calculation. The unit test covers HeadersSyncState() behavior while the functional test covers net_processing.cpp behavior. Co-authored-by: Lőrinc --- src/test/headers_sync_chainwork_tests.cpp | 13 +++++++++++++ .../p2p_headers_sync_with_minchainwork.py | 8 ++++++++ 2 files changed, 21 insertions(+) diff --git a/src/test/headers_sync_chainwork_tests.cpp b/src/test/headers_sync_chainwork_tests.cpp index bba612f8b46..e18b6f4a027 100644 --- a/src/test/headers_sync_chainwork_tests.cpp +++ b/src/test/headers_sync_chainwork_tests.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include @@ -252,4 +253,16 @@ BOOST_AUTO_TEST_CASE(too_little_work) /*exp_locator_hash=*/std::nullopt); } +BOOST_AUTO_TEST_CASE(system_clock_lagging_behind_chain_start) +{ + FakeNodeClock clock{(chain_start.GetBlockTime() - MAX_FUTURE_BLOCK_TIME) * 1s}; + 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_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 1dc38faadb2..cd46a82c2a1 100755 --- a/test/functional/p2p_headers_sync_with_minchainwork.py +++ b/test/functional/p2p_headers_sync_with_minchainwork.py @@ -15,6 +15,7 @@ from test_framework.messages import ( ) from test_framework.blocktools import ( + MAX_FUTURE_BLOCK_TIME, NORMAL_GBT_REQUEST_PARAMS, create_block, ) @@ -144,6 +145,13 @@ class RejectLowDifficultyHeadersTest(BitcoinTestFramework): # getpeerinfo should show a sync in progress assert_equal(node.getpeerinfo()[0]['presynced_headers'], 2000) + self.log.info("Test whether a lagging clock aborts low-work headers sync") + node.disconnect_p2ps() + 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. + 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") From ff3e2e4ebdcef21f1c3a81ae08e737adb7b78a78 Mon Sep 17 00:00:00 2001 From: Hodlinator <172445034+hodlinator@users.noreply.github.com> Date: Wed, 19 Aug 2026 09:41:54 +0200 Subject: [PATCH 2/2] net: Trigger process abort when behind start block MTP MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/headerssync.cpp | 12 ++++++++++-- src/headerssync.h | 10 +++++++++- src/net_processing.cpp | 15 +++++++++++++-- src/test/headers_sync_chainwork_tests.cpp | 5 +---- .../p2p_headers_sync_with_minchainwork.py | 6 +++++- 5 files changed, 38 insertions(+), 10 deletions(-) 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")