Merge pull request #562 from filecoin-project/fix/price-list-snafu

Strongly typed gas units, and fix two related bugs

Author: Stebalien

fvm/src/call_manager/default.rs

@@ -11,7 +11,7 @@ use num_traits::Zero;
 use super::{Backtrace, CallManager, InvocationResult, NO_DATA_BLOCK_ID};
 use crate::call_manager::backtrace::Frame;
 use crate::call_manager::FinishRet;
-use crate::gas::GasTracker;
+use crate::gas::{Gas, GasTracker};
 use crate::kernel::{ExecutionError, Kernel, Result, SyscallError};
 use crate::machine::Machine;
 use crate::syscalls::error::Abort;
@@ -71,7 +71,7 @@ where
     fn new(machine: M, gas_limit: i64, origin: Address, nonce: u64) -> Self {
         DefaultCallManager(Some(Box::new(InnerDefaultCallManager {
             machine,
-            gas_tracker: GasTracker::new(gas_limit, 0),
+            gas_tracker: GasTracker::new(Gas::new(gas_limit), Gas::zero()),
             origin,
             nonce,
             num_actors_created: 0,
@@ -154,7 +154,7 @@ where
     }
 
     fn finish(mut self) -> (FinishRet, Self::Machine) {
-        let gas_used = self.gas_tracker.gas_used().max(0);
+        let gas_used = self.gas_tracker.gas_used().max(Gas::zero()).round_up();
 
         let inner = self.0.take().expect("call manager is poisoned");
         // TODO: Having to check against zero here is fishy, but this is what lotus does.

fvm/src/executor/default.rs

@@ -15,7 +15,7 @@ use num_traits::Zero;
 
 use super::{ApplyFailure, ApplyKind, ApplyRet, Executor};
 use crate::call_manager::{backtrace, CallManager, InvocationResult};
-use crate::gas::{milligas_to_gas, GasCharge, GasOutputs};
+use crate::gas::{Gas, GasCharge, GasOutputs};
 use crate::kernel::{ClassifyResult, Context as _, ExecutionError, Kernel};
 use crate::machine::{Machine, BURNT_FUNDS_ACTOR_ADDR, REWARD_ACTOR_ADDR};
 
@@ -231,10 +231,13 @@ where
         let pl = &self.context().price_list;
 
         let (inclusion_cost, miner_penalty_amount) = match apply_kind {
-            ApplyKind::Implicit => (GasCharge::new("none", 0, 0), Default::default()),
+            ApplyKind::Implicit => (
+                GasCharge::new("none", Gas::zero(), Gas::zero()),
+                Default::default(),
+            ),
             ApplyKind::Explicit => {
                 let inclusion_cost = pl.on_chain_message(raw_length);
-                let inclusion_total = milligas_to_gas(inclusion_cost.total(), true);
+                let inclusion_total = inclusion_cost.total().round_up();
 
                 // Verify the cost of the message is not over the message gas limit.
                 if inclusion_total > msg.gas_limit {

fvm/src/gas/charge.rs

@@ -1,20 +1,20 @@
 // Copyright 2019-2022 ChainSafe Systems
 // SPDX-License-Identifier: Apache-2.0, MIT
 
-use crate::gas::Milligas;
+use super::Gas;
 
 /// Single gas charge in the VM. Contains information about what gas was for, as well
 /// as the amount of gas needed for computation and storage respectively.
 pub struct GasCharge<'a> {
     pub name: &'a str,
-    /// Compute costs in milligas.
-    pub compute_gas: Milligas,
-    /// Storage costs in milligas.
-    pub storage_gas: Milligas,
+    /// Compute costs
+    pub compute_gas: Gas,
+    /// Storage costs
+    pub storage_gas: Gas,
 }
 
 impl<'a> GasCharge<'a> {
-    pub fn new(name: &'a str, compute_gas: Milligas, storage_gas: Milligas) -> Self {
+    pub fn new(name: &'a str, compute_gas: Gas, storage_gas: Gas) -> Self {
         Self {
             name,
             compute_gas,
@@ -24,7 +24,7 @@ impl<'a> GasCharge<'a> {
 
     /// Calculates total gas charge (in milligas) by summing compute and
     /// storage gas associated with this charge.
-    pub fn total(&self) -> Milligas {
+    pub fn total(&self) -> Gas {
         self.compute_gas + self.storage_gas
     }
 }

fvm/src/gas/mod.rs

@@ -1,6 +1,9 @@
 // Copyright 2019-2022 ChainSafe Systems
 // SPDX-License-Identifier: Apache-2.0, MIT
 
+use std::fmt::{Debug, Display};
+use std::ops::{Add, AddAssign, Mul, Sub, SubAssign};
+
 pub use self::charge::GasCharge;
 pub(crate) use self::outputs::GasOutputs;
 pub use self::price_list::{price_list_by_network_version, PriceList, WasmGasPrices};
@@ -12,88 +15,190 @@ mod price_list;
 
 pub const MILLIGAS_PRECISION: i64 = 1000;
 
-// Type aliases to disambiguate units in interfaces.
-pub type Gas = i64;
-pub type Milligas = i64;
+/// A typesafe representation of gas (internally stored as milligas).
+///
+/// - All math operations are _saturating_ and never overflow.
+/// - Enforces correct units by making it impossible to, e.g., get gas squared (by multiplying gas
+///   by gas).
+/// - Makes it harder to confuse gas and milligas.
+#[derive(Hash, Eq, PartialEq, Ord, PartialOrd, Copy, Clone, Default)]
+pub struct Gas(i64 /* milligas */);
+
+impl Debug for Gas {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        if self.0 == 0 {
+            f.debug_tuple("Gas").field(&0 as &dyn Debug).finish()
+        } else {
+            let integral = self.0 / MILLIGAS_PRECISION;
+            let fractional = self.0 % MILLIGAS_PRECISION;
+            f.debug_tuple("Gas")
+                .field(&format_args!("{integral}.{fractional:03}"))
+                .finish()
+        }
+    }
+}
+
+impl Display for Gas {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        if self.0 == 0 {
+            f.write_str("0")
+        } else {
+            let integral = self.0 / MILLIGAS_PRECISION;
+            let fractional = self.0 % MILLIGAS_PRECISION;
+            write!(f, "{integral}.{fractional:03}")
+        }
+    }
+}
+
+impl Gas {
+    /// Construct a `Gas` from milligas.
+    #[inline]
+    pub fn from_milligas(milligas: i64) -> Gas {
+        Gas(milligas)
+    }
+
+    /// Construct a `Gas` from gas, scaling up. If this exceeds the width of an i64, it saturates at
+    /// `i64::MAX` milligas.
+    #[inline]
+    pub fn new(gas: i64) -> Gas {
+        Gas(gas.saturating_mul(MILLIGAS_PRECISION))
+    }
+
+    #[inline]
+    pub fn is_saturated(&self) -> bool {
+        self.0 == i64::MAX
+    }
+
+    /// Returns the gas value as an integer, rounding the fractional part up.
+    #[inline]
+    pub fn round_up(&self) -> i64 {
+        milligas_to_gas(self.0, true)
+    }
+
+    /// Returns the gas value as an integer, truncating the fractional part.
+    #[inline]
+    pub fn round_down(&self) -> i64 {
+        milligas_to_gas(self.0, false)
+    }
+
+    /// Returns the gas value as milligas, without loss of precision.
+    #[inline]
+    pub fn as_milligas(&self) -> i64 {
+        self.0
+    }
+}
+
+impl num_traits::Zero for Gas {
+    fn zero() -> Self {
+        Gas(0)
+    }
+
+    fn is_zero(&self) -> bool {
+        self.0 == 0
+    }
+}
+
+impl Add for Gas {
+    type Output = Gas;
+
+    #[inline]
+    fn add(self, rhs: Self) -> Self::Output {
+        Self(self.0.saturating_add(rhs.0))
+    }
+}
+
+impl AddAssign for Gas {
+    #[inline]
+    fn add_assign(&mut self, rhs: Self) {
+        self.0 = self.0.saturating_add(rhs.0)
+    }
+}
+
+impl SubAssign for Gas {
+    #[inline]
+    fn sub_assign(&mut self, rhs: Self) {
+        self.0 = self.0.saturating_sub(rhs.0)
+    }
+}
+
+impl Sub for Gas {
+    type Output = Gas;
+
+    #[inline]
+    fn sub(self, rhs: Self) -> Self::Output {
+        Self(self.0.saturating_sub(rhs.0))
+    }
+}
+
+impl Mul<i64> for Gas {
+    type Output = Gas;
+
+    #[inline]
+    fn mul(self, rhs: i64) -> Self::Output {
+        Self(self.0.saturating_mul(rhs))
+    }
+}
+
+impl Mul<i32> for Gas {
+    type Output = Gas;
+
+    #[inline]
+    fn mul(self, rhs: i32) -> Self::Output {
+        Self(self.0.saturating_mul(rhs.into()))
+    }
+}
 
 pub struct GasTracker {
-    milligas_limit: i64,
-    milligas_used: i64,
+    gas_limit: Gas,
+    gas_used: Gas,
 }
 
 impl GasTracker {
     /// Gas limit and gas used are provided in protocol units (i.e. full units).
     /// They are converted to milligas for internal canonical accounting.
     pub fn new(gas_limit: Gas, gas_used: Gas) -> Self {
         Self {
-            milligas_limit: gas_to_milligas(gas_limit),
-            milligas_used: gas_to_milligas(gas_used),
+            gas_limit,
+            gas_used,
         }
     }
 
     /// Safely consumes gas and returns an out of gas error if there is not sufficient
     /// enough gas remaining for charge.
-    pub fn charge_milligas(&mut self, name: &str, to_use: Milligas) -> Result<()> {
-        match self.milligas_used.checked_add(to_use) {
-            None => {
-                log::trace!("gas overflow: {}", name);
-                self.milligas_used = self.milligas_limit;
-                Err(ExecutionError::OutOfGas)
-            }
-            Some(used) => {
-                log::trace!("charged {} gas: {}", to_use, name);
-                if used > self.milligas_limit {
-                    log::trace!("out of gas: {}", name);
-                    self.milligas_used = self.milligas_limit;
-                    Err(ExecutionError::OutOfGas)
-                } else {
-                    self.milligas_used = used;
-                    Ok(())
-                }
-            }
+    pub fn charge_gas(&mut self, name: &str, to_use: Gas) -> Result<()> {
+        log::trace!("charging gas: {} {}", name, to_use);
+        // The gas type uses saturating math.
+        self.gas_used += to_use;
+        if self.gas_used > self.gas_limit {
+            log::trace!("gas limit reached");
+            self.gas_used = self.gas_limit;
+            Err(ExecutionError::OutOfGas)
+        } else {
+            Ok(())
         }
     }
 
     /// Applies the specified gas charge, where quantities are supplied in milligas.
     pub fn apply_charge(&mut self, charge: GasCharge) -> Result<()> {
-        self.charge_milligas(charge.name, charge.total())
+        self.charge_gas(charge.name, charge.total())
     }
 
-    /// Getter for gas available.
+    /// Getter for the maximum gas usable by this message.
     pub fn gas_limit(&self) -> Gas {
-        milligas_to_gas(self.milligas_limit, false)
-    }
-
-    /// Getter for milligas available.
-    pub fn milligas_limit(&self) -> Milligas {
-        self.milligas_limit
+        self.gas_limit
     }
 
     /// Getter for gas used.
     pub fn gas_used(&self) -> Gas {
-        milligas_to_gas(self.milligas_used, true)
-    }
-
-    /// Getter for milligas used.
-    pub fn milligas_used(&self) -> Milligas {
-        self.milligas_used
+        self.gas_used
     }
 
+    /// Getter for gas available.
     pub fn gas_available(&self) -> Gas {
-        milligas_to_gas(self.milligas_available(), false)
-    }
-
-    pub fn milligas_available(&self) -> Milligas {
-        self.milligas_limit.saturating_sub(self.milligas_used)
+        self.gas_limit - self.gas_used
     }
 }
 
-/// Converts the specified gas into equivalent fractional gas units
-#[inline]
-pub(crate) fn gas_to_milligas(gas: i64) -> i64 {
-    gas.saturating_mul(MILLIGAS_PRECISION)
-}
-
 /// Converts the specified fractional gas units into gas units
 #[inline]
 pub(crate) fn milligas_to_gas(milligas: i64, round_up: bool) -> i64 {
@@ -108,18 +213,20 @@ pub(crate) fn milligas_to_gas(milligas: i64, round_up: bool) -> i64 {
 
 #[cfg(test)]
 mod tests {
+    use num_traits::Zero;
+
     use super::*;
 
     #[test]
     #[allow(clippy::identity_op)]
     fn basic_gas_tracker() -> Result<()> {
-        let mut t = GasTracker::new(20, 10);
-        t.apply_charge(GasCharge::new("", 5 * MILLIGAS_PRECISION, 0))?;
-        assert_eq!(t.gas_used(), 15);
-        t.apply_charge(GasCharge::new("", 5 * MILLIGAS_PRECISION, 0))?;
-        assert_eq!(t.gas_used(), 20);
+        let mut t = GasTracker::new(Gas::new(20), Gas::new(10));
+        t.apply_charge(GasCharge::new("", Gas::new(5), Gas::zero()))?;
+        assert_eq!(t.gas_used(), Gas::new(15));
+        t.apply_charge(GasCharge::new("", Gas::new(5), Gas::zero()))?;
+        assert_eq!(t.gas_used(), Gas::new(20));
         assert!(t
-            .apply_charge(GasCharge::new("", 1 * MILLIGAS_PRECISION, 0))
+            .apply_charge(GasCharge::new("", Gas::new(1), Gas::zero()))
             .is_err());
         Ok(())
     }