diff --git a/.ai/CODE_REVIEW.md b/.ai/CODE_REVIEW.md index e4da3b22d5..2ce60c80fd 100644 --- a/.ai/CODE_REVIEW.md +++ b/.ai/CODE_REVIEW.md @@ -190,6 +190,14 @@ we typically try to avoid runtime panics outside of startup." - Edge cases handled? - Context provided with errors? +## Large PR Strategy + +Large PRs (10+ files) make it easy to miss subtle bugs in individual files. + +- **Group files by subsystem** (networking, store, types, etc.) and review each group, but pay extra attention to changes that cross subsystem boundaries. +- **Review shared type/interface changes first** — changes to function signatures, return types, or struct definitions ripple through all callers. When reviewing a large PR, identify these first and trace their impact across the codebase. Downstream code may silently change behavior even if it looks untouched. +- **Flag missing test coverage for changed behavior** — if a code path's semantics change (even subtly), check that tests exercise it. If not, flag the gap. + ## Deep Review Techniques ### Verify Against Specifications @@ -275,3 +283,4 @@ Group related state and behavior together. If two fields are always set together - [ ] Tests present: Non-trivial changes have tests - [ ] Lock safety: Lock ordering is safe and documented - [ ] No blocking: Async code doesn't block runtime + diff --git a/.claude/settings.json b/.claude/settings.json new file mode 100644 index 0000000000..ae426dd254 --- /dev/null +++ b/.claude/settings.json @@ -0,0 +1,15 @@ +{ + "hooks": { + "PostToolUse": [ + { + "matcher": "Edit|Write", + "hooks": [ + { + "type": "command", + "command": "echo '\n[Reminder] Run: cargo fmt --all && make lint-fix'" + } + ] + } + ] + } +} diff --git a/.githooks/pre-commit b/.githooks/pre-commit new file mode 100755 index 0000000000..42a5ca79e0 --- /dev/null +++ b/.githooks/pre-commit @@ -0,0 +1,5 @@ +#!/bin/sh +# Pre-commit hook: runs cargo fmt --check +# Install with: make install-hooks + +exec cargo fmt --check diff --git a/CLAUDE.md b/CLAUDE.md index 441c8e4274..79ed344e35 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -2,6 +2,14 @@ This file provides guidance for AI assistants (Claude Code, Codex, etc.) working with Lighthouse. +## CRITICAL - Always Follow + +After completing ANY code changes: +1. **MUST** run `cargo fmt --all && make lint-fix` to format and fix linting issues +2. **MUST** run `cargo check` to verify compilation before considering task complete + +Run `make install-hooks` if you have not already to install git hooks. Never skip git hooks. If cargo is not available install the toolchain. + ## Quick Reference ```bash diff --git a/Cargo.lock b/Cargo.lock index 5a8e76a8a8..419ba679db 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3427,9 +3427,9 @@ dependencies = [ [[package]] name = "fallible-iterator" -version = "0.2.0" +version = "0.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "4443176a9f2c162692bd3d352d745ef9413eec5782a80d8fd6f8a1ac692a07f7" +checksum = "2acce4a10f12dc2fb14a218589d4f1f62ef011b2d0cc4b3cb1bba8e94da14649" [[package]] name = "fallible-streaming-iterator" @@ -3933,7 +3933,6 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e5274423e17b7c9fc20b6e7e208532f9b19825d82dfd615708b70edd83df41f1" dependencies = [ "ahash", - "allocator-api2", ] [[package]] @@ -3958,15 +3957,6 @@ dependencies = [ "serde_core", ] -[[package]] -name = "hashlink" -version = "0.8.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e8094feaf31ff591f651a2664fb9cfd92bba7a60ce3197265e9482ebe753c8f7" -dependencies = [ - "hashbrown 0.14.5", -] - [[package]] name = "hashlink" version = "0.9.1" @@ -3985,6 +3975,15 @@ dependencies = [ "hashbrown 0.15.5", ] +[[package]] +name = "hashlink" +version = "0.11.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ea0b22561a9c04a7cb1a302c013e0259cd3b4bb619f145b32f72b8b4bcbed230" +dependencies = [ + "hashbrown 0.16.1", +] + [[package]] name = "hdrhistogram" version = "7.5.4" @@ -5323,9 +5322,9 @@ dependencies = [ [[package]] name = "libsqlite3-sys" -version = "0.25.2" +version = "0.36.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "29f835d03d717946d28b1d1ed632eb6f0e24a299388ee623d0c23118d3e8a7fa" +checksum = "95b4103cffefa72eb8428cb6b47d6627161e51c2739fc5e3b734584157bc642a" dependencies = [ "cc", "pkg-config", @@ -7163,12 +7162,13 @@ dependencies = [ [[package]] name = "r2d2_sqlite" -version = "0.21.0" +version = "0.32.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b4f5d0337e99cd5cacd91ffc326c6cc9d8078def459df560c4f9bf9ba4a51034" +checksum = "a2ebd03c29250cdf191da93a35118b4567c2ef0eacab54f65e058d6f4c9965f6" dependencies = [ "r2d2", "rusqlite", + "uuid 1.19.0", ] [[package]] @@ -7503,6 +7503,16 @@ dependencies = [ "archery", ] +[[package]] +name = "rsqlite-vfs" +version = "0.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a8a1f2315036ef6b1fbacd1972e8ee7688030b0a2121edfc2a6550febd41574d" +dependencies = [ + "hashbrown 0.16.1", + "thiserror 2.0.17", +] + [[package]] name = "rtnetlink" version = "0.13.1" @@ -7558,16 +7568,17 @@ checksum = "48fd7bd8a6377e15ad9d42a8ec25371b94ddc67abe7c8b9127bec79bebaaae18" [[package]] name = "rusqlite" -version = "0.28.0" +version = "0.38.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "01e213bc3ecb39ac32e81e51ebe31fd888a940515173e3a18a35f8c6e896422a" +checksum = "f1c93dd1c9683b438c392c492109cb702b8090b2bfc8fed6f6e4eb4523f17af3" dependencies = [ - "bitflags 1.3.2", + "bitflags 2.10.0", "fallible-iterator", "fallible-streaming-iterator", - "hashlink 0.8.4", + "hashlink 0.11.0", "libsqlite3-sys", "smallvec", + "sqlite-wasm-rs", ] [[package]] @@ -8374,6 +8385,18 @@ dependencies = [ "der", ] +[[package]] +name = "sqlite-wasm-rs" +version = "0.5.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2f4206ed3a67690b9c29b77d728f6acc3ce78f16bf846d83c94f76400320181b" +dependencies = [ + "cc", + "js-sys", + "rsqlite-vfs", + "wasm-bindgen", +] + [[package]] name = "ssz_types" version = "0.14.0" @@ -9514,6 +9537,7 @@ checksum = "e2e054861b4bd027cd373e18e8d8d8e6548085000e41290d95ce0c373a654b4a" dependencies = [ "getrandom 0.3.4", "js-sys", + "rand 0.9.2", "wasm-bindgen", ] @@ -10479,13 +10503,13 @@ dependencies = [ [[package]] name = "yaml-rust2" -version = "0.8.1" +version = "0.11.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8902160c4e6f2fb145dbe9d6760a75e3c9522d8bf796ed7047c85919ac7115f8" +checksum = "631a50d867fafb7093e709d75aaee9e0e0d5deb934021fcea25ac2fe09edc51e" dependencies = [ "arraydeque", "encoding_rs", - "hashlink 0.8.4", + "hashlink 0.11.0", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index 98e8c057b5..3b5a7dd6ba 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -227,7 +227,7 @@ reqwest = { version = "0.12", default-features = false, features = [ ] } ring = "0.17" rpds = "0.11" -rusqlite = { version = "0.28", features = ["bundled"] } +rusqlite = { version = "0.38", features = ["bundled"] } rust_eth_kzg = "0.9" safe_arith = "0.1" sensitive_url = { version = "0.1", features = ["serde"] } @@ -240,7 +240,7 @@ signing_method = { path = "validator_client/signing_method" } slasher = { path = "slasher", default-features = false } slashing_protection = { path = "validator_client/slashing_protection" } slot_clock = { path = "common/slot_clock" } -smallvec = { version = "1.11.2", features = ["arbitrary"] } +smallvec = "1" snap = "1" ssz_types = { version = "0.14.0", features = ["context_deserialize", "runtime_types"] } state_processing = { path = "consensus/state_processing" } diff --git a/Makefile b/Makefile index 0995a869f4..9786c17cc9 100644 --- a/Makefile +++ b/Makefile @@ -361,3 +361,9 @@ clean: cargo clean make -C $(EF_TESTS) clean make -C $(STATE_TRANSITION_VECTORS) clean + +# Installs git hooks from .githooks/ directory +install-hooks: + @ln -sf ../../.githooks/pre-commit .git/hooks/pre-commit + @chmod +x .githooks/pre-commit + @echo "Git hooks installed. Pre-commit hook runs 'cargo fmt --check'." diff --git a/beacon_node/beacon_chain/tests/payload_invalidation.rs b/beacon_node/beacon_chain/tests/payload_invalidation.rs index eb8e57a5d5..7fd70f0e77 100644 --- a/beacon_node/beacon_chain/tests/payload_invalidation.rs +++ b/beacon_node/beacon_chain/tests/payload_invalidation.rs @@ -6,7 +6,7 @@ use beacon_chain::{ INVALID_JUSTIFIED_PAYLOAD_SHUTDOWN_REASON, NotifyExecutionLayer, OverrideForkchoiceUpdate, StateSkipConfig, WhenSlotSkipped, canonical_head::{CachedHead, CanonicalHead}, - test_utils::{BeaconChainHarness, EphemeralHarnessType, test_spec}, + test_utils::{BeaconChainHarness, EphemeralHarnessType, fork_name_from_env, test_spec}, }; use execution_layer::{ ExecutionLayer, ForkchoiceState, PayloadAttributes, @@ -389,6 +389,9 @@ impl InvalidPayloadRig { /// Simple test of the different import types. #[tokio::test] async fn valid_invalid_syncing() { + if fork_name_from_env().is_some_and(|f| !f.bellatrix_enabled()) { + return; + } let mut rig = InvalidPayloadRig::new(); rig.move_to_terminal_block(); @@ -404,6 +407,9 @@ async fn valid_invalid_syncing() { /// `latest_valid_hash`. #[tokio::test] async fn invalid_payload_invalidates_parent() { + if fork_name_from_env().is_some_and(|f| !f.bellatrix_enabled()) { + return; + } let mut rig = InvalidPayloadRig::new().enable_attestations(); rig.move_to_terminal_block(); rig.import_block(Payload::Valid).await; // Import a valid transition block. @@ -460,6 +466,9 @@ async fn immediate_forkchoice_update_invalid_test( #[tokio::test] async fn immediate_forkchoice_update_payload_invalid() { + if fork_name_from_env().is_some_and(|f| !f.bellatrix_enabled()) { + return; + } immediate_forkchoice_update_invalid_test(|latest_valid_hash| Payload::Invalid { latest_valid_hash, }) @@ -468,11 +477,17 @@ async fn immediate_forkchoice_update_payload_invalid() { #[tokio::test] async fn immediate_forkchoice_update_payload_invalid_block_hash() { + if fork_name_from_env().is_some_and(|f| !f.bellatrix_enabled()) { + return; + } immediate_forkchoice_update_invalid_test(|_| Payload::InvalidBlockHash).await } #[tokio::test] async fn immediate_forkchoice_update_payload_invalid_terminal_block() { + if fork_name_from_env().is_some_and(|f| !f.bellatrix_enabled()) { + return; + } immediate_forkchoice_update_invalid_test(|_| Payload::Invalid { latest_valid_hash: Some(ExecutionBlockHash::zero()), }) @@ -482,6 +497,9 @@ async fn immediate_forkchoice_update_payload_invalid_terminal_block() { /// Ensure the client tries to exit when the justified checkpoint is invalidated. #[tokio::test] async fn justified_checkpoint_becomes_invalid() { + if fork_name_from_env().is_some_and(|f| !f.bellatrix_enabled()) { + return; + } let mut rig = InvalidPayloadRig::new().enable_attestations(); rig.move_to_terminal_block(); rig.import_block(Payload::Valid).await; // Import a valid transition block. @@ -524,6 +542,9 @@ async fn justified_checkpoint_becomes_invalid() { /// Ensure that a `latest_valid_hash` for a pre-finality block only reverts a single block. #[tokio::test] async fn pre_finalized_latest_valid_hash() { + if fork_name_from_env().is_some_and(|f| !f.bellatrix_enabled()) { + return; + } let num_blocks = E::slots_per_epoch() * 4; let finalized_epoch = 2; @@ -571,6 +592,9 @@ async fn pre_finalized_latest_valid_hash() { /// - Will not validate `latest_valid_root` and its ancestors. #[tokio::test] async fn latest_valid_hash_will_not_validate() { + if fork_name_from_env().is_some_and(|f| !f.bellatrix_enabled()) { + return; + } const LATEST_VALID_SLOT: u64 = 3; let mut rig = InvalidPayloadRig::new().enable_attestations(); @@ -618,6 +642,9 @@ async fn latest_valid_hash_will_not_validate() { /// Check behaviour when the `latest_valid_hash` is a junk value. #[tokio::test] async fn latest_valid_hash_is_junk() { + if fork_name_from_env().is_some_and(|f| !f.bellatrix_enabled()) { + return; + } let num_blocks = E::slots_per_epoch() * 5; let finalized_epoch = 3; @@ -659,6 +686,9 @@ async fn latest_valid_hash_is_junk() { /// Check that descendants of invalid blocks are also invalidated. #[tokio::test] async fn invalidates_all_descendants() { + if fork_name_from_env().is_some_and(|f| !f.bellatrix_enabled()) { + return; + } let num_blocks = E::slots_per_epoch() * 4 + E::slots_per_epoch() / 2; let finalized_epoch = 2; let finalized_slot = E::slots_per_epoch() * 2; @@ -766,6 +796,9 @@ async fn invalidates_all_descendants() { /// Check that the head will switch after the canonical branch is invalidated. #[tokio::test] async fn switches_heads() { + if fork_name_from_env().is_some_and(|f| !f.bellatrix_enabled()) { + return; + } let num_blocks = E::slots_per_epoch() * 4 + E::slots_per_epoch() / 2; let finalized_epoch = 2; let finalized_slot = E::slots_per_epoch() * 2; @@ -869,6 +902,9 @@ async fn switches_heads() { #[tokio::test] async fn invalid_during_processing() { + if fork_name_from_env().is_some_and(|f| !f.bellatrix_enabled()) { + return; + } let mut rig = InvalidPayloadRig::new(); rig.move_to_terminal_block(); @@ -901,6 +937,9 @@ async fn invalid_during_processing() { #[tokio::test] async fn invalid_after_optimistic_sync() { + if fork_name_from_env().is_some_and(|f| !f.bellatrix_enabled()) { + return; + } let mut rig = InvalidPayloadRig::new().enable_attestations(); rig.move_to_terminal_block(); rig.import_block(Payload::Valid).await; // Import a valid transition block. @@ -939,6 +978,9 @@ async fn invalid_after_optimistic_sync() { #[tokio::test] async fn manually_validate_child() { + if fork_name_from_env().is_some_and(|f| !f.bellatrix_enabled()) { + return; + } let mut rig = InvalidPayloadRig::new().enable_attestations(); rig.move_to_terminal_block(); rig.import_block(Payload::Valid).await; // Import a valid transition block. @@ -957,6 +999,9 @@ async fn manually_validate_child() { #[tokio::test] async fn manually_validate_parent() { + if fork_name_from_env().is_some_and(|f| !f.bellatrix_enabled()) { + return; + } let mut rig = InvalidPayloadRig::new().enable_attestations(); rig.move_to_terminal_block(); rig.import_block(Payload::Valid).await; // Import a valid transition block. @@ -975,6 +1020,9 @@ async fn manually_validate_parent() { #[tokio::test] async fn payload_preparation() { + if fork_name_from_env().is_some_and(|f| !f.bellatrix_enabled()) { + return; + } let mut rig = InvalidPayloadRig::new(); rig.move_to_terminal_block(); rig.import_block(Payload::Valid).await; @@ -1036,6 +1084,9 @@ async fn payload_preparation() { #[tokio::test] async fn invalid_parent() { + if fork_name_from_env().is_some_and(|f| !f.bellatrix_enabled()) { + return; + } let mut rig = InvalidPayloadRig::new(); rig.move_to_terminal_block(); rig.import_block(Payload::Valid).await; // Import a valid transition block. @@ -1108,6 +1159,9 @@ async fn invalid_parent() { /// Tests to ensure that we will still send a proposer preparation #[tokio::test] async fn payload_preparation_before_transition_block() { + if fork_name_from_env().is_some_and(|f| !f.bellatrix_enabled()) { + return; + } let rig = InvalidPayloadRig::new(); let el = rig.execution_layer(); @@ -1180,6 +1234,9 @@ async fn payload_preparation_before_transition_block() { #[tokio::test] async fn attesting_to_optimistic_head() { + if fork_name_from_env().is_some_and(|f| !f.bellatrix_enabled()) { + return; + } let mut rig = InvalidPayloadRig::new(); rig.move_to_terminal_block(); rig.import_block(Payload::Valid).await; // Import a valid transition block. @@ -1392,6 +1449,9 @@ impl InvalidHeadSetup { #[tokio::test] async fn recover_from_invalid_head_by_importing_blocks() { + if fork_name_from_env().is_some_and(|f| !f.bellatrix_enabled()) { + return; + } let InvalidHeadSetup { rig, fork_block, @@ -1437,6 +1497,9 @@ async fn recover_from_invalid_head_by_importing_blocks() { #[tokio::test] async fn recover_from_invalid_head_after_persist_and_reboot() { + if fork_name_from_env().is_some_and(|f| !f.bellatrix_enabled()) { + return; + } let InvalidHeadSetup { rig, fork_block: _, @@ -1479,6 +1542,9 @@ async fn recover_from_invalid_head_after_persist_and_reboot() { #[tokio::test] async fn weights_after_resetting_optimistic_status() { + if fork_name_from_env().is_some_and(|f| !f.bellatrix_enabled()) { + return; + } let mut rig = InvalidPayloadRig::new().enable_attestations(); rig.move_to_terminal_block(); rig.import_block(Payload::Valid).await; // Import a valid transition block. diff --git a/beacon_node/lighthouse_network/src/rpc/codec.rs b/beacon_node/lighthouse_network/src/rpc/codec.rs index 36d9726dd9..d1a3182fad 100644 --- a/beacon_node/lighthouse_network/src/rpc/codec.rs +++ b/beacon_node/lighthouse_network/src/rpc/codec.rs @@ -457,6 +457,9 @@ fn handle_error( Ok(None) } } + // All snappy errors from the snap crate bubble up as `Other` kind errors + // that imply invalid response + ErrorKind::Other => Err(RPCError::InvalidData(err.to_string())), _ => Err(RPCError::from(err)), } } @@ -2317,4 +2320,43 @@ mod tests { RPCError::InvalidData(_) )); } + + /// Test invalid snappy response. + #[test] + fn test_invalid_snappy_response() { + let spec = spec_with_all_forks_enabled(); + let fork_ctx = Arc::new(fork_context(ForkName::latest(), &spec)); + let max_packet_size = spec.max_payload_size as usize; // 10 MiB. + + let protocol = ProtocolId::new(SupportedProtocol::BlocksByRangeV2, Encoding::SSZSnappy); + + let mut codec = SSZSnappyOutboundCodec::::new( + protocol.clone(), + max_packet_size, + fork_ctx.clone(), + ); + + let mut payload = BytesMut::new(); + payload.extend_from_slice(&[0u8]); + let deneb_epoch = spec.deneb_fork_epoch.unwrap(); + payload.extend_from_slice(&fork_ctx.context_bytes(deneb_epoch)); + + // Claim the MAXIMUM allowed size (10 MiB) + let claimed_size = max_packet_size; + let mut uvi_codec: Uvi = Uvi::default(); + uvi_codec.encode(claimed_size, &mut payload).unwrap(); + payload.extend_from_slice(&[0xBB; 16]); // Junk snappy. + + let result = codec.decode(&mut payload); + + assert!(result.is_err(), "Expected decode to fail"); + + // IoError = reached snappy decode (allocation happened). + let err = result.unwrap_err(); + assert!( + matches!(err, RPCError::InvalidData(_)), + "Should return invalid data variant {}", + err + ); + } } diff --git a/beacon_node/lighthouse_network/src/service/mod.rs b/beacon_node/lighthouse_network/src/service/mod.rs index 3d709ed9b5..94e0ad0710 100644 --- a/beacon_node/lighthouse_network/src/service/mod.rs +++ b/beacon_node/lighthouse_network/src/service/mod.rs @@ -1861,8 +1861,6 @@ impl Network { self.inject_upnp_event(e); None } - #[allow(unreachable_patterns)] - BehaviourEvent::ConnectionLimits(le) => libp2p::core::util::unreachable(le), }, SwarmEvent::ConnectionEstablished { .. } => None, SwarmEvent::ConnectionClosed { .. } => None, diff --git a/beacon_node/network/src/network_beacon_processor/rpc_methods.rs b/beacon_node/network/src/network_beacon_processor/rpc_methods.rs index 5edd661bb6..279870d444 100644 --- a/beacon_node/network/src/network_beacon_processor/rpc_methods.rs +++ b/beacon_node/network/src/network_beacon_processor/rpc_methods.rs @@ -977,7 +977,10 @@ impl NetworkBeaconProcessor { }; // remove all skip slots i.e. duplicated roots - Ok(block_roots.into_iter().unique().collect::>()) + Ok(block_roots + .into_iter() + .unique_by(|(root, _)| *root) + .collect::>()) } /// Handle a `BlobsByRange` request from the peer. diff --git a/beacon_node/network/src/network_beacon_processor/tests.rs b/beacon_node/network/src/network_beacon_processor/tests.rs index aa03ee931d..4b0ca0d46c 100644 --- a/beacon_node/network/src/network_beacon_processor/tests.rs +++ b/beacon_node/network/src/network_beacon_processor/tests.rs @@ -120,6 +120,39 @@ impl TestRig { .await } + pub async fn new_with_skip_slots(chain_length: u64, skip_slots: &HashSet) -> Self { + let mut spec = test_spec::(); + spec.shard_committee_period = 2; + let spec = Arc::new(spec); + let beacon_processor_config = BeaconProcessorConfig::default(); + let harness = BeaconChainHarness::builder(MainnetEthSpec) + .spec(spec.clone()) + .deterministic_keypairs(VALIDATOR_COUNT) + .fresh_ephemeral_store() + .mock_execution_layer() + .node_custody_type(NodeCustodyType::Fullnode) + .chain_config(<_>::default()) + .build(); + + harness.advance_slot(); + + for slot in 1..=chain_length { + if !skip_slots.contains(&slot) { + harness + .extend_chain( + 1, + BlockStrategy::OnCanonicalHead, + AttestationStrategy::AllValidators, + ) + .await; + } + + harness.advance_slot(); + } + + Self::from_harness(harness, beacon_processor_config, spec).await + } + pub async fn new_parametric( chain_length: u64, beacon_processor_config: BeaconProcessorConfig, @@ -150,6 +183,14 @@ impl TestRig { harness.advance_slot(); } + Self::from_harness(harness, beacon_processor_config, spec).await + } + + async fn from_harness( + harness: BeaconChainHarness, + beacon_processor_config: BeaconProcessorConfig, + spec: Arc, + ) -> Self { let head = harness.chain.head_snapshot(); assert_eq!( @@ -1986,3 +2027,78 @@ async fn test_data_columns_by_range_request_only_returns_requested_columns() { "Should have received at least some data columns" ); } + +/// Test that DataColumnsByRange does not return duplicate data columns for skip slots. +/// +/// When skip slots occur, `forwards_iter_block_roots` returns the same block root for +/// consecutive slots. The deduplication in `get_block_roots_from_store` must use +/// `unique_by` on the root (not the full `(root, slot)` tuple) to avoid serving +/// duplicate data columns for the same block. +#[tokio::test] +async fn test_data_columns_by_range_no_duplicates_with_skip_slots() { + if test_spec::().fulu_fork_epoch.is_none() { + return; + }; + + // Build a chain of 128 slots (4 epochs) with skip slots at positions 5 and 6. + // After 4 epochs, finalized_epoch=2 (finalized_slot=64). Requesting slots 0-9 + // satisfies req_start_slot + req_count <= finalized_slot (10 <= 64), which routes + // through `get_block_roots_from_store` — the code path with the bug. + let skip_slots: HashSet = [5, 6].into_iter().collect(); + let mut rig = TestRig::new_with_skip_slots(128, &skip_slots).await; + + let all_custody_columns = rig.chain.custody_columns_for_epoch(Some(Epoch::new(0))); + let requested_column = vec![all_custody_columns[0]]; + + // Request a range that spans the skip slots (slots 0 through 9). + let start_slot = 0; + let slot_count = 10; + + rig.network_beacon_processor + .send_data_columns_by_range_request( + PeerId::random(), + InboundRequestId::new_unchecked(42, 24), + DataColumnsByRangeRequest { + start_slot, + count: slot_count, + columns: requested_column.clone(), + }, + ) + .unwrap(); + + // Collect block roots from all data column responses. + let mut block_roots: Vec = Vec::new(); + + while let Some(next) = rig.network_rx.recv().await { + if let NetworkMessage::SendResponse { + peer_id: _, + response: Response::DataColumnsByRange(data_column), + inbound_request_id: _, + } = next + { + if let Some(column) = data_column { + block_roots.push(column.block_root()); + } else { + break; + } + } else { + panic!("unexpected message {:?}", next); + } + } + + assert!( + !block_roots.is_empty(), + "Should have received at least some data columns" + ); + + // Before the fix, skip slots caused the same block root to appear multiple times + // (once per skip slot) because .unique() on (Hash256, Slot) tuples didn't deduplicate. + let unique_roots: HashSet<_> = block_roots.iter().collect(); + assert_eq!( + block_roots.len(), + unique_roots.len(), + "Response contained duplicate block roots: got {} columns but only {} unique roots", + block_roots.len(), + unique_roots.len(), + ); +} diff --git a/book/src/help_vm_import.md b/book/src/help_vm_import.md index 3c768f6705..09c1b74f4d 100644 --- a/book/src/help_vm_import.md +++ b/book/src/help_vm_import.md @@ -24,6 +24,9 @@ Options: --debug-level Specifies the verbosity level used when emitting logs to the terminal. [default: info] [possible values: info, debug, trace, warn, error] + --enabled + When provided, the imported validator will be enabled or disabled. + [possible values: true, false] --gas-limit When provided, the imported validator will use this gas limit. It is recommended to leave this as the default value by not specifying this diff --git a/common/eth2/src/lib.rs b/common/eth2/src/lib.rs index a5b4f9afdd..ac96da6173 100644 --- a/common/eth2/src/lib.rs +++ b/common/eth2/src/lib.rs @@ -77,8 +77,6 @@ const HTTP_GET_BEACON_BLOCK_SSZ_TIMEOUT_QUOTIENT: u32 = 4; const HTTP_GET_DEBUG_BEACON_STATE_QUOTIENT: u32 = 4; const HTTP_GET_DEPOSIT_SNAPSHOT_QUOTIENT: u32 = 4; const HTTP_GET_VALIDATOR_BLOCK_TIMEOUT_QUOTIENT: u32 = 4; -// Generally the timeout for events should be longer than a slot. -const HTTP_GET_EVENTS_TIMEOUT_MULTIPLIER: u32 = 50; const HTTP_DEFAULT_TIMEOUT_QUOTIENT: u32 = 4; /// A struct to define a variety of different timeouts for different validator tasks to ensure @@ -99,7 +97,6 @@ pub struct Timeouts { pub get_debug_beacon_states: Duration, pub get_deposit_snapshot: Duration, pub get_validator_block: Duration, - pub events: Duration, pub default: Duration, } @@ -120,7 +117,6 @@ impl Timeouts { get_debug_beacon_states: timeout, get_deposit_snapshot: timeout, get_validator_block: timeout, - events: HTTP_GET_EVENTS_TIMEOUT_MULTIPLIER * timeout, default: timeout, } } @@ -143,7 +139,6 @@ impl Timeouts { get_debug_beacon_states: base_timeout / HTTP_GET_DEBUG_BEACON_STATE_QUOTIENT, get_deposit_snapshot: base_timeout / HTTP_GET_DEPOSIT_SNAPSHOT_QUOTIENT, get_validator_block: base_timeout / HTTP_GET_VALIDATOR_BLOCK_TIMEOUT_QUOTIENT, - events: HTTP_GET_EVENTS_TIMEOUT_MULTIPLIER * base_timeout, default: base_timeout / HTTP_DEFAULT_TIMEOUT_QUOTIENT, } } @@ -3079,10 +3074,14 @@ impl BeaconNodeHttpClient { .join(","); path.query_pairs_mut().append_pair("topics", &topic_string); + // Do not use a timeout for the events endpoint. Using a regular timeout will trigger a + // timeout every `timeout` seconds, regardless of any data streamed from the endpoint. + // In future we could add a read_timeout, but that can only be configured globally on the + // Client. let mut es = self .client .get(path) - .timeout(self.timeouts.events) + .timeout(Duration::MAX) .eventsource() .map_err(Error::SseEventSource)?; // If we don't await `Event::Open` here, then the consumer diff --git a/consensus/int_to_bytes/Cargo.toml b/consensus/int_to_bytes/Cargo.toml index c639dfce8d..75196d7437 100644 --- a/consensus/int_to_bytes/Cargo.toml +++ b/consensus/int_to_bytes/Cargo.toml @@ -9,4 +9,4 @@ bytes = { workspace = true } [dev-dependencies] hex = { workspace = true } -yaml-rust2 = "0.8" +yaml-rust2 = "0.11" diff --git a/consensus/state_processing/Cargo.toml b/consensus/state_processing/Cargo.toml index a83e443e80..7426995439 100644 --- a/consensus/state_processing/Cargo.toml +++ b/consensus/state_processing/Cargo.toml @@ -8,6 +8,8 @@ edition = { workspace = true } default = [] fake_crypto = ["bls/fake_crypto"] arbitrary-fuzz = [ + "dep:arbitrary", + "smallvec/arbitrary", "types/arbitrary-fuzz", "merkle_proof/arbitrary", "ethereum_ssz/arbitrary", @@ -17,7 +19,7 @@ arbitrary-fuzz = [ portable = ["bls/supranational-portable"] [dependencies] -arbitrary = { workspace = true } +arbitrary = { workspace = true, optional = true } bls = { workspace = true } educe = { workspace = true } ethereum_hashing = { workspace = true } diff --git a/consensus/state_processing/src/verify_operation.rs b/consensus/state_processing/src/verify_operation.rs index 1f76f19586..a13786f9f6 100644 --- a/consensus/state_processing/src/verify_operation.rs +++ b/consensus/state_processing/src/verify_operation.rs @@ -7,6 +7,7 @@ use crate::per_block_processing::{ verify_attester_slashing, verify_bls_to_execution_change, verify_exit, verify_proposer_slashing, }; +#[cfg(feature = "arbitrary-fuzz")] use arbitrary::Arbitrary; use educe::Educe; use smallvec::{SmallVec, smallvec}; @@ -39,13 +40,17 @@ pub trait TransformPersist { /// /// The inner `op` field is private, meaning instances of this type can only be constructed /// by calling `validate`. -#[derive(Educe, Debug, Clone, Arbitrary)] +#[derive(Educe, Debug, Clone)] +#[cfg_attr(feature = "arbitrary-fuzz", derive(Arbitrary))] #[educe( PartialEq, Eq, Hash(bound(T: TransformPersist + std::hash::Hash, E: EthSpec)) )] -#[arbitrary(bound = "T: TransformPersist + Arbitrary<'arbitrary>, E: EthSpec")] +#[cfg_attr( + feature = "arbitrary-fuzz", + arbitrary(bound = "T: TransformPersist + Arbitrary<'arbitrary>, E: EthSpec") +)] pub struct SigVerifiedOp { op: T, verified_against: VerifiedAgainst, @@ -133,7 +138,8 @@ struct SigVerifiedOpDecode { /// /// We need to store multiple `ForkVersion`s because attester slashings contain two indexed /// attestations which may be signed using different versions. -#[derive(Debug, PartialEq, Eq, Clone, Hash, Encode, Decode, TestRandom, Arbitrary)] +#[derive(Debug, PartialEq, Eq, Clone, Hash, Encode, Decode, TestRandom)] +#[cfg_attr(feature = "arbitrary-fuzz", derive(Arbitrary))] pub struct VerifiedAgainst { fork_versions: SmallVec<[ForkVersion; MAX_FORKS_VERIFIED_AGAINST]>, } diff --git a/consensus/types/Cargo.toml b/consensus/types/Cargo.toml index a4b879ddb2..e7e382714b 100644 --- a/consensus/types/Cargo.toml +++ b/consensus/types/Cargo.toml @@ -16,6 +16,7 @@ sqlite = ["dep:rusqlite"] arbitrary = [ "dep:arbitrary", "bls/arbitrary", + "kzg/arbitrary", "ethereum_ssz/arbitrary", "milhouse/arbitrary", "ssz_types/arbitrary", diff --git a/crypto/bls/Cargo.toml b/crypto/bls/Cargo.toml index 4661288679..ac04e1fecf 100644 --- a/crypto/bls/Cargo.toml +++ b/crypto/bls/Cargo.toml @@ -5,7 +5,7 @@ authors = ["Paul Hauner "] edition = { workspace = true } [features] -arbitrary = [] +arbitrary = ["dep:arbitrary"] default = ["supranational"] fake_crypto = [] supranational = ["blst"] @@ -14,7 +14,7 @@ supranational-force-adx = ["supranational", "blst/force-adx"] [dependencies] alloy-primitives = { workspace = true } -arbitrary = { workspace = true } +arbitrary = { workspace = true, optional = true } blst = { version = "0.3.3", optional = true } ethereum_hashing = { workspace = true } ethereum_serde_utils = { workspace = true } diff --git a/crypto/kzg/Cargo.toml b/crypto/kzg/Cargo.toml index d2558663d5..840f8cfc9c 100644 --- a/crypto/kzg/Cargo.toml +++ b/crypto/kzg/Cargo.toml @@ -7,10 +7,11 @@ edition = "2021" [features] default = [] +arbitrary = ["dep:arbitrary"] fake_crypto = [] [dependencies] -arbitrary = { workspace = true } +arbitrary = { workspace = true, optional = true } c-kzg = { workspace = true } educe = { workspace = true } ethereum_hashing = { workspace = true } diff --git a/crypto/kzg/src/kzg_commitment.rs b/crypto/kzg/src/kzg_commitment.rs index 5a5e689429..bc5fc5f5aa 100644 --- a/crypto/kzg/src/kzg_commitment.rs +++ b/crypto/kzg/src/kzg_commitment.rs @@ -114,6 +114,7 @@ impl Debug for KzgCommitment { } } +#[cfg(feature = "arbitrary")] impl arbitrary::Arbitrary<'_> for KzgCommitment { fn arbitrary(u: &mut arbitrary::Unstructured<'_>) -> arbitrary::Result { let mut bytes = [0u8; BYTES_PER_COMMITMENT]; diff --git a/crypto/kzg/src/kzg_proof.rs b/crypto/kzg/src/kzg_proof.rs index 5a83466d0c..aa9ed185a0 100644 --- a/crypto/kzg/src/kzg_proof.rs +++ b/crypto/kzg/src/kzg_proof.rs @@ -110,6 +110,7 @@ impl Debug for KzgProof { } } +#[cfg(feature = "arbitrary")] impl arbitrary::Arbitrary<'_> for KzgProof { fn arbitrary(u: &mut arbitrary::Unstructured<'_>) -> arbitrary::Result { let mut bytes = [0u8; BYTES_PER_PROOF]; diff --git a/lcli/src/main.rs b/lcli/src/main.rs index a21dfd4386..63dd0f2c5b 100644 --- a/lcli/src/main.rs +++ b/lcli/src/main.rs @@ -492,10 +492,20 @@ fn main() { .long("jwt-output-path") .value_name("PATH") .action(ArgAction::Set) - .required(true) + .required_unless_present("jwt-secret-path") + .conflicts_with("jwt-secret-path") .help("Path to write the JWT secret.") .display_order(0) ) + .arg( + Arg::new("jwt-secret-path") + .long("jwt-secret-path") + .value_name("PATH") + .action(ArgAction::Set) + .help("Path to an existing hex-encoded JWT secret file. \ + When provided, this secret is used instead of the default.") + .display_order(0) + ) .arg( Arg::new("listen-address") .long("listen-address") diff --git a/lcli/src/mock_el.rs b/lcli/src/mock_el.rs index d6bdfb0d71..544010b6a2 100644 --- a/lcli/src/mock_el.rs +++ b/lcli/src/mock_el.rs @@ -2,7 +2,7 @@ use clap::ArgMatches; use clap_utils::{parse_optional, parse_required}; use environment::Environment; use execution_layer::{ - auth::JwtKey, + auth::{JwtKey, strip_prefix}, test_utils::{ Config, DEFAULT_JWT_SECRET, DEFAULT_TERMINAL_BLOCK, MockExecutionConfig, MockServer, }, @@ -13,7 +13,8 @@ use std::sync::Arc; use types::*; pub fn run(mut env: Environment, matches: &ArgMatches) -> Result<(), String> { - let jwt_path: PathBuf = parse_required(matches, "jwt-output-path")?; + let jwt_output_path: Option = parse_optional(matches, "jwt-output-path")?; + let jwt_secret_path: Option = parse_optional(matches, "jwt-secret-path")?; let listen_addr: Ipv4Addr = parse_required(matches, "listen-address")?; let listen_port: u16 = parse_required(matches, "listen-port")?; let all_payloads_valid: bool = parse_required(matches, "all-payloads-valid")?; @@ -25,8 +26,23 @@ pub fn run(mut env: Environment, matches: &ArgMatches) -> Result< let handle = env.core_context().executor.handle().unwrap(); let spec = Arc::new(E::default_spec()); - let jwt_key = JwtKey::from_slice(&DEFAULT_JWT_SECRET).unwrap(); - std::fs::write(jwt_path, hex::encode(DEFAULT_JWT_SECRET)).unwrap(); + + let jwt_key = if let Some(secret_path) = jwt_secret_path { + let hex_str = std::fs::read_to_string(&secret_path) + .map_err(|e| format!("Failed to read JWT secret file: {}", e))?; + let secret_bytes = hex::decode(strip_prefix(hex_str.trim())) + .map_err(|e| format!("Invalid hex in JWT secret file: {}", e))?; + JwtKey::from_slice(&secret_bytes) + .map_err(|e| format!("Invalid JWT secret length (expected 32 bytes): {}", e))? + } else if let Some(jwt_path) = jwt_output_path { + let jwt_key = JwtKey::from_slice(&DEFAULT_JWT_SECRET) + .map_err(|e| format!("Default JWT secret invalid: {}", e))?; + std::fs::write(jwt_path, hex::encode(jwt_key.as_bytes())) + .map_err(|e| format!("Failed to write JWT secret to output path: {}", e))?; + jwt_key + } else { + return Err("either --jwt-secret-path or --jwt-output-path must be provided".to_string()); + }; let config = MockExecutionConfig { server_config: Config { diff --git a/lighthouse/tests/validator_manager.rs b/lighthouse/tests/validator_manager.rs index d6d720a561..9bad1cdc91 100644 --- a/lighthouse/tests/validator_manager.rs +++ b/lighthouse/tests/validator_manager.rs @@ -16,6 +16,7 @@ use validator_manager::{ list_validators::ListConfig, move_validators::{MoveConfig, PasswordSource, Validators}, }; +use zeroize::Zeroizing; const EXAMPLE_ETH1_ADDRESS: &str = "0x00000000219ab540356cBB839Cbe05303d7705Fa"; @@ -280,6 +281,40 @@ pub fn validator_import_using_both_file_flags() { .assert_failed(); } +#[test] +pub fn validator_import_keystore_file_without_password_flag_should_fail() { + CommandLineTest::validators_import() + .flag("--vc-token", Some("./token.json")) + .flag("--keystore-file", Some("./keystore.json")) + .assert_failed(); +} + +#[test] +pub fn validator_import_keystore_file_with_password_flag_should_pass() { + CommandLineTest::validators_import() + .flag("--vc-token", Some("./token.json")) + .flag("--keystore-file", Some("./keystore.json")) + .flag("--password", Some("abcd")) + .assert_success(|config| { + let expected = ImportConfig { + validators_file_path: None, + keystore_file_path: Some(PathBuf::from("./keystore.json")), + vc_url: SensitiveUrl::parse("http://localhost:5062").unwrap(), + vc_token_path: PathBuf::from("./token.json"), + ignore_duplicates: false, + password: Some(Zeroizing::new("abcd".into())), + fee_recipient: None, + builder_boost_factor: None, + gas_limit: None, + builder_proposals: None, + enabled: None, + prefer_builder_proposals: None, + }; + assert_eq!(expected, config); + println!("{:?}", expected); + }); +} + #[test] pub fn validator_import_missing_both_file_flags() { CommandLineTest::validators_import() @@ -287,6 +322,36 @@ pub fn validator_import_missing_both_file_flags() { .assert_failed(); } +#[test] +pub fn validator_import_fee_recipient_override() { + CommandLineTest::validators_import() + .flag("--validators-file", Some("./vals.json")) + .flag("--vc-token", Some("./token.json")) + .flag("--suggested-fee-recipient", Some(EXAMPLE_ETH1_ADDRESS)) + .flag("--gas-limit", Some("1337")) + .flag("--builder-proposals", Some("true")) + .flag("--builder-boost-factor", Some("150")) + .flag("--prefer-builder-proposals", Some("true")) + .flag("--enabled", Some("false")) + .assert_success(|config| { + let expected = ImportConfig { + validators_file_path: Some(PathBuf::from("./vals.json")), + keystore_file_path: None, + vc_url: SensitiveUrl::parse("http://localhost:5062").unwrap(), + vc_token_path: PathBuf::from("./token.json"), + ignore_duplicates: false, + password: None, + fee_recipient: Some(Address::from_str(EXAMPLE_ETH1_ADDRESS).unwrap()), + builder_boost_factor: Some(150), + gas_limit: Some(1337), + builder_proposals: Some(true), + enabled: Some(false), + prefer_builder_proposals: Some(true), + }; + assert_eq!(expected, config); + }); +} + #[test] pub fn validator_move_defaults() { CommandLineTest::validators_move() diff --git a/testing/ef_tests/download_test_vectors.sh b/testing/ef_tests/download_test_vectors.sh index 21f74e817f..ff5b61bb47 100755 --- a/testing/ef_tests/download_test_vectors.sh +++ b/testing/ef_tests/download_test_vectors.sh @@ -4,7 +4,7 @@ set -Eeuo pipefail TESTS=("general" "minimal" "mainnet") version=${1} -if [[ "$version" == "nightly" ]]; then +if [[ "$version" == "nightly" || "$version" =~ ^nightly-[0-9]+$ ]]; then if [[ -z "${GITHUB_TOKEN:-}" ]]; then echo "Error GITHUB_TOKEN is not set" exit 1 @@ -21,9 +21,13 @@ if [[ "$version" == "nightly" ]]; then api="https://api.github.com" auth_header="Authorization: token ${GITHUB_TOKEN}" - run_id=$(curl -s -H "${auth_header}" \ - "${api}/repos/${repo}/actions/workflows/generate_vectors.yml/runs?branch=dev&status=success&per_page=1" | - jq -r '.workflow_runs[0].id') + if [[ "$version" == "nightly" ]]; then + run_id=$(curl --fail -s -H "${auth_header}" \ + "${api}/repos/${repo}/actions/workflows/nightly-reftests.yml/runs?branch=master&status=success&per_page=1" | + jq -r '.workflow_runs[0].id') + else + run_id="${version#nightly-}" + fi if [[ "${run_id}" == "null" || -z "${run_id}" ]]; then echo "No successful nightly workflow run found" @@ -31,7 +35,7 @@ if [[ "$version" == "nightly" ]]; then fi echo "Downloading nightly test vectors for run: ${run_id}" - curl -s -H "${auth_header}" "${api}/repos/${repo}/actions/runs/${run_id}/artifacts" | + curl --fail -H "${auth_header}" "${api}/repos/${repo}/actions/runs/${run_id}/artifacts" | jq -c '.artifacts[] | {name, url: .archive_download_url}' | while read -r artifact; do name=$(echo "${artifact}" | jq -r .name) diff --git a/validator_client/slashing_protection/Cargo.toml b/validator_client/slashing_protection/Cargo.toml index 86df6d01fe..695a693385 100644 --- a/validator_client/slashing_protection/Cargo.toml +++ b/validator_client/slashing_protection/Cargo.toml @@ -6,18 +6,18 @@ edition = { workspace = true } autotests = false [features] -arbitrary-fuzz = ["types/arbitrary-fuzz", "eip_3076/arbitrary-fuzz"] +arbitrary-fuzz = ["dep:arbitrary", "types/arbitrary-fuzz", "eip_3076/arbitrary-fuzz"] portable = ["types/portable"] [dependencies] -arbitrary = { workspace = true, features = ["derive"] } +arbitrary = { workspace = true, features = ["derive"], optional = true } bls = { workspace = true } eip_3076 = { workspace = true, features = ["json"] } ethereum_serde_utils = { workspace = true } filesystem = { workspace = true } fixed_bytes = { workspace = true } r2d2 = { workspace = true } -r2d2_sqlite = "0.21.0" +r2d2_sqlite = "0.32" rusqlite = { workspace = true } serde = { workspace = true } serde_json = { workspace = true } diff --git a/validator_manager/src/import_validators.rs b/validator_manager/src/import_validators.rs index 24917f7d1b..0d6d358edb 100644 --- a/validator_manager/src/import_validators.rs +++ b/validator_manager/src/import_validators.rs @@ -112,8 +112,7 @@ pub fn cli_app() -> Command { .value_name("ETH1_ADDRESS") .help("When provided, the imported validator will use the suggested fee recipient. Omit this flag to use the default value from the VC.") .action(ArgAction::Set) - .display_order(0) - .requires(KEYSTORE_FILE_FLAG), + .display_order(0), ) .arg( Arg::new(GAS_LIMIT) @@ -122,8 +121,7 @@ pub fn cli_app() -> Command { .help("When provided, the imported validator will use this gas limit. It is recommended \ to leave this as the default value by not specifying this flag.",) .action(ArgAction::Set) - .display_order(0) - .requires(KEYSTORE_FILE_FLAG), + .display_order(0), ) .arg( Arg::new(BUILDER_PROPOSALS) @@ -132,8 +130,7 @@ pub fn cli_app() -> Command { blocks via builder rather than the local EL.",) .value_parser(["true","false"]) .action(ArgAction::Set) - .display_order(0) - .requires(KEYSTORE_FILE_FLAG), + .display_order(0), ) .arg( Arg::new(BUILDER_BOOST_FACTOR) @@ -144,8 +141,7 @@ pub fn cli_app() -> Command { when choosing between a builder payload header and payload from \ the local execution node.",) .action(ArgAction::Set) - .display_order(0) - .requires(KEYSTORE_FILE_FLAG), + .display_order(0), ) .arg( Arg::new(PREFER_BUILDER_PROPOSALS) @@ -154,8 +150,16 @@ pub fn cli_app() -> Command { constructed by builders, regardless of payload value.",) .value_parser(["true","false"]) .action(ArgAction::Set) - .display_order(0) - .requires(KEYSTORE_FILE_FLAG), + .display_order(0), + ) + .arg( + Arg::new(ENABLED) + .long(ENABLED) + .help("When provided, the imported validator will be \ + enabled or disabled.",) + .value_parser(["true","false"]) + .action(ArgAction::Set) + .display_order(0), ) } @@ -225,48 +229,113 @@ async fn run(config: ImportConfig) -> Result<(), String> { enabled, } = config; - let validators: Vec = - if let Some(validators_format_path) = &validators_file_path { - if !validators_format_path.exists() { - return Err(format!( - "Unable to find file at {:?}", - validators_format_path - )); - } + let validators: Vec = if let Some(validators_format_path) = + &validators_file_path + { + if !validators_format_path.exists() { + return Err(format!( + "Unable to find file at {:?}", + validators_format_path + )); + } - let validators_file = fs::OpenOptions::new() - .read(true) - .create(false) - .open(validators_format_path) - .map_err(|e| format!("Unable to open {:?}: {:?}", validators_format_path, e))?; + let validators_file = fs::OpenOptions::new() + .read(true) + .create(false) + .open(validators_format_path) + .map_err(|e| format!("Unable to open {:?}: {:?}", validators_format_path, e))?; - serde_json::from_reader(&validators_file).map_err(|e| { + // Define validators as mutable so that if a relevant flag is supplied, the fields can be overridden. + let mut validators: Vec = serde_json::from_reader(&validators_file) + .map_err(|e| { format!( "Unable to parse JSON in {:?}: {:?}", validators_format_path, e ) - })? - } else if let Some(keystore_format_path) = &keystore_file_path { - vec![ValidatorSpecification { - voting_keystore: KeystoreJsonStr( - Keystore::from_json_file(keystore_format_path).map_err(|e| format!("{e:?}"))?, - ), - voting_keystore_password: password.ok_or_else(|| { - "The --password flag is required to supply the keystore password".to_string() - })?, - slashing_protection: None, - fee_recipient, - gas_limit, - builder_proposals, - builder_boost_factor, - prefer_builder_proposals, - enabled, - }] - } else { - return Err(format!( - "One of the flag --{VALIDATORS_FILE_FLAG} or --{KEYSTORE_FILE_FLAG} is required." - )); - }; + })?; + + // Log the overridden note when one or more flags is supplied + if let Some(override_fee_recipient) = fee_recipient { + eprintln!( + "Please note! --suggested-fee-recipient is provided. This will override existing fee recipient defined in validators.json with: {:?}", + override_fee_recipient + ); + } + if let Some(override_gas_limit) = gas_limit { + eprintln!( + "Please note! --gas-limit is provided. This will override existing gas limit defined in validators.json with: {}", + override_gas_limit + ); + } + if let Some(override_builder_proposals) = builder_proposals { + eprintln!( + "Please note! --builder-proposals is provided. This will override existing builder proposal setting defined in validators.json with: {}", + override_builder_proposals + ); + } + if let Some(override_builder_boost_factor) = builder_boost_factor { + eprintln!( + "Please note! --builder-boost-factor is provided. This will override existing builder boost factor defined in validators.json with: {}", + override_builder_boost_factor + ); + } + if let Some(override_prefer_builder_proposals) = prefer_builder_proposals { + eprintln!( + "Please note! --prefer-builder-proposals is provided. This will override existing prefer builder proposal setting defined in validators.json with: {}", + override_prefer_builder_proposals + ); + } + if let Some(override_enabled) = enabled { + eprintln!( + "Please note! --enabled flag is provided. This will override existing setting defined in validators.json with: {}", + override_enabled + ); + } + + // Override the fields in validators.json file if the flag is supplied + for validator in &mut validators { + if let Some(override_fee_recipient) = fee_recipient { + validator.fee_recipient = Some(override_fee_recipient); + } + if let Some(override_gas_limit) = gas_limit { + validator.gas_limit = Some(override_gas_limit); + } + if let Some(override_builder_proposals) = builder_proposals { + validator.builder_proposals = Some(override_builder_proposals); + } + if let Some(override_builder_boost_factor) = builder_boost_factor { + validator.builder_boost_factor = Some(override_builder_boost_factor); + } + if let Some(override_prefer_builder_proposals) = prefer_builder_proposals { + validator.prefer_builder_proposals = Some(override_prefer_builder_proposals); + } + if let Some(override_enabled) = enabled { + validator.enabled = Some(override_enabled); + } + } + + validators + } else if let Some(keystore_format_path) = &keystore_file_path { + vec![ValidatorSpecification { + voting_keystore: KeystoreJsonStr( + Keystore::from_json_file(keystore_format_path).map_err(|e| format!("{e:?}"))?, + ), + voting_keystore_password: password.ok_or_else(|| { + "The --password flag is required to supply the keystore password".to_string() + })?, + slashing_protection: None, + fee_recipient, + gas_limit, + builder_proposals, + builder_boost_factor, + prefer_builder_proposals, + enabled, + }] + } else { + return Err(format!( + "One of the flag --{VALIDATORS_FILE_FLAG} or --{KEYSTORE_FILE_FLAG} is required." + )); + }; let count = validators.len();