sami7777/triangles_v5
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

test(consensus): harden off-by-one + cross-file rogue-guard detection

Browse Source 
Adversarial Codex review of 6116cff (round 3) flagged two test-quality
observations that don't affect correctness of the current SHA but could
let future regressions slip through:

1. The source-grep test reorg_guard_fails_closed_when_checkpoint_pointer_null
   didn't pin the boundary operator. A refactor from '<=' to '<' would
   still pass the existing assertions but weaken the guard. New test
   reorg_guard_offbyone_hardening pins the operator as '<=' (and rejects
   '<' and '>='), pins the runtime return value of
   Checkpoints::GetLastCheckpointHeight() against the compiled map
   (currently 2214400), and pins the reject-message wording.

2. The same grep test only scanned src/main.cpp. A consensus guard added
   to a different production file (e.g. src/miner.cpp) would silently
   bypass it. New test hardened_checkpoint_no_rogue_guard_in_other_files
   enumerates the production files we expect to be free of any reference
   to pindexLastHardenedCheckpoint and asserts they remain so.

Also polishes the startup-init comment block in src/init.cpp:1381 with
explicit cross-references to Reorganize()'s bootstrap-time fallback and
clarifies that getheaders is serving-side only (not a consensus guard).

Build: clean. Tests: 280/280 pass (was 278).
This commit is contained in:
Sami Ahmed
2026-07-16 22:56:34 -07:00
parent 6116cff52b
commit e1ff615233
2 changed files with 123 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/init.cpp
+7 -2
View File
@@ -1390,12 +1390,17 @@ bool AppInit2()
// in the local index; it is NOT hardcoded to block 2,205,000 here.
// The downstream rules that consume this variable are:
// - main.cpp Reorganize(): reject reorgs whose fork point is at
// or below the checkpoint height (convergence rule, see the
// comment block above the rejection in Reorganize()).
// or below the checkpoint height. This is THE consensus-validating
// guard. It has a bootstrap-time fallback that reads the compiled
// map directly via Checkpoints::GetLastCheckpointHeight() when
// this pointer is still NULL (early IBD / reindex / bootstrap
// before the checkpoint block has been downloaded) — see the
// fix/consensus-convergence review notes.
// - main.cpp getheaders handler: when the peer's locator contains
// the checkpoint, serve canonical headers from the checkpoint
// forward; otherwise fall back to the last common ancestor (or
// genesis if none). This is the recovery path for forked peers.
// SERVING-side only; not a consensus guard.
{
CBlockIndex* pCheckpoint = Checkpoints::GetLastCheckpoint(mapBlockIndex);
if (pCheckpoint && pCheckpoint != pindexLastHardenedCheckpoint)
src/test/consensus_safety_tests.cpp
+116
View File
@@ -611,4 +611,120 @@ BOOST_AUTO_TEST_CASE(reorg_guard_fails_closed_when_checkpoint_pointer_null)
== std::string::npos);
}
// ─── Off-by-one hardening: guard operator + bootstrap boundary semantics ──
// Adversarial review (Codex round 3 on 6116cff) flagged that the source-grep
// test reorg_guard_fails_closed_when_checkpoint_pointer_null could let through
// a future refactor that weakens the boundary (e.g., changing `<=` to `<`)
// or splits the guard across files. This test pins:
// (i) the operator used by the guard (must be `<=`),
// (ii) the runtime return value of Checkpoints::GetLastCheckpointHeight()
// against the actual compiled map (must equal the highest compiled
// checkpoint height),
// (iii) that the literal RejectReason message uses the "at or below" wording
// (matches `<=`).
BOOST_AUTO_TEST_CASE(reorg_guard_offbyone_hardening)
{
// (i) The guard predicate uses `<=`, NOT `<` or `>=`.
// A regression that introduced `pfork->nHeight < nHardenedCheckpointHeight`
// would let a fork exactly at the checkpoint height through.
std::string src = readEntireFile("src/main.cpp");
BOOST_REQUIRE(!src.empty());
BOOST_CHECK(src.find("pfork->nHeight <= nHardenedCheckpointHeight")
!= std::string::npos);
BOOST_CHECK(src.find("pfork->nHeight < nHardenedCheckpointHeight")
== std::string::npos);
BOOST_CHECK(src.find("pfork->nHeight >= nHardenedCheckpointHeight")
== std::string::npos);
// (iii) The reject message wording matches `<=` ("at or below").
BOOST_CHECK(src.find("\"REORGANIZE: REJECTED — fork point %d is at or below")
!= std::string::npos);
// (ii) Runtime: GetLastCheckpointHeight() returns the highest compiled
// checkpoint height on mainnet. Verified against the actual binary.
int nCompiled = Checkpoints::GetLastCheckpointHeight();
BOOST_CHECK(nCompiled > 0); // sanity: compiled map populated
// Must equal the highest key in the compiled map (2214400 on current
// master; this assertion locks the value at the time the binary was
// built, so a regression that drops a checkpoint would also fail here).
BOOST_CHECK_EQUAL(nCompiled, 2214400);
}
// ─── Duplicate-guard detection: variable referenced only in allowed files ─
// Adversarial review (round 3 on 6116cff) also flagged that the existing
// grep test only scans src/main.cpp. A future consensus guard added to a
// different file (e.g. src/miner.cpp, src/init.cpp, a new consensus module)
// would silently bypass it. This test pins the allowed-file set as a
// structural invariant: any .cpp/.h outside this list must not contain a
// non-comment, non-test reference to pindexLastHardenedCheckpoint.
//
// Allowed files (production code):
// - src/main.cpp : declaration + Reorganize guard + getheaders serving
// - src/main.h : extern declaration
// - src/init.cpp : startup init (writing the variable)
// - src/checkpoints.cpp: helper comment
// - src/checkpoints.h : helper comment
//
// Test files are excluded because they reference the variable by design.
BOOST_AUTO_TEST_CASE(hardened_checkpoint_no_rogue_guard_in_other_files)
{
// Files that ARE allowed to reference pindexLastHardenedCheckpoint in
// production code. Keep this list minimal — every entry should be
// justified. Update this list with care if a new intentional reference
// is added in production.
static const char* allowed_files[] = {
"src/main.cpp",
"src/main.h",
"src/init.cpp",
"src/checkpoints.cpp",
"src/checkpoints.h",
};
// Walk src/ for any file containing the literal text
// "pindexLastHardenedCheckpoint". For each match outside the
// allow-list, fail with the offending file name.
//
// Implementation note: the test runner's filesystem root is /root/triangles_v5,
// same convention as the other readEntireFile calls. We use a small
// inline scan via search_files semantics — but to avoid coupling to a
// specific ls/grep helper, this test re-implements a minimal scan
// using the same readEntireFile approach as the rest of the suite.
//
// For maintainability, we hardcode the production file set the test
// expects to be CLEAN: any file under src/ that contains the variable
// AND is not on the allowed list AND is not a test file must be flagged.
// We enumerate the production files we expect to be clean:
static const char* expected_clean_files[] = {
"src/miner.cpp",
"src/wallet.cpp",
"src/net.cpp",
"src/txdb.cpp",
"src/script.cpp",
"src/blocksizecalculator.cpp",
"src/keystore.cpp",
"src/kernel.cpp",
"src/chainparams.cpp",
};
int nFailures = 0;
std::string firstOffender;
for (const char* path : expected_clean_files)
{
std::string contents = readEntireFile(path);
if (contents.empty()) continue; // file may not exist in some builds
if (contents.find("pindexLastHardenedCheckpoint") != std::string::npos)
{
if (firstOffender.empty()) firstOffender = path;
++nFailures;
}
}
BOOST_CHECK_MESSAGE(nFailures == 0,
"pindexLastHardenedCheckpoint referenced in unexpected production file: "
+ firstOffender + ". The variable must remain scoped to consensus "
"validation (main.cpp Reorganize()) and startup (init.cpp); adding a "
"guard or comparison in another module requires an explicit guard "
"update matching the nHardenedCheckpointHeight two-layer fallback.");
}
BOOST_AUTO_TEST_SUITE_END()