From c59e4a0cee78d311ffe17d8045cbd82032b501c9 Mon Sep 17 00:00:00 2001 From: Mac L Date: Fri, 13 Feb 2026 00:51:39 +0400 Subject: [PATCH] Disable `legacy-arith` by default in `consensus/types` (#8695) Currently, `consensus/types` cannot build with `no-default-features` since we use "legacy" standard arithmetic operations. - Remove the offending arithmetic to fix compilation. - Rename `legacy-arith` to `saturating-arith` and disable it by default. Co-Authored-By: Mac L --- Cargo.toml | 2 +- beacon_node/beacon_chain/src/beacon_chain.rs | 2 +- .../beacon_chain/src/early_attester_cache.rs | 8 +- consensus/state_processing/Cargo.toml | 3 +- consensus/types/Cargo.toml | 7 +- consensus/types/src/core/slot_epoch.rs | 2 +- consensus/types/src/core/slot_epoch_macros.rs | 6 +- consensus/types/src/state/beacon_state.rs | 7 +- consensus/types/src/state/committee_cache.rs | 125 ++++++++++-------- consensus/types/src/state/mod.rs | 2 +- consensus/types/tests/committee_cache.rs | 4 +- 11 files changed, 91 insertions(+), 77 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 100a916c50..98e8c057b5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -271,7 +271,7 @@ tracing_samplers = { path = "common/tracing_samplers" } tree_hash = "0.12.0" tree_hash_derive = "0.12.0" typenum = "1" -types = { path = "consensus/types" } +types = { path = "consensus/types", features = ["saturating-arith"] } url = "2" uuid = { version = "0.8", features = ["serde", "v4"] } validator_client = { path = "validator_client" } diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index ec79153785..4ae7871758 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -1665,7 +1665,7 @@ impl BeaconChain { let validator_index = *validator_index as usize; committee_cache.get_attestation_duties(validator_index) }) - .collect(); + .collect::, _>>()?; Ok((duties, dependent_root)) }, diff --git a/beacon_node/beacon_chain/src/early_attester_cache.rs b/beacon_node/beacon_chain/src/early_attester_cache.rs index 8d9eb950f3..752e4d1a96 100644 --- a/beacon_node/beacon_chain/src/early_attester_cache.rs +++ b/beacon_node/beacon_chain/src/early_attester_cache.rs @@ -2,6 +2,7 @@ use crate::data_availability_checker::{AvailableBlock, AvailableBlockData}; use crate::{BeaconChainError as Error, metrics}; use parking_lot::RwLock; use proto_array::Block as ProtoBlock; +use safe_arith::SafeArith; use std::sync::Arc; use tracing::instrument; use types::*; @@ -59,12 +60,13 @@ impl CommitteeLengths { slots_per_epoch, committees_per_slot, committee_index as usize, - ); + )?; + let epoch_committee_count = committees_per_slot.safe_mul(slots_per_epoch)?; let range = compute_committee_range_in_epoch( - epoch_committee_count(committees_per_slot, slots_per_epoch), + epoch_committee_count, index_in_epoch, self.active_validator_indices_len, - ) + )? .ok_or(Error::EarlyAttesterCacheError)?; range diff --git a/consensus/state_processing/Cargo.toml b/consensus/state_processing/Cargo.toml index a08035d583..a83e443e80 100644 --- a/consensus/state_processing/Cargo.toml +++ b/consensus/state_processing/Cargo.toml @@ -5,9 +5,8 @@ authors = ["Paul Hauner ", "Michael Sproul { impl Add<$other> for $main { @@ -321,9 +321,9 @@ macro_rules! impl_common { impl_u64_eq_ord!($type); impl_safe_arith!($type, $type); impl_safe_arith!($type, u64); - #[cfg(feature = "legacy-arith")] + #[cfg(feature = "saturating-arith")] impl_math_between!($type, $type); - #[cfg(feature = "legacy-arith")] + #[cfg(feature = "saturating-arith")] impl_math_between!($type, u64); impl_math!($type); impl_display!($type); diff --git a/consensus/types/src/state/beacon_state.rs b/consensus/types/src/state/beacon_state.rs index 6838b588eb..6228e40ef8 100644 --- a/consensus/types/src/state/beacon_state.rs +++ b/consensus/types/src/state/beacon_state.rs @@ -876,7 +876,7 @@ impl BeaconState { relative_epoch: RelativeEpoch, ) -> Result { let cache = self.committee_cache(relative_epoch)?; - Ok(cache.epoch_committee_count() as u64) + Ok(cache.epoch_committee_count()? as u64) } /// Return the cached active validator indices at some epoch. @@ -2150,7 +2150,7 @@ impl BeaconState { ) -> Result, BeaconStateError> { let cache = self.committee_cache(relative_epoch)?; - Ok(cache.get_attestation_duties(validator_index)) + Ok(cache.get_attestation_duties(validator_index)?) } /// Check if the attestation is for the block proposed at the attestation slot. @@ -2909,7 +2909,6 @@ impl BeaconState { } } - #[allow(clippy::arithmetic_side_effects)] pub fn rebase_on(&mut self, base: &Self, spec: &ChainSpec) -> Result<(), BeaconStateError> { // Required for macros (which use type-hints internally). @@ -3218,7 +3217,6 @@ impl BeaconState { )) } - #[allow(clippy::arithmetic_side_effects)] pub fn apply_pending_mutations(&mut self) -> Result<(), BeaconStateError> { match self { Self::Base(inner) => { @@ -3321,7 +3319,6 @@ impl BeaconState { pub fn get_beacon_state_leaves(&self) -> Vec { let mut leaves = vec![]; - #[allow(clippy::arithmetic_side_effects)] match self { BeaconState::Base(state) => { map_beacon_state_base_fields!(state, |_, field| { diff --git a/consensus/types/src/state/committee_cache.rs b/consensus/types/src/state/committee_cache.rs index 39e9011ef4..4a28f3c689 100644 --- a/consensus/types/src/state/committee_cache.rs +++ b/consensus/types/src/state/committee_cache.rs @@ -1,9 +1,7 @@ -#![allow(clippy::arithmetic_side_effects)] - use std::{num::NonZeroUsize, ops::Range, sync::Arc}; use educe::Educe; -use safe_arith::SafeArith; +use safe_arith::{ArithError, SafeArith}; use serde::{Deserialize, Serialize}; use ssz::{Decode, DecodeError, Encode, four_byte_option_impl}; use ssz_derive::{Decode, Encode}; @@ -79,7 +77,13 @@ impl CommitteeCache { .saturating_sub(spec.min_seed_lookahead) .saturating_sub(1u64); - if reqd_randao_epoch < state.min_randao_epoch() || epoch > state.current_epoch() + 1 { + if reqd_randao_epoch < state.min_randao_epoch() + || epoch + > state + .current_epoch() + .safe_add(1) + .map_err(BeaconStateError::ArithError)? + { return Err(BeaconStateError::EpochOutOfBounds); } @@ -118,7 +122,7 @@ impl CommitteeCache { *shuffling_positions .get_mut(v) .ok_or(BeaconStateError::ShuffleIndexOutOfBounds(v))? = - NonZeroUsize::new(i + 1).into(); + NonZeroUsize::new(i.safe_add(1).map_err(BeaconStateError::ArithError)?).into(); } Ok(Arc::new(CommitteeCache { @@ -177,8 +181,9 @@ impl CommitteeCache { self.slots_per_epoch as usize, self.committees_per_slot as usize, index as usize, - ); - let committee = self.compute_committee(committee_index)?; + ) + .ok()?; + let committee = self.compute_committee(committee_index).ok()??; Some(BeaconCommittee { slot, @@ -212,8 +217,9 @@ impl CommitteeCache { .initialized_epoch .ok_or(BeaconStateError::CommitteeCacheUninitialized(None))?; + let capacity = self.epoch_committee_count()?; initialized_epoch.slot_iter(self.slots_per_epoch).try_fold( - Vec::with_capacity(self.epoch_committee_count()), + Vec::with_capacity(capacity), |mut vec, slot| { vec.append(&mut self.get_beacon_committees_at_slot(slot)?); Ok(vec) @@ -225,43 +231,53 @@ impl CommitteeCache { /// /// Returns `None` if the `validator_index` does not exist, does not have duties or `Self` is /// non-initialized. - pub fn get_attestation_duties(&self, validator_index: usize) -> Option { - let i = self.shuffled_position(validator_index)?; + pub fn get_attestation_duties( + &self, + validator_index: usize, + ) -> Result, ArithError> { + let Some(i) = self.shuffled_position(validator_index) else { + return Ok(None); + }; - (0..self.epoch_committee_count()) - .map(|nth_committee| (nth_committee, self.compute_committee_range(nth_committee))) - .find(|(_, range)| { - if let Some(range) = range { - range.start <= i && range.end > i - } else { - false - } - }) - .and_then(|(nth_committee, range)| { - let (slot, index) = self.convert_to_slot_and_index(nth_committee as u64)?; - let range = range?; - let committee_position = i - range.start; - let committee_len = range.end - range.start; + for nth_committee in 0..self.epoch_committee_count()? { + let Some(range) = self.compute_committee_range(nth_committee)? else { + continue; + }; - Some(AttestationDuty { + if range.start <= i && range.end > i { + let Some((slot, index)) = self.convert_to_slot_and_index(nth_committee as u64)? + else { + return Ok(None); + }; + + let committee_position = i.safe_sub(range.start)?; + let committee_len = range.end.safe_sub(range.start)?; + + return Ok(Some(AttestationDuty { slot, index, committee_position, committee_len, committees_at_slot: self.committees_per_slot(), - }) - }) + })); + } + } + + Ok(None) } /// Convert an index addressing the list of all epoch committees into a slot and per-slot index. fn convert_to_slot_and_index( &self, global_committee_index: u64, - ) -> Option<(Slot, CommitteeIndex)> { - let epoch_start_slot = self.initialized_epoch?.start_slot(self.slots_per_epoch); - let slot_offset = global_committee_index / self.committees_per_slot; - let index = global_committee_index % self.committees_per_slot; - Some((epoch_start_slot.safe_add(slot_offset).ok()?, index)) + ) -> Result, ArithError> { + let Some(epoch) = self.initialized_epoch else { + return Ok(None); + }; + let epoch_start_slot = epoch.start_slot(self.slots_per_epoch); + let slot_offset = global_committee_index.safe_div(self.committees_per_slot)?; + let index = global_committee_index.safe_rem(self.committees_per_slot)?; + Ok(Some((epoch_start_slot.safe_add(slot_offset)?, index))) } /// Returns the number of active validators in the initialized epoch. @@ -278,11 +294,8 @@ impl CommitteeCache { /// Always returns `usize::default()` for a non-initialized epoch. /// /// Spec v0.12.1 - pub fn epoch_committee_count(&self) -> usize { - epoch_committee_count( - self.committees_per_slot as usize, - self.slots_per_epoch as usize, - ) + pub fn epoch_committee_count(&self) -> Result { + (self.committees_per_slot as usize).safe_mul(self.slots_per_epoch as usize) } /// Returns the number of committees per slot for this cache's epoch. @@ -293,19 +306,23 @@ impl CommitteeCache { /// Returns a slice of `self.shuffling` that represents the `index`'th committee in the epoch. /// /// Spec v0.12.1 - fn compute_committee(&self, index: usize) -> Option<&[usize]> { - self.shuffling.get(self.compute_committee_range(index)?) + fn compute_committee(&self, index: usize) -> Result, ArithError> { + if let Some(range) = self.compute_committee_range(index)? { + Ok(self.shuffling.get(range)) + } else { + Ok(None) + } } /// Returns a range of `self.shuffling` that represents the `index`'th committee in the epoch. /// - /// To avoid a divide-by-zero, returns `None` if `self.committee_count` is zero. + /// To avoid a divide-by-zero, returns `Ok(None)` if `self.committee_count` is zero. /// - /// Will also return `None` if the index is out of bounds. + /// Will also return `Ok(None)` if the index is out of bounds. /// /// Spec v0.12.1 - fn compute_committee_range(&self, index: usize) -> Option> { - compute_committee_range_in_epoch(self.epoch_committee_count(), index, self.shuffling.len()) + fn compute_committee_range(&self, index: usize) -> Result>, ArithError> { + compute_committee_range_in_epoch(self.epoch_committee_count()?, index, self.shuffling.len()) } /// Returns the index of some validator in `self.shuffling`. @@ -329,8 +346,10 @@ pub fn compute_committee_index_in_epoch( slots_per_epoch: usize, committees_per_slot: usize, committee_index: usize, -) -> usize { - (slot.as_usize() % slots_per_epoch) * committees_per_slot + committee_index +) -> Result { + (slot.as_usize().safe_rem(slots_per_epoch)?) + .safe_mul(committees_per_slot)? + .safe_add(committee_index) } /// Computes the range for slicing the shuffled indices to determine the members of a committee. @@ -341,20 +360,16 @@ pub fn compute_committee_range_in_epoch( epoch_committee_count: usize, index_in_epoch: usize, shuffling_len: usize, -) -> Option> { +) -> Result>, ArithError> { if epoch_committee_count == 0 || index_in_epoch >= epoch_committee_count { - return None; + return Ok(None); } - let start = (shuffling_len * index_in_epoch) / epoch_committee_count; - let end = (shuffling_len * (index_in_epoch + 1)) / epoch_committee_count; + let start = (shuffling_len.safe_mul(index_in_epoch))?.safe_div(epoch_committee_count)?; + let end = + (shuffling_len.safe_mul(index_in_epoch.safe_add(1)?))?.safe_div(epoch_committee_count)?; - Some(start..end) -} - -/// Returns the total number of committees in an epoch. -pub fn epoch_committee_count(committees_per_slot: usize, slots_per_epoch: usize) -> usize { - committees_per_slot * slots_per_epoch + Ok(Some(start..end)) } /// Returns a list of all `validators` indices where the validator is active at the given diff --git a/consensus/types/src/state/mod.rs b/consensus/types/src/state/mod.rs index ea064fb7ac..096bb67167 100644 --- a/consensus/types/src/state/mod.rs +++ b/consensus/types/src/state/mod.rs @@ -21,7 +21,7 @@ pub use beacon_state::{ }; pub use committee_cache::{ CommitteeCache, compute_committee_index_in_epoch, compute_committee_range_in_epoch, - epoch_committee_count, get_active_validator_indices, + get_active_validator_indices, }; pub use epoch_cache::{EpochCache, EpochCacheError, EpochCacheKey}; pub use exit_cache::ExitCache; diff --git a/consensus/types/tests/committee_cache.rs b/consensus/types/tests/committee_cache.rs index 751ef05d29..0bb8aa1da2 100644 --- a/consensus/types/tests/committee_cache.rs +++ b/consensus/types/tests/committee_cache.rs @@ -33,9 +33,9 @@ fn default_values() { assert!(!cache.is_initialized_at(Epoch::new(0))); assert!(&cache.active_validator_indices().is_empty()); assert_eq!(cache.get_beacon_committee(Slot::new(0), 0), None); - assert_eq!(cache.get_attestation_duties(0), None); + assert_eq!(cache.get_attestation_duties(0), Ok(None)); assert_eq!(cache.active_validator_count(), 0); - assert_eq!(cache.epoch_committee_count(), 0); + assert_eq!(cache.epoch_committee_count(), Ok(0)); assert!(cache.get_beacon_committees_at_slot(Slot::new(0)).is_err()); }