Merge #4907 #4911

4907: Remove unchecked arithmetic r=mpapierski a=mpapierski

Closes #2103 

This PR removes unchecked arithmetic operations. Although nearly impossible this is for correctness reason to avoid using unchecked math even on large numbers.

4911: GH-4164: Update comment r=mpapierski a=mpapierski

Closes #4164 

Make the chainspec comment clearer 

Co-authored-by: Michał Papierski <[email protected]>

execution_engine/src/runtime/mod.rs

@@ -3321,7 +3321,9 @@ where
     where
         T: AsRef<[HostFunctionCost]> + Copy,
     {
-        let cost = host_function.calculate_gas_cost(weights);
+        let cost = host_function
+            .calculate_gas_cost(weights)
+            .ok_or(ExecError::GasLimit)?; // Overflowing gas calculation means gas limit was exceeded
         self.gas(cost)?;
         Ok(())
     }

execution_engine/src/runtime_context/tests.rs

@@ -1008,7 +1008,10 @@ fn should_meter_for_gas_storage_write() {
         gas_usage_before
     );
 
-    assert_eq!(gas_usage_after, gas_usage_before + expected_write_cost);
+    assert_eq!(
+        Some(gas_usage_after),
+        gas_usage_before.checked_add(expected_write_cost)
+    );
 }
 
 #[test]
@@ -1044,7 +1047,10 @@ fn should_meter_for_gas_storage_add() {
         gas_usage_before
     );
 
-    assert_eq!(gas_usage_after, gas_usage_before + expected_add_cost);
+    assert_eq!(
+        Some(gas_usage_after),
+        gas_usage_before.checked_add(expected_add_cost)
+    );
 }
 
 #[test]

execution_engine_testing/tests/src/test/check_transfer_success.rs

