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 diff --git a/.github/workflows/warpbuild-ubuntu-latest-snapshot.yml b/.github/workflows/warpbuild-ubuntu-latest-snapshot.yml index f32a0f0545..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' @@ -16,7 +19,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: | diff --git a/Cargo.lock b/Cargo.lock index c0f337a6a8..c0344bb7c8 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" @@ -376,7 +364,7 @@ dependencies = [ "either", "futures", "futures-utils-wasm", - "lru 0.13.0", + "lru", "parking_lot", "pin-project", "reqwest", @@ -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]] @@ -1241,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", @@ -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]] @@ -3407,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", @@ -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" @@ -4265,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", @@ -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", @@ -5574,6 +5549,7 @@ dependencies = [ "fixed_bytes", "fnv", "futures", + "hashlink", "hex", "if-addrs 0.14.0", "itertools 0.14.0", @@ -5581,7 +5557,6 @@ dependencies = [ "libp2p-mplex", "lighthouse_version", "logging", - "lru 0.12.5", "lru_cache", "metrics", "network_utils", @@ -5728,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" @@ -6330,7 +6296,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 +7134,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 +7147,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 +7218,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 +7255,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 +7724,7 @@ dependencies = [ "bitflags 2.10.0", "fallible-iterator", "fallible-streaming-iterator", - "hashlink 0.11.0", + "hashlink", "libsqlite3-sys", "smallvec", "sqlite-wasm-rs", @@ -7836,7 +7802,7 @@ dependencies = [ "errno", "libc", "linux-raw-sys", - "windows-sys 0.59.0", + "windows-sys 0.61.2", ] [[package]] @@ -8440,10 +8406,10 @@ dependencies = [ "filesystem", "fixed_bytes", "flate2", + "hashlink", "libmdbx", "lmdb-rkv", "lmdb-rkv-sys", - "lru 0.12.5", "maplit", "metrics", "parking_lot", @@ -8559,7 +8525,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]] @@ -8677,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", @@ -8904,7 +8870,7 @@ dependencies = [ "getrandom 0.3.4", "once_cell", "rustix", - "windows-sys 0.59.0", + "windows-sys 0.61.2", ] [[package]] @@ -10268,7 +10234,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 +10826,7 @@ checksum = "631a50d867fafb7093e709d75aaee9e0e0d5deb934021fcea25ac2fe09edc51e" dependencies = [ "arraydeque", "encoding_rs", - "hashlink 0.11.0", + "hashlink", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index 50b1733232..6e0b1ddf3a 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" } @@ -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/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 c6d6a09399..646aab6fc7 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 @@ -4248,12 +4249,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 +4280,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 +5027,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, @@ -5207,7 +5165,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 +5201,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 +5244,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/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/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/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 cf11773292..ca9d222ddc 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/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..3c0f43fef0 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::{ @@ -201,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 @@ -218,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); @@ -339,7 +344,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/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/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/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..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, @@ -310,7 +311,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 +337,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 +385,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 +407,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 +429,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 +456,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 +484,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 +509,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 +535,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 +557,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 +573,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 +610,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 +647,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 +676,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 +703,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 +758,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..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,18 +108,15 @@ 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" ); // 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); @@ -198,6 +195,7 @@ impl BeaconChain { block_root, payload_verification_outcome, } = *envelope; + let slot = envelope.envelope.slot(); let block_root = { let chain = self.clone(); @@ -214,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/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/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_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/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/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/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/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/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/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/beacon/execution_payload_envelopes.rs b/beacon_node/http_api/src/beacon/execution_payload_envelopes.rs index f8ab8cddc8..d058f66001 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/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/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/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/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/interactive_tests.rs b/beacon_node/http_api/tests/interactive_tests.rs index 7b5fb02714..9258dab1af 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, @@ -909,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 @@ -921,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/http_api/tests/tests.rs b/beacon_node/http_api/tests/tests.rs index 455a957337..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 } @@ -7967,8 +7980,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 +7994,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 +8033,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 +9611,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/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/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/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/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/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/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/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 98c143eaeb..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. @@ -3795,16 +3903,83 @@ 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(_, 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(_) => {} + Err(e) => { + debug!( + ?beacon_block_root, + %peer_id, + error = ?e, + "Execution payload envelope processing failed" + ); + } } } + /// 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, @@ -3829,7 +4004,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 +4184,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( @@ -4044,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 e2226af094..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,6 +371,13 @@ impl NetworkBeaconProcessor { let result: Result = result.map_err(|e| BlockError::InternalError(format!("envelope: {e}"))); + // 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 { process_type, result: result.into(), @@ -1018,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/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| { 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/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/beacon_node/src/cli.rs b/beacon_node/src/cli.rs index 988e2d1fc5..3cf6a6efd2 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( @@ -681,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) ) /* @@ -1375,12 +1370,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..d27909bddf 100644 --- a/beacon_node/src/config.rs +++ b/beacon_node/src/config.rs @@ -1,13 +1,13 @@ 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; 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; @@ -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. @@ -771,19 +769,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 = @@ -1443,8 +1432,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/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/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..45e10b0d11 100644 --- a/book/src/help_bn.md +++ b/book/src/help_bn.md @@ -84,6 +84,14 @@ 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] + --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 @@ -308,12 +316,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 @@ -476,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. @@ -494,13 +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-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 - 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/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/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/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/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); diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index f15a6b2932..fbdfe577e9 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::{ @@ -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 @@ -622,7 +643,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 +675,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 +685,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 +695,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) @@ -772,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); @@ -784,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 @@ -837,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 @@ -866,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) = @@ -1036,6 +1071,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, @@ -1585,9 +1622,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/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/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/fork_choice_test_definition.rs b/consensus/proto_array/src/fork_choice_test_definition.rs index 7cde1023cf..7299aae550 100644 --- a/consensus/proto_array/src/fork_choice_test_definition.rs +++ b/consensus/proto_array/src/fork_choice_test_definition.rs @@ -132,6 +132,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)] @@ -329,6 +338,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) @@ -629,6 +639,30 @@ impl ForkChoiceTestDefinition { assert_eq!( actual, expected, "latest_parent_full_block mismatch at op index {}", + 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 8417e2964b..e7033ef0a3 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/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.rs b/consensus/proto_array/src/proto_array.rs index 929792528e..d735c9e156 100644 --- a/consensus/proto_array/src/proto_array.rs +++ b/consensus/proto_array/src/proto_array.rs @@ -1596,10 +1596,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 { @@ -1611,10 +1614,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 62a5c6e8e9..10a5263700 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 { @@ -385,10 +387,6 @@ pub enum DoNotReOrg { MissingHeadFinalizedCheckpoint, ParentDistance, HeadDistance, - ShufflingUnstable, - DisallowedOffset { - offset: u64, - }, JustificationAndFinalizationNotCompetitive, ChainNotFinalizing { epochs_since_finalization: u64, @@ -413,10 +411,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 +456,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, @@ -535,6 +504,7 @@ impl ProtoArrayForkChoice { execution_payload_parent_hash, execution_payload_block_hash, proposer_index: Some(proposer_index), + payload_received: false, }; proto_array @@ -724,7 +694,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 +702,6 @@ impl ProtoArrayForkChoice { justified_balances, re_org_head_threshold, re_org_parent_threshold, - disallowed_offsets, max_epochs_since_finalization, )?; @@ -784,7 +752,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 +790,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() @@ -1007,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), }) } @@ -1016,6 +972,7 @@ impl ProtoArrayForkChoice { &self, block_root: &Hash256, parent_payload_status: PayloadStatus, + current_slot: Slot, ) -> Result { let block_index = self .proto_array @@ -1033,7 +990,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:?}")) } @@ -1445,6 +1402,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, @@ -1473,6 +1431,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, @@ -1609,6 +1568,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, 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/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/lighthouse/tests/beacon_node.rs b/lighthouse/tests/beacon_node.rs index 38d4275a02..b8fd978ac5 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(); } @@ -2812,10 +2785,49 @@ 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() - .flag("enable-partial-columns", None) + .flag("enable-partial-columns", Some("true")) .run_with_zero_port() .with_config(|config| { assert!(config.network.enable_partial_columns); @@ -2830,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() @@ -2853,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); @@ -2865,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", - ); -} 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/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); } } 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 f640583189..9edc5b85c8 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::{ @@ -54,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)] @@ -133,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, @@ -170,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)] @@ -241,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, }, ) } @@ -390,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()) } @@ -674,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); @@ -787,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); @@ -849,7 +863,6 @@ impl Tester { block_delay, &state, PayloadVerificationStatus::Irrelevant, - block.message().proposer_index(), &self.harness.chain.spec, ); @@ -864,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) { @@ -910,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> { @@ -1040,7 +1080,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 { 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(); 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) 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(()) => {