This approach of setting up m_max_commitments for a later failure feels safe in the context of pre-existing code, but the error is quite indirect, and the cause of the error becomes unclear.
Have you considered throwing an exception from the constructor instead?
<details><summary>Potential diff</summary>
diff --git a/src/headerssync.cpp b/src/headerssync.cpp
index 30a57702c8..a34fb37ca7 100644
--- a/src/headerssync.cpp
+++ b/src/headerssync.cpp
@@ -6,10 +6,13 @@
#include <logging.h>
#include <pow.h>
+#include <tinyformat.h>
#include <util/check.h>
#include <util/time.h>
#include <util/vector.h>
+#include <stdexcept>
+
// Our memory analysis in headerssync-params.py assumes this many bytes for a
// CompressedHeader (we should re-calculate parameters if we compress further).
static_assert(sizeof(CompressedHeader) == 48);
@@ -40,7 +43,10 @@ HeadersSyncState::HeadersSyncState(NodeId id,
// could try again, if necessary, to sync a longer chain).
const int64_t max_seconds_since_start{
Ticks<std::chrono::seconds>(NodeClock::now() - NodeSeconds{std::chrono::seconds{chain_start.GetMedianTimePast()}}) + MAX_FUTURE_BLOCK_TIME};
- m_max_commitments = max_seconds_since_start > 0 ? 6 * max_seconds_since_start / m_params.commitment_period : 0;
+ if (max_seconds_since_start <= 0) {
+ throw std::runtime_error{strprintf("System clock is earlier than chain-start's MTP minus %d seconds", MAX_FUTURE_BLOCK_TIME)};
+ }
+ 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/test/fuzz/headerssync.cpp b/src/test/fuzz/headerssync.cpp
index 742a496707..0c363c0183 100644
--- a/src/test/fuzz/headerssync.cpp
+++ b/src/test/fuzz/headerssync.cpp
@@ -16,6 +16,8 @@
#include <validation.h>
#include <iterator>
+#include <memory>
+#include <stdexcept>
#include <vector>
static void initialize_headers_sync_state_fuzz()
@@ -70,11 +72,16 @@ FUZZ_TARGET(headers_sync_state, .init = initialize_headers_sync_state_fuzz)
.redownload_buffer_size = fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, Params().HeadersSync().redownload_buffer_size * 2),
};
arith_uint256 min_work{UintToArith256(ConsumeUInt256(fuzzed_data_provider))};
- FuzzedHeadersSyncState headers_sync(
- params,
- /*commit_offset=*/fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, params.commitment_period - 1),
- /*chain_start=*/start_index,
- /*minimum_required_work=*/min_work);
+ std::unique_ptr<FuzzedHeadersSyncState> headers_sync;
+ try {
+ headers_sync = std::make_unique<FuzzedHeadersSyncState>(
+ params,
+ /*commit_offset=*/fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, params.commitment_period - 1),
+ /*chain_start=*/start_index,
+ /*minimum_required_work=*/min_work);
+ } catch (const std::runtime_error& e) {
+ assert(strcmp(e.what(), "System clock is earlier than chain-start's MTP minus 7200 seconds") == 0);
+ }
// Store headers for potential redownload phase.
std::vector<CBlockHeader> all_headers;
@@ -105,14 +112,14 @@ FUZZ_TARGET(headers_sync_state, .init = initialize_headers_sync_state_fuzz)
}
if (headers.empty()) return;
- auto result = headers_sync.ProcessNextHeaders(headers, fuzzed_data_provider.ConsumeBool());
+ auto result = headers_sync->ProcessNextHeaders(headers, fuzzed_data_provider.ConsumeBool());
requested_more = result.request_more;
if (result.request_more) {
if (presync) {
all_headers.insert(all_headers.cend(), headers.cbegin(), headers.cend());
- if (headers_sync.GetState() == HeadersSyncState::State::REDOWNLOAD) {
+ if (headers_sync->GetState() == HeadersSyncState::State::REDOWNLOAD) {
presync = false;
redownloaded_it = all_headers.cbegin();
@@ -122,7 +129,7 @@ FUZZ_TARGET(headers_sync_state, .init = initialize_headers_sync_state_fuzz)
}
}
- (void)headers_sync.NextHeadersRequestLocator();
+ (void)headers_sync->NextHeadersRequestLocator();
}
}
}
diff --git a/src/test/headers_sync_chainwork_tests.cpp b/src/test/headers_sync_chainwork_tests.cpp
index a4b917fada..dd71f8a2a2 100644
--- a/src/test/headers_sync_chainwork_tests.cpp
+++ b/src/test/headers_sync_chainwork_tests.cpp
@@ -14,6 +14,7 @@
#include <validation.h>
#include <cstddef>
+#include <stdexcept>
#include <vector>
#include <boost/test/unit_test.hpp>
@@ -145,13 +146,8 @@ BOOST_FIXTURE_TEST_SUITE(headers_sync_chainwork_tests, HeadersGeneratorSetup)
BOOST_AUTO_TEST_CASE(future_chain_start_mtp_bounds_commitments)
{
const NodeClockContext clock_ctx{std::chrono::seconds{genesis.GetBlockTime() - MAX_FUTURE_BLOCK_TIME - 1}};
- HeadersSyncState hss{CreateState(/*commitment_period=*/1)};
-
- CHECK_RESULT(hss.ProcessNextHeaders({{FirstChain().front()}}, /*full_headers_message=*/true),
- hss, /*exp_state=*/State::FINAL,
- /*exp_success=*/false, /*exp_request_more=*/false,
- /*exp_headers_size=*/0, /*exp_pow_validated_prev=*/std::nullopt,
- /*exp_locator_hash=*/std::nullopt);
+ BOOST_CHECK_EXCEPTION(CreateState(/*commitment_period=*/1), std::runtime_error,
+ HasReason("System clock is earlier than chain-start's MTP minus 7200 seconds"));
}
BOOST_AUTO_TEST_CASE(sneaky_redownload)
</details>
If we figure out the current approach in the PR is more desirable, maybe we could repeat the m_max_commitments value in the log output when aborting and raise the log level to info?
- LogDebug(BCLog::NET, "Initial headers sync aborted with peer=%d: exceeded max commitments at height=%i (presync phase)\n", m_id, next_height);
+ LogInfo("Initial headers sync aborted with peer=%d: exceeded max commitments (%d) at height=%i (presync phase)", m_id, m_max_commitments, next_height);
A third alternative would be to emit a LogWarning() when we detect this condition in the constructor, I think that's more appropriate if you want to avoid adding the exception.
