gnosis/lighthouse
1
0
Fork 0
mirror of https://github.com/sigp/lighthouse.git synced 2026-04-30 19:23:50 +00:00
Code Issues Packages Projects Releases Wiki Activity

Remove saturating arith from state_processing (#1644)

Browse Source 
## Issue Addressed

Resolves #1100

## Proposed Changes

* Implement the `SafeArith` trait for `Slot` and `Epoch`, so that methods like `safe_add` become available.
* Tweak the `SafeArith` trait to allow a different `Rhs` type (analagous to `std::ops::Add`, etc).
* Add a `legacy-arith` feature to `types` and `state_processing` that conditionally enables implementations of
  the `std` ops with saturating semantics.
* Check compilation of `types` and `state_processing` _without_ `legacy-arith` on CI,
  thus guaranteeing that they only use the `SafeArith` primitives 🎉

## Additional Info

The `legacy-arith` feature gets turned on by all higher-level crates that depend on `state_processing` or `types`, thus allowing the beacon chain, networking, and other components to continue to rely on the availability of ops like `+`, `-`, `*`, etc.

**This is a consensus-breaking change**, but brings us in line with the spec, and our incompatibilities shouldn't have been reachable with any valid configuration of Eth2 parameters.
This commit is contained in:
Michael Sproul
2020-09-25 05:18:21 +00:00
parent 28b6d921c6
commit 3412a3ec54
26 changed files with 250 additions and 141 deletions
Download Patch File Download Diff File Expand all files Collapse all files

49
consensus/types/src/beacon_state.rs
View File

@@ -100,10 +100,10 @@ enum AllowNextEpoch {
}
impl AllowNextEpoch {
fn upper_bound_of(self, current_epoch: Epoch) -> Epoch {
fn upper_bound_of(self, current_epoch: Epoch) -> Result<Epoch, Error> {
match self {
AllowNextEpoch::True => current_epoch + 1,
AllowNextEpoch::False => current_epoch,
AllowNextEpoch::True => Ok(current_epoch.safe_add(1)?),
AllowNextEpoch::False => Ok(current_epoch),
}
}
}
@@ -323,7 +323,9 @@ impl<T: EthSpec> BeaconState<T> {
pub fn previous_epoch(&self) -> Epoch {
let current_epoch = self.current_epoch();
if current_epoch > T::genesis_epoch() {
current_epoch - 1
current_epoch
.safe_sub(1)
.expect("current epoch greater than genesis implies greater than 0")
} else {
current_epoch
}
@@ -332,8 +334,8 @@ impl<T: EthSpec> BeaconState<T> {
/// The epoch following `self.current_epoch()`.
///
/// Spec v0.12.1
pub fn next_epoch(&self) -> Epoch {
self.current_epoch() + 1
pub fn next_epoch(&self) -> Result<Epoch, Error> {
Ok(self.current_epoch().safe_add(1)?)
}
/// Compute the number of committees at `slot`.
@@ -378,7 +380,7 @@ impl<T: EthSpec> BeaconState<T> {
epoch: Epoch,
spec: &ChainSpec,
) -> Result<Vec<usize>, Error> {
if epoch >= self.compute_activation_exit_epoch(self.current_epoch(), spec) {
if epoch >= self.compute_activation_exit_epoch(self.current_epoch(), spec)? {
Err(BeaconStateError::EpochOutOfBounds)
} else {
Ok(get_active_validator_indices(&self.validators, epoch))
@@ -475,7 +477,7 @@ impl<T: EthSpec> BeaconState<T> {
{
return Ok(candidate_index);
}
i.increment()?;
i.safe_add_assign(1)?;
}
}
@@ -553,7 +555,7 @@ impl<T: EthSpec> BeaconState<T> {
///
/// Spec v0.12.1
fn get_latest_block_roots_index(&self, slot: Slot) -> Result<usize, Error> {
if slot < self.slot && self.slot <= slot + self.block_roots.len() as u64 {
if slot < self.slot && self.slot <= slot.safe_add(self.block_roots.len() as u64)? {
Ok(slot.as_usize().safe_rem(self.block_roots.len())?)
} else {
Err(BeaconStateError::SlotOutOfBounds)
@@ -605,7 +607,9 @@ impl<T: EthSpec> BeaconState<T> {
let current_epoch = self.current_epoch();
let len = T::EpochsPerHistoricalVector::to_u64();
if current_epoch < epoch + len && epoch <= allow_next_epoch.upper_bound_of(current_epoch) {
if current_epoch < epoch.safe_add(len)?
&& epoch <= allow_next_epoch.upper_bound_of(current_epoch)?
{
Ok(epoch.as_usize().safe_rem(len as usize)?)
} else {
Err(Error::EpochOutOfBounds)
@@ -652,7 +656,7 @@ impl<T: EthSpec> BeaconState<T> {
///
/// Spec v0.12.1
fn get_latest_state_roots_index(&self, slot: Slot) -> Result<usize, Error> {
if slot < self.slot && self.slot <= slot + self.state_roots.len() as u64 {
if slot < self.slot && self.slot <= slot.safe_add(self.state_roots.len() as u64)? {
Ok(slot.as_usize().safe_rem(self.state_roots.len())?)
} else {
Err(BeaconStateError::SlotOutOfBounds)
@@ -672,7 +676,7 @@ impl<T: EthSpec> BeaconState<T> {
/// Spec v0.12.1
pub fn get_oldest_state_root(&self) -> Result<&Hash256, Error> {
let i =
self.get_latest_state_roots_index(self.slot - Slot::from(self.state_roots.len()))?;
self.get_latest_state_roots_index(self.slot.saturating_sub(self.state_roots.len()))?;
Ok(&self.state_roots[i])
}
@@ -680,7 +684,9 @@ impl<T: EthSpec> BeaconState<T> {
///
/// Spec v0.12.1
pub fn get_oldest_block_root(&self) -> Result<&Hash256, Error> {
let i = self.get_latest_block_roots_index(self.slot - self.block_roots.len() as u64)?;
let i = self.get_latest_block_roots_index(
self.slot.saturating_sub(self.block_roots.len() as u64),
)?;
Ok(&self.block_roots[i])
}
@@ -712,8 +718,8 @@ impl<T: EthSpec> BeaconState<T> {
// We allow the slashings vector to be accessed at any cached epoch at or before
// the current epoch, or the next epoch if `AllowNextEpoch::True` is passed.
let current_epoch = self.current_epoch();
if current_epoch < epoch + T::EpochsPerSlashingsVector::to_u64()
&& epoch <= allow_next_epoch.upper_bound_of(current_epoch)
if current_epoch < epoch.safe_add(T::EpochsPerSlashingsVector::to_u64())?
&& epoch <= allow_next_epoch.upper_bound_of(current_epoch)?
{
Ok(epoch
.as_usize()
@@ -775,7 +781,10 @@ impl<T: EthSpec> BeaconState<T> {
// Bypass the safe getter for RANDAO so we can gracefully handle the scenario where `epoch
// == 0`.
let mix = {
let i = epoch + T::EpochsPerHistoricalVector::to_u64() - spec.min_seed_lookahead - 1;
let i = epoch
.safe_add(T::EpochsPerHistoricalVector::to_u64())?
.safe_sub(spec.min_seed_lookahead)?
.safe_sub(1)?;
self.randao_mixes[i.as_usize().safe_rem(self.randao_mixes.len())?]
};
let domain_bytes = int_to_bytes4(spec.get_domain_constant(domain_type));
@@ -811,8 +820,12 @@ impl<T: EthSpec> BeaconState<T> {
/// Return the epoch at which an activation or exit triggered in ``epoch`` takes effect.
///
/// Spec v0.12.1
pub fn compute_activation_exit_epoch(&self, epoch: Epoch, spec: &ChainSpec) -> Epoch {
epoch + 1 + spec.max_seed_lookahead
pub fn compute_activation_exit_epoch(
&self,
epoch: Epoch,
spec: &ChainSpec,
) -> Result<Epoch, Error> {
Ok(epoch.safe_add(1)?.safe_add(spec.max_seed_lookahead)?)
}
/// Return the churn limit for the current epoch (number of validators who can leave per epoch).

3
consensus/types/src/beacon_state/committee_cache.rs
View File

@@ -3,6 +3,7 @@
use super::BeaconState;
use crate::*;
use core::num::NonZeroUsize;
use safe_arith::SafeArith;
use serde_derive::{Deserialize, Serialize};
use ssz_derive::{Decode, Encode};
use std::ops::Range;
@@ -197,7 +198,7 @@ impl CommitteeCache {
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 + slot_offset, index))
Some((epoch_start_slot.safe_add(slot_offset).ok()?, index))
}
/// Returns the number of active validators in the initialized epoch.

8
consensus/types/src/beacon_state/committee_cache/tests.rs
View File

@@ -53,8 +53,8 @@ fn initializes_with_the_right_epoch() {
let cache = CommitteeCache::initialized(&state, state.previous_epoch(), &spec).unwrap();
assert_eq!(cache.initialized_epoch, Some(state.previous_epoch()));
let cache = CommitteeCache::initialized(&state, state.next_epoch(), &spec).unwrap();
assert_eq!(cache.initialized_epoch, Some(state.next_epoch()));
let cache = CommitteeCache::initialized(&state, state.next_epoch().unwrap(), &spec).unwrap();
assert_eq!(cache.initialized_epoch, Some(state.next_epoch().unwrap()));
}
#[test]
@@ -81,7 +81,7 @@ fn shuffles_for_the_right_epoch() {
.get_seed(state.current_epoch(), Domain::BeaconAttester, spec)
.unwrap();
let next_seed = state
.get_seed(state.next_epoch(), Domain::BeaconAttester, spec)
.get_seed(state.next_epoch().unwrap(), Domain::BeaconAttester, spec)
.unwrap();
assert!((previous_seed != current_seed) && (current_seed != next_seed));
@@ -114,7 +114,7 @@ fn shuffles_for_the_right_epoch() {
assert_eq!(cache.shuffling, shuffling_with_seed(previous_seed));
assert_shuffling_positions_accurate(&cache);
let cache = CommitteeCache::initialized(&state, state.next_epoch(), spec).unwrap();
let cache = CommitteeCache::initialized(&state, state.next_epoch().unwrap(), spec).unwrap();
assert_eq!(cache.shuffling, shuffling_with_seed(next_seed));
assert_shuffling_positions_accurate(&cache);
}

2
consensus/types/src/beacon_state/exit_cache.rs
View File

@@ -44,7 +44,7 @@ impl ExitCache {
self.exit_epoch_counts
.entry(exit_epoch)
.or_insert(0)
.increment()?;
.safe_add_assign(1)?;
Ok(())
}

17
consensus/types/src/relative_epoch.rs
View File

@@ -1,9 +1,17 @@
use crate::*;
use safe_arith::{ArithError, SafeArith};
#[derive(Debug, PartialEq, Clone, Copy)]
pub enum Error {
EpochTooLow { base: Epoch, other: Epoch },
EpochTooHigh { base: Epoch, other: Epoch },
ArithError(ArithError),
}
impl From<ArithError> for Error {
fn from(e: ArithError) -> Self {
Self::ArithError(e)
}
}
#[cfg(feature = "arbitrary-fuzz")]
@@ -32,8 +40,8 @@ impl RelativeEpoch {
match self {
// Due to saturating nature of epoch, check for current first.
RelativeEpoch::Current => base,
RelativeEpoch::Previous => base - 1,
RelativeEpoch::Next => base + 1,
RelativeEpoch::Previous => base.saturating_sub(1u64),
RelativeEpoch::Next => base.saturating_add(1u64),
}
}
@@ -46,12 +54,11 @@ impl RelativeEpoch {
///
/// Spec v0.12.1
pub fn from_epoch(base: Epoch, other: Epoch) -> Result<Self, Error> {
// Due to saturating nature of epoch, check for current first.
if other == base {
Ok(RelativeEpoch::Current)
} else if other == base - 1 {
} else if other.safe_add(1)? == base {
Ok(RelativeEpoch::Previous)
} else if other == base + 1 {
} else if other == base.safe_add(1)? {
Ok(RelativeEpoch::Next)
} else if other < base {
Err(Error::EpochTooLow { base, other })

19
consensus/types/src/slot_epoch.rs
View File

@@ -14,21 +14,23 @@ use crate::test_utils::TestRandom;
use crate::SignedRoot;
use rand::RngCore;
use safe_arith::SafeArith;
use serde_derive::{Deserialize, Serialize};
use ssz::{ssz_encode, Decode, DecodeError, Encode};
use std::cmp::{Ord, Ordering};
use std::fmt;
use std::hash::{Hash, Hasher};
use std::hash::Hash;
use std::iter::Iterator;
#[cfg(feature = "legacy-arith")]
use std::ops::{Add, AddAssign, Div, DivAssign, Mul, MulAssign, Rem, Sub, SubAssign};
#[cfg_attr(feature = "arbitrary-fuzz", derive(arbitrary::Arbitrary))]
#[derive(Eq, Clone, Copy, Default, Serialize, Deserialize)]
#[derive(Clone, Copy, Default, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize, Deserialize)]
#[serde(transparent)]
pub struct Slot(u64);
#[cfg_attr(feature = "arbitrary-fuzz", derive(arbitrary::Arbitrary))]
#[derive(Eq, Clone, Copy, Default, Serialize, Deserialize)]
#[derive(Clone, Copy, Default, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize, Deserialize)]
#[serde(transparent)]
pub struct Epoch(u64);
@@ -41,7 +43,9 @@ impl Slot {
}
pub fn epoch(self, slots_per_epoch: u64) -> Epoch {
Epoch::from(self.0) / Epoch::from(slots_per_epoch)
Epoch::new(self.0)
.safe_div(slots_per_epoch)
.expect("slots_per_epoch is not 0")
}
pub fn max_value() -> Slot {
@@ -96,9 +100,6 @@ impl Epoch {
}
}
impl SignedRoot for Epoch {}
impl SignedRoot for Slot {}
pub struct SlotIter<'a> {
current_iteration: u64,
epoch: &'a Epoch,
@@ -115,7 +116,7 @@ impl<'a> Iterator for SlotIter<'a> {
let start_slot = self.epoch.start_slot(self.slots_per_epoch);
let previous = self.current_iteration;
self.current_iteration = self.current_iteration.checked_add(1)?;
Some(start_slot + previous)
start_slot.safe_add(previous).ok()
}
}
}

134
consensus/types/src/slot_epoch_macros.rs
View File

@@ -42,22 +42,84 @@ macro_rules! impl_from_into_usize {
};
}
macro_rules! impl_u64_eq_ord {
($type: ident) => {
impl PartialEq<u64> for $type {
fn eq(&self, other: &u64) -> bool {
self.as_u64() == *other
}
}
impl PartialOrd<u64> for $type {
fn partial_cmp(&self, other: &u64) -> Option<std::cmp::Ordering> {
self.as_u64().partial_cmp(other)
}
}
};
}
macro_rules! impl_safe_arith {
($type: ident, $rhs_ty: ident) => {
impl safe_arith::SafeArith<$rhs_ty> for $type {
const ZERO: Self = $type::new(0);
const ONE: Self = $type::new(1);
fn safe_add(&self, other: $rhs_ty) -> safe_arith::Result<Self> {
self.0
.checked_add(other.into())
.map(Self::new)
.ok_or(safe_arith::ArithError::Overflow)
}
fn safe_sub(&self, other: $rhs_ty) -> safe_arith::Result<Self> {
self.0
.checked_sub(other.into())
.map(Self::new)
.ok_or(safe_arith::ArithError::Overflow)
}
fn safe_mul(&self, other: $rhs_ty) -> safe_arith::Result<Self> {
self.0
.checked_mul(other.into())
.map(Self::new)
.ok_or(safe_arith::ArithError::Overflow)
}
fn safe_div(&self, other: $rhs_ty) -> safe_arith::Result<Self> {
self.0
.checked_div(other.into())
.map(Self::new)
.ok_or(safe_arith::ArithError::DivisionByZero)
}
fn safe_rem(&self, other: $rhs_ty) -> safe_arith::Result<Self> {
self.0
.checked_rem(other.into())
.map(Self::new)
.ok_or(safe_arith::ArithError::DivisionByZero)
}
fn safe_shl(&self, other: u32) -> safe_arith::Result<Self> {
self.0
.checked_shl(other)
.map(Self::new)
.ok_or(safe_arith::ArithError::Overflow)
}
fn safe_shr(&self, other: u32) -> safe_arith::Result<Self> {
self.0
.checked_shr(other)
.map(Self::new)
.ok_or(safe_arith::ArithError::Overflow)
}
}
};
}
// Deprecated: prefer `SafeArith` methods for new code.
#[cfg(feature = "legacy-arith")]
macro_rules! impl_math_between {
($main: ident, $other: ident) => {
impl PartialOrd<$other> for $main {
/// Utilizes `partial_cmp` on the underlying `u64`.
fn partial_cmp(&self, other: &$other) -> Option<Ordering> {
Some(self.0.cmp(&(*other).into()))
}
}
impl PartialEq<$other> for $main {
fn eq(&self, other: &$other) -> bool {
let other: u64 = (*other).into();
self.0 == other
}
}
impl Add<$other> for $main {
type Output = $main;
@@ -144,33 +206,17 @@ macro_rules! impl_math {
($type: ident) => {
impl $type {
pub fn saturating_sub<T: Into<$type>>(&self, other: T) -> $type {
*self - other.into()
$type::new(self.as_u64().saturating_sub(other.into().as_u64()))
}
pub fn saturating_add<T: Into<$type>>(&self, other: T) -> $type {
*self + other.into()
}
pub fn checked_div<T: Into<$type>>(&self, rhs: T) -> Option<$type> {
let rhs: $type = rhs.into();
if rhs == 0 {
None
} else {
Some(*self / rhs)
}
$type::new(self.as_u64().saturating_add(other.into().as_u64()))
}
pub fn is_power_of_two(&self) -> bool {
self.0.is_power_of_two()
}
}
impl Ord for $type {
fn cmp(&self, other: &$type) -> Ordering {
let other: u64 = (*other).into();
self.0.cmp(&other)
}
}
};
}
@@ -257,6 +303,8 @@ macro_rules! impl_ssz {
}
}
impl SignedRoot for $type {}
impl TestRandom for $type {
fn random_for_test(rng: &mut impl RngCore) -> Self {
$type::from(u64::random_for_test(rng))
@@ -265,29 +313,21 @@ macro_rules! impl_ssz {
};
}
macro_rules! impl_hash {
($type: ident) => {
// Implemented to stop clippy lint:
// https://rust-lang.github.io/rust-clippy/master/index.html#derive_hash_xor_eq
impl Hash for $type {
fn hash<H: Hasher>(&self, state: &mut H) {
ssz_encode(self).hash(state)
}
}
};
}
macro_rules! impl_common {
($type: ident) => {
impl_from_into_u64!($type);
impl_from_into_usize!($type);
impl_u64_eq_ord!($type);
impl_safe_arith!($type, $type);
impl_safe_arith!($type, u64);
#[cfg(feature = "legacy-arith")]
impl_math_between!($type, $type);
#[cfg(feature = "legacy-arith")]
impl_math_between!($type, u64);
impl_math!($type);
impl_display!($type);
impl_debug!($type);
impl_ssz!($type);
impl_hash!($type);
};
}
@@ -335,6 +375,7 @@ macro_rules! math_between_tests {
($type: ident, $other: ident) => {
#[test]
fn partial_ord() {
use std::cmp::Ordering;
let assert_partial_ord = |a: u64, partial_ord: Ordering, b: u64| {
let other: $other = $type(b).into();
assert_eq!($type(a).partial_cmp(&other), Some(partial_ord));
@@ -518,7 +559,7 @@ macro_rules! math_tests {
#[test]
fn checked_div() {
let assert_checked_div = |a: u64, b: u64, result: Option<u64>| {
let division_result_as_u64 = match $type(a).checked_div($type(b)) {
let division_result_as_u64 = match $type(a).safe_div($type(b)).ok() {
None => None,
Some(val) => Some(val.as_u64()),
};
@@ -560,6 +601,7 @@ macro_rules! math_tests {
#[test]
fn ord() {
use std::cmp::Ordering;
let assert_ord = |a: u64, ord: Ordering, b: u64| {
assert_eq!($type(a).cmp(&$type(b)), ord);
};

14
consensus/types/src/test_utils/builders/testing_attestation_data_builder.rs
View File

@@ -1,5 +1,6 @@
use crate::test_utils::AttestationTestTask;
use crate::*;
use safe_arith::SafeArith;
/// Builds an `AttestationData` to be used for testing purposes.
///
@@ -49,12 +50,19 @@ impl TestingAttestationDataBuilder {
match test_task {
AttestationTestTask::IncludedTooEarly => {
slot = state.slot - spec.min_attestation_inclusion_delay + 1
slot = state
.slot
.safe_sub(spec.min_attestation_inclusion_delay)
.unwrap()
.safe_add(1u64)
.unwrap();
}
AttestationTestTask::IncludedTooLate => slot -= T::SlotsPerEpoch::to_u64(),
AttestationTestTask::IncludedTooLate => slot
.safe_sub_assign(Slot::new(T::SlotsPerEpoch::to_u64()))
.unwrap(),
AttestationTestTask::TargetEpochSlotMismatch => {
target = Checkpoint {
epoch: current_epoch + 1,
epoch: current_epoch.safe_add(1u64).unwrap(),
root: Hash256::zero(),
};
assert_ne!(target.epoch, slot.epoch(T::slots_per_epoch()));

10
consensus/types/src/test_utils/builders/testing_beacon_block_builder.rs
View File

@@ -9,6 +9,7 @@ use crate::{
use int_to_bytes::int_to_bytes32;
use merkle_proof::MerkleTree;
use rayon::prelude::*;
use safe_arith::SafeArith;
use tree_hash::TreeHash;
/// Builds a beacon block to be used for testing purposes.
@@ -172,7 +173,10 @@ impl<T: EthSpec> TestingBeaconBlockBuilder<T> {
num_attestations: usize,
spec: &ChainSpec,
) -> Result<(), BeaconStateError> {
let mut slot = self.block.slot - spec.min_attestation_inclusion_delay;
let mut slot = self
.block
.slot
.safe_sub(spec.min_attestation_inclusion_delay)?;
let mut attestations_added = 0;
// Stores the following (in order):
@@ -192,7 +196,7 @@ impl<T: EthSpec> TestingBeaconBlockBuilder<T> {
// - The slot is too old to be included in a block at this slot.
// - The `MAX_ATTESTATIONS`.
loop {
if state.slot >= slot + T::slots_per_epoch() {
if state.slot >= slot.safe_add(T::slots_per_epoch())? {
break;
}
@@ -211,7 +215,7 @@ impl<T: EthSpec> TestingBeaconBlockBuilder<T> {
attestations_added += 1;
}
slot -= 1;
slot.safe_sub_assign(1u64)?;
}
// Loop through all the committees, splitting each one in half until we have

6
consensus/types/src/test_utils/builders/testing_beacon_state_builder.rs
View File

@@ -143,11 +143,11 @@ impl<T: EthSpec> TestingBeaconStateBuilder<T> {
state.slot = slot;
state.previous_justified_checkpoint.epoch = epoch - 3;
state.current_justified_checkpoint.epoch = epoch - 2;
state.previous_justified_checkpoint.epoch = epoch.saturating_sub(3u64);
state.current_justified_checkpoint.epoch = epoch.saturating_sub(2u64);
state.justification_bits = BitVector::from_bytes(vec![0b0000_1111]).unwrap();
state.finalized_checkpoint.epoch = epoch - 3;
state.finalized_checkpoint.epoch = state.previous_justified_checkpoint.epoch;
}
/// Creates a full set of attestations for the `BeaconState`. Each attestation has full