Reject blocks with zero hash in justifications_roots#216
Conversation
The spec asserts that justifications_roots must not contain zero hashes at the start of process_attestations (state.py L389). Add the equivalent check as a hard error to guard against corrupt state ingestion via checkpoint sync from malicious peers.
🤖 Kimi Code ReviewSecurity/Correctness ReviewCritical Issue - Line 227-234: The zero hash check is implemented incorrectly. The spec reference mentions "spec state.py L389" which likely refers to checking that justification bits are not set for zero hashes, not that the roots array itself contains zero hashes. This check could incorrectly reject valid states where unused justification slots contain zero hashes. Performance Issue - Line 227-234: The zero hash check uses Code Quality Issues:
Recommendations
The current implementation appears to be addressing the wrong concern at the wrong time, potentially breaking valid state transitions. Automated review by Kimi (Moonshot AI) · custom prompt |
🤖 Codex Code ReviewNo high-severity correctness or security issues found in this PR. The new guard is a sensible fail-fast invariant check for malformed state.
Notes:
Automated review by OpenAI Codex · custom prompt |
🤖 Claude Code ReviewHere is my review of PR 216. Review: Reject blocks with zero hash in
|
Greptile SummaryThis PR adds a spec-compliance guard (FINDING-004) to Key points:
Confidence Score: 4/5
|
| Filename | Overview |
|---|---|
| crates/blockchain/state_transition/src/lib.rs | Adds ZeroHashInJustificationRoots error variant and a precondition guard at the top of process_attestations. Implementation is correct and matches the leanSpec assertion at state.py L389-391, but the new code path has no dedicated unit test. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[state_transition] --> B[process_slots]
B --> C[process_block]
C --> D[process_block_header\nmodifies state]
D --> E[process_attestations]
E --> F{any root in\njustifications_roots\n== H256::ZERO?}
F -- Yes --> G[Err: ZeroHashInJustificationRoots\nPR change - defence-in-depth]
F -- No --> H[Deserialize justifications\ninto HashMap]
H --> I[Iterate attestations\nvia is_valid_vote]
I --> J{supermajority\nthreshold reached?}
J -- Yes --> K[Justify checkpoint\ntry_finalize]
J -- No --> I
K --> L[serialize_justifications\nback to state]
L --> M[Ok]
G --> N[Block rejected]
Prompt To Fix All With AI
This is a comment left during a code review.
Path: crates/blockchain/state_transition/src/lib.rs
Line: 222-232
Comment:
**New error path has no test coverage**
The PR description explicitly acknowledges that no existing spec fixture exercises this branch. Since the invariant is the main deliverable of this PR (FINDING-004), the absence of a targeted unit test means the check is not verified to fire correctly. A regression (e.g., wrong field name, off-by-one in the iterator) would be silently missed.
Consider adding a unit test similar to the pattern used in `checkpoint_sync.rs`:
```rust
#[cfg(test)]
mod tests {
use super::*;
use ethlambda_types::{
block::{AggregatedAttestations, Block, BlockBody, BlockHeader},
checkpoint::Checkpoint,
primitives::{H256, VariableList},
state::{ChainConfig, JustificationValidators, JustifiedSlots, State},
};
fn make_minimal_state_with_zero_root() -> State {
// Construct a state whose justifications_roots contains H256::ZERO
// ... (fill in required fields) ...
}
#[test]
fn process_attestations_rejects_zero_hash_in_justifications_roots() {
let mut state = make_minimal_state_with_zero_root();
let block = /* a valid next block */;
let result = state_transition(&mut state, &block);
assert!(matches!(result, Err(Error::ZeroHashInJustificationRoots)));
}
}
```
Without this, CI gives no signal that the defence-in-depth measure is actually reachable.
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: 7b004c9
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
MegaRedHand
left a comment
MegaRedHand
left a comment
There was a problem hiding this comment.
We should add a test vector for this in leanSpec
2 participants
Motivation
A spec-to-code compliance audit (FINDING-004) identified a missing invariant check. The leanSpec Python reference asserts at the start of
process_attestationsthat no justification root is the zero hash:The Rust code had no equivalent check.
Why this matters
Under normal operation, zero hashes cannot enter
justifications_rootsbecauseis_valid_voteRule 3 rejects any attestation withtarget.root == H256::ZERObefore it reaches the justifications HashMap. So the zero hash is filtered at the input boundary and never persisted byserialize_justifications.However, this assertion guards against corrupt state ingestion — specifically, when a node receives a state via checkpoint sync from a malicious peer that has a zero hash already embedded in
justifications_roots. Without this check, the node would silently operate on corrupted state. With this check, it rejects the block immediately, matching the spec's behavior.This is a defense-in-depth measure: even though the normal code path prevents zero hashes from accumulating, an explicit precondition check catches the case where state arrives from an external source with the invariant already violated.
Description
Adds a precondition check at the top of
process_attestations(before deserialization into the HashMap) that returnsError::ZeroHashInJustificationRootsif any root instate.justifications_rootsisH256::ZERO.Changes:
Errorenum: New variantZeroHashInJustificationRootsprocess_attestations: Early return with error if any justification root is zero, placed before the HashMap deserialization to match the spec's assertion orderingSpec reference
leanSpec/src/lean_spec/subspecs/containers/state/state.pyL389-391 (commitd39d101)How to Test
cargo test --workspace --release— all tests passcargo clippy --workspace -- -D warnings— cleancargo fmt --all -- --check— cleanjustifications_rootsand verifying the error would further strengthen coverage.