From 6d282db216afdd5c7b2b05537dd11e90f9a99bb0 Mon Sep 17 00:00:00 2001 From: DanS Date: Mon, 31 Aug 2026 02:12:17 -0500 Subject: [PATCH] consensus: stop BLOCK_VALID_CONTEXT overwriting the block validity level BLOCK_VALID_CONTEXT was 6. The validity levels above it are sequential VALUES packed into a 3-bit field, not independent bits, so BLOCK_VALID_MASK is 1|2|3|4|5 == 7 and the value 6 sat entirely inside it. `pindex->nStatus |= BLOCK_VALID_CONTEXT` (main.cpp:5616, and :3285) therefore did not set a flag -- it overwrote the validity level. Consequences, all long-standing: - A header-only block raised to BLOCK_VALID_TREE(2) became 2|6 == 6, which reads as >= BLOCK_VALID_CHAIN(4) and >= BLOCK_VALID_SCRIPTS(5). A block merely written to disk reported full script validity. - Every later RaiseValidity() silently no-opped, because 6 >= every level. ConnectBlock's RaiseValidity(BLOCK_VALID_SCRIPTS) was a permanent no-op. - CheckBlockIndex's "CHAIN valid implies all parents are CHAIN valid" invariant was violated whenever a stored block sat above a still-header-only ancestor, i.e. ordinary out-of-order parallel block download. fDefaultConsistencyChecks is true only for regtest, so the abort was regtest-only -- but the garbled index is written identically on mainnet, where only the detection is off. Not a v1.3.0 regression: introduced upstream in Komodo fa309e5b0 (2019-04-02), inherited via Hush, and byte-identical in v1.0.3, which the production network runs today. Validity was only ever INFLATED, never deflated, so no valid block was rejected and no invalid block skipped validation -- ConnectBlock's CheckBlock and full script/proof verification always ran. The user-visible effects were misreports: getchaintips labelling never-connected forks "valid-fork", submitblock answering "duplicate" for unvalidated blocks, and ProcessGetData serving them. The material cost was to QA: multi-node regtest tests aborted the syncing node at random, making the rpc-test suite unusable. Moves the flag to 512, the next free bit above BLOCK_IN_TMPFILE(256), and adds static_asserts that every nStatus flag is disjoint from BLOCK_VALID_MASK so this cannot be reintroduced silently. nStatus is serialized as VARINT, so the wider value needs no format change. Verified under gdb on regtest. A connected block's nStatus: before 0x1e validity field 6 (measured on the previous binary) after 0x21d validity field 5 = SCRIPTS, context bit set wallet_sapling.py, which aborted the syncing node deterministically, now runs to completion with no assert. No migration: the original level is unrecoverable from a polluted entry (1|6, 3|6 and 5|6 all give 7; 2|6 and 4|6 both give 6), and the only safe guess is downward, which would risk revalidation work on a 3.25M-block index for a cosmetic gain. Legacy entries simply have the flag bit clear, so they re-run the contextual check they previously skipped -- more checking, not less -- and resolve on any reindex. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FU87LdsJZiZkfq1eXubpeo --- src/chain.h | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/src/chain.h b/src/chain.h index 56771486d..aa039e83f 100644 --- a/src/chain.h +++ b/src/chain.h @@ -113,10 +113,9 @@ enum BlockStatus: uint32_t { //! Scripts & signatures ok. Implies all parents are also at least SCRIPTS. BLOCK_VALID_SCRIPTS = 5, - // flag to check if contextual check block has passed in Accept block, if it has not check at connect block. - BLOCK_VALID_CONTEXT = 6, - //! All validity bits. + //! NOTE: the levels above are sequential VALUES occupying this 3-bit field, not independent + //! bits, so any flag stored in nStatus must live entirely outside this mask. BLOCK_VALID_MASK = BLOCK_VALID_HEADER | BLOCK_VALID_TREE | BLOCK_VALID_TRANSACTIONS | BLOCK_VALID_CHAIN | BLOCK_VALID_SCRIPTS, @@ -129,9 +128,29 @@ enum BlockStatus: uint32_t { BLOCK_FAILED_MASK = BLOCK_FAILED_VALID | BLOCK_FAILED_CHILD, BLOCK_ACTIVATES_UPGRADE = 128, //! block activates a network upgrade - BLOCK_IN_TMPFILE = 256 + BLOCK_IN_TMPFILE = 256, + + //! ContextualCheckBlock already passed in AcceptBlock, so ConnectBlock may skip re-running it. + //! Was 6 until v1.3.0, which put it INSIDE BLOCK_VALID_MASK (1|2|3|4|5 == 7): `nStatus |= + //! BLOCK_VALID_CONTEXT` then overwrote the validity level rather than setting a flag, so a + //! block that was only written to disk read back as BLOCK_VALID_SCRIPTS and every later + //! RaiseValidity() silently no-opped. Detected by CheckBlockIndex's "CHAIN valid implies all + //! parents are CHAIN valid" assert, which aborts any node doing out-of-order block download + //! (regtest only, where fDefaultConsistencyChecks is true). Validity was only ever inflated, + //! never deflated, so ConnectBlock's full validation was never skipped -- see git history. + //! Legacy block indexes still carry the polluted low bits; they resolve on reindex, and until + //! then simply re-run the contextual check they used to skip. + BLOCK_VALID_CONTEXT = 512 }; +//! The validity level is a small integer packed into BLOCK_VALID_MASK, so every other nStatus flag +//! must be disjoint from it. Enforced here so this class of bug cannot be reintroduced silently. +static_assert((BLOCK_VALID_CONTEXT & BLOCK_VALID_MASK) == 0, "BLOCK_VALID_CONTEXT overlaps the validity-level field"); +static_assert((BLOCK_HAVE_MASK & BLOCK_VALID_MASK) == 0, "BLOCK_HAVE_MASK overlaps the validity-level field"); +static_assert((BLOCK_FAILED_MASK & BLOCK_VALID_MASK) == 0, "BLOCK_FAILED_MASK overlaps the validity-level field"); +static_assert((BLOCK_ACTIVATES_UPGRADE & BLOCK_VALID_MASK) == 0, "BLOCK_ACTIVATES_UPGRADE overlaps the validity-level field"); +static_assert((BLOCK_IN_TMPFILE & BLOCK_VALID_MASK) == 0, "BLOCK_IN_TMPFILE overlaps the validity-level field"); + //! Short-hand for the highest consensus validity we implement. //! Blocks with this validity are assumed to satisfy all consensus rules. static const BlockStatus BLOCK_VALID_CONSENSUS = BLOCK_VALID_SCRIPTS;