SIP-5 draft: block hash + state root commitment consistency (Bug B fix)#2
Merged
Merged
Conversation
Discovered 2026-05-31 testnet h=5,817,132 cascade incident. After Bug A (off-trie state drift, #750) caused state divergence at h=5,817,131, the block at h=5,817,132 had internally-inconsistent block.hash vs justification.block_hash. Root cause: block_executor.rs:1561-1563 stamps state_root post-apply + recomputes block.hash. Embedded justification.block_hash stays at the pre-recompute hash. Latent — chain progression works on stored block.hash; strict_justification fork inactive by default so signature recovery never runs against the stale j.block_hash. This SIP evaluates two architectural fixes: - B1 (recommended): speculative apply at propose time. Proposer pre-applies block to compute state_root, includes in proposal, validators verify before voting. Block hash committed with real state_root from broadcast. Justification refs same hash throughout lifecycle. Cost: 2x apply CPU. - B4 (alternative): drop state_root from Block::calculate_hash. State_root becomes metadata verified separately. Cheaper but removes state_root from hash chain — safety downgrade. Recommendation: B1. Tendermint-style chains pay the 2x apply cost; safety of state_root in hash chain is worth preserving. Activation plan: fork-gated via BLOCK_HASH_WITH_REAL_STATE_ROOT_HEIGHT. Mainnet default u64::MAX. Testnet activates at SIP merge time + 50K blocks buffer. Bake 1+ week before mainnet activation height chosen. Related: #750 (Bug A), #751 (Bug B), #752 (defense check, gated).
📝 WalkthroughWalkthroughSIP-5 adds a specification to fix a consensus bug where stored blocks can have inconsistent Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sips/sip-5.md`:
- Line 179: Update the spec text for the function signature
Blockchain::speculative_apply_for_state_root(&block) -> SentrixResult<[u8; 32]>
to use "txs" instead of "tx's" so the description reads "clones the trie +
applies block txs + returns root WITHOUT mutating chain state" and ensure any
other occurrences in the same sentence or nearby lines use the same "txs"
wording for consistency.
- Around line 113-120: Specify the fork activation predicate explicitly and use
it everywhere: define that new rules apply when block.height >=
BLOCK_HASH_WITH_REAL_STATE_ROOT_HEIGHT (or choose > if preferred) and then
update all occurrences in the SIP (proposer, validator, apply sections and the
text around lines 145-153 and the paragraphs describing pre/post-fork behavior)
to reference that canonical predicate; ensure wording is exact (e.g., "When
block.height >= BLOCK_HASH_WITH_REAL_STATE_ROOT_HEIGHT, proposer must pre-apply
and validators must accept hashes with real state root") so there is a single
authoritative rule used throughout.
- Around line 82-93: Add an explicit verification that the received block's
declared hash matches a locally computed hash: after deserializing the proposed
Block in the BftMessage::Propose handler (main.rs) compute the hash via the
block's hash function (e.g. block.calculate_hash() or equivalent) and compare it
to block.hash; if block.hash is missing or differs, log a warning and
reject/return (do not prevote) before proceeding to the state_root check
(bc_read.speculative_apply_for_state_root and block.state_root handling).
- Around line 99-100: Remove the recomputation/reassignment lines that set
last.state_root and last.hash (the L1561-1563 pattern and siblings) because the
proposer-supplied block already carries state_root and hash; in add_block (and
any validator code that currently does computed_root ==
block.state_root.unwrap()) replace the unwrap usage with an explicit
validation/reject branch that returns an error if block.state_root is None or
malformed and only compares when present (i.e., check block.state_root.is_some()
then compare computed_root to the contained value and reject on mismatch), and
ensure any places referencing last.calculate_hash() do not overwrite proposer
values but instead verify consistency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
…g, canonical >= predicate
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sips/sip-5.md (1)
220-222:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInclude validator
block.hashrecomputation in the reference-implementation checklist.The normative spec now requires validators to verify both hash claim and state_root claim, but the skeleton only lists state_root verification on the validator path. Please add an explicit checklist item for recomputing
block.calculate_hash()and rejecting on mismatch so implementers don’t accidentally ship only half the validation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sips/sip-5.md` around lines 220 - 222, The checklist in the reference implementation omits validator-side hash recomputation: update the validator path notes to explicitly require recomputing block.calculate_hash() and rejecting proposals when it mismatches the proposer’s claimed block.hash in addition to verifying block.state_root; specifically mention adding this check in the BftMessage::Propose handler (the validator path) and ensure add_block_impl behavior around block.state_root and legacy recompute still documents that validators must recompute and compare block.calculate_hash() unless block.state_root was provided and post-fork rules exempt recompute.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@sips/sip-5.md`:
- Around line 220-222: The checklist in the reference implementation omits
validator-side hash recomputation: update the validator path notes to explicitly
require recomputing block.calculate_hash() and rejecting proposals when it
mismatches the proposer’s claimed block.hash in addition to verifying
block.state_root; specifically mention adding this check in the
BftMessage::Propose handler (the validator path) and ensure add_block_impl
behavior around block.state_root and legacy recompute still documents that
validators must recompute and compare block.calculate_hash() unless
block.state_root was provided and post-fork rules exempt recompute.
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
1 participant
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.
Draft SIP for Bug B fix (2026-05-31 testnet cascade). See sips/sip-5.md for full text. Approaches B1 (speculative apply, recommended) vs B4 (drop state_root from hash). Activation plan, rollback, review questions inside. Related: sentrix-labs/sentrix#751 (Bug B), #750 (Bug A), #752 (defense check).
Summary by CodeRabbit
Bug Fixes
Documentation