From c06046b6043edacb3687c732abbee8825f3a0969 Mon Sep 17 00:00:00 2001 From: Krystie Date: Tue, 7 Jul 2026 15:59:29 -0700 Subject: [PATCH] consensus: keep live PoS checks during stale-tip IBD --- notes/audit-progress.md | 30 +++++++++++++++++++++++++++ src/main.cpp | 32 ++++++++++++++++++++--------- src/main.h | 4 ++++ src/test/consensus_safety_tests.cpp | 22 ++++++++++++++++++++ 4 files changed, 78 insertions(+), 10 deletions(-) diff --git a/notes/audit-progress.md b/notes/audit-progress.md index be40869..4f79109 100644 --- a/notes/audit-progress.md +++ b/notes/audit-progress.md @@ -565,3 +565,33 @@ Verification: - Normal build/test: `ctest --output-on-failure` => 4/4 passed in build. This work intentionally does not touch production datadir `/root/.triangles/`, wallet files, consensus constants, or live daemon state. + + +## 2026-07-07 -- Krystie (stake modifier / PoS validation audit) + +### Action taken: checked stake modifier and fixed stale-tip PoS validation bypass + +Sami asked to check the stake modifier. Findings: + +1. **Stake modifier interval deviation (documented, not changed):** + - Upstream Peercoin v0.3/v0.4 uses the full `GetStakeModifierSelectionInterval()` in `GetKernelStakeModifier()`. + - Triangles has a 2014 consensus override: `nStakeModifierSelectionInterval = 2 * nModifierInterval`. + - Mainnet numbers: `nModifierInterval = 300s`; full 64-section interval = `10554s` (~2h56m); Triangles lookup delay = `600s` (~10m). + - Because minting and validation both use this, it is live consensus. Removing it without an activation gate would hard-fork historical/live behavior. Treat restoring the upstream interval as a future coordinated protocol upgrade, not a silent patch. + +2. **Critical stale-tip IBD validation bug (fixed on branch `audit/stake-modifier-review`):** + - `IsInitialBlockDownload()` also returns true when a synced node's tip is stale for >24h. + - `AcceptBlock()` used that operational IBD state to skip `CheckProofOfStake()` for any PoS block. + - `ConnectBlock()` used the same state to skip coinstake reward limit enforcement. + - Result: a stale-but-above-checkpoint node could accept live PoS blocks without kernel-target validation and without reward-limit validation. + - Fix: introduced `IsConsensusAssumeValidHeight(int nHeight)` so only the height-based historical fast path (hardcoded checkpoint / rolling assume-valid) skips PoS kernel/reward checks. Stale-tip IBD no longer disables live PoS checks. + +Cross-check: Z.Ai agreed the interval finding is correctly framed as a consensus/security weakening requiring activation, and agreed the stale-tip IBD validation bypass is a real critical bug with the height-based fix direction. + +Verification: +- Watched new regression test fail before implementation (missing helper / compile red). +- Targeted test: `./bin/test_triangles --run_test=consensus_safety_tests/pos_validation_skip_is_only_historical_fast_path --catch_system_errors=no --log_level=test_suite` => pass. +- Normal build: `ctest --output-on-failure` in `build` => 4/4 passed. +- Sanitizer build with CI flags: `ctest --output-on-failure` in `build-san-local` => 4/4 passed. + +No wallet files, production datadir, or live daemon state touched. diff --git a/src/main.cpp b/src/main.cpp index 1c9fdd6..4684307 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -1670,6 +1670,12 @@ bool IsInitialBlockDownload() return false; } +bool IsConsensusAssumeValidHeight(int nHeight) +{ + return (nHeight <= Checkpoints::GetTotalBlocksEstimate()) + || (nHeight <= nAssumeValidThreshold); +} + void static InvalidChainFound(CBlockIndex* pindexNew) { if (pindexNew->nChainTrust > nBestInvalidTrust) @@ -2192,8 +2198,7 @@ bool CBlock::ConnectBlock(CTxDBBase& txdb, CBlockIndex* pindex, bool fJustCheck) // are fully validated every time. Everything older takes the fast // path because we've already connected it successfully. A reorg that // tries to rewrite within the buffer is caught by full validation. - bool fAssumeValid = (pindex->nHeight <= Checkpoints::GetTotalBlocksEstimate()) - || (pindex->nHeight <= nAssumeValidThreshold); + bool fAssumeValid = IsConsensusAssumeValidHeight(pindex->nHeight); bool fIsInitialDownload = IsInitialBlockDownload(); //// issue here: it doesn't know the version @@ -2371,9 +2376,11 @@ bool CBlock::ConnectBlock(CTxDBBase& txdb, CBlockIndex* pindex, bool fJustCheck) int64_t nCalculatedStakeReward = GetProofOfStakeReward(nCoinAge, nFees); - // Enforce coinstake reward check only after IBD completes. - // During IBD the UTXO set is incomplete, causing nCalculatedStakeReward=0. - if (!IsInitialBlockDownload()) + // Enforce coinstake reward for every fully validated block. + // Historical checkpoint / rolling-assume-valid blocks take the + // fAssumeValid fast path above; stale-tip IBD must not disable + // live reward validation for blocks above that fast path. + if (!fAssumeValid) { if (nStakeReward > nCalculatedStakeReward) return DoS(100, error("ConnectBlock() : coinstake pays too much(actual=%" PRId64 " vs calculated=%" PRId64 ")", nStakeReward, nCalculatedStakeReward)); @@ -3345,17 +3352,22 @@ bool CBlock::AcceptBlock() uint256 hashProofOfStake = 0, targetProofOfStake = 0; if (IsProofOfStake()) { - if (IsInitialBlockDownload()) + if (IsConsensusAssumeValidHeight(nHeight)) { - // During IBD the UTXO set isn't fully loaded; CheckProofOfStake() - // would fail reading txPrev. Skip with a throttled log. + // Historical fast path: blocks at/below hardcoded checkpoint or + // rolling assume-valid have already been accepted by chain-level + // trust, so skip expensive PoS kernel verification there only. + // Do not key this off IsInitialBlockDownload(): stale-tip IBD is + // operational state, not permission to accept unchecked live PoS. if (nHeight % 10000 == 0) - printf("SKIP: PoS kernel check skipped for block %d during IBD\n", nHeight); + printf("SKIP: PoS kernel check skipped for historical fast-path block %d\n", nHeight); hashProofOfStake = 0; targetProofOfStake = 0; } else { - // Post-IBD: verify the PoS kernel signature normally. + // Verify the PoS kernel signature normally for every live block + // above the historical fast path, even if the tip is stale enough + // for IsInitialBlockDownload() to be true. if (!CheckProofOfStake(vtx[1], nBits, hashProofOfStake, targetProofOfStake)) return DoS(100, error("AcceptBlock() : check proof-of-stake failed for block %d", nHeight)); } diff --git a/src/main.h b/src/main.h index 063424f..104c6a9 100644 --- a/src/main.h +++ b/src/main.h @@ -147,6 +147,10 @@ unsigned int ComputeMinWork(unsigned int nBase, int64_t nTime); unsigned int ComputeMinStake(unsigned int nBase, int64_t nTime, unsigned int nBlockTime); int GetNumBlocksOfPeers(); [[nodiscard]] bool IsInitialBlockDownload(); +// Height-based consensus fast path for historical checkpoint / rolling +// assume-valid validation. This intentionally excludes operational IBD states +// such as a stale tip; stale-tip IBD must not disable live PoS checks. +[[nodiscard]] bool IsConsensusAssumeValidHeight(int nHeight); std::string GetWarnings(std::string strFor); bool GetTransaction(const uint256 &hash, CTransaction &tx, uint256 &hashBlock); uint256 WantedByOrphan(const CBlock* pblockOrphan); diff --git a/src/test/consensus_safety_tests.cpp b/src/test/consensus_safety_tests.cpp index 199e402..94c1bc2 100644 --- a/src/test/consensus_safety_tests.cpp +++ b/src/test/consensus_safety_tests.cpp @@ -18,6 +18,7 @@ #include "../main.h" #include "../kernel.h" #include "../script.h" +#include "../checkpoints.h" extern CBlockIndex* pindexBest; extern unsigned int nTargetSpacing; @@ -317,6 +318,27 @@ BOOST_AUTO_TEST_CASE(stake_age_soft_cap_does_not_apply_pre_v5) BOOST_CHECK_EQUAL(GetWeight(justPastMax, now), (int64_t)nStakeMaxAge); } +// ─── PoS validation fast path must be height-based (P0) ─────────────────── +// IsInitialBlockDownload() can also mean "tip is stale". That operational +// state must never disable proof-of-stake kernel/reward validation for new +// blocks above the hardened-checkpoint / rolling-assume-valid fast path. +BOOST_AUTO_TEST_CASE(pos_validation_skip_is_only_historical_fast_path) +{ + int oldAssumeValid = nAssumeValidThreshold; + nAssumeValidThreshold = 0; + + const int checkpointHeight = Checkpoints::GetTotalBlocksEstimate(); + + BOOST_CHECK(IsConsensusAssumeValidHeight(checkpointHeight)); + BOOST_CHECK(!IsConsensusAssumeValidHeight(checkpointHeight + 1)); + + nAssumeValidThreshold = checkpointHeight + 25; + BOOST_CHECK(IsConsensusAssumeValidHeight(checkpointHeight + 25)); + BOOST_CHECK(!IsConsensusAssumeValidHeight(checkpointHeight + 26)); + + nAssumeValidThreshold = oldAssumeValid; +} + // ─── Orphan block cap (P1 — DoS) ────────────────────────────────────────── // The cap on stored orphan blocks prevents an attacker from filling // memory with garbage. If too low, legitimate orphans are dropped. If