c475892 refactor(p2p): Extract the max commitments computation from HeadersSyncState():
ComputeMaxCommitments() still calls NodeClock::now(), so the boundary depends on global mock time in tests and fuzzing.
Could we pass the sampled node time in from TryLowWorkHeadersSync() instead?
That would avoid using a FakeNodeClock and multiple side-effectful NodeClock::now() calls in the tests.
<details><summary>Details</summary>
diff --git a/src/headerssync.cpp b/src/headerssync.cpp
index 87683a9ae5..9b1178f7e8 100644
--- a/src/headerssync.cpp
+++ b/src/headerssync.cpp
@@ -16,7 +16,7 @@
// CompressedHeader (we should re-calculate parameters if we compress further).
static_assert(sizeof(CompressedHeader) == 48);
-util::Expected<uint64_t, std::string> HeadersSyncState::ComputeMaxCommitments(const HeadersSyncParams& params, const CBlockIndex& chain_start)
+util::Expected<uint64_t, std::string> HeadersSyncState::ComputeMaxCommitments(const HeadersSyncParams& params, const CBlockIndex& chain_start, NodeSeconds now)
{
Assert(params.commitment_period > 0);
@@ -28,7 +28,6 @@ util::Expected<uint64_t, std::string> HeadersSyncState::ComputeMaxCommitments(co
// 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 NodeClock::time_point 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) {
diff --git a/src/headerssync.h b/src/headerssync.h
index 138b41c3ab..e0cd1237af 100644
--- a/src/headerssync.h
+++ b/src/headerssync.h
@@ -14,6 +14,7 @@
#include <util/bitdeque.h>
#include <util/expected.h>
#include <util/hasher.h>
+#include <util/time.h>
#include <deque>
#include <vector>
@@ -129,7 +130,7 @@ public:
/** Return the amount of work in the chain received during the PRESYNC phase. */
arith_uint256 GetPresyncWork() const { return m_current_chain_work; }
- static util::Expected<uint64_t, std::string> ComputeMaxCommitments(const HeadersSyncParams& params, const CBlockIndex& chain_start);
+ static util::Expected<uint64_t, std::string> ComputeMaxCommitments(const HeadersSyncParams& params, const CBlockIndex& chain_start, NodeSeconds now);
/** Construct a HeadersSyncState object representing a headers sync via this
* download-twice mechanism).
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 24272a2a60..9b50e4c3ab 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2819,7 +2819,7 @@ 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.
- const util::Expected max_commitments{HeadersSyncState::ComputeMaxCommitments(m_chainparams.HeadersSync(), chain_start_header)};
+ const util::Expected max_commitments{HeadersSyncState::ComputeMaxCommitments(m_chainparams.HeadersSync(), chain_start_header, Now<NodeSeconds>())};
if (!max_commitments) {
m_chainman.GetNotifications().fatalError(Untranslated(max_commitments.error()));
headers = {};
diff --git a/src/test/fuzz/headerssync.cpp b/src/test/fuzz/headerssync.cpp
index fcbd92492e..e0eacbd135 100644
--- a/src/test/fuzz/headerssync.cpp
+++ b/src/test/fuzz/headerssync.cpp
@@ -9,7 +9,6 @@
#include <test/fuzz/fuzz.h>
#include <test/fuzz/util.h>
#include <test/util/setup_common.h>
-#include <test/util/time.h>
#include <uint256.h>
#include <util/chaintype.h>
#include <util/expected.h>
@@ -63,7 +62,7 @@ FUZZ_TARGET(headers_sync_state, .init = initialize_headers_sync_state_fuzz)
CBlockHeader genesis_header{Params().GenesisBlock()};
CBlockIndex start_index(genesis_header);
- FakeNodeClock clock{ConsumeTime(fuzzed_data_provider, /*min=*/start_index.GetMedianTimePast() - 2 * MAX_FUTURE_BLOCK_TIME)};
+ const NodeSeconds now{ConsumeTime(fuzzed_data_provider, /*min=*/start_index.GetMedianTimePast() - 2 * MAX_FUTURE_BLOCK_TIME)};
const uint256 genesis_hash = genesis_header.GetHash();
start_index.phashBlock = &genesis_hash;
@@ -72,9 +71,9 @@ FUZZ_TARGET(headers_sync_state, .init = initialize_headers_sync_state_fuzz)
.commitment_period = fuzzed_data_provider.ConsumeIntegralInRange<size_t>(1, Params().HeadersSync().commitment_period * 2),
.redownload_buffer_size = fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, Params().HeadersSync().redownload_buffer_size * 2),
};
- const util::Expected max_commitments{HeadersSyncState::ComputeMaxCommitments(params, start_index)};
+ const util::Expected max_commitments{HeadersSyncState::ComputeMaxCommitments(params, start_index, now)};
if (!max_commitments) {
- assert(NodeClock::now() < NodeSeconds{std::chrono::seconds{start_index.GetMedianTimePast() - MAX_FUTURE_BLOCK_TIME}});
+ assert(now < NodeSeconds{std::chrono::seconds{start_index.GetMedianTimePast() - MAX_FUTURE_BLOCK_TIME}});
return;
}
arith_uint256 min_work{UintToArith256(ConsumeUInt256(fuzzed_data_provider))};
diff --git a/src/test/headers_sync_chainwork_tests.cpp b/src/test/headers_sync_chainwork_tests.cpp
index f88e27794f..f181291523 100644
--- a/src/test/headers_sync_chainwork_tests.cpp
+++ b/src/test/headers_sync_chainwork_tests.cpp
@@ -10,8 +10,8 @@
#include <pow.h>
#include <test/util/common.h>
#include <test/util/setup_common.h>
-#include <test/util/time.h>
#include <util/expected.h>
+#include <util/time.h>
#include <validation.h>
#include <cstddef>
@@ -81,13 +81,13 @@ struct HeadersGeneratorSetup : public RegTestingSetup {
return second_chain;
}
- util::Expected<HeadersSyncState, std::string> CreateState()
+ util::Expected<HeadersSyncState, std::string> CreateState(NodeSeconds now = Now<NodeSeconds>())
{
const HeadersSyncParams params{
.commitment_period = COMMITMENT_PERIOD,
.redownload_buffer_size = REDOWNLOAD_BUFFER_SIZE,
};
- util::Expected max_commitments{HeadersSyncState::ComputeMaxCommitments(params, chain_start)};
+ util::Expected max_commitments{HeadersSyncState::ComputeMaxCommitments(params, chain_start, now)};
if (!max_commitments) return util::Unexpected{std::move(max_commitments.error())};
return HeadersSyncState{/*id=*/0,
@@ -261,10 +261,9 @@ BOOST_AUTO_TEST_CASE(too_little_work)
BOOST_AUTO_TEST_CASE(system_clock_lagging_behind_chain_start)
{
- FakeNodeClock clock{(genesis.GetBlockTime() - MAX_FUTURE_BLOCK_TIME) * 1s};
- BOOST_CHECK(CreateState());
- clock -= 1s;
- BOOST_CHECK(!CreateState());
+ const NodeSeconds boundary{(genesis.GetBlockTime() - MAX_FUTURE_BLOCK_TIME) * 1s};
+ BOOST_CHECK(CreateState(boundary));
+ BOOST_CHECK(!CreateState(boundary - 1s));
}
BOOST_AUTO_TEST_SUITE_END()
</details>
