[Altair] Remove duplicate vars, panic on u64::pow overflow (#2350)

sigp · May 24, 2021 · 5cb0211 · 5cb0211
1 parent 76d31cd
commit 5cb0211
Show file tree

Hide file tree

Showing 4 changed files with 38 additions and 16 deletions.
diff --git a/consensus/state_processing/src/per_epoch_processing/altair/inactivity_updates.rs b/consensus/state_processing/src/per_epoch_processing/altair/inactivity_updates.rs
@@ -4,7 +4,7 @@ use core::result::Result::Ok;
 use safe_arith::SafeArith;
 use types::beacon_state::BeaconState;
 use types::chain_spec::ChainSpec;
-use types::consts::altair::{INACTIVITY_SCORE_BIAS, TIMELY_TARGET_FLAG_INDEX};
+use types::consts::altair::TIMELY_TARGET_FLAG_INDEX;
 use types::eth_spec::EthSpec;
 
 // FIXME(altair): there's no EF test for this one (yet)
@@ -26,7 +26,7 @@ pub fn process_inactivity_updates<T: EthSpec>(
         } else if state.is_in_inactivity_leak(spec) {
             state
                 .get_inactivity_score_mut(index)?
-                .safe_add_assign(INACTIVITY_SCORE_BIAS)?;
+                .safe_add_assign(spec.inactivity_score_bias)?;
         }
     }
     Ok(())

diff --git a/consensus/state_processing/src/per_epoch_processing/altair/rewards_and_penalties.rs b/consensus/state_processing/src/per_epoch_processing/altair/rewards_and_penalties.rs
@@ -1,7 +1,6 @@
 use safe_arith::SafeArith;
 use types::consts::altair::{
-    FLAG_INDICES_AND_WEIGHTS, INACTIVITY_PENALTY_QUOTIENT_ALTAIR, INACTIVITY_SCORE_BIAS,
-    TIMELY_TARGET_FLAG_INDEX, WEIGHT_DENOMINATOR,
+    FLAG_INDICES_AND_WEIGHTS, TIMELY_TARGET_FLAG_INDEX, WEIGHT_DENOMINATOR,
 };
 use types::{BeaconState, ChainSpec, EthSpec};
 
@@ -120,8 +119,9 @@ pub fn get_inactivity_penalty_deltas<T: EthSpec>(
                     .get_validator(index)?
                     .effective_balance
                     .safe_mul(state.get_inactivity_score(index)?)?;
-                let penalty_denominator =
-                    INACTIVITY_SCORE_BIAS.safe_mul(INACTIVITY_PENALTY_QUOTIENT_ALTAIR)?;
+                let penalty_denominator = spec
+                    .inactivity_score_bias
+                    .safe_mul(spec.inactivity_penalty_quotient_altair)?;
                 delta.penalize(penalty_numerator.safe_div(penalty_denominator)?)?;
             }
             deltas

diff --git a/consensus/types/src/chain_spec.rs b/consensus/types/src/chain_spec.rs
@@ -304,10 +304,22 @@ impl ChainSpec {
             /*
              *  Gwei values
              */
-            min_deposit_amount: u64::pow(2, 0).saturating_mul(u64::pow(10, 9)),
-            max_effective_balance: u64::pow(2, 5).saturating_mul(u64::pow(10, 9)),
-            ejection_balance: u64::pow(2, 4).saturating_mul(u64::pow(10, 9)),
-            effective_balance_increment: u64::pow(2, 0).saturating_mul(u64::pow(10, 9)),
+            min_deposit_amount: option_wrapper(|| {
+                u64::checked_pow(2, 0)?.checked_mul(u64::checked_pow(10, 9)?)
+            })
+            .expect("calculation does not overflow"),
+            max_effective_balance: option_wrapper(|| {
+                u64::checked_pow(2, 5)?.checked_mul(u64::checked_pow(10, 9)?)
+            })
+            .expect("calculation does not overflow"),
+            ejection_balance: option_wrapper(|| {
+                u64::checked_pow(2, 4)?.checked_mul(u64::checked_pow(10, 9)?)
+            })
+            .expect("calculation does not overflow"),
+            effective_balance_increment: option_wrapper(|| {
+                u64::checked_pow(2, 0)?.checked_mul(u64::checked_pow(10, 9)?)
+            })
+            .expect("calculation does not overflow"),
 
             /*
              * Initial Values
@@ -333,7 +345,7 @@ impl ChainSpec {
             base_reward_factor: 64,
             whistleblower_reward_quotient: 512,
             proposer_reward_quotient: 8,
-            inactivity_penalty_quotient: u64::pow(2, 26),
+            inactivity_penalty_quotient: u64::checked_pow(2, 26).expect("pow does not overflow"),
             min_slashing_penalty_quotient: 128,
             proportional_slashing_multiplier: 1,
 
@@ -367,8 +379,12 @@ impl ChainSpec {
             /*
              * Altair hard fork params
              */
-            inactivity_penalty_quotient_altair: u64::pow(2, 24).saturating_mul(3),
-            min_slashing_penalty_quotient_altair: u64::pow(2, 6),
+            inactivity_penalty_quotient_altair: option_wrapper(|| {
+                u64::checked_pow(2, 24)?.checked_mul(3)
+            })
+            .expect("calculation does not overflow"),
+            min_slashing_penalty_quotient_altair: u64::checked_pow(2, 6)
+                .expect("pow does not overflow"),
             proportional_slashing_multiplier_altair: 2,
             inactivity_score_bias: 4,
             epochs_per_sync_committee_period: Epoch::new(256),
@@ -408,7 +424,7 @@ impl ChainSpec {
             shard_committee_period: 64,
             genesis_delay: 300,
             seconds_per_slot: 6,
-            inactivity_penalty_quotient: u64::pow(2, 25),
+            inactivity_penalty_quotient: u64::checked_pow(2, 25).expect("pow does not overflow"),
             min_slashing_penalty_quotient: 64,
             proportional_slashing_multiplier: 2,
             safe_slots_to_update_justified: 2,
@@ -930,6 +946,14 @@ impl AltairConfig {
     }
 }
 
+/// A simple wrapper to permit the in-line use of `?`.
+fn option_wrapper<F, T>(f: F) -> Option<T>
+where
+    F: Fn() -> Option<T>,
+{
+    f()
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;

diff --git a/consensus/types/src/consts.rs b/consensus/types/src/consts.rs
@@ -8,8 +8,6 @@ pub mod altair {
     pub const SYNC_REWARD_WEIGHT: u64 = 8;
     pub const PROPOSER_WEIGHT: u64 = 8;
     pub const WEIGHT_DENOMINATOR: u64 = 64;
-    pub const INACTIVITY_SCORE_BIAS: u64 = 4;
-    pub const INACTIVITY_PENALTY_QUOTIENT_ALTAIR: u64 = u64::pow(2, 24).saturating_mul(3);
 
     pub const FLAG_INDICES_AND_WEIGHTS: [(u32, u64); NUM_FLAG_INDICES] = [
         (TIMELY_HEAD_FLAG_INDEX, TIMELY_HEAD_WEIGHT),