diff --git a/CLAUDE.md b/CLAUDE.md index 19864d7..9509c54 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -74,7 +74,7 @@ Fork choice head update ```bash make fmt # Format code (cargo fmt --all) make lint # Clippy with -D warnings -make test # All tests + forkchoice (with skip-signature-verification) +make test # All tests + forkchoice spec tests ``` ### Common Operations @@ -295,21 +295,22 @@ GENESIS_VALIDATORS: ### Test Categories 1. **Unit tests**: Embedded in source files 2. **Spec tests**: From `leanSpec/fixtures/consensus/` - - `forkchoice_spectests.rs` (requires `skip-signature-verification`) + - `forkchoice_spectests.rs` (uses `on_block_without_verification`) - `signature_spectests.rs` - `stf_spectests.rs` (state transition) ### Running Tests ```bash cargo test --workspace --release # All workspace tests -cargo test -p ethlambda-blockchain --features skip-signature-verification --test forkchoice_spectests -cargo test -p ethlambda-blockchain --features skip-signature-verification --test forkchoice_spectests -- --test-threads=1 # Sequential +cargo test -p ethlambda-blockchain --test forkchoice_spectests +cargo test -p ethlambda-blockchain --test forkchoice_spectests -- --test-threads=1 # Sequential ``` ## Common Gotchas ### Signature Verification -- Tests require `skip-signature-verification` feature for performance +- Fork choice tests use `on_block_without_verification()` to skip signature checks +- Signature spec tests use `on_block()` which always verifies - Crypto tests marked `#[ignore]` (slow leanVM operations) ### Storage Architecture diff --git a/Makefile b/Makefile index 36bdc13..ae133ca 100644 --- a/Makefile +++ b/Makefile @@ -9,10 +9,9 @@ fmt: ## ๐ŸŽจ Format all code using rustfmt lint: ## ๐Ÿ” Run clippy on all workspace crates cargo clippy --workspace --all-targets -- -D warnings -test: leanSpec/fixtures ## ๐Ÿงช Run all tests, then forkchoice tests with skip-signature-verification +test: leanSpec/fixtures ## ๐Ÿงช Run all tests # Tests need to be run on release to avoid stack overflows during signature verification/aggregation cargo test --workspace --release - cargo test -p ethlambda-blockchain --features skip-signature-verification --test forkchoice_spectests GIT_COMMIT=$(shell git rev-parse HEAD) GIT_BRANCH=$(shell git rev-parse --abbrev-ref HEAD) diff --git a/RELEASE.md b/RELEASE.md index e720f6d..e5044e6 100644 --- a/RELEASE.md +++ b/RELEASE.md @@ -67,13 +67,13 @@ The Dockerfile accepts build arguments for customizing the build: | Argument | Default | Description | |----------|---------|-------------| | `BUILD_PROFILE` | `release` | Cargo build profile | -| `FEATURES` | `""` | Extra Cargo features (e.g. `skip-signature-verification`) | +| `FEATURES` | `""` | Extra Cargo features | | `RUSTFLAGS` | `""` | Extra Rust compiler flags | Example with custom args: ```bash -docker build --build-arg FEATURES=skip-signature-verification -t ethlambda:test . +docker build --build-arg BUILD_PROFILE=debug -t ethlambda:debug . ``` `GIT_COMMIT` and `GIT_BRANCH` are also available but set automatically by CI. diff --git a/crates/blockchain/Cargo.toml b/crates/blockchain/Cargo.toml index d8e0825..8f28be5 100644 --- a/crates/blockchain/Cargo.toml +++ b/crates/blockchain/Cargo.toml @@ -10,10 +10,6 @@ rust-version.workspace = true version.workspace = true autotests = false -[features] -# To skip signature verification during tests -skip-signature-verification = [] - [dependencies] ethlambda-storage.workspace = true ethlambda-state-transition.workspace = true @@ -45,7 +41,6 @@ ssz_types = "0.14.0" name = "forkchoice_spectests" path = "tests/forkchoice_spectests.rs" harness = false -required-features = ["skip-signature-verification"] [[test]] name = "signature_spectests" diff --git a/crates/blockchain/src/store.rs b/crates/blockchain/src/store.rs index a7208da..1a33642 100644 --- a/crates/blockchain/src/store.rs +++ b/crates/blockchain/src/store.rs @@ -188,10 +188,36 @@ pub fn on_tick(store: &mut Store, timestamp: u64, has_proposal: bool) { } } -/// Process a gossiped attestation. +/// Process a gossiped attestation (with signature verification). +/// +/// This is the safe default: it always verifies the validator's XMSS signature +/// and stores it for future block building. pub fn on_gossip_attestation( store: &mut Store, signed_attestation: SignedAttestation, +) -> Result<(), StoreError> { + on_gossip_attestation_core(store, signed_attestation, true) +} + +/// Process a gossiped attestation without signature verification. +/// +/// This skips all cryptographic checks and signature storage. Use only in tests +/// where signatures are absent or irrelevant. +pub fn on_gossip_attestation_without_verification( + store: &mut Store, + signed_attestation: SignedAttestation, +) -> Result<(), StoreError> { + on_gossip_attestation_core(store, signed_attestation, false) +} + +/// Core gossip attestation processing logic. +/// +/// When `verify` is true, the validator's XMSS signature is checked and stored +/// for future block building. When false, all signature checks are skipped. +fn on_gossip_attestation_core( + store: &mut Store, + signed_attestation: SignedAttestation, + verify: bool, ) -> Result<(), StoreError> { let validator_id = signed_attestation.validator_id; let attestation = Attestation { @@ -200,35 +226,36 @@ pub fn on_gossip_attestation( }; validate_attestation(store, &attestation) .inspect_err(|_| metrics::inc_attestations_invalid("gossip"))?; - let target = attestation.data.target; - let target_state = store - .get_state(&target.root) - .ok_or(StoreError::MissingTargetState(target.root))?; - if validator_id >= target_state.validators.len() as u64 { - return Err(StoreError::InvalidValidatorIndex); - } - let validator_pubkey = target_state.validators[validator_id as usize] - .get_pubkey() - .map_err(|_| StoreError::PubkeyDecodingFailed(validator_id))?; - let message = attestation.data.tree_hash_root(); - if cfg!(not(feature = "skip-signature-verification")) { - use ethlambda_types::signature::ValidatorSignature; - // Use attestation.data.slot as epoch (matching what Zeam and ethlambda use for signing) + + if verify { + let target = attestation.data.target; + let target_state = store + .get_state(&target.root) + .ok_or(StoreError::MissingTargetState(target.root))?; + if validator_id >= target_state.validators.len() as u64 { + return Err(StoreError::InvalidValidatorIndex); + } + let validator_pubkey = target_state.validators[validator_id as usize] + .get_pubkey() + .map_err(|_| StoreError::PubkeyDecodingFailed(validator_id))?; + let message = attestation.data.tree_hash_root(); + + // Verify the validator's XMSS signature let epoch: u32 = attestation.data.slot.try_into().expect("slot exceeds u32"); let signature = ValidatorSignature::from_bytes(&signed_attestation.signature) .map_err(|_| StoreError::SignatureDecodingFailed)?; if !signature.is_valid(&validator_pubkey, epoch, &message) { return Err(StoreError::SignatureVerificationFailed); } - } - on_attestation(store, attestation.clone(), false)?; - if cfg!(not(feature = "skip-signature-verification")) { + on_attestation(store, attestation.clone(), false)?; + // Store signature for later lookup during block building - let signature = ValidatorSignature::from_bytes(&signed_attestation.signature) - .map_err(|_| StoreError::SignatureDecodingFailed)?; store.insert_gossip_signature(&attestation.data, validator_id, signature); + } else { + on_attestation(store, attestation.clone(), false)?; } + metrics::inc_attestations_valid("gossip"); let slot = attestation.data.slot; @@ -312,23 +339,40 @@ fn on_attestation( Ok(()) } -/// Process a new block and update the forkchoice state. +/// Process a new block and update the forkchoice state (with signature verification). /// -/// This method integrates a block into the forkchoice store by: -/// 1. Validating the block's parent exists -/// 2. Computing the post-state via the state transition function -/// 3. Processing attestations included in the block body (on-chain) -/// 4. Updating the forkchoice head -/// 5. Processing the proposer's attestation (as if gossiped) +/// This is the safe default: it always verifies cryptographic signatures +/// and stores them for future block building. Use this for all production paths. pub fn on_block( store: &mut Store, signed_block: SignedBlockWithAttestation, +) -> Result<(), StoreError> { + on_block_core(store, signed_block, true) +} + +/// Process a new block without signature verification. +/// +/// This skips all cryptographic checks and signature storage. Use only in tests +/// where signatures are absent or irrelevant (e.g., fork choice spec tests). +pub fn on_block_without_verification( + store: &mut Store, + signed_block: SignedBlockWithAttestation, +) -> Result<(), StoreError> { + on_block_core(store, signed_block, false) +} + +/// Core block processing logic. +/// +/// When `verify` is true, cryptographic signatures are validated and stored +/// for future block building. When false, all signature checks are skipped. +fn on_block_core( + store: &mut Store, + signed_block: SignedBlockWithAttestation, + verify: bool, ) -> Result<(), StoreError> { let _timing = metrics::time_fork_choice_block_processing(); - // Unpack block components - let block = signed_block.message.block.clone(); - let proposer_attestation = signed_block.message.proposer_attestation.clone(); + let block = &signed_block.message.block; let block_root = block.tree_hash_root(); let slot = block.slot; @@ -348,15 +392,18 @@ pub fn on_block( slot, })?; - // Validate cryptographic signatures - // TODO: extract signature verification to a pre-checks function - // to avoid the need for this - if cfg!(not(feature = "skip-signature-verification")) { + if verify { + // Validate cryptographic signatures verify_signatures(&parent_state, &signed_block)?; } + let block = signed_block.message.block.clone(); + let proposer_attestation = signed_block.message.proposer_attestation.clone(); + let block_root = block.tree_hash_root(); + let slot = block.slot; + // Execute state transition function to compute post-block state - let mut post_state = parent_state.clone(); + let mut post_state = parent_state; ethlambda_state_transition::state_transition(&mut post_state, &block)?; // Cache the state root in the latest block header @@ -414,11 +461,7 @@ pub fn on_block( // to prevent the proposer from gaining circular weight advantage. update_head(store); - // Process proposer attestation as if received via gossip - // The proposer's attestation should NOT affect this block's fork choice position. - // It is treated as pending until interval 3 (end of slot). - - if cfg!(not(feature = "skip-signature-verification")) { + if verify { // Store the proposer's signature for potential future block building let proposer_sig = ValidatorSignature::from_bytes(&signed_block.signature.proposer_signature) @@ -430,7 +473,9 @@ pub fn on_block( ); } - // Process proposer attestation (enters "new" stage, not "known") + // Process proposer attestation as if received via gossip + // The proposer's attestation should NOT affect this block's fork choice position. + // It is treated as pending until interval 3 (end of slot). // TODO: validate attestations before processing let _ = on_attestation(store, proposer_attestation, false) .inspect(|_| metrics::inc_attestations_valid("gossip")) diff --git a/crates/blockchain/tests/forkchoice_spectests.rs b/crates/blockchain/tests/forkchoice_spectests.rs index 7a9fd62..092f416 100644 --- a/crates/blockchain/tests/forkchoice_spectests.rs +++ b/crates/blockchain/tests/forkchoice_spectests.rs @@ -63,7 +63,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, true); - let result = store::on_block(&mut store, signed_block); + let result = store::on_block_without_verification(&mut store, signed_block); match (result.is_ok(), step.valid) { (true, false) => { @@ -303,46 +303,46 @@ fn validate_attestation_check( })?; // Validate attestation slot if specified - if let Some(expected_slot) = check.attestation_slot { - if attestation.slot != expected_slot { - return Err(format!( - "Step {}: attestation slot mismatch for validator {}: expected {}, got {}", - step_idx, validator_id, expected_slot, attestation.slot - ) - .into()); - } + if let Some(expected_slot) = check.attestation_slot + && attestation.slot != expected_slot + { + return Err(format!( + "Step {}: attestation slot mismatch for validator {}: expected {}, got {}", + step_idx, validator_id, expected_slot, attestation.slot + ) + .into()); } - if let Some(expected_head_slot) = check.head_slot { - if attestation.head.slot != expected_head_slot { - return Err(format!( - "Step {}: attestation head slot mismatch: expected {}, got {}", - step_idx, expected_head_slot, attestation.head.slot - ) - .into()); - } + if let Some(expected_head_slot) = check.head_slot + && attestation.head.slot != expected_head_slot + { + return Err(format!( + "Step {}: attestation head slot mismatch: expected {}, got {}", + step_idx, expected_head_slot, attestation.head.slot + ) + .into()); } // Validate source slot if specified - if let Some(expected_source_slot) = check.source_slot { - if attestation.source.slot != expected_source_slot { - return Err(format!( - "Step {}: attestation source slot mismatch: expected {}, got {}", - step_idx, expected_source_slot, attestation.source.slot - ) - .into()); - } + if let Some(expected_source_slot) = check.source_slot + && attestation.source.slot != expected_source_slot + { + return Err(format!( + "Step {}: attestation source slot mismatch: expected {}, got {}", + step_idx, expected_source_slot, attestation.source.slot + ) + .into()); } // Validate target slot if specified - if let Some(expected_target_slot) = check.target_slot { - if attestation.target.slot != expected_target_slot { - return Err(format!( - "Step {}: attestation target slot mismatch: expected {}, got {}", - step_idx, expected_target_slot, attestation.target.slot - ) - .into()); - } + if let Some(expected_target_slot) = check.target_slot + && attestation.target.slot != expected_target_slot + { + return Err(format!( + "Step {}: attestation target slot mismatch: expected {}, got {}", + step_idx, expected_target_slot, attestation.target.slot + ) + .into()); } Ok(())