@@ -72,8 +72,10 @@ fn test_check_transfer_success_with_source_only() {
 
     let transaction_fee = builder.get_proposer_purse_balance() - proposer_starting_balance;
     let expected_source_ending_balance = Motes::new(DEFAULT_ACCOUNT_INITIAL_BALANCE)
-        - Motes::new(transfer_amount)
-        - Motes::new(transaction_fee);
+        .checked_sub(Motes::new(transfer_amount))
+        .unwrap()
+        .checked_sub(Motes::new(transaction_fee))
+        .unwrap();
     let actual_source_ending_balance = Motes::new(builder.get_purse_balance(source_purse));
 
     assert_eq!(expected_source_ending_balance, actual_source_ending_balance);
@@ -133,8 +135,10 @@ fn test_check_transfer_success_with_source_only_errors() {
 
     let transaction_fee = builder.get_proposer_purse_balance() - proposer_starting_balance;
     let expected_source_ending_balance = Motes::new(DEFAULT_ACCOUNT_INITIAL_BALANCE)
-        - Motes::new(transfer_amount)
-        - Motes::new(transaction_fee);
+        .checked_sub(Motes::new(transfer_amount))
+        .unwrap()
+        .checked_sub(Motes::new(transaction_fee))
+        .unwrap();
     let actual_source_ending_balance = Motes::new(builder.get_purse_balance(source_purse));
 
     assert!(expected_source_ending_balance != actual_source_ending_balance);
@@ -191,8 +195,10 @@ fn test_check_transfer_success_with_source_and_target() {
 
     let transaction_fee = builder.get_proposer_purse_balance() - proposer_starting_balance;
     let expected_source_ending_balance = Motes::new(DEFAULT_ACCOUNT_INITIAL_BALANCE)
-        - Motes::new(transfer_amount)
-        - Motes::new(transaction_fee);
+        .checked_sub(Motes::new(transfer_amount))
+        .unwrap()
+        .checked_sub(Motes::new(transaction_fee))
+        .unwrap();
     let actual_source_ending_balance = Motes::new(builder.get_purse_balance(source_purse));
 
     assert_eq!(expected_source_ending_balance, actual_source_ending_balance);

execution_engine_testing/tests/src/test/gas_counter.rs

@@ -163,7 +163,12 @@ fn should_correctly_measure_gas_for_opcodes() {
     builder.exec(exec_request).commit().expect_success();
 
     let gas_cost = builder.last_exec_gas_cost();
-    let expected_cost = accounted_opcodes.clone().into_iter().map(Gas::from).sum();
+    let expected_cost = accounted_opcodes
+        .clone()
+        .into_iter()
+        .map(Gas::from)
+        .try_fold(Gas::default(), |acc, cost| acc.checked_add(cost))
+        .expect("should add gas costs");
     assert_eq!(
         gas_cost, expected_cost,
         "accounted costs {:?}",

execution_engine_testing/tests/src/test/regression/ee_598.rs

@@ -38,7 +38,9 @@ fn should_handle_unbond_for_more_than_stake_as_full_unbond_of_stake_ee_598_regre
         let mut tmp: Vec<GenesisAccount> = DEFAULT_ACCOUNTS.clone();
         let account = GenesisAccount::account(
             public_key,
-            Motes::new(GENESIS_VALIDATOR_STAKE) * Motes::new(2),
+            Motes::new(GENESIS_VALIDATOR_STAKE)
+                .checked_mul(Motes::new(2))
+                .unwrap(),
             Some(GenesisValidator::new(
                 Motes::new(GENESIS_VALIDATOR_STAKE),
                 DelegationRate::zero(),

execution_engine_testing/tests/src/test/regression/slow_input.rs

@@ -172,8 +172,10 @@ fn should_charge_extra_per_amount_of_br_table_elements() {
     );
 
     assert_eq!(
-        gas_cost_2 - gas_cost_1,
-        Gas::from((M_ELEMENTS - N_ELEMENTS) * DEFAULT_CONTROL_FLOW_BR_TABLE_MULTIPLIER),
+        gas_cost_2.checked_sub(gas_cost_1),
+        Some(Gas::from(
+            (M_ELEMENTS - N_ELEMENTS) * DEFAULT_CONTROL_FLOW_BR_TABLE_MULTIPLIER
+        )),
         "the cost difference should equal to exactly the size of br_table difference "
     );
 }

node/src/types/appendable_block.rs

@@ -215,7 +215,12 @@ impl Display for AppendableBlock {
         let auction_count = self.category_count(AUCTION_LANE_ID);
         let install_upgrade_count = self.category_count(INSTALL_UPGRADE_LANE_ID);
         let wasm_count = total_count - mint_count - auction_count - install_upgrade_count;
-        let total_gas_limit: Gas = self.transactions.values().map(|f| f.gas_limit).sum();
+        let total_gas_limit: Gas = self
+            .transactions
+            .values()
+            .map(|f| f.gas_limit)
+            .try_fold(Gas::new(0), |acc, gas| acc.checked_add(gas))
+            .unwrap_or(Gas::MAX);
         let total_approvals_count: usize = self
             .transactions
             .values()

resources/local/chainspec.toml.in

@@ -96,7 +96,10 @@ signature_rewards_max_delay = 3
 #
 # Changing this option makes sense only for private chains which dont need auctioning new validator slots.
 allow_auction_bids = true
-# Allow peer to peer transfers between users. Setting this to false makes sense only on private chains.
+# Allows transfers between accounts in the blockchain network.
+#
+# Setting this to false restricts normal accounts from sending tokens to other accounts, allowing transfers only to administrators.
+# Changing this option makes sense only on private chains.
 allow_unrestricted_transfers = true
 # If set to false, then consensus doesn't compute rewards and always uses 0.
 compute_rewards = true

resources/production/chainspec.toml

@@ -99,9 +99,10 @@ finders_fee = [1, 5]
 finality_signature_proportion = [1, 2]
 # Lookback interval indicating which past block we are looking at to reward.
 signature_rewards_max_delay = 3
-# Allows peer to peer transfers between users.
+# Allows transfers between accounts in the blockchain network.
 #
-# Setting this to false makes sense only for private chains.
+# Setting this to false restricts normal accounts from sending tokens to other accounts, allowing transfers only to administrators.
+# Changing this option makes sense only on private chains.
 allow_unrestricted_transfers = true
 # Enables the auction entry points 'delegate' and 'add_bid'.
 #

storage/src/system/genesis.rs

@@ -363,7 +363,7 @@ where
         for (validator_public_key, delegator_public_key, _, delegated_amount) in
             genesis_delegators.iter()
         {
-            if delegated_amount.is_zero() {
+            if *delegated_amount == &Motes::zero() {
                 return Err(GenesisError::InvalidDelegatedAmount {
                     public_key: (*delegator_public_key).clone(),
                 }

types/src/chainspec/vm_config/host_function_costs.rs

@@ -156,14 +156,15 @@ where
     }
 
     /// Calculate gas cost for a host function
-    pub fn calculate_gas_cost(&self, weights: T) -> Gas {
+    pub fn calculate_gas_cost(&self, weights: T) -> Option<Gas> {
         let mut gas = Gas::new(self.cost);
         for (argument, weight) in self.arguments.as_ref().iter().zip(weights.as_ref()) {
             let lhs = Gas::new(*argument);
             let rhs = Gas::new(*weight);
-            gas += lhs * rhs;
+            let product = lhs.checked_mul(rhs)?;
+            gas = gas.checked_add(product)?;
         }
-        gas
+        Some(gas)
     }
 }
 
@@ -1101,7 +1102,7 @@ mod tests {
             + (ARGUMENT_COSTS[2] * WEIGHTS[2]);
         assert_eq!(
             host_function.calculate_gas_cost(WEIGHTS),
-            Gas::new(expected_cost)
+            Some(Gas::new(expected_cost))
         );
     }
 
@@ -1120,7 +1121,7 @@ mod tests {
         let large_value = U512::from(large_value);
         let rhs = large_value + (U512::from(4) * large_value * large_value);
 
-        assert_eq!(lhs, Gas::new(rhs));
+        assert_eq!(lhs, Some(Gas::new(rhs)));
     }
 }
 

types/src/execution/execution_result_v2.rs

@@ -146,7 +146,9 @@ impl ExecutionResultV2 {
         let range = limit.value().as_u64();
 
         // can range from 0 to limit
-        let consumed = limit - Gas::new(rng.gen_range(0..=range));
+        let consumed = limit
+            .checked_sub(Gas::new(rng.gen_range(0..=range)))
+            .expect("consumed");
         let size_estimate = rng.gen();
 
         let payment = vec![PaymentInfo { source: rng.gen() }];

types/src/gas.rs

@@ -1,15 +1,10 @@
 //! The `gas` module is used for working with Gas including converting to and from Motes.
 
 use alloc::vec::Vec;
-use core::{
-    fmt,
-    iter::Sum,
-    ops::{Add, AddAssign, Div, Mul, Sub},
-};
+use core::fmt;
 
 #[cfg(feature = "datasize")]
 use datasize::DataSize;
-use num::Zero;
 #[cfg(any(feature = "testing", test))]
 use rand::Rng;
 #[cfg(feature = "json-schema")]
@@ -32,6 +27,9 @@ use crate::{
 pub struct Gas(U512);
 
 impl Gas {
+    /// The maximum value of `Gas`.
+    pub const MAX: Gas = Gas(U512::MAX);
+
     /// Constructs a new `Gas`.
     pub fn new<T: Into<U512>>(value: T) -> Self {
         Gas(value.into())
@@ -86,6 +84,16 @@ impl Gas {
         self.0.checked_sub(rhs.value()).map(Self::new)
     }
 
+    /// Checked integer subtraction. Computes `self * rhs`, returning `None` if overflow occurred.
+    pub fn checked_mul(&self, rhs: Self) -> Option<Self> {
+        self.0.checked_mul(rhs.value()).map(Self::new)
+    }
+
+    /// Checked integer division. Computes `self / rhs`, returning `None` if overflow occurred.
+    pub fn checked_div(&self, rhs: Self) -> Option<Self> {
+        self.0.checked_div(rhs.value()).map(Self::new)
+    }
+
     /// Returns a random `Gas`.
     #[cfg(any(feature = "testing", test))]
     pub fn random(rng: &mut TestRng) -> Self {
@@ -120,64 +128,6 @@ impl fmt::Display for Gas {
     }
 }
 
-impl Add for Gas {
-    type Output = Gas;
-
-    fn add(self, rhs: Self) -> Self::Output {
-        let val = self.value() + rhs.value();
-        Gas::new(val)
-    }
-}
-
-impl Sub for Gas {
-    type Output = Gas;
-
-    fn sub(self, rhs: Self) -> Self::Output {
-        let val = self.value() - rhs.value();
-        Gas::new(val)
-    }
-}
-
-impl Div for Gas {
-    type Output = Gas;
-
-    fn div(self, rhs: Self) -> Self::Output {
-        let val = self.value() / rhs.value();
-        Gas::new(val)
-    }
-}
-
-impl Mul for Gas {
-    type Output = Gas;
-
-    fn mul(self, rhs: Self) -> Self::Output {
-        let val = self.value() * rhs.value();
-        Gas::new(val)
-    }
-}
-
-impl AddAssign for Gas {
-    fn add_assign(&mut self, rhs: Self) {
-        self.0 += rhs.0
-    }
-}
-
-impl Zero for Gas {
-    fn zero() -> Self {
-        Gas::new(U512::zero())
-    }
-
-    fn is_zero(&self) -> bool {
-        self.0.is_zero()
-    }
-}
-
-impl Sum for Gas {
-    fn sum<I: Iterator<Item = Self>>(iter: I) -> Self {
-        iter.fold(Gas::zero(), Add::add)
-    }
-}
-
 impl From<u32> for Gas {
     fn from(gas: u32) -> Self {
         let gas_u512: U512 = gas.into();
@@ -223,31 +173,47 @@ mod tests {
         let left_gas = Gas::new(U512::from(1));
         let right_gas = Gas::new(U512::from(1));
         let expected_gas = Gas::new(U512::from(2));
-        assert_eq!((left_gas + right_gas), expected_gas, "should be equal")
+        assert_eq!(
+            left_gas.checked_add(right_gas),
+            Some(expected_gas),
+            "should be equal"
+        )
     }
 
     #[test]
     fn should_be_able_to_subtract_two_instances_of_gas() {
         let left_gas = Gas::new(U512::from(1));
         let right_gas = Gas::new(U512::from(1));
         let expected_gas = Gas::new(U512::from(0));
-        assert_eq!((left_gas - right_gas), expected_gas, "should be equal")
+        assert_eq!(
+            left_gas.checked_sub(right_gas),
+            Some(expected_gas),
+            "should be equal"
+        )
     }
 
     #[test]
     fn should_be_able_to_multiply_two_instances_of_gas() {
         let left_gas = Gas::new(U512::from(100));
         let right_gas = Gas::new(U512::from(10));
         let expected_gas = Gas::new(U512::from(1000));
-        assert_eq!((left_gas * right_gas), expected_gas, "should be equal")
+        assert_eq!(
+            left_gas.checked_mul(right_gas),
+            Some(expected_gas),
+            "should be equal"
+        )
     }
 
     #[test]
     fn should_be_able_to_divide_two_instances_of_gas() {
         let left_gas = Gas::new(U512::from(1000));
         let right_gas = Gas::new(U512::from(100));
         let expected_gas = Gas::new(U512::from(10));
-        assert_eq!((left_gas / right_gas), expected_gas, "should be equal")
+        assert_eq!(
+            left_gas.checked_div(right_gas),
+            Some(expected_gas),
+            "should be equal"
+        )
     }
 
     #[test]