p2p: Correct unrealistic headerssync unit test behavior#32579
Merged
Conversation
Contributor
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32579. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
954971a to
315736d
Compare
Contributor
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
l0rinc
suggested changes
May 27, 2025
l0rinc
left a comment
Contributor
There was a problem hiding this comment.
Concept ACK – this PR replaces the imminently outdated hard-coded test target of 15'000 headers with a value derived from the live REDOWNLOAD_BUFFER_SIZE constant.
Moving both header-sync tuning constants into a shared header, and making the unit test reference them, keeps the test exercising the 'buffer-full' path as the parameters grow each release.
Recommendations:
- split the series into more commits, separating low-risk refactors from ones that change behaviour: strict move-only; refactor / cleanup with no behavioural change; extra assertions; wiring the test to the new parameter header;
- in the test file, wrap each independent
ProcessNextHeaderscall inside{}blocks and use structured bindings so temporary variables aren’t reused accidentally; - rename the new file to
headerssync_params.h(underscore instead of-) for consistency with other similar headers; - update
contrib/devtools/README.mdto reference the header instead of the .cpp; - Tweak a few comments whose wording is unclear or slightly misleading.
Contributor
|
Concept ACK |
315736d to
486e8ca
Compare
hodlinator
commented
May 30, 2025
hodlinator
left a comment
Contributor
Author
There was a problem hiding this comment.
Thanks for your feedback so far @l0rinc! Took most of your suggestions (#32579 (review)).
486e8ca to
bbe5609
Compare
Member
|
Concept ACK |
hodlinator
commented
Jun 2, 2025
bbe5609 to
42e7460
Compare
hodlinator
commented
Jun 4, 2025
Contributor
Author
There was a problem hiding this comment.
Latest push (42e746096b93b8026569817d8925750089d6daf4, compare):
- Relaxes the hardcoding of the header commitment period and the redownload buffer size in
HeadersSyncState.- This makes things more consistent with the constructor already taking
Consensus::Params(implying different chains are supported). - Moves specification of the above constants from the top of headerssync.cpp into kernel/chainparams.cpp (similar to many other constants).
- Returns the chainwork test suite back to only using an integer literal for
TARGET_BLOCKS(instead of depending on an #included constant). Also now uses test-local values for commitment period and redownload buffer size.
- This makes things more consistent with the constructor already taking
- Adds a warning in
HeadersSyncState::PopHeadersReadyForAcceptance()for the case when we reach minimum PoW before exceeding the redownload buffer size together with tests.
(Updated PR description).
This was referenced Jun 5, 2025
Closed
l0rinc
suggested changes
Jun 20, 2025
Contributor
There was a problem hiding this comment.
I might have gone a bit overboard with the test review, but I think we can make it a lot less noisy - here's the patch of many of my suggestions for convenience:
Patch
diff --git a/src/kernel/chainparams.cpp b/src/kernel/chainparams.cpp
index d64dd43aba..658ea7f2c3 100644
--- a/src/kernel/chainparams.cpp
+++ b/src/kernel/chainparams.cpp
@@ -193,8 +193,7 @@ public:
// Generated by headerssync-params.py on 2025-03-04.
m_headers_sync_params = HeadersSyncParams{
.commitment_period = 624,
- .redownload_buffer_size = 14827, // 14827/624 = ~23.8 commitments
- };
+ .redownload_buffer_size = 14827}; // ~24 commitments
}
};
diff --git a/src/test/headers_sync_chainwork_tests.cpp b/src/test/headers_sync_chainwork_tests.cpp
index 1f85c69838..ccbbc1be50 100644
--- a/src/test/headers_sync_chainwork_tests.cpp
+++ b/src/test/headers_sync_chainwork_tests.cpp
@@ -13,10 +13,13 @@
#include <validation.h>
#include <cstddef>
+#include <optional>
#include <vector>
#include <boost/test/unit_test.hpp>
+using State = HeadersSyncState::State;
+
constexpr size_t TARGET_BLOCKS{15'000};
constexpr arith_uint256 CHAIN_WORK{TARGET_BLOCKS * 2};
constexpr size_t REDOWNLOAD_BUFFER_SIZE{TARGET_BLOCKS - (MAX_HEADERS_RESULTS + 123)};
@@ -37,18 +40,18 @@ static void FindProofOfWork(CBlockHeader& starting_header)
* prev_time, and with a fixed merkle root hash.
*/
static void GenerateHeaders(std::vector<CBlockHeader>& headers,
- size_t count, const uint256& starting_hash, const int nVersion, int prev_time,
- const uint256& merkle_root, const uint32_t nBits)
+ size_t count, const uint256& starting_hash, int nVersion, int prev_time,
+ const uint256& merkle_root, uint32_t nBits)
{
- uint256 prev_hash = starting_hash;
+ uint256 prev_hash{starting_hash};
while (headers.size() < count) {
headers.emplace_back();
- CBlockHeader& next_header = headers.back();
+ CBlockHeader& next_header{headers.back()};
next_header.nVersion = nVersion;
next_header.hashPrevBlock = prev_hash;
next_header.hashMerkleRoot = merkle_root;
- next_header.nTime = prev_time+1;
+ next_header.nTime = prev_time + 1;
next_header.nBits = nBits;
FindProofOfWork(next_header);
@@ -57,220 +60,167 @@ static void GenerateHeaders(std::vector<CBlockHeader>& headers,
}
}
-static HeadersSyncState CreateState(const CBlockIndex* chain_start, const CChainParams& chain_params)
+static HeadersSyncState CreateState(const CChainParams& chain_params, const HeadersSyncParams& sync_params, const CBlockIndex* chain_start)
{
return {/*id=*/0,
chain_params.GetConsensus(),
- HeadersSyncParams{
- .commitment_period = COMMITMENT_PERIOD,
- .redownload_buffer_size = REDOWNLOAD_BUFFER_SIZE,
- },
+ sync_params,
chain_start,
- /*minimum_required_work=*/CHAIN_WORK};
+ /*minimum_required_work=*/CHAIN_WORK};
}
-BOOST_FIXTURE_TEST_SUITE(headers_sync_chainwork_tests, RegTestingSetup)
-
-// In this test, we construct two sets of headers from genesis, one with
-// sufficient proof of work and one without.
-// 1. We deliver the first set of headers and verify that the headers sync state
-// updates to the REDOWNLOAD phase successfully.
-// 2. Then we deliver the second set of headers and verify that they fail
-// processing (presumably due to commitments not matching).
-static void SneakyRedownload(const CBlockIndex* chain_start,
- const std::vector<CBlockHeader>& first_chain,
- const std::vector<CBlockHeader>& second_chain);
-// 3. Verify that repeating with the first set of headers in both phases is
-// successful.
-static void HappyPath(const CBlockIndex* chain_start,
- const std::vector<CBlockHeader>& first_chain);
-// 4. Finally, repeat the second set of headers in both phases to demonstrate
-// behavior when the chain a peer provides has too little work.
-static void TooLittleWork(const CBlockIndex* chain_start,
- const std::vector<CBlockHeader>& second_chain);
-
-static void TooBigBuffer(const CBlockIndex* chain_start,
- const std::vector<CBlockHeader>& first_chain);
-
-BOOST_AUTO_TEST_CASE(headers_sync_state)
+// Standard set of checks common to all scenarios; macro keeps failure lines at the call-site.
+#define CHECK_RESULT(res, state, exp_state, exp_success, exp_request_more, exp_headers_size) \
+ do { \
+ const auto& [res_headers, res_success, res_request_more]{res}; \
+ BOOST_REQUIRE_EQUAL(state.GetState(), exp_state); \
+ BOOST_CHECK_EQUAL(res_headers.size(), exp_headers_size); \
+ BOOST_CHECK_EQUAL(res_success, exp_success); \
+ BOOST_CHECK_EQUAL(res_request_more, exp_request_more); \
+ } while (false)
+
+// Deliver the first set of headers from genesis to reach REDOWNLOAD,
+// then deliver the second set and verify that commitments don’t match.
+static void SneakyRedownload(const CBlockIndex* chain_start, const std::vector<CBlockHeader>& chain1, const std::vector<CBlockHeader>& chain2)
{
- std::vector<CBlockHeader> first_chain;
- std::vector<CBlockHeader> second_chain;
-
- const auto genesis{Params().GenesisBlock()};
-
- // Generate headers for two different chains (using differing merkle roots
- // to ensure the headers are different).
- GenerateHeaders(first_chain, TARGET_BLOCKS - 1, genesis.GetHash(), genesis.nVersion,
- genesis.nTime, /*merkle_root=*/uint256::ZERO, genesis.nBits);
- GenerateHeaders(second_chain, TARGET_BLOCKS - 2, genesis.GetHash(), genesis.nVersion,
- genesis.nTime, /*merkle_root=*/uint256::ONE, genesis.nBits);
-
- const CBlockIndex* chain_start = WITH_LOCK(::cs_main, return m_node.chainman->m_blockman.LookupBlockIndex(genesis.GetHash()));
+ HeadersSyncState state{CreateState(Params(), HeadersSyncParams{COMMITMENT_PERIOD, REDOWNLOAD_BUFFER_SIZE}, chain_start)};
+ const auto genesis_hash{Params().GenesisBlock().GetHash()};
- SneakyRedownload(chain_start, first_chain, second_chain);
- HappyPath(chain_start, first_chain);
- TooLittleWork(chain_start, second_chain);
- TooBigBuffer(chain_start, first_chain);
+ const std::span headers{chain1};
+ CHECK_RESULT(state.ProcessNextHeaders(headers.first(1), /*full_headers_message=*/true),
+ state, State::PRESYNC,
+ /*exp_success=*/true,
+ /*exp_request_more=*/true,
+ /*exp_headers_size=*/0);
+ BOOST_CHECK_EQUAL(state.NextHeadersRequestLocator().vHave.front(), chain1.front().GetHash());
+
+ // Pretend the message is still "full", so we don't abort.
+ // This chain should look valid, and we should have met the proof-of-work
+ // requirement during PRESYNC and transitioned to REDOWNLOAD.
+ CHECK_RESULT(state.ProcessNextHeaders(headers.last(headers.size() - 1), /*full_headers_message=*/true),
+ state, State::REDOWNLOAD,
+ /*exp_success=*/true,
+ /*exp_request_more=*/true,
+ /*exp_headers_size=*/0);
+ BOOST_CHECK_EQUAL(state.NextHeadersRequestLocator().vHave.front(), genesis_hash);
+
+ // Try to sneakily feed back the second chain during REDOWNLOAD.
+ CHECK_RESULT(state.ProcessNextHeaders(chain2, /*full_headers_message=*/true),
+ state, State::FINAL,
+ /*exp_success=*/false,
+ /*exp_request_more=*/false,
+ /*exp_headers_size=*/0);
}
-static void SneakyRedownload(const CBlockIndex* chain_start,
- const std::vector<CBlockHeader>& first_chain,
- const std::vector<CBlockHeader>& second_chain)
+// Verify that repeating with the first set of headers in both phases is successful.
+static void HappyPath(const CBlockIndex* chain_start, const std::vector<CBlockHeader>& chain1)
{
- // Feed the first chain to HeadersSyncState, by delivering 1 header
- // initially and then the rest.
- HeadersSyncState hss{CreateState(chain_start, Params())};
- {
- // Just feed one header and check state.
- auto result{hss.ProcessNextHeaders(std::span{first_chain.begin(), 1}, true)};
- BOOST_REQUIRE_EQUAL(hss.GetState(), HeadersSyncState::State::PRESYNC);
- BOOST_CHECK(result.success);
- BOOST_CHECK(result.request_more);
- BOOST_CHECK_EQUAL(result.pow_validated_headers.size(), 0);
- BOOST_CHECK_EQUAL(hss.NextHeadersRequestLocator().vHave.front(), first_chain.front().GetHash());
- }
-
- {
- // Pretend the message is still "full", so we don't abort.
- auto result{hss.ProcessNextHeaders(std::span{first_chain.begin() + 1, first_chain.end()}, true)};
- // This chain should look valid, and we should have met the proof-of-work
- // requirement during PRESYNC and transitioned to REDOWNLOAD.
- BOOST_REQUIRE_EQUAL(hss.GetState(), HeadersSyncState::State::REDOWNLOAD);
- BOOST_CHECK(result.success);
- BOOST_CHECK(result.request_more);
- BOOST_CHECK_EQUAL(result.pow_validated_headers.size(), 0);
- // The locator should reset to genesis.
- BOOST_CHECK_EQUAL(hss.NextHeadersRequestLocator().vHave.front(), Params().GenesisBlock().GetHash());
- }
+ // Feed the first chain twice.
+ HeadersSyncState state{CreateState(Params(), HeadersSyncParams{COMMITMENT_PERIOD, REDOWNLOAD_BUFFER_SIZE}, chain_start)};
+ const auto genesis_hash{Params().GenesisBlock().GetHash()};
+ const std::span headers{chain1};
- {
- // Try to sneakily feed back the second chain during REDOWNLOAD.
- auto result{hss.ProcessNextHeaders(second_chain, true)};
- BOOST_REQUIRE_EQUAL(hss.GetState(), HeadersSyncState::State::FINAL);
- BOOST_CHECK(!result.success); // Foiled! We detected mismatching headers.
- BOOST_CHECK(!result.request_more);
- BOOST_CHECK_EQUAL(result.pow_validated_headers.size(), 0);
- }
+ // During normal operation we shouldn't get the redownload buffer size warning.
+ ASSERT_NO_DEBUG_LOG(BUFFER_SIZE_WARNING);
+
+ // Switched from PRESYNC to REDOWNLOAD after reaching sufficient work:
+ CHECK_RESULT(state.ProcessNextHeaders(headers, /*full_headers_message=*/true),
+ state, State::REDOWNLOAD,
+ /*exp_success=*/true,
+ /*exp_request_more=*/true,
+ /*exp_headers_size=*/0);
+ BOOST_CHECK_EQUAL(state.NextHeadersRequestLocator().vHave.front(), genesis_hash);
+
+ // Process only so that the internal threshold isn't met, meaning validated headers shouldn't be returned yet:
+ CHECK_RESULT(state.ProcessNextHeaders(headers.first(REDOWNLOAD_BUFFER_SIZE), /*full_headers_message=*/true),
+ state, State::REDOWNLOAD,
+ /*exp_success=*/true,
+ /*exp_request_more=*/true,
+ /*exp_headers_size=*/0);
+ BOOST_CHECK_EQUAL(state.NextHeadersRequestLocator().vHave.front(), chain1[REDOWNLOAD_BUFFER_SIZE - 1].GetHash());
+
+ // Next header should make us exceed the threshold, but still not be done:
+ const auto res_threshold{state.ProcessNextHeaders(headers.subspan(REDOWNLOAD_BUFFER_SIZE, 1), /*full_headers_message=*/true)};
+ CHECK_RESULT(res_threshold,
+ state, State::REDOWNLOAD,
+ /*exp_success=*/true,
+ /*exp_request_more=*/true,
+ /*exp_headers_size=*/1);
+ BOOST_CHECK_EQUAL(state.NextHeadersRequestLocator().vHave.front(), chain1[REDOWNLOAD_BUFFER_SIZE].GetHash());
+
+ // Feed in remaining headers, meeting the work threshold again and completing the REDOWNLOAD phase:
+ CHECK_RESULT(state.ProcessNextHeaders(headers.subspan(REDOWNLOAD_BUFFER_SIZE + 1), /*full_headers_message=*/true),
+ state, State::FINAL,
+ /*exp_success=*/true,
+ /*exp_request_more=*/false,
+ /*exp_headers_size=*/chain1.size() - 1);
+ BOOST_CHECK_EQUAL(res_threshold.pow_validated_headers.front().hashPrevBlock, genesis_hash);
}
-static void HappyPath(const CBlockIndex* chain_start,
- const std::vector<CBlockHeader>& first_chain)
+// Repeat the second set of headers in both phases to demonstrate
+// behavior when the chain a peer provides has too little work.
+static void TooLittleWork(const CBlockIndex* chain_start, const std::vector<CBlockHeader>& chain2)
{
- // This time we feed the first chain twice.
- HeadersSyncState hss{CreateState(chain_start, Params())};
-
- // During normal operation we shouldn't get the redownload buffer size warning.
- DebugLogHelper forbidden{BUFFER_SIZE_WARNING, [](const std::string* str) {
- if (str == nullptr) {
- return false; // Disable exception for not finding a match.
- } else {
- throw std::runtime_error{strprintf("'%s' should not exist in debug log\n", str)};
- }
- }};
-
- const auto genesis_hash{Params().GenesisBlock().GetHash()};
- {
- auto result{hss.ProcessNextHeaders(first_chain, true)};
- // Switched from PRESYNC to REDOWNLOAD after reaching sufficient work:
- BOOST_REQUIRE_EQUAL(hss.GetState(), HeadersSyncState::State::REDOWNLOAD);
- BOOST_CHECK(result.success);
- BOOST_CHECK(result.request_more);
- BOOST_CHECK_EQUAL(result.pow_validated_headers.size(), 0);
- // The locator should reset to genesis.
- BOOST_CHECK_EQUAL(hss.NextHeadersRequestLocator().vHave.front(), genesis_hash);
- }
-
- {
- // Process only so that the internal threshold isn't met, meaning validated
- // headers shouldn't be returned yet:
- auto result{hss.ProcessNextHeaders(std::span{first_chain.begin(), REDOWNLOAD_BUFFER_SIZE}, true)};
- // Not done:
- BOOST_REQUIRE_EQUAL(hss.GetState(), HeadersSyncState::State::REDOWNLOAD);
- BOOST_CHECK(result.success);
- BOOST_CHECK(result.request_more);
- BOOST_CHECK_EQUAL(result.pow_validated_headers.size(), 0);
- BOOST_CHECK_EQUAL(hss.NextHeadersRequestLocator().vHave.front(), first_chain[REDOWNLOAD_BUFFER_SIZE - 1].GetHash());
- }
-
- CBlockHeader first_after_genesis;
- {
- // Next header should make us exceed the threshold, but still not be done:
- auto result{hss.ProcessNextHeaders(std::span{first_chain.begin() + REDOWNLOAD_BUFFER_SIZE, 1}, true)};
- BOOST_REQUIRE_EQUAL(hss.GetState(), HeadersSyncState::State::REDOWNLOAD);
- BOOST_CHECK(result.success);
- BOOST_CHECK(result.request_more);
- BOOST_REQUIRE_EQUAL(result.pow_validated_headers.size(), 1);
- BOOST_CHECK_EQUAL(hss.NextHeadersRequestLocator().vHave.front(), first_chain[REDOWNLOAD_BUFFER_SIZE].GetHash());
- first_after_genesis = result.pow_validated_headers.front();
- BOOST_CHECK_EQUAL(first_after_genesis.hashPrevBlock, genesis_hash);
- }
-
- {
- // Feed in remaining headers, meeting the work threshold again and
- // completing the REDOWNLOAD phase.
- auto result{hss.ProcessNextHeaders(std::span{first_chain.begin() + REDOWNLOAD_BUFFER_SIZE + 1, first_chain.end()}, true)};
- // Nothing left for the sync logic to do:
- BOOST_REQUIRE_EQUAL(hss.GetState(), HeadersSyncState::State::FINAL);
- BOOST_CHECK(result.success);
- BOOST_CHECK(!result.request_more);
- // All headers except the one already returned above:
- BOOST_REQUIRE_EQUAL(result.pow_validated_headers.size(), first_chain.size() - 1);
- BOOST_CHECK_EQUAL(result.pow_validated_headers.front().hashPrevBlock, first_after_genesis.GetHash());
- }
+ // Verify that just trying to process the second chain would not succeed (too little work).
+ HeadersSyncState state{CreateState(Params(), HeadersSyncParams{COMMITMENT_PERIOD, REDOWNLOAD_BUFFER_SIZE}, chain_start)};
+ const std::span headers{chain2};
+
+ // Pretend just the first message is "full", so we don't abort.
+ CHECK_RESULT(state.ProcessNextHeaders(headers.first(1), /*full_headers_message=*/true),
+ state, State::PRESYNC,
+ /*exp_success=*/true,
+ /*exp_request_more=*/true,
+ /*exp_headers_size=*/0);
+
+ // Non-full message causes sync to end with no headers accepted.
+ CHECK_RESULT(state.ProcessNextHeaders(headers.subspan(1), /*full_headers_message=*/false),
+ state, State::FINAL,
+ /*exp_success=*/true,
+ /*exp_request_more=*/false,
+ /*exp_headers_size=*/0);
}
-static void TooLittleWork(const CBlockIndex* chain_start,
- const std::vector<CBlockHeader>& second_chain)
+// Scenario that intentionally uses an oversized buffer to trigger the warning.
+static void TooBigBuffer(const CBlockIndex* chain_start, const std::vector<CBlockHeader>& chain1)
{
- // Verify that just trying to process the second chain would not succeed
- // (too little work).
- HeadersSyncState hss{CreateState(chain_start, Params())};
- BOOST_REQUIRE_EQUAL(hss.GetState(), HeadersSyncState::State::PRESYNC);
- {
- // Pretend just the first message is "full", so we don't abort.
- auto result{hss.ProcessNextHeaders(std::span{second_chain.begin(), 1}, true)};
- BOOST_REQUIRE_EQUAL(hss.GetState(), HeadersSyncState::State::PRESYNC);
- BOOST_CHECK(result.success);
- BOOST_CHECK(result.request_more);
- BOOST_CHECK_EQUAL(result.pow_validated_headers.size(), 0);
- }
-
- {
- // Tell the sync logic that the headers message was not full, implying no
- // more headers can be requested. For a low-work-chain, this should cause
- // the sync to end with no headers for acceptance.
- auto result{hss.ProcessNextHeaders(std::span{second_chain.begin() + 1, second_chain.end()}, false)};
- BOOST_REQUIRE_EQUAL(hss.GetState(), HeadersSyncState::State::FINAL);
- BOOST_CHECK(!result.request_more);
- BOOST_CHECK_EQUAL(result.pow_validated_headers.size(), 0);
- // Nevertheless, no validation errors should have been detected with the
- // chain:
- BOOST_CHECK(result.success);
- }
+ // Intentionally too big redownload buffer in order to trigger warning.
+ HeadersSyncState state{CreateState(Params(), HeadersSyncParams{COMMITMENT_PERIOD, chain1.size()}, chain_start)};
+
+ CHECK_RESULT(state.ProcessNextHeaders(chain1, /*full_headers_message=*/true),
+ state, State::REDOWNLOAD,
+ /*exp_success=*/true,
+ /*exp_request_more=*/true,
+ /*exp_headers_size=*/0);
+
+ ASSERT_DEBUG_LOG(BUFFER_SIZE_WARNING);
+ CHECK_RESULT(state.ProcessNextHeaders(chain1, /*full_headers_message=*/true),
+ state, State::FINAL,
+ /*exp_success=*/true,
+ /*exp_request_more=*/false,
+ /*exp_headers_size=*/chain1.size());
}
-static void TooBigBuffer(const CBlockIndex* chain_start,
- const std::vector<CBlockHeader>& first_chain)
+BOOST_FIXTURE_TEST_SUITE(headers_sync_chainwork_tests, RegTestingSetup)
+
+BOOST_AUTO_TEST_CASE(headers_sync_state)
{
- // Intentionally too big redownload buffer in order to trigger warning.
- HeadersSyncState hss{/*id=*/0,
- Params().GetConsensus(),
- HeadersSyncParams{
- .commitment_period = COMMITMENT_PERIOD,
- .redownload_buffer_size = first_chain.size(),
- },
- chain_start,
- /*minimum_required_work=*/CHAIN_WORK};
- (void)hss.ProcessNextHeaders(first_chain, true);
- BOOST_REQUIRE_EQUAL(hss.GetState(), HeadersSyncState::State::REDOWNLOAD);
-
- {
- ASSERT_DEBUG_LOG(BUFFER_SIZE_WARNING);
- auto result{hss.ProcessNextHeaders(first_chain, true)};
- BOOST_REQUIRE_EQUAL(hss.GetState(), HeadersSyncState::State::FINAL);
- BOOST_CHECK(result.success);
- }
+ const auto& genesis{Params().GenesisBlock()};
+ const uint256 genesis_hash{genesis.GetHash()};
+
+ // Generate headers for two different chains (using differing merkle roots
+ // to ensure the headers are different).
+ std::vector<CBlockHeader> chain1{};
+ GenerateHeaders(chain1, TARGET_BLOCKS - 1, genesis_hash, genesis.nVersion, genesis.nTime,
+ /*merkle_root=*/uint256::ZERO, genesis.nBits);
+ std::vector<CBlockHeader> chain2{};
+ GenerateHeaders(chain2, TARGET_BLOCKS - 2, genesis_hash, genesis.nVersion, genesis.nTime,
+ /*merkle_root=*/uint256::ONE, genesis.nBits);
+
+ const CBlockIndex* chain_start{WITH_LOCK(::cs_main, return m_node.chainman->m_blockman.LookupBlockIndex(genesis_hash))};
+ SneakyRedownload(chain_start, chain1, chain2);
+ HappyPath(chain_start, chain1);
+ TooLittleWork(chain_start, chain2);
+ TooBigBuffer(chain_start, chain1);
}
BOOST_AUTO_TEST_SUITE_END()
diff --git a/src/test/util/logging.h b/src/test/util/logging.h
index 73ac23825f..622ff58938 100644
--- a/src/test/util/logging.h
+++ b/src/test/util/logging.h
@@ -36,6 +36,16 @@ public:
~DebugLogHelper() { check_found(); }
};
+// Test fails if the pattern *DOES NOT* show up.
#define ASSERT_DEBUG_LOG(message) DebugLogHelper UNIQUE_NAME(debugloghelper)(message)
+// Test fails if the pattern *DOES* show up.
+#define ASSERT_NO_DEBUG_LOG(message) \
+ DebugLogHelper UNIQUE_NAME(nologhelper){ \
+ message, \
+ [](const std::string* line) { \
+ if (line) throw std::runtime_error("unexpected log line: " + *line); \
+ return false; /* suppress default 'not found' failure */ \
+ } \
+ }
#endif // BITCOIN_TEST_UTIL_LOGGING_Hb006f4b to
9c4a045
Compare
hodlinator
commented
Jun 23, 2025
hodlinator
left a comment
Contributor
Author
There was a problem hiding this comment.
Thanks for the extensive review @l0rinc! Implemented many of the suggestions and answered the other points.
Removed the commit breaking out scopes in favor of the suggested CHECK_RESULT macro (#32579 (comment)). Broke out the comment improvements into their own commit ahead of that since it gives a "skeleton" for the next commit's diff to hang off of.
9c4a045 to
14e933a
Compare
l0rinc
approved these changes
Jun 24, 2025
l0rinc
left a comment
Contributor
There was a problem hiding this comment.
I find the current layout of tiny, focused commits a lot easier to follow, thanks!
Left a few comments, after that I will test it again locally and I can ACK.
7fe7341 to
c3bc47c
Compare
33d550d to
794a171
Compare
Contributor
|
reACK 794a17186d3019713d29213bedd866baa1c81378 - the only change since last ACK was the span nits being applied |
…unctions Made arith_uint256 constexpr-constructible so it can be used for compile time constants. Co-authored-by: Lőrinc <pap.lorinc@gmail.com>
More lightweight than vectors which needed to be copied in tests. Also good to get rid of headers_batch-vector before breaking up test.
Helps logically separate the scenarios being tested. Also adds missing comment for part 4. (unique_ptrs and ProcessingResults will be cleaned up in next commit).
+ new assert helping explain why CHAIN_WORK == TARGET_BLOCKS * 2.
Introduces CHECK_RESULT for consistently validating ProcessingResult. * Verifies HeadersSyncState::State directly after ProcessNextHeaders(). * Uses BOOST_REQUIRE_EQUAL for HeadersSyncState::State - Nicer failure output and prevents continuing test in nonsensical state. * Encourages checking Locator and result.pow_validated_headers. Changes happy_path to test both full & non-full headers messages.
Adding these checks necessitates increasing the length of the generated test chains so that we can properly exceed the REDOWNLOAD_BUFFER_SIZE during the test. One can check out this commit and locally revert the TARGET_BLOCKS value change to prove the need for tests being able to control the buffer size, as is done by the next commit. Beyond the current REDOWNLOAD_BUFFER_SIZE of 15'009 we need 3 extra - 15'012 TARGET_BLOCKS: * 1 for the genesis block. * 1 for the test wanting to check that we start receiving headers for permanent storage *before* the final header (first_chain.back()). * 1 to exceed REDOWNLOAD_BUFFER_SIZE in HeadersSyncState::PopHeadersReadyForAcceptance(). (The release process includes an occasional increase of the REDOWNLOAD_BUFFER_SIZE value, see release-process.md and history of headerssync.cpp).
Move calculated constants from the top of src/headerssync.cpp into src/kernel/chainparams.cpp. Instead of being hardcoded to mainnet parameters, HeadersSyncState can now vary depending on chain or test. (This means we can reset TARGET_BLOCKS back to the nice round number of 15'000). Signet and testnets got new HeadersSyncParams constants through temporarily altering headerssync-params.py with corresponding GENESIS_TIME and MINCHAINWORK_HEADERS (based off defaultAssumeValid block height comments, corresponding to nMinimumChainWork). Regtest doesn't have a default assume valid block height, so the values are copied from Testnet 4. Since the constants only affect memory usage, and have very low impact unless dealing with a largely malicious chain, it's not that critical to keep updating them for non-mainnet chains. GENESIS_TIMEs (UTC): Testnet3: 1296688602 = datetime(2011, 2, 2) Testnet4: 1714777860 = datetime(2024, 5, 3) Signet: 1598918400 = datetime(2020, 9, 1)
794a171 to
cc5dda1
Compare
Contributor
Author
|
Latest push resolves conflict with #33332. (Also changed PR-title (category) from "headerssync:" to "p2p:" based off https://github.com/maflcko/DrahtBot/blob/14ccfd94bc7cc584e547e09582373cb87a0e4553/webhook_features/config.yml and the fact that #25717 used that topic. Maybe one could advocate for expanding the number of categories, but I'll punt on it for now. Hat-tip to l0rinc for the DrahtBot reference). |
Contributor
|
reACK cc5dda1 The only difference after the rebase is a removal of a
nit: the commits still state |
marcofleon
reviewed
Sep 16, 2025
Contributor
There was a problem hiding this comment.
code review ACK cc5dda1
I don't have a strong opinion on the refactors or commit order, although I can see how this PR might not be the most straightforward to review. Overall, the core changes in 8fd1c28 and cc5dda1 lgtm. Making the headers sync params configurable ensures that the new test can cover the redownload buffer boundary logic.
edit: I should clarify that I think the refactor of the unit test in 7b00643 is a clear improvement. Just no strong opinion on the structure/order of the commits themselves.
danielabrozzoni
left a comment
Member
There was a problem hiding this comment.
reACK cc5dda1
I checked the diff since my last view (#32579 (review) + subspan nit + #32579 (comment)), and read the whole code once again to make sure I still understand what's going on :)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
11 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Background
As part of the release process we often run contrib/devtools/headerssync-params.py and increase the values of the constants
HEADER_COMMITMENT_PERIODandREDOWNLOAD_BUFFER_SIZEin src/headerssync.cpp as per doc/release-process.md (example: 11a2d3a). This helps fine tune the memory consumption perHeadersSyncState-instance in the face of malicious peers.(The
REDOWNLOAD_BUFFER_SIZE/HEADER_COMMITMENT_PERIODratio determines how many Headers Sync commitment bits must match between PRESYNC & REDOWNLOAD phases before we start permanently storing headers from a peer. For more details see comments in src/headerssync.h and contrib/devtools/headerssync-params.py).Problem: Not feeding back headers until completing sync
During v30 release process #33274 made
REDOWNLOAD_BUFFER_SIZEexceed thetarget_blocksconstant used to control the length of chains generated for testing Headers Sync (15000, headers_sync_chainwork_tests.cpp).The
HeadersSyncState::m_redownloaded_headers-buffer now does not reach theREDOWNLOAD_BUFFER_SIZE-threshold during those unit tests. As a consequenceHeadersSyncState::PopHeadersReadyForAcceptance()will not start feeding back headers until the PoW threshold has been met. While this will not cause the unit test to start failing on master, it means we have gone from testing behavior that resembles mainnet (way more thanREDOWNLOAD_BUFFER_SIZEheaders to reach the PoW limit), to behavior that is not possible/expected there.Solution
Avoid testing this unrealistic condition of completing Headers Sync before reaching
REDOWNLOAD_BUFFER_SIZEby making tests able to define their own values through the newHeadersSyncParamsinstead of having them hard-coded for all chains & tests.Commits
REDOWNLOAD_BUFFER_SIZEthreshold.HeadersSyncStateno longer hard-coded to mainnet.Notes
This PR used to be called "headerssync: Preempt unrealistic unit test behavior".