From 80598d5bba59f6800e651634f3905b3d2573ba60 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Gr=C3=BCner?= <47506558+MegaRedHand@users.noreply.github.com> Date: Fri, 17 Apr 2026 19:36:36 -0300 Subject: [PATCH] test: verify real signatures in fork-choice spec tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Swap the fork-choice harness's `_without_verification` shortcuts for real signature verification where it makes sense, and collapse the aggregated-attestation paths back into a single verifying entry point. Changes: - `store.rs`: delete `on_gossip_attestation_without_verification`; the test harness now uses the real verifying `on_gossip_attestation`. `on_block` / `on_block_without_verification` / `on_block_core(verify)` stay as they are — fork-choice fixtures don't ship block signatures, so the test harness still calls `on_block_without_verification`. - `test-fixtures` crate: hoist `deser_xmss_hex` out of `signature_types.rs` so both blockchain test binaries can share it. - `forkchoice_spectests.rs`: `attestation` steps decode the fixture's real XMSS signature and call `on_gossip_attestation(&signed, is_aggregator)`. `gossipAggregatedAttestation` steps now carry the real ~180 KB aggregated proof bytes through to `on_gossip_aggregated_attestation`. Extract `assert_step_outcome` helper to replace three duplicated match blocks. `AttestationStepData::signature` deserializes straight to `Option` via a new `deser_opt_xmss_hex`. - `SKIP_TESTS`: replace the previous placeholder skips with the 13 fixtures whose aggregated XMSS proofs are rejected because our verifier pins `leanEthereum/leanMultisig@2eb4b9d` while the fixtures are generated by `anshalshukla/leanMultisig@devnet-4`. The two forks disagree on the Fiat-Shamir `public_input` encoding and the aggregation bytecode program — every proof rejects with `InvalidProof`. Resolving that fork mismatch is a separate, devnet-wide coordination issue. Previously-skipped fixtures that now pass with real verification: - `test_gossip_attestation_with_invalid_signature` (correctly rejected) - `test_equivocating_proposer_with_split_attestations` --- crates/blockchain/src/store.rs | 53 ++---- .../blockchain/tests/forkchoice_spectests.rs | 152 +++++++----------- crates/blockchain/tests/signature_types.rs | 23 +-- crates/blockchain/tests/types.rs | 43 ++++- crates/common/test-fixtures/src/lib.rs | 16 ++ 5 files changed, 129 insertions(+), 158 deletions(-) diff --git a/crates/blockchain/src/store.rs b/crates/blockchain/src/store.rs index 04d3c0c6..58f478f2 100644 --- a/crates/blockchain/src/store.rs +++ b/crates/blockchain/src/store.rs @@ -602,8 +602,9 @@ pub fn on_gossip_attestation( /// Process a gossiped aggregated attestation from the aggregation subnet. /// /// Aggregated attestations arrive from committee aggregators and contain a proof -/// covering multiple validators. We store one aggregated payload entry per -/// participating validator so the fork choice extraction works uniformly. +/// covering multiple validators. After signature verification, one entry is +/// stored per unique attestation data (not per participating validator) in the +/// pending pool; participant bits are carried in the proof itself. pub fn on_gossip_aggregated_attestation( store: &mut Store, aggregated: SignedAggregatedAttestation, @@ -611,7 +612,6 @@ pub fn on_gossip_aggregated_attestation( validate_attestation_data(store, &aggregated.data) .inspect_err(|_| metrics::inc_attestations_invalid())?; - // Verify aggregated proof signature let target_state = store .get_state(&aggregated.data.target.root) .ok_or(StoreError::MissingTargetState(aggregated.data.target.root))?; @@ -647,18 +647,22 @@ pub fn on_gossip_aggregated_attestation( } .map_err(StoreError::AggregateVerificationFailed)?; - // Store one proof per attestation data (not per validator) - store.insert_new_aggregated_payload(hashed, aggregated.proof.clone()); + // Read stats before moving the proof into the store. let num_participants = aggregated.proof.participants.count_ones(); + let target_slot = aggregated.data.target.slot; + let target_root = aggregated.data.target.root; + let source_slot = aggregated.data.source.slot; + let slot = aggregated.data.slot; + + store.insert_new_aggregated_payload(hashed, aggregated.proof); metrics::update_latest_new_aggregated_payloads(store.new_aggregated_payloads_count()); - let slot = aggregated.data.slot; info!( slot, num_participants, - target_slot = aggregated.data.target.slot, - target_root = %ShortRoot(&aggregated.data.target.root.0), - source_slot = aggregated.data.source.slot, + target_slot, + target_root = %ShortRoot(&target_root.0), + source_slot, "Aggregated attestation processed" ); @@ -686,37 +690,6 @@ pub fn on_block_without_verification( on_block_core(store, signed_block, false) } -/// Process a gossip attestation without signature verification. -/// -/// Validates the attestation data and inserts it directly into the known -/// attestation payloads (bypassing the gossip → aggregate → promote pipeline). -/// Use only in tests where signatures are absent (e.g., fork choice spec tests). -pub fn on_gossip_attestation_without_verification( - store: &mut Store, - validator_id: u64, - data: AttestationData, -) -> Result<(), StoreError> { - validate_attestation_data(store, &data)?; - - // Validate the validator index exists in the target state - let target_state = store - .get_state(&data.target.root) - .ok_or(StoreError::MissingTargetState(data.target.root))?; - if validator_id >= target_state.validators.len() as u64 { - return Err(StoreError::InvalidValidatorIndex); - } - - let bits = aggregation_bits_from_validator_indices(&[validator_id]); - let proof = AggregatedSignatureProof::empty(bits); - let hashed = HashedAttestationData::new(data); - store.insert_known_aggregated_payload(hashed, proof); - - // Recalculate fork choice head after inserting the attestation - update_head(store, false); - - Ok(()) -} - /// Core block processing logic. /// /// When `verify` is true, cryptographic signatures are validated and stored diff --git a/crates/blockchain/tests/forkchoice_spectests.rs b/crates/blockchain/tests/forkchoice_spectests.rs index 38cc1ff1..98f2b98d 100644 --- a/crates/blockchain/tests/forkchoice_spectests.rs +++ b/crates/blockchain/tests/forkchoice_spectests.rs @@ -7,9 +7,9 @@ use std::{ use ethlambda_blockchain::{MILLISECONDS_PER_INTERVAL, MILLISECONDS_PER_SLOT, store}; use ethlambda_storage::{Store, backend::InMemoryBackend}; use ethlambda_types::{ - attestation::{AttestationData, XmssSignature}, + attestation::{AttestationData, SignedAggregatedAttestation, SignedAttestation, XmssSignature}, block::{AggregatedSignatureProof, Block, BlockSignatures, SignedBlock}, - primitives::{H256, HashTreeRoot as _}, + primitives::{ByteList, H256, HashTreeRoot as _}, signature::SIGNATURE_SIZE, state::State, }; @@ -21,29 +21,32 @@ const SUPPORTED_FIXTURE_FORMAT: &str = "fork_choice_test"; mod common; mod types; -// We don't check signatures in spec-tests, so invalid signature tests always pass. -// The gossipAggregatedAttestation/attestation tests fail because the harness inserts -// individual gossip attestations into known payloads (should be no-op) and aggregated -// attestations with validator_id=0 into known (should use proof.participants into new). -// The last three skips are fixtures whose attestation checks require the harness to -// route `gossipAggregatedAttestation` steps through the real aggregated path (see the -// follow-up PR). They're unblocked there. -// TODO: fix these +// Tests skipped until the leanMultisig fork mismatch is resolved: proofs in +// these fixtures are generated by `anshalshukla/leanMultisig@devnet-4`, while +// our verifier is pinned to `leanEthereum/leanMultisig@2eb4b9d`. The two +// disagree on the Fiat-Shamir `public_input` encoding and the aggregation +// bytecode program, so every proof rejects with `InvalidProof`. const SKIP_TESTS: &[&str] = &[ - "test_gossip_attestation_with_invalid_signature", "test_block_builder_fixed_point_advances_justification", - "test_equivocating_proposer_with_split_attestations", "test_finalization_prunes_stale_aggregated_payloads", + "test_finalization_prunes_stale_attestation_signatures", + "test_produce_block_enforces_max_attestations_data_limit", + "test_produce_block_includes_pending_attestations", + "test_safe_target_advances_incrementally_along_the_chain", + "test_safe_target_does_not_advance_below_supermajority", + "test_safe_target_follows_heavier_fork_on_split", + "test_safe_target_is_conservative_relative_to_lmd_ghost_head", "test_safe_target_uses_merged_pools_at_interval_3", "test_tick_interval_0_skips_acceptance_when_not_proposer", "test_tick_interval_progression_through_full_slot", + "test_valid_gossip_aggregated_attestation", ]; fn run(path: &Path) -> datatest_stable::Result<()> { if let Some(stem) = path.file_stem().and_then(|s| s.to_str()) && SKIP_TESTS.contains(&stem) { - println!("Skipping {stem} (gossip attestation not serialized in fixture)"); + println!("Skipping {stem} (see SKIP_TESTS comment)"); return Ok(()); } let tests = ForkChoiceTestVector::from_file(path)?; @@ -89,25 +92,7 @@ fn run(path: &Path) -> datatest_stable::Result<()> { // NOTE: the has_proposal argument is set to true, following the spec store::on_tick(&mut store, block_time_ms, true, false); let result = store::on_block_without_verification(&mut store, signed_block); - - match (result.is_ok(), step.valid) { - (true, false) => { - return Err(format!( - "Step {} expected failure but got success", - step_idx - ) - .into()); - } - (false, true) => { - return Err(format!( - "Step {} expected success but got failure: {:?}", - step_idx, - result.err() - ) - .into()); - } - _ => {} - } + assert_step_outcome(step_idx, step.valid, result)?; } "tick" => { // Fixtures use either `time` (UNIX seconds) or `interval` @@ -127,75 +112,44 @@ fn run(path: &Path) -> datatest_stable::Result<()> { let att_data = step .attestation .expect("attestation step missing attestation data"); - let domain_data: ethlambda_types::attestation::AttestationData = - att_data.data.into(); - let validator_id = att_data - .validator_id - .expect("attestation step missing validator_id"); + let signed_attestation = SignedAttestation { + validator_id: att_data + .validator_id + .expect("attestation step missing validator_id"), + data: att_data.data.into(), + signature: att_data + .signature + .expect("attestation step missing signature"), + }; + let is_aggregator = step.is_aggregator.unwrap_or(false); - let result = store::on_gossip_attestation_without_verification( + let result = store::on_gossip_attestation( &mut store, - validator_id, - domain_data, + &signed_attestation, + is_aggregator, ); - - match (result.is_ok(), step.valid) { - (true, false) => { - return Err(format!( - "Step {} expected failure but got success", - step_idx - ) - .into()); - } - (false, true) => { - return Err(format!( - "Step {} expected success but got failure: {:?}", - step_idx, - result.err() - ) - .into()); - } - _ => {} - } + assert_step_outcome(step_idx, step.valid, result)?; } "gossipAggregatedAttestation" => { - // Aggregated attestation fixtures now carry proof data with a - // participants bitfield, but the harness still uses the - // single-validator bypass here. Tests whose checks rely on the - // correct participants or pool routing are skipped via - // `SKIP_TESTS`; the follow-up PR wires the real verifying - // path through. let att_data = step .attestation .expect("gossipAggregatedAttestation step missing attestation data"); - let domain_data: ethlambda_types::attestation::AttestationData = - att_data.data.into(); - let validator_id = att_data.validator_id.unwrap_or(0); - - let result = store::on_gossip_attestation_without_verification( - &mut store, - validator_id, - domain_data, - ); + let proof_fixture = att_data + .proof + .expect("gossipAggregatedAttestation step missing proof"); + let proof_bytes: Vec = proof_fixture.proof_data.into(); + let proof_data = ByteList::try_from(proof_bytes) + .expect("aggregated proof data fits in ByteListMiB"); + let aggregated = SignedAggregatedAttestation { + data: att_data.data.into(), + proof: AggregatedSignatureProof::new( + proof_fixture.participants.into(), + proof_data, + ), + }; - match (result.is_ok(), step.valid) { - (true, false) => { - return Err(format!( - "Step {} expected failure but got success", - step_idx - ) - .into()); - } - (false, true) => { - return Err(format!( - "Step {} expected success but got failure: {:?}", - step_idx, - result.err() - ) - .into()); - } - _ => {} - } + let result = store::on_gossip_aggregated_attestation(&mut store, aggregated); + assert_step_outcome(step_idx, step.valid, result)?; } other => { return Err(format!("Unsupported step type '{other}'").into()); @@ -211,6 +165,20 @@ fn run(path: &Path) -> datatest_stable::Result<()> { Ok(()) } +fn assert_step_outcome( + step_idx: usize, + expected_valid: bool, + result: Result, +) -> datatest_stable::Result<()> { + match (result, expected_valid) { + (Ok(_), false) => Err(format!("Step {step_idx} expected failure but got success").into()), + (Err(err), true) => { + Err(format!("Step {step_idx} expected success but got failure: {err:?}").into()) + } + _ => Ok(()), + } +} + fn build_signed_block(block_data: types::BlockStepData) -> SignedBlock { let block: Block = block_data.to_block(); diff --git a/crates/blockchain/tests/signature_types.rs b/crates/blockchain/tests/signature_types.rs index 99d26cf0..5f955aaf 100644 --- a/crates/blockchain/tests/signature_types.rs +++ b/crates/blockchain/tests/signature_types.rs @@ -1,4 +1,4 @@ -use super::common::{AggregationBits, Block, Container, TestInfo, TestState}; +use super::common::{AggregationBits, Block, Container, TestInfo, TestState, deser_xmss_hex}; use ethlambda_types::attestation::{AggregationBits as EthAggregationBits, XmssSignature}; use ethlambda_types::block::{ AggregatedSignatureProof, AttestationSignatures, BlockSignatures, SignedBlock, @@ -110,24 +110,3 @@ pub struct AttestationSignature { pub struct ProofData { pub data: String, } - -// ============================================================================ -// Helpers -// ============================================================================ - -pub fn deser_xmss_hex<'de, D>(d: D) -> Result -where - D: serde::Deserializer<'de>, -{ - use serde::de::Error; - - let value = String::deserialize(d)?; - let bytes = hex::decode(value.strip_prefix("0x").unwrap_or(&value)) - .map_err(|_| D::Error::custom("XmssSignature value is not valid hex"))?; - XmssSignature::try_from(bytes).map_err(|_| { - D::Error::custom(format!( - "XmssSignature length != {}", - ethlambda_types::signature::SIGNATURE_SIZE - )) - }) -} diff --git a/crates/blockchain/tests/types.rs b/crates/blockchain/tests/types.rs index 4bf65e4a..a1f0232b 100644 --- a/crates/blockchain/tests/types.rs +++ b/crates/blockchain/tests/types.rs @@ -1,6 +1,7 @@ -use super::common::{self, Block, TestInfo, TestState}; +use super::common::{self, Block, TestInfo, TestState, deser_xmss_hex}; +use ethlambda_types::attestation::XmssSignature; use ethlambda_types::primitives::H256; -use serde::Deserialize; +use serde::{Deserialize, Deserializer}; use std::collections::HashMap; use std::path::Path; @@ -59,6 +60,8 @@ pub struct ForkChoiceStep { pub interval: Option, #[serde(rename = "hasProposal")] pub has_proposal: Option, + #[serde(rename = "isAggregator")] + pub is_aggregator: Option, } #[derive(Debug, Clone, Deserialize)] @@ -66,8 +69,40 @@ pub struct AttestationStepData { #[serde(rename = "validatorId")] pub validator_id: Option, pub data: common::AttestationData, - #[allow(dead_code)] - pub signature: Option, + #[serde(default, deserialize_with = "deser_opt_xmss_hex")] + pub signature: Option, + /// Present on `gossipAggregatedAttestation` steps. + pub proof: Option, +} + +#[derive(Debug, Clone, Deserialize)] +pub struct ProofStepData { + pub participants: common::AggregationBits, + #[serde(rename = "proofData")] + pub proof_data: HexByteList, +} + +/// Hex-encoded byte list in the fixture format: `{ "data": "0xdeadbeef" }`. +#[derive(Debug, Clone, Deserialize)] +pub struct HexByteList { + data: String, +} + +impl From for Vec { + fn from(value: HexByteList) -> Self { + let stripped = value.data.strip_prefix("0x").unwrap_or(&value.data); + hex::decode(stripped).expect("invalid hex in proof data") + } +} + +fn deser_opt_xmss_hex<'de, D>(d: D) -> Result, D::Error> +where + D: Deserializer<'de>, +{ + #[derive(Deserialize)] + struct Wrap(#[serde(deserialize_with = "deser_xmss_hex")] XmssSignature); + + Ok(Option::::deserialize(d)?.map(|w| w.0)) } #[derive(Debug, Clone, Deserialize)] diff --git a/crates/common/test-fixtures/src/lib.rs b/crates/common/test-fixtures/src/lib.rs index a05ea135..b3ce4b7e 100644 --- a/crates/common/test-fixtures/src/lib.rs +++ b/crates/common/test-fixtures/src/lib.rs @@ -2,10 +2,12 @@ use ethlambda_types::{ attestation::{ AggregatedAttestation as DomainAggregatedAttestation, AggregationBits as DomainAggregationBits, AttestationData as DomainAttestationData, + XmssSignature, }, block::{Block as DomainBlock, BlockBody as DomainBlockBody}, checkpoint::Checkpoint as DomainCheckpoint, primitives::H256, + signature::SIGNATURE_SIZE, state::{ ChainConfig, JustificationValidators, JustifiedSlots, State, Validator as DomainValidator, ValidatorPubkeyBytes, @@ -313,3 +315,17 @@ where .map_err(|_| D::Error::custom("ValidatorPubkey length != 52"))?; Ok(pubkey) } + +pub fn deser_xmss_hex<'de, D>(d: D) -> Result +where + D: serde::Deserializer<'de>, +{ + use serde::Deserialize; + use serde::de::Error; + + let value = String::deserialize(d)?; + let bytes = hex::decode(value.strip_prefix("0x").unwrap_or(&value)) + .map_err(|_| D::Error::custom("XmssSignature value is not valid hex"))?; + XmssSignature::try_from(bytes) + .map_err(|_| D::Error::custom(format!("XmssSignature length != {SIGNATURE_SIZE}"))) +}