From 539a77c531da42a4d54933203737604cf51261ab Mon Sep 17 00:00:00 2001 From: Jimmy Chen Date: Fri, 12 Jun 2026 12:17:49 +1000 Subject: [PATCH 01/15] Update warp snapshot build job (#9459) There's a recent breaking change that deprecated dashboard-based snapshot enablement, and is breaking our CI: https://www.warpbuild.com/docs/ci/changelog/2026-june#june-8-2026 > Legacy dashboard-based snapshot enablement has been removed. Runners previously configured with snapshots enabled from the dashboard will no longer use snapshots. To continue using snapshot runners, migrate to the label-based configuration by adding snapshot.enabled=true or snapshot.key= to your runs-on labels. See the [Snapshot Runners docs](https://www.warpbuild.com/docs/ci/features/snapshot-runners) for migration details. Co-Authored-By: Jimmy Chen --- .github/workflows/warpbuild-ubuntu-latest-snapshot.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/warpbuild-ubuntu-latest-snapshot.yml b/.github/workflows/warpbuild-ubuntu-latest-snapshot.yml index f32a0f0545..92f491214c 100644 --- a/.github/workflows/warpbuild-ubuntu-latest-snapshot.yml +++ b/.github/workflows/warpbuild-ubuntu-latest-snapshot.yml @@ -16,7 +16,7 @@ concurrency: jobs: bake: - runs-on: warp-ubuntu-latest-x64-8x + runs-on: warp-ubuntu-latest-x64-8x;snapshot.enabled=true steps: - name: Install system deps run: | From 90528469db9890d6bbaca521e747f29bbb50ee5d Mon Sep 17 00:00:00 2001 From: Mac L Date: Fri, 12 Jun 2026 23:36:47 +0400 Subject: [PATCH 02/15] Bump discv5 (#9394) Bump `discv5` to the latest release. This removes the duplicated `hashlink` dependency and also removes `ahash`. Co-Authored-By: Mac L --- Cargo.lock | 71 ++++++------------- Cargo.toml | 2 +- boot_node/src/config.rs | 16 ++++- boot_node/src/server.rs | 2 +- common/network_utils/src/discovery_metrics.rs | 2 +- 5 files changed, 41 insertions(+), 52 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c0f337a6a8..223a6192c8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -90,18 +90,6 @@ dependencies = [ "subtle", ] -[[package]] -name = "ahash" -version = "0.8.12" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5a15f179cd60c4584b8a8c596927aadc462e27f2ca70c04e0071964a73ba7a75" -dependencies = [ - "cfg-if", - "once_cell", - "version_check", - "zerocopy", -] - [[package]] name = "aho-corasick" version = "1.1.4" @@ -695,7 +683,7 @@ version = "1.1.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "40c48f72fd53cd289104fc64099abca73db4166ad86ea0b4341abe65af83dadc" dependencies = [ - "windows-sys 0.60.2", + "windows-sys 0.61.2", ] [[package]] @@ -706,7 +694,7 @@ checksum = "291e6a250ff86cd4a820112fb8898808a366d8f9f58ce16d1f538353ad55747d" dependencies = [ "anstyle", "once_cell_polyfill", - "windows-sys 0.60.2", + "windows-sys 0.61.2", ] [[package]] @@ -1400,7 +1388,7 @@ dependencies = [ "bitflags 2.10.0", "cexpr", "clang-sys", - "itertools 0.12.1", + "itertools 0.13.0", "log", "prettyplease", "proc-macro2", @@ -2708,9 +2696,9 @@ dependencies = [ [[package]] name = "discv5" -version = "0.10.2" +version = "0.11.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f170f4f6ed0e1df52bf43b403899f0081917ecf1500bfe312505cc3b515a8899" +checksum = "470ad731fddfc5b184e8bd2e3e24ddc6c7b006212af2b3989fd37a3de1217b4b" dependencies = [ "aes", "aes-gcm", @@ -2721,18 +2709,17 @@ dependencies = [ "enr", "fnv", "futures", - "hashlink 0.9.1", + "hashlink", "hex", "hkdf", "lazy_static", "libp2p-identity", - "lru 0.12.5", "more-asserts", "multiaddr", "parking_lot", "rand 0.8.5", "smallvec", - "socket2 0.5.10", + "socket2 0.6.4", "tokio", "tracing", "uint 0.10.0", @@ -3127,7 +3114,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "39cab71617ae0d63f51a36d69f866391735b51691dbda63cf6f96d042b63efeb" dependencies = [ "libc", - "windows-sys 0.59.0", + "windows-sys 0.61.2", ] [[package]] @@ -3979,9 +3966,6 @@ name = "hashbrown" version = "0.14.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e5274423e17b7c9fc20b6e7e208532f9b19825d82dfd615708b70edd83df41f1" -dependencies = [ - "ahash", -] [[package]] name = "hashbrown" @@ -4005,15 +3989,6 @@ dependencies = [ "serde_core", ] -[[package]] -name = "hashlink" -version = "0.9.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6ba4ff7128dee98c7dc9794b6a411377e1404dba1c97deb8d1a55297bd25d8af" -dependencies = [ - "hashbrown 0.14.5", -] - [[package]] name = "hashlink" version = "0.11.0" @@ -4434,7 +4409,7 @@ dependencies = [ "libc", "percent-encoding", "pin-project-lite", - "socket2 0.5.10", + "socket2 0.6.4", "tokio", "tower-service", "tracing", @@ -5212,7 +5187,7 @@ dependencies = [ "futures", "futures-timer", "getrandom 0.2.16", - "hashlink 0.11.0", + "hashlink", "hex_fmt", "libp2p-core", "libp2p-identity", @@ -5372,7 +5347,7 @@ dependencies = [ "futures", "futures-timer", "getrandom 0.2.16", - "hashlink 0.11.0", + "hashlink", "libp2p-core", "libp2p-identity", "libp2p-swarm-derive", @@ -6330,7 +6305,7 @@ version = "0.50.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7957b9740744892f114936ab4a57b3f487491bbeafaf8083688b16841a4240e5" dependencies = [ - "windows-sys 0.59.0", + "windows-sys 0.61.2", ] [[package]] @@ -7168,7 +7143,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8a56d757972c98b346a9b766e3f02746cde6dd1cd1d1d563472929fdd74bec4d" dependencies = [ "anyhow", - "itertools 0.12.1", + "itertools 0.14.0", "proc-macro2", "quote", "syn 2.0.117", @@ -7181,7 +7156,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "27c6023962132f4b30eb4c172c91ce92d933da334c59c23cddee82358ddafb0b" dependencies = [ "anyhow", - "itertools 0.10.5", + "itertools 0.14.0", "proc-macro2", "quote", "syn 2.0.117", @@ -7252,7 +7227,7 @@ dependencies = [ "quinn-udp", "rustc-hash 2.1.1", "rustls 0.23.40", - "socket2 0.5.10", + "socket2 0.6.4", "thiserror 2.0.17", "tokio", "tracing", @@ -7289,9 +7264,9 @@ dependencies = [ "cfg_aliases", "libc", "once_cell", - "socket2 0.5.10", + "socket2 0.6.4", "tracing", - "windows-sys 0.59.0", + "windows-sys 0.60.2", ] [[package]] @@ -7758,7 +7733,7 @@ dependencies = [ "bitflags 2.10.0", "fallible-iterator", "fallible-streaming-iterator", - "hashlink 0.11.0", + "hashlink", "libsqlite3-sys", "smallvec", "sqlite-wasm-rs", @@ -7836,7 +7811,7 @@ dependencies = [ "errno", "libc", "linux-raw-sys", - "windows-sys 0.59.0", + "windows-sys 0.61.2", ] [[package]] @@ -8559,7 +8534,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "52d1cfed4120b4d927bf7c0f86d2087a4a7d6027c906d9f9d525a80573b9be51" dependencies = [ "libc", - "windows-sys 0.60.2", + "windows-sys 0.61.2", ] [[package]] @@ -8904,7 +8879,7 @@ dependencies = [ "getrandom 0.3.4", "once_cell", "rustix", - "windows-sys 0.59.0", + "windows-sys 0.61.2", ] [[package]] @@ -10268,7 +10243,7 @@ version = "0.1.11" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c2a7b1c03c876122aa43f3020e6c3c3ee5c05081c9a00739faf7503aeba10d22" dependencies = [ - "windows-sys 0.59.0", + "windows-sys 0.61.2", ] [[package]] @@ -10860,7 +10835,7 @@ checksum = "631a50d867fafb7093e709d75aaee9e0e0d5deb934021fcea25ac2fe09edc51e" dependencies = [ "arraydeque", "encoding_rs", - "hashlink 0.11.0", + "hashlink", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index 50b1733232..23bae317b4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -127,7 +127,7 @@ delay_map = "0.4" deposit_contract = { path = "common/deposit_contract" } directory = { path = "common/directory" } dirs = "3" -discv5 = { version = "0.10", features = ["libp2p"] } +discv5 = { version = "0.11", features = ["libp2p"] } doppelganger_service = { path = "validator_client/doppelganger_service" } educe = "0.6" eip_3076 = { path = "common/eip_3076" } diff --git a/boot_node/src/config.rs b/boot_node/src/config.rs index 5b13b95c97..95c4f8f38d 100644 --- a/boot_node/src/config.rs +++ b/boot_node/src/config.rs @@ -11,7 +11,7 @@ use lighthouse_network::{ use network_utils::enr_ext::CombinedKeyExt; use serde::{Deserialize, Serialize}; use ssz::Encode; -use std::net::{SocketAddrV4, SocketAddrV6}; +use std::net::{SocketAddr, SocketAddrV4, SocketAddrV6}; use std::time::Duration; use std::{marker::PhantomData, path::PathBuf}; use tracing::{info, warn}; @@ -218,6 +218,20 @@ impl BootNodeConfigSerialization { Some(SocketAddrV4::new(ipv4, ipv4_port)), Some(SocketAddrV6::new(ipv6, ipv6_port, 0, 0)), ), + lighthouse_network::discv5::ListenConfig::FromSockets { ref ipv4, ref ipv6 } => ( + ipv4.as_ref() + .and_then(|socket| socket.local_addr().ok()) + .and_then(|addr| match addr { + SocketAddr::V4(addr) => Some(addr), + SocketAddr::V6(_) => None, + }), + ipv6.as_ref() + .and_then(|socket| socket.local_addr().ok()) + .and_then(|addr| match addr { + SocketAddr::V6(addr) => Some(addr), + SocketAddr::V4(_) => None, + }), + ), }; BootNodeConfigSerialization { diff --git a/boot_node/src/server.rs b/boot_node/src/server.rs index fce734bd70..3999e6b06b 100644 --- a/boot_node/src/server.rs +++ b/boot_node/src/server.rs @@ -128,7 +128,7 @@ pub async fn run( } // display server metrics - let metrics = discv5.metrics(); + let metrics = Discv5::metrics(); info!( connected_peers = discv5.connected_peers(), active_sessions = metrics.active_sessions, diff --git a/common/network_utils/src/discovery_metrics.rs b/common/network_utils/src/discovery_metrics.rs index 26a9e8a45f..8fceb70623 100644 --- a/common/network_utils/src/discovery_metrics.rs +++ b/common/network_utils/src/discovery_metrics.rs @@ -35,7 +35,7 @@ pub static DISCOVERY_SESSIONS: LazyLock> = LazyLock::new(|| { }); pub fn scrape_discovery_metrics() { - let metrics = discv5::metrics::Metrics::from(discv5::Discv5::raw_metrics()); + let metrics = discv5::Discv5::metrics(); set_float_gauge(&DISCOVERY_REQS, metrics.unsolicited_requests_per_second); set_gauge(&DISCOVERY_SESSIONS, metrics.active_sessions as i64); set_gauge_vec(&DISCOVERY_BYTES, &["inbound"], metrics.bytes_recv as i64); From 1d921b2b4e471e2cd1593e8d6e0d7b498e2d5b2b Mon Sep 17 00:00:00 2001 From: antondlr Date: Sat, 13 Jun 2026 15:23:30 +0200 Subject: [PATCH 03/15] Only bake warpbuild snapshot from stable (#9462) Co-Authored-By: antondlr --- .github/workflows/warpbuild-ubuntu-latest-snapshot.yml | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/.github/workflows/warpbuild-ubuntu-latest-snapshot.yml b/.github/workflows/warpbuild-ubuntu-latest-snapshot.yml index 92f491214c..3cbd737748 100644 --- a/.github/workflows/warpbuild-ubuntu-latest-snapshot.yml +++ b/.github/workflows/warpbuild-ubuntu-latest-snapshot.yml @@ -5,8 +5,11 @@ on: schedule: # Every week (Sunday at 00:00 UTC) - cron: "0 0 * * 0" - pull_request: - branches: [stable, unstable] + push: + # The warpbuild snapshot is global across all branches, so only `stable` + # bakes it. This avoids non-stable branches (and Mergify merge-queue PRs) + # clobbering the shared snapshot and racking up duplicate bakes. + branches: [stable] paths: - '.github/workflows/warpbuild-ubuntu-latest-snapshot.yml' From 227a7c976f4d5b289903bcae70f5bf8f496314bc Mon Sep 17 00:00:00 2001 From: hopinheimer <48147533+hopinheimer@users.noreply.github.com> Date: Sat, 13 Jun 2026 23:02:48 -0400 Subject: [PATCH 04/15] deprecate `disallowed-offsets` config for re-orgs (#9449) https://github.com/ethereum/consensus-specs/pull/5348. As per discussions in issue we no longer need to support `disallowed-offsets` for pre-Fulu forks. Co-Authored-By: hopinheimer --- beacon_node/beacon_chain/src/beacon_chain.rs | 97 +++++++++++-------- .../beacon_chain/src/block_production/mod.rs | 1 - beacon_node/beacon_chain/src/builder.rs | 10 -- beacon_node/beacon_chain/src/chain_config.rs | 7 -- .../http_api/tests/interactive_tests.rs | 41 +------- beacon_node/src/cli.rs | 7 +- beacon_node/src/config.rs | 17 +--- book/src/advanced_re-orgs.md | 10 +- book/src/help_bn.md | 7 +- consensus/fork_choice/src/fork_choice.rs | 8 +- consensus/proto_array/src/error.rs | 1 - consensus/proto_array/src/lib.rs | 4 +- .../src/proto_array_fork_choice.rs | 48 --------- lighthouse/tests/beacon_node.rs | 33 +------ testing/ef_tests/src/cases/fork_choice.rs | 2 - 15 files changed, 77 insertions(+), 216 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 569f0eea50..a7f1a7cfcd 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -5207,7 +5207,6 @@ impl BeaconChain { head_block_root, re_org_head_threshold, re_org_parent_threshold, - &self.config.re_org_disallowed_offsets, re_org_max_epochs_since_finalization, ) .map_err(|e| e.map_inner_error(Error::ProposerHeadForkChoiceError))?; @@ -5244,44 +5243,6 @@ impl BeaconChain { return Err(Box::new(DoNotReOrg::HeadDistance.into())); } - // Only attempt a re-org if we have a proposer registered for the re-org slot. - let proposing_at_re_org_slot = { - // We know our re-org block is not on the epoch boundary, so it has the same proposer - // shuffling as the head (but not necessarily the parent which may lie in the previous - // epoch). - let shuffling_decision_root = if self - .spec - .fork_name_at_slot::(re_org_block_slot) - .fulu_enabled() - { - info.head_node.current_epoch_shuffling_id() - } else { - info.head_node.next_epoch_shuffling_id() - } - .shuffling_decision_block; - let proposer_index = self - .beacon_proposer_cache - .lock() - .get_slot::(shuffling_decision_root, re_org_block_slot) - .ok_or_else(|| { - debug!( - slot = %re_org_block_slot, - decision_root = ?shuffling_decision_root, - "Fork choice override proposer shuffling miss" - ); - Box::new(DoNotReOrg::NotProposing.into()) - })? - .index as u64; - - self.execution_layer - .as_ref() - .ok_or(ProposerHeadError::Error(Error::ExecutionLayerMissing))? - .has_proposer_preparation_data_blocking(proposer_index) - }; - if !proposing_at_re_org_slot { - return Err(Box::new(DoNotReOrg::NotProposing.into())); - } - // TODO(gloas): reorg weight logic needs updating for Gloas. For now use // total weight which is correct for pre-Gloas and conservative for post-Gloas. let head_weight = info.head_node.weight(); @@ -5325,6 +5286,64 @@ impl BeaconChain { return Err(Box::new(DoNotReOrg::HeadNotLate.into())); } + // Only attempt a re-org if we have a proposer registered for the re-org slot. This check + // runs after the cheaper checks above because it may compute (and cache) the proposer + // shuffling for the re-org slot's epoch on a cache miss. + let proposing_at_re_org_slot = { + // Since Fulu, proposer shuffling is computed one epoch in advance, so the shuffling + // for the re-org block's epoch is always decided by an ancestor of the head, even + // when the re-org block lies in the epoch after the head (epoch boundary re-org). + let proposal_in_head_epoch = re_org_block_slot.epoch(T::EthSpec::slots_per_epoch()) + == head_slot.epoch(T::EthSpec::slots_per_epoch()); + let shuffling_decision_root = if self + .spec + .fork_name_at_slot::(re_org_block_slot) + .fulu_enabled() + && proposal_in_head_epoch + { + info.head_node.current_epoch_shuffling_id() + } else { + info.head_node.next_epoch_shuffling_id() + } + .shuffling_decision_block; + let proposer_index = self + .with_proposer_cache::( + shuffling_decision_root, + re_org_block_slot.epoch(T::EthSpec::slots_per_epoch()), + |proposers| { + proposers + .get_slot::(re_org_block_slot) + .map(|proposer| proposer.index as u64) + }, + || { + debug!( + slot = %re_org_block_slot, + decision_root = ?shuffling_decision_root, + "Fork choice override proposer shuffling miss" + ); + let head = self.canonical_head.cached_head(); + Ok((head.head_state_root(), head.snapshot.beacon_state.clone())) + }, + ) + .map_err(|e| match e { + Error::ProposerCacheIncorrectState { .. } => { + // The head changed while we were computing the proposer shuffling. + // Decline the re-org rather than erroring out. + warn!("Head changed during fork choice override check"); + Box::new(ProposerHeadError::from(DoNotReOrg::NotProposing)) + } + e => Box::new(ProposerHeadError::Error(e)), + })?; + + self.execution_layer + .as_ref() + .ok_or(ProposerHeadError::Error(Error::ExecutionLayerMissing))? + .has_proposer_preparation_data_blocking(proposer_index) + }; + if !proposing_at_re_org_slot { + return Err(Box::new(DoNotReOrg::NotProposing.into())); + } + // TODO(gloas): V29 nodes don't carry execution_status, so this returns // None for post-Gloas re-orgs. Need to source the EL block hash from // the bid's block_hash instead. Re-org is disabled for Gloas for now. diff --git a/beacon_node/beacon_chain/src/block_production/mod.rs b/beacon_node/beacon_chain/src/block_production/mod.rs index 17fa34ce02..84fb886b8f 100644 --- a/beacon_node/beacon_chain/src/block_production/mod.rs +++ b/beacon_node/beacon_chain/src/block_production/mod.rs @@ -220,7 +220,6 @@ impl BeaconChain { canonical_head, re_org_head_threshold, re_org_parent_threshold, - &self.config.re_org_disallowed_offsets, re_org_max_epochs_since_finalization, ) .map_err(|e| match e { diff --git a/beacon_node/beacon_chain/src/builder.rs b/beacon_node/beacon_chain/src/builder.rs index 6df0b9c1a9..c24c7b6fc6 100644 --- a/beacon_node/beacon_chain/src/builder.rs +++ b/beacon_node/beacon_chain/src/builder.rs @@ -30,7 +30,6 @@ use kzg::Kzg; use logging::crit; use operation_pool::{OperationPool, PersistedOperationPool}; use parking_lot::{Mutex, RwLock}; -use proto_array::DisallowedReOrgOffsets; use rand::RngCore; use rayon::prelude::*; use slasher::Slasher; @@ -176,15 +175,6 @@ where self } - /// Sets the proposer re-org disallowed offsets list. - pub fn proposer_re_org_disallowed_offsets( - mut self, - disallowed_offsets: DisallowedReOrgOffsets, - ) -> Self { - self.chain_config.re_org_disallowed_offsets = disallowed_offsets; - self - } - /// Sets the store (database). /// /// Should generally be called early in the build chain. diff --git a/beacon_node/beacon_chain/src/chain_config.rs b/beacon_node/beacon_chain/src/chain_config.rs index dde09bf105..939e6d8eed 100644 --- a/beacon_node/beacon_chain/src/chain_config.rs +++ b/beacon_node/beacon_chain/src/chain_config.rs @@ -1,5 +1,4 @@ use crate::custody_context::NodeCustodyType; -pub use proto_array::DisallowedReOrgOffsets; use serde::{Deserialize, Serialize}; use std::str::FromStr; use std::{collections::HashSet, sync::LazyLock, time::Duration}; @@ -36,11 +35,6 @@ pub struct ChainConfig { pub archive: bool, /// The max size of a message that can be sent over the network. pub max_network_size: usize, - /// Additional epoch offsets at which re-orging block proposals are not permitted. - /// - /// By default this list is empty, but it can be useful for reacting to network conditions, e.g. - /// slow gossip of re-org blocks at slot 1 in the epoch. - pub re_org_disallowed_offsets: DisallowedReOrgOffsets, /// Number of milliseconds to wait for fork choice before proposing a block. /// /// If set to 0 then block proposal will not wait for fork choice at all. @@ -123,7 +117,6 @@ impl Default for ChainConfig { weak_subjectivity_checkpoint: None, archive: false, max_network_size: 10 * 1_048_576, // 10M - re_org_disallowed_offsets: DisallowedReOrgOffsets::default(), fork_choice_before_proposal_timeout_ms: DEFAULT_FORK_CHOICE_BEFORE_PROPOSAL_TIMEOUT, // Builder fallback configs that are set in `clap` will override these. builder_fallback_skips: 3, diff --git a/beacon_node/http_api/tests/interactive_tests.rs b/beacon_node/http_api/tests/interactive_tests.rs index 7b5fb02714..e6135a81c7 100644 --- a/beacon_node/http_api/tests/interactive_tests.rs +++ b/beacon_node/http_api/tests/interactive_tests.rs @@ -2,7 +2,6 @@ use beacon_chain::custody_context::NodeCustodyType; use beacon_chain::{ ChainConfig, - chain_config::DisallowedReOrgOffsets, test_utils::{ AttestationStrategy, BlockStrategy, LightClientStrategy, SyncCommitteeStrategy, test_spec, }, @@ -188,8 +187,6 @@ pub struct ReOrgTest { misprediction: bool, /// Whether to expect withdrawals to change on epoch boundaries. expect_withdrawals_change_on_epoch: bool, - /// Epoch offsets to avoid proposing reorg blocks at. - disallowed_offsets: Vec, } impl Default for ReOrgTest { @@ -205,7 +202,6 @@ impl Default for ReOrgTest { should_re_org: true, misprediction: false, expect_withdrawals_change_on_epoch: false, - disallowed_offsets: vec![], } } } @@ -217,11 +213,13 @@ pub async fn proposer_boost_re_org_zero_weight() { proposer_boost_re_org_test(ReOrgTest::default()).await; } +// Since Fulu, proposer shuffling is stable across epoch boundaries, so re-orgs of the last block +// in an epoch are permitted. #[tokio::test(flavor = "multi_thread", worker_threads = 2)] pub async fn proposer_boost_re_org_epoch_boundary() { proposer_boost_re_org_test(ReOrgTest { head_slot: Slot::new(E::slots_per_epoch() - 1), - should_re_org: false, + should_re_org: true, ..Default::default() }) .await; @@ -317,32 +315,6 @@ pub async fn proposer_boost_re_org_head_distance() { .await; } -// Check that a re-org at a disallowed offset fails. -#[tokio::test(flavor = "multi_thread", worker_threads = 2)] -pub async fn proposer_boost_re_org_disallowed_offset() { - let offset = 4; - proposer_boost_re_org_test(ReOrgTest { - head_slot: Slot::new(E::slots_per_epoch() + offset - 1), - disallowed_offsets: vec![offset], - should_re_org: false, - ..Default::default() - }) - .await; -} - -// Check that a re-org at the *only* allowed offset succeeds. -#[tokio::test(flavor = "multi_thread", worker_threads = 2)] -pub async fn proposer_boost_re_org_disallowed_offset_exact() { - let offset = 4; - let disallowed_offsets = (0..E::slots_per_epoch()).filter(|o| *o != offset).collect(); - proposer_boost_re_org_test(ReOrgTest { - head_slot: Slot::new(E::slots_per_epoch() + offset - 1), - disallowed_offsets, - ..Default::default() - }) - .await; -} - #[tokio::test(flavor = "multi_thread", worker_threads = 2)] pub async fn proposer_boost_re_org_very_unhealthy() { proposer_boost_re_org_test(ReOrgTest { @@ -390,7 +362,6 @@ pub async fn proposer_boost_re_org_test( should_re_org, misprediction, expect_withdrawals_change_on_epoch, - disallowed_offsets, }: ReOrgTest, ) { assert!(head_slot > 0); @@ -419,11 +390,7 @@ pub async fn proposer_boost_re_org_test( Some(spec), validator_count, None, - Some(Box::new(move |builder| { - builder.proposer_re_org_disallowed_offsets( - DisallowedReOrgOffsets::new::(disallowed_offsets).unwrap(), - ) - })), + None, Default::default(), false, NodeCustodyType::Fullnode, diff --git a/beacon_node/src/cli.rs b/beacon_node/src/cli.rs index 988e2d1fc5..f06d0045b6 100644 --- a/beacon_node/src/cli.rs +++ b/beacon_node/src/cli.rs @@ -1375,12 +1375,7 @@ pub fn cli_app() -> Command { .long("proposer-reorg-disallowed-offsets") .action(ArgAction::Set) .value_name("N1,N2,...") - .help("Comma-separated list of integer offsets which can be used to avoid \ - proposing reorging blocks at certain slots. An offset of N means that \ - reorging proposals will not be attempted at any slot such that \ - `slot % SLOTS_PER_EPOCH == N`. By default only re-orgs at offset 0 will be \ - avoided. Any offsets supplied with this flag will impose additional \ - restrictions.") + .help("DEPRECATED. This flag has no effect.") .conflicts_with("disable-proposer-reorgs") .display_order(0) ) diff --git a/beacon_node/src/config.rs b/beacon_node/src/config.rs index ddf8d07c4e..1793dfa091 100644 --- a/beacon_node/src/config.rs +++ b/beacon_node/src/config.rs @@ -1,6 +1,6 @@ use account_utils::{STDIN_INPUTS_FLAG, read_input_from_user}; use beacon_chain::chain_config::{ - DEFAULT_PREPARE_PAYLOAD_LOOKAHEAD_FACTOR, DisallowedReOrgOffsets, INVALID_HOLESKY_BLOCK_ROOT, + DEFAULT_PREPARE_PAYLOAD_LOOKAHEAD_FACTOR, INVALID_HOLESKY_BLOCK_ROOT, }; use beacon_chain::custody_context::NodeCustodyType; use beacon_chain::graffiti_calculator::GraffitiOrigin; @@ -771,19 +771,10 @@ pub fn get_config( warn!("The proposer-reorg-parent-threshold flag is deprecated"); } - if let Some(disallowed_offsets_str) = - clap_utils::parse_optional::(cli_args, "proposer-reorg-disallowed-offsets")? + if clap_utils::parse_optional::(cli_args, "proposer-reorg-disallowed-offsets")? + .is_some() { - let disallowed_offsets = disallowed_offsets_str - .split(',') - .map(|s| { - s.parse() - .map_err(|e| format!("invalid disallowed-offsets: {e:?}")) - }) - .collect::, _>>()?; - client_config.chain.re_org_disallowed_offsets = - DisallowedReOrgOffsets::new::(disallowed_offsets) - .map_err(|e| format!("invalid disallowed-offsets: {e:?}"))?; + warn!("The proposer-reorg-disallowed-offsets flag is deprecated"); } client_config.chain.prepare_payload_lookahead = diff --git a/book/src/advanced_re-orgs.md b/book/src/advanced_re-orgs.md index 71751f354f..4e60d56192 100644 --- a/book/src/advanced_re-orgs.md +++ b/book/src/advanced_re-orgs.md @@ -11,15 +11,11 @@ attestations and transactions that can be included. ## Command line flags -There are three flags which control the re-orging behaviour: +There is one flag which controls the re-orging behaviour: * `--disable-proposer-reorgs`: turn re-orging off (it's on by default). -* `--proposer-reorg-disallowed-offsets N1,N2,N3...`: Prohibit Lighthouse from attempting to reorg at - specific offsets in each epoch. A disallowed offset `N` prevents reorging blocks from being - proposed at any `slot` such that `slot % SLOTS_PER_EPOCH == N`. The value to this flag is a - comma-separated list of integer offsets. -All flags should be applied to `lighthouse bn`. The default configuration is recommended as it +This flag should be applied to `lighthouse bn`. The default configuration is recommended as it balances the chance of the re-org succeeding against the chance of failure due to attestations arriving late and making the re-org block non-viable. @@ -32,8 +28,6 @@ The full conditions are described in [the spec][] but the most important ones ar * Only single-slot re-orgs: Lighthouse will build a block at N + 1 to re-org N by building on the parent N - 1. The result is a chain with exactly one skipped slot. -* No epoch boundaries: to ensure that the selected proposer does not change, Lighthouse will - not propose a re-orging block in the 0th slot of an epoch. ## Logs diff --git a/book/src/help_bn.md b/book/src/help_bn.md index 1f57db1b59..f02f1cb10e 100644 --- a/book/src/help_bn.md +++ b/book/src/help_bn.md @@ -308,12 +308,7 @@ Options: --proposer-reorg-cutoff DEPRECATED. This flag has no effect. --proposer-reorg-disallowed-offsets - Comma-separated list of integer offsets which can be used to avoid - proposing reorging blocks at certain slots. An offset of N means that - reorging proposals will not be attempted at any slot such that `slot % - SLOTS_PER_EPOCH == N`. By default only re-orgs at offset 0 will be - avoided. Any offsets supplied with this flag will impose additional - restrictions. + DEPRECATED. This flag has no effect. --proposer-reorg-epochs-since-finalization DEPRECATED. This flag has no effect. --proposer-reorg-parent-threshold diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index edced9b246..7816fb4483 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -3,8 +3,8 @@ use crate::{ForkChoiceStore, InvalidationOperation}; use fixed_bytes::FixedBytesExtended; use logging::crit; use proto_array::{ - Block as ProtoBlock, DisallowedReOrgOffsets, ExecutionStatus, JustifiedBalances, LatestMessage, - PayloadStatus, ProposerHeadError, ProposerHeadInfo, ProtoArrayForkChoice, ReOrgThreshold, + Block as ProtoBlock, ExecutionStatus, JustifiedBalances, LatestMessage, PayloadStatus, + ProposerHeadError, ProposerHeadInfo, ProtoArrayForkChoice, ReOrgThreshold, }; use ssz_derive::{Decode, Encode}; use state_processing::{ @@ -622,7 +622,6 @@ where canonical_head: Hash256, re_org_head_threshold: ReOrgThreshold, re_org_parent_threshold: ReOrgThreshold, - disallowed_offsets: &DisallowedReOrgOffsets, max_epochs_since_finalization: Epoch, ) -> Result>> { // Ensure that fork choice has already been updated for the current slot. This prevents @@ -655,7 +654,6 @@ where self.fc_store.justified_balances(), re_org_head_threshold, re_org_parent_threshold, - disallowed_offsets, max_epochs_since_finalization, ) .map_err(ProposerHeadError::convert_inner_error) @@ -666,7 +664,6 @@ where canonical_head: Hash256, re_org_head_threshold: ReOrgThreshold, re_org_parent_threshold: ReOrgThreshold, - disallowed_offsets: &DisallowedReOrgOffsets, max_epochs_since_finalization: Epoch, ) -> Result>> { let current_slot = self.fc_store.get_current_slot(); @@ -677,7 +674,6 @@ where self.fc_store.justified_balances(), re_org_head_threshold, re_org_parent_threshold, - disallowed_offsets, max_epochs_since_finalization, ) .map_err(ProposerHeadError::convert_inner_error) diff --git a/consensus/proto_array/src/error.rs b/consensus/proto_array/src/error.rs index eb0f30cc87..383c1946c6 100644 --- a/consensus/proto_array/src/error.rs +++ b/consensus/proto_array/src/error.rs @@ -50,7 +50,6 @@ pub enum Error { block_root: Hash256, parent_root: Hash256, }, - InvalidEpochOffset(u64), Arith(ArithError), InvalidNodeVariant { block_root: Hash256, diff --git a/consensus/proto_array/src/lib.rs b/consensus/proto_array/src/lib.rs index 702c014f07..5c3a4e3d7e 100644 --- a/consensus/proto_array/src/lib.rs +++ b/consensus/proto_array/src/lib.rs @@ -8,8 +8,8 @@ mod ssz_container; pub use crate::justified_balances::JustifiedBalances; pub use crate::proto_array::{InvalidationOperation, calculate_committee_fraction}; pub use crate::proto_array_fork_choice::{ - Block, DisallowedReOrgOffsets, DoNotReOrg, ExecutionStatus, LatestMessage, PayloadStatus, - ProposerHeadError, ProposerHeadInfo, ProtoArrayForkChoice, ReOrgThreshold, + Block, DoNotReOrg, ExecutionStatus, LatestMessage, PayloadStatus, ProposerHeadError, + ProposerHeadInfo, ProtoArrayForkChoice, ReOrgThreshold, }; pub use error::Error; diff --git a/consensus/proto_array/src/proto_array_fork_choice.rs b/consensus/proto_array/src/proto_array_fork_choice.rs index 2c1195b491..f0b599dbcd 100644 --- a/consensus/proto_array/src/proto_array_fork_choice.rs +++ b/consensus/proto_array/src/proto_array_fork_choice.rs @@ -385,10 +385,6 @@ pub enum DoNotReOrg { MissingHeadFinalizedCheckpoint, ParentDistance, HeadDistance, - ShufflingUnstable, - DisallowedOffset { - offset: u64, - }, JustificationAndFinalizationNotCompetitive, ChainNotFinalizing { epochs_since_finalization: u64, @@ -413,10 +409,6 @@ impl std::fmt::Display for DoNotReOrg { Self::MissingHeadFinalizedCheckpoint => write!(f, "finalized checkpoint missing"), Self::ParentDistance => write!(f, "parent too far from head"), Self::HeadDistance => write!(f, "head too far from current slot"), - Self::ShufflingUnstable => write!(f, "shuffling unstable at epoch boundary"), - Self::DisallowedOffset { offset } => { - write!(f, "re-orgs disabled at offset {offset}") - } Self::JustificationAndFinalizationNotCompetitive => { write!(f, "justification or finalization not competitive") } @@ -462,31 +454,6 @@ impl std::fmt::Display for DoNotReOrg { #[serde(transparent)] pub struct ReOrgThreshold(pub u64); -/// New-type for disallowed re-org slots. -#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] -#[serde(transparent)] -pub struct DisallowedReOrgOffsets { - // Vecs are faster than hashmaps for small numbers of items. - offsets: Vec, -} - -impl Default for DisallowedReOrgOffsets { - fn default() -> Self { - DisallowedReOrgOffsets { offsets: vec![0] } - } -} - -impl DisallowedReOrgOffsets { - pub fn new(offsets: Vec) -> Result { - for &offset in &offsets { - if offset >= E::slots_per_epoch() { - return Err(Error::InvalidEpochOffset(offset)); - } - } - Ok(Self { offsets }) - } -} - #[derive(PartialEq)] pub struct ProtoArrayForkChoice { pub(crate) proto_array: ProtoArray, @@ -724,7 +691,6 @@ impl ProtoArrayForkChoice { justified_balances: &JustifiedBalances, re_org_head_threshold: ReOrgThreshold, re_org_parent_threshold: ReOrgThreshold, - disallowed_offsets: &DisallowedReOrgOffsets, max_epochs_since_finalization: Epoch, ) -> Result> { let info = self.get_proposer_head_info::( @@ -733,7 +699,6 @@ impl ProtoArrayForkChoice { justified_balances, re_org_head_threshold, re_org_parent_threshold, - disallowed_offsets, max_epochs_since_finalization, )?; @@ -784,7 +749,6 @@ impl ProtoArrayForkChoice { justified_balances: &JustifiedBalances, re_org_head_threshold: ReOrgThreshold, re_org_parent_threshold: ReOrgThreshold, - disallowed_offsets: &DisallowedReOrgOffsets, max_epochs_since_finalization: Epoch, ) -> Result> { let mut nodes = self @@ -823,18 +787,6 @@ impl ProtoArrayForkChoice { return Err(DoNotReOrg::ParentDistance.into()); } - // Check shuffling stability. - let shuffling_stable = re_org_block_slot % E::slots_per_epoch() != 0; - if !shuffling_stable { - return Err(DoNotReOrg::ShufflingUnstable.into()); - } - - // Check allowed slot offsets. - let offset = (re_org_block_slot % E::slots_per_epoch()).as_u64(); - if disallowed_offsets.offsets.contains(&offset) { - return Err(DoNotReOrg::DisallowedOffset { offset }.into()); - } - // Check FFG. let ffg_competitive = parent_node.unrealized_justified_checkpoint() == head_node.unrealized_justified_checkpoint() diff --git a/lighthouse/tests/beacon_node.rs b/lighthouse/tests/beacon_node.rs index 38d4275a02..8bdab48282 100644 --- a/lighthouse/tests/beacon_node.rs +++ b/lighthouse/tests/beacon_node.rs @@ -1,7 +1,5 @@ use crate::exec::{CommandLineTestExec, CompletedTest}; -use beacon_node::beacon_chain::chain_config::{ - DEFAULT_SYNC_TOLERANCE_EPOCHS, DisallowedReOrgOffsets, -}; +use beacon_node::beacon_chain::chain_config::DEFAULT_SYNC_TOLERANCE_EPOCHS; use beacon_node::beacon_chain::custody_context::NodeCustodyType; use beacon_node::{ ClientConfig as Config, beacon_chain::graffiti_calculator::GraffitiOrigin, @@ -2361,35 +2359,10 @@ fn disable_proposer_re_orgs() { } #[test] -fn proposer_re_org_disallowed_offsets_default() { - CommandLineTest::new() - .run_with_zero_port() - .with_config(|config| { - assert_eq!( - config.chain.re_org_disallowed_offsets, - DisallowedReOrgOffsets::new::(vec![0]).unwrap() - ) - }); -} - -#[test] -fn proposer_re_org_disallowed_offsets_override() { +fn proposer_re_org_disallowed_offsets_deprecated() { + // The deprecated flag should be accepted but have no effect. CommandLineTest::new() .flag("proposer-reorg-disallowed-offsets", Some("1,2,3")) - .run_with_zero_port() - .with_config(|config| { - assert_eq!( - config.chain.re_org_disallowed_offsets, - DisallowedReOrgOffsets::new::(vec![1, 2, 3]).unwrap() - ) - }); -} - -#[test] -#[should_panic] -fn proposer_re_org_disallowed_offsets_invalid() { - CommandLineTest::new() - .flag("proposer-reorg-disallowed-offsets", Some("32,33,34")) .run_with_zero_port(); } diff --git a/testing/ef_tests/src/cases/fork_choice.rs b/testing/ef_tests/src/cases/fork_choice.rs index f640583189..cf122dbd65 100644 --- a/testing/ef_tests/src/cases/fork_choice.rs +++ b/testing/ef_tests/src/cases/fork_choice.rs @@ -3,7 +3,6 @@ use crate::decode::{ssz_decode_file, ssz_decode_file_with, ssz_decode_state, yam use ::fork_choice::{AttestationFromBlock, PayloadVerificationStatus, ProposerHeadError}; use beacon_chain::beacon_proposer_cache::compute_proposer_duties_from_head; use beacon_chain::block_verification_types::LookupBlock; -use beacon_chain::chain_config::DisallowedReOrgOffsets; use beacon_chain::data_column_verification::GossipVerifiedDataColumn; use beacon_chain::slot_clock::SlotClock; use beacon_chain::{ @@ -1040,7 +1039,6 @@ impl Tester { canonical_head, ReOrgThreshold(self.spec.reorg_head_weight_threshold), ReOrgThreshold(self.spec.reorg_parent_weight_threshold), - &DisallowedReOrgOffsets::default(), Epoch::new(self.spec.reorg_max_epochs_since_finalization), ); let proposer_head = match proposer_head_result { From 378f01670b824544da9f3ee01486ebf2ff4bfc07 Mon Sep 17 00:00:00 2001 From: Lion - dapplion <35266934+dapplion@users.noreply.github.com> Date: Sun, 14 Jun 2026 05:54:24 +0200 Subject: [PATCH 05/15] Allow release branches to target stable in target-branch-check (#9457) Co-Authored-By: dapplion <35266934+dapplion@users.noreply.github.com> --- .github/workflows/test-suite.yml | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/.github/workflows/test-suite.yml b/.github/workflows/test-suite.yml index 3db4804bd1..be0d0d5708 100644 --- a/.github/workflows/test-suite.yml +++ b/.github/workflows/test-suite.yml @@ -71,7 +71,15 @@ jobs: if: github.event_name == 'pull_request' || github.event_name == 'merge_group' steps: - name: Check that the pull request is not targeting the stable branch - run: test ${{ github.base_ref }} != "stable" + env: + BASE_REF: ${{ github.base_ref }} + HEAD_REF: ${{ github.head_ref }} + HEAD_REPO: ${{ github.event.pull_request.head.repo.full_name }} + run: | + if [[ "$BASE_REF" == "stable" && ! ( "$HEAD_REF" == release-* && "$HEAD_REPO" == "sigp/lighthouse" ) ]]; then + echo "::error::Pull requests must target unstable, not stable (sigp/lighthouse release-* branches excepted)." + exit 1 + fi forbidden-files-check: name: forbidden-files-check From d8e406b6acb31b26ae8f242dfa699a4c6880ce51 Mon Sep 17 00:00:00 2001 From: chonghe <44791194+chong-he@users.noreply.github.com> Date: Mon, 15 Jun 2026 14:03:13 +0800 Subject: [PATCH 06/15] Append client version info to user graffiti by default (#9313) Closes issue #9287 Co-Authored-By: Tan Chee Keong Co-Authored-By: chonghe <44791194+chong-he@users.noreply.github.com> Co-Authored-By: Michael Sproul --- .../beacon_chain/src/graffiti_calculator.rs | 6 +- beacon_node/execution_layer/src/engine_api.rs | 8 +- beacon_node/http_api/tests/tests.rs | 101 +++++++++++++++--- book/src/help_vc.md | 8 +- book/src/validator_graffiti.md | 38 ++++++- common/eth2/src/lib.rs | 15 +-- common/eth2/src/types.rs | 4 +- lighthouse/tests/validator_client.rs | 53 +++++++++ validator_client/src/cli.rs | 9 +- validator_client/src/config.rs | 2 +- 10 files changed, 203 insertions(+), 41 deletions(-) diff --git a/beacon_node/beacon_chain/src/graffiti_calculator.rs b/beacon_node/beacon_chain/src/graffiti_calculator.rs index 403873cc00..705121824c 100644 --- a/beacon_node/beacon_chain/src/graffiti_calculator.rs +++ b/beacon_node/beacon_chain/src/graffiti_calculator.rs @@ -62,7 +62,7 @@ impl GraffitiSettings { validator_graffiti .map(|graffiti| Self::Specified { graffiti, - policy: policy.unwrap_or(GraffitiPolicy::PreserveUserGraffiti), + policy: policy.unwrap_or_default(), }) .unwrap_or(Self::Unspecified) } @@ -480,9 +480,9 @@ mod tests { // for the case of empty append_graffiti_string, i.e., user-specified graffiti is 30-32 characters graffiti.to_string() } else { - // There is a space between the client version info and user graffiti + // There is a space between the user graffiti and client version info // as defined in calculate_graffiti function in engine_api.rs - format!("{} {}", append_graffiti_string, graffiti) + format!("{} {}", graffiti, append_graffiti_string) }; let expected_graffiti_prefix_bytes = expected_graffiti_string.as_bytes(); diff --git a/beacon_node/execution_layer/src/engine_api.rs b/beacon_node/execution_layer/src/engine_api.rs index d9dd9aaf4c..079a82ff98 100644 --- a/beacon_node/execution_layer/src/engine_api.rs +++ b/beacon_node/execution_layer/src/engine_api.rs @@ -832,16 +832,16 @@ impl ClientVersionV1 { // 12 characters for append_graffiti_full, plus one character for spacing // that leaves user specified graffiti to be 32-12-1 = 19 characters max, i.e., <20 if graffiti_length < 20 { - format!("{} {}", append_graffiti_full, graffiti_str) + format!("{} {}", graffiti_str, append_graffiti_full) // user-specified graffiti is between 20-23 characters } else if (20..24).contains(&graffiti_length) { - format!("{} {}", append_graffiti_one_byte, graffiti_str) + format!("{} {}", graffiti_str, append_graffiti_one_byte) // user-specified graffiti is between 24-27 characters } else if (24..28).contains(&graffiti_length) { - format!("{} {}", append_graffiti_no_commit, graffiti_str) + format!("{} {}", graffiti_str, append_graffiti_no_commit) // user-specified graffiti is between 28-29 characters } else if (28..30).contains(&graffiti_length) { - format!("{} {}", append_graffiti_only_el, graffiti_str) + format!("{} {}", graffiti_str, append_graffiti_only_el) // if user-specified graffiti is between 30-32 characters, append nothing } else { return graffiti; diff --git a/beacon_node/http_api/tests/tests.rs b/beacon_node/http_api/tests/tests.rs index 455a957337..843e055827 100644 --- a/beacon_node/http_api/tests/tests.rs +++ b/beacon_node/http_api/tests/tests.rs @@ -7967,8 +7967,8 @@ impl ApiTester { let graffiti = Some(Graffiti::from([0; GRAFFITI_BYTES_LEN])); let builder_boost_factor = None; - // Default case where GraffitiPolicy is None - let default_path = self + // When GraffitiPolicy is None + let no_graffiti_policy_path = self .client .get_validator_blocks_v3_path( slot, @@ -7981,13 +7981,30 @@ impl ApiTester { .await .unwrap(); + // Default case where GraffitiPolicy is AppendClientVersions + let default_path = self + .client + .get_validator_blocks_v3_path( + slot, + &randao_reveal, + graffiti.as_ref(), + SkipRandaoVerification::Yes, + builder_boost_factor, + Some(GraffitiPolicy::AppendClientVersions), + ) + .await + .unwrap(); + + let query_none_path = no_graffiti_policy_path.query().unwrap_or(""); let query_default_path = default_path.query().unwrap_or(""); - // When GraffitiPolicy is None, the HTTP API query path should not contain "graffiti_policy" + // When GraffitiPolicy is AppendClientVersions (default GraffitiPolicy), the HTTP API query path should not contain "graffiti_policy" assert!( !query_default_path.contains("graffiti_policy"), "URL should not contain graffiti_policy parameter (same as PreserveUserGraffiti). URL is: {}", query_default_path ); + // The HTTP API query path for GraffiliPolicy is None should be the same as the default (GraffitiPolicy = AppendClientVersions) + assert_eq!(query_none_path, query_default_path); let preserve_path = self .client @@ -8003,36 +8020,86 @@ impl ApiTester { .unwrap(); let query_preserve_path = preserve_path.query().unwrap_or(""); - // When GraffitiPolicy is set to PreserveUserGraffiti, the HTTP API query path should not contain "graffiti_policy" + // When GraffitiPolicy is set to PreserveUserGraffiti, the HTTP API query path should contain "graffiti_policy" assert!( - !query_preserve_path.contains("graffiti_policy"), + query_preserve_path.contains("graffiti_policy"), "URL should not contain graffiti_policy parameter when using PreserveUserGraffiti. URL is: {}", query_preserve_path ); - // The HTTP API query path for PreserveUserGraffiti should be the same as the default - assert_eq!(query_default_path, query_preserve_path); + self + } - let append_path = self + async fn get_validator_blocks_v4_path_graffiti_policy(self) -> Self { + let slot = self.chain.slot().unwrap(); + let epoch = self.chain.epoch().unwrap(); + let (_, randao_reveal) = self.get_test_randao(slot, epoch).await; + let graffiti = Some(Graffiti::from([0; GRAFFITI_BYTES_LEN])); + let builder_boost_factor = None; + + // When GraffitiPolicy is None + let no_graffiti_policy_path = self .client - .get_validator_blocks_v3_path( + .get_validator_blocks_v4_path( slot, &randao_reveal, graffiti.as_ref(), - SkipRandaoVerification::No, + SkipRandaoVerification::Yes, + None, + builder_boost_factor, + None, + ) + .await + .unwrap(); + + // Default case where GraffitiPolicy is AppendClientVersions + let default_path = self + .client + .get_validator_blocks_v4_path( + slot, + &randao_reveal, + graffiti.as_ref(), + SkipRandaoVerification::Yes, + None, builder_boost_factor, Some(GraffitiPolicy::AppendClientVersions), ) .await .unwrap(); - let query_append_path = append_path.query().unwrap_or(""); - // When GraffitiPolicy is AppendClientVersions, the HTTP API query path should contain "graffiti_policy" + let query_none_path = no_graffiti_policy_path.query().unwrap_or(""); + let query_default_path = default_path.query().unwrap_or(""); + // When GraffitiPolicy is AppendClientVersions (default GraffitiPolicy), the HTTP API query path should not contain "graffiti_policy" assert!( - query_append_path.contains("graffiti_policy"), - "URL should contain graffiti_policy=AppendClientVersions parameter. URL is: {}", - query_append_path + !query_default_path.contains("graffiti_policy"), + "URL should not contain graffiti_policy parameter (same as PreserveUserGraffiti). URL is: {}", + query_default_path ); + // The HTTP API query path for GraffiliPolicy is None should be the same as the default (GraffitiPolicy = AppendClientVersions) + assert_eq!(query_none_path, query_default_path); + + let preserve_path = self + .client + .get_validator_blocks_v4_path( + slot, + &randao_reveal, + graffiti.as_ref(), + SkipRandaoVerification::Yes, + None, + builder_boost_factor, + Some(GraffitiPolicy::PreserveUserGraffiti), + ) + .await + .unwrap(); + + let query_preserve_path = preserve_path.query().unwrap_or(""); + // When GraffitiPolicy is set to PreserveUserGraffiti, the HTTP API query path should contain "graffiti_policy" + assert!( + query_preserve_path.contains("graffiti_policy"), + "URL should not contain graffiti_policy parameter when using PreserveUserGraffiti. URL is: {}", + query_preserve_path + ); + self } } @@ -9531,10 +9598,12 @@ async fn get_beacon_rewards_attestations_fulu() { } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn get_validator_blocks_v3_http_api_path() { +async fn get_validator_blocks_http_api_path() { ApiTester::new() .await .get_validator_blocks_v3_path_graffiti_policy() + .await + .get_validator_blocks_v4_path_graffiti_policy() .await; } diff --git a/book/src/help_vc.md b/book/src/help_vc.md index 4647780ea8..f1a342197c 100644 --- a/book/src/help_vc.md +++ b/book/src/help_vc.md @@ -227,10 +227,10 @@ Flags: automatically enabled for <= 64 validators. Enabling this metric for higher validator counts will lead to higher volume of prometheus metrics being collected. - --graffiti-append - When used, client version info will be prepended to user custom - graffiti, with a space in between. This should only be used with a - Lighthouse beacon node. + --graffiti-append [] + Client version info will be appended to user custom graffiti, with a + space in between. This should only be set to false when using a + Lighthouse beacon node. [default: true] [possible values: true, false] -h, --help Prints help information --http diff --git a/book/src/validator_graffiti.md b/book/src/validator_graffiti.md index 9908d056da..801d73d676 100644 --- a/book/src/validator_graffiti.md +++ b/book/src/validator_graffiti.md @@ -60,7 +60,7 @@ Usage: `lighthouse vc --graffiti example` ## 4. Using the "--graffiti" flag on the beacon node -Users can also specify a common graffiti using the `--graffiti` flag on the beacon node as a common graffiti for all validators. +Users can also specify a common graffiti using the `--graffiti` flag on the beacon node as a common graffiti for all validators. Usage: `lighthouse bn --graffiti fortytwo` @@ -93,3 +93,39 @@ curl -X PATCH "http://localhost:5062/lighthouse/validators/0xb0148e6348264131bf4 ``` A `null` response indicates that the request is successful. + +## Automatically append client version info to user graffiti + +> Note: this feature only works when a Lighthouse validator client is connected to a Lighthouse beacon node. + +In the interest of obtaining client diversity data, Lighthouse will by default automatically append client version info +to user graffiti in proposed blocks. + +For example, you set the graffiti in the validator client as `This is my graffiti`. You are using Lighthouse (`LH`) v8.1.3 +with commit hash `176cce5` and Reth (`RH`) v2.2.0 with commit hash `88505c7`. The appended graffiti will include: + +- Execution layer client code +- First two bytes of the execution layer commit hash +- Consensus layer client code +- First two bytes of the consensus layer commit hash + +When the user graffiti is less than 20 characters, as in the above example, the appended graffiti when proposing a block +will be: `This is my graffiti RH8850LH176c`. + +Given that the total size of the graffiti is 32 bytes, if the appended graffiti exceeds the size, the appended +client version info will automatically be shortened. Some examples are as follows, where the last part of the graffiti is the +appended client version info. + +When the user graffiti is between 20-23 characters: +`This is my graffiti yo RH88LH17` + +When the user graffiti is between 24-27 characters: +`This is my graffiti string RHLH` + +When the user graffiti is between 28-29 characters: +`This is my graffiti string yo RH` + +When the user graffiti is between 30-32 characters, no client version info will be appended: +`This is my graffiti string yo yo` + +To opt out from this, when using a Lighthouse beacon node, use the flag `--graffiti-append false` on the validator client. This will retain your own graffiti when proposing a block, without appending any client version info. diff --git a/common/eth2/src/lib.rs b/common/eth2/src/lib.rs index 572f9522ee..46f553542a 100644 --- a/common/eth2/src/lib.rs +++ b/common/eth2/src/lib.rs @@ -2393,12 +2393,12 @@ impl BeaconNodeHttpClient { .append_pair("builder_boost_factor", &builder_booster_factor.to_string()); } - // Only append the HTTP URL request if the graffiti_policy is to AppendClientVersions - // If PreserveUserGraffiti (default), then the HTTP URL request does not contain graffiti_policy + // Only append the HTTP URL request if the graffiti_policy is PreserveUserGraffiti + // If AppendClientVersions (default), then we do not modify the HTTP URL request // so that the default case is compliant to the spec - if let Some(GraffitiPolicy::AppendClientVersions) = graffiti_policy { + if let Some(GraffitiPolicy::PreserveUserGraffiti) = graffiti_policy { path.query_pairs_mut() - .append_pair("graffiti_policy", "AppendClientVersions"); + .append_pair("graffiti_policy", "PreserveUserGraffiti"); } Ok(path) @@ -2600,9 +2600,12 @@ impl BeaconNodeHttpClient { .append_pair("builder_boost_factor", &builder_booster_factor.to_string()); } - if let Some(GraffitiPolicy::AppendClientVersions) = graffiti_policy { + // Only append the HTTP URL request if the graffiti_policy is PreserveUserGraffiti + // If AppendClientVersions (default), then we do not modify the HTTP URL request + // so that the default case is compliant to the spec + if let Some(GraffitiPolicy::PreserveUserGraffiti) = graffiti_policy { path.query_pairs_mut() - .append_pair("graffiti_policy", "AppendClientVersions"); + .append_pair("graffiti_policy", "PreserveUserGraffiti"); } Ok(path) diff --git a/common/eth2/src/types.rs b/common/eth2/src/types.rs index 449ea88685..049ffba657 100644 --- a/common/eth2/src/types.rs +++ b/common/eth2/src/types.rs @@ -758,10 +758,10 @@ pub struct ProposerData { pub slot: Slot, } -#[derive(Clone, Copy, Serialize, Deserialize, Default, Debug)] +#[derive(Clone, Copy, Serialize, Deserialize, Default, Debug, PartialEq)] pub enum GraffitiPolicy { - #[default] PreserveUserGraffiti, + #[default] AppendClientVersions, } diff --git a/lighthouse/tests/validator_client.rs b/lighthouse/tests/validator_client.rs index 6fd5a6538c..945e363ab5 100644 --- a/lighthouse/tests/validator_client.rs +++ b/lighthouse/tests/validator_client.rs @@ -2,6 +2,7 @@ use beacon_node_fallback::{ApiTopic, beacon_node_health::BeaconNodeSyncDistanceT use crate::exec::CommandLineTestExec; use bls::{Keypair, PublicKeyBytes}; +use eth2::types::GraffitiPolicy; use initialized_validators::DEFAULT_WEB3SIGNER_KEEP_ALIVE; use sensitive_url::SensitiveUrl; use std::fs::File; @@ -261,6 +262,58 @@ fn graffiti_file_with_pk_flag() { }); } +// Tests for graffiti-append flag +#[test] +fn graffiti_append_default() { + CommandLineTest::new().run().with_config(|config| { + assert_eq!( + config.graffiti_policy, + Some(GraffitiPolicy::AppendClientVersions) + ); + }); +} + +#[test] +fn graffiti_append_true_flag() { + CommandLineTest::new() + .flag("graffiti-append", Some("true")) + .run() + .with_config(|config| { + assert_eq!( + config.graffiti_policy, + Some(GraffitiPolicy::AppendClientVersions) + ); + }); +} + +#[test] +fn graffiti_append_false_flag() { + CommandLineTest::new() + .flag("graffiti-append", Some("false")) + .run() + .with_config(|config| { + assert_eq!( + config.graffiti_policy, + Some(GraffitiPolicy::PreserveUserGraffiti) + ); + }); +} + +// Retain previous behaviour: `--graffiti-append` with no value is the same as +// `--graffiti-append true`. +#[test] +fn graffiti_append_no_value() { + CommandLineTest::new() + .flag("graffiti-append", None) + .run() + .with_config(|config| { + assert_eq!( + config.graffiti_policy, + Some(GraffitiPolicy::AppendClientVersions) + ); + }); +} + // Tests for suggested-fee-recipient flags. #[test] fn fee_recipient_flag() { diff --git a/validator_client/src/cli.rs b/validator_client/src/cli.rs index 0eb0e9e5dd..e5fe1580da 100644 --- a/validator_client/src/cli.rs +++ b/validator_client/src/cli.rs @@ -152,13 +152,14 @@ pub struct ValidatorClient { #[clap( long, - requires = "graffiti", - help = "When used, client version info will be prepended to user custom graffiti, with a space in between. \ - This should only be used with a Lighthouse beacon node.", + num_args = 0..=1, + help = "Client version info will be appended to user custom graffiti, with a space in between. \ + This should only be set to false when using a Lighthouse beacon node.", display_order = 0, + default_value = "true", help_heading = FLAG_HEADER )] - pub graffiti_append: bool, + pub graffiti_append: Option, #[clap( long, diff --git a/validator_client/src/config.rs b/validator_client/src/config.rs index d68a78b705..418cd385da 100644 --- a/validator_client/src/config.rs +++ b/validator_client/src/config.rs @@ -239,7 +239,7 @@ impl Config { } } - config.graffiti_policy = if validator_client_config.graffiti_append { + config.graffiti_policy = if validator_client_config.graffiti_append.unwrap_or(true) { Some(GraffitiPolicy::AppendClientVersions) } else { Some(GraffitiPolicy::PreserveUserGraffiti) From 58e35bc96fab30e5f4085ac36f70db1f88ca5bb1 Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Mon, 15 Jun 2026 16:56:09 -0700 Subject: [PATCH 07/15] Gloas alpha spec 9 (#9393) Changes implemented Ensure bids are for a higher slot than their parent (https://github.com/ethereum/consensus-specs/pull/5302) Ignore PTC attestations for empty assigned slots (https://github.com/ethereum/consensus-specs/pull/5281) Limit should_build_on_full checks to the previous slot (https://github.com/ethereum/consensus-specs/pull/5309) Apply proposer boost if dependent roots match (https://github.com/ethereum/consensus-specs/pull/5306) Exclude slashed validators from proposing (EIP-8045) (https://github.com/ethereum/consensus-specs/pull/5115) Force the proposer to reorg late payloads (https://github.com/ethereum/consensus-specs/pull/5210) Remove support for old deposit mechanism in Fulu (https://github.com/ethereum/consensus-specs/pull/4704) Co-Authored-By: Eitan Seri-Levi Co-Authored-By: dapplion <35266934+dapplion@users.noreply.github.com> Co-Authored-By: Eitan Seri-Levi Co-Authored-By: Michael Sproul Co-Authored-By: Michael Sproul --- beacon_node/beacon_chain/src/beacon_chain.rs | 43 ------ .../src/block_production/gloas.rs | 2 +- .../gossip_verified_payload_attestation.rs | 16 ++- .../payload_attestation_verification/mod.rs | 12 ++ .../payload_attestation_verification/tests.rs | 96 +++++--------- .../gossip_verified_bid.rs | 12 +- .../src/payload_bid_verification/mod.rs | 2 + .../src/payload_bid_verification/tests.rs | 62 ++++++--- .../payload_envelope_verification/import.rs | 3 - .../beacon_chain/tests/op_verification.rs | 61 +++++++++ .../tests/payload_invalidation.rs | 1 - .../src/beacon/execution_payload_envelopes.rs | 46 ++++--- beacon_node/http_api/src/block_id.rs | 5 - .../gossip_methods.rs | 31 +++-- .../network_beacon_processor/sync_methods.rs | 4 + beacon_node/operation_pool/src/lib.rs | 103 ++++++++++++++- consensus/fork_choice/src/fork_choice.rs | 67 +++++++--- consensus/fork_choice/tests/tests.rs | 2 - .../src/fork_choice_test_definition.rs | 33 +++++ .../gloas_payload.rs | 90 +++++++++++++ consensus/proto_array/src/proto_array.rs | 18 ++- .../src/proto_array_fork_choice.rs | 3 +- .../process_operations.rs | 7 +- .../src/per_epoch_processing/single_pass.rs | 6 +- consensus/types/src/state/beacon_state.rs | 52 +++++--- consensus/types/src/state/slashings_cache.rs | 124 ++++++++++++++++++ testing/ef_tests/Makefile | 2 +- testing/ef_tests/check_all_files_accessed.py | 2 + .../ef_tests/src/cases/epoch_processing.rs | 3 + testing/ef_tests/src/cases/fork_choice.rs | 81 +++++++++--- testing/ef_tests/src/cases/operations.rs | 7 +- testing/ef_tests/src/handler.rs | 12 +- testing/ef_tests/tests/tests.rs | 24 ++-- 33 files changed, 785 insertions(+), 247 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index a7f1a7cfcd..5a521d18e6 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -4248,12 +4248,6 @@ impl BeaconChain { let cached_head = self.canonical_head.cached_head(); let old_head_slot = cached_head.head_slot(); - // Compute the expected proposer for `current_slot` on the canonical chain. This is used by - // `on_block` to gate proposer boost on the block's proposer matching the canonical proposer - // (per spec `update_proposer_boost_root` added in v1.7.0-alpha.5). - let canonical_head_proposer_index = - self.canonical_head_proposer_index(current_slot, &cached_head)?; - // Take an upgradable read lock on fork choice so we can check if this block has already // been imported. We don't want to repeat work importing a block that is already imported. let fork_choice_reader = self.canonical_head.fork_choice_upgradable_read_lock(); @@ -4285,7 +4279,6 @@ impl BeaconChain { block_delay, &state, payload_verification_status, - canonical_head_proposer_index, &self.spec, ) .map_err(|e| BlockError::BeaconChainError(Box::new(e.into())))?; @@ -5033,42 +5026,6 @@ impl BeaconChain { })) } - /// Compute the expected beacon proposer for `slot` on the canonical chain extending `cached_head`. - /// - /// Uses the beacon proposer cache to avoid recomputing the shuffling on every block import. - /// - /// This is used by `update_proposer_boost_root` to gate proposer boost on the block's proposer - /// matching the canonical proposer, per consensus-specs v1.7.0-alpha.5. - /// - /// This function should never error unless there is some corruption of the head state. If a - /// state advance is needed, it will be handled by the proposer cache. - pub fn canonical_head_proposer_index( - &self, - slot: Slot, - cached_head: &CachedHead, - ) -> Result { - let proposal_epoch = slot.epoch(T::EthSpec::slots_per_epoch()); - let head_block_root = cached_head.head_block_root(); - let head_state = &cached_head.snapshot.beacon_state; - - let shuffling_decision_root = head_state.proposer_shuffling_decision_root_at_epoch( - proposal_epoch, - head_block_root, - &self.spec, - )?; - - self.with_proposer_cache::<_, Error>( - shuffling_decision_root, - proposal_epoch, - |proposers| { - proposers - .get_slot::(slot) - .map(|p| p.index as u64) - }, - || Ok((cached_head.head_state_root(), head_state.clone())), - ) - } - pub fn get_expected_withdrawals( &self, forkchoice_update_params: &ForkchoiceUpdateParameters, diff --git a/beacon_node/beacon_chain/src/block_production/gloas.rs b/beacon_node/beacon_chain/src/block_production/gloas.rs index 82dad6f6ad..90fc60524a 100644 --- a/beacon_node/beacon_chain/src/block_production/gloas.rs +++ b/beacon_node/beacon_chain/src/block_production/gloas.rs @@ -163,7 +163,7 @@ impl BeaconChain { let should_build_on_full = self .canonical_head .fork_choice_read_lock() - .should_build_on_full(&parent_root, parent_payload_status) + .should_build_on_full(&parent_root, parent_payload_status, produce_at_slot) .map_err(|e| { BlockProductionError::BeaconChain(Box::new(BeaconChainError::ForkChoiceError(e))) })?; diff --git a/beacon_node/beacon_chain/src/payload_attestation_verification/gossip_verified_payload_attestation.rs b/beacon_node/beacon_chain/src/payload_attestation_verification/gossip_verified_payload_attestation.rs index 3e9f9e4b60..f8a1143b2e 100644 --- a/beacon_node/beacon_chain/src/payload_attestation_verification/gossip_verified_payload_attestation.rs +++ b/beacon_node/beacon_chain/src/payload_attestation_verification/gossip_verified_payload_attestation.rs @@ -70,13 +70,21 @@ impl VerifiedPayloadAttestationMessage { // 2. Blocks we've seen that are invalid (REJECT). // Presently both cases return IGNORE. let beacon_block_root = payload_attestation_message.data.beacon_block_root; - if ctx + let block = ctx .canonical_head .fork_choice_read_lock() .get_block(&beacon_block_root) - .is_none() - { - return Err(Error::UnknownHeadBlock { beacon_block_root }); + .ok_or(Error::UnknownHeadBlock { beacon_block_root })?; + + // [IGNORE] The block referenced by `data.beacon_block_root` is at slot `data.slot`, i.e. + // the block has `block.slot == data.slot`. A PTC member assigned to an empty slot must not + // attest, so ignore messages that reference an earlier block. + if block.slot != slot { + return Err(Error::BlockNotAtSlot { + beacon_block_root, + block_slot: block.slot, + data_slot: slot, + }); } let message_epoch = slot.epoch(T::EthSpec::slots_per_epoch()); diff --git a/beacon_node/beacon_chain/src/payload_attestation_verification/mod.rs b/beacon_node/beacon_chain/src/payload_attestation_verification/mod.rs index 89ae1bbbdd..d1fa7f52fb 100644 --- a/beacon_node/beacon_chain/src/payload_attestation_verification/mod.rs +++ b/beacon_node/beacon_chain/src/payload_attestation_verification/mod.rs @@ -60,6 +60,18 @@ pub enum Error { /// The attestation points to a block we have not yet imported. It's unclear if the /// attestation is valid or not. UnknownHeadBlock { beacon_block_root: Hash256 }, + /// The block referenced by `data.beacon_block_root` is not at slot `data.slot`, i.e. the + /// PTC member's assigned slot was likely empty. + /// + /// ## Peer scoring + /// + /// PTC members should not attest for empty slots, so we + /// ignore the message. + BlockNotAtSlot { + beacon_block_root: Hash256, + block_slot: Slot, + data_slot: Slot, + }, /// The validator index is not a member of the PTC for this slot. /// /// ## Peer scoring diff --git a/beacon_node/beacon_chain/src/payload_attestation_verification/tests.rs b/beacon_node/beacon_chain/src/payload_attestation_verification/tests.rs index d4b82c41fc..6c52e5ce2d 100644 --- a/beacon_node/beacon_chain/src/payload_attestation_verification/tests.rs +++ b/beacon_node/beacon_chain/src/payload_attestation_verification/tests.rs @@ -3,7 +3,6 @@ use std::time::Duration; use bls::Signature; use slot_clock::{SlotClock, TestingSlotClock}; -use state_processing::AllCaches; use types::{ Domain, Epoch, EthSpec, ForkName, Hash256, MinimalEthSpec, PayloadAttestationData, PayloadAttestationMessage, SignedRoot, Slot, @@ -167,6 +166,25 @@ fn unknown_head_block() { ); } +#[test] +fn block_not_at_slot() { + if !fork_name_from_env().is_some_and(|f| f.gloas_enabled()) { + return; + } + let ctx = TestContext::new(); + let gossip = ctx.gossip_ctx(); + + // The genesis block is at slot 0, but the message claims slot 1. A PTC member assigned to an + // empty slot must not attest, so this must be ignored (per consensus-specs #5281). + let msg = make_payload_attestation(Slot::new(1), 0, ctx.genesis_block_root); + let result = VerifiedPayloadAttestationMessage::new(msg, &gossip); + assert!( + matches!(result, Err(PayloadAttestationError::BlockNotAtSlot { .. })), + "expected BlockNotAtSlot, got: {:?}", + result + ); +} + #[test] fn not_in_ptc() { if !fork_name_from_env().is_some_and(|f| f.gloas_enabled()) { @@ -174,7 +192,7 @@ fn not_in_ptc() { } let ctx = TestContext::new(); let gossip = ctx.gossip_ctx(); - let slot = Slot::new(1); + let slot = Slot::new(0); let ptc_members = ctx.ptc_members(slot); let non_ptc_validator = (0..NUM_VALIDATORS as u64) @@ -196,7 +214,7 @@ fn invalid_signature() { } let ctx = TestContext::new(); let gossip = ctx.gossip_ctx(); - let slot = Slot::new(1); + let slot = Slot::new(0); let ptc_members = ctx.ptc_members(slot); let validator_index = ptc_members[0] as u64; @@ -216,7 +234,7 @@ fn valid_payload_attestation() { } let ctx = TestContext::new(); let gossip = ctx.gossip_ctx(); - let slot = Slot::new(1); + let slot = Slot::new(0); let ptc_members = ctx.ptc_members(slot); let validator_index = ptc_members[0] as u64; @@ -243,7 +261,7 @@ fn duplicate_after_valid() { } let ctx = TestContext::new(); let gossip = ctx.gossip_ctx(); - let slot = Slot::new(1); + let slot = Slot::new(0); let ptc_members = ctx.ptc_members(slot); let validator_index = ptc_members[0] as u64; @@ -300,10 +318,8 @@ async fn ptc_cache_is_primed_at_gloas_fork_boundary() { .mock_execution_layer() .build(); - harness.extend_to_slot(fork_boundary_slot).await; - for slot in test_slots { - harness.chain.slot_clock.set_slot(slot.as_u64()); + harness.extend_to_slot(slot).await; assert!( harness .chain @@ -350,10 +366,9 @@ async fn ptc_cache_is_primed_at_gloas_fork_boundary() { } } -/// Exercises payload attestation gossip verification when the message epoch is ahead of the -/// canonical head due to many missed slots. +/// Check that a payload attestation whose assigned slot is empty is ignored. #[tokio::test] -async fn stale_head_payload_attestation() { +async fn stale_head_empty_slot_payload_attestation_ignored() { if !fork_name_from_env().is_some_and(|f| f.gloas_enabled()) { return; } @@ -363,9 +378,9 @@ async fn stale_head_payload_attestation() { let head_slot = Slot::new(slots_per_epoch); let missed_epochs = 4; let target_slot = Slot::new(slots_per_epoch * (1 + missed_epochs)); - let target_epoch = target_slot.epoch(slots_per_epoch); - // GIVEN a chain with blocks through epoch 1 (so the store has states). + // Given a chain with blocks through epoch 1, then a slot clock advanced 4 epochs without + // producing blocks (simulating missed slots). let harness = BeaconChainHarness::builder(E::default()) .default_spec() .deterministic_keypairs(64) @@ -373,71 +388,30 @@ async fn stale_head_payload_attestation() { .mock_execution_layer() .build(); harness.extend_to_slot(head_slot).await; - - let head = harness.chain.canonical_head.cached_head(); - let head_epoch = head.snapshot.beacon_state.current_epoch(); - assert!( - target_epoch > head_epoch + harness.spec.min_seed_lookahead, - "precondition: message epoch must exceed head + min_seed_lookahead" - ); - - // GIVEN a slot clock advanced to epoch 5 without producing blocks - // (simulating missed slots during a liveness failure). harness.chain.slot_clock.set_slot(target_slot.as_u64()); - // Advance a reference state to compute the PTC at the target slot. - let mut reference_state = head.snapshot.beacon_state.clone(); - state_processing::state_advance::partial_state_advance( - &mut reference_state, - Some(head.snapshot.beacon_state_root()), - target_slot, - &harness.spec, - ) - .expect("should advance reference state"); - reference_state - .build_all_caches(&harness.spec) - .expect("should build caches"); + let head = harness.chain.canonical_head.cached_head(); - let ptc = reference_state - .get_ptc(target_slot, &harness.spec) - .expect("should get PTC from reference state"); - let validator_index = *ptc.0.first().expect("PTC should have at least one member") as u64; - - // WHEN a properly-signed payload attestation from a PTC member is verified. The signature - // domain should come from the spec fork schedule and genesis validators root, not a loaded - // state in the verifier. - let domain = harness.spec.get_domain( - target_epoch, - Domain::PTCAttester, - &reference_state.fork(), - reference_state.genesis_validators_root(), - ); + // When a payload attestation for empty target slot references a stale block root + // it is ignored because target_slot != block.slot let data = PayloadAttestationData { beacon_block_root: head.head_block_root(), slot: target_slot, payload_present: true, blob_data_available: true, }; - let message = data.signing_root(domain); - let signature = harness.validator_keypairs[validator_index as usize] - .sk - .sign(message); let msg = PayloadAttestationMessage { - validator_index, + validator_index: 0, data, - signature, + signature: Signature::empty(), }; - // THEN verification succeeds despite the head being 4 epochs stale. let result = harness .chain .verify_payload_attestation_message_for_gossip(msg); assert!( - result.is_ok(), - "expected Ok (head epoch {}, message epoch {}), got: {:?}", - head_epoch, - target_epoch, - result.unwrap_err() + matches!(result, Err(PayloadAttestationError::BlockNotAtSlot { .. })), + "expected BlockNotAtSlot, got: {result:?}" ); } diff --git a/beacon_node/beacon_chain/src/payload_bid_verification/gossip_verified_bid.rs b/beacon_node/beacon_chain/src/payload_bid_verification/gossip_verified_bid.rs index 354705b92c..1687760d25 100644 --- a/beacon_node/beacon_chain/src/payload_bid_verification/gossip_verified_bid.rs +++ b/beacon_node/beacon_chain/src/payload_bid_verification/gossip_verified_bid.rs @@ -144,9 +144,17 @@ impl GossipVerifiedPayloadBid { let fork_choice = ctx.canonical_head.fork_choice_read_lock(); // TODO(gloas) reprocess bids whose parent_block_root becomes known & canonical after a reorg? - if !fork_choice.contains_block(&bid_parent_block_root) { - return Err(PayloadBidError::ParentBlockRootUnknown { + let parent_block = fork_choice.get_block(&bid_parent_block_root).ok_or( + PayloadBidError::ParentBlockRootUnknown { parent_block_root: bid_parent_block_root, + }, + )?; + + // [REJECT] The bid is for a higher slot than its parent block. + if bid_slot <= parent_block.slot { + return Err(PayloadBidError::BidNotDescendantOfParent { + bid_slot, + parent_slot: parent_block.slot, }); } diff --git a/beacon_node/beacon_chain/src/payload_bid_verification/mod.rs b/beacon_node/beacon_chain/src/payload_bid_verification/mod.rs index a40fd14872..e23c537e18 100644 --- a/beacon_node/beacon_chain/src/payload_bid_verification/mod.rs +++ b/beacon_node/beacon_chain/src/payload_bid_verification/mod.rs @@ -37,6 +37,8 @@ pub enum PayloadBidError { }, /// The bids slot is not the current slot or the next slot. InvalidBidSlot { bid_slot: Slot }, + /// The bid's slot is not greater than the slot of its parent block. + BidNotDescendantOfParent { bid_slot: Slot, parent_slot: Slot }, /// The slot clock cannot be read. UnableToReadSlot, /// No proposer preferences for the current slot. diff --git a/beacon_node/beacon_chain/src/payload_bid_verification/tests.rs b/beacon_node/beacon_chain/src/payload_bid_verification/tests.rs index ccdf64d41d..04eb875bd9 100644 --- a/beacon_node/beacon_chain/src/payload_bid_verification/tests.rs +++ b/beacon_node/beacon_chain/src/payload_bid_verification/tests.rs @@ -310,7 +310,7 @@ fn builder_already_seen_for_slot() { } let ctx = TestContext::new(); let gossip = ctx.gossip_ctx(); - let slot = Slot::new(0); + let slot = Slot::new(1); seed_preferences(&ctx, slot, Address::ZERO, 30_000_000); let bid = make_signed_bid(slot, 42, Address::ZERO, 30_000_000, 100, Hash256::ZERO); @@ -336,7 +336,7 @@ fn bid_value_below_cached() { } let ctx = TestContext::new(); let gossip = ctx.gossip_ctx(); - let slot = Slot::new(0); + let slot = Slot::new(1); seed_preferences(&ctx, slot, Address::ZERO, 30_000_000); let high_bid = GossipVerifiedPayloadBid { @@ -384,7 +384,7 @@ fn fee_recipient_mismatch() { } let ctx = TestContext::new(); let gossip = ctx.gossip_ctx(); - let slot = Slot::new(0); + let slot = Slot::new(1); seed_preferences(&ctx, slot, Address::repeat_byte(0xaa), 30_000_000); let bid = make_signed_bid( @@ -406,7 +406,7 @@ fn gas_limit_mismatch() { } let ctx = TestContext::new(); let gossip = ctx.gossip_ctx(); - let slot = Slot::new(0); + let slot = Slot::new(1); seed_preferences(&ctx, slot, Address::ZERO, 30_000_000); let bid = make_signed_bid( @@ -428,7 +428,7 @@ fn execution_payment_nonzero() { } let ctx = TestContext::new(); let gossip = ctx.gossip_ctx(); - let slot = Slot::new(0); + let slot = Slot::new(1); seed_preferences(&ctx, slot, Address::ZERO, 30_000_000); let bid = Arc::new(SignedExecutionPayloadBid { @@ -455,7 +455,7 @@ fn unknown_builder_index() { } let ctx = TestContext::new(); let gossip = ctx.gossip_ctx(); - let slot = Slot::new(0); + let slot = Slot::new(1); seed_preferences(&ctx, slot, Address::ZERO, 30_000_000); // Use a builder_index that doesn't exist in the registry. @@ -483,7 +483,7 @@ fn inactive_builder() { } let ctx = TestContext::new(); let gossip = ctx.gossip_ctx(); - let slot = Slot::new(0); + let slot = Slot::new(1); seed_preferences(&ctx, slot, Address::ZERO, 30_000_000); let bid = make_signed_bid( @@ -508,7 +508,7 @@ fn builder_cant_cover_bid() { } let ctx = TestContext::new(); let gossip = ctx.gossip_ctx(); - let slot = Slot::new(0); + let slot = Slot::new(1); seed_preferences(&ctx, slot, Address::ZERO, 30_000_000); // Builder index 0 exists but bid value far exceeds their balance. @@ -534,7 +534,7 @@ fn parent_block_root_unknown() { } let ctx = TestContext::new(); let gossip = ctx.gossip_ctx(); - let slot = Slot::new(0); + let slot = Slot::new(1); seed_preferences(&ctx, slot, Address::ZERO, 30_000_000); // Parent block root not in fork choice. @@ -556,7 +556,9 @@ fn parent_block_root_not_canonical() { } let ctx = TestContext::new(); let gossip = ctx.gossip_ctx(); - let slot = Slot::new(0); + // The non-canonical fork block is at slot 1, so use slot 2 to satisfy the `bid.slot > parent + // block slot` rule and exercise the bid descendant from parent check specifically. + let slot = Slot::new(2); seed_preferences(&ctx, slot, Address::ZERO, 30_000_000); let fork_root = ctx.insert_non_canonical_block(); @@ -570,6 +572,36 @@ fn parent_block_root_not_canonical() { ); } +#[test] +fn bid_slot_not_after_parent() { + if !fork_name_from_env().is_some_and(|f| f.gloas_enabled()) { + return; + } + let ctx = TestContext::new(); + let gossip = ctx.gossip_ctx(); + // The genesis (parent) block is at slot 0, so a bid at slot 0 is not for a higher slot than + // its parent and must be rejected. + let slot = Slot::new(0); + seed_preferences(&ctx, slot, Address::ZERO, 30_000_000); + + let bid = make_signed_bid( + slot, + 0, + Address::ZERO, + 30_000_000, + 0, + ctx.genesis_block_root, + ); + let result = GossipVerifiedPayloadBid::new(bid, &gossip); + assert!( + matches!( + result, + Err(PayloadBidError::BidNotDescendantOfParent { .. }) + ), + "expected BidNotDescendantOfParent, got: {result:?}" + ); +} + #[test] fn invalid_blob_kzg_commitments() { if !fork_name_from_env().is_some_and(|f| f.gloas_enabled()) { @@ -577,7 +609,7 @@ fn invalid_blob_kzg_commitments() { } let ctx = TestContext::new(); let gossip = ctx.gossip_ctx(); - let slot = Slot::new(0); + let slot = Slot::new(1); seed_preferences(&ctx, slot, Address::ZERO, 30_000_000); let max_blobs = ctx @@ -614,7 +646,7 @@ fn bad_signature() { } let ctx = TestContext::new(); let gossip = ctx.gossip_ctx(); - let slot = Slot::new(0); + let slot = Slot::new(1); seed_preferences(&ctx, slot, Address::ZERO, 30_000_000); // All checks pass but signature is empty/invalid. @@ -643,7 +675,7 @@ fn valid_bid() { } let ctx = TestContext::new(); let gossip = ctx.gossip_ctx(); - let slot = Slot::new(0); + let slot = Slot::new(1); seed_preferences(&ctx, slot, Address::ZERO, 30_000_000); let bid = ctx.sign_bid(ExecutionPayloadBid { @@ -670,7 +702,7 @@ fn two_builders_coexist_in_cache() { } let ctx = TestContext::new(); let gossip = ctx.gossip_ctx(); - let slot = Slot::new(0); + let slot = Slot::new(1); seed_preferences(&ctx, slot, Address::ZERO, 30_000_000); let bid_0 = ctx.sign_bid(ExecutionPayloadBid { @@ -725,7 +757,7 @@ fn bid_equal_to_cached_value_rejected() { } let ctx = TestContext::new(); let gossip = ctx.gossip_ctx(); - let slot = Slot::new(0); + let slot = Slot::new(1); seed_preferences(&ctx, slot, Address::ZERO, 30_000_000); // Seed a cached bid with value 100. diff --git a/beacon_node/beacon_chain/src/payload_envelope_verification/import.rs b/beacon_node/beacon_chain/src/payload_envelope_verification/import.rs index 00806f0e17..90cdb4fe97 100644 --- a/beacon_node/beacon_chain/src/payload_envelope_verification/import.rs +++ b/beacon_node/beacon_chain/src/payload_envelope_verification/import.rs @@ -117,9 +117,6 @@ impl BeaconChain { ); // TODO(gloas) do we need to send a `PayloadImported` event to the reprocess queue? - // TODO(gloas) do we need to recompute head? - // should canonical_head return the block and the payload now? - self.recompute_head_at_current_slot().await; metrics::inc_counter(&metrics::ENVELOPE_PROCESSING_SUCCESSES); diff --git a/beacon_node/beacon_chain/tests/op_verification.rs b/beacon_node/beacon_chain/tests/op_verification.rs index adc14541a9..d1df72234f 100644 --- a/beacon_node/beacon_chain/tests/op_verification.rs +++ b/beacon_node/beacon_chain/tests/op_verification.rs @@ -299,6 +299,67 @@ async fn proposer_slashing_duplicate_in_state() { )); } +#[tokio::test] +async fn slashings_cache_matches_state_after_block_import() { + let db_path = tempdir().unwrap(); + let store = get_store(&db_path); + let harness = get_harness(store.clone(), VALIDATOR_COUNT); + + // Slash a spread of validators by importing proposer slashings into the op pool, exactly as + // they would arrive over gossip. + let slashed_validators = [0u64, 7, VALIDATOR_COUNT as u64 - 1]; + for &validator_index in &slashed_validators { + let slashing = harness.make_proposer_slashing(validator_index); + let ObservationOutcome::New(verified_slashing) = harness + .chain + .verify_proposer_slashing_for_gossip(slashing) + .unwrap() + else { + panic!("slashing should verify"); + }; + harness.chain.import_proposer_slashing(verified_slashing); + } + + // Produce and import a block that includes the slashings. This drives the production flow: + // `per_block_processing` -> `slash_validator` -> `SlashingsCache::record_validator_slashing`. + harness + .extend_chain( + 1, + BlockStrategy::OnCanonicalHead, + AttestationStrategy::AllValidators, + ) + .await; + + let state = harness.get_current_state(); + + // The block processing above should have left the slashings cache initialized for the head. + assert!( + state.slashings_cache_is_initialized(), + "slashings cache should be initialized after block import" + ); + + // The targeted validators must actually be slashed in the state (i.e. the slashings were + // included and applied, not silently dropped). + for &validator_index in &slashed_validators { + assert!( + state + .get_validator(validator_index as usize) + .unwrap() + .slashed, + "validator {validator_index} should be slashed in the state" + ); + } + + // The cache must agree with the `slashed` flag of *every* validator in the state. + for index in 0..state.validators().len() { + assert_eq!( + state.slashings_cache().is_slashed(index), + state.get_validator(index).unwrap().slashed, + "slashings cache disagrees with state at validator {index}" + ); + } +} + #[test] fn attester_slashing() { let db_path = tempdir().unwrap(); diff --git a/beacon_node/beacon_chain/tests/payload_invalidation.rs b/beacon_node/beacon_chain/tests/payload_invalidation.rs index 42a78d740f..c89be0f5dd 100644 --- a/beacon_node/beacon_chain/tests/payload_invalidation.rs +++ b/beacon_node/beacon_chain/tests/payload_invalidation.rs @@ -1077,7 +1077,6 @@ async fn invalid_parent() { Duration::from_secs(0), &state, PayloadVerificationStatus::Optimistic, - block.message().proposer_index(), &rig.harness.chain.spec, ), Err(ForkChoiceError::ProtoArrayStringError(message)) diff --git a/beacon_node/http_api/src/beacon/execution_payload_envelopes.rs b/beacon_node/http_api/src/beacon/execution_payload_envelopes.rs index f8ab8cddc8..b6b681e091 100644 --- a/beacon_node/http_api/src/beacon/execution_payload_envelopes.rs +++ b/beacon_node/http_api/src/beacon/execution_payload_envelopes.rs @@ -8,7 +8,9 @@ use crate::version::{ }; use beacon_chain::data_column_verification::{GossipDataColumnError, GossipVerifiedDataColumn}; use beacon_chain::payload_envelope_verification::EnvelopeError; -use beacon_chain::{BeaconChain, BeaconChainTypes, NotifyExecutionLayer}; +use beacon_chain::{ + AvailabilityProcessingStatus, BeaconChain, BeaconChainTypes, NotifyExecutionLayer, +}; use bytes::Bytes; use eth2::types as api_types; use lighthouse_network::PubsubMessage; @@ -160,12 +162,16 @@ pub async fn publish_execution_payload_envelope( ) .await; - if let Err(e) = import_result { - warn!(%slot, error = ?e, "Failed to import execution payload envelope"); - return Err(warp_utils::reject::custom_server_error(format!( - "envelope import failed: {e}" - ))); - } + let mut envelope_imported = match &import_result { + Ok(AvailabilityProcessingStatus::Imported(_)) => true, + Ok(AvailabilityProcessingStatus::MissingComponents(_, _)) => false, + Err(e) => { + warn!(%slot, error = ?e, "Failed to import execution payload envelope"); + return Err(warp_utils::reject::custom_server_error(format!( + "envelope import failed: {e}" + ))); + } + }; // From here on the envelope is on the wire. `take_blobs` already consumed the cache // entry, so a retry would not republish columns; returning Err would mislead the @@ -201,19 +207,27 @@ pub async fn publish_execution_payload_envelope( .collect::>(); // Local processing only — envelope already broadcast, so log and fall through. - if !sampling_columns.is_empty() - && let Err(e) = - Box::pin(chain.process_gossip_data_columns(sampling_columns, || Ok(()))).await - { - error!( - %slot, - error = ?e, - "Failed to process sampling data columns during envelope publication" - ); + if !sampling_columns.is_empty() { + match Box::pin(chain.process_gossip_data_columns(sampling_columns, || Ok(()))).await + { + Ok(AvailabilityProcessingStatus::Imported(_)) => envelope_imported = true, + Ok(AvailabilityProcessingStatus::MissingComponents(_, _)) => {} + Err(e) => { + error!( + %slot, + error = ?e, + "Failed to process sampling data columns during envelope publication" + ); + } + } } } } + if envelope_imported { + chain.recompute_head_at_current_slot().await; + } + Ok(warp::reply().into_response()) } diff --git a/beacon_node/http_api/src/block_id.rs b/beacon_node/http_api/src/block_id.rs index dce4713245..50d5c8d165 100644 --- a/beacon_node/http_api/src/block_id.rs +++ b/beacon_node/http_api/src/block_id.rs @@ -621,10 +621,6 @@ mod tests { .unwrap(); let current_slot = harness.get_current_slot(); - let cached_head = chain.canonical_head.cached_head(); - let canonical_head_proposer_index = chain - .canonical_head_proposer_index(current_slot, &cached_head) - .unwrap(); chain .canonical_head @@ -636,7 +632,6 @@ mod tests { Duration::ZERO, &post_state, PayloadVerificationStatus::Verified, - canonical_head_proposer_index, &chain.spec, ) .unwrap(); diff --git a/beacon_node/network/src/network_beacon_processor/gossip_methods.rs b/beacon_node/network/src/network_beacon_processor/gossip_methods.rs index 98c143eaeb..b52732000e 100644 --- a/beacon_node/network/src/network_beacon_processor/gossip_methods.rs +++ b/beacon_node/network/src/network_beacon_processor/gossip_methods.rs @@ -3795,13 +3795,19 @@ impl NetworkBeaconProcessor { // TODO(gloas) metrics // register_process_result_metrics(&result, metrics::BlockSource::Gossip, "envelope"); - if let Err(e) = &result { - debug!( - ?beacon_block_root, - %peer_id, - error = ?e, - "Execution payload envelope processing failed" - ); + match &result { + Ok(AvailabilityProcessingStatus::Imported(_)) => { + self.chain.recompute_head_at_current_slot().await; + } + Ok(AvailabilityProcessingStatus::MissingComponents(_, _)) => {} + Err(e) => { + debug!( + ?beacon_block_root, + %peer_id, + error = ?e, + "Execution payload envelope processing failed" + ); + } } } @@ -3829,7 +3835,8 @@ impl NetworkBeaconProcessor { | PayloadBidError::InvalidBuilder { .. } | PayloadBidError::InvalidFeeRecipient | PayloadBidError::ExecutionPaymentNonZero { .. } - | PayloadBidError::InvalidBlobKzgCommitments { .. }, + | PayloadBidError::InvalidBlobKzgCommitments { .. } + | PayloadBidError::BidNotDescendantOfParent { .. }, ) => { self.propagate_validation_result(message_id, peer_id, MessageAcceptance::Reject); self.gossip_penalize_peer( @@ -4008,6 +4015,14 @@ impl NetworkBeaconProcessor { *beacon_block_root, )) } + PayloadAttestationError::BlockNotAtSlot { .. } => { + debug!( + %peer_id, + %message_slot, + "Payload attestation references block at wrong slot" + ); + self.propagate_validation_result(message_id, peer_id, MessageAcceptance::Ignore); + } PayloadAttestationError::NotInPTC { .. } => { self.propagate_validation_result(message_id, peer_id, MessageAcceptance::Reject); self.gossip_penalize_peer( diff --git a/beacon_node/network/src/network_beacon_processor/sync_methods.rs b/beacon_node/network/src/network_beacon_processor/sync_methods.rs index e2226af094..caf718732b 100644 --- a/beacon_node/network/src/network_beacon_processor/sync_methods.rs +++ b/beacon_node/network/src/network_beacon_processor/sync_methods.rs @@ -376,6 +376,10 @@ impl NetworkBeaconProcessor { let result: Result = result.map_err(|e| BlockError::InternalError(format!("envelope: {e}"))); + if matches!(result, Ok(AvailabilityProcessingStatus::Imported(_))) { + self.chain.recompute_head_at_current_slot().await; + } + self.send_sync_message(SyncMessage::BlockComponentProcessed { process_type, result: result.into(), diff --git a/beacon_node/operation_pool/src/lib.rs b/beacon_node/operation_pool/src/lib.rs index a1789e3b19..45563c2ee9 100644 --- a/beacon_node/operation_pool/src/lib.rs +++ b/beacon_node/operation_pool/src/lib.rs @@ -546,9 +546,15 @@ impl OperationPool { } }); + let max_attester_slashings = if state.fork_name_unchecked().electra_enabled() { + E::max_attester_slashings_electra() + } else { + E::MaxAttesterSlashings::to_usize() + }; + maximum_cover( relevant_attester_slashings, - E::MaxAttesterSlashings::to_usize(), + max_attester_slashings, "attester_slashings", ) .into_iter() @@ -927,6 +933,29 @@ mod release_tests { harness } + /// The maximum number of attester slashings allowed in a block for the state's fork. + fn max_attester_slashings(state: &BeaconState) -> usize { + if state.fork_name_unchecked().electra_enabled() { + E::max_attester_slashings_electra() + } else { + E::MaxAttesterSlashings::to_usize() + } + } + + /// Given the candidate slashings ordered most-profitable first, return the prefix that a + /// block on the state's fork would actually include (i.e. the N most profitable, where N + /// is the per-block attester slashing limit). This keeps the max-cover assertions generic + /// across forks. + fn most_profitable_slashings( + state: &BeaconState, + ordered_by_profitability: Vec, + ) -> Vec { + ordered_by_profitability + .into_iter() + .take(max_attester_slashings(state)) + .collect() + } + /// Test state for attestation-related tests. fn attestation_test_state( num_committees: usize, @@ -1594,7 +1623,10 @@ mod release_tests { op_pool.insert_attester_slashing(slashing_4.clone().validate(&state, spec).unwrap()); let best_slashings = op_pool.get_slashings_and_exits(&state, &harness.spec); - assert_eq!(best_slashings.1, vec![slashing_4, slashing_3]); + assert_eq!( + best_slashings.1, + most_profitable_slashings(&state, vec![slashing_4, slashing_3]) + ); } // Check that we get maximum coverage for attester slashings with overlapping indices @@ -1616,7 +1648,10 @@ mod release_tests { op_pool.insert_attester_slashing(slashing_4.clone().validate(&state, spec).unwrap()); let best_slashings = op_pool.get_slashings_and_exits(&state, &harness.spec); - assert_eq!(best_slashings.1, vec![slashing_1, slashing_3]); + assert_eq!( + best_slashings.1, + most_profitable_slashings(&state, vec![slashing_1, slashing_3]) + ); } // Max coverage of attester slashings taking into account proposer slashings @@ -1638,7 +1673,10 @@ mod release_tests { op_pool.insert_attester_slashing(a_slashing_3.clone().validate(&state, spec).unwrap()); let best_slashings = op_pool.get_slashings_and_exits(&state, &harness.spec); - assert_eq!(best_slashings.1, vec![a_slashing_1, a_slashing_3]); + assert_eq!( + best_slashings.1, + most_profitable_slashings(&state, vec![a_slashing_1, a_slashing_3]) + ); } //Max coverage checking that non overlapping indices are still recognized for their value @@ -1661,7 +1699,10 @@ mod release_tests { op_pool.insert_attester_slashing(slashing_3.clone().validate(&state, spec).unwrap()); let best_slashings = op_pool.get_slashings_and_exits(&state, &harness.spec); - assert_eq!(best_slashings.1, vec![slashing_1, slashing_3]); + assert_eq!( + best_slashings.1, + most_profitable_slashings(&state, vec![slashing_1, slashing_3]) + ); } // Max coverage should be affected by the overall effective balances @@ -1684,7 +1725,10 @@ mod release_tests { op_pool.insert_attester_slashing(slashing_3.clone().validate(&state, spec).unwrap()); let best_slashings = op_pool.get_slashings_and_exits(&state, &harness.spec); - assert_eq!(best_slashings.1, vec![slashing_2, slashing_3]); + assert_eq!( + best_slashings.1, + most_profitable_slashings(&state, vec![slashing_2, slashing_3]) + ); } /// End-to-end test of basic sync contribution handling. @@ -2177,6 +2221,53 @@ mod release_tests { assert_eq!(op_pool.attester_slashings.read().len(), 1); } + /// Regression test to ensure that we are using the correct spec value for max attester slashings post-Electra. + #[tokio::test] + async fn attester_slashings_capped_at_electra_limit() { + let (harness, spec) = cross_fork_harness::(); + let slots_per_epoch = MainnetEthSpec::slots_per_epoch(); + let electra_fork_epoch = spec.electra_fork_epoch.unwrap(); + let deneb_fork_epoch = spec.deneb_fork_epoch.unwrap(); + + let op_pool = OperationPool::::new(); + + harness + .extend_to_slot(electra_fork_epoch.start_slot(slots_per_epoch)) + .await; + let electra_head = harness.chain.canonical_head.cached_head().snapshot; + assert!( + electra_head + .beacon_state + .fork_name_unchecked() + .electra_enabled() + ); + + // Create two slashings + for validators in [vec![0], vec![1]] { + let slashing = harness.make_attester_slashing_with_epochs( + validators, + Some(Epoch::new(0)), + Some(deneb_fork_epoch - 1), + Some(Epoch::new(0)), + Some(deneb_fork_epoch - 1), + ); + let verified = slashing + .validate(&electra_head.beacon_state, &harness.chain.spec) + .unwrap(); + op_pool.insert_attester_slashing(verified); + } + assert_eq!(op_pool.attester_slashings.read().len(), 2); + + // Despite two valid slashings being pending, only one may be extracted post-Electra. + let mut to_be_slashed = HashSet::new(); + let attester_slashings = + op_pool.get_attester_slashings(&electra_head.beacon_state, &mut to_be_slashed); + assert_eq!( + attester_slashings.len(), + MainnetEthSpec::max_attester_slashings_electra() + ); + } + fn make_payload_attestation_message( slot: Slot, validator_index: u64, diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index 7816fb4483..90f2bb9a67 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -542,6 +542,27 @@ where } } + /// Returns the dependent root for `block_root`, per the spec `get_dependent_root` helper. + fn get_dependent_root( + &self, + block_root: Hash256, + current_slot: Slot, + spec: &ChainSpec, + ) -> Result, Error> { + let epoch = current_slot.epoch(E::slots_per_epoch()); + + if epoch <= spec.min_seed_lookahead { + return Ok(Some(Hash256::zero())); + } + + let dependent_slot = epoch + .saturating_sub(spec.min_seed_lookahead) + .start_slot(E::slots_per_epoch()) + .saturating_sub(1_u64); + + self.get_ancestor(block_root, dependent_slot) + } + /// Run the fork choice rule to determine the head. /// /// ## Specification @@ -768,7 +789,6 @@ where block_delay: Duration, state: &BeaconState, payload_verification_status: PayloadVerificationStatus, - canonical_head_proposer_index: u64, spec: &ChainSpec, ) -> Result<(), Error> { let _timer = metrics::start_timer(&metrics::FORK_CHOICE_ON_BLOCK_TIMES); @@ -780,10 +800,17 @@ where return Ok(()); } - // Provide the slot (as per the system clock) to the `fc_store` and then return its view of - // the current slot. The `fc_store` will ensure that the `current_slot` is never - // decreasing, a property which we must maintain. - let current_slot = self.update_time(system_time_current_slot)?; + let head_root = if system_time_current_slot == self.fc_store.get_current_slot() { + // Fork choice has already run for the current slot, so we can safely use the cached + // head without recomputing it. + self.cached_fork_choice_view().head_block_root + } else { + // Fork choice hasn't run for the current slot yet: run it, updating the fork choice + // store's current slot in the process. + self.get_head(system_time_current_slot, spec)?.0 + }; + let current_slot = self.fc_store.get_current_slot(); + debug_assert_eq!(current_slot, system_time_current_slot); // Parent block must be known. let parent_block = self @@ -833,19 +860,24 @@ where let attestation_threshold = spec.get_attestation_due::(block.slot()); - // Add proposer score boost if the block is the first timely block for this slot and its - // proposer matches the expected proposer on the canonical chain (per spec - // `update_proposer_boost_root`, introduced in v1.7.0-alpha.5). + // Add proposer score boost if the block is the first timely block for this slot and it + // shares the same dependent root as the canonical chain head (per spec + // `update_proposer_boost_root`). let is_before_attesting_interval = block_delay < attestation_threshold; - + let is_timely = current_slot == block.slot() && is_before_attesting_interval; let is_first_block = self.fc_store.proposer_boost_root().is_zero(); - let is_canonical_proposer = block.proposer_index() == canonical_head_proposer_index; - if current_slot == block.slot() - && is_before_attesting_interval - && is_first_block - && is_canonical_proposer - { - self.fc_store.set_proposer_boost_root(block_root); + + if is_timely && is_first_block { + // The block isn't in fork choice so resolve its dependent root via its parent. + let block_dependent_root = + self.get_dependent_root(block.parent_root(), current_slot, spec)?; + let head_dependent_root = self.get_dependent_root(head_root, current_slot, spec)?; + + // Add proposer score boost if the block is timely, not conflicting with an + // existing block, with the same dependent root as the canonical chain head. + if block_dependent_root.is_some() && block_dependent_root == head_dependent_root { + self.fc_store.set_proposer_boost_root(block_root); + } } // Update store with checkpoints if necessary @@ -1581,9 +1613,10 @@ where &self, block_root: &Hash256, parent_payload_status: PayloadStatus, + current_slot: Slot, ) -> Result> { self.proto_array - .should_build_on_full::(block_root, parent_payload_status) + .should_build_on_full::(block_root, parent_payload_status, current_slot) .map_err(Error::ProtoArrayStringError) } diff --git a/consensus/fork_choice/tests/tests.rs b/consensus/fork_choice/tests/tests.rs index 848834b4d8..02229e6f33 100644 --- a/consensus/fork_choice/tests/tests.rs +++ b/consensus/fork_choice/tests/tests.rs @@ -316,7 +316,6 @@ impl ForkChoiceTest { Duration::from_secs(0), &state, PayloadVerificationStatus::Verified, - block.message().proposer_index(), &self.harness.chain.spec, ) .unwrap(); @@ -360,7 +359,6 @@ impl ForkChoiceTest { Duration::from_secs(0), &state, PayloadVerificationStatus::Verified, - block.message().proposer_index(), &self.harness.chain.spec, ) .expect_err("on_block did not return an error"); diff --git a/consensus/proto_array/src/fork_choice_test_definition.rs b/consensus/proto_array/src/fork_choice_test_definition.rs index 3dc5406212..7ffa763308 100644 --- a/consensus/proto_array/src/fork_choice_test_definition.rs +++ b/consensus/proto_array/src/fork_choice_test_definition.rs @@ -124,6 +124,15 @@ pub enum Operation { #[serde(default)] proposer_boost_root: Option, }, + /// Assert the result of `should_build_on_full` for the parent `block_root`, where + /// `parent_payload_status` is the status the proposer would build on and `proposal_slot` + /// is the slot being proposed. + AssertShouldBuildOnFull { + block_root: Hash256, + parent_payload_status: PayloadStatus, + proposal_slot: Slot, + expected: bool, + }, } #[derive(Debug, Clone, Serialize, Deserialize)] @@ -606,6 +615,30 @@ impl ForkChoiceTestDefinition { op_index ); } + Operation::AssertShouldBuildOnFull { + block_root, + parent_payload_status, + proposal_slot, + expected, + } => { + let actual = fork_choice + .should_build_on_full::( + &block_root, + parent_payload_status, + proposal_slot, + ) + .unwrap_or_else(|e| { + panic!( + "should_build_on_full op at index {} returned error: {}", + op_index, e + ) + }); + assert_eq!( + actual, expected, + "should_build_on_full mismatch at op index {}", + op_index + ); + } } } } diff --git a/consensus/proto_array/src/fork_choice_test_definition/gloas_payload.rs b/consensus/proto_array/src/fork_choice_test_definition/gloas_payload.rs index bf79a0170f..07262fb0d7 100644 --- a/consensus/proto_array/src/fork_choice_test_definition/gloas_payload.rs +++ b/consensus/proto_array/src/fork_choice_test_definition/gloas_payload.rs @@ -971,6 +971,90 @@ pub fn get_gloas_proposer_boost_flips_ancestor_test_definition() -> ForkChoiceTe } } +/// Tests the slot check in `should_build_on_full`. When the parent is from an earlier slot the +/// function returns `true` and ignores PTC data-availability votes. It only checks those votes +/// when the parent is from the immediately preceding slot. +pub fn get_gloas_should_build_on_full_test_definition() -> ForkChoiceTestDefinition { + let mut ops = vec![]; + + // Block 1 at slot 1, child of genesis. + ops.push(Operation::ProcessBlock { + slot: Slot::new(1), + root: get_root(1), + parent_root: get_root(0), + justified_checkpoint: get_checkpoint(0), + finalized_checkpoint: get_checkpoint(0), + execution_payload_parent_hash: Some(get_hash(0)), + execution_payload_block_hash: Some(get_hash(1)), + }); + + // PTC has voted the payload data unavailable. `is_timely` sets `payload_received` so the votes + // are consulted, and clearing the data-availability bits gives the "false" votes a majority. + ops.push(Operation::SetPayloadTiebreak { + block_root: get_root(1), + is_timely: true, + is_data_available: false, + }); + + // When the parent is `Empty` `should_build_on_full` returns `false`. This check runs before + // the slot check, so the result is `false` for both the previous-slot case (block slot 1, proposal slot 2) + // and an earlier-slot case (proposal slot 3). + ops.push(Operation::AssertShouldBuildOnFull { + block_root: get_root(1), + parent_payload_status: PayloadStatus::Empty, + proposal_slot: Slot::new(2), + expected: false, + }); + ops.push(Operation::AssertShouldBuildOnFull { + block_root: get_root(1), + parent_payload_status: PayloadStatus::Empty, + proposal_slot: Slot::new(3), + expected: false, + }); + + // `Full` parent from the immediately preceding slot (block slot 1, proposal slot 2). The PTC + // votes are consulted, and since data is unavailable the proposer does not build on full. + ops.push(Operation::AssertShouldBuildOnFull { + block_root: get_root(1), + parent_payload_status: PayloadStatus::Full, + proposal_slot: Slot::new(2), + expected: false, + }); + + // `Full` parent from an *earlier* slot (block slot 1, proposal slot 3). The slot check + // short-circuits to `true` without consulting the (unavailable) PTC votes. + ops.push(Operation::AssertShouldBuildOnFull { + block_root: get_root(1), + parent_payload_status: PayloadStatus::Full, + proposal_slot: Slot::new(3), + expected: true, + }); + + // Flip the PTC view to *available* and re-check the previous-slot case. The votes now permit + // building on full. + ops.push(Operation::SetPayloadTiebreak { + block_root: get_root(1), + is_timely: true, + is_data_available: true, + }); + ops.push(Operation::AssertShouldBuildOnFull { + block_root: get_root(1), + parent_payload_status: PayloadStatus::Full, + proposal_slot: Slot::new(2), + expected: true, + }); + + ForkChoiceTestDefinition { + finalized_block_slot: Slot::new(0), + justified_checkpoint: get_checkpoint(0), + finalized_checkpoint: get_checkpoint(0), + operations: ops, + execution_payload_parent_hash: Some(get_hash(42)), + execution_payload_block_hash: Some(get_hash(0)), + spec: Some(gloas_spec()), + } +} + #[cfg(test)] mod tests { use super::*; @@ -1160,6 +1244,12 @@ mod tests { test.run(); } + #[test] + fn should_build_on_full_slot_check() { + let test = get_gloas_should_build_on_full_test_definition(); + test.run(); + } + /// Test that execution payload invalidation propagates across the V17→V29 fork /// boundary: after invalidating a V17 parent, head must not select any descendant. /// diff --git a/consensus/proto_array/src/proto_array.rs b/consensus/proto_array/src/proto_array.rs index bd15bb4599..d45412c608 100644 --- a/consensus/proto_array/src/proto_array.rs +++ b/consensus/proto_array/src/proto_array.rs @@ -1569,10 +1569,13 @@ impl ProtoArray { /// Called by the proposer to decide whether to build on the full or empty /// parent pending node. Returns false if the PTC has voted the data as unavailable. + /// For a parent from an earlier slot the `Empty` or `Full` node has already been resolved + /// by attestation weight in `get_head`. pub fn should_build_on_full( &self, fc_node: &IndexedForkChoiceNode, proto_node: &ProtoNode, + current_slot: Slot, ) -> Result { if fc_node.payload_status == PayloadStatus::Pending { return Err(Error::InvalidPayloadStatus { @@ -1584,10 +1587,23 @@ impl ProtoArray { if fc_node.payload_status == PayloadStatus::Empty { return Ok(false); } + + if proto_node.slot().saturating_add(1u64) != current_slot { + return Ok(true); + } + // Check that false votes have not achieved an absolute majority. This allows the payload to be // considered available when either a majority have voted true or not enough votes have // been cast either way. - Ok(!proto_node.payload_data_availability::(false)?) + if proto_node.payload_data_availability::(false)? { + return Ok(false); + } + + if proto_node.payload_timeliness::(false)? { + return Ok(false); + } + + Ok(true) } pub fn should_extend_payload( diff --git a/consensus/proto_array/src/proto_array_fork_choice.rs b/consensus/proto_array/src/proto_array_fork_choice.rs index f0b599dbcd..69202486a7 100644 --- a/consensus/proto_array/src/proto_array_fork_choice.rs +++ b/consensus/proto_array/src/proto_array_fork_choice.rs @@ -968,6 +968,7 @@ impl ProtoArrayForkChoice { &self, block_root: &Hash256, parent_payload_status: PayloadStatus, + current_slot: Slot, ) -> Result { let block_index = self .proto_array @@ -985,7 +986,7 @@ impl ProtoArrayForkChoice { payload_status: parent_payload_status, }; self.proto_array - .should_build_on_full::(&fc_node, proto_node) + .should_build_on_full::(&fc_node, proto_node, current_slot) .map_err(|e| format!("{e:?}")) } diff --git a/consensus/state_processing/src/per_block_processing/process_operations.rs b/consensus/state_processing/src/per_block_processing/process_operations.rs index f88a325d4e..876e66d3af 100644 --- a/consensus/state_processing/src/per_block_processing/process_operations.rs +++ b/consensus/state_processing/src/per_block_processing/process_operations.rs @@ -880,8 +880,11 @@ pub fn process_deposit_requests_pre_gloas( spec: &ChainSpec, ) -> Result<(), BlockProcessingError> { for request in deposit_requests { - // Set deposit receipt start index - if state.deposit_requests_start_index()? == spec.unset_deposit_requests_start_index { + // Set deposit receipt start index if pre-Fulu. + // Support for the former Eth1 bridge deposit mechanism was removed in Fulu. + if !state.fork_name_unchecked().fulu_enabled() + && state.deposit_requests_start_index()? == spec.unset_deposit_requests_start_index + { *state.deposit_requests_start_index_mut()? = request.index } let slot = state.slot(); diff --git a/consensus/state_processing/src/per_epoch_processing/single_pass.rs b/consensus/state_processing/src/per_epoch_processing/single_pass.rs index 881e6bb16c..43d6606f07 100644 --- a/consensus/state_processing/src/per_epoch_processing/single_pass.rs +++ b/consensus/state_processing/src/per_epoch_processing/single_pass.rs @@ -1105,8 +1105,10 @@ impl PendingDepositsContext { let pending_deposits = state.pending_deposits()?; for deposit in pending_deposits.iter() { - // Do not process deposit requests if the Eth1 bridge deposits are not yet applied. - if deposit.slot > spec.genesis_slot + // Do not process deposit requests if pre-Fulu and the Eth1 bridge deposits are not yet applied. + // Support for the former Eth1 bridge deposit mechanism was removed in Fulu. + if !state.fork_name_unchecked().fulu_enabled() + && deposit.slot > spec.genesis_slot && state.eth1_deposit_index() < state.deposit_requests_start_index()? { break; diff --git a/consensus/types/src/state/beacon_state.rs b/consensus/types/src/state/beacon_state.rs index 027acfab7f..26f28eda45 100644 --- a/consensus/types/src/state/beacon_state.rs +++ b/consensus/types/src/state/beacon_state.rs @@ -1127,21 +1127,33 @@ impl BeaconState { // Post-Fulu we must never compute proposer indices using insufficient lookahead. This // would be very dangerous as it would lead to conflicts between the *true* proposer as // defined by `self.proposer_lookahead` and the output of this function. - // With MIN_SEED_LOOKAHEAD=1 (common config), this is equivalent to checking that the - // requested epoch is not the current epoch. // - // We do not run this check if this function is called from `upgrade_to_fulu`, - // which runs *after* the slot is incremented, and needs to compute the proposer - // shuffling for the epoch that was just transitioned into. - if self.fork_name_unchecked().fulu_enabled() - && epoch < current_epoch.safe_add(spec.min_seed_lookahead)? - { - return Err( - BeaconStateError::ComputeProposerIndicesInsufficientLookahead { - current_epoch, - request_epoch: epoch, - }, - ); + // Furthermore, post-Gloas, we must never compute proposers at any slot other than the + // dependent root slot itself, as slashings at subsequent slots have the ability to + // change the shuffling. + // + // For simplicity these two checks are combined into a single check on the dependent + // slot, which is safe for Fulu and Gloas. This function is always called from + // `get_beacon_proposer_indices`, which uses the cached lookahead for `current_epoch` and + // `next_epoch`. The only epoch's shuffling that should ordinarily be computed therefore + // is `next_epoch + 1`, which for Fulu and Gloas is computed during the epoch transition + // in the last slot of `current_epoch` (before the slot is incremented into + // `next_epoch`). + // + // The only case where computation of proposers in `current_epoch` and `next_epoch` is + // directly required is during the fork to Fulu itself + // (`upgrade_to_fulu`/`initialize_proposer_lookahead`), in which case the state is not + // yet the Fulu variant, and we omit the check. + if self.fork_name_unchecked().fulu_enabled() { + let dependent_slot = spec.proposer_shuffling_decision_slot::(epoch); + if self.slot() != dependent_slot { + return Err( + BeaconStateError::ComputeProposerIndicesInsufficientLookahead { + current_epoch, + request_epoch: epoch, + }, + ); + } } } else { // Pre-Fulu the situation is reversed, we *should not* compute proposer indices using @@ -1375,7 +1387,7 @@ impl BeaconState { spec: &ChainSpec, ) -> Result, BeaconStateError> { // This isn't in the spec, but we remove the footgun that is requesting the current epoch - // for a Fulu state. + // or next epoch for a Fulu state. if let Ok(proposer_lookahead) = self.proposer_lookahead() && epoch >= self.current_epoch() && epoch <= self.next_epoch()? @@ -1394,7 +1406,15 @@ impl BeaconState { } // Not using the cached validator indices since they are shuffled. - let indices = self.get_active_validator_indices(epoch, spec)?; + let mut indices = self.get_active_validator_indices(epoch, spec)?; + + // Post-Gloas, slashed validators are excluded from proposer selection + if self.fork_name_unchecked().gloas_enabled() { + let latest_block_slot = self.latest_block_header().slot; + let slashings_cache = self.slashings_cache(); + slashings_cache.check_initialized(latest_block_slot)?; + indices.retain(|&index| !slashings_cache.is_slashed(index)); + } let preimage = self.get_seed(epoch, Domain::BeaconProposer, spec)?; self.compute_proposer_indices(epoch, preimage.as_slice(), &indices, spec) diff --git a/consensus/types/src/state/slashings_cache.rs b/consensus/types/src/state/slashings_cache.rs index b6ed583df8..cdaa2c6c89 100644 --- a/consensus/types/src/state/slashings_cache.rs +++ b/consensus/types/src/state/slashings_cache.rs @@ -64,3 +64,127 @@ impl SlashingsCache { self.latest_block_slot = Some(latest_block_slot); } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::{Epoch, Hash256}; + use bls::PublicKeyBytes; + + /// Build a minimal validator with the given `slashed` flag. The other fields are irrelevant to + /// the slashings cache. + fn validator(slashed: bool) -> Validator { + Validator { + pubkey: PublicKeyBytes::empty(), + withdrawal_credentials: Hash256::ZERO, + effective_balance: 0, + slashed, + activation_eligibility_epoch: Epoch::new(0), + activation_epoch: Epoch::new(0), + exit_epoch: Epoch::new(0), + withdrawable_epoch: Epoch::new(0), + } + } + + /// Validators 1 and 3 are slashed, the rest are not. + fn validators() -> Vec { + vec![ + validator(false), + validator(true), + validator(false), + validator(true), + validator(false), + ] + } + + #[test] + fn new_captures_slashed_indices() { + let validators = validators(); + let cache = SlashingsCache::new(Slot::new(7), validators.iter()); + + // The cache is initialized at the block slot it was built for. + assert!(cache.is_initialized(Slot::new(7))); + assert!(!cache.is_initialized(Slot::new(8))); + + // Each index reports the same `slashed` status as the source validator. + for (index, validator) in validators.iter().enumerate() { + assert_eq!( + cache.is_slashed(index), + validator.slashed, + "validator {index} slashed status mismatch" + ); + } + + // An out-of-bounds index is not slashed. + assert!(!cache.is_slashed(validators.len())); + } + + #[test] + fn default_is_uninitialized() { + let cache = SlashingsCache::default(); + + // A default cache is not initialized at any slot. + assert!(!cache.is_initialized(Slot::new(0))); + assert_eq!( + cache.check_initialized(Slot::new(0)), + Err(BeaconStateError::SlashingsCacheUninitialized { + initialized_slot: None, + latest_block_slot: Slot::new(0), + }) + ); + + // It reports nothing as slashed. This is exactly why callers must check initialization + // before trusting `is_slashed`. + assert!(!cache.is_slashed(0)); + } + + #[test] + fn check_initialized_matches_block_slot() { + let cache = SlashingsCache::new(Slot::new(3), validators().iter()); + + assert_eq!(cache.check_initialized(Slot::new(3)), Ok(())); + assert_eq!( + cache.check_initialized(Slot::new(4)), + Err(BeaconStateError::SlashingsCacheUninitialized { + initialized_slot: Some(Slot::new(3)), + latest_block_slot: Slot::new(4), + }) + ); + } + + #[test] + fn record_validator_slashing_requires_matching_slot() { + let mut cache = SlashingsCache::new(Slot::new(3), validators().iter()); + + // Index 0 starts unslashed. + assert!(!cache.is_slashed(0)); + + // Recording at the initialized slot succeeds and marks the validator slashed. + cache.record_validator_slashing(Slot::new(3), 0).unwrap(); + assert!(cache.is_slashed(0)); + + // Recording at a slot the cache is not initialized for errors and leaves the set unchanged. + assert_eq!( + cache.record_validator_slashing(Slot::new(4), 2), + Err(BeaconStateError::SlashingsCacheUninitialized { + initialized_slot: Some(Slot::new(3)), + latest_block_slot: Slot::new(4), + }) + ); + assert!(!cache.is_slashed(2)); + } + + #[test] + fn update_latest_block_slot_preserves_slashed_set() { + let mut cache = SlashingsCache::new(Slot::new(3), validators().iter()); + + cache.update_latest_block_slot(Slot::new(4)); + + // The initialized slot moves forward without clearing the recorded slashings. + assert!(!cache.is_initialized(Slot::new(3))); + assert!(cache.is_initialized(Slot::new(4))); + assert!(cache.is_slashed(1)); + assert!(cache.is_slashed(3)); + assert!(!cache.is_slashed(0)); + } +} diff --git a/testing/ef_tests/Makefile b/testing/ef_tests/Makefile index f566a89ded..a497fdaeff 100644 --- a/testing/ef_tests/Makefile +++ b/testing/ef_tests/Makefile @@ -1,6 +1,6 @@ # To download/extract nightly tests, run: # CONSENSUS_SPECS_TEST_VERSION=nightly make -CONSENSUS_SPECS_TEST_VERSION ?= v1.7.0-alpha.8 +CONSENSUS_SPECS_TEST_VERSION ?= v1.7.0-alpha.10 REPO_NAME := consensus-spec-tests OUTPUT_DIR := ./$(REPO_NAME) diff --git a/testing/ef_tests/check_all_files_accessed.py b/testing/ef_tests/check_all_files_accessed.py index 723c5e7e9e..c81f94d8e9 100755 --- a/testing/ef_tests/check_all_files_accessed.py +++ b/testing/ef_tests/check_all_files_accessed.py @@ -84,6 +84,8 @@ excluded_paths = [ "tests/.*/.*/networking/gossip_sync_committee_message/.*", "tests/.*/.*/networking/gossip_sync_committee_contribution_and_proof/.*", "tests/.*/.*/networking/gossip_blob_sidecar/.*", + "tests/.*/.*/networking/gossip_data_column_sidecar/.*", + "tests/.*/.*/networking/gossip_partial_data_column_sidecar/.*", # TODO: fast confirmation rule not merged yet "tests/.*/.*/fast_confirmation", ] diff --git a/testing/ef_tests/src/cases/epoch_processing.rs b/testing/ef_tests/src/cases/epoch_processing.rs index ec243f05cc..b89a72ca77 100644 --- a/testing/ef_tests/src/cases/epoch_processing.rs +++ b/testing/ef_tests/src/cases/epoch_processing.rs @@ -423,6 +423,9 @@ impl> Case for EpochProcessing { // Processing requires the committee caches. pre_state.build_all_committee_caches(spec).unwrap(); + // Proposer index computation (e.g. proposer lookahead) requires the slashings cache post-Gloas + pre_state.build_slashings_cache().unwrap(); + let mut state = pre_state.clone(); let mut expected = self.post.clone(); diff --git a/testing/ef_tests/src/cases/fork_choice.rs b/testing/ef_tests/src/cases/fork_choice.rs index cf122dbd65..9edc5b85c8 100644 --- a/testing/ef_tests/src/cases/fork_choice.rs +++ b/testing/ef_tests/src/cases/fork_choice.rs @@ -53,6 +53,9 @@ pub struct PowBlock { pub struct Head { slot: Slot, root: Hash256, + // Post-gloas, the head check also asserts the payload status of the head block + #[serde(default)] + payload_status: Option, } #[derive(Debug, Clone, Copy, PartialEq, Deserialize)] @@ -132,6 +135,10 @@ pub enum Step< }, Attestation { attestation: TAttestation, + // Post-Gloas `on_attestation` tests can assert that an attestation is rejected (e.g. an + // invalid payload-present index). Defaults to `true` for the pre-Gloas tests that omit it. + #[serde(default = "default_true")] + valid: bool, }, AttesterSlashing { attester_slashing: TAttesterSlashing, @@ -169,8 +176,12 @@ fn default_true() -> bool { #[derive(Debug, Clone, Deserialize)] #[serde(deny_unknown_fields)] pub struct Meta { - #[serde(rename(deserialize = "description"))] - _description: String, + #[serde(default, rename(deserialize = "description"))] + _description: Option, + // Some Gloas fork choice tests carry a `bls_setting` instead of a description. We accept and + // ignore it: the value is always `1` (BLS required), which matches our default behaviour. + #[serde(default, rename(deserialize = "bls_setting"))] + _bls_setting: Option, } #[derive(Debug)] @@ -240,17 +251,19 @@ impl LoadCase for ForkChoiceTest { valid, }) } - Step::Attestation { attestation } => { + Step::Attestation { attestation, valid } => { if fork_name.electra_enabled() { ssz_decode_file(&path.join(format!("{}.ssz_snappy", attestation))).map( |attestation| Step::Attestation { attestation: Attestation::Electra(attestation), + valid, }, ) } else { ssz_decode_file(&path.join(format!("{}.ssz_snappy", attestation))).map( |attestation| Step::Attestation { attestation: Attestation::Base(attestation), + valid, }, ) } @@ -389,7 +402,9 @@ impl Case for ForkChoiceTest { proofs.clone(), *valid, )?, - Step::Attestation { attestation } => tester.process_attestation(attestation)?, + Step::Attestation { attestation, valid } => { + tester.process_attestation(attestation, *valid)? + } Step::AttesterSlashing { attester_slashing } => { tester.process_attester_slashing(attester_slashing.to_ref()) } @@ -673,7 +688,7 @@ impl Tester { if success { for attestation in block.message().body().attestations() { let att = attestation.clone_as_attestation(); - let _ = self.process_attestation(&att); + let _ = self.process_attestation(&att, true); } for attester_slashing in block.message().body().attester_slashings() { self.process_attester_slashing(attester_slashing); @@ -786,7 +801,7 @@ impl Tester { if success { for attestation in block.message().body().attestations() { let att = attestation.clone_as_attestation(); - let _ = self.process_attestation(&att); + let _ = self.process_attestation(&att, true); } for attester_slashing in block.message().body().attester_slashings() { self.process_attester_slashing(attester_slashing); @@ -848,7 +863,6 @@ impl Tester { block_delay, &state, PayloadVerificationStatus::Irrelevant, - block.message().proposer_index(), &self.harness.chain.spec, ); @@ -863,22 +877,41 @@ impl Tester { Ok(()) } - pub fn process_attestation(&self, attestation: &Attestation) -> Result<(), Error> { - let (indexed_attestation, _) = obtain_indexed_attestation_and_committees_per_slot( + pub fn process_attestation( + &self, + attestation: &Attestation, + valid: bool, + ) -> Result<(), Error> { + // Post-Gloas `on_attestation` tests can assert that an attestation is rejected (e.g. an + // invalid same-slot/payload-present index). Treat any failure in either indexing or fork + // choice application as a rejection so it can be compared against the expected `valid` flag. + let result = obtain_indexed_attestation_and_committees_per_slot( &self.harness.chain, attestation.to_ref(), ) - .map_err(|e| Error::InternalError(format!("attestation indexing failed with {:?}", e)))?; - let verified_attestation: ManuallyVerifiedAttestation> = - ManuallyVerifiedAttestation { - attestation, - indexed_attestation, - }; + .map_err(|e| format!("attestation indexing failed with {:?}", e)) + .and_then(|(indexed_attestation, _)| { + let verified_attestation: ManuallyVerifiedAttestation> = + ManuallyVerifiedAttestation { + attestation, + indexed_attestation, + }; - self.harness - .chain - .apply_attestation_to_fork_choice(&verified_attestation) - .map_err(|e| Error::InternalError(format!("attestation import failed with {:?}", e))) + self.harness + .chain + .apply_attestation_to_fork_choice(&verified_attestation) + .map_err(|e| format!("attestation import failed with {:?}", e)) + }); + + if valid { + result.map_err(Error::InternalError) + } else if result.is_ok() { + Err(Error::DidntFail( + "attestation was valid but the test expects it to be rejected".to_string(), + )) + } else { + Ok(()) + } } pub fn process_attester_slashing(&self, attester_slashing: AttesterSlashingRef) { @@ -909,9 +942,17 @@ impl Tester { let chain_head = Head { slot: head.head_slot(), root: head.head_block_root(), + // Compared separately below so the slot/root equality is not affected. + payload_status: expected_head.payload_status, }; - check_equal("head", chain_head, expected_head) + check_equal("head", chain_head, expected_head)?; + + if let Some(expected_status) = expected_head.payload_status { + self.check_head_payload_status(expected_status)?; + } + + Ok(()) } pub fn check_time(&self, expected_time: u64) -> Result<(), Error> { diff --git a/testing/ef_tests/src/cases/operations.rs b/testing/ef_tests/src/cases/operations.rs index f5c999920d..d851427a95 100644 --- a/testing/ef_tests/src/cases/operations.rs +++ b/testing/ef_tests/src/cases/operations.rs @@ -204,7 +204,12 @@ impl Operation for Deposit { ssz_decode_file(path) } - fn is_enabled_for_fork(_: ForkName) -> bool { + fn is_enabled_for_fork(fork_name: ForkName) -> bool { + // The standalone `deposit` operation tests were removed in Fulu (deposits are processed + // via `deposit_request` from Electra onwards). + if fork_name.fulu_enabled() { + return false; + } // Some deposit tests require signature verification but are not marked as such. cfg!(not(feature = "fake_crypto")) } diff --git a/testing/ef_tests/src/handler.rs b/testing/ef_tests/src/handler.rs index df1ece49dd..b45ea3a230 100644 --- a/testing/ef_tests/src/handler.rs +++ b/testing/ef_tests/src/handler.rs @@ -708,13 +708,6 @@ impl Handler for ForkChoiceHandler { return false; } - // No FCU override tests prior to bellatrix, and removed in Gloas. - if self.handler_name == "should_override_forkchoice_update" - && (!fork_name.bellatrix_enabled() || fork_name.gloas_enabled()) - { - return false; - } - // Deposit tests exist only for Electra and later. if self.handler_name == "deposit_with_reorg" && !fork_name.electra_enabled() { return false; @@ -725,9 +718,10 @@ impl Handler for ForkChoiceHandler { return false; } - // on_execution_payload_envelope, get_parent_payload_status, and + // on_attestation, on_execution_payload_envelope, get_parent_payload_status, and // on_payload_attestation_message tests exist only for Gloas and later. - if (self.handler_name == "on_execution_payload_envelope" + if (self.handler_name == "on_attestation" + || self.handler_name == "on_execution_payload_envelope" || self.handler_name == "get_parent_payload_status" || self.handler_name == "on_payload_attestation_message") && !fork_name.gloas_enabled() diff --git a/testing/ef_tests/tests/tests.rs b/testing/ef_tests/tests/tests.rs index 6e1c4fdc10..9af88e0201 100644 --- a/testing/ef_tests/tests/tests.rs +++ b/testing/ef_tests/tests/tests.rs @@ -708,14 +708,18 @@ mod ssz_static { #[test] fn blob_sidecar() { - SszStaticHandler::, MinimalEthSpec>::deneb_and_later().run(); - SszStaticHandler::, MainnetEthSpec>::deneb_and_later().run(); + SszStaticHandler::, MinimalEthSpec>::deneb_only().run(); + SszStaticHandler::, MainnetEthSpec>::deneb_only().run(); + SszStaticHandler::, MinimalEthSpec>::electra_only().run(); + SszStaticHandler::, MainnetEthSpec>::electra_only().run(); } #[test] fn blob_identifier() { - SszStaticHandler::::deneb_and_later().run(); - SszStaticHandler::::deneb_and_later().run(); + SszStaticHandler::::deneb_only().run(); + SszStaticHandler::::deneb_only().run(); + SszStaticHandler::::electra_only().run(); + SszStaticHandler::::electra_only().run(); } #[test] @@ -1025,6 +1029,12 @@ fn fork_choice_get_head() { ForkChoiceHandler::::new("get_head").run(); } +#[test] +fn fork_choice_on_attestation() { + ForkChoiceHandler::::new("on_attestation").run(); + ForkChoiceHandler::::new("on_attestation").run(); +} + #[test] fn fork_choice_on_block() { ForkChoiceHandler::::new("on_block").run(); @@ -1049,12 +1059,6 @@ fn fork_choice_withholding() { // There is no mainnet variant for this test. } -#[test] -fn fork_choice_should_override_forkchoice_update() { - ForkChoiceHandler::::new("should_override_forkchoice_update").run(); - ForkChoiceHandler::::new("should_override_forkchoice_update").run(); -} - #[test] fn fork_choice_get_proposer_head() { ForkChoiceHandler::::new("get_proposer_head").run(); From 41dff2d96523cf16ab3cf1f04993d69447d1ea81 Mon Sep 17 00:00:00 2001 From: Daniel Knopik <107140945+dknopik@users.noreply.github.com> Date: Tue, 16 Jun 2026 08:53:25 +0200 Subject: [PATCH 08/15] Enable mplex by default and change --enable-mplex to take bool (#9476) Enable Mplex by default, and let `--enable-mplex` take a bool argument. Co-Authored-By: Daniel Knopik Co-Authored-By: Michael Sproul Co-Authored-By: Michael Sproul --- beacon_node/lighthouse_network/src/config.rs | 4 +- beacon_node/src/cli.rs | 9 +++-- beacon_node/src/config.rs | 6 +-- book/src/help_bn.md | 7 ++-- lighthouse/tests/beacon_node.rs | 39 ++++++++++++++++++++ 5 files changed, 54 insertions(+), 11 deletions(-) diff --git a/beacon_node/lighthouse_network/src/config.rs b/beacon_node/lighthouse_network/src/config.rs index 8f7c1dd8de..f54a7ee5b9 100644 --- a/beacon_node/lighthouse_network/src/config.rs +++ b/beacon_node/lighthouse_network/src/config.rs @@ -125,7 +125,7 @@ pub struct Config { /// Whether light client protocols should be enabled. pub enable_light_client_server: bool, - /// Whether to enable the deprecated mplex multiplexer alongside yamux. + /// Whether to enable the mplex multiplexer alongside yamux. Enabled by default. pub enable_mplex: bool, /// Configuration for the outbound rate limiter (requests made by this node). @@ -365,7 +365,7 @@ impl Default for Config { proposer_only: false, metrics_enabled: false, enable_light_client_server: true, - enable_mplex: false, + enable_mplex: true, outbound_rate_limiter_config: None, invalid_block_storage: None, inbound_rate_limiter_config: None, diff --git a/beacon_node/src/cli.rs b/beacon_node/src/cli.rs index f06d0045b6..942b30120b 100644 --- a/beacon_node/src/cli.rs +++ b/beacon_node/src/cli.rs @@ -390,9 +390,12 @@ pub fn cli_app() -> Command { .arg( Arg::new("enable-mplex") .long("enable-mplex") - .action(ArgAction::SetTrue) - .help_heading(FLAG_HEADER) - .help("Enables mplex multiplexer alongside yamux. Yamux is preferred when both are available.") + .value_name("BOOLEAN") + .action(ArgAction::Set) + .num_args(0..=1) + .default_value("true") + .default_missing_value("true") + .help("Enables the mplex multiplexer alongside yamux. Yamux is preferred when both are available. Enabled by default; set to \"false\" to disable.") .display_order(0) ) .arg( diff --git a/beacon_node/src/config.rs b/beacon_node/src/config.rs index 1793dfa091..4f42923010 100644 --- a/beacon_node/src/config.rs +++ b/beacon_node/src/config.rs @@ -7,7 +7,7 @@ use beacon_chain::graffiti_calculator::GraffitiOrigin; use bls::PublicKeyBytes; use clap::{ArgMatches, Id, parser::ValueSource}; use clap_utils::flags::DISABLE_MALLOC_TUNING_FLAG; -use clap_utils::{parse_flag, parse_required}; +use clap_utils::{parse_flag, parse_optional, parse_required}; use client::{ClientConfig, ClientGenesis}; use directory::{DEFAULT_BEACON_NODE_DIR, DEFAULT_NETWORK_DIR, DEFAULT_ROOT_DIR}; use environment::RuntimeContext; @@ -1434,8 +1434,8 @@ pub fn set_network_config( config.disable_quic_support = true; } - if parse_flag(cli_args, "enable-mplex") { - config.enable_mplex = true; + if let Some(enable_mplex) = parse_optional(cli_args, "enable-mplex")? { + config.enable_mplex = enable_mplex; } if parse_flag(cli_args, "disable-upnp") { diff --git a/book/src/help_bn.md b/book/src/help_bn.md index f02f1cb10e..1612d35b0e 100644 --- a/book/src/help_bn.md +++ b/book/src/help_bn.md @@ -84,6 +84,10 @@ Options: --discovery-port6 The UDP port that discovery will listen on over IPv6 if listening over both IPv4 and IPv6. Defaults to `port6` + --enable-mplex [] + Enables the mplex multiplexer alongside yamux. Yamux is preferred when + both are available. Enabled by default; set to "false" to disable. + [default: true] --enr-address
... The IP address/ DNS address to broadcast to other peers on how to reach this node. If a DNS address is provided, the enr-address is set @@ -489,9 +493,6 @@ Flags: Sets the local ENR IP address and port to match those set for lighthouse. Specifically, the IP address will be the value of --listen-address and the UDP port will be --discovery-port. - --enable-mplex - Enables mplex multiplexer alongside yamux. Yamux is preferred when - both are available. --enable-partial-columns Enable partial messages for data columns. This can reduce the amount of data sent over the network. Enabled by default on Hoodi and diff --git a/lighthouse/tests/beacon_node.rs b/lighthouse/tests/beacon_node.rs index 8bdab48282..d52558da93 100644 --- a/lighthouse/tests/beacon_node.rs +++ b/lighthouse/tests/beacon_node.rs @@ -2785,6 +2785,45 @@ fn invalid_block_roots_default_mainnet() { }) } +#[test] +fn enable_mplex_default() { + CommandLineTest::new() + .run_with_zero_port() + .with_config(|config| { + assert!(config.network.enable_mplex); + }) +} + +#[test] +fn enable_mplex_true() { + CommandLineTest::new() + .flag("enable-mplex", Some("true")) + .run_with_zero_port() + .with_config(|config| { + assert!(config.network.enable_mplex); + }) +} + +#[test] +fn enable_mplex_false() { + CommandLineTest::new() + .flag("enable-mplex", Some("false")) + .run_with_zero_port() + .with_config(|config| { + assert!(!config.network.enable_mplex); + }) +} + +#[test] +fn enable_mplex_no_value() { + CommandLineTest::new() + .flag("enable-mplex", None) + .run_with_zero_port() + .with_config(|config| { + assert!(config.network.enable_mplex); + }) +} + #[test] fn partial_columns() { CommandLineTest::new() From e0ff3b5709f9888c0c8fd36a67624d087e6de0a7 Mon Sep 17 00:00:00 2001 From: Mac L Date: Tue, 16 Jun 2026 10:54:11 +0400 Subject: [PATCH 09/15] Use `hashlink` over `lru` for `LruCache` (#8911) Use the `LruCache` implementation provided by `hashlink` instead of the current `lru` one. This is mostly a 1-to-1 swap with only slight API incompatibilities. I have decided to leave some config files which previously used `NonZeroUsize` but they may not be required anymore and could potentially switch to `usize`. Co-Authored-By: Mac L --- Cargo.lock | 23 +++----- Cargo.toml | 3 +- beacon_node/beacon_chain/Cargo.toml | 2 +- .../beacon_chain/src/beacon_proposer_cache.rs | 13 ++--- .../src/data_availability_checker.rs | 9 ++- .../overflow_lru_cache.rs | 21 +++---- .../beacon_chain/src/fetch_blobs/tests.rs | 3 +- .../src/light_client_server_cache.rs | 13 ++--- .../src/partial_data_column_assembler.rs | 57 ++++++++++--------- .../src/pending_payload_cache/mod.rs | 15 ++--- .../src/pre_finalization_cache.rs | 28 +++++---- beacon_node/client/src/builder.rs | 4 +- beacon_node/execution_layer/Cargo.toml | 2 +- beacon_node/execution_layer/src/engines.rs | 8 +-- .../execution_layer/src/payload_cache.rs | 10 ++-- beacon_node/http_api/Cargo.toml | 2 +- beacon_node/http_api/src/caches.rs | 7 +-- beacon_node/http_api/src/test_utils.rs | 4 +- beacon_node/lighthouse_network/Cargo.toml | 2 +- .../lighthouse_network/src/discovery/mod.rs | 16 +++--- .../service/partial_column_header_tracker.rs | 8 +-- beacon_node/store/Cargo.toml | 2 +- beacon_node/store/src/historic_state_cache.rs | 13 ++--- beacon_node/store/src/hot_cold_store.rs | 36 ++++++------ beacon_node/store/src/state_cache.rs | 43 +++++++------- deny.toml | 1 + slasher/Cargo.toml | 2 +- slasher/src/database.rs | 11 ++-- 28 files changed, 166 insertions(+), 192 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 223a6192c8..c0344bb7c8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -364,7 +364,7 @@ dependencies = [ "either", "futures", "futures-utils-wasm", - "lru 0.13.0", + "lru", "parking_lot", "pin-project", "reqwest", @@ -1229,13 +1229,13 @@ dependencies = [ "fork_choice", "futures", "genesis", + "hashlink", "hex", "int_to_bytes", "itertools 0.14.0", "kzg", "lighthouse_version", "logging", - "lru 0.12.5", "maplit", "merkle_proof", "metrics", @@ -3394,13 +3394,13 @@ dependencies = [ "fork_choice", "hash-db", "hash256-std-hasher", + "hashlink", "hex", "jsonwebtoken", "keccak-hash", "kzg", "lighthouse_version", "logging", - "lru 0.12.5", "metrics", "parking_lot", "pretty_reqwest_error", @@ -4240,12 +4240,12 @@ dependencies = [ "fixed_bytes", "futures", "genesis", + "hashlink", "health_metrics", "hex", "lighthouse_network", "lighthouse_version", "logging", - "lru 0.12.5", "metrics", "network", "network_utils", @@ -5549,6 +5549,7 @@ dependencies = [ "fixed_bytes", "fnv", "futures", + "hashlink", "hex", "if-addrs 0.14.0", "itertools 0.14.0", @@ -5556,7 +5557,6 @@ dependencies = [ "libp2p-mplex", "lighthouse_version", "logging", - "lru 0.12.5", "lru_cache", "metrics", "network_utils", @@ -5703,15 +5703,6 @@ dependencies = [ "thiserror 1.0.69", ] -[[package]] -name = "lru" -version = "0.12.5" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "234cf4f4a04dc1f57e24b96cc0cd600cf2af460d4161ac5ecdd0af8e1f3b2a38" -dependencies = [ - "hashbrown 0.15.5", -] - [[package]] name = "lru" version = "0.13.0" @@ -8415,10 +8406,10 @@ dependencies = [ "filesystem", "fixed_bytes", "flate2", + "hashlink", "libmdbx", "lmdb-rkv", "lmdb-rkv-sys", - "lru 0.12.5", "maplit", "metrics", "parking_lot", @@ -8652,10 +8643,10 @@ dependencies = [ "ethereum_ssz", "ethereum_ssz_derive", "fixed_bytes", + "hashlink", "itertools 0.14.0", "leveldb", "logging", - "lru 0.12.5", "metrics", "milhouse", "parking_lot", diff --git a/Cargo.toml b/Cargo.toml index 23bae317b4..6e0b1ddf3a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -152,7 +152,7 @@ fs2 = "0.4" futures = "0.3" genesis = { path = "beacon_node/genesis" } graffiti_file = { path = "validator_client/graffiti_file" } -hashlink = "0.9.0" +hashlink = "0.11" health_metrics = { path = "common/health_metrics" } hex = "0.4" http_api = { path = "beacon_node/http_api" } @@ -170,7 +170,6 @@ lockfile = { path = "common/lockfile" } log = "0.4" logging = { path = "common/logging" } logroller = "0.1.8" -lru = "0.12" lru_cache = { path = "common/lru_cache" } malloc_utils = { path = "common/malloc_utils" } maplit = "1" diff --git a/beacon_node/beacon_chain/Cargo.toml b/beacon_node/beacon_chain/Cargo.toml index 47ef4d7a03..236ce42abd 100644 --- a/beacon_node/beacon_chain/Cargo.toml +++ b/beacon_node/beacon_chain/Cargo.toml @@ -35,13 +35,13 @@ fixed_bytes = { workspace = true } fork_choice = { workspace = true } futures = { workspace = true } genesis = { workspace = true } +hashlink = { workspace = true } hex = { workspace = true } int_to_bytes = { workspace = true } itertools = { workspace = true } kzg = { workspace = true } lighthouse_version = { workspace = true } logging = { workspace = true } -lru = { workspace = true } merkle_proof = { workspace = true } metrics = { workspace = true } milhouse = { workspace = true } diff --git a/beacon_node/beacon_chain/src/beacon_proposer_cache.rs b/beacon_node/beacon_chain/src/beacon_proposer_cache.rs index b258d7471f..d35c9f003f 100644 --- a/beacon_node/beacon_chain/src/beacon_proposer_cache.rs +++ b/beacon_node/beacon_chain/src/beacon_proposer_cache.rs @@ -10,21 +10,19 @@ use crate::{BeaconChain, BeaconChainError, BeaconChainTypes}; use fork_choice::ExecutionStatus; -use lru::LruCache; +use hashlink::lru_cache::LruCache; use once_cell::sync::OnceCell; use parking_lot::Mutex; use safe_arith::SafeArith; use smallvec::SmallVec; use state_processing::state_advance::partial_state_advance; -use std::num::NonZeroUsize; use std::sync::Arc; use tracing::{debug, instrument}; use typenum::Unsigned; -use types::new_non_zero_usize; use types::{BeaconState, BeaconStateError, ChainSpec, Epoch, EthSpec, Fork, Hash256, Slot}; /// The number of sets of proposer indices that should be cached. -const CACHE_SIZE: NonZeroUsize = new_non_zero_usize(16); +const CACHE_SIZE: usize = 16; /// This value is fairly unimportant, it's used to avoid heap allocations. The result of it being /// incorrect is non-substantial from a consensus perspective (and probably also from a @@ -138,7 +136,8 @@ impl BeaconProposerCache { ) -> Arc> { let key = (epoch, shuffling_decision_block); self.cache - .get_or_insert(key, || Arc::new(OnceCell::new())) + .entry(key) + .or_insert_with(|| Arc::new(OnceCell::new())) .clone() } @@ -155,10 +154,10 @@ impl BeaconProposerCache { fork: Fork, ) -> Result<(), BeaconStateError> { let key = (epoch, shuffling_decision_block); - if !self.cache.contains(&key) { + if !self.cache.contains_key(&key) { let epoch_proposers = EpochBlockProposers::new(epoch, fork, proposers); self.cache - .put(key, Arc::new(OnceCell::with_value(epoch_proposers))); + .insert(key, Arc::new(OnceCell::with_value(epoch_proposers))); } Ok(()) diff --git a/beacon_node/beacon_chain/src/data_availability_checker.rs b/beacon_node/beacon_chain/src/data_availability_checker.rs index a0b117f072..29cbf84235 100644 --- a/beacon_node/beacon_chain/src/data_availability_checker.rs +++ b/beacon_node/beacon_chain/src/data_availability_checker.rs @@ -11,7 +11,6 @@ use slot_clock::SlotClock; use std::collections::HashSet; use std::fmt; use std::fmt::Debug; -use std::num::NonZeroUsize; use std::sync::Arc; use std::time::Duration; use task_executor::TaskExecutor; @@ -20,7 +19,7 @@ use types::data::{BlobIdentifier, FixedBlobSidecarList, PartialDataColumn}; use types::{ BlobSidecar, BlobSidecarList, BlockImportSource, ChainSpec, DataColumnSidecar, DataColumnSidecarList, Epoch, EthSpec, Hash256, PartialDataColumnSidecarError, - PartialDataColumnSidecarRef, SignedBeaconBlock, Slot, new_non_zero_usize, + PartialDataColumnSidecarRef, SignedBeaconBlock, Slot, }; mod error; @@ -49,7 +48,7 @@ pub use error::{Error as AvailabilityCheckError, ErrorCategory as AvailabilityCh /// /// `PendingComponents` are now never removed from the cache manually are only removed via LRU /// eviction to prevent race conditions (#7961), so we expect this cache to be full all the time. -const OVERFLOW_LRU_CAPACITY_NON_ZERO: NonZeroUsize = new_non_zero_usize(32); +const OVERFLOW_LRU_CAPACITY: usize = 32; /// Cache to hold fully valid data that can't be imported to fork-choice yet. After Dencun hard-fork /// blocks have a sidecar of data that is received separately from the network. We call the concept @@ -124,13 +123,13 @@ impl DataAvailabilityChecker { enable_partial_columns: bool, ) -> Result { let inner = DataAvailabilityCheckerInner::new( - OVERFLOW_LRU_CAPACITY_NON_ZERO, + OVERFLOW_LRU_CAPACITY, custody_context.clone(), spec.clone(), )?; let partial_assembler = if enable_partial_columns { Some(Arc::new(PartialDataColumnAssembler::new( - OVERFLOW_LRU_CAPACITY_NON_ZERO, + OVERFLOW_LRU_CAPACITY, ))) } else { None diff --git a/beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs b/beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs index 2254728850..47740cdf5e 100644 --- a/beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs +++ b/beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs @@ -7,11 +7,10 @@ use crate::block_verification_types::{ use crate::data_availability_checker::{Availability, AvailabilityCheckError}; use crate::data_column_verification::KzgVerifiedCustodyDataColumn; use crate::{BeaconChainTypes, BlockProcessStatus}; -use lru::LruCache; +use hashlink::lru_cache::LruCache; use parking_lot::{MappedRwLockReadGuard, RwLock, RwLockReadGuard, RwLockWriteGuard}; use ssz_types::RuntimeFixedVector; use std::cmp::Ordering; -use std::num::NonZeroUsize; use std::sync::Arc; use tracing::{Span, debug, debug_span}; use types::data::BlobIdentifier; @@ -365,7 +364,7 @@ pub(crate) enum ReconstructColumnsDecision { impl DataAvailabilityCheckerInner { pub fn new( - capacity: NonZeroUsize, + capacity: usize, custody_context: Arc>, spec: Arc, ) -> Result { @@ -565,7 +564,7 @@ impl DataAvailabilityCheckerInner { let mut write_lock = self.critical.write(); { - let pending_components = write_lock.get_or_insert_mut(block_root, || { + let pending_components = write_lock.entry(block_root).or_insert_with(|| { PendingComponents::empty(block_root, self.spec.max_blobs_per_block(epoch) as usize) }); update_fn(pending_components)? @@ -672,7 +671,7 @@ impl DataAvailabilityCheckerInner { if let Some(BlockProcessStatus::NotValidated(_, _)) = self.get_cached_block(block_root) { // If the block is execution invalid, this status is permanent and idempotent to this // block_root. We drop its components (e.g. columns) because they will never be useful. - self.critical.write().pop(block_root); + self.critical.write().remove(block_root); } } @@ -733,7 +732,7 @@ impl DataAvailabilityCheckerInner { } // Now remove keys for key in keys_to_remove { - write_lock.pop(&key); + write_lock.remove(&key); } Ok(()) @@ -765,7 +764,6 @@ mod test { use store::{HotColdDB, ItemStore, StoreConfig, database::interface::BeaconNodeBackend}; use tempfile::{TempDir, tempdir}; use tracing::info; - use types::new_non_zero_usize; use types::{DataColumnSubnetId, MinimalEthSpec}; const LOW_VALIDATOR_COUNT: usize = 32; @@ -930,19 +928,14 @@ mod test { let chain_db_path = tempdir().expect("should get temp dir"); let harness = get_fulu_chain(&chain_db_path).await; let spec = harness.spec.clone(); - let capacity_non_zero = new_non_zero_usize(capacity); let custody_context = Arc::new(CustodyContext::new( NodeCustodyType::Fullnode, generate_data_column_indices_rand_order::(), &spec, )); let cache = Arc::new( - DataAvailabilityCheckerInner::::new( - capacity_non_zero, - custody_context, - spec.clone(), - ) - .expect("should create cache"), + DataAvailabilityCheckerInner::::new(capacity, custody_context, spec.clone()) + .expect("should create cache"), ); (harness, cache, chain_db_path) } diff --git a/beacon_node/beacon_chain/src/fetch_blobs/tests.rs b/beacon_node/beacon_chain/src/fetch_blobs/tests.rs index 99cb4b5a78..4a37113fd9 100644 --- a/beacon_node/beacon_chain/src/fetch_blobs/tests.rs +++ b/beacon_node/beacon_chain/src/fetch_blobs/tests.rs @@ -9,7 +9,6 @@ use eth2::types::BlobsBundle; use execution_layer::json_structures::{BlobAndProof, BlobAndProofV1, BlobAndProofV2}; use execution_layer::test_utils::generate_blobs; use maplit::hashset; -use std::num::NonZeroUsize; use std::sync::{Arc, Mutex}; use task_executor::test_utils::TestRuntime; use types::{ @@ -339,7 +338,7 @@ fn mock_beacon_adapter(fork_name: ForkName, get_blobs_v3: bool) -> MockFetchBlob let test_runtime = TestRuntime::default(); let spec = Arc::new(fork_name.make_genesis_spec(E::default_spec())); let kzg = get_kzg(&spec); - let partial_assembler = PartialDataColumnAssembler::new(NonZeroUsize::new(32).unwrap()); + let partial_assembler = PartialDataColumnAssembler::new(32); let mut mock_adapter = MockFetchBlobsBeaconAdapter::default(); mock_adapter.expect_spec().return_const(spec.clone()); diff --git a/beacon_node/beacon_chain/src/light_client_server_cache.rs b/beacon_node/beacon_chain/src/light_client_server_cache.rs index 5b405234e7..d9d3fdd63e 100644 --- a/beacon_node/beacon_chain/src/light_client_server_cache.rs +++ b/beacon_node/beacon_chain/src/light_client_server_cache.rs @@ -1,15 +1,14 @@ use crate::errors::BeaconChainError; use crate::{BeaconChainTypes, BeaconStore, metrics}; +use hashlink::lru_cache::LruCache; use parking_lot::{Mutex, RwLock}; use safe_arith::SafeArith; use ssz::Decode; -use std::num::NonZeroUsize; use std::sync::Arc; use store::DBColumn; use store::KeyValueStore; use tracing::debug; use tree_hash::TreeHash; -use types::new_non_zero_usize; use types::{ BeaconBlockRef, BeaconState, ChainSpec, Checkpoint, EthSpec, ForkName, Hash256, LightClientBootstrap, LightClientFinalityUpdate, LightClientOptimisticUpdate, @@ -19,7 +18,7 @@ use types::{ /// A prev block cache miss requires to re-generate the state of the post-parent block. Items in the /// prev block cache are very small 32 * (6 + 1) = 224 bytes. 32 is an arbitrary number that /// represents unlikely re-orgs, while keeping the cache very small. -const PREV_BLOCK_CACHE_SIZE: NonZeroUsize = new_non_zero_usize(32); +const PREV_BLOCK_CACHE_SIZE: usize = 32; /// This cache computes light client messages ahead of time, required to satisfy p2p and API /// requests. These messages include proofs on historical states, so on-demand computation is @@ -39,7 +38,7 @@ pub struct LightClientServerCache { /// Caches the current sync committee, latest_written_current_sync_committee: RwLock>>>, /// Caches state proofs by block root - prev_block_cache: Mutex>>, + prev_block_cache: Mutex>>, /// Tracks the latest broadcasted finality update latest_broadcasted_finality_update: RwLock>>, /// Tracks the latest broadcasted optimistic update @@ -55,7 +54,7 @@ impl LightClientServerCache { latest_written_current_sync_committee: None.into(), latest_broadcasted_finality_update: None.into(), latest_broadcasted_optimistic_update: None.into(), - prev_block_cache: lru::LruCache::new(PREV_BLOCK_CACHE_SIZE).into(), + prev_block_cache: LruCache::new(PREV_BLOCK_CACHE_SIZE).into(), } } @@ -74,7 +73,7 @@ impl LightClientServerCache { if fork_name.altair_enabled() { // Persist in memory cache for a descendent block let cached_data = LightClientCachedData::from_state(block_post_state)?; - self.prev_block_cache.lock().put(block_root, cached_data); + self.prev_block_cache.lock().insert(block_root, cached_data); } Ok(()) @@ -335,7 +334,7 @@ impl LightClientServerCache { // Insert value and return owned self.prev_block_cache .lock() - .put(*block_root, new_value.clone()); + .insert(*block_root, new_value.clone()); Ok(new_value) } diff --git a/beacon_node/beacon_chain/src/partial_data_column_assembler.rs b/beacon_node/beacon_chain/src/partial_data_column_assembler.rs index ee59102cfd..f8580352b2 100644 --- a/beacon_node/beacon_chain/src/partial_data_column_assembler.rs +++ b/beacon_node/beacon_chain/src/partial_data_column_assembler.rs @@ -1,10 +1,9 @@ use crate::data_column_verification::{ KzgVerifiedCustodyDataColumn, KzgVerifiedCustodyPartialDataColumn, }; -use lru::LruCache; +use hashlink::lru_cache::LruCache; use parking_lot::RwLock; use std::collections::HashMap; -use std::num::NonZeroUsize; use std::sync::Arc; use tracing::error; use types::core::{Epoch, EthSpec, Hash256}; @@ -44,7 +43,7 @@ pub struct PartialMergeResult { } impl PartialDataColumnAssembler { - pub fn new(capacity: NonZeroUsize) -> Self { + pub fn new(capacity: usize) -> Self { Self { assemblies: RwLock::new(LruCache::new(capacity)), } @@ -55,7 +54,7 @@ impl PartialDataColumnAssembler { pub fn init(&self, block_root: Hash256, header: Arc>) -> bool { let mut assemblies = self.assemblies.write(); - if assemblies.contains(&block_root) { + if assemblies.contains_key(&block_root) { return false; } @@ -65,7 +64,7 @@ impl PartialDataColumnAssembler { columns: HashMap::new(), }; - assemblies.put(block_root, assembly); + assemblies.insert(block_root, assembly); true } @@ -79,11 +78,13 @@ impl PartialDataColumnAssembler { header: Arc>, ) -> Option> { let mut assemblies = self.assemblies.write(); - let assembly = assemblies.get_or_insert_mut(block_root, || PartialAssembly { - header: header.clone(), - has_local_blobs: false, - columns: HashMap::new(), - }); + let assembly = assemblies + .entry(block_root) + .or_insert_with(|| PartialAssembly { + header: header.clone(), + has_local_blobs: false, + columns: HashMap::new(), + }); let mut full_columns = Vec::new(); let mut updated_partials = Vec::new(); @@ -165,15 +166,17 @@ impl PartialDataColumnAssembler { }; let mut assemblies = self.assemblies.write(); - let assembly = assemblies.get_or_insert_mut(block_root, || PartialAssembly { - header: Arc::new(PartialDataColumnHeader { - kzg_commitments: fulu.kzg_commitments.clone(), - signed_block_header: fulu.signed_block_header.clone(), - kzg_commitments_inclusion_proof: fulu.kzg_commitments_inclusion_proof.clone(), - }), - has_local_blobs: false, - columns: Default::default(), - }); + let assembly = assemblies + .entry(block_root) + .or_insert_with(|| PartialAssembly { + header: Arc::new(PartialDataColumnHeader { + kzg_commitments: fulu.kzg_commitments.clone(), + signed_block_header: fulu.signed_block_header.clone(), + kzg_commitments_inclusion_proof: fulu.kzg_commitments_inclusion_proof.clone(), + }), + has_local_blobs: false, + columns: Default::default(), + }); let prev = assembly .columns .insert(column.index(), AssemblyColumn::Complete(column.clone())); @@ -215,11 +218,13 @@ impl PartialDataColumnAssembler { header: &Arc>, ) -> Vec> { let mut assemblies = self.assemblies.write(); - let assembly = assemblies.get_or_insert_mut(block_root, || PartialAssembly { - header: header.clone(), - has_local_blobs: true, - columns: Default::default(), - }); + let assembly = assemblies + .entry(block_root) + .or_insert_with(|| PartialAssembly { + header: header.clone(), + has_local_blobs: true, + columns: Default::default(), + }); assembly.has_local_blobs = true; @@ -253,7 +258,7 @@ impl PartialDataColumnAssembler { } for root in to_remove { - assemblies.pop(&root); + assemblies.remove(&root); } } } @@ -362,7 +367,7 @@ mod tests { } fn make_assembler() -> PartialDataColumnAssembler { - PartialDataColumnAssembler::new(NonZeroUsize::new(16).unwrap()) + PartialDataColumnAssembler::new(16) } // -- init and get_header tests -- diff --git a/beacon_node/beacon_chain/src/pending_payload_cache/mod.rs b/beacon_node/beacon_chain/src/pending_payload_cache/mod.rs index 2100a5fe9f..c5c97418c7 100644 --- a/beacon_node/beacon_chain/src/pending_payload_cache/mod.rs +++ b/beacon_node/beacon_chain/src/pending_payload_cache/mod.rs @@ -15,13 +15,12 @@ use crate::payload_envelope_verification::{ AvailabilityPendingExecutedEnvelope, AvailableExecutedEnvelope, }; use crate::{BeaconChainTypes, CustodyContext, metrics}; +use hashlink::lru_cache::LruCache; use kzg::Kzg; -use lru::LruCache; use parking_lot::{MappedRwLockReadGuard, RwLock, RwLockReadGuard, RwLockWriteGuard}; use std::collections::HashMap; use std::fmt; use std::fmt::Debug; -use std::num::NonZeroUsize; use std::sync::Arc; use tracing::{Span, debug, error, instrument}; use types::{ @@ -41,7 +40,6 @@ use crate::metrics::{ use crate::observed_data_sidecars::ObservationStrategy; use pending_components::{PendingComponents, ReconstructColumnsDecision}; use types::SignedExecutionPayloadBid; -use types::new_non_zero_usize; /// The LRU Cache stores `PendingComponents`, which store the block root, the execution payload bid, and its associated column data. /// The execution payload bid stores the kzg commitments which we use to verify against incoming column data. @@ -49,7 +47,7 @@ use types::new_non_zero_usize; /// /// `PendingComponents` are now never removed from the cache manually and are only removed via LRU /// eviction to prevent race conditions (#7961), so we expect this cache to be full all the time. -const AVAILABILITY_CACHE_CAPACITY: NonZeroUsize = new_non_zero_usize(32); +const AVAILABILITY_CACHE_CAPACITY: usize = 32; /// This type is returned after adding a bid / column to the `DataAvailabilityChecker`. /// @@ -206,7 +204,9 @@ impl PendingPayloadCache { /// This will silently drop the bid if a bid for this block root already exists in the cache. pub fn insert_bid(&self, block_root: Hash256, bid: Arc>) { let mut write_lock = self.availability_cache.write(); - write_lock.get_or_insert_mut(block_root, || PendingComponents::new(block_root, bid)); + write_lock + .entry(block_root) + .or_insert_with(|| PendingComponents::new(block_root, bid)); } /// Perform KZG verification on RPC custody columns and insert them into the cache. @@ -423,7 +423,8 @@ impl PendingPayloadCache { { let pending_components = write_lock - .get_or_insert_mut(block_root, || PendingComponents::new(block_root, bid)); + .entry(block_root) + .or_insert_with(|| PendingComponents::new(block_root, bid)); update_fn(pending_components) } @@ -496,7 +497,7 @@ impl PendingPayloadCache { } } for key in keys_to_remove { - write_lock.pop(&key); + write_lock.remove(&key); } Ok(()) diff --git a/beacon_node/beacon_chain/src/pre_finalization_cache.rs b/beacon_node/beacon_chain/src/pre_finalization_cache.rs index 54bd5c1940..b90d02db5e 100644 --- a/beacon_node/beacon_chain/src/pre_finalization_cache.rs +++ b/beacon_node/beacon_chain/src/pre_finalization_cache.rs @@ -1,15 +1,13 @@ use crate::{BeaconChain, BeaconChainError, BeaconChainTypes}; +use hashlink::lru_cache::LruCache; use itertools::process_results; -use lru::LruCache; use parking_lot::Mutex; -use std::num::NonZeroUsize; use std::time::Duration; use tracing::debug; use types::Hash256; -use types::new_non_zero_usize; -const BLOCK_ROOT_CACHE_LIMIT: NonZeroUsize = new_non_zero_usize(512); -const LOOKUP_LIMIT: NonZeroUsize = new_non_zero_usize(8); +const BLOCK_ROOT_CACHE_LIMIT: usize = 512; +const LOOKUP_LIMIT: usize = 8; const METRICS_TIMEOUT: Duration = Duration::from_millis(100); /// Cache for rejecting attestations to blocks from before finalization. @@ -49,13 +47,13 @@ impl BeaconChain { let mut cache = self.pre_finalization_block_cache.cache.lock(); // Check the cache to see if we already know this pre-finalization block root. - if cache.block_roots.contains(&block_root) { + if cache.block_roots.contains_key(&block_root) { return Ok(true); } // Avoid repeating the disk lookup for blocks that are already subject to a network lookup. // Sync will take care of de-duplicating the single block lookups. - if cache.in_progress_lookups.contains(&block_root) { + if cache.in_progress_lookups.contains_key(&block_root) { return Ok(false); } @@ -68,19 +66,19 @@ impl BeaconChain { .map_err(BeaconChainError::BeaconStateError) })?; if is_recent_finalized_block { - cache.block_roots.put(block_root, ()); + cache.block_roots.insert(block_root, ()); return Ok(true); } // 2. Check on disk. if self.store.get_blinded_block(&block_root)?.is_some() { - cache.block_roots.put(block_root, ()); + cache.block_roots.insert(block_root, ()); return Ok(true); } // 3. Check the network with a single block lookup. - cache.in_progress_lookups.put(block_root, ()); - if cache.in_progress_lookups.len() == LOOKUP_LIMIT.get() { + cache.in_progress_lookups.insert(block_root, ()); + if cache.in_progress_lookups.len() == LOOKUP_LIMIT { // NOTE: we expect this to occur sometimes if a lot of blocks that we look up fail to be // imported for reasons other than being pre-finalization. The cache will eventually // self-repair in this case by replacing old entries with new ones until all the failed @@ -95,8 +93,8 @@ impl BeaconChain { pub fn pre_finalization_block_rejected(&self, block_root: Hash256) { // Future requests can know that this block is invalid without having to look it up again. let mut cache = self.pre_finalization_block_cache.cache.lock(); - cache.in_progress_lookups.pop(&block_root); - cache.block_roots.put(block_root, ()); + cache.in_progress_lookups.remove(&block_root); + cache.block_roots.insert(block_root, ()); } } @@ -104,11 +102,11 @@ impl PreFinalizationBlockCache { pub fn block_processed(&self, block_root: Hash256) { // Future requests will find this block in fork choice, so no need to cache it in the // ongoing lookup cache any longer. - self.cache.lock().in_progress_lookups.pop(&block_root); + self.cache.lock().in_progress_lookups.remove(&block_root); } pub fn contains(&self, block_root: Hash256) -> bool { - self.cache.lock().block_roots.contains(&block_root) + self.cache.lock().block_roots.contains_key(&block_root) } pub fn metrics(&self) -> Option<(usize, usize)> { diff --git a/beacon_node/client/src/builder.rs b/beacon_node/client/src/builder.rs index 0a3c414632..1624f73e9b 100644 --- a/beacon_node/client/src/builder.rs +++ b/beacon_node/client/src/builder.rs @@ -36,7 +36,6 @@ use rand::SeedableRng; use rand::rngs::{OsRng, StdRng}; use slasher::Slasher; use slasher_service::SlasherService; -use std::num::NonZeroUsize; use std::path::{Path, PathBuf}; use std::sync::Arc; use std::time::Duration; @@ -641,8 +640,7 @@ where beacon_processor_send: Some(beacon_processor_channels.beacon_processor_tx.clone()), sse_logging_components: runtime_context.sse_logging_components.clone(), historical_committee_cache: Arc::new(http_api::HistoricalCommitteeCache::new( - NonZeroUsize::new(self.http_api_config.historical_committee_cache_size) - .unwrap_or(NonZeroUsize::MIN), + self.http_api_config.historical_committee_cache_size, )), }); diff --git a/beacon_node/execution_layer/Cargo.toml b/beacon_node/execution_layer/Cargo.toml index a23ea948e4..91eb74e621 100644 --- a/beacon_node/execution_layer/Cargo.toml +++ b/beacon_node/execution_layer/Cargo.toml @@ -20,13 +20,13 @@ fixed_bytes = { workspace = true } fork_choice = { workspace = true } hash-db = "0.15.2" hash256-std-hasher = "0.15.2" +hashlink = { workspace = true } hex = { workspace = true } jsonwebtoken = "9" keccak-hash = "0.10.0" kzg = { workspace = true } lighthouse_version = { workspace = true } logging = { workspace = true } -lru = { workspace = true } metrics = { workspace = true } parking_lot = { workspace = true } pretty_reqwest_error = { workspace = true } diff --git a/beacon_node/execution_layer/src/engines.rs b/beacon_node/execution_layer/src/engines.rs index 3e6f78abbe..aac170d48c 100644 --- a/beacon_node/execution_layer/src/engines.rs +++ b/beacon_node/execution_layer/src/engines.rs @@ -5,9 +5,8 @@ use crate::engine_api::{ PayloadId, }; use crate::{ClientVersionV1, HttpJsonRpc}; -use lru::LruCache; +use hashlink::lru_cache::LruCache; use std::future::Future; -use std::num::NonZeroUsize; use std::sync::Arc; use std::time::Duration; use task_executor::TaskExecutor; @@ -15,12 +14,11 @@ use tokio::sync::{Mutex, RwLock, watch}; use tokio_stream::wrappers::WatchStream; use tracing::{debug, error, info, warn}; use types::ExecutionBlockHash; -use types::new_non_zero_usize; /// The number of payload IDs that will be stored for each `Engine`. /// /// Since the size of each value is small (~800 bytes) a large number is used for safety. -const PAYLOAD_ID_LRU_CACHE_SIZE: NonZeroUsize = new_non_zero_usize(512); +const PAYLOAD_ID_LRU_CACHE_SIZE: usize = 512; const CACHED_RESPONSE_AGE_LIMIT: Duration = Duration::from_secs(900); // 15 minutes /// Stores the remembered state of a engine. @@ -175,7 +173,7 @@ impl Engine { if let Some(key) = payload_attributes .map(|pa| PayloadIdCacheKey::new(&forkchoice_state.head_block_hash, &pa)) { - self.payload_id_cache.lock().await.put(key, payload_id); + self.payload_id_cache.lock().await.insert(key, payload_id); } else { debug!(?payload_id, "Engine returned unexpected payload_id"); } diff --git a/beacon_node/execution_layer/src/payload_cache.rs b/beacon_node/execution_layer/src/payload_cache.rs index ce65a53ef1..958bf12dc2 100644 --- a/beacon_node/execution_layer/src/payload_cache.rs +++ b/beacon_node/execution_layer/src/payload_cache.rs @@ -1,12 +1,10 @@ use eth2::types::FullPayloadContents; -use lru::LruCache; +use hashlink::lru_cache::LruCache; use parking_lot::Mutex; -use std::num::NonZeroUsize; use tree_hash::TreeHash; -use types::new_non_zero_usize; use types::{EthSpec, Hash256}; -pub const DEFAULT_PAYLOAD_CACHE_SIZE: NonZeroUsize = new_non_zero_usize(10); +pub const DEFAULT_PAYLOAD_CACHE_SIZE: usize = 10; /// A cache mapping execution payloads by tree hash roots. pub struct PayloadCache { @@ -27,11 +25,11 @@ impl Default for PayloadCache { impl PayloadCache { pub fn put(&self, payload: FullPayloadContents) -> Option> { let root = payload.payload_ref().tree_hash_root(); - self.payloads.lock().put(PayloadCacheId(root), payload) + self.payloads.lock().insert(PayloadCacheId(root), payload) } pub fn pop(&self, root: &Hash256) -> Option> { - self.payloads.lock().pop(&PayloadCacheId(*root)) + self.payloads.lock().remove(&PayloadCacheId(*root)) } pub fn get(&self, hash: &Hash256) -> Option> { diff --git a/beacon_node/http_api/Cargo.toml b/beacon_node/http_api/Cargo.toml index dd15a76c7a..fb01f655d9 100644 --- a/beacon_node/http_api/Cargo.toml +++ b/beacon_node/http_api/Cargo.toml @@ -20,12 +20,12 @@ ethereum_ssz = { workspace = true } execution_layer = { workspace = true } fixed_bytes = { workspace = true } futures = { workspace = true } +hashlink = { workspace = true } health_metrics = { workspace = true } hex = { workspace = true } lighthouse_network = { workspace = true } lighthouse_version = { workspace = true } logging = { workspace = true } -lru = { workspace = true } metrics = { workspace = true } network = { workspace = true } network_utils = { workspace = true } diff --git a/beacon_node/http_api/src/caches.rs b/beacon_node/http_api/src/caches.rs index d92571594a..0f8c0ee6e0 100644 --- a/beacon_node/http_api/src/caches.rs +++ b/beacon_node/http_api/src/caches.rs @@ -1,6 +1,5 @@ -use lru::LruCache; +use hashlink::lru_cache::LruCache; use parking_lot::Mutex; -use std::num::NonZeroUsize; use std::sync::Arc; use types::{AttestationShufflingId, CommitteeCache, Epoch}; @@ -25,7 +24,7 @@ pub struct HistoricalCommitteeCache { } impl HistoricalCommitteeCache { - pub fn new(size: NonZeroUsize) -> Self { + pub fn new(size: usize) -> Self { Self { committees: Mutex::new(LruCache::new(size)), } @@ -38,6 +37,6 @@ impl HistoricalCommitteeCache { } pub fn insert(&self, id: HistoricalShufflingId, cache: Arc) { - self.committees.lock().put(id, cache); + self.committees.lock().insert(id, cache); } } diff --git a/beacon_node/http_api/src/test_utils.rs b/beacon_node/http_api/src/test_utils.rs index 467a5216b1..9a705e4162 100644 --- a/beacon_node/http_api/src/test_utils.rs +++ b/beacon_node/http_api/src/test_utils.rs @@ -22,10 +22,10 @@ use lighthouse_network::{ }; use network::{NetworkReceivers, NetworkSenders}; use sensitive_url::SensitiveUrl; +use std::future::Future; use std::net::SocketAddr; use std::sync::Arc; use std::time::Duration; -use std::{future::Future, num::NonZeroUsize}; use store::MemoryStore; use task_executor::test_utils::TestRuntime; use types::{ChainSpec, EthSpec}; @@ -294,7 +294,7 @@ pub async fn create_api_server_with_config( beacon_processor_send: Some(beacon_processor_send), sse_logging_components: None, historical_committee_cache: Arc::new(HistoricalCommitteeCache::new( - NonZeroUsize::new(http_config.historical_committee_cache_size).unwrap(), + http_config.historical_committee_cache_size, )), }); diff --git a/beacon_node/lighthouse_network/Cargo.toml b/beacon_node/lighthouse_network/Cargo.toml index 659886f0f1..f69f13612a 100644 --- a/beacon_node/lighthouse_network/Cargo.toml +++ b/beacon_node/lighthouse_network/Cargo.toml @@ -21,6 +21,7 @@ ethereum_ssz_derive = { workspace = true } fixed_bytes = { workspace = true } fnv = { workspace = true } futures = { workspace = true } +hashlink = { workspace = true } hex = { workspace = true } if-addrs = "0.14" itertools = { workspace = true } @@ -28,7 +29,6 @@ libp2p = { workspace = true } libp2p-mplex = { git = "https://github.com/libp2p/rust-libp2p.git" } lighthouse_version = { workspace = true } logging = { workspace = true } -lru = { workspace = true } lru_cache = { workspace = true } metrics = { workspace = true } network_utils = { workspace = true } diff --git a/beacon_node/lighthouse_network/src/discovery/mod.rs b/beacon_node/lighthouse_network/src/discovery/mod.rs index 21b1146aff..964c0cd5fb 100644 --- a/beacon_node/lighthouse_network/src/discovery/mod.rs +++ b/beacon_node/lighthouse_network/src/discovery/mod.rs @@ -18,6 +18,7 @@ use alloy_rlp::bytes::Bytes; use enr::{ATTESTATION_BITFIELD_ENR_KEY, ETH2_ENR_KEY, SYNC_COMMITTEE_BITFIELD_ENR_KEY}; use futures::prelude::*; use futures::stream::FuturesUnordered; +use hashlink::lru_cache::LruCache; use libp2p::core::transport::PortUse; use libp2p::multiaddr::Protocol; use libp2p::swarm::THandlerInEvent; @@ -31,10 +32,8 @@ pub use libp2p::{ }, }; use logging::crit; -use lru::LruCache; use network_utils::discovery_metrics; use ssz::Encode; -use std::num::NonZeroUsize; use std::{ collections::{HashMap, VecDeque}, net::{IpAddr, SocketAddr}, @@ -51,7 +50,6 @@ use types::{ChainSpec, EnrForkId, EthSpec}; mod subnet_predicate; use crate::discovery::enr::{NEXT_FORK_DIGEST_ENR_KEY, PEERDAS_CUSTODY_GROUP_COUNT_ENR_KEY}; pub use subnet_predicate::subnet_predicate; -use types::new_non_zero_usize; /// Local ENR storage filename. pub const ENR_FILENAME: &str = "enr.dat"; @@ -74,7 +72,7 @@ pub const FIND_NODE_QUERY_CLOSEST_PEERS: usize = 16; /// The threshold for updating `min_ttl` on a connected peer. const DURATION_DIFFERENCE: Duration = Duration::from_millis(1); /// The capacity of the Discovery ENR cache. -const ENR_CACHE_CAPACITY: NonZeroUsize = new_non_zero_usize(50); +const ENR_CACHE_CAPACITY: usize = 50; /// A query has completed. This result contains a mapping of discovered peer IDs to the `min_ttl` /// of the peer if it is specified. @@ -358,7 +356,7 @@ impl Discovery { /// Removes a cached ENR from the list. pub fn remove_cached_enr(&mut self, peer_id: &PeerId) -> Option { - self.cached_enrs.pop(peer_id) + self.cached_enrs.remove(peer_id) } /// This adds a new `FindPeers` query to the queue if one doesn't already exist. @@ -394,7 +392,7 @@ impl Discovery { /// Add an ENR to the routing table of the discovery mechanism. pub fn add_enr(&mut self, enr: Enr) { // add the enr to seen caches - self.cached_enrs.put(enr.peer_id(), enr.clone()); + self.cached_enrs.insert(enr.peer_id(), enr.clone()); if let Err(e) = self.discv5.add_enr(enr) { debug!( @@ -665,7 +663,7 @@ impl Discovery { } // Remove the peer from the cached list, to prevent redialing disconnected // peers. - self.cached_enrs.pop(peer_id); + self.cached_enrs.remove(peer_id); } /* Internal Functions */ @@ -875,7 +873,7 @@ impl Discovery { .into_iter() .map(|enr| { // cache the found ENR's - self.cached_enrs.put(enr.peer_id(), enr.clone()); + self.cached_enrs.insert(enr.peer_id(), enr.clone()); (enr, None) }) .collect(); @@ -910,7 +908,7 @@ impl Discovery { // cache the found ENR's for enr in r.iter().cloned() { - self.cached_enrs.put(enr.peer_id(), enr); + self.cached_enrs.insert(enr.peer_id(), enr); } // Map each subnet query's min_ttl to the set of ENR's returned for that subnet. diff --git a/beacon_node/lighthouse_network/src/service/partial_column_header_tracker.rs b/beacon_node/lighthouse_network/src/service/partial_column_header_tracker.rs index bb588fe3d8..309674a885 100644 --- a/beacon_node/lighthouse_network/src/service/partial_column_header_tracker.rs +++ b/beacon_node/lighthouse_network/src/service/partial_column_header_tracker.rs @@ -1,12 +1,11 @@ use crate::types::HeaderSentSet; -use lru::LruCache; +use hashlink::lru_cache::LruCache; use parking_lot::Mutex; use std::collections::HashSet; -use std::num::NonZeroUsize; use std::sync::Arc; use types::core::Hash256; -const MAX_BLOCKS: NonZeroUsize = NonZeroUsize::new(4).unwrap(); +const MAX_BLOCKS: usize = 4; pub struct PartialColumnHeaderTracker { blocks: LruCache, @@ -22,7 +21,8 @@ impl PartialColumnHeaderTracker { pub fn get_for_block(&mut self, hash: Hash256) -> HeaderSentSet { Arc::clone( self.blocks - .get_or_insert(hash, || Arc::new(Mutex::new(HashSet::new()))), + .entry(hash) + .or_insert_with(|| Arc::new(Mutex::new(HashSet::new()))), ) } } diff --git a/beacon_node/store/Cargo.toml b/beacon_node/store/Cargo.toml index 50028fe73f..5f810ea76b 100644 --- a/beacon_node/store/Cargo.toml +++ b/beacon_node/store/Cargo.toml @@ -16,10 +16,10 @@ directory = { workspace = true } ethereum_ssz = { workspace = true } ethereum_ssz_derive = { workspace = true } fixed_bytes = { workspace = true } +hashlink = { workspace = true } itertools = { workspace = true } leveldb = { version = "0.8.6", optional = true, default-features = false } logging = { workspace = true } -lru = { workspace = true } metrics = { workspace = true } milhouse = { workspace = true } parking_lot = { workspace = true } diff --git a/beacon_node/store/src/historic_state_cache.rs b/beacon_node/store/src/historic_state_cache.rs index e5abb04c07..02eb68ad70 100644 --- a/beacon_node/store/src/historic_state_cache.rs +++ b/beacon_node/store/src/historic_state_cache.rs @@ -1,7 +1,6 @@ use crate::hdiff::{Error, HDiffBuffer}; use crate::metrics; -use lru::LruCache; -use std::num::NonZeroUsize; +use hashlink::lru_cache::LruCache; use types::{BeaconState, ChainSpec, EthSpec, Slot}; /// Holds a combination of finalized states in two formats: @@ -25,7 +24,7 @@ pub struct Metrics { } impl HistoricStateCache { - pub fn new(hdiff_buffer_cache_size: NonZeroUsize, state_cache_size: NonZeroUsize) -> Self { + pub fn new(hdiff_buffer_cache_size: usize, state_cache_size: usize) -> Self { Self { hdiff_buffers: LruCache::new(hdiff_buffer_cache_size), states: LruCache::new(state_cache_size), @@ -47,7 +46,7 @@ impl HistoricStateCache { ); let cloned = buffer.clone(); drop(_timer); - self.hdiff_buffers.put(slot, cloned); + self.hdiff_buffers.insert(slot, cloned); Some(buffer) } else { None @@ -63,7 +62,7 @@ impl HistoricStateCache { Ok(Some(state.clone())) } else if let Some(buffer) = self.hdiff_buffers.get(&slot) { let state = buffer.as_state(spec)?; - self.states.put(slot, state.clone()); + self.states.insert(slot, state.clone()); Ok(Some(state)) } else { Ok(None) @@ -71,11 +70,11 @@ impl HistoricStateCache { } pub fn put_state(&mut self, slot: Slot, state: BeaconState) { - self.states.put(slot, state); + self.states.insert(slot, state); } pub fn put_hdiff_buffer(&mut self, slot: Slot, buffer: HDiffBuffer) { - self.hdiff_buffers.put(slot, buffer); + self.hdiff_buffers.insert(slot, buffer); } pub fn put_both(&mut self, slot: Slot, state: BeaconState, buffer: HDiffBuffer) { diff --git a/beacon_node/store/src/hot_cold_store.rs b/beacon_node/store/src/hot_cold_store.rs index a625a97004..7484c271ae 100644 --- a/beacon_node/store/src/hot_cold_store.rs +++ b/beacon_node/store/src/hot_cold_store.rs @@ -19,8 +19,8 @@ use crate::{ parse_data_column_key, }; use fixed_bytes::FixedBytesExtended; +use hashlink::lru_cache::LruCache; use itertools::{Itertools, process_results}; -use lru::LruCache; use parking_lot::{Mutex, RwLock}; use safe_arith::SafeArith; use serde::{Deserialize, Serialize}; @@ -34,7 +34,6 @@ use std::cmp::{Ordering, min}; use std::collections::{HashMap, HashSet}; use std::io::{Read, Write}; use std::marker::PhantomData; -use std::num::NonZeroUsize; use std::path::Path; use std::sync::Arc; use std::time::Duration; @@ -97,7 +96,7 @@ struct BlockCache { } impl BlockCache { - pub fn new(size: NonZeroUsize) -> Self { + pub fn new(size: usize) -> Self { Self { block_cache: LruCache::new(size), blob_cache: LruCache::new(size), @@ -106,14 +105,15 @@ impl BlockCache { } } pub fn put_block(&mut self, block_root: Hash256, block: SignedBeaconBlock) { - self.block_cache.put(block_root, block); + self.block_cache.insert(block_root, block); } pub fn put_blobs(&mut self, block_root: Hash256, blobs: BlobSidecarList) { - self.blob_cache.put(block_root, blobs); + self.blob_cache.insert(block_root, blobs); } pub fn put_data_column(&mut self, block_root: Hash256, data_column: Arc>) { self.data_column_cache - .get_or_insert_mut(block_root, Default::default) + .entry(block_root) + .or_insert_with(Default::default) .insert(*data_column.index(), data_column); } pub fn put_data_column_custody_info( @@ -143,13 +143,13 @@ impl BlockCache { self.data_column_custody_info_cache.clone() } pub fn delete_block(&mut self, block_root: &Hash256) { - let _ = self.block_cache.pop(block_root); + let _ = self.block_cache.remove(block_root); } pub fn delete_blobs(&mut self, block_root: &Hash256) { - let _ = self.blob_cache.pop(block_root); + let _ = self.blob_cache.remove(block_root); } pub fn delete_data_columns(&mut self, block_root: &Hash256) { - let _ = self.data_column_cache.pop(block_root); + let _ = self.data_column_cache.remove(block_root); } pub fn delete(&mut self, block_root: &Hash256) { self.delete_block(block_root); @@ -236,17 +236,16 @@ impl HotColdDB { cold_db: MemoryStore::open(), blobs_db: MemoryStore::open(), hot_db: MemoryStore::open(), - block_cache: NonZeroUsize::new(config.block_cache_size) - .map(BlockCache::new) - .map(Mutex::new), + block_cache: (config.block_cache_size > 0) + .then(|| Mutex::new(BlockCache::new(config.block_cache_size))), state_cache: Mutex::new(StateCache::new( config.state_cache_size, config.state_cache_headroom, config.hot_hdiff_buffer_cache_size, )), historic_state_cache: Mutex::new(HistoricStateCache::new( - config.cold_hdiff_buffer_cache_size, - config.historic_state_cache_size, + config.cold_hdiff_buffer_cache_size.get(), + config.historic_state_cache_size.get(), )), config, hierarchy, @@ -290,17 +289,16 @@ impl HotColdDB { blobs_db: BeaconNodeBackend::open(&config, blobs_db_path)?, cold_db: BeaconNodeBackend::open(&config, cold_path)?, hot_db, - block_cache: NonZeroUsize::new(config.block_cache_size) - .map(BlockCache::new) - .map(Mutex::new), + block_cache: (config.block_cache_size > 0) + .then(|| Mutex::new(BlockCache::new(config.block_cache_size))), state_cache: Mutex::new(StateCache::new( config.state_cache_size, config.state_cache_headroom, config.hot_hdiff_buffer_cache_size, )), historic_state_cache: Mutex::new(HistoricStateCache::new( - config.cold_hdiff_buffer_cache_size, - config.historic_state_cache_size, + config.cold_hdiff_buffer_cache_size.get(), + config.historic_state_cache_size.get(), )), config, hierarchy, diff --git a/beacon_node/store/src/state_cache.rs b/beacon_node/store/src/state_cache.rs index 6d159c9361..9a1f2524c1 100644 --- a/beacon_node/store/src/state_cache.rs +++ b/beacon_node/store/src/state_cache.rs @@ -3,7 +3,7 @@ use crate::{ Error, metrics::{self, HOT_METRIC}, }; -use lru::LruCache; +use hashlink::lru_cache::LruCache; use std::collections::{BTreeMap, HashMap, HashSet}; use std::num::NonZeroUsize; use tracing::instrument; @@ -86,9 +86,9 @@ impl StateCache { ) -> Self { StateCache { finalized_state: None, - states: LruCache::new(state_capacity), + states: LruCache::new(state_capacity.get()), block_map: BlockMap::default(), - hdiff_buffers: HotHDiffBufferCache::new(hdiff_capacity), + hdiff_buffers: HotHDiffBufferCache::new(hdiff_capacity.get()), max_epoch: Epoch::new(0), head_block_root: Hash256::ZERO, headroom, @@ -100,7 +100,7 @@ impl StateCache { } pub fn capacity(&self) -> usize { - self.states.cap().get() + self.states.capacity() } pub fn num_hdiff_buffers(&self) -> usize { @@ -154,7 +154,7 @@ impl StateCache { // preferences older slots. // NOTE: This isn't perfect as it prunes by slot: there could be multiple buffers // at some slots in the case of long forks without finality. - let new_hdiff_cache = HotHDiffBufferCache::new(self.hdiff_buffers.cap()); + let new_hdiff_cache = HotHDiffBufferCache::new(self.hdiff_buffers.capacity()); let old_hdiff_cache = std::mem::replace(&mut self.hdiff_buffers, new_hdiff_cache); for (state_root, (slot, buffer)) in old_hdiff_cache.hdiff_buffers { if pre_finalized_slots_to_retain.contains(&slot) { @@ -164,7 +164,7 @@ impl StateCache { // Delete states. for state_root in state_roots_to_prune { - if let Some((_, state)) = self.states.pop(&state_root) { + if let Some((_, state)) = self.states.remove(&state_root) { // Add the hdiff buffer for this state to the hdiff cache if it is now part of // the pre-finalized grid. The `put` method will take care of keeping the most // useful buffers. @@ -260,7 +260,7 @@ impl StateCache { // Insert the full state into the cache. if let Some((deleted_state_root, _)) = - self.states.put(state_root, (state_root, state.clone())) + self.states.insert(state_root, (state_root, state.clone())) { deleted_states.push(deleted_state_root); } @@ -334,14 +334,14 @@ impl StateCache { } pub fn delete_state(&mut self, state_root: &Hash256) { - self.states.pop(state_root); + self.states.remove(state_root); self.block_map.delete(state_root); } pub fn delete_block_states(&mut self, block_root: &Hash256) { if let Some(slot_map) = self.block_map.delete_block_states(block_root) { for state_root in slot_map.slots.values() { - self.states.pop(state_root); + self.states.remove(state_root); } } } @@ -366,9 +366,10 @@ impl StateCache { let mut old_boundary_state_roots = vec![]; let mut good_boundary_state_roots = vec![]; - // Skip the `cull_exempt` most-recently used, then reverse the iterator to start at - // least-recently used states. - for (&state_root, (_, state)) in self.states.iter().skip(cull_exempt).rev() { + // Start at the least-recently used states, excluding the `cull_exempt` most-recently + // used (which are the final entries of the iterator). + let num_cull_candidates = self.states.len().saturating_sub(cull_exempt); + for (&state_root, (_, state)) in self.states.iter().take(num_cull_candidates) { let is_advanced = state.slot() > state.latest_block_header().slot; let is_boundary = state.slot() % E::slots_per_epoch() == 0; let could_finalize = @@ -450,7 +451,7 @@ impl BlockMap { } impl HotHDiffBufferCache { - pub fn new(capacity: NonZeroUsize) -> Self { + pub fn new(capacity: usize) -> Self { Self { hdiff_buffers: LruCache::new(capacity), } @@ -467,8 +468,8 @@ impl HotHDiffBufferCache { /// If the value was inserted then `true` is returned. pub fn put(&mut self, state_root: Hash256, slot: Slot, buffer: HDiffBuffer) -> bool { // If the cache is not full, simply insert the value. - if self.hdiff_buffers.len() != self.hdiff_buffers.cap().get() { - self.hdiff_buffers.put(state_root, (slot, buffer)); + if self.hdiff_buffers.len() != self.hdiff_buffers.capacity() { + self.hdiff_buffers.insert(state_root, (slot, buffer)); return true; } @@ -484,23 +485,23 @@ impl HotHDiffBufferCache { return false; }; - if self.hdiff_buffers.cap().get() > 1 || slot < min_slot { + if self.hdiff_buffers.capacity() > 1 || slot < min_slot { // Remove LRU value. Cache is now at size `cap - 1`. let Some((removed_state_root, (removed_slot, removed_buffer))) = - self.hdiff_buffers.pop_lru() + self.hdiff_buffers.remove_lru() else { // Unreachable: cache is full so should have at least one entry to pop. return false; }; // Insert new value. Cache size is now at size `cap`. - self.hdiff_buffers.put(state_root, (slot, buffer)); + self.hdiff_buffers.insert(state_root, (slot, buffer)); // If the removed value had the min slot and we didn't intend to replace it (cap=1) // then we reinsert it. if removed_slot == min_slot && slot >= min_slot { self.hdiff_buffers - .put(removed_state_root, (removed_slot, removed_buffer)); + .insert(removed_state_root, (removed_slot, removed_buffer)); } true } else { @@ -509,8 +510,8 @@ impl HotHDiffBufferCache { } } - pub fn cap(&self) -> NonZeroUsize { - self.hdiff_buffers.cap() + pub fn capacity(&self) -> usize { + self.hdiff_buffers.capacity() } #[allow(clippy::len_without_is_empty)] diff --git a/deny.toml b/deny.toml index 015f2ec88b..5a8691fd5c 100644 --- a/deny.toml +++ b/deny.toml @@ -21,6 +21,7 @@ deny = [ { crate = "scrypt", deny-multiple-versions = true, reason = "takes a long time to compile" }, { crate = "syn", deny-multiple-versions = true, reason = "takes a long time to compile" }, { crate = "uuid", deny-multiple-versions = true, reason = "dependency hygiene" }, + { crate = "lru", deny-multiple-versions = true, reason = "use hashlink instead" }, ] [sources] diff --git a/slasher/Cargo.toml b/slasher/Cargo.toml index a068b2e885..83cfb4861e 100644 --- a/slasher/Cargo.toml +++ b/slasher/Cargo.toml @@ -22,9 +22,9 @@ ethereum_ssz_derive = { workspace = true } filesystem = { workspace = true } fixed_bytes = { workspace = true } flate2 = { version = "1.0.14", features = ["zlib"], default-features = false } +hashlink = { workspace = true } lmdb-rkv = { git = "https://github.com/sigp/lmdb-rs", rev = "f33845c6469b94265319aac0ed5085597862c27e", optional = true } lmdb-rkv-sys = { git = "https://github.com/sigp/lmdb-rs", rev = "f33845c6469b94265319aac0ed5085597862c27e", optional = true } -lru = { workspace = true } # MDBX is pinned at the last version with Windows and macOS support. mdbx = { package = "libmdbx", git = "https://github.com/sigp/libmdbx-rs", rev = "e6ff4b9377c1619bcf0bfdf52bee5a980a432a1a", optional = true } diff --git a/slasher/src/database.rs b/slasher/src/database.rs index 80d073a81c..ed8079689b 100644 --- a/slasher/src/database.rs +++ b/slasher/src/database.rs @@ -9,8 +9,8 @@ use crate::{ }; use bls::AggregateSignature; use byteorder::{BigEndian, ByteOrder}; +use hashlink::lru_cache::LruCache; use interface::{Environment, OpenDatabases, RwTransaction}; -use lru::LruCache; use parking_lot::Mutex; use serde::de::DeserializeOwned; use ssz::{Decode, Encode}; @@ -305,7 +305,8 @@ impl SlasherDB { } } - let attestation_root_cache = Mutex::new(LruCache::new(config.attestation_root_cache_size)); + let attestation_root_cache = + Mutex::new(LruCache::new(config.attestation_root_cache_size.get())); let mut db = Self { env, @@ -559,7 +560,7 @@ impl SlasherDB { let indexed_attestation = self.get_indexed_attestation(txn, indexed_id)?; let attestation_data_root = indexed_attestation.data().tree_hash_root(); - cache.put(indexed_id, attestation_data_root); + cache.insert(indexed_id, attestation_data_root); Ok((attestation_data_root, Some(indexed_attestation))) } @@ -570,13 +571,13 @@ impl SlasherDB { attestation_data_root: Hash256, ) { let mut cache = self.attestation_root_cache.lock(); - cache.put(indexed_attestation_id, attestation_data_root); + cache.insert(indexed_attestation_id, attestation_data_root); } fn delete_attestation_data_roots(&self, ids: impl IntoIterator) { let mut cache = self.attestation_root_cache.lock(); for indexed_id in ids { - cache.pop(&indexed_id); + cache.remove(&indexed_id); } } From 9de2e9e6e1c148d90009130afcf5a11d51f3350a Mon Sep 17 00:00:00 2001 From: Daniel Knopik <107140945+dknopik@users.noreply.github.com> Date: Wed, 17 Jun 2026 01:54:00 +0200 Subject: [PATCH 10/15] Make single block lookup respect `earliest_available_slot` for column requests (#9447) Single block lookups do not respect the `earliest_available_slot` peers sent. This causes us to potentially request columns from peers that do not custody columns yet (but will soon). Pass down the block's slot and only consider peers where `earliest_available_slot <= block_slot` for custody column requests. Co-Authored-By: Daniel Knopik --- .../src/peer_manager/peerdb.rs | 113 ++++++++++++++---- .../src/peer_manager/peerdb/peer_info.rs | 10 +- .../lighthouse_network/src/types/globals.rs | 19 +-- .../network/src/sync/network_context.rs | 5 +- .../src/sync/network_context/custody.rs | 7 +- 5 files changed, 121 insertions(+), 33 deletions(-) diff --git a/beacon_node/lighthouse_network/src/peer_manager/peerdb.rs b/beacon_node/lighthouse_network/src/peer_manager/peerdb.rs index 0a338bb011..693fdebb69 100644 --- a/beacon_node/lighthouse_network/src/peer_manager/peerdb.rs +++ b/beacon_node/lighthouse_network/src/peer_manager/peerdb.rs @@ -257,17 +257,9 @@ impl PeerDB { .iter() .filter(move |(_, info)| { info.is_connected() - && match info.sync_status() { - SyncStatus::Synced { info } => { - info.has_slot(epoch.start_slot(E::slots_per_epoch())) - } - SyncStatus::Advanced { info } => { - info.has_slot(epoch.start_slot(E::slots_per_epoch())) - } - SyncStatus::IrrelevantPeer - | SyncStatus::Behind { .. } - | SyncStatus::Unknown => false, - } + && info.is_synced_or_advanced_with_available_slot( + epoch.start_slot(E::slots_per_epoch()), + ) }) .map(|(peer_id, _)| peer_id) } @@ -301,10 +293,11 @@ impl PeerDB { } /// Returns an iterator of all good gossipsub peers that are supposed to be custodying - /// the given subnet id. + /// the given subnet id, with data available at the given slot. pub fn good_custody_subnet_peer( &self, subnet: DataColumnSubnetId, + slot: Slot, ) -> impl Iterator { self.peers .iter() @@ -314,7 +307,7 @@ impl PeerDB { info.is_connected() && info.is_good_gossipsub_peer() && is_custody_subnet_peer - && info.is_synced_or_advanced() + && info.is_synced_or_advanced_with_available_slot(slot) }) .map(|(peer_id, _)| peer_id) } @@ -330,14 +323,9 @@ impl PeerDB { let good_sync_peers_for_epoch = self.peers.values().filter(|&info| { info.is_connected() - && match info.sync_status() { - SyncStatus::Synced { info } | SyncStatus::Advanced { info } => { - info.has_slot(epoch.start_slot(E::slots_per_epoch())) - } - SyncStatus::IrrelevantPeer - | SyncStatus::Behind { .. } - | SyncStatus::Unknown => false, - } + && info.is_synced_or_advanced_with_available_slot( + epoch.start_slot(E::slots_per_epoch()), + ) }); for info in good_sync_peers_for_epoch { @@ -2211,6 +2199,89 @@ mod tests { ); } + #[test] + fn test_good_custody_subnet_peer_respects_earliest_available_slot() { + let mut pdb = get_db(); + let subnet = DataColumnSubnetId::new(0); + let request_slot = Slot::new(10); + + fn sync_info(earliest_available_slot: Option) -> SyncInfo { + SyncInfo { + head_slot: Slot::new(100), + head_root: Hash256::ZERO, + finalized_epoch: Epoch::new(0), + finalized_root: Hash256::ZERO, + earliest_available_slot, + } + } + + let add_custody_peer = |pdb: &mut PeerDB, sync_status: SyncStatus| { + let peer_id = PeerId::random(); + pdb.connect_ingoing(&peer_id, "/ip4/0.0.0.0".parse().unwrap(), None); + pdb.__set_custody_subnets(&peer_id, HashSet::from([subnet])) + .unwrap(); + pdb.update_sync_status(&peer_id, sync_status); + peer_id + }; + + let peer_with_data = add_custody_peer( + &mut pdb, + SyncStatus::Synced { + info: sync_info(Some(Slot::new(5))), + }, + ); + let peer_at_boundary = add_custody_peer( + &mut pdb, + SyncStatus::Synced { + info: sync_info(Some(request_slot)), + }, + ); + let peer_pruned = add_custody_peer( + &mut pdb, + SyncStatus::Synced { + info: sync_info(Some(Slot::new(11))), + }, + ); + let peer_no_eas = add_custody_peer( + &mut pdb, + SyncStatus::Synced { + info: sync_info(None), + }, + ); + let peer_behind = add_custody_peer( + &mut pdb, + SyncStatus::Behind { + info: sync_info(Some(Slot::new(0))), + }, + ); + + let good_peers = pdb + .good_custody_subnet_peer(subnet, request_slot) + .copied() + .collect::>(); + + assert!( + good_peers.contains(&peer_with_data), + "peer with earliest_available_slot before the request slot should be returned" + ); + assert!( + good_peers.contains(&peer_at_boundary), + "peer with earliest_available_slot equal to the request slot should be returned" + ); + assert!( + !good_peers.contains(&peer_pruned), + "peer with earliest_available_slot after the request slot should be excluded" + ); + assert!( + good_peers.contains(&peer_no_eas), + "peer without an advertised earliest_available_slot should be returned" + ); + assert!( + !good_peers.contains(&peer_behind), + "behind peer should be excluded regardless of earliest_available_slot" + ); + } + #[test] fn test_disable_peer_scoring() { let peer = PeerId::random(); diff --git a/beacon_node/lighthouse_network/src/peer_manager/peerdb/peer_info.rs b/beacon_node/lighthouse_network/src/peer_manager/peerdb/peer_info.rs index 8ad7d10a88..658a6355e3 100644 --- a/beacon_node/lighthouse_network/src/peer_manager/peerdb/peer_info.rs +++ b/beacon_node/lighthouse_network/src/peer_manager/peerdb/peer_info.rs @@ -15,7 +15,7 @@ use std::collections::HashSet; use std::net::IpAddr; use std::time::Instant; use strum::AsRefStr; -use types::{DataColumnSubnetId, EthSpec}; +use types::{DataColumnSubnetId, EthSpec, Slot}; /// Information about a given connected peer. #[derive(Clone, Debug, Serialize)] @@ -339,6 +339,14 @@ impl PeerInfo { ) } + /// Checks if the peer is synced or advanced, and has data available for the given slot. + pub fn is_synced_or_advanced_with_available_slot(&self, slot: Slot) -> bool { + match &self.sync_status { + SyncStatus::Synced { info } | SyncStatus::Advanced { info } => info.has_slot(slot), + SyncStatus::IrrelevantPeer | SyncStatus::Behind { .. } | SyncStatus::Unknown => false, + } + } + /// Checks if the status is connected. pub fn is_dialing(&self) -> bool { matches!(self.connection_status, PeerConnectionStatus::Dialing { .. }) diff --git a/beacon_node/lighthouse_network/src/types/globals.rs b/beacon_node/lighthouse_network/src/types/globals.rs index df8dbdc559..1f770a5847 100644 --- a/beacon_node/lighthouse_network/src/types/globals.rs +++ b/beacon_node/lighthouse_network/src/types/globals.rs @@ -11,7 +11,7 @@ use std::collections::HashSet; use std::sync::Arc; use tracing::{debug, error}; use types::data::{compute_subnets_from_custody_group, get_custody_groups}; -use types::{ChainSpec, ColumnIndex, DataColumnSubnetId, EthSpec}; +use types::{ChainSpec, ColumnIndex, DataColumnSubnetId, EthSpec, Slot}; pub struct NetworkGlobals { /// The current local ENR. @@ -196,14 +196,19 @@ impl NetworkGlobals { /// Returns a connected peer that: /// 1. is connected /// 2. assigned to custody the column based on it's `custody_subnet_count` from ENR or metadata - /// 3. has a good score - pub fn custody_peers_for_column(&self, column_index: ColumnIndex) -> Vec { + /// 3. has data available past the given slot + /// 4. has a good score + pub fn custody_peers_for_column( + &self, + column_index: ColumnIndex, + block_slot: Slot, + ) -> Vec { self.peers .read() - .good_custody_subnet_peer(DataColumnSubnetId::from_column_index( - column_index, - &self.spec, - )) + .good_custody_subnet_peer( + DataColumnSubnetId::from_column_index(column_index, &self.spec), + block_slot, + ) .cloned() .collect::>() } diff --git a/beacon_node/network/src/sync/network_context.rs b/beacon_node/network/src/sync/network_context.rs index 8e8abd4fa6..d2ced9fd9d 100644 --- a/beacon_node/network/src/sync/network_context.rs +++ b/beacon_node/network/src/sync/network_context.rs @@ -397,9 +397,9 @@ impl SyncNetworkContext { .collect() } - pub fn get_custodial_peers(&self, column_index: ColumnIndex) -> Vec { + pub fn get_custodial_peers(&self, column_index: ColumnIndex, block_slot: Slot) -> Vec { self.network_globals() - .custody_peers_for_column(column_index) + .custody_peers_for_column(column_index, block_slot) } pub fn network_globals(&self) -> &NetworkGlobals { @@ -1161,6 +1161,7 @@ impl SyncNetworkContext { let requester = CustodyRequester(id); let mut request = ActiveCustodyRequest::new( block_root, + block_slot, CustodyId { requester }, &custody_indexes_to_fetch, lookup_peers, diff --git a/beacon_node/network/src/sync/network_context/custody.rs b/beacon_node/network/src/sync/network_context/custody.rs index b1a4b52867..3518eecf09 100644 --- a/beacon_node/network/src/sync/network_context/custody.rs +++ b/beacon_node/network/src/sync/network_context/custody.rs @@ -13,7 +13,7 @@ use std::hash::{BuildHasher, RandomState}; use std::time::{Duration, Instant}; use std::{collections::HashMap, marker::PhantomData, sync::Arc}; use tracing::{Span, debug, debug_span, warn}; -use types::{DataColumnSidecar, Hash256, data::ColumnIndex}; +use types::{DataColumnSidecar, Hash256, Slot, data::ColumnIndex}; use types::{DataColumnSidecarList, EthSpec}; use super::{LookupRequestResult, PeerGroup, RpcResponseResult, SyncNetworkContext}; @@ -22,6 +22,7 @@ const MAX_STALE_NO_PEERS_DURATION: Duration = Duration::from_secs(30); pub struct ActiveCustodyRequest { block_root: Hash256, + block_slot: Slot, custody_id: CustodyId, /// List of column indices this request needs to download to complete successfully column_requests: FnvHashMap>, @@ -62,6 +63,7 @@ pub type CustodyRequestResult = Result ActiveCustodyRequest { pub(crate) fn new( block_root: Hash256, + block_slot: Slot, custody_id: CustodyId, column_indices: &[ColumnIndex], lookup_peers: Arc>>, @@ -73,6 +75,7 @@ impl ActiveCustodyRequest { ); Self { block_root, + block_slot, custody_id, column_requests: HashMap::from_iter( column_indices @@ -365,7 +368,7 @@ impl ActiveCustodyRequest { // We draw from the total set of peers, but prioritize those peers who we have // received an attestation or a block from (`lookup_peers`), as the `lookup_peers` may take // time to build up and we are likely to not find any column peers initially. - let custodial_peers = cx.get_custodial_peers(column_index); + let custodial_peers = cx.get_custodial_peers(column_index, self.block_slot); let mut prioritized_peers = custodial_peers .iter() .filter(|peer| { From e8472b9d77e160418884056f07565c202aca2383 Mon Sep 17 00:00:00 2001 From: Daniel Knopik <107140945+dknopik@users.noreply.github.com> Date: Wed, 17 Jun 2026 02:45:30 +0200 Subject: [PATCH 11/15] Remove --disable-partial-columns in favour of bool argument to `--enable...` (#9478) Similar to https://github.com/sigp/lighthouse/pull/9476, partial columns is a feature expected to become the default soon, so instead of introducing a CLI option that will be removed again soon, consolidate into `--enable-partial-columns` which now takes a boolean argument. Co-Authored-By: Daniel Knopik --- beacon_node/src/cli.rs | 20 ++++++------------- beacon_node/src/config.rs | 6 ++---- book/src/help_bn.md | 11 ++++------- lighthouse/tests/beacon_node.rs | 34 ++++++++++++++++----------------- 4 files changed, 29 insertions(+), 42 deletions(-) diff --git a/beacon_node/src/cli.rs b/beacon_node/src/cli.rs index 942b30120b..3cf6a6efd2 100644 --- a/beacon_node/src/cli.rs +++ b/beacon_node/src/cli.rs @@ -684,21 +684,13 @@ pub fn cli_app() -> Command { .arg( Arg::new("enable-partial-columns") .long("enable-partial-columns") + .value_name("BOOLEAN") .help("Enable partial messages for data columns. This can reduce the amount of \ - data sent over the network. Enabled by default on Hoodi and Sepolia; use \ - --disable-partial-columns to opt out.") - .action(ArgAction::SetTrue) - .help_heading(FLAG_HEADER) - .display_order(0) - ) - .arg( - Arg::new("disable-partial-columns") - .long("disable-partial-columns") - .help("Disable partial messages for data columns. Use this on Hoodi or Sepolia \ - to opt out of the default-enabled behavior.") - .action(ArgAction::SetTrue) - .conflicts_with("enable-partial-columns") - .help_heading(FLAG_HEADER) + data sent over the network. Enabled by default on Hoodi and Sepolia; set to \ + \"false\" to opt out.") + .action(ArgAction::Set) + .num_args(0..=1) + .default_missing_value("true") .display_order(0) ) /* diff --git a/beacon_node/src/config.rs b/beacon_node/src/config.rs index 4f42923010..d27909bddf 100644 --- a/beacon_node/src/config.rs +++ b/beacon_node/src/config.rs @@ -112,10 +112,8 @@ pub fn get_config( .config_name .as_ref() .is_some_and(|name| matches!(name.as_str(), "hoodi" | "sepolia")); - let user_disable_partial_columns = parse_flag(cli_args, "disable-partial-columns"); - let user_enable_partial_columns = parse_flag(cli_args, "enable-partial-columns"); - let enable_partial_columns = !user_disable_partial_columns - && (user_enable_partial_columns || default_partial_columns_enabled); + let enable_partial_columns = clap_utils::parse_optional(cli_args, "enable-partial-columns")? + .unwrap_or(default_partial_columns_enabled); if enable_partial_columns { // Partial messages assume that each subnet maps to exactly one column. diff --git a/book/src/help_bn.md b/book/src/help_bn.md index 1612d35b0e..45e10b0d11 100644 --- a/book/src/help_bn.md +++ b/book/src/help_bn.md @@ -88,6 +88,10 @@ Options: Enables the mplex multiplexer alongside yamux. Yamux is preferred when both are available. Enabled by default; set to "false" to disable. [default: true] + --enable-partial-columns [] + Enable partial messages for data columns. This can reduce the amount + of data sent over the network. Enabled by default on Hoodi and + Sepolia; set to "false" to opt out. --enr-address
... The IP address/ DNS address to broadcast to other peers on how to reach this node. If a DNS address is provided, the enr-address is set @@ -475,9 +479,6 @@ Flags: --disable-packet-filter Disables the discovery packet filter. Useful for testing in smaller networks - --disable-partial-columns - Disable partial messages for data columns. Use this on Hoodi or - Sepolia to opt out of the default-enabled behavior. --disable-proposer-reorgs Do not attempt to reorg late blocks from other validators when proposing. @@ -493,10 +494,6 @@ Flags: Sets the local ENR IP address and port to match those set for lighthouse. Specifically, the IP address will be the value of --listen-address and the UDP port will be --discovery-port. - --enable-partial-columns - Enable partial messages for data columns. This can reduce the amount - of data sent over the network. Enabled by default on Hoodi and - Sepolia; use --disable-partial-columns to opt out. --enable-private-discovery Lighthouse by default does not discover private IP addresses. Set this flag to enable connection attempts to local addresses. diff --git a/lighthouse/tests/beacon_node.rs b/lighthouse/tests/beacon_node.rs index d52558da93..b8fd978ac5 100644 --- a/lighthouse/tests/beacon_node.rs +++ b/lighthouse/tests/beacon_node.rs @@ -2827,7 +2827,7 @@ fn enable_mplex_no_value() { #[test] fn partial_columns() { CommandLineTest::new() - .flag("enable-partial-columns", None) + .flag("enable-partial-columns", Some("true")) .run_with_zero_port() .with_config(|config| { assert!(config.network.enable_partial_columns); @@ -2842,6 +2842,18 @@ fn partial_columns() { }) } +#[test] +fn partial_columns_no_value() { + // Passing the flag without a value should enable partial columns. + CommandLineTest::new() + .flag("enable-partial-columns", None) + .run_with_zero_port() + .with_config(|config| { + assert!(config.network.enable_partial_columns); + assert!(config.chain.enable_partial_columns); + }); +} + #[test] fn partial_columns_default_hoodi() { CommandLineTest::new() @@ -2865,10 +2877,10 @@ fn partial_columns_default_sepolia() { } #[test] -fn partial_columns_disable_overrides_hoodi_default() { +fn partial_columns_false_overrides_hoodi_default() { CommandLineTest::new() .flag("network", Some("hoodi")) - .flag("disable-partial-columns", None) + .flag("enable-partial-columns", Some("false")) .run_with_zero_port() .with_config(|config| { assert!(!config.network.enable_partial_columns); @@ -2877,24 +2889,12 @@ fn partial_columns_disable_overrides_hoodi_default() { } #[test] -fn partial_columns_disable_on_mainnet_no_op() { +fn partial_columns_false_on_mainnet() { CommandLineTest::new() - .flag("disable-partial-columns", None) + .flag("enable-partial-columns", Some("false")) .run_with_zero_port() .with_config(|config| { assert!(!config.network.enable_partial_columns); assert!(!config.chain.enable_partial_columns); }); } - -#[test] -fn partial_columns_enable_disable_conflict() { - let mut cmd = base_cmd(); - cmd.arg("--enable-partial-columns") - .arg("--disable-partial-columns"); - let output = cmd.output().expect("should run command"); - assert!( - !output.status.success(), - "expected clap to reject --enable-partial-columns and --disable-partial-columns together", - ); -} From eb0da57044d15768673b5df1789aa8752c8799d5 Mon Sep 17 00:00:00 2001 From: Vansh Sahay Date: Wed, 17 Jun 2026 06:46:09 +0530 Subject: [PATCH 12/15] fix(http_api): ignore committee_index in attestation data endpoint (#9437) Co-Authored-By: Vansh Sahay Co-Authored-By: chonghe <44791194+chong-he@users.noreply.github.com> --- beacon_node/http_api/src/validator/mod.rs | 4 +++- beacon_node/http_api/tests/tests.rs | 13 +++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/beacon_node/http_api/src/validator/mod.rs b/beacon_node/http_api/src/validator/mod.rs index 8639914774..ee699b3adc 100644 --- a/beacon_node/http_api/src/validator/mod.rs +++ b/beacon_node/http_api/src/validator/mod.rs @@ -277,8 +277,10 @@ pub fn get_validator_attestation_data( ))); } + // Always use committee_index 0 regardless of the query parameter, since + // attestation data does not depend on the committee index post-Electra. chain - .produce_unaggregated_attestation(query.slot, query.committee_index) + .produce_unaggregated_attestation(query.slot, 0) .map(|attestation| attestation.data().clone()) .map(GenericResponse::from) .map_err(warp_utils::reject::unhandled_error) diff --git a/beacon_node/http_api/tests/tests.rs b/beacon_node/http_api/tests/tests.rs index 843e055827..4867852645 100644 --- a/beacon_node/http_api/tests/tests.rs +++ b/beacon_node/http_api/tests/tests.rs @@ -4856,6 +4856,19 @@ impl ApiTester { assert_eq!(result, expected); } + // The committee_index in the response must always be 0 post-Electra, + // regardless of the query parameter. + let committee_count = state.get_committee_count_at_slot(slot).unwrap(); + if committee_count > 0 { + let result = self + .client + .get_validator_attestation_data(slot, 1) + .await + .unwrap() + .data; + assert_eq!(result.index, 0); + } + self } From 3bc9148e0e90f40eabb9496659a15d965292b4f7 Mon Sep 17 00:00:00 2001 From: chonghe <44791194+chong-he@users.noreply.github.com> Date: Wed, 17 Jun 2026 18:17:10 +0800 Subject: [PATCH 13/15] SSZ fallback to JSON in `proposer_preferences` (#9475) Co-Authored-By: Tan Chee Keong Co-Authored-By: Eitan Seri-Levi --- .../src/proposer_preferences_service.rs | 29 ++++++++++++------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/validator_client/validator_services/src/proposer_preferences_service.rs b/validator_client/validator_services/src/proposer_preferences_service.rs index fc17a1bce6..5d5c40a6cd 100644 --- a/validator_client/validator_services/src/proposer_preferences_service.rs +++ b/validator_client/validator_services/src/proposer_preferences_service.rs @@ -182,24 +182,33 @@ impl ProposerPreferencesSer .first_success(|beacon_node| { let signed = signed.clone(); async move { - match beacon_node + beacon_node .post_validator_proposer_preferences_ssz(&signed, fork_name) .await - { - Ok(()) => Ok(()), - Err(ssz_err) => { - debug!(error = ?ssz_err, "SSZ publish failed, falling back to JSON"); + .map_err(|e| format!("Failed to publish proposer preferences (SSZ): {e:?}")) + } + }) + .await; + + let result = match result { + Ok(()) => Ok(()), + Err(ssz_err) => { + debug!(error = %ssz_err, "SSZ publish failed, falling back to JSON"); + self.beacon_nodes + .first_success(|beacon_node| { + let signed = signed.clone(); + async move { beacon_node .post_validator_proposer_preferences(&signed, fork_name) .await .map_err(|e| { - format!("Failed to publish proposer preferences: {e:?}") + format!("Failed to publish proposer preferences (JSON): {e:?}") }) } - } - } - }) - .await; + }) + .await + } + }; match result { Ok(()) => { From a46620155b1d6ed1ec2a775a0f4fa10e53cebd3f Mon Sep 17 00:00:00 2001 From: Lion - dapplion <35266934+dapplion@users.noreply.github.com> Date: Wed, 17 Jun 2026 14:44:21 +0200 Subject: [PATCH 14/15] Gloas attestation payload reprocess (#9440) Handle payload-present attestations before the payload is seen (gloas) A gloas beacon_attestation with index == 1 claims a past block's payload is already present. If we haven't seen that block's payload envelope yet, we shouldn't reject it the envelope may just be in flight. So instead we IGNORE it (new AttnError::UnknownPayloadEnvelope), ask sync to fetch the envelope, and park the attestation in the reprocess queue. When the envelope is imported, the parked attestations are released and re-verified. The envelope lookup itself is stubbed here and wired up in #9155 or a follow up PR Co-Authored-By: dapplion <35266934+dapplion@users.noreply.github.com> Co-Authored-By: Eitan Seri-Levi --- .../src/attestation_verification.rs | 30 ++ beacon_node/beacon_chain/src/beacon_chain.rs | 17 +- .../beacon_chain/src/fetch_blobs/tests.rs | 10 +- .../src/payload_bid_verification/tests.rs | 1 + .../payload_envelope_verification/import.rs | 7 +- .../tests/attestation_verification.rs | 170 ++++++++ .../beacon_chain/tests/column_verification.rs | 5 +- .../src/scheduler/work_reprocessing_queue.rs | 391 ++++++++++++++---- .../src/beacon/execution_payload_envelopes.rs | 4 +- .../http_api/src/publish_attestations.rs | 40 ++ beacon_node/http_api/src/publish_blocks.rs | 4 +- .../http_api/tests/interactive_tests.rs | 6 +- beacon_node/network/src/metrics.rs | 7 + .../gossip_methods.rs | 286 +++++++++++-- .../src/network_beacon_processor/mod.rs | 22 +- .../network_beacon_processor/sync_methods.rs | 22 +- beacon_node/network/src/sync/manager.rs | 28 ++ beacon_node/network/src/sync/tests/lookups.rs | 2 +- consensus/fork_choice/src/fork_choice.rs | 2 + consensus/proto_array/benches/find_head.rs | 1 + .../src/fork_choice_test_definition.rs | 1 + .../src/proto_array_fork_choice.rs | 7 + 22 files changed, 893 insertions(+), 170 deletions(-) diff --git a/beacon_node/beacon_chain/src/attestation_verification.rs b/beacon_node/beacon_chain/src/attestation_verification.rs index 635ca3a2ae..90ac7d68cf 100644 --- a/beacon_node/beacon_chain/src/attestation_verification.rs +++ b/beacon_node/beacon_chain/src/attestation_verification.rs @@ -174,6 +174,14 @@ pub enum Error { /// The attestation points to a block we have not yet imported. It's unclear if the attestation /// is valid or not. UnknownHeadBlock { beacon_block_root: Hash256 }, + /// An attestation indicating the presence of a payload (`index == 1`) references a block whose + /// execution payload envelope has not been seen yet. + /// + /// ## Peer scoring + /// + /// The attestation may be valid once the payload envelope is retrieved; it's unclear if the + /// attestation is valid or not, so it is ignored (not penalized) pending the envelope. + UnknownPayloadEnvelope { beacon_block_root: Hash256 }, /// The `attestation.data.beacon_block_root` block is from before the finalized checkpoint. /// /// ## Peer scoring @@ -612,6 +620,18 @@ impl<'a, T: BeaconChainTypes> IndexedAggregatedAttestation<'a, T> { )); } + // [New in Gloas]: `index == 1` claims the block's execution payload is present. Ignore the + // attestation until we have seen the block's payload envelope, so it can be re-processed + // (and the envelope retrieved) once the payload is received. + if fork_name.gloas_enabled() + && attestation.data().index == 1 + && !head_block.payload_received + { + return Err(Error::UnknownPayloadEnvelope { + beacon_block_root: attestation.data().beacon_block_root, + }); + } + // Check the attestation target root is consistent with the head root. // // This check is not in the specification, however we guard against it since it opens us up @@ -923,6 +943,16 @@ impl<'a, T: BeaconChainTypes> IndexedUnaggregatedAttestation<'a, T> { )); } + // [New in Gloas]: `index == 1` claims the block's execution payload is present. Ignore the + // attestation until we have seen the block's payload envelope, so it can be re-processed + // (and the envelope retrieved) once the payload is received. + if fork_name.gloas_enabled() && attestation.data.index == 1 && !head_block.payload_received + { + return Err(Error::UnknownPayloadEnvelope { + beacon_block_root: attestation.data.beacon_block_root, + }); + } + // Check the attestation target root is consistent with the head root. verify_attestation_target_root::(&head_block, &attestation.data)?; diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 5a521d18e6..f09b9b0520 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -196,7 +196,7 @@ pub enum WhenSlotSkipped { #[derive(Copy, Clone, Debug, PartialEq)] pub enum AvailabilityProcessingStatus { MissingComponents(Slot, Hash256), - Imported(Hash256), + Imported(Slot, Hash256), } impl TryInto for AvailabilityProcessingStatus { @@ -204,7 +204,7 @@ impl TryInto for AvailabilityProcessingStatus { fn try_into(self) -> Result { match self { - AvailabilityProcessingStatus::Imported(hash) => Ok(hash.into()), + AvailabilityProcessingStatus::Imported(_, hash) => Ok(hash.into()), _ => Err(()), } } @@ -215,7 +215,7 @@ impl TryInto for AvailabilityProcessingStatus { fn try_into(self) -> Result { match self { - AvailabilityProcessingStatus::Imported(hash) => Ok(hash), + AvailabilityProcessingStatus::Imported(_, hash) => Ok(hash), _ => Err(()), } } @@ -3159,9 +3159,9 @@ impl BeaconChain { { Ok(status) => { match status { - AvailabilityProcessingStatus::Imported(block_root) => { + AvailabilityProcessingStatus::Imported(slot, block_root) => { // The block was imported successfully. - imported_blocks.push((block_root, block_slot)); + imported_blocks.push((block_root, slot)); } AvailabilityProcessingStatus::MissingComponents(slot, block_root) => { warn!( @@ -3808,10 +3808,10 @@ impl BeaconChain { // Verify and import the block. match import_block.await { // The block was successfully verified and imported. Yay. - Ok(status @ AvailabilityProcessingStatus::Imported(block_root)) => { + Ok(status @ AvailabilityProcessingStatus::Imported(slot, block_root)) => { debug!( ?block_root, - %block_slot, + %slot, source = %block_source, "Beacon block imported" ); @@ -4149,6 +4149,7 @@ impl BeaconChain { payload_verification_outcome, } = *block; + let slot = block.slot(); let BlockImportData { block_root, state, @@ -4183,7 +4184,7 @@ impl BeaconChain { .await?? }; - Ok(AvailabilityProcessingStatus::Imported(block_root)) + Ok(AvailabilityProcessingStatus::Imported(slot, block_root)) } /// Accepts a fully-verified and available block and imports it into the chain without performing any diff --git a/beacon_node/beacon_chain/src/fetch_blobs/tests.rs b/beacon_node/beacon_chain/src/fetch_blobs/tests.rs index 4a37113fd9..3c0f43fef0 100644 --- a/beacon_node/beacon_chain/src/fetch_blobs/tests.rs +++ b/beacon_node/beacon_chain/src/fetch_blobs/tests.rs @@ -200,7 +200,10 @@ mod get_blobs_v2 { .returning(|_, _| None); mock_process_engine_blobs_result( &mut mock_adapter, - Ok(AvailabilityProcessingStatus::Imported(block_root)), + Ok(AvailabilityProcessingStatus::Imported( + block.slot(), + block_root, + )), ); // Trigger fetch blobs on the block @@ -217,7 +220,10 @@ mod get_blobs_v2 { assert_eq!( processing_status, - Some(AvailabilityProcessingStatus::Imported(block_root)) + Some(AvailabilityProcessingStatus::Imported( + block.slot(), + block_root + )) ); let published_columns = extract_published_blobs(publish_fn_args); diff --git a/beacon_node/beacon_chain/src/payload_bid_verification/tests.rs b/beacon_node/beacon_chain/src/payload_bid_verification/tests.rs index 04eb875bd9..e764f0beb5 100644 --- a/beacon_node/beacon_chain/src/payload_bid_verification/tests.rs +++ b/beacon_node/beacon_chain/src/payload_bid_verification/tests.rs @@ -223,6 +223,7 @@ impl TestContext { execution_payload_parent_hash: Some(ExecutionBlockHash::zero()), execution_payload_block_hash: Some(ExecutionBlockHash::repeat_byte(0xab)), proposer_index: Some(0), + payload_received: false, }, Slot::new(1), &self.spec, diff --git a/beacon_node/beacon_chain/src/payload_envelope_verification/import.rs b/beacon_node/beacon_chain/src/payload_envelope_verification/import.rs index 90cdb4fe97..29782b3294 100644 --- a/beacon_node/beacon_chain/src/payload_envelope_verification/import.rs +++ b/beacon_node/beacon_chain/src/payload_envelope_verification/import.rs @@ -108,10 +108,10 @@ impl BeaconChain { // Verify and import the payload envelope. match import_envelope.await { // The payload envelope was successfully verified and imported. - Ok(status @ AvailabilityProcessingStatus::Imported(block_root)) => { + Ok(status @ AvailabilityProcessingStatus::Imported(slot, block_root)) => { info!( ?block_root, - %block_slot, + %slot, source = %envelope_source, "Execution payload envelope imported" ); @@ -195,6 +195,7 @@ impl BeaconChain { block_root, payload_verification_outcome, } = *envelope; + let slot = envelope.envelope.slot(); let block_root = { let chain = self.clone(); @@ -211,7 +212,7 @@ impl BeaconChain { .await?? }; - Ok(AvailabilityProcessingStatus::Imported(block_root)) + Ok(AvailabilityProcessingStatus::Imported(slot, block_root)) } /// Accepts a fully-verified and available envelope and imports it into the chain without performing any diff --git a/beacon_node/beacon_chain/tests/attestation_verification.rs b/beacon_node/beacon_chain/tests/attestation_verification.rs index 03b8ae58ac..ad369c79ee 100644 --- a/beacon_node/beacon_chain/tests/attestation_verification.rs +++ b/beacon_node/beacon_chain/tests/attestation_verification.rs @@ -1970,6 +1970,176 @@ async fn gloas_aggregated_attestation_same_slot_index_must_be_zero() { ); } +/// [New in Gloas]: An unaggregated attestation claiming payload-present (`data.index == 1`) for a +/// block whose payload envelope has not yet been seen (`payload_received == false`) must be +/// rejected with `UnknownPayloadEnvelope`, so it can be parked for re-processing once the envelope +/// arrives. +#[tokio::test] +async fn gloas_unaggregated_attestation_unknown_payload_envelope() { + if !test_spec::() + .fork_name_at_epoch(Epoch::new(0)) + .gloas_enabled() + { + return; + } + + let harness = get_harness(VALIDATOR_COUNT); + + // Build some chain depth. `extend_chain` imports each block's payload envelope, so every block + // produced so far has `payload_received == true`. + harness + .extend_chain( + MainnetEthSpec::slots_per_epoch() as usize * 2, + BlockStrategy::OnCanonicalHead, + AttestationStrategy::AllValidators, + ) + .await; + + // Produce one more block but do NOT import its payload envelope, leaving the head block with + // `payload_received == false`. + let head = harness.chain.head_snapshot(); + let block_slot = head.beacon_block.slot() + 1; + let ((signed_block, blobs), _envelope, _post_state) = harness + .make_block_with_envelope(head.beacon_state.clone(), block_slot) + .await; + let block_root = signed_block.canonical_root(); + harness + .process_block(block_slot, block_root, (signed_block, blobs)) + .await + .expect("payload-less block should import"); + + // The block should be the head, and its payload envelope should not be recorded. + assert!( + !harness + .chain + .canonical_head + .fork_choice_read_lock() + .get_block(&block_root) + .expect("block should be in fork choice") + .payload_received, + "block should not have its payload envelope recorded" + ); + + // Advance a slot so the attestation slot is later than the (payload-less) head block's slot, + // which avoids the same-slot `index == 0` requirement. + harness.advance_slot(); + + // Produce a valid attestation for the head block, then claim payload-present (`index == 1`). + // The gloas payload-envelope check runs before signature verification, so mutating the index + // is sufficient to exercise the arm. + let (mut attestation, _attester_sk, subnet_id) = + get_valid_unaggregated_attestation(&harness.chain); + assert_eq!( + attestation.data.beacon_block_root, block_root, + "attestation should be for the payload-less head block" + ); + attestation.data.index = 1; + + let result = harness + .chain + .verify_unaggregated_attestation_for_gossip(&attestation, Some(subnet_id)); + assert!( + matches!( + result, + Err(AttnError::UnknownPayloadEnvelope { beacon_block_root }) + if beacon_block_root == block_root + ), + "gloas: payload-present attestation for a block with an unseen payload envelope should be \ + rejected with UnknownPayloadEnvelope, got {:?}", + result.err() + ); +} + +/// [New in Gloas]: The aggregate counterpart of +/// `gloas_unaggregated_attestation_unknown_payload_envelope`. An aggregate claiming payload-present +/// (`data.index == 1`) for a block whose payload envelope has not been seen must be rejected with +/// `UnknownPayloadEnvelope`. +#[tokio::test] +async fn gloas_aggregated_attestation_unknown_payload_envelope() { + // Skip unless running with the gloas fork, before paying for harness setup. + if !test_spec::() + .fork_name_at_epoch(Epoch::new(0)) + .gloas_enabled() + { + return; + } + + let harness = get_harness(VALIDATOR_COUNT); + + // Build some chain depth. `extend_chain` imports each block's payload envelope, so every block + // produced so far has `payload_received == true`. + harness + .extend_chain( + MainnetEthSpec::slots_per_epoch() as usize * 2, + BlockStrategy::OnCanonicalHead, + AttestationStrategy::AllValidators, + ) + .await; + + // Produce one more block but do NOT import its payload envelope, leaving the head block with + // `payload_received == false`. + let head = harness.chain.head_snapshot(); + let block_slot = head.beacon_block.slot() + 1; + let ((signed_block, blobs), _envelope, _post_state) = harness + .make_block_with_envelope(head.beacon_state.clone(), block_slot) + .await; + let block_root = signed_block.canonical_root(); + harness + .process_block(block_slot, block_root, (signed_block, blobs)) + .await + .expect("payload-less block should import"); + + // Advance a slot so the attestation slot is later than the (payload-less) head block's slot, + // which avoids the same-slot `index == 0` requirement. + harness.advance_slot(); + + let head = harness.chain.head_snapshot(); + let current_slot = harness.chain.slot().expect("should get slot"); + + // Build a valid aggregate for the head block, then claim payload-present (`index == 1`). The + // gloas payload-envelope check runs before signature verification, so mutating the index is + // sufficient to exercise the arm. + let (valid_attestation, _, _) = get_valid_unaggregated_attestation(&harness.chain); + assert_eq!( + valid_attestation.data.beacon_block_root, block_root, + "attestation should be for the payload-less head block" + ); + let committee = head + .beacon_state + .get_beacon_committee(current_slot, valid_attestation.committee_index) + .expect("should get committee"); + let fork_name = harness + .spec + .fork_name_at_slot::(valid_attestation.data.slot); + let aggregate_attestation = + single_attestation_to_attestation(&valid_attestation, committee.committee, fork_name) + .unwrap(); + let (mut valid_aggregate, _, _) = + get_valid_aggregated_attestation(&harness.chain, aggregate_attestation); + + valid_aggregate + .as_electra_mut() + .unwrap() + .message + .aggregate + .data + .index = 1; + + let result = harness + .chain + .verify_aggregated_attestation_for_gossip(&valid_aggregate); + assert!( + matches!( + result, + Err(AttnError::UnknownPayloadEnvelope { beacon_block_root }) + if beacon_block_root == block_root + ), + "gloas: payload-present aggregate for a block with an unseen payload envelope should be \ + rejected with UnknownPayloadEnvelope, got {:?}", + result.err() + ); +} + /// Regression test: a SingleAttestation with a huge bogus attester_index must not be forwarded to /// the slasher. Previously the slasher received the IndexedAttestation before committee-membership /// validation, causing an OOM when the slasher tried to allocate based on the untrusted index. diff --git a/beacon_node/beacon_chain/tests/column_verification.rs b/beacon_node/beacon_chain/tests/column_verification.rs index 06a5f44e5f..180e187e90 100644 --- a/beacon_node/beacon_chain/tests/column_verification.rs +++ b/beacon_node/beacon_chain/tests/column_verification.rs @@ -274,5 +274,8 @@ async fn verify_header_signature_fork_block_bug() { .process_rpc_custody_columns(data_column_sidecars) .await .unwrap(); - assert_eq!(status, AvailabilityProcessingStatus::Imported(block_root)); + assert_eq!( + status, + AvailabilityProcessingStatus::Imported(signed_block.slot(), block_root) + ); } diff --git a/beacon_node/beacon_processor/src/scheduler/work_reprocessing_queue.rs b/beacon_node/beacon_processor/src/scheduler/work_reprocessing_queue.rs index 62ed86fbad..dddf2a740d 100644 --- a/beacon_node/beacon_processor/src/scheduler/work_reprocessing_queue.rs +++ b/beacon_node/beacon_processor/src/scheduler/work_reprocessing_queue.rs @@ -115,10 +115,10 @@ pub enum ReprocessQueueMessage { RpcBlock(QueuedRpcBlock), /// A block that was successfully processed. We use this to handle attestations updates /// for unknown blocks. - BlockImported { - block_root: Hash256, - parent_root: Hash256, - }, + BlockImported { block_root: Hash256 }, + /// A block's execution payload envelope was imported. We use this to release attestations that + /// claim payload-present (`index == 1`) for a block whose payload had not yet been seen. + PayloadEnvelopeImported { block_root: Hash256 }, /// A new `LightClientOptimisticUpdate` has been produced. We use this to handle light client /// updates for unknown parent blocks. NewLightClientOptimisticUpdate { parent_root: Hash256 }, @@ -126,6 +126,12 @@ pub enum ReprocessQueueMessage { UnknownBlockUnaggregate(QueuedUnaggregate), /// An aggregated attestation that references an unknown block. UnknownBlockAggregate(QueuedAggregate), + /// An unaggregated attestation (`index == 1`) whose block's execution payload envelope has not + /// been seen yet. + UnknownPayloadUnaggregate(QueuedUnaggregate), + /// An aggregated attestation (`index == 1`) whose block's execution payload envelope has not + /// been seen yet. + UnknownPayloadAggregate(QueuedAggregate), /// A light client optimistic update that references a parent root that has not been seen as a parent. UnknownLightClientOptimisticUpdate(QueuedLightClientUpdate), /// A new backfill batch that needs to be scheduled for processing. @@ -296,6 +302,9 @@ struct ReprocessQueue { queued_unaggregates: FnvHashMap, /// Attestations (aggregated and unaggregated) per root. awaiting_attestations_per_root: HashMap>, + /// Attestations (aggregated and unaggregated) awaiting a block's execution payload envelope, + /// keyed by block root. Released on `PayloadEnvelopeImported`. + awaiting_attestations_per_payload: HashMap>, /// Queued Light Client Updates. queued_lc_updates: FnvHashMap, /// Light Client Updates per parent_root. @@ -331,6 +340,20 @@ enum QueuedAttestationId { Unaggregate(usize), } +/// An attestation queued for re-processing, of either aggregation kind. +enum QueuedAttestation { + Aggregate(QueuedAggregate), + Unaggregate(QueuedUnaggregate), +} + +/// The component an attestation is waiting on before it can be re-processed. +enum AwaitingComponent { + /// The attestation's head block has not been seen. + Block, + /// The block's execution payload envelope has not been seen (`index == 1`, post-Gloas). + Payload, +} + impl QueuedAggregate { pub fn beacon_block_root(&self) -> &Hash256 { &self.beacon_block_root @@ -494,6 +517,7 @@ impl ReprocessQueue { queued_aggregates: FnvHashMap::default(), queued_unaggregates: FnvHashMap::default(), awaiting_attestations_per_root: HashMap::new(), + awaiting_attestations_per_payload: HashMap::new(), awaiting_lc_updates_per_parent_root: HashMap::new(), queued_backfill_batches: Vec::new(), queued_column_reconstructions: HashMap::new(), @@ -512,6 +536,65 @@ impl ReprocessQueue { } } + /// Queue an attestation for re-processing once the component it is waiting on (`awaiting`) is + /// imported. Shared by the unknown-block and unknown-payload paths for both aggregate and + /// unaggregate attestations. + fn queue_awaiting_attestation( + &mut self, + attestation: QueuedAttestation, + awaiting: AwaitingComponent, + ) { + if self.attestations_delay_queue.len() >= MAXIMUM_QUEUED_ATTESTATIONS { + if self.attestation_delay_debounce.elapsed() { + error!( + queue_size = MAXIMUM_QUEUED_ATTESTATIONS, + msg = "system resources may be saturated", + "Attestation delay queue is full" + ); + } + // Drop the attestation. + return; + } + + let id = self.next_attestation; + let (att_id, beacon_block_root) = match &attestation { + QueuedAttestation::Aggregate(a) => { + (QueuedAttestationId::Aggregate(id), *a.beacon_block_root()) + } + QueuedAttestation::Unaggregate(u) => { + (QueuedAttestationId::Unaggregate(id), *u.beacon_block_root()) + } + }; + + // Register the delay. + let delay_key = self + .attestations_delay_queue + .insert(att_id, QUEUED_ATTESTATION_DELAY); + + // Register this attestation against the component it awaits. + match awaiting { + AwaitingComponent::Block => &mut self.awaiting_attestations_per_root, + AwaitingComponent::Payload => &mut self.awaiting_attestations_per_payload, + } + .entry(beacon_block_root) + .or_default() + .push(att_id); + + // Store the attestation and its info. + match attestation { + QueuedAttestation::Aggregate(queued_aggregate) => { + self.queued_aggregates + .insert(id, (queued_aggregate, delay_key)); + } + QueuedAttestation::Unaggregate(queued_unaggregate) => { + self.queued_unaggregates + .insert(id, (queued_unaggregate, delay_key)); + } + } + + self.next_attestation += 1; + } + fn handle_message(&mut self, msg: InboundEvent) { use ReprocessQueueMessage::*; match msg { @@ -654,70 +737,26 @@ impl ReprocessQueue { error!("Failed to send rpc block to beacon processor"); } } - InboundEvent::Msg(UnknownBlockAggregate(queued_aggregate)) => { - if self.attestations_delay_queue.len() >= MAXIMUM_QUEUED_ATTESTATIONS { - if self.attestation_delay_debounce.elapsed() { - error!( - queue_size = MAXIMUM_QUEUED_ATTESTATIONS, - msg = "system resources may be saturated", - "Aggregate attestation delay queue is full" - ); - } - // Drop the attestation. - return; - } - - let att_id = QueuedAttestationId::Aggregate(self.next_attestation); - - // Register the delay. - let delay_key = self - .attestations_delay_queue - .insert(att_id, QUEUED_ATTESTATION_DELAY); - - // Register this attestation for the corresponding root. - self.awaiting_attestations_per_root - .entry(*queued_aggregate.beacon_block_root()) - .or_default() - .push(att_id); - - // Store the attestation and its info. - self.queued_aggregates - .insert(self.next_attestation, (queued_aggregate, delay_key)); - - self.next_attestation += 1; - } - InboundEvent::Msg(UnknownBlockUnaggregate(queued_unaggregate)) => { - if self.attestations_delay_queue.len() >= MAXIMUM_QUEUED_ATTESTATIONS { - if self.attestation_delay_debounce.elapsed() { - error!( - queue_size = MAXIMUM_QUEUED_ATTESTATIONS, - msg = "system resources may be saturated", - "Attestation delay queue is full" - ); - } - // Drop the attestation. - return; - } - - let att_id = QueuedAttestationId::Unaggregate(self.next_attestation); - - // Register the delay. - let delay_key = self - .attestations_delay_queue - .insert(att_id, QUEUED_ATTESTATION_DELAY); - - // Register this attestation for the corresponding root. - self.awaiting_attestations_per_root - .entry(*queued_unaggregate.beacon_block_root()) - .or_default() - .push(att_id); - - // Store the attestation and its info. - self.queued_unaggregates - .insert(self.next_attestation, (queued_unaggregate, delay_key)); - - self.next_attestation += 1; - } + InboundEvent::Msg(UnknownBlockAggregate(queued_aggregate)) => self + .queue_awaiting_attestation( + QueuedAttestation::Aggregate(queued_aggregate), + AwaitingComponent::Block, + ), + InboundEvent::Msg(UnknownBlockUnaggregate(queued_unaggregate)) => self + .queue_awaiting_attestation( + QueuedAttestation::Unaggregate(queued_unaggregate), + AwaitingComponent::Block, + ), + InboundEvent::Msg(UnknownPayloadAggregate(queued_aggregate)) => self + .queue_awaiting_attestation( + QueuedAttestation::Aggregate(queued_aggregate), + AwaitingComponent::Payload, + ), + InboundEvent::Msg(UnknownPayloadUnaggregate(queued_unaggregate)) => self + .queue_awaiting_attestation( + QueuedAttestation::Unaggregate(queued_unaggregate), + AwaitingComponent::Payload, + ), InboundEvent::Msg(UnknownBlockDataColumn(queued_data_column)) => { let block_root = queued_data_column.beacon_block_root; @@ -785,10 +824,7 @@ impl ReprocessQueue { self.next_lc_update += 1; } - InboundEvent::Msg(BlockImported { - block_root, - parent_root, - }) => { + InboundEvent::Msg(BlockImported { block_root }) => { // Unqueue the envelope we have for this root, if any. if let Some((envelope, delay_key)) = self.awaiting_envelopes_per_root.remove(&block_root) @@ -853,7 +889,6 @@ impl ReprocessQueue { if failed_to_send_count > 0 { error!( hint = "system may be overloaded", - ?parent_root, ?block_root, failed_count = failed_to_send_count, sent_count, @@ -881,6 +916,59 @@ impl ReprocessQueue { } } } + InboundEvent::Msg(PayloadEnvelopeImported { block_root }) => { + // Release attestations that were awaiting this block's execution payload envelope. + if let Some(queued_ids) = self.awaiting_attestations_per_payload.remove(&block_root) + { + let mut failed_to_send_count = 0; + + for id in queued_ids { + metrics::inc_counter( + &metrics::BEACON_PROCESSOR_REPROCESSING_QUEUE_MATCHED_ATTESTATIONS, + ); + + if let Some((work, delay_key)) = match id { + QueuedAttestationId::Aggregate(id) => self + .queued_aggregates + .remove(&id) + .map(|(aggregate, delay_key)| { + (ReadyWork::Aggregate(aggregate), delay_key) + }), + QueuedAttestationId::Unaggregate(id) => self + .queued_unaggregates + .remove(&id) + .map(|(unaggregate, delay_key)| { + (ReadyWork::Unaggregate(unaggregate), delay_key) + }), + } { + // Remove the delay. + self.attestations_delay_queue.remove(&delay_key); + + // Send the work. + if self.ready_work_tx.try_send(work).is_err() { + failed_to_send_count += 1; + } + } else { + // There is a mismatch between the attestation ids registered for this + // root and the queued attestations. This should never happen. + error!( + ?block_root, + att_id = ?id, + "Unknown queued attestation for payload envelope" + ); + } + } + + if failed_to_send_count > 0 { + error!( + hint = "system may be overloaded", + ?block_root, + failed_count = failed_to_send_count, + "Ignored scheduled attestation(s) for payload envelope" + ); + } + } + } InboundEvent::Msg(NewLightClientOptimisticUpdate { parent_root }) => { // Unqueue the light client optimistic updates we have for this root, if any. if let Some(queued_lc_id) = self @@ -1033,18 +1121,25 @@ impl ReprocessQueue { ); } - if let Entry::Occupied(mut queued_atts) = - self.awaiting_attestations_per_root.entry(root) - && let Some(index) = - queued_atts.get().iter().position(|&id| id == queued_id) - { - let queued_atts_mut = queued_atts.get_mut(); - queued_atts_mut.swap_remove(index); + // The attestation is awaiting either its block or its payload envelope; prune it + // from whichever map holds it (the other lookup is a no-op) to avoid leaking the + // entry on expiry. + for awaiting in [ + &mut self.awaiting_attestations_per_root, + &mut self.awaiting_attestations_per_payload, + ] { + if let Entry::Occupied(mut queued_atts) = awaiting.entry(root) + && let Some(index) = + queued_atts.get().iter().position(|&id| id == queued_id) + { + let queued_atts_mut = queued_atts.get_mut(); + queued_atts_mut.swap_remove(index); - // If the vec is empty after this attestation's removal, we need to delete - // the entry to prevent bloating the hashmap indefinitely. - if queued_atts_mut.is_empty() { - queued_atts.remove_entry(); + // If the vec is empty after this attestation's removal, we need to + // delete the entry to prevent bloating the hashmap indefinitely. + if queued_atts_mut.is_empty() { + queued_atts.remove_entry(); + } } } } @@ -1412,6 +1507,131 @@ mod tests { assert!(queue.awaiting_attestations_per_root.is_empty()); } + // Regression test for the same memory leak as `prune_awaiting_attestations_per_root`, but for + // attestations awaiting a block's execution payload envelope. + #[tokio::test] + async fn prune_awaiting_attestations_per_payload() { + create_test_tracing_subscriber(); + + let mut queue = test_queue(); + + // Pause time so it only advances manually + tokio::time::pause(); + + let beacon_block_root = Hash256::repeat_byte(0xaf); + + // Insert a payload-present attestation awaiting its payload envelope. + let att = ReprocessQueueMessage::UnknownPayloadUnaggregate(QueuedUnaggregate { + beacon_block_root, + process_fn: Box::new(|| {}), + }); + queue.handle_message(InboundEvent::Msg(att)); + + // Check that it is queued. + assert_eq!(queue.awaiting_attestations_per_payload.len(), 1); + assert!( + queue + .awaiting_attestations_per_payload + .contains_key(&beacon_block_root) + ); + + // Advance time to expire the attestation. + advance_time(&queue.slot_clock, 2 * QUEUED_ATTESTATION_DELAY).await; + let ready_msg = queue.next().await.unwrap(); + assert!(matches!(ready_msg, InboundEvent::ReadyAttestation(_))); + queue.handle_message(ready_msg); + + // The entry should be pruned on expiry. + assert!(queue.awaiting_attestations_per_payload.is_empty()); + } + + // The payload envelope import releases attestations awaiting that block's payload. + #[tokio::test] + async fn release_awaiting_attestations_on_payload_envelope_imported() { + create_test_tracing_subscriber(); + + let mut queue = test_queue(); + tokio::time::pause(); + + let beacon_block_root = Hash256::repeat_byte(0xaf); + + let att = ReprocessQueueMessage::UnknownPayloadUnaggregate(QueuedUnaggregate { + beacon_block_root, + process_fn: Box::new(|| {}), + }); + queue.handle_message(InboundEvent::Msg(att)); + assert_eq!(queue.awaiting_attestations_per_payload.len(), 1); + + // Importing the payload envelope drains the awaiting attestations for that root. + queue.handle_message(InboundEvent::Msg( + ReprocessQueueMessage::PayloadEnvelopeImported { + block_root: beacon_block_root, + }, + )); + assert!(queue.awaiting_attestations_per_payload.is_empty()); + } + + // As `prune_awaiting_attestations_per_payload`, but for an aggregated payload-present + // attestation (`UnknownPayloadAggregate`). + #[tokio::test] + async fn prune_awaiting_attestations_per_payload_aggregate() { + create_test_tracing_subscriber(); + + let mut queue = test_queue(); + tokio::time::pause(); + + let beacon_block_root = Hash256::repeat_byte(0xaf); + + let att = ReprocessQueueMessage::UnknownPayloadAggregate(QueuedAggregate { + beacon_block_root, + process_fn: Box::new(|| {}), + }); + queue.handle_message(InboundEvent::Msg(att)); + + assert_eq!(queue.awaiting_attestations_per_payload.len(), 1); + assert!( + queue + .awaiting_attestations_per_payload + .contains_key(&beacon_block_root) + ); + + // Advance time to expire the attestation. + advance_time(&queue.slot_clock, 2 * QUEUED_ATTESTATION_DELAY).await; + let ready_msg = queue.next().await.unwrap(); + assert!(matches!(ready_msg, InboundEvent::ReadyAttestation(_))); + queue.handle_message(ready_msg); + + // The entry should be pruned on expiry. + assert!(queue.awaiting_attestations_per_payload.is_empty()); + } + + // As `release_awaiting_attestations_on_payload_envelope_imported`, but for an aggregated + // payload-present attestation (`UnknownPayloadAggregate`). + #[tokio::test] + async fn release_awaiting_aggregate_on_payload_envelope_imported() { + create_test_tracing_subscriber(); + + let mut queue = test_queue(); + tokio::time::pause(); + + let beacon_block_root = Hash256::repeat_byte(0xaf); + + let att = ReprocessQueueMessage::UnknownPayloadAggregate(QueuedAggregate { + beacon_block_root, + process_fn: Box::new(|| {}), + }); + queue.handle_message(InboundEvent::Msg(att)); + assert_eq!(queue.awaiting_attestations_per_payload.len(), 1); + + // Importing the payload envelope drains the awaiting attestations for that root. + queue.handle_message(InboundEvent::Msg( + ReprocessQueueMessage::PayloadEnvelopeImported { + block_root: beacon_block_root, + }, + )); + assert!(queue.awaiting_attestations_per_payload.is_empty()); + } + // This is a regression test for a memory leak in `awaiting_lc_updates_per_parent_root`. // See: https://github.com/sigp/lighthouse/pull/8065 #[tokio::test] @@ -1622,7 +1842,6 @@ mod tests { tokio::time::pause(); let beacon_block_root = Hash256::repeat_byte(0xaf); - let parent_root = Hash256::repeat_byte(0xab); // Insert an envelope. let msg = ReprocessQueueMessage::UnknownBlockForEnvelope(QueuedGossipEnvelope { @@ -1640,7 +1859,6 @@ mod tests { // Simulate block import. let imported = ReprocessQueueMessage::BlockImported { block_root: beacon_block_root, - parent_root, }; queue.handle_message(InboundEvent::Msg(imported)); @@ -1716,7 +1934,6 @@ mod tests { // Simulate block import. queue.handle_message(InboundEvent::Msg(ReprocessQueueMessage::BlockImported { block_root: beacon_block_root, - parent_root: Hash256::repeat_byte(0x00), })); // Internal state should be cleaned up. diff --git a/beacon_node/http_api/src/beacon/execution_payload_envelopes.rs b/beacon_node/http_api/src/beacon/execution_payload_envelopes.rs index b6b681e091..d058f66001 100644 --- a/beacon_node/http_api/src/beacon/execution_payload_envelopes.rs +++ b/beacon_node/http_api/src/beacon/execution_payload_envelopes.rs @@ -163,7 +163,7 @@ pub async fn publish_execution_payload_envelope( .await; let mut envelope_imported = match &import_result { - Ok(AvailabilityProcessingStatus::Imported(_)) => true, + Ok(AvailabilityProcessingStatus::Imported(_, _)) => true, Ok(AvailabilityProcessingStatus::MissingComponents(_, _)) => false, Err(e) => { warn!(%slot, error = ?e, "Failed to import execution payload envelope"); @@ -210,7 +210,7 @@ pub async fn publish_execution_payload_envelope( if !sampling_columns.is_empty() { match Box::pin(chain.process_gossip_data_columns(sampling_columns, || Ok(()))).await { - Ok(AvailabilityProcessingStatus::Imported(_)) => envelope_imported = true, + Ok(AvailabilityProcessingStatus::Imported(_, _)) => envelope_imported = true, Ok(AvailabilityProcessingStatus::MissingComponents(_, _)) => {} Err(e) => { error!( diff --git a/beacon_node/http_api/src/publish_attestations.rs b/beacon_node/http_api/src/publish_attestations.rs index b93f2a0b7b..c1ea241b79 100644 --- a/beacon_node/http_api/src/publish_attestations.rs +++ b/beacon_node/http_api/src/publish_attestations.rs @@ -189,6 +189,46 @@ pub async fn publish_attestations( PublishAttestationResult::Reprocessing(rx) } } + Err(Error::Validation(AttestationError::UnknownPayloadEnvelope { + beacon_block_root, + })) => { + if !allow_reprocess { + return PublishAttestationResult::Failure(Error::ReprocessDisabled); + }; + // Re-process once the block's payload envelope is seen (Gloas). + let (tx, rx) = oneshot::channel(); + let reprocess_chain = chain.clone(); + let reprocess_network_tx = network_tx.clone(); + let reprocess_fn = move || { + let result = verify_and_publish_attestation( + &reprocess_chain, + &attestation, + seen_timestamp, + &reprocess_network_tx, + ); + // Ignore failure on the oneshot that reports the result. This + // shouldn't happen unless some catastrophe befalls the waiting + // thread which causes it to drop. + let _ = tx.send(result); + }; + let reprocess_msg = ReprocessQueueMessage::UnknownPayloadUnaggregate( + QueuedUnaggregate { + beacon_block_root, + process_fn: Box::new(reprocess_fn), + }, + ); + if task_spawner + .try_send(WorkEvent { + drop_during_sync: false, + work: Work::Reprocess(reprocess_msg), + }) + .is_err() + { + PublishAttestationResult::Failure(Error::ReprocessFull) + } else { + PublishAttestationResult::Reprocessing(rx) + } + } Err(Error::Validation(AttestationError::PriorAttestationKnown { .. })) => PublishAttestationResult::AlreadyKnown, diff --git a/beacon_node/http_api/src/publish_blocks.rs b/beacon_node/http_api/src/publish_blocks.rs index b46576ddad..8b45a4b04c 100644 --- a/beacon_node/http_api/src/publish_blocks.rs +++ b/beacon_node/http_api/src/publish_blocks.rs @@ -266,7 +266,7 @@ pub async fn publish_block>( Err(BlockError::DuplicateFullyImported(root)) => { if publish_fn_completed.load(Ordering::SeqCst) { post_block_import_logging_and_response( - Ok(AvailabilityProcessingStatus::Imported(root)), + Ok(AvailabilityProcessingStatus::Imported(slot, root)), validation_level, block, is_locally_built_block, @@ -474,7 +474,7 @@ async fn post_block_import_logging_and_response( // result of the block being imported from gossip, OR it could be that it finished importing // after processing of a gossip blob. In the latter case we MUST run fork choice to // re-compute the head. - Ok(AvailabilityProcessingStatus::Imported(root)) + Ok(AvailabilityProcessingStatus::Imported(_, root)) | Err(BlockError::DuplicateFullyImported(root)) => { let delay = get_block_delay_ms(seen_timestamp, block.message(), &chain.slot_clock); info!( diff --git a/beacon_node/http_api/tests/interactive_tests.rs b/beacon_node/http_api/tests/interactive_tests.rs index e6135a81c7..9258dab1af 100644 --- a/beacon_node/http_api/tests/interactive_tests.rs +++ b/beacon_node/http_api/tests/interactive_tests.rs @@ -876,7 +876,6 @@ async fn queue_attestations_from_http() { // In parallel, apply the block. We need to manually notify the reprocess queue, because the // `beacon_chain` does not know about the queue and will not update it for us. - let parent_root = block.0.parent_root(); harness .process_block(attestation_slot, block_root, block) .await @@ -888,10 +887,7 @@ async fn queue_attestations_from_http() { .unwrap() .try_send(WorkEvent { drop_during_sync: false, - work: Work::Reprocess(ReprocessQueueMessage::BlockImported { - block_root, - parent_root, - }), + work: Work::Reprocess(ReprocessQueueMessage::BlockImported { block_root }), }) .unwrap(); diff --git a/beacon_node/network/src/metrics.rs b/beacon_node/network/src/metrics.rs index c043133cee..1a664662df 100644 --- a/beacon_node/network/src/metrics.rs +++ b/beacon_node/network/src/metrics.rs @@ -22,6 +22,13 @@ pub(crate) enum BlockSource { Rpc, } +/// The path through which a payload envelope was imported. +#[derive(Debug, Clone, Copy, AsRefStr)] +pub(crate) enum EnvelopeSource { + Gossip, + Rpc, +} + pub static BEACON_BLOCK_MESH_PEERS_PER_CLIENT: LazyLock> = LazyLock::new(|| { try_create_int_gauge_vec( diff --git a/beacon_node/network/src/network_beacon_processor/gossip_methods.rs b/beacon_node/network/src/network_beacon_processor/gossip_methods.rs index b52732000e..20342c1aa9 100644 --- a/beacon_node/network/src/network_beacon_processor/gossip_methods.rs +++ b/beacon_node/network/src/network_beacon_processor/gossip_methods.rs @@ -1,5 +1,5 @@ use crate::{ - metrics::{self, register_process_result_metrics}, + metrics::{self, EnvelopeSource, register_process_result_metrics}, network_beacon_processor::{InvalidBlockStorage, NetworkBeaconProcessor}, service::NetworkMessage, sync::SyncMessage, @@ -70,6 +70,45 @@ use beacon_processor::{ /// messages. const STRICT_LATE_MESSAGE_PENALTIES: bool = false; +/// Tracks which kinds of attestation re-processing are still permitted for a gossip attestation +/// or aggregate. +/// +/// A new attestation may be re-queued for an unknown block, then (post-Gloas) for an unknown +/// payload envelope, and finally not at all. Each re-queue narrows the allowance to the next +/// variant. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ReprocessAllowance { + /// Re-queue for either an unknown block or an unknown payload envelope. + BlockAndPayload, + /// Re-queue only for an unknown payload envelope (already re-queued once for the block). + PayloadOnly, + /// Do not re-queue again. + None, +} + +impl ReprocessAllowance { + /// Whether the attestation may be re-queued for an unknown block. + fn allows_block(self) -> bool { + matches!(self, ReprocessAllowance::BlockAndPayload) + } + + /// Whether the attestation may be re-queued for an unknown payload envelope. + fn allows_payload(self) -> bool { + matches!( + self, + ReprocessAllowance::BlockAndPayload | ReprocessAllowance::PayloadOnly + ) + } + + /// Re-queuing always narrows the allowance so a message can't loop indefinitely. + fn next_requeue(self) -> Self { + match self { + ReprocessAllowance::BlockAndPayload => ReprocessAllowance::PayloadOnly, + ReprocessAllowance::PayloadOnly | ReprocessAllowance::None => ReprocessAllowance::None, + } + } +} + /// An attestation that has been validated by the `BeaconChain`. /// /// Since this struct implements `beacon_chain::VerifiedAttestation`, it would be a logic error to @@ -233,7 +272,7 @@ impl NetworkBeaconProcessor { attestation: Box, subnet_id: SubnetId, should_import: bool, - allow_reprocess: bool, + reprocess_allowance: ReprocessAllowance, seen_timestamp: Duration, ) { let result = match self @@ -256,7 +295,7 @@ impl NetworkBeaconProcessor { message_id, peer_id, subnet_id, - allow_reprocess, + reprocess_allowance, should_import, seen_timestamp, ); @@ -265,7 +304,7 @@ impl NetworkBeaconProcessor { pub fn process_gossip_attestation_batch( self: Arc, packages: GossipAttestationBatch, - allow_reprocess: bool, + reprocess_allowance: ReprocessAllowance, ) { let attestations_and_subnets = packages .iter() @@ -326,7 +365,7 @@ impl NetworkBeaconProcessor { package.message_id, package.peer_id, package.subnet_id, - allow_reprocess, + reprocess_allowance, package.should_import, package.seen_timestamp, ); @@ -342,7 +381,7 @@ impl NetworkBeaconProcessor { message_id: MessageId, peer_id: PeerId, subnet_id: SubnetId, - allow_reprocess: bool, + reprocess_allowance: ReprocessAllowance, should_import: bool, seen_timestamp: Duration, ) { @@ -426,7 +465,7 @@ impl NetworkBeaconProcessor { should_import, seen_timestamp, }, - allow_reprocess, + reprocess_allowance, error, seen_timestamp, ); @@ -446,7 +485,7 @@ impl NetworkBeaconProcessor { message_id: MessageId, peer_id: PeerId, aggregate: Box>, - allow_reprocess: bool, + reprocess_allowance: ReprocessAllowance, seen_timestamp: Duration, ) { let beacon_block_root = aggregate.message().aggregate().data().beacon_block_root; @@ -470,7 +509,7 @@ impl NetworkBeaconProcessor { beacon_block_root, message_id, peer_id, - allow_reprocess, + reprocess_allowance, seen_timestamp, ); } @@ -478,7 +517,7 @@ impl NetworkBeaconProcessor { pub fn process_gossip_aggregate_batch( self: Arc, packages: Vec>, - allow_reprocess: bool, + reprocess_allowance: ReprocessAllowance, ) { let aggregates = packages.iter().map(|package| package.aggregate.as_ref()); @@ -532,7 +571,7 @@ impl NetworkBeaconProcessor { package.beacon_block_root, package.message_id, package.peer_id, - allow_reprocess, + reprocess_allowance, package.seen_timestamp, ); } @@ -544,7 +583,7 @@ impl NetworkBeaconProcessor { beacon_block_root: Hash256, message_id: MessageId, peer_id: PeerId, - allow_reprocess: bool, + reprocess_allowance: ReprocessAllowance, seen_timestamp: Duration, ) { match result { @@ -624,7 +663,7 @@ impl NetworkBeaconProcessor { attestation: signed_aggregate, seen_timestamp, }, - allow_reprocess, + reprocess_allowance, error, seen_timestamp, ); @@ -918,12 +957,13 @@ impl NetworkBeaconProcessor { match result { Ok(availability) => match availability { - AvailabilityProcessingStatus::Imported(block_root) => { + AvailabilityProcessingStatus::Imported(slot, block_root) => { debug!( %block_root, "Gossipsub data column processed, imported fully available block" ); self.chain.recompute_head_at_current_slot().await; + self.notify_import_after_column(slot, block_root); metrics::set_gauge( &metrics::BEACON_BLOB_DELAY_FULL_VERIFICATION, @@ -1311,12 +1351,13 @@ impl NetworkBeaconProcessor { match &result { Ok(availability) => match availability { - AvailabilityProcessingStatus::Imported(block_root) => { + AvailabilityProcessingStatus::Imported(slot, block_root) => { debug!( %block_root, "Data column from partial processed, imported fully available block" ); self.chain.recompute_head_at_current_slot().await; + self.notify_import_after_column(*slot, *block_root); metrics::set_gauge( &metrics::BEACON_BLOB_DELAY_FULL_VERIFICATION, @@ -1784,24 +1825,8 @@ impl NetworkBeaconProcessor { register_process_result_metrics(&result, metrics::BlockSource::Gossip, "block"); match &result { - Ok(AvailabilityProcessingStatus::Imported(block_root)) => { - if self - .beacon_processor_send - .try_send(WorkEvent { - drop_during_sync: false, - work: Work::Reprocess(ReprocessQueueMessage::BlockImported { - block_root: *block_root, - parent_root: block.message().parent_root(), - }), - }) - .is_err() - { - error!( - source = "gossip", - ?block_root, - "Failed to inform block import" - ) - }; + Ok(AvailabilityProcessingStatus::Imported(_, block_root)) => { + self.notify_block_imported(*block_root); debug!( ?block_root, @@ -2458,7 +2483,7 @@ impl NetworkBeaconProcessor { peer_id: PeerId, message_id: MessageId, failed_att: FailedAtt, - allow_reprocess: bool, + reprocess_allowance: ReprocessAllowance, error: AttnError, seen_timestamp: Duration, ) { @@ -2717,7 +2742,7 @@ impl NetworkBeaconProcessor { block = ?beacon_block_root, "Attestation for unknown block" ); - if allow_reprocess { + if reprocess_allowance.allows_block() { // We don't know the block, get the sync manager to handle the block lookup, and // send the attestation to be scheduled for re-processing. self.send_sync_message(SyncMessage::UnknownBlockHashFromAttestation( @@ -2740,7 +2765,7 @@ impl NetworkBeaconProcessor { message_id, peer_id, attestation, - false, // Do not allow this attestation to be re-processed beyond this point. + reprocess_allowance.next_requeue(), seen_timestamp, ) }), @@ -2765,7 +2790,7 @@ impl NetworkBeaconProcessor { attestation, subnet_id, should_import, - false, // Do not allow this attestation to be re-processed beyond this point. + reprocess_allowance.next_requeue(), seen_timestamp, ) }), @@ -2797,6 +2822,89 @@ impl NetworkBeaconProcessor { return; } + AttnError::UnknownPayloadEnvelope { beacon_block_root } => { + trace!( + %peer_id, + block = ?beacon_block_root, + "Payload-present attestation for block with unseen payload envelope" + ); + if reprocess_allowance.allows_payload() { + // We haven't seen the block's payload envelope yet. Ask the sync manager to + // retrieve it, and schedule the attestation for re-processing once it arrives. + self.send_sync_message(SyncMessage::UnknownPayloadEnvelopeFromAttestation( + peer_id, + *beacon_block_root, + )); + let msg = match failed_att { + FailedAtt::Aggregate { + attestation, + seen_timestamp, + } => { + metrics::inc_counter( + &metrics::BEACON_PROCESSOR_AGGREGATED_ATTESTATION_REQUEUED_TOTAL, + ); + let processor = self.clone(); + ReprocessQueueMessage::UnknownPayloadAggregate(QueuedAggregate { + beacon_block_root: *beacon_block_root, + process_fn: Box::new(move || { + processor.process_gossip_aggregate( + message_id, + peer_id, + attestation, + reprocess_allowance.next_requeue(), + seen_timestamp, + ) + }), + }) + } + FailedAtt::Unaggregate { + attestation, + subnet_id, + should_import, + seen_timestamp, + } => { + metrics::inc_counter( + &metrics::BEACON_PROCESSOR_UNAGGREGATED_ATTESTATION_REQUEUED_TOTAL, + ); + let processor = self.clone(); + ReprocessQueueMessage::UnknownPayloadUnaggregate(QueuedUnaggregate { + beacon_block_root: *beacon_block_root, + process_fn: Box::new(move || { + processor.process_gossip_attestation( + message_id, + peer_id, + attestation, + subnet_id, + should_import, + reprocess_allowance.next_requeue(), + seen_timestamp, + ) + }), + }) + } + }; + + if self + .beacon_processor_send + .try_send(WorkEvent { + drop_during_sync: false, + work: Work::Reprocess(msg), + }) + .is_err() + { + error!("Failed to send attestation for re-processing") + } + } else { + // We shouldn't make any further attempts to process this attestation. + self.propagate_validation_result( + message_id, + peer_id, + MessageAcceptance::Ignore, + ); + } + + return; + } AttnError::UnknownTargetRoot(_) => { /* * The block indicated by the target root is not known to us. @@ -3796,10 +3904,13 @@ impl NetworkBeaconProcessor { // register_process_result_metrics(&result, metrics::BlockSource::Gossip, "envelope"); match &result { - Ok(AvailabilityProcessingStatus::Imported(_)) => { + Ok(AvailabilityProcessingStatus::Imported(_, block_root)) => { self.chain.recompute_head_at_current_slot().await; + // The payload envelope is imported (`is_payload_received` is now true); release any + // attestations awaiting this block's payload so they can be re-processed. + self.notify_payload_envelope_imported(*block_root, EnvelopeSource::Gossip); } - Ok(AvailabilityProcessingStatus::MissingComponents(_, _)) => {} + Ok(_) => {} Err(e) => { debug!( ?beacon_block_root, @@ -3811,6 +3922,64 @@ impl NetworkBeaconProcessor { } } + /// Inform the reprocess queue that a fully available block (or its payload envelope, post-gloas) + /// has been imported, so any attestations waiting on it can be released. + fn notify_import_after_column(&self, slot: Slot, block_root: Hash256) { + if self + .chain + .spec + .fork_name_at_slot::(slot) + .gloas_enabled() + { + self.notify_payload_envelope_imported(block_root, EnvelopeSource::Gossip); + } else { + self.notify_block_imported(block_root); + } + } + + /// Inform the reprocess queue that `block_root` has been imported as a full block. + fn notify_block_imported(&self, block_root: Hash256) { + if self + .beacon_processor_send + .try_send(WorkEvent { + drop_during_sync: false, + work: Work::Reprocess(ReprocessQueueMessage::BlockImported { block_root }), + }) + .is_err() + { + error!( + source = "gossip", + ?block_root, + "Failed to inform block import" + ) + }; + } + + /// Inform the reprocess queue that `block_root`'s payload envelope has been imported, releasing + /// any attestations awaiting the payload. `source` identifies the import path for logging. + pub(crate) fn notify_payload_envelope_imported( + &self, + block_root: Hash256, + source: EnvelopeSource, + ) { + if self + .beacon_processor_send + .try_send(WorkEvent { + drop_during_sync: false, + work: Work::Reprocess(ReprocessQueueMessage::PayloadEnvelopeImported { + block_root, + }), + }) + .is_err() + { + error!( + source = source.as_ref(), + ?block_root, + "Failed to inform payload envelope import" + ) + }; + } + #[instrument( name = "lh_process_execution_payload_bid", parent = None, @@ -4059,3 +4228,42 @@ impl NetworkBeaconProcessor { } } } + +#[cfg(test)] +mod tests { + use super::ReprocessAllowance::{BlockAndPayload, None, PayloadOnly}; + + #[test] + fn reprocess_allowance_gates() { + // A block re-queue is only permitted for a freshly received attestation. + assert!(BlockAndPayload.allows_block()); + assert!(!PayloadOnly.allows_block()); + assert!(!None.allows_block()); + + // A payload-envelope re-queue is permitted until we've already re-queued for it. + assert!(BlockAndPayload.allows_payload()); + assert!(PayloadOnly.allows_payload()); + assert!(!None.allows_payload()); + } + + #[test] + fn reprocess_allowance_progression() { + // Each re-queue narrows the allowance to the next variant in the progression. + assert_eq!(BlockAndPayload.next_requeue(), PayloadOnly); + assert_eq!(PayloadOnly.next_requeue(), None); + assert_eq!(None.next_requeue(), None); + } + + #[test] + fn reprocess_allowance_is_bounded() { + // Safety property: from any starting state, re-queuing twice reaches the terminal `None`, + // so an attestation can never loop indefinitely. + for start in [BlockAndPayload, PayloadOnly, None] { + assert_eq!( + start.next_requeue().next_requeue(), + None, + "re-queuing twice from {start:?} should be terminal" + ); + } + } +} diff --git a/beacon_node/network/src/network_beacon_processor/mod.rs b/beacon_node/network/src/network_beacon_processor/mod.rs index a9579caaeb..7619f706cc 100644 --- a/beacon_node/network/src/network_beacon_processor/mod.rs +++ b/beacon_node/network/src/network_beacon_processor/mod.rs @@ -39,6 +39,8 @@ use { pub use sync_methods::{BlockProcessingResult, ChainSegmentProcessId}; +use gossip_methods::ReprocessAllowance; + pub type Error = TrySendError>; mod gossip_methods; @@ -93,15 +95,17 @@ impl NetworkBeaconProcessor { package.attestation, package.subnet_id, package.should_import, - true, + ReprocessAllowance::BlockAndPayload, package.seen_timestamp, ) }; // Define a closure for processing batches of attestations. let processor = self.clone(); - let process_batch = - move |attestations| processor.process_gossip_attestation_batch(attestations, true); + let process_batch = move |attestations| { + processor + .process_gossip_attestation_batch(attestations, ReprocessAllowance::BlockAndPayload) + }; self.try_send(BeaconWorkEvent { drop_during_sync: true, @@ -135,15 +139,17 @@ impl NetworkBeaconProcessor { package.message_id, package.peer_id, package.aggregate, - true, + ReprocessAllowance::BlockAndPayload, package.seen_timestamp, ) }; // Define a closure for processing batches of attestations. let processor = self.clone(); - let process_batch = - move |aggregates| processor.process_gossip_aggregate_batch(aggregates, true); + let process_batch = move |aggregates| { + processor + .process_gossip_aggregate_batch(aggregates, ReprocessAllowance::BlockAndPayload) + }; let beacon_block_root = aggregate.message().aggregate().data().beacon_block_root; self.try_send(BeaconWorkEvent { @@ -932,7 +938,7 @@ impl NetworkBeaconProcessor { .await { Ok(Some(availability)) => match availability { - AvailabilityProcessingStatus::Imported(_) => { + AvailabilityProcessingStatus::Imported(..) => { debug!( result = "imported block and custody columns", %block_root, @@ -1020,7 +1026,7 @@ impl NetworkBeaconProcessor { Ok(Some((availability_processing_status, data_columns_to_publish))) => { self.publish_data_columns_gradually(data_columns_to_publish, block_root); match &availability_processing_status { - AvailabilityProcessingStatus::Imported(hash) => { + AvailabilityProcessingStatus::Imported(_, hash) => { debug!( result = "imported block and custody columns", block_hash = %hash, diff --git a/beacon_node/network/src/network_beacon_processor/sync_methods.rs b/beacon_node/network/src/network_beacon_processor/sync_methods.rs index caf718732b..35437e1a2e 100644 --- a/beacon_node/network/src/network_beacon_processor/sync_methods.rs +++ b/beacon_node/network/src/network_beacon_processor/sync_methods.rs @@ -1,4 +1,4 @@ -use crate::metrics::{self, register_process_result_metrics}; +use crate::metrics::{self, EnvelopeSource, register_process_result_metrics}; use crate::network_beacon_processor::{FUTURE_SLOT_TOLERANCE, NetworkBeaconProcessor}; use crate::sync::BatchProcessResult; use crate::sync::manager::CustodyBatchProcessResult; @@ -158,8 +158,6 @@ impl NetworkBeaconProcessor { return; }; - let slot = block.slot(); - let parent_root = block.message().parent_root(); let commitments_formatted = block.as_block().commitments_formatted(); debug!( @@ -186,17 +184,14 @@ impl NetworkBeaconProcessor { // RPC block imported, regardless of process type match result.as_ref() { - Ok(AvailabilityProcessingStatus::Imported(hash)) => { + Ok(AvailabilityProcessingStatus::Imported(slot, hash)) => { info!( %slot, %hash, "New RPC block received", ); // Trigger processing for work referencing this block. - let reprocess_msg = ReprocessQueueMessage::BlockImported { - block_root: *hash, - parent_root, - }; + let reprocess_msg = ReprocessQueueMessage::BlockImported { block_root: *hash }; if self .beacon_processor_send .try_send(WorkEvent { @@ -213,7 +208,7 @@ impl NetworkBeaconProcessor { }; self.chain.block_times_cache.write().set_time_observed( *hash, - slot, + *slot, seen_timestamp, None, None, @@ -294,7 +289,7 @@ impl NetworkBeaconProcessor { match &result { Ok(availability) => match availability { - AvailabilityProcessingStatus::Imported(hash) => { + AvailabilityProcessingStatus::Imported(_, hash) => { debug!( result = "imported block and custody columns", block_hash = %hash, @@ -376,8 +371,11 @@ impl NetworkBeaconProcessor { let result: Result = result.map_err(|e| BlockError::InternalError(format!("envelope: {e}"))); - if matches!(result, Ok(AvailabilityProcessingStatus::Imported(_))) { + // The payload envelope is imported; release any attestations awaiting this block's payload + // so they can be re-processed (parity with the gossip import path). + if let Ok(AvailabilityProcessingStatus::Imported(_, block_root)) = &result { self.chain.recompute_head_at_current_slot().await; + self.notify_payload_envelope_imported(*block_root, EnvelopeSource::Rpc); } self.send_sync_message(SyncMessage::BlockComponentProcessed { @@ -1022,7 +1020,7 @@ impl From> for BlockProcessingR )) } match result { - Ok(AvailabilityProcessingStatus::Imported(_)) => Self::Imported(true, "imported"), + Ok(AvailabilityProcessingStatus::Imported(..)) => Self::Imported(true, "imported"), Ok(AvailabilityProcessingStatus::MissingComponents(_, _)) => { Self::Imported(false, "missing_components") } diff --git a/beacon_node/network/src/sync/manager.rs b/beacon_node/network/src/sync/manager.rs index 8e7b8cd05a..3282f7f083 100644 --- a/beacon_node/network/src/sync/manager.rs +++ b/beacon_node/network/src/sync/manager.rs @@ -156,6 +156,11 @@ pub enum SyncMessage { /// manager to attempt to find the block matching the unknown hash. UnknownBlockHashFromAttestation(PeerId, Hash256), + /// A peer has sent a payload-present attestation (`index == 1`) for a block whose execution + /// payload envelope we have not seen. This triggers the manager to fetch the payload envelope + /// for `block_root` via `ExecutionPayloadEnvelopesByRoot`. + UnknownPayloadEnvelopeFromAttestation(PeerId, Hash256), + /// A peer has disconnected. Disconnect(PeerId), @@ -260,6 +265,10 @@ pub struct SyncManager { /// may forward us thousands of a attestations, each one triggering an individual event. Only /// one event is useful, the rest generating log noise and wasted cycles notified_unknown_roots: LRUTimeCache<(PeerId, Hash256)>, + /// Debounce duplicated `UnknownPayloadEnvelopeFromAttestation` for the same root/peer tuple, + /// for the same reason as `notified_unknown_roots`: a peer may forward many payload-present + /// attestations for a block whose execution payload envelope we have not yet seen. + notified_unknown_payload_roots: LRUTimeCache<(PeerId, Hash256)>, } /// Spawns a new `SyncManager` thread which has a weak reference to underlying beacon @@ -320,6 +329,9 @@ impl SyncManager { notified_unknown_roots: LRUTimeCache::new(Duration::from_secs( NOTIFIED_UNKNOWN_ROOT_EXPIRY_SECONDS, )), + notified_unknown_payload_roots: LRUTimeCache::new(Duration::from_secs( + NOTIFIED_UNKNOWN_ROOT_EXPIRY_SECONDS, + )), } } @@ -895,6 +907,22 @@ impl SyncManager { self.handle_unknown_block_root(peer_id, block_root); } } + SyncMessage::UnknownPayloadEnvelopeFromAttestation(peer_id, block_root) => { + if !self + .notified_unknown_payload_roots + .contains(&(peer_id, block_root)) + { + self.notified_unknown_payload_roots + .insert((peer_id, block_root)); + // TODO(gloas): trigger a payload-envelope lookup for `block_root` via + // `ExecutionPayloadEnvelopesByRoot`. Wired up in the gloas lookup-sync PR (#9155). + debug!( + ?block_root, + ?peer_id, + "Received unknown payload envelope from attestation" + ); + } + } SyncMessage::Disconnect(peer_id) => { debug!(%peer_id, "Received disconnected message"); self.peer_disconnect(&peer_id); diff --git a/beacon_node/network/src/sync/tests/lookups.rs b/beacon_node/network/src/sync/tests/lookups.rs index 13eeaee9aa..621824c7d2 100644 --- a/beacon_node/network/src/sync/tests/lookups.rs +++ b/beacon_node/network/src/sync/tests/lookups.rs @@ -1989,7 +1989,7 @@ impl TestRig { block: Arc>, ) { match self.import_block_to_da_checker(block).await { - AvailabilityProcessingStatus::Imported(_) => { + AvailabilityProcessingStatus::Imported(..) => { panic!("block removed from da_checker, available") } AvailabilityProcessingStatus::MissingComponents(_, block_root) => { diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index 90f2bb9a67..e648d5669b 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -1064,6 +1064,8 @@ where execution_payload_parent_hash, execution_payload_block_hash, proposer_index: Some(block.proposer_index()), + // Set on payload-envelope import, not block import. + payload_received: false, }, current_slot, spec, diff --git a/consensus/proto_array/benches/find_head.rs b/consensus/proto_array/benches/find_head.rs index 98077a7f97..07edc4d46f 100644 --- a/consensus/proto_array/benches/find_head.rs +++ b/consensus/proto_array/benches/find_head.rs @@ -68,6 +68,7 @@ fn build_chain(num_blocks: u64, gloas: bool) -> (ProtoArrayForkChoice, types::Ch }, execution_payload_block_hash: if is_gloas { Some(get_hash(i)) } else { None }, proposer_index: Some(0), + payload_received: false, }; fork_choice diff --git a/consensus/proto_array/src/fork_choice_test_definition.rs b/consensus/proto_array/src/fork_choice_test_definition.rs index 7ffa763308..d9acda1258 100644 --- a/consensus/proto_array/src/fork_choice_test_definition.rs +++ b/consensus/proto_array/src/fork_choice_test_definition.rs @@ -330,6 +330,7 @@ impl ForkChoiceTestDefinition { execution_payload_parent_hash, execution_payload_block_hash, proposer_index: Some(0), + payload_received: false, }; fork_choice .process_block::(block, slot, &spec, Duration::ZERO) diff --git a/consensus/proto_array/src/proto_array_fork_choice.rs b/consensus/proto_array/src/proto_array_fork_choice.rs index 69202486a7..90143f1dd1 100644 --- a/consensus/proto_array/src/proto_array_fork_choice.rs +++ b/consensus/proto_array/src/proto_array_fork_choice.rs @@ -242,6 +242,8 @@ pub struct Block { pub execution_payload_parent_hash: Option, pub execution_payload_block_hash: Option, pub proposer_index: Option, + /// Whether the block's execution payload envelope has been received. Always `false` pre-Gloas. + pub payload_received: bool, } impl Block { @@ -502,6 +504,7 @@ impl ProtoArrayForkChoice { execution_payload_parent_hash, execution_payload_block_hash, proposer_index: Some(proposer_index), + payload_received: false, }; proto_array @@ -959,6 +962,7 @@ impl ProtoArrayForkChoice { execution_payload_parent_hash: block.execution_payload_parent_hash().ok(), execution_payload_block_hash: block.execution_payload_block_hash().ok(), proposer_index: block.proposer_index().ok(), + payload_received: block.payload_received().unwrap_or(false), }) } @@ -1383,6 +1387,7 @@ mod test_compute_deltas { execution_payload_parent_hash: None, execution_payload_block_hash: None, proposer_index: Some(0), + payload_received: false, }, genesis_slot + 1, &spec, @@ -1411,6 +1416,7 @@ mod test_compute_deltas { execution_payload_parent_hash: None, execution_payload_block_hash: None, proposer_index: Some(0), + payload_received: false, }, genesis_slot + 1, &spec, @@ -1547,6 +1553,7 @@ mod test_compute_deltas { execution_payload_parent_hash: None, execution_payload_block_hash: None, proposer_index: Some(0), + payload_received: false, }, Slot::from(block.slot), &spec, From 716528f44e25ca9bac031c7d7e1662eab01bb78f Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Wed, 17 Jun 2026 23:40:07 +1000 Subject: [PATCH 15/15] Correct unrealized justification for blocks with slashings (#9471) Fix a bug in fork choice whereby the unrealized justified and finalized checkpoints from a parent block would be incorrectly carried over to a child block in the same epoch. The documented optimisation in `fork_choice.rs` was wrong because it failed to account for slashings. This bug is not considered to be sensitive due to the difficulty of triggering it, and the low payoff for doing so (fleeting divergence). Keep the optimisation, updating it to correctly skip reusing the parent checkpoints when slashings are present. A more minimal alternative would be to scrap the optimisation altogether (always compute the checkpoints), however this would come with a minor performance downside. I think the updated optimisation is simple enough to be worth retaining. There are 3 regression tests added which confirm the correct behaviour. Temporarily setting `has_slashings` to `false` causes all 3 tests to fail. Co-Authored-By: Michael Sproul Co-Authored-By: dapplion <35266934+dapplion@users.noreply.github.com> --- beacon_node/beacon_chain/tests/main.rs | 1 + .../tests/unrealized_checkpoints.rs | 342 ++++++++++++++++++ consensus/fork_choice/src/fork_choice.rs | 15 +- 3 files changed, 354 insertions(+), 4 deletions(-) create mode 100644 beacon_node/beacon_chain/tests/unrealized_checkpoints.rs diff --git a/beacon_node/beacon_chain/tests/main.rs b/beacon_node/beacon_chain/tests/main.rs index d31db128c5..e84f561fac 100644 --- a/beacon_node/beacon_chain/tests/main.rs +++ b/beacon_node/beacon_chain/tests/main.rs @@ -12,4 +12,5 @@ mod schema_stability; mod store_tests; mod sync_committee_verification; mod tests; +mod unrealized_checkpoints; mod validator_monitor; diff --git a/beacon_node/beacon_chain/tests/unrealized_checkpoints.rs b/beacon_node/beacon_chain/tests/unrealized_checkpoints.rs new file mode 100644 index 0000000000..01ae86c44d --- /dev/null +++ b/beacon_node/beacon_chain/tests/unrealized_checkpoints.rs @@ -0,0 +1,342 @@ +#![cfg(not(debug_assertions))] + +//! This file contains regression tests for a bug in fork choice whereby the unrealized justified +//! and finalized checkpoints of a block were assumed to carry over to its child. This is NOT TRUE +//! in general, as the child block may contain slashings which invalidate the +//! justification/finalization from the parent. The tests in this file reproduce this scenario using +//! both attester slashings and proposer slashings. + +use beacon_chain::{ + StateSkipConfig, + test_utils::{ + AttestationStrategy, BeaconChainHarness, BlockStrategy, EphemeralHarnessType, test_spec, + }, +}; +use state_processing::per_epoch_processing; +use std::sync::Arc; +use types::{Checkpoint, Epoch, EthSpec, MinimalEthSpec, consts::altair::TIMELY_TARGET_FLAG_INDEX}; + +type E = MinimalEthSpec; + +// Proposer slashings are limited to MaxProposerSlashings (16) per block. With 32 validators, +// dropping below the 2/3 justification threshold requires only ~11 slashes, which fits. +const VALIDATOR_COUNT: usize = 32; + +fn ceil_two_thirds(value: u64) -> u64 { + (2 * value).div_ceil(3) +} + +struct SameEpochSlashingChild { + harness: BeaconChainHarness>, + stored_parent_justified: Checkpoint, + stored_parent_finalized: Checkpoint, + stored_child_justified: Checkpoint, + stored_child_finalized: Checkpoint, + expected_child_justified: Checkpoint, + expected_child_finalized: Checkpoint, + parent_epoch: Epoch, +} + +/// Basic test checking that the child has correct unrealized justified/finalized checkpoints in the +/// case where the slashings are attester slashings. +#[tokio::test] +async fn child_unrealized_checkpoints_recomputed_after_same_epoch_slashing() { + let scenario = same_epoch_attester_slashing_child().await; + + assert_eq!( + scenario.stored_child_justified, scenario.expected_child_justified, + "child unrealized justified checkpoint should be recomputed from the child state" + ); + assert_eq!( + scenario.stored_child_finalized, scenario.expected_child_finalized, + "child unrealized finalized checkpoint should be recomputed from the child state" + ); +} + +/// Variation on the attester slashing test, checking that the inferior justification of the child +/// results in fork choice correctly reverting to the justified checkpoint once the child block is +/// no longer considered viable. +#[tokio::test] +async fn child_with_stale_voting_source_not_head_at_epoch_plus_two() { + let scenario = same_epoch_attester_slashing_child().await; + let slots_per_epoch = E::slots_per_epoch(); + let divergence_slot = scenario + .parent_epoch + .saturating_add(2u64) + .start_slot(slots_per_epoch); + + assert_eq!( + scenario.stored_child_justified, scenario.expected_child_justified, + "child unrealized justified checkpoint should be recomputed from the child state" + ); + assert_eq!( + scenario.stored_child_finalized, scenario.expected_child_finalized, + "child unrealized finalized checkpoint should be recomputed from the child state" + ); + assert!( + scenario.expected_child_justified.epoch.saturating_add(2u64) + < divergence_slot.epoch(slots_per_epoch), + "with the spec-computed checkpoint the child is outside the viability window at epoch N + 2" + ); + + while scenario.harness.get_current_slot() < divergence_slot { + scenario.harness.advance_slot(); + } + + let mut fork_choice = scenario + .harness + .chain + .canonical_head + .fork_choice_write_lock(); + let head_result = fork_choice.get_head(divergence_slot, &scenario.harness.chain.spec); + + assert_eq!( + fork_choice.justified_checkpoint(), + scenario.stored_parent_justified, + "the store should realize the parent's unrealized justification at the epoch boundary" + ); + assert_eq!( + fork_choice.finalized_checkpoint(), + scenario.stored_parent_finalized, + "the store should realize the parent's unrealized finalization at the epoch boundary" + ); + + // No epoch N + 1 blocks were produced after the slashing child. Under the spec-computed child + // checkpoint, the child is the only leaf below the justified root and is outside the viability + // window. The spec-correct result is to set the justified checkpoint as the head. + assert_eq!( + head_result.unwrap().0, + fork_choice.justified_checkpoint().root + ); +} + +/// Basic test checking child checkpoints but with proposer slashings instead of attester slashings. +#[tokio::test] +async fn child_unrealized_checkpoints_recomputed_after_same_epoch_proposer_slashing() { + let scenario = same_epoch_proposer_slashing_child().await; + + assert_eq!( + scenario.stored_child_justified, scenario.expected_child_justified, + "child unrealized justified checkpoint should be recomputed from the child state" + ); + assert_eq!( + scenario.stored_child_finalized, scenario.expected_child_finalized, + "child unrealized finalized checkpoint should be recomputed from the child state" + ); +} + +async fn same_epoch_attester_slashing_child() -> SameEpochSlashingChild { + same_epoch_slashing_child(VALIDATOR_COUNT, |harness, slash_indices| { + harness + .add_attester_slashing(slash_indices.to_vec()) + .expect("should add attester slashing to operation pool"); + }) + .await +} + +async fn same_epoch_proposer_slashing_child() -> SameEpochSlashingChild { + same_epoch_slashing_child(VALIDATOR_COUNT, |harness, slash_indices| { + for &index in slash_indices { + harness + .add_proposer_slashing(index) + .expect("should add proposer slashing to operation pool"); + } + }) + .await +} + +/// Generic scenario builder with `inject_slashings` capable of injecting attester or proposer +/// slashings. +async fn same_epoch_slashing_child( + validator_count: usize, + inject_slashings: F, +) -> SameEpochSlashingChild +where + F: FnOnce(&BeaconChainHarness>, &[u64]), +{ + let spec = test_spec::(); + + let harness: BeaconChainHarness> = + BeaconChainHarness::builder(E::default()) + .spec(Arc::new(spec)) + .deterministic_keypairs(validator_count) + .fresh_ephemeral_store() + .mock_execution_layer() + .build(); + + let slots_per_epoch = E::slots_per_epoch(); + + // Minimum warm-up for the parent to reach FFG steady state (justified == epoch, finalized == + // epoch - 1); 2 epochs is too few. + let warmup_epochs: u64 = 3; + harness.advance_slot(); + harness + .extend_chain( + slots_per_epoch as usize * warmup_epochs as usize, + BlockStrategy::OnCanonicalHead, + AttestationStrategy::AllValidators, + ) + .await; + + let parent_epoch = Epoch::new(warmup_epochs); + // ceil(Minimal::slots_per_epoch() * 2/3) = 6 + let parent_slot = parent_epoch.start_slot(slots_per_epoch) + 6; + let parent_root = harness.extend_to_slot(parent_slot).await; + + let mut parent_state = harness + .chain + .state_at_slot(parent_slot, StateSkipConfig::WithStateRoots) + .expect("should load parent state"); + parent_state + .build_caches(&harness.chain.spec) + .expect("should build parent state caches"); + + let total_active_balance = parent_state + .get_total_active_balance() + .expect("should get parent total active balance"); + let required_balance = ceil_two_thirds(total_active_balance); + let effective_balance = parent_state + .validators() + .get(0) + .expect("validator 0 should exist") + .effective_balance; + let slash_count_needed = total_active_balance + .checked_sub(required_balance) + .expect("total active balance should be at least the required balance") + / effective_balance + + 1; + + let child_slot = parent_slot + 1; + let child_proposer = parent_state + .get_beacon_proposer_index(child_slot, &harness.chain.spec) + .expect("should get child proposer") as u64; + + let (_, _, _, current_participation, _, _, _, _) = parent_state + .mutable_validator_fields() + .expect("parent state should have Altair validator fields"); + // Slash this epoch's timely-target voters (they count toward the current-epoch target balance), + // excluding the child proposer, to drop target balance below the 2/3 justification threshold. + let slash_indices = current_participation + .iter() + .enumerate() + .filter_map(|(index, flags)| { + flags + .has_flag(TIMELY_TARGET_FLAG_INDEX) + .ok() + .and_then(|has_flag| has_flag.then_some(index as u64)) + }) + .filter(|index| *index != child_proposer) + .take(slash_count_needed as usize) + .collect::>(); + + assert_eq!( + slash_indices.len(), + slash_count_needed as usize, + "should have enough current target attesters to slash" + ); + + inject_slashings(&harness, &slash_indices); + + harness.advance_slot(); + let child_root = harness + .extend_chain( + 1, + BlockStrategy::OnCanonicalHead, + AttestationStrategy::AllValidators, + ) + .await; + + let ( + stored_parent_justified, + stored_parent_finalized, + stored_child_justified, + stored_child_finalized, + child_slot, + ) = { + let fork_choice = harness.chain.canonical_head.fork_choice_read_lock(); + let parent_block = fork_choice + .get_block(&parent_root) + .expect("parent should be in fork choice"); + let child_block = fork_choice + .get_block(&child_root) + .expect("child should be in fork choice"); + + let parent_justified = parent_block + .unrealized_justified_checkpoint + .expect("parent should have unrealized justified checkpoint"); + let parent_finalized = parent_block + .unrealized_finalized_checkpoint + .expect("parent should have unrealized finalized checkpoint"); + + assert_eq!(parent_block.slot, parent_slot); + assert_eq!(parent_justified.epoch, parent_epoch); + assert_eq!(parent_finalized.epoch.saturating_add(1u64), parent_epoch); + assert_eq!(child_block.slot, parent_slot + 1); + assert_eq!(child_block.slot.epoch(slots_per_epoch), parent_epoch); + + ( + parent_justified, + parent_finalized, + child_block + .unrealized_justified_checkpoint + .expect("child should have unrealized justified checkpoint"), + child_block + .unrealized_finalized_checkpoint + .expect("child should have unrealized finalized checkpoint"), + child_block.slot, + ) + }; + + let mut child_state = harness + .chain + .state_at_slot(child_slot, StateSkipConfig::WithStateRoots) + .expect("should load child state"); + child_state + .build_caches(&harness.chain.spec) + .expect("should build child state caches"); + + let slashed = child_state + .validators() + .iter() + .enumerate() + .filter_map(|(index, validator)| validator.slashed.then_some(index as u64)) + .collect::>(); + let child_total_active_balance = child_state + .get_total_active_balance() + .expect("should get child total active balance"); + let child_current_target_balance = child_state + .progressive_balances_cache() + .current_epoch_target_attesting_balance() + .expect("should get child current target balance"); + let child_justification_and_finalization = + per_epoch_processing::altair::process_justification_and_finalization(&child_state) + .expect("should recompute child justification and finalization"); + let expected_child_justified = + child_justification_and_finalization.current_justified_checkpoint(); + let expected_child_finalized = child_justification_and_finalization.finalized_checkpoint(); + + assert_eq!(slashed, slash_indices); + assert!( + child_current_target_balance < ceil_two_thirds(child_total_active_balance), + "slashings should reduce current target balance below the justification threshold" + ); + assert_ne!( + expected_child_justified, stored_parent_justified, + "test setup should make the child justified checkpoint differ from the parent's" + ); + assert_ne!( + expected_child_finalized, stored_parent_finalized, + "test setup should make the child finalized checkpoint differ from the parent's" + ); + + SameEpochSlashingChild { + harness, + stored_parent_justified, + stored_parent_finalized, + stored_child_justified, + stored_child_finalized, + expected_child_justified, + expected_child_finalized, + parent_epoch, + } +} diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index e648d5669b..aca5ab7851 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -894,22 +894,29 @@ where // Update unrealized justified/finalized checkpoints. let block_epoch = block.slot().epoch(E::slots_per_epoch()); - // If the parent checkpoints are already at the same epoch as the block being imported, - // it's impossible for the unrealized checkpoints to differ from the parent's. This - // holds true because: + // If the block has no slashings and the parent checkpoints are already at the same epoch as + // the block being imported, it's impossible for the unrealized checkpoints to differ from + // the parent's. This holds true because: // // 1. A child block cannot have lower FFG checkpoints than its parent. // 2. A block in epoch `N` cannot contain attestations which would justify an epoch higher than `N`. // 3. A block in epoch `N` cannot contain attestations which would finalize an epoch higher than `N - 1`. // + // Slashings are excluded from this optimization because they can reduce unslashed + // participation in the child state and therefore lower the child's unrealized checkpoints. + // // This is an optimization. It should reduce the amount of times we run // `process_justification_and_finalization` by approximately 1/3rd when the chain is // performing optimally. + let has_slashings = !block.body().proposer_slashings().is_empty() + || block.body().attester_slashings_len() > 0; let parent_checkpoints = parent_block .unrealized_justified_checkpoint .zip(parent_block.unrealized_finalized_checkpoint) .filter(|(parent_justified, parent_finalized)| { - parent_justified.epoch == block_epoch && parent_finalized.epoch + 1 == block_epoch + !has_slashings + && parent_justified.epoch == block_epoch + && parent_finalized.epoch.saturating_add(1u64) == block_epoch }); let (unrealized_justified_checkpoint, unrealized_finalized_checkpoint) =