From 34eb367ec13ac1e02bf6ab9e4ee46d85a6d764fc Mon Sep 17 00:00:00 2001 From: Nojus Sandovas Date: Tue, 27 Jan 2026 00:49:39 +0200 Subject: [PATCH 1/3] Fixed fork-choice tests --- lean_client/Cargo.lock | 1 + lean_client/containers/src/block.rs | 9 ++ lean_client/containers/src/public_key.rs | 42 +++--- lean_client/containers/src/state.rs | 131 +++++++++++------- lean_client/fork_choice/Cargo.toml | 1 + lean_client/fork_choice/src/handlers.rs | 53 ++++++- lean_client/fork_choice/src/store.rs | 11 +- .../tests/fork_choice_test_vectors.rs | 18 ++- .../tests/unit_tests/fork_choice.rs | 2 +- lean_client/networking/src/discovery/mod.rs | 10 +- lean_client/src/main.rs | 4 +- lean_client/validator/src/lib.rs | 48 ++++--- 12 files changed, 234 insertions(+), 96 deletions(-) diff --git a/lean_client/Cargo.lock b/lean_client/Cargo.lock index d5c4dd4..a8df01a 100644 --- a/lean_client/Cargo.lock +++ b/lean_client/Cargo.lock @@ -1642,6 +1642,7 @@ dependencies = [ "serde_json", "ssz", "test-generator", + "tracing", ] [[package]] diff --git a/lean_client/containers/src/block.rs b/lean_client/containers/src/block.rs index d9de1fb..3052737 100644 --- a/lean_client/containers/src/block.rs +++ b/lean_client/containers/src/block.rs @@ -2,6 +2,7 @@ use crate::{ Attestation, Bytes32, MultisigAggregatedSignature, Signature, Slot, State, ValidatorIndex, }; use serde::{Deserialize, Serialize}; +use ssz::SszHash; use ssz_derive::Ssz; use crate::attestation::{AggregatedAttestations, AttestationSignatures}; @@ -80,6 +81,14 @@ pub fn hash_tree_root(value: &T) -> Bytes32 { Bytes32(h) } +/// Compute the canonical block root for a Block. +/// +/// This uses the Block's own hash_tree_root, which must be consistent +/// with how latest_block_header is stored and hashed in state transitions. +pub fn compute_block_root(block: &Block) -> Bytes32 { + Bytes32(block.hash_tree_root()) +} + impl SignedBlockWithAttestation { /// Verify all XMSS signatures in this signed block. /// diff --git a/lean_client/containers/src/public_key.rs b/lean_client/containers/src/public_key.rs index 114b17c..529b00c 100644 --- a/lean_client/containers/src/public_key.rs +++ b/lean_client/containers/src/public_key.rs @@ -41,27 +41,20 @@ impl SszSize for PublicKey { // 2. Define how to write (Serialize) impl SszWrite for PublicKey { - fn write_fixed(&self, _bytes: &mut [u8]) { - panic!("SszWrite::write_fixed must be implemented for fixed-size types"); + fn write_fixed(&self, bytes: &mut [u8]) { + // Write the 52 bytes of the public key + bytes[..PUBLIC_KEY_SIZE].copy_from_slice(&self.inner); } fn write_variable(&self, _bytes: &mut Vec) -> Result<(), WriteError> { - panic!("SszWrite::write_variable must be implemented for variable-size types"); + // PublicKey is fixed-size, this should not be called + panic!("PublicKey is fixed-size, write_variable should not be called"); } fn to_ssz(&self) -> Result, WriteError> { - match Self::SIZE { - Size::Fixed { size } => { - let mut bytes = vec![0; size]; - self.write_fixed(bytes.as_mut_slice()); - Ok(bytes) - } - Size::Variable { minimum_size } => { - let mut bytes = Vec::with_capacity(minimum_size); - self.write_variable(&mut bytes)?; - Ok(bytes) - } - } + let mut bytes = vec![0u8; PUBLIC_KEY_SIZE]; + self.write_fixed(&mut bytes); + Ok(bytes) } } @@ -100,11 +93,26 @@ impl SszHash for PublicKey { type PackingFactor = typenum::U1; fn hash_tree_root(&self) -> H256 { - // Simple implementation: hash the inner bytes directly + // SSZ hash_tree_root for fixed-size types > 32 bytes: + // 1. Split into 32-byte chunks + // 2. Pad last chunk with zeros if needed + // 3. Merkleize the chunks use sha2::{Digest, Sha256}; + + // For 52 bytes: 2 chunks (32 + 20 bytes, second chunk padded to 32) + let mut chunk1 = [0u8; 32]; + let mut chunk2 = [0u8; 32]; + + chunk1.copy_from_slice(&self.inner[0..32]); + chunk2[..20].copy_from_slice(&self.inner[32..52]); + // Remaining 12 bytes of chunk2 are already zeros (padding) + + // Merkleize: hash(chunk1 || chunk2) let mut hasher = Sha256::new(); - hasher.update(&self.inner); + hasher.update(&chunk1); + hasher.update(&chunk2); let result = hasher.finalize(); + H256::from_slice(&result) } } diff --git a/lean_client/containers/src/state.rs b/lean_client/containers/src/state.rs index cf8db72..e61e0c2 100644 --- a/lean_client/containers/src/state.rs +++ b/lean_client/containers/src/state.rs @@ -320,6 +320,16 @@ impl State { let latest_header_for_hash = self.latest_block_header.clone(); let parent_root = hash_tree_root(&latest_header_for_hash); if block.parent_root != parent_root { + tracing::error!( + expected_parent_root = %format!("0x{:x}", parent_root.0), + block_parent_root = %format!("0x{:x}", block.parent_root.0), + header_slot = self.latest_block_header.slot.0, + header_proposer = self.latest_block_header.proposer_index.0, + header_parent = %format!("0x{:x}", self.latest_block_header.parent_root.0), + header_state_root = %format!("0x{:x}", self.latest_block_header.state_root.0), + header_body_root = %format!("0x{:x}", self.latest_block_header.body_root.0), + "Block parent root mismatch - debug info" + ); return Err(String::from("Block parent root mismatch")); } @@ -332,33 +342,49 @@ impl State { .push(parent_root) .expect("within limit"); - // Calculate total number of slots to track + // Calculate number of empty slots (skipped slots between parent and this block) let num_empty_slots = (block.slot.0 - self.latest_block_header.slot.0 - 1) as usize; - let new_len = self.justified_slots.len() + 1 + num_empty_slots; - - // Build new BitList with extended length - let mut new_justified_slots = JustifiedSlots::new(false, new_len); - for i in 0..self.justified_slots.len() { - if let Some(bit) = self.justified_slots.get(i) { - if *bit { - new_justified_slots.set(i, true); - } - } - } - // Set the bit for the latest block header - new_justified_slots.set( - self.justified_slots.len(), - self.latest_block_header.slot == Slot(0), - ); - // Empty slots remain false (already initialized) - // Add empty slots to historical hashes + // Add ZERO_HASH entries for empty slots to historical hashes for _ in 0..num_empty_slots { new_historical_hashes .push(Bytes32(ssz::H256::zero())) .expect("within limit"); } + // Extend justified_slots to cover slots from finalized_slot+1 to last_materialized_slot + // per leanSpec: justified_slots is stored RELATIVE to the finalized boundary + // The first entry corresponds to slot (finalized_slot + 1) + let last_materialized_slot = block.slot.0.saturating_sub(1); + let finalized_slot = self.latest_finalized.slot.0; + + let new_justified_slots = if last_materialized_slot > finalized_slot { + // Calculate relative index: slot X maps to index (X - finalized_slot - 1) + let relative_index = (last_materialized_slot - finalized_slot - 1) as usize; + let required_capacity = relative_index + 1; + let current_len = self.justified_slots.len(); + + if required_capacity > current_len { + // Extend the bitlist + let mut new_slots = JustifiedSlots::new(false, required_capacity); + // Copy existing bits + for i in 0..current_len { + if let Some(bit) = self.justified_slots.get(i) { + if *bit { + new_slots.set(i, true); + } + } + } + // New slots are initialized to false (unjustified) + new_slots + } else { + self.justified_slots.clone() + } + } else { + // last_materialized_slot <= finalized_slot: no extension needed + self.justified_slots.clone() + }; + let body_for_hash = block.body.clone(); let body_root = hash_tree_root(&body_for_hash); @@ -434,6 +460,9 @@ impl State { } /// Process a single attestation's votes. + /// + /// NOTE: justified_slots uses RELATIVE indexing. Slot X maps to index (X - finalized_slot - 1). + /// Slots at or before finalized_slot are implicitly justified (not stored in the bitlist). fn process_single_attestation( &self, vote: &crate::attestation::AttestationData, @@ -449,33 +478,31 @@ impl State { let target_root = vote.target.root; let source_root = vote.source.root; - let target_slot_int = target_slot.0 as usize; - let source_slot_int = source_slot.0 as usize; + let finalized_slot_int = initial_finalized_slot.0 as i64; - let source_is_justified = justified_slots_working - .get(source_slot_int) - .copied() - .unwrap_or(false); - let target_already_justified = justified_slots_working - .get(target_slot_int) - .copied() - .unwrap_or(false); + // Helper to check if a slot is justified using RELATIVE indexing + // Per leanSpec: slots at or before finalized_slot are implicitly justified + let is_slot_justified = |slot: Slot, justified_slots: &[bool]| -> bool { + if slot.0 as i64 <= finalized_slot_int { + // Slots at or before finalized boundary are implicitly justified + return true; + } + // Calculate relative index: slot X maps to index (X - finalized_slot - 1) + let relative_index = (slot.0 as i64 - finalized_slot_int - 1) as usize; + justified_slots.get(relative_index).copied().unwrap_or(false) + }; + + let source_is_justified = is_slot_justified(source_slot, justified_slots_working); + let target_already_justified = is_slot_justified(target_slot, justified_slots_working); + + let source_slot_int = source_slot.0 as usize; + let target_slot_int = target_slot.0 as usize; - // Special case for slot 0 (genesis): historical_block_hashes[0] is initialized as 0x0 - // in genesis, but validators attest with the actual genesis block hash (set in - // get_forkchoice_store). Allow any source_root when source is slot 0 and - // historical_block_hashes[0] is the zero hash. + // Check root matches using absolute slot for historical_block_hashes lookup let source_root_matches = self .historical_block_hashes .get(source_slot_int as u64) - .map(|r| { - if source_slot_int == 0 && r.0.is_zero() { - // Genesis slot: accept any root when historical hash is 0x0 - true - } else { - *r == source_root - } - }) + .map(|r| *r == source_root) .unwrap_or(false); let target_root_matches = self .historical_block_hashes @@ -483,9 +510,15 @@ impl State { .map(|r| *r == target_root) .unwrap_or(false); + // Ignore votes that reference zero-hash slots (per leanSpec) + if source_root.0.is_zero() || target_root.0.is_zero() { + return; + } + let is_valid_vote = source_is_justified && !target_already_justified - && (source_root_matches || target_root_matches) + && source_root_matches + && target_root_matches && target_slot > source_slot && target_slot.is_justifiable_after(initial_finalized_slot); @@ -554,11 +587,15 @@ impl State { ); *latest_justified = vote.target.clone(); - justified_slots_working.extend(std::iter::repeat_n( - false, - (target_slot_int + 1).saturating_sub(justified_slots_working.len()), - )); - justified_slots_working[target_slot_int] = true; + // Use RELATIVE indexing for justified_slots_working + // Calculate relative index for target slot + let target_relative_index = (target_slot.0 as i64 - finalized_slot_int - 1) as usize; + + // Extend the working vec if needed + if target_relative_index >= justified_slots_working.len() { + justified_slots_working.resize(target_relative_index + 1, false); + } + justified_slots_working[target_relative_index] = true; justifications.remove(&target_root); diff --git a/lean_client/fork_choice/Cargo.toml b/lean_client/fork_choice/Cargo.toml index c50bd99..f503590 100644 --- a/lean_client/fork_choice/Cargo.toml +++ b/lean_client/fork_choice/Cargo.toml @@ -10,6 +10,7 @@ default = [] env-config = { path = "../env-config", default-features = false } containers = { path = "../containers", default-features = false } ssz = { workspace = true } +tracing = "0.1" [dev-dependencies] serde_json = "1.0" diff --git a/lean_client/fork_choice/src/handlers.rs b/lean_client/fork_choice/src/handlers.rs index 5888942..353dca2 100644 --- a/lean_client/fork_choice/src/handlers.rs +++ b/lean_client/fork_choice/src/handlers.rs @@ -5,6 +5,7 @@ use containers::{ attestation::SignedAttestation, block::SignedBlockWithAttestation, Bytes32, ValidatorIndex, }; use ssz::SszHash; +use tracing::{debug, info}; #[inline] pub fn on_tick(store: &mut Store, time: u64, has_proposal: bool) { @@ -195,7 +196,7 @@ fn on_attestation_internal( /// 4. Updating the forkchoice head /// 5. Processing the proposer's attestation (as if gossiped) pub fn on_block(store: &mut Store, signed_block: SignedBlockWithAttestation) -> Result<(), String> { - let block_root = Bytes32(signed_block.message.block.hash_tree_root()); + let block_root = containers::block::compute_block_root(&signed_block.message.block); if store.blocks.contains_key(&block_root) { return Ok(()); @@ -228,6 +229,7 @@ fn process_block_internal( block_root: Bytes32, ) -> Result<(), String> { let block = signed_block.message.block.clone(); + let attestations_count = block.body.attestations.len_u64(); // Get parent state for validation let state = match store.states.get(&block.parent_root) { @@ -239,20 +241,63 @@ fn process_block_internal( } }; + // Debug: Log parent state checkpoints before transition + tracing::debug!( + block_slot = block.slot.0, + attestations_in_block = attestations_count, + parent_justified_slot = state.latest_justified.slot.0, + parent_finalized_slot = state.latest_finalized.slot.0, + justified_slots_len = state.justified_slots.len(), + "Processing block - parent state info" + ); + // Execute state transition to get post-state let new_state = state.state_transition_with_validation(signed_block.clone(), true, true)?; + // Debug: Log new state checkpoints after transition + tracing::debug!( + block_slot = block.slot.0, + new_justified_slot = new_state.latest_justified.slot.0, + new_finalized_slot = new_state.latest_finalized.slot.0, + new_justified_slots_len = new_state.justified_slots.len(), + "Block processed - new state info" + ); + // Store block and state, store the plain Block (not SignedBlockWithAttestation) store.blocks.insert(block_root, block.clone()); store.states.insert(block_root, new_state.clone()); - if new_state.latest_justified.slot > store.latest_justified.slot { + let justified_updated = new_state.latest_justified.slot > store.latest_justified.slot; + let finalized_updated = new_state.latest_finalized.slot > store.latest_finalized.slot; + + if justified_updated { + tracing::info!( + old_justified = store.latest_justified.slot.0, + new_justified = new_state.latest_justified.slot.0, + "Store justified checkpoint updated!" + ); store.latest_justified = new_state.latest_justified.clone(); } - if new_state.latest_finalized.slot > store.latest_finalized.slot { + if finalized_updated { + tracing::info!( + old_finalized = store.latest_finalized.slot.0, + new_finalized = new_state.latest_finalized.slot.0, + "Store finalized checkpoint updated!" + ); store.latest_finalized = new_state.latest_finalized.clone(); } + if !justified_updated && !finalized_updated { + tracing::debug!( + block_slot = block.slot.0, + store_justified = store.latest_justified.slot.0, + store_finalized = store.latest_finalized.slot.0, + state_justified = new_state.latest_justified.slot.0, + state_finalized = new_state.latest_finalized.slot.0, + "No checkpoint updates from this block" + ); + } + // Process block body attestations as on-chain (is_from_block=true) let signatures = &signed_block.signature; let aggregated_attestations = &block.body.attestations; @@ -331,7 +376,7 @@ fn process_pending_blocks(store: &mut Store, mut roots: Vec) { while let Some(parent_root) = roots.pop() { if let Some(purgatory) = store.blocks_queue.remove(&parent_root) { for block in purgatory { - let block_origins = Bytes32(block.message.block.hash_tree_root()); + let block_origins = containers::block::compute_block_root(&block.message.block); if let Ok(()) = process_block_internal(store, block, block_origins) { roots.push(block_origins); } diff --git a/lean_client/fork_choice/src/store.rs b/lean_client/fork_choice/src/store.rs index 07100ba..5eb27d3 100644 --- a/lean_client/fork_choice/src/store.rs +++ b/lean_client/fork_choice/src/store.rs @@ -54,8 +54,10 @@ pub fn get_forkchoice_store( ) -> Store { // Extract the plain Block from the signed block let block = anchor_block.message.block.clone(); - let block_root = Bytes32(block.hash_tree_root()); let block_slot = block.slot; + + // Compute block root using the header hash (canonical block root) + let block_root = containers::block::compute_block_root(&block); let latest_justified = if anchor_state.latest_justified.root.0.is_zero() { Checkpoint { @@ -75,6 +77,9 @@ pub fn get_forkchoice_store( anchor_state.latest_finalized.clone() }; + // Store the original anchor_state - do NOT modify it + // Modifying checkpoints would change its hash_tree_root(), breaking the + // consistency with block.state_root Store { time: block_slot.0 * INTERVALS_PER_SLOT, config, @@ -338,8 +343,8 @@ pub fn produce_block_with_signatures( Some(&store.aggregated_payloads), )?; - // Compute block root - let block_root = Bytes32(final_block.hash_tree_root()); + // Compute block root using the header hash (canonical block root) + let block_root = containers::block::compute_block_root(&final_block); // Store block and state (per devnet-2, we store the plain Block) store.blocks.insert(block_root, final_block.clone()); diff --git a/lean_client/fork_choice/tests/fork_choice_test_vectors.rs b/lean_client/fork_choice/tests/fork_choice_test_vectors.rs index 689d20b..802742a 100644 --- a/lean_client/fork_choice/tests/fork_choice_test_vectors.rs +++ b/lean_client/fork_choice/tests/fork_choice_test_vectors.rs @@ -254,7 +254,7 @@ impl Into for TestBlock { slot: Slot(self.slot), proposer_index: ValidatorIndex(self.proposer_index), parent_root: parse_root(&self.parent_root), - state_root: parse_root(&self.parent_root), + state_root: parse_root(&self.state_root), body: self.body.into(), } } @@ -323,7 +323,7 @@ struct TestAggregatedAttestation { impl Into for TestAggregatedAttestation { fn into(self) -> AggregatedAttestation { AggregatedAttestation { - aggregation_bits: self.aggregation_bits.data, + aggregation_bits: self.aggregation_bits.into(), data: self.data.into(), } } @@ -331,7 +331,17 @@ impl Into for TestAggregatedAttestation { #[derive(Debug, Deserialize)] struct TestAggregationBits { - data: AggregationBits, + data: Vec, +} + +impl Into for TestAggregationBits { + fn into(self) -> AggregationBits { + let mut bitlist = ssz::BitList::with_length(self.data.len()); + for (i, &bit) in self.data.iter().enumerate() { + bitlist.set(i, bit); + } + AggregationBits(bitlist) + } } #[derive(Debug, Deserialize)] @@ -552,7 +562,7 @@ fn forkchoice(spec_file: &str) { message: block, signature: BlockSignatures::default(), }; - let block_root = Bytes32(signed_block.message.block.hash_tree_root()); + let block_root = containers::block::compute_block_root(&signed_block.message.block); // Advance time to the block's slot to ensure attestations are processable // SECONDS_PER_SLOT is 4 (not 12) diff --git a/lean_client/fork_choice/tests/unit_tests/fork_choice.rs b/lean_client/fork_choice/tests/unit_tests/fork_choice.rs index 684a799..068f716 100644 --- a/lean_client/fork_choice/tests/unit_tests/fork_choice.rs +++ b/lean_client/fork_choice/tests/unit_tests/fork_choice.rs @@ -42,7 +42,7 @@ fn test_get_vote_target_chain() { body: BlockBody::default(), }; - let block_root = Bytes32(block.hash_tree_root()); + let block_root = containers::block::compute_block_root(&block); // Insert Block directly per leanSpec store.blocks.insert(block_root, block); diff --git a/lean_client/networking/src/discovery/mod.rs b/lean_client/networking/src/discovery/mod.rs index 7ee532b..6bceae2 100644 --- a/lean_client/networking/src/discovery/mod.rs +++ b/lean_client/networking/src/discovery/mod.rs @@ -16,6 +16,8 @@ use libp2p_identity::{Keypair, PeerId}; use tokio::sync::mpsc; use tracing::{debug, info, warn}; +use crate::enr_ext::EnrExt; + pub use config::DiscoveryConfig; /// Discovery service that wraps discv5 for peer discovery. @@ -127,7 +129,13 @@ impl DiscoveryService { .ip4() .map(IpAddr::V4) .or_else(|| enr.ip6().map(IpAddr::V6))?; - let libp2p_port = enr.tcp4().or_else(|| enr.tcp6())?; + + // Try TCP ports first (lean_client stores QUIC port in TCP field), + // then fall back to QUIC ports (genesis tools may use quic field directly) + let libp2p_port = enr.tcp4() + .or_else(|| enr.tcp6()) + .or_else(|| enr.quic4()) + .or_else(|| enr.quic6())?; let peer_id = enr_to_peer_id(enr)?; diff --git a/lean_client/src/main.rs b/lean_client/src/main.rs index cea50eb..97a9d75 100644 --- a/lean_client/src/main.rs +++ b/lean_client/src/main.rs @@ -394,7 +394,7 @@ async fn main() { match vs.build_block_proposal(&mut store, Slot(current_slot), proposer_idx) { Ok(signed_block) => { - let block_root = Bytes32(signed_block.message.block.hash_tree_root()); + let block_root = containers::block::compute_block_root(&signed_block.message.block); info!( slot = current_slot, block_root = %format!("0x{:x}", block_root.0), @@ -482,7 +482,7 @@ async fn main() { } => { let block_slot = signed_block_with_attestation.message.block.slot.0; let proposer = signed_block_with_attestation.message.block.proposer_index.0; - let block_root = Bytes32(signed_block_with_attestation.message.block.hash_tree_root()); + let block_root = containers::block::compute_block_root(&signed_block_with_attestation.message.block); let parent_root = signed_block_with_attestation.message.block.parent_root; info!( diff --git a/lean_client/validator/src/lib.rs b/lean_client/validator/src/lib.rs index f84b70a..4f5ea82 100644 --- a/lean_client/validator/src/lib.rs +++ b/lean_client/validator/src/lib.rs @@ -128,6 +128,13 @@ impl ValidatorService { ); let parent_root = get_proposal_head(store, slot); + + info!( + parent_root = %format!("0x{:x}", parent_root.0), + store_head = %format!("0x{:x}", store.head.0), + "Using parent root for block proposal" + ); + let parent_state = store .states .get(&parent_root) @@ -173,32 +180,39 @@ impl ValidatorService { // 3. Target block must be known // 4. Target is not already justified in parent state // 5. Source is justified in parent state + + // Helper: check if a slot is justified using RELATIVE indexing + // Slots at or before finalized_slot are implicitly justified + let finalized_slot = parent_state.latest_finalized.slot.0 as i64; + let is_slot_justified = |slot: Slot| -> bool { + if (slot.0 as i64) <= finalized_slot { + return true; // Implicitly justified (at or before finalized) + } + let relative_index = (slot.0 as i64 - finalized_slot - 1) as usize; + parent_state + .justified_slots + .get(relative_index) + .map(|b| *b) + .unwrap_or(false) + }; + let valid_attestations: Vec = store .latest_known_attestations .iter() .filter(|(_, data)| { - // Source must match the parent state's justified checkpoint (not store's!) - let source_matches = data.source == parent_state.latest_justified; + // Source must match the store's justified checkpoint + // (attestations are created with store.latest_justified as source) + let source_matches = data.source == store.latest_justified; // Target must be strictly after source let target_after_source = data.target.slot > data.source.slot; // Target block must be known let target_known = store.blocks.contains_key(&data.target.root); - // Check if target is NOT already justified (matching process_single_attestation) - let target_slot_idx = data.target.slot.0 as usize; - let target_already_justified = parent_state - .justified_slots - .get(target_slot_idx) - .map(|b| *b) - .unwrap_or(false); - - // Check if source is justified - let source_slot_idx = data.source.slot.0 as usize; - let source_is_justified = parent_state - .justified_slots - .get(source_slot_idx) - .map(|b| *b) - .unwrap_or(false); + // Check if target is NOT already justified (using relative indexing) + let target_already_justified = is_slot_justified(data.target.slot); + + // Check if source is justified (using relative indexing) + let source_is_justified = is_slot_justified(data.source.slot); source_matches && target_after_source From 61c32c0706f67c151ed0557b9ee3656ed2e37a60 Mon Sep 17 00:00:00 2001 From: Nojus Sandovas Date: Tue, 27 Jan 2026 01:11:29 +0200 Subject: [PATCH 2/3] Changes based on requested changes in PR #63 --- lean_client/containers/src/block.rs | 78 +++++++++++-------- lean_client/containers/src/state.rs | 4 +- .../containers/tests/test_vectors/runner.rs | 39 +++++----- .../tests/unit_tests/state_process.rs | 11 ++- lean_client/networking/src/network/service.rs | 12 +-- 5 files changed, 77 insertions(+), 67 deletions(-) diff --git a/lean_client/containers/src/block.rs b/lean_client/containers/src/block.rs index 3052737..f76551d 100644 --- a/lean_client/containers/src/block.rs +++ b/lean_client/containers/src/block.rs @@ -82,9 +82,6 @@ pub fn hash_tree_root(value: &T) -> Bytes32 { } /// Compute the canonical block root for a Block. -/// -/// This uses the Block's own hash_tree_root, which must be consistent -/// with how latest_block_header is stored and hashed in state transitions. pub fn compute_block_root(block: &Block) -> Bytes32 { Bytes32(block.hash_tree_root()) } @@ -133,7 +130,9 @@ impl SignedBlockWithAttestation { /// Verifies all attestation signatures using lean-multisig aggregated proofs. /// Each attestation has a single `MultisigAggregatedSignature` proof that covers /// all participating validators. - pub fn verify_signatures(&self, parent_state: State) -> bool { + /// + /// Returns `Ok(())` if all signatures are valid, or an error describing the failure. + pub fn verify_signatures(&self, parent_state: State) -> Result<(), String> { // Unpack the signed block components let block = &self.message.block; let signatures = &self.signature; @@ -141,11 +140,13 @@ impl SignedBlockWithAttestation { let attestation_signatures = signatures.attestation_signatures.clone(); // Verify signature count matches aggregated attestation count - assert_eq!( - aggregated_attestations.len_u64(), - attestation_signatures.len_u64(), - "Attestation signature groups must align with block body attestations" - ); + if aggregated_attestations.len_u64() != attestation_signatures.len_u64() { + return Err(format!( + "Attestation signature count mismatch: {} attestations vs {} signatures", + aggregated_attestations.len_u64(), + attestation_signatures.len_u64() + )); + } let validators = &parent_state.validators; let num_validators = validators.len_u64(); @@ -161,15 +162,26 @@ impl SignedBlockWithAttestation { // Ensure all validators exist in the active set for validator_id in &validator_ids { - assert!( - *validator_id < num_validators, - "Validator index out of range" - ); + if *validator_id >= num_validators { + return Err(format!( + "Validator index {} out of range (max {})", + validator_id, num_validators + )); + } } let attestation_data_root: [u8; 32] = hash_tree_root(&aggregated_attestation.data).0.into(); + // Collect validators, returning error if any not found + let mut collected_validators = Vec::with_capacity(validator_ids.len()); + for vid in &validator_ids { + let validator = validators + .get(*vid) + .map_err(|_| format!("Validator {} not found in state", vid))?; + collected_validators.push(validator); + } + // Verify the lean-multisig aggregated proof for this attestation // // The proof verifies that all validators in aggregation_bits signed @@ -177,41 +189,39 @@ impl SignedBlockWithAttestation { _aggregated_signature_proof .proof_data .verify_aggregated_payload( - &validator_ids - .iter() - .map(|vid| validators.get(*vid).expect("validator must exist")) - .collect::>(), + &collected_validators, &attestation_data_root, aggregated_attestation.data.slot.0 as u32, ) - .expect("Attestation aggregated signature verification failed"); + .map_err(|e| format!("Attestation aggregated signature verification failed: {:?}", e))?; } // Verify the proposer attestation signature (outside the attestation loop) let proposer_attestation = &self.message.proposer_attestation; let proposer_signature = &signatures.proposer_signature; - assert!( - proposer_attestation.validator_id.0 < num_validators, - "Proposer index out of range" - ); + if proposer_attestation.validator_id.0 >= num_validators { + return Err(format!( + "Proposer index {} out of range (max {})", + proposer_attestation.validator_id.0, num_validators + )); + } let proposer = validators .get(proposer_attestation.validator_id.0) - .expect("proposer must exist"); + .map_err(|_| format!("Proposer {} not found in state", proposer_attestation.validator_id.0))?; let proposer_root: [u8; 32] = hash_tree_root(&proposer_attestation.data).0.into(); - assert!( - verify_xmss_signature( - proposer.pubkey, - proposer_attestation.data.slot, - &proposer_root, - proposer_signature, - ), - "Proposer attestation signature verification failed" - ); - - true + if !verify_xmss_signature( + proposer.pubkey, + proposer_attestation.data.slot, + &proposer_root, + proposer_signature, + ) { + return Err("Proposer attestation signature verification failed".to_string()); + } + + Ok(()) } } diff --git a/lean_client/containers/src/state.rs b/lean_client/containers/src/state.rs index e61e0c2..6d981fa 100644 --- a/lean_client/containers/src/state.rs +++ b/lean_client/containers/src/state.rs @@ -566,7 +566,7 @@ impl State { if let Some(votes) = justifications.get(&target_root) { let num_validators = self.validators.len_u64() as usize; let count = votes.iter().filter(|&&v| v).count(); - let threshold = (2 * num_validators + 2) / 3; // ceil(2/3) + let threshold = (2 * num_validators).div_ceil(3); tracing::info!( target_slot = target_slot.0, @@ -583,7 +583,7 @@ impl State { tracing::info!( target_slot = target_slot.0, target_root = %format!("0x{:x}", target_root.0), - "JUSTIFICATION THRESHOLD REACHED!" + "Justification threshold reached" ); *latest_justified = vote.target.clone(); diff --git a/lean_client/containers/tests/test_vectors/runner.rs b/lean_client/containers/tests/test_vectors/runner.rs index 6192dad..687731a 100644 --- a/lean_client/containers/tests/test_vectors/runner.rs +++ b/lean_client/containers/tests/test_vectors/runner.rs @@ -683,27 +683,30 @@ impl TestRunner { if let Some(ref exception) = test_case.expect_exception { println!(" Expecting exception: {}", exception); - // Verify signatures - we expect this to fail (return false) - let result = signed_block.verify_signatures(anchor_state); - - if result { - println!(" \x1b[31m✗ FAIL: Signatures verified successfully but should have failed!\x1b[0m\n"); - return Err("Expected signature verification to fail, but it succeeded".into()); + // Verify signatures - we expect this to fail (return Err) + match signed_block.verify_signatures(anchor_state) { + Ok(()) => { + println!(" \x1b[31m✗ FAIL: Signatures verified successfully but should have failed!\x1b[0m\n"); + return Err("Expected signature verification to fail, but it succeeded".into()); + } + Err(_) => { + println!(" ✓ Correctly rejected: Invalid signatures detected"); + println!("\n\x1b[32m✓ PASS\x1b[0m\n"); + return Ok(()); + } } - - println!(" ✓ Correctly rejected: Invalid signatures detected"); - println!("\n\x1b[32m✓ PASS\x1b[0m\n"); - return Ok(()); } - let result = signed_block.verify_signatures(anchor_state); - if !result { - println!(" \x1b[31m✗ FAIL: Signature verification failed\x1b[0m\n"); - return Err("Signature verification failed".into()); + match signed_block.verify_signatures(anchor_state) { + Ok(()) => { + println!(" ✓ All signatures verified successfully"); + println!("\n\x1b[32m✓ PASS\x1b[0m\n"); + Ok(()) + } + Err(e) => { + println!(" \x1b[31m✗ FAIL: Signature verification failed: {}\x1b[0m\n", e); + Err(format!("Signature verification failed: {}", e).into()) + } } - - println!(" ✓ All signatures verified successfully"); - println!("\n\x1b[32m✓ PASS\x1b[0m\n"); - Ok(()) } } diff --git a/lean_client/containers/tests/unit_tests/state_process.rs b/lean_client/containers/tests/unit_tests/state_process.rs index 632b0b5..0c980e8 100644 --- a/lean_client/containers/tests/unit_tests/state_process.rs +++ b/lean_client/containers/tests/unit_tests/state_process.rs @@ -84,12 +84,17 @@ fn test_process_block_header_valid() { new_state.historical_block_hashes.get(0).ok(), Some(&genesis_header_root) ); - let justified_slot_0 = new_state + // After processing just the block header (no attestations), justified_slots + // uses relative indexing (slot X maps to index X - finalized_slot - 1). + // With finalized_slot = 0 and no attestations to justify slot 1, + // justified_slots should be empty or all false. + let justified_slot_1_relative = new_state .justified_slots - .get(0) + .get(0) // relative index 0 = slot 1 .map(|b| *b) .unwrap_or(false); - assert_eq!(justified_slot_0, true); + // Slot 1 is NOT justified yet (no attestations have been processed) + assert_eq!(justified_slot_1_relative, false); assert_eq!(new_state.latest_block_header.slot, Slot(1)); assert_eq!( new_state.latest_block_header.state_root, diff --git a/lean_client/networking/src/network/service.rs b/lean_client/networking/src/network/service.rs index cf78011..b0d37ee 100644 --- a/lean_client/networking/src/network/service.rs +++ b/lean_client/networking/src/network/service.rs @@ -655,11 +655,7 @@ where match signed_block_with_attestation.to_ssz() { Ok(bytes) => { if let Err(err) = self.publish_to_topic(GossipsubKind::Block, bytes) { - // Duplicate errors are expected - we receive our own blocks back from peers - let err_str = format!("{:?}", err); - if !err_str.contains("Duplicate") { - warn!(slot = slot, ?err, "Publish block with attestation failed"); - } + warn!(slot = slot, ?err, "Publish block with attestation failed"); } else { info!(slot = slot, "Broadcasted block with attestation"); } @@ -675,11 +671,7 @@ where match signed_attestation.to_ssz() { Ok(bytes) => { if let Err(err) = self.publish_to_topic(GossipsubKind::Attestation, bytes) { - // Duplicate errors are expected - we receive our own attestations back from peers - let err_str = format!("{:?}", err); - if !err_str.contains("Duplicate") { - warn!(slot = slot, ?err, "Publish attestation failed"); - } + warn!(slot = slot, ?err, "Publish attestation failed"); } else { info!(slot = slot, "Broadcasted attestation"); } From b059928fc92041ac1596bcad3090bf11113a54f0 Mon Sep 17 00:00:00 2001 From: Nojus Sandovas Date: Tue, 27 Jan 2026 02:09:56 +0200 Subject: [PATCH 3/3] Cargo fmt --- lean_client/containers/src/block.rs | 16 +++++++++++++--- lean_client/containers/src/state.rs | 12 ++++++++---- .../containers/tests/test_vectors/runner.rs | 5 ++++- .../containers/tests/unit_tests/state_process.rs | 4 ++-- lean_client/fork_choice/src/store.rs | 4 ++-- .../tests/fork_choice_test_vectors.rs | 3 ++- lean_client/networking/src/discovery/mod.rs | 5 +++-- lean_client/validator/src/lib.rs | 8 ++++---- 8 files changed, 38 insertions(+), 19 deletions(-) diff --git a/lean_client/containers/src/block.rs b/lean_client/containers/src/block.rs index f76551d..141aebc 100644 --- a/lean_client/containers/src/block.rs +++ b/lean_client/containers/src/block.rs @@ -130,7 +130,7 @@ impl SignedBlockWithAttestation { /// Verifies all attestation signatures using lean-multisig aggregated proofs. /// Each attestation has a single `MultisigAggregatedSignature` proof that covers /// all participating validators. - /// + /// /// Returns `Ok(())` if all signatures are valid, or an error describing the failure. pub fn verify_signatures(&self, parent_state: State) -> Result<(), String> { // Unpack the signed block components @@ -193,7 +193,12 @@ impl SignedBlockWithAttestation { &attestation_data_root, aggregated_attestation.data.slot.0 as u32, ) - .map_err(|e| format!("Attestation aggregated signature verification failed: {:?}", e))?; + .map_err(|e| { + format!( + "Attestation aggregated signature verification failed: {:?}", + e + ) + })?; } // Verify the proposer attestation signature (outside the attestation loop) @@ -209,7 +214,12 @@ impl SignedBlockWithAttestation { let proposer = validators .get(proposer_attestation.validator_id.0) - .map_err(|_| format!("Proposer {} not found in state", proposer_attestation.validator_id.0))?; + .map_err(|_| { + format!( + "Proposer {} not found in state", + proposer_attestation.validator_id.0 + ) + })?; let proposer_root: [u8; 32] = hash_tree_root(&proposer_attestation.data).0.into(); if !verify_xmss_signature( diff --git a/lean_client/containers/src/state.rs b/lean_client/containers/src/state.rs index 6d981fa..6d56e7a 100644 --- a/lean_client/containers/src/state.rs +++ b/lean_client/containers/src/state.rs @@ -460,7 +460,7 @@ impl State { } /// Process a single attestation's votes. - /// + /// /// NOTE: justified_slots uses RELATIVE indexing. Slot X maps to index (X - finalized_slot - 1). /// Slots at or before finalized_slot are implicitly justified (not stored in the bitlist). fn process_single_attestation( @@ -489,7 +489,10 @@ impl State { } // Calculate relative index: slot X maps to index (X - finalized_slot - 1) let relative_index = (slot.0 as i64 - finalized_slot_int - 1) as usize; - justified_slots.get(relative_index).copied().unwrap_or(false) + justified_slots + .get(relative_index) + .copied() + .unwrap_or(false) }; let source_is_justified = is_slot_justified(source_slot, justified_slots_working); @@ -589,8 +592,9 @@ impl State { // Use RELATIVE indexing for justified_slots_working // Calculate relative index for target slot - let target_relative_index = (target_slot.0 as i64 - finalized_slot_int - 1) as usize; - + let target_relative_index = + (target_slot.0 as i64 - finalized_slot_int - 1) as usize; + // Extend the working vec if needed if target_relative_index >= justified_slots_working.len() { justified_slots_working.resize(target_relative_index + 1, false); diff --git a/lean_client/containers/tests/test_vectors/runner.rs b/lean_client/containers/tests/test_vectors/runner.rs index 687731a..5aa84d7 100644 --- a/lean_client/containers/tests/test_vectors/runner.rs +++ b/lean_client/containers/tests/test_vectors/runner.rs @@ -704,7 +704,10 @@ impl TestRunner { Ok(()) } Err(e) => { - println!(" \x1b[31m✗ FAIL: Signature verification failed: {}\x1b[0m\n", e); + println!( + " \x1b[31m✗ FAIL: Signature verification failed: {}\x1b[0m\n", + e + ); Err(format!("Signature verification failed: {}", e).into()) } } diff --git a/lean_client/containers/tests/unit_tests/state_process.rs b/lean_client/containers/tests/unit_tests/state_process.rs index 0c980e8..6d93223 100644 --- a/lean_client/containers/tests/unit_tests/state_process.rs +++ b/lean_client/containers/tests/unit_tests/state_process.rs @@ -84,9 +84,9 @@ fn test_process_block_header_valid() { new_state.historical_block_hashes.get(0).ok(), Some(&genesis_header_root) ); - // After processing just the block header (no attestations), justified_slots + // After processing just the block header (no attestations), justified_slots // uses relative indexing (slot X maps to index X - finalized_slot - 1). - // With finalized_slot = 0 and no attestations to justify slot 1, + // With finalized_slot = 0 and no attestations to justify slot 1, // justified_slots should be empty or all false. let justified_slot_1_relative = new_state .justified_slots diff --git a/lean_client/fork_choice/src/store.rs b/lean_client/fork_choice/src/store.rs index 5eb27d3..f1b1e51 100644 --- a/lean_client/fork_choice/src/store.rs +++ b/lean_client/fork_choice/src/store.rs @@ -55,7 +55,7 @@ pub fn get_forkchoice_store( // Extract the plain Block from the signed block let block = anchor_block.message.block.clone(); let block_slot = block.slot; - + // Compute block root using the header hash (canonical block root) let block_root = containers::block::compute_block_root(&block); @@ -78,7 +78,7 @@ pub fn get_forkchoice_store( }; // Store the original anchor_state - do NOT modify it - // Modifying checkpoints would change its hash_tree_root(), breaking the + // Modifying checkpoints would change its hash_tree_root(), breaking the // consistency with block.state_root Store { time: block_slot.0 * INTERVALS_PER_SLOT, diff --git a/lean_client/fork_choice/tests/fork_choice_test_vectors.rs b/lean_client/fork_choice/tests/fork_choice_test_vectors.rs index 802742a..cd19be7 100644 --- a/lean_client/fork_choice/tests/fork_choice_test_vectors.rs +++ b/lean_client/fork_choice/tests/fork_choice_test_vectors.rs @@ -562,7 +562,8 @@ fn forkchoice(spec_file: &str) { message: block, signature: BlockSignatures::default(), }; - let block_root = containers::block::compute_block_root(&signed_block.message.block); + let block_root = + containers::block::compute_block_root(&signed_block.message.block); // Advance time to the block's slot to ensure attestations are processable // SECONDS_PER_SLOT is 4 (not 12) diff --git a/lean_client/networking/src/discovery/mod.rs b/lean_client/networking/src/discovery/mod.rs index 6bceae2..5d05d0b 100644 --- a/lean_client/networking/src/discovery/mod.rs +++ b/lean_client/networking/src/discovery/mod.rs @@ -129,10 +129,11 @@ impl DiscoveryService { .ip4() .map(IpAddr::V4) .or_else(|| enr.ip6().map(IpAddr::V6))?; - + // Try TCP ports first (lean_client stores QUIC port in TCP field), // then fall back to QUIC ports (genesis tools may use quic field directly) - let libp2p_port = enr.tcp4() + let libp2p_port = enr + .tcp4() .or_else(|| enr.tcp6()) .or_else(|| enr.quic4()) .or_else(|| enr.quic6())?; diff --git a/lean_client/validator/src/lib.rs b/lean_client/validator/src/lib.rs index 4f5ea82..adb17fb 100644 --- a/lean_client/validator/src/lib.rs +++ b/lean_client/validator/src/lib.rs @@ -128,13 +128,13 @@ impl ValidatorService { ); let parent_root = get_proposal_head(store, slot); - + info!( parent_root = %format!("0x{:x}", parent_root.0), store_head = %format!("0x{:x}", store.head.0), "Using parent root for block proposal" ); - + let parent_state = store .states .get(&parent_root) @@ -180,7 +180,7 @@ impl ValidatorService { // 3. Target block must be known // 4. Target is not already justified in parent state // 5. Source is justified in parent state - + // Helper: check if a slot is justified using RELATIVE indexing // Slots at or before finalized_slot are implicitly justified let finalized_slot = parent_state.latest_finalized.slot.0 as i64; @@ -195,7 +195,7 @@ impl ValidatorService { .map(|b| *b) .unwrap_or(false) }; - + let valid_attestations: Vec = store .latest_known_attestations .iter()