Split ArithmeticError into more granular errors

CHANGELOG.md

@@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
   Created a new `checked_transaction::CheckError` that combines `ValidityError`
   and `PredicateVerificationFailed` errors into one. It allows the return of the
   `PredicateVerificationFailed` to the end user instead of losing the reason why predicate verification failed.
+- [#625](https://github.com/FuelLabs/fuel-vm/pull/625): Use `ArithmeticError` only for arithmetic operations, and introduce new errors like `BalanceOverflow` for others. Whenever an error is internally caused by a type conversion to `usize`, so that an overflowing value wouldn't map to a valid index anyway, return the missing item error instead.
 - [#623](https://github.com/FuelLabs/fuel-vm/pull/623):
   Added support for transaction policies. The `Script` and `Create`
   transactions received a new field, `policies`. Policies allow the addition

fuel-asm/src/panic_reason.rs

@@ -104,7 +104,7 @@ enum_from! {
         ContractMismatch = 0x21,
         /// Attempting to send message data longer than `MAX_MESSAGE_DATA_LENGTH`
         MessageDataTooLong = 0x22,
-        /// Mathimatically invalid arguments where given to an arithmetic instruction.
+        /// Mathematically invalid arguments where given to an arithmetic instruction.
         /// For instance, division by zero produces this.
         /// These errors are ignored using the UNSAFEMATH flag.
         ArithmeticError = 0x23,
@@ -122,6 +122,13 @@ enum_from! {
         PolicyNotFound = 0x29,
         /// Receipt context is full
         TooManyReceipts = 0x2a,
+        /// Balance of a contract overflowed
+        BalanceOverflow = 0x2b,
+        /// Block height value is invalid, typically because it is too large
+        InvalidBlockHeight = 0x2c,
+        /// Attempt to use sequential memory instructions with too large slot count,
+        /// typically because it cannot fit into usize
+        TooManySlots = 0x2d,
     }
 }
 

fuel-tx/src/transaction.rs

@@ -618,7 +618,7 @@ pub mod field {
         #[inline(always)]
         fn set_maturity(&mut self, block_height: BlockHeight) {
             self.policies_mut()
-                .set(PolicyType::Maturity, Some(block_height.as_usize() as u64))
+                .set(PolicyType::Maturity, Some(*block_height.deref() as u64))
         }
     }
 

fuel-tx/src/transaction/policies.rs

@@ -1,3 +1,4 @@
+use core::ops::Deref;
 use fuel_types::{
     canonical::{
         Deserialize,
@@ -126,7 +127,7 @@ impl Policies {
 
     /// Sets the `maturity` policy.
     pub fn with_maturity(mut self, maturity: BlockHeight) -> Self {
-        self.set(PolicyType::Maturity, Some(maturity.as_usize() as u64));
+        self.set(PolicyType::Maturity, Some(*maturity.deref() as u64));
         self
     }
 

fuel-tx/src/transaction/validity/error.rs

@@ -132,7 +132,8 @@ pub enum ValidityError {
         /// The total amount provided by coin inputs
         provided: u64,
     },
-    /// The user provided amounts for coins or gas prices that caused an arithmetic
-    /// overflow.
-    ArithmeticOverflow,
+    /// The given coins is too large
+    BalanceOverflow,
+    /// The given gas costs is are too large
+    GasCostsCoinsOverflow,
 }

fuel-types/src/numeric_types.rs

@@ -63,16 +63,6 @@ macro_rules! key_methods {
                 pub fn to_bytes(self) -> [u8; SIZE] {
                     self.0.to_be_bytes()
                 }
-
-                /// Convert to usize.
-                pub const fn to_usize(self) -> usize {
-                    self.0 as usize
-                }
-
-                /// Convert to usize.
-                pub const fn as_usize(&self) -> usize {
-                    self.0 as usize
-                }
             }
 
             #[cfg(feature = "typescript")]

fuel-vm/src/checked_transaction.rs

@@ -1419,7 +1419,7 @@ mod tests {
             .into_checked(Default::default(), &consensus_params)
             .expect_err("overflow expected");
 
-        assert_eq!(err, CheckError::Validity(ValidityError::ArithmeticOverflow));
+        assert_eq!(err, CheckError::Validity(ValidityError::BalanceOverflow));
     }
 
     #[test]
@@ -1437,7 +1437,7 @@ mod tests {
             .into_checked(Default::default(), &consensus_params)
             .expect_err("overflow expected");
 
-        assert_eq!(err, CheckError::Validity(ValidityError::ArithmeticOverflow));
+        assert_eq!(err, CheckError::Validity(ValidityError::BalanceOverflow));
     }
 
     #[test]
@@ -1588,7 +1588,7 @@ mod tests {
             .saturating_add(witness_limit_allowance);
         let max_fee: u64 = gas_to_fee(max_gas, tx.price(), fee_params.gas_price_factor)
             .try_into()
-            .map_err(|_| ValidityError::ArithmeticOverflow)?;
+            .map_err(|_| ValidityError::BalanceOverflow)?;
 
         let result = max_fee == available_balances.fee.max_fee();
         Ok(result)
@@ -1624,7 +1624,7 @@ mod tests {
         let rounded_fee = fee.saturating_add(fee_remainder);
         let min_fee: u64 = rounded_fee
             .try_into()
-            .map_err(|_| ValidityError::ArithmeticOverflow)?;
+            .map_err(|_| ValidityError::BalanceOverflow)?;
 
         Ok(min_fee == available_balances.fee.min_fee())
     }

fuel-vm/src/checked_transaction/balances.rs

@@ -68,7 +68,7 @@ where
 
     // Deduct fee from base asset
     let fee = TransactionFee::checked_from_tx(gas_costs, params, transaction)
-        .ok_or(ValidityError::ArithmeticOverflow)?;
+        .ok_or(ValidityError::BalanceOverflow)?;
 
     let base_asset_balance = non_retryable_balances.entry(*base_asset_id).or_default();
 

fuel-vm/src/convert.rs

@@ -0,0 +1,4 @@
+/// Converts value to usize is a way that's consistet on 32-bit and 64-bit platforms.
+pub(crate) fn to_usize(value: u64) -> Option<usize> {
+    usize::try_from(u32::try_from(value).ok()?).ok()
+}

fuel-vm/src/interpreter.rs

@@ -430,7 +430,7 @@ pub trait ExecutableTransaction:
     {
         let gas_refund = self
             .refund_fee(gas_costs, fee_params, used_gas)
-            .ok_or(ValidityError::ArithmeticOverflow)?;
+            .ok_or(ValidityError::GasCostsCoinsOverflow)?;
 
         self.outputs_mut().iter_mut().try_for_each(|o| match o {
             // If revert, set base asset to initial balance and refund unused gas
@@ -442,7 +442,7 @@ pub trait ExecutableTransaction:
                 [base_asset_id]
                 .checked_add(gas_refund)
                 .map(|v| *amount = v)
-                .ok_or(ValidityError::ArithmeticOverflow),
+                .ok_or(ValidityError::BalanceOverflow),
 
             // If revert, reset any non-base asset to its initial balance
             Output::Change {
@@ -458,7 +458,7 @@ pub trait ExecutableTransaction:
             } if asset_id == base_asset_id => balances[asset_id]
                 .checked_add(gas_refund)
                 .map(|v| *amount = v)
-                .ok_or(ValidityError::ArithmeticOverflow),
+                .ok_or(ValidityError::BalanceOverflow),
 
             // Set changes to the remainder provided balances
             Output::Change {

fuel-vm/src/interpreter/balances.rs

@@ -71,7 +71,7 @@ impl TryFrom<InitialBalances> for RuntimeBalances {
             let entry = balances.entry(retryable_amount.base_asset_id).or_default();
             *entry = entry
                 .checked_add(retryable_amount.amount)
-                .ok_or(ValidityError::ArithmeticOverflow)?;
+                .ok_or(ValidityError::BalanceOverflow)?;
         }
         Self::try_from_iter(balances)
     }
@@ -96,7 +96,7 @@ impl RuntimeBalances {
                     .entry(asset)
                     .or_insert_with(|| Balance::new(0, offset))
                     .checked_add(balance)
-                    .ok_or(ValidityError::ArithmeticOverflow)?;
+                    .ok_or(ValidityError::BalanceOverflow)?;
 
                 Ok(state)
             })
@@ -306,7 +306,7 @@ fn try_from_iter_wont_overflow() {
     let err =
         RuntimeBalances::try_from_iter(balances).expect_err("overflow set should fail");
 
-    assert_eq!(ValidityError::ArithmeticOverflow, err);
+    assert_eq!(ValidityError::BalanceOverflow, err);
 }
 
 #[test]

fuel-vm/src/interpreter/blockchain.rs

@@ -30,7 +30,6 @@ use super::{
     RuntimeBalances,
 };
 use crate::{
-    arith::checked_add_word,
     call::CallFrame,
     constraints::{
         reg_key::*,
@@ -39,6 +38,7 @@ use crate::{
     },
     consts::*,
     context::Context,
+    convert,
     error::{
         IoResult,
         RuntimeError,
@@ -652,7 +652,7 @@ where
         let asset_id = contract_id.asset_id(sub_id);
 
         let balance = balance(self.storage, contract_id, &asset_id)?;
-        let balance = checked_add_word(balance, a)?;
+        let balance = balance.checked_add(a).ok_or(PanicReason::BalanceOverflow)?;
 
         self.storage
             .merkle_contract_asset_id_balance_insert(contract_id, &asset_id, balance)
@@ -728,7 +728,7 @@ pub(crate) fn block_hash<S: InterpreterStorage>(
     b: Word,
 ) -> IoResult<(), S::DataError> {
     let height = u32::try_from(b)
-        .map_err(|_| PanicReason::ArithmeticOverflow)?
+        .map_err(|_| PanicReason::InvalidBlockHeight)?
         .into();
     let hash = storage.block_hash(height).map_err(RuntimeError::Storage)?;
 
@@ -926,7 +926,7 @@ pub(crate) fn timestamp<S: InterpreterStorage>(
     b: Word,
 ) -> IoResult<(), S::DataError> {
     let b = u32::try_from(b)
-        .map_err(|_| PanicReason::ArithmeticOverflow)?
+        .map_err(|_| PanicReason::InvalidBlockHeight)?
         .into();
     (b <= block_height)
         .then_some(())
@@ -1043,8 +1043,7 @@ impl StateReadQWord {
         num_slots: Word,
         ownership_registers: OwnershipRegisters,
     ) -> SimpleResult<Self> {
-        let num_slots =
-            usize::try_from(num_slots).map_err(|_| PanicReason::ArithmeticOverflow)?;
+        let num_slots = convert::to_usize(num_slots).ok_or(PanicReason::TooManySlots)?;
         let destination_address_memory_range = MemoryRange::new(
             destination_memory_address,
             Bytes32::LEN.saturating_mul(num_slots),
@@ -1167,8 +1166,9 @@ impl StateClearQWord {
         let start_storage_key_memory_range = CheckedMemConstLen::<{ Bytes32::LEN }>::new(
             start_storage_key_memory_address,
         )?;
-        let num_slots =
-            usize::try_from(num_slots).map_err(|_| PanicReason::ArithmeticOverflow)?;
+
+        let num_slots = convert::to_usize(num_slots).ok_or(PanicReason::TooManySlots)?;
+
         Ok(Self {
             start_storage_key_memory_range,
             num_slots,

fuel-vm/src/interpreter/blockchain/other_tests.rs

@@ -105,7 +105,7 @@ fn test_burn(
 #[test_case(false, 0, None, Word::MAX, [0; 32] => Ok(()); "Mint max from nothing")]
 #[test_case(false, 0, 0, Word::MAX, [0; 32] => Ok(()); "Mint max from zero")]
 #[test_case(true, 0, 100, 10, [0; 32] => Err(RuntimeError::Recoverable(PanicReason::ExpectedInternalContext)); "Can't mint from external context")]
-#[test_case(false, 0, 1, Word::MAX, [0; 32] => Err(RuntimeError::Recoverable(PanicReason::ArithmeticOverflow)); "Can't mint too much")]
+#[test_case(false, 0, 1, Word::MAX, [0; 32] => Err(RuntimeError::Recoverable(PanicReason::BalanceOverflow)); "Can't mint too much")]
 fn test_mint(
     external: bool,
     fp: Word,

fuel-vm/src/interpreter/contract.rs

@@ -21,6 +21,7 @@ use crate::{
     },
     consts::*,
     context::Context,
+    convert,
     error::{
         IoResult,
         RuntimeError,
@@ -291,7 +292,7 @@ impl<'vm, S, Tx> TransferCtx<'vm, S, Tx> {
         S: ContractsAssetsStorage,
     {
         let out_idx =
-            usize::try_from(output_index).map_err(|_| PanicReason::ArithmeticOverflow)?;
+            convert::to_usize(output_index).ok_or(PanicReason::OutputNotFound)?;
         let to = Address::from(read_bytes(self.memory, recipient_offset)?);
         let asset_id = AssetId::from(read_bytes(self.memory, asset_id_offset)?);
         let amount = transfer_amount;
@@ -386,7 +387,7 @@ where
     let balance = balance(storage, contract, asset_id)?;
     let balance = balance
         .checked_add(amount)
-        .ok_or(PanicReason::ArithmeticOverflow)?;
+        .ok_or(PanicReason::BalanceOverflow)?;
     storage
         .merkle_contract_asset_id_balance_insert(contract, asset_id, balance)
         .map_err(RuntimeError::Storage)?;

fuel-vm/src/interpreter/contract/tests.rs

@@ -340,8 +340,8 @@ fn test_transfer_output(
 #[test_case(None, Word::MAX => Ok(()); "Can initialize balance to max")]
 #[test_case(0, Word::MAX => Ok(()); "Can add max to zero")]
 #[test_case(Word::MAX, 0 => Ok(()); "Can add zero to max")]
-#[test_case(1, Word::MAX => Err(RuntimeError::Recoverable(PanicReason::ArithmeticOverflow)); "Overflowing add")]
-#[test_case(Word::MAX, 1 => Err(RuntimeError::Recoverable(PanicReason::ArithmeticOverflow)); "Overflowing 1 add")]
+#[test_case(1, Word::MAX => Err(RuntimeError::Recoverable(PanicReason::BalanceOverflow)); "Overflowing add")]
+#[test_case(Word::MAX, 1 => Err(RuntimeError::Recoverable(PanicReason::BalanceOverflow)); "Overflowing 1 add")]
 fn test_balance_increase(
     initial: impl Into<Option<Word>>,
     amount: Word,

fuel-vm/src/interpreter/flow.rs

@@ -23,7 +23,6 @@ use super::{
     RuntimeBalances,
 };
 use crate::{
-    arith,
     call::{
         Call,
         CallFrame,
@@ -585,7 +584,7 @@ where
         );
 
         let old_sp = *self.registers.system_registers.sp;
-        let new_sp = arith::checked_add_word(old_sp, len)?;
+        let new_sp = old_sp.checked_add(len).ok_or(PanicReason::MemoryOverflow)?;
 
         set_frame_pointer(
             self.context,

fuel-vm/src/interpreter/metadata.rs

@@ -8,6 +8,7 @@ use crate::{
     constraints::reg_key::*,
     consts::*,
     context::Context,
+    convert,
     error::SimpleResult,
 };
 
@@ -147,7 +148,7 @@ impl<Tx> GTFInput<'_, Tx> {
     where
         Tx: ExecutableTransaction,
     {
-        let b = usize::try_from(b).map_err(|_| PanicReason::ArithmeticOverflow)?;
+        let b = convert::to_usize(b).ok_or(PanicReason::InvalidMetadataIdentifier)?;
         let args = GTFArgs::try_from(imm)?;
         let tx = self.tx;
         let ofs = self.tx_offset;

fuel-vm/src/lib.rs

@@ -14,13 +14,13 @@ pub extern crate alloc;
 #[cfg(feature = "std")]
 extern crate libm as _; // Not needed with stdlib
 
-pub mod arith;
 pub mod backtrace;
 pub mod call;
 pub mod checked_transaction;
 pub mod constraints;
 pub mod consts;
 pub mod context;
+mod convert;
 pub mod crypto;
 pub mod error;
 pub mod interpreter;