fix(blockchain): allow older-but-justified sources in build_block#366
Conversation
Mirrors leanSpec#716. The fixed-point attestation loop in `build_block` required `att.source == current_justified` exactly. When a sibling fork advances the store's justified checkpoint past what the canonical head's state has proven, gap-closing attestations in the head chain's pool were filtered out because their source did not match the head's `latest_justified` checkpoint, and block production aborted with "Fixed-point attestation loop did not converge". Relax the filter to a slot bound (`source.slot <= current_justified.slot`) and add the now-required sanity checks: - chain match: source.root and target.root must match the canonical chain at their slots, using a `process_block_header`-style extended view that includes parent_root and ZERO_HASH placeholders for empty slots up to the candidate slot - skip attestations whose target slot is already justified on this chain (with a genesis self-vote exception for fork-choice bootstrapping) - track justified_slots and finalized_slot through STF iterations so filters reflect the latest post-state Adds a `attestation_data_matches_chain` helper to the state_transition crate and re-exports `justified_slots_ops` so it can be reused by `build_block`. Updates the cap regression test to set up valid chain context since the new chain-match check rejects fake roots.
🤖 Kimi Code ReviewOverall Assessment Critical Issues
Safety & Correctness
Consensus Logic Review
Testing & Maintainability
Minor Observations
Summary Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Claude Code ReviewPR #366 Review:
|
🤖 Codex Code ReviewFindings
I couldn’t run Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
Greptile SummaryThis PR relaxes the attestation source filter in
Confidence Score: 4/5Safe to merge; the relaxed source filter is correctly bounded by chain-view root validation and no new cross-fork attestation vectors are opened. The core logic change (slot-comparison + attestation_data_matches_chain) is well-reasoned and the STF remains the authoritative gatekeeper. Two minor quality gaps exist: the pre-filter admits zero-root attestations at empty-slot positions that the STF will silently discard, and the genesis self-vote exception can fire after finalization has moved past slot 0, both of which waste attestation budget slots without threatening correctness or liveness in realistic conditions. attestation_data_matches_chain in state_transition/src/lib.rs — the zero-root guard is worth adding before this ships to a multi-client devnet.
|
| Filename | Overview |
|---|---|
| crates/blockchain/src/store.rs | Core build_block logic updated: source filter relaxed from exact-match to slot-comparison, chain-view matching added, finalization tracking added, test updated to use a valid head state with real historical_block_hashes. |
| crates/blockchain/state_transition/src/lib.rs | Adds public attestation_data_matches_chain helper and makes justified_slots_ops pub; both changes are minimal and correct. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[build_block called] --> B{aggregated_payloads empty?}
B -- yes --> Z[compact & emit block]
B -- no --> C[Build extended_historical_block_hashes\nhead.hbh ++ parent_root ++ ZERO×empty_slots]
C --> D[Sort entries by target.slot]
D --> E[Loop: scan sorted_entries]
E --> F{data_root already processed?}
F -- yes --> E
F -- no --> G{len ≥ MAX_ATTESTATIONS_DATA?}
G -- yes --> K[break for-loop]
G -- no --> H{head.root in known_block_roots?}
H -- no --> E
H -- yes --> I{source.slot > current_justified.slot?}
I -- yes --> E
I -- no --> J{attestation_data_matches_chain?}
J -- no --> E
J -- yes --> L{target already justified?\nexcept genesis self-vote}
L -- yes --> E
L -- no --> M[Accept: mark processed, add proofs]
M --> E
K --> N{found_new?}
N -- no --> Z
N -- yes --> O[Run STF: process_slots + process_block]
O --> P{justified or finalized advanced?}
P -- yes --> Q[Update current_justified\ncurrent_finalized_slot\ncurrent_justified_slots]
Q --> E
P -- no --> Z
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
crates/blockchain/state_transition/src/lib.rs:498-503
The STF's `is_valid_vote` explicitly rejects attestations with a zero root (rule 3: "Neither root is zero hash"), silently skipping them via `continue`. `attestation_data_matches_chain` accepts any attestation whose source or target slot maps to an empty (skipped) slot position, because those positions hold `H256::ZERO`. In practice no honest validator produces zero-root attestations, but if one slips into the pool the pre-filter will admit it, burn one of the 16 `MAX_ATTESTATIONS_DATA` budget slots, and the STF will then silently discard it — leaving a useless entry in the block body.
```suggestion
if source_slot >= historical_block_hashes.len() || target_slot >= historical_block_hashes.len()
{
return false;
}
let source_root = historical_block_hashes[source_slot];
let target_root = historical_block_hashes[target_slot];
// Mirror the STF's "neither root is zero hash" rule so the pre-filter
// agrees with is_valid_vote and never wastes an attestation budget slot
// on an entry the STF will silently discard.
if source_root == H256::ZERO || target_root == H256::ZERO {
return false;
}
source_root == data.source.root && target_root == data.target.root
```
### Issue 2 of 2
crates/blockchain/src/store.rs:1105-1114
**Genesis self-vote exception persists past finalization**
`is_genesis_self_vote` is true whenever `source.slot == 0 && target.slot == 0`, regardless of the current `current_finalized_slot`. Once finalization has advanced beyond slot 0, `is_slot_justified(slots, finalized_slot > 0, 0)` returns `true` (slot 0 is implicitly finalized), and without the guard the attestation is skipped — which is correct. But if the exception fires while `finalized_slot > 0`, it lets a genesis attestation bypass the "already justified" skip, reserving one of the 16 `MAX_ATTESTATIONS_DATA` slots for an entry that cannot advance the chain. `attestation_data_matches_chain` ensures the roots are valid so there is no safety issue; this is a minor budget-waste risk during the very first epoch of a live chain.
Reviews (1): Last reviewed commit: "fix(blockchain): allow older-but-justifi..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…atches_chain Greptile flagged that the helper accepts any attestation whose source or target slot maps to a skipped-slot position in extended_historical_block_hashes (those positions hold H256::ZERO). The STF's is_valid_vote then silently discards the vote as a zero-hash reference, but the entry has already burned one of the 16 MAX_ATTESTATIONS_DATA budget slots. Mirror is_valid_vote's "neither root is zero hash" rule in the pre-filter so process_attestations and build_block agree.
…ata_matches_chain Mirrors leanSpec#716 follow-up. Replace the duplicated zero-hash guard + checkpoint_exists pair in is_valid_vote with a single call to the chain-match helper (which now rejects zero source/target roots inline, just like the spec). Removes checkpoint_exists and the standalone zero-hash check from is_valid_vote. Reorders the helper to test source/target roots for zero before the slot-bounds check, matching the spec's structure. No semantic change: is_valid_vote rejects exactly the same attestations, just through one helper call instead of three branches.
pablodeymo
left a comment
pablodeymo
left a comment
There was a problem hiding this comment.
Thorough port of leanSpec #716 — the refactor is clean and the logic matches the spec. Four things I'd like addressed before merge.
Verified locally: cargo fmt --check clean, cargo clippy -D warnings clean, cargo test --workspace --release all green (forkchoice_spectests 84 / stf_spectests 49 / blockchain unit tests).
1. Underflow on num_empty_slots (store.rs:1063)
let num_empty_slots = (slot - parent_slot - 1) as usize;If slot <= parent_slot, this wraps to a huge u64 → huge usize and repeat_n panics on allocation before the STF gets a chance to reject with Error::ParentSlotIsNewer. In practice produce_block_with_signatures is the only caller and the slot always comes from validator-duty logic so head should be at a lower slot, but the pre-PR build_block didn't compute this upfront and we should keep the same defensive posture as process_block_header's HISTORICAL_ROOTS_LIMIT guard right above it.
Add a StoreError variant:
#[error("Proposal slot {slot} must be strictly after parent slot {parent_slot}")]
InvalidProposalSlot { slot: u64, parent_slot: u64 },and guard at the top of build_block:
let parent_slot = head_state.latest_block_header.slot;
if slot <= parent_slot {
return Err(StoreError::InvalidProposalSlot { slot, parent_slot });
}2. Pre-filter source check is looser than is_valid_vote
if att_data.source.slot > current_justified.slot { continue; }is_valid_vote requires the source slot to actually be justified (is_slot_justified(state.justified_slots, finalized, source.slot)). The pre-filter accepts any source.slot <= current_justified.slot, including slots the chain passed without justifying. Those would burn a MAX_ATTESTATIONS_DATA budget slot and be dropped by the STF.
We already have current_justified_slots in scope, so:
if !justified_slots_ops::is_slot_justified(
¤t_justified_slots,
current_finalized_slot,
att_data.source.slot,
) {
continue;
}Worth raising in leanSpec#716 too so the spec eventually tightens to match — otherwise we end up subtly stricter than the reference forever.
3. No Rust test exercises the actual fix
The modified build_block_caps_attestation_data_entries only proves the 16-entry cap. The relaxed source check, the chain-match rejection, and the genesis-self-vote bypass are all untested in this branch.
Concrete sketch — Rust analogue of the spec scenario:
/// Regression test for leanSpec PR #716: build_block must absorb
/// gap-closing attestations whose source is justified on the head
/// chain but older than `latest_justified` (e.g., a sibling fork
/// advanced the store's justified past what the canonical head has
/// proven). Without the relaxed `source.slot <= current_justified.slot`
/// filter, the exact-equality check would drop the attestation and
/// justification would never converge on this chain.
#[test]
fn build_block_absorbs_older_but_justified_source() {
use ethlambda_types::{
block::BlockHeader,
state::{ChainConfig, JustificationValidators, JustifiedSlots},
};
use libssz_types::SszList;
const NUM_VALIDATORS: usize = 50;
const SUPERMAJORITY: usize = 34; // ceil(2 * 50 / 3)
const HEAD_SLOT: u64 = 5;
const JUSTIFIED_SLOT: u64 = 1;
const GAP_TARGET_SLOT: u64 = 2;
let validators: Vec<_> = (0..NUM_VALIDATORS)
.map(|i| ethlambda_types::state::Validator {
attestation_pubkey: [i as u8; 52],
proposal_pubkey: [i as u8; 52],
index: i as u64,
})
.collect();
let hashes: Vec<H256> =
(0..HEAD_SLOT).map(|i| H256([(i + 1) as u8; 32])).collect();
let mut justified_slots = JustifiedSlots::new();
justified_slots_ops::set_justified(&mut justified_slots, 0, JUSTIFIED_SLOT);
let head_header = BlockHeader {
slot: HEAD_SLOT,
proposer_index: 0,
parent_root: H256::ZERO,
state_root: H256::ZERO,
body_root: BlockBody::default().hash_tree_root(),
};
let head_state = State {
config: ChainConfig { genesis_time: 1000 },
slot: HEAD_SLOT,
latest_block_header: head_header,
latest_justified: Checkpoint {
root: hashes[JUSTIFIED_SLOT as usize],
slot: JUSTIFIED_SLOT,
},
latest_finalized: Checkpoint::default(),
historical_block_hashes: SszList::try_from(hashes.clone()).unwrap(),
justified_slots,
validators: SszList::try_from(validators).unwrap(),
justifications_roots: Default::default(),
justifications_validators: JustificationValidators::new(),
};
let mut header_for_root = head_state.latest_block_header.clone();
header_for_root.state_root = head_state.hash_tree_root();
let parent_root = header_for_root.hash_tree_root();
let slot = HEAD_SLOT + 1;
let proposer_index = slot % NUM_VALIDATORS as u64;
// source = genesis (slot 0): older than head.latest_justified at
// slot 1. Pre-PR exact-equality filter would drop this; post-PR
// it's absorbed and the candidate justifies GAP_TARGET_SLOT.
let att_data = AttestationData {
slot,
head: Checkpoint { root: hashes[0], slot: 0 },
target: Checkpoint {
root: hashes[GAP_TARGET_SLOT as usize],
slot: GAP_TARGET_SLOT,
},
source: Checkpoint { root: hashes[0], slot: 0 },
};
let data_root = att_data.hash_tree_root();
let mut bits = AggregationBits::with_length(NUM_VALIDATORS).unwrap();
for i in 0..SUPERMAJORITY {
bits.set(i, true).unwrap();
}
let proof =
AggregatedSignatureProof::new(bits, SszList::try_from(vec![0xAB; 64]).unwrap());
let mut aggregated_payloads = HashMap::new();
aggregated_payloads.insert(data_root, (att_data.clone(), vec![proof]));
let mut known_block_roots = HashSet::new();
known_block_roots.insert(parent_root);
known_block_roots.insert(hashes[0]);
let (block, _signatures, post_checkpoints) = build_block(
&head_state,
slot,
proposer_index,
parent_root,
&known_block_roots,
&aggregated_payloads,
)
.expect("build_block should succeed");
let targets: Vec<_> = block
.body
.attestations
.iter()
.map(|att| att.data.target)
.collect();
assert!(
targets.contains(&att_data.target),
"produced block missing gap-closing attestation: {targets:?}"
);
assert_eq!(post_checkpoints.justified.slot, GAP_TARGET_SLOT);
assert_eq!(
post_checkpoints.justified.root,
hashes[GAP_TARGET_SLOT as usize]
);
}Bonus: please port the spec's renamed test_build_block_skips_other_chain_source (from the test_state_aggregation.py diff in leanSpec#716) so we have direct coverage of the chain-match rejection too — feeds two payloads with the same source.slot but one whose source.root doesn't match the chain, asserts only the matching one is included.
4. Nits (optional, can be follow-ups)
head_state.historical_block_hashes.iter().copied().collect()allocates a full copy. The STF avoids it viastd::mem::take. Perf only — not relevant at current chain length.pub mod justified_slots_opswidens the module's surface area more than needed. Onlyis_slot_justifiedis used externally. Eitherpub(crate) mod+pub usethe single fn, or leave it if you expect more callers soon.
…t underflow Two fixes from PR review: 1. `slot.saturating_sub(parent_slot).saturating_sub(1)` instead of raw subtraction. If `slot <= parent_slot` (caller bug), the prior code wrapped to a huge u64 and `repeat_n` panicked on allocation before the STF could reject with `Error::ParentSlotIsNewer`. Now we degrade to 0 empty slots and let the STF error normally. 2. Replace the source-slot bound (`source.slot <= current_justified.slot`) with `is_slot_justified(source.slot)`. Justification has gaps: between finalized and latest_justified, some slots are justified and some aren't. The slot bound admitted unjustified-but-≤-current_justified sources that the STF then dropped via is_valid_vote, wasting a MAX_ATTESTATIONS_DATA budget slot per such entry. The tighter check matches the STF predicate exactly. Worth raising upstream on leanSpec#716 — the spec PR has the same gap.
Mirrors the scenario from leanSpec#716 in Rust: head state has latest_justified at slot 1, an attestation with source=genesis (older but justified) and target=slot 2 sits in the gossip pool. The pre-PR exact-equality filter dropped it; with the relaxed is_slot_justified(source.slot) check the attestation is absorbed and the candidate block's post-state justifies slot 2. Asserts both the body contains the gap-closing attestation and the post-state checkpoints advanced past head.latest_justified.
…ild_block attestation_data_matches_chain now takes an Option<H256> parent_root that represents one virtual entry at index historical.len(). Lets build_block pass head_state.historical_block_hashes directly (no clone of up to HISTORICAL_ROOTS_LIMIT entries) plus parent_root to cover the slot the candidate block will materialize. process_attestations passes None: by then the post-state already contains the parent's entry. Slots between parent and candidate are still rejected — they'd resolve to ZERO_HASH in the conceptual extended view, but the zero-hash guard at the top of the helper drops those attestations first.
…on in build_block" This reverts commit 1474f55.
2 participants
Summary
Mirrors leanSpec#716. The fixed-point attestation loop in
build_blockrequiredatt.sourceto equalcurrent_justifiedexactly. When the store's justified checkpoint has advanced via a sibling fork while the canonical head's chain has only proven an earlier slot, the head chain's pool holds a gap-closing attestation whose source differs from the head'slatest_justifiedcheckpoint. The exact-match filter dropped it and block production aborted withJustifiedDivergenceNotClosed("Fixed-point attestation loop did not converge").Scenario (from the spec PR)
block_4: 6/8 attesttarget=block_1(justifies slot 1).block_5: 2/8 attesttarget=block_4(fork-choice weight only).block_6(sibling of block_4): 6/8 attesttarget=block_2(justifies slot 2 on the store).Fork choice picks
block_5as head; the store'slatest_justifiedisblock_2, butblock_5's post-state still sits atblock_1. The proposer at slot 7 builds onblock_5and must absorbblock_6's attestation from the gossip pool to close the gap.Changes
In
build_block(crates/blockchain/src/store.rs):att.source != current_justifiedtoatt.source.slot > current_justified.slot, so older-but-already-justified sources can flow through.process_block_headerwould produce on the candidate block (the head'shistorical_block_hashesplusparent_rootat the parent slot plusH256::ZEROfor each empty slot between parent and the candidate) and reject attestations whosesource.rootortarget.rootdo not match it. Prevents pulling in attestations about other forks now that the exact source check is gone.source.slot == 0 && target.slot == 0) exception for fork-choice bootstrapping.current_finalized_slotandcurrent_justified_slotsalongsidecurrent_justified, refreshing them frompost_statewhenever the STF iteration advances either checkpoint.In
state_transition:attestation_data_matches_chain(&[H256], &AttestationData) -> boolhelper.justified_slots_opsmodulepubsois_slot_justifiedcan be reused frombuild_blockwithout duplicating the relative-index logic.The existing process_attestations path already factored the source/target chain match into
checkpoint_existsand is left unchanged.Test update
build_block_caps_attestation_data_entriespreviously used fake target roots that don't appear in the chain. Restructured the setup to build a head state at slot 51 with populatedhistorical_block_hashesand varyAttestationData.slot(rather than the target root) to produce distinct entries. Still verifies theMAX_ATTESTATIONS_DATAcap and gossip-size bound.Test plan
cargo fmt --allcargo clippy --workspace --all-targets -- -D warningscargo test --workspace --release(includingforkchoice_spectests,stf_spectests,signature_spectestsagainst the pinned leanSpec fixtures)