fix(fvm): fix (and prevent) two bugs in out of gas handling.

My last commit introduced a bug where I ignored the error from a call to
`CallManager::charge_gas` when we _meant_ to simply ignore the gas
timer, not the entire error. This bug was in the charge for
`on_method_return`.

So I decided to remove the `must_use` marker from gas timers _in
general_ so we didn't have to explicitly ignore the results. We
generally don't care about these timers anyways.

While doing that, I found another bug in the `tipset_cid` syscall where
we made the same mistake. this issue is tricker because this code is
already deployed to mainnet, but we should be able to fix it without any
consensus issues. Basically, right now:

1. We won't detect the out of gas condition when we should.
2. We'll fetch the requested tipset CID from state.
3. We'll return it to the user.
4. The actor (the EVM) will try to charge a bit of gas when handling the
return value from the syscall.
5. The actor will run out of gas and abort with an OutOfGas.

Now, we'll just abort with an OutOfGas error immediately.

fvm/src/call_manager/default.rs

@@ -398,7 +398,7 @@ where
             None => {
                 // We charge for creating the actor (storage) but not for address assignment as the
                 // init actor has already handled that for us.
-                let _ = self.charge_gas(self.price_list().on_create_actor(false))?;
+                self.charge_gas(self.price_list().on_create_actor(false))?;
                 ActorState::new_empty(code_id, delegated_address)
             }
         };
@@ -422,8 +422,7 @@ where
             return Ok(Some(id));
         }
         if !self.state_access_tracker.get_address_lookup_state(address) {
-            let _ = self
-                .gas_tracker
+            self.gas_tracker
                 .apply_charge(self.price_list().on_resolve_address())?;
         }
         let id = self.state_tree().lookup_id(address)?;
@@ -436,8 +435,7 @@ where
     fn get_actor(&self, id: ActorID) -> Result<Option<ActorState>> {
         let access = self.state_access_tracker.get_actor_access_state(id);
         if access < Some(ActorAccessState::Read) {
-            let _ = self
-                .gas_tracker
+            self.gas_tracker
                 .apply_charge(self.price_list().on_actor_lookup())?;
         }
         let actor = self.state_tree().get_actor(id)?;
@@ -448,13 +446,11 @@ where
     fn set_actor(&mut self, id: ActorID, state: ActorState) -> Result<()> {
         let access = self.state_access_tracker.get_actor_access_state(id);
         if access < Some(ActorAccessState::Read) {
-            let _ = self
-                .gas_tracker
+            self.gas_tracker
                 .apply_charge(self.price_list().on_actor_lookup())?;
         }
         if access < Some(ActorAccessState::Updated) {
-            let _ = self
-                .gas_tracker
+            self.gas_tracker
                 .apply_charge(self.price_list().on_actor_update())?;
         }
         self.state_tree_mut().set_actor(id, state);
@@ -465,13 +461,11 @@ where
     fn delete_actor(&mut self, id: ActorID) -> Result<()> {
         let access = self.state_access_tracker.get_actor_access_state(id);
         if access < Some(ActorAccessState::Read) {
-            let _ = self
-                .gas_tracker
+            self.gas_tracker
                 .apply_charge(self.price_list().on_actor_lookup())?;
         }
         if access < Some(ActorAccessState::Updated) {
-            let _ = self
-                .gas_tracker
+            self.gas_tracker
                 .apply_charge(self.price_list().on_actor_update())?;
         }
         self.state_tree_mut().delete_actor(id);
@@ -536,7 +530,7 @@ where
     fn create_actor_from_send(&mut self, addr: &Address, act: ActorState) -> Result<ActorID> {
         // This will charge for the address assignment and the actor storage, but not the actor
         // lookup/update (charged below in `set_actor`).
-        let _ = self.charge_gas(self.price_list().on_create_actor(true))?;
+        self.charge_gas(self.price_list().on_create_actor(true))?;
         let addr_id = self.state_tree_mut().register_new_address(addr)?;
         self.state_access_tracker.record_lookup_address(addr);
 
@@ -778,7 +772,7 @@ where
             });
 
             // Process the result, updating the backtrace if necessary.
-            let ret = match result {
+            let mut ret = match result {
                 Ok(ret) => Ok(InvocationResult {
                     exit_code: ExitCode::OK,
                     value: ret.cloned(),
@@ -851,7 +845,9 @@ where
                     .price_list()
                     .on_method_return(cm.call_stack_depth, ret_size)
                 {
-                    let _ = cm.charge_gas(charge);
+                    if let Err(e) = cm.charge_gas(charge) {
+                        ret = Err(e);
+                    }
                 }
             }
 

fvm/src/gas/mod.rs

@@ -298,9 +298,9 @@ mod tests {
     #[allow(clippy::identity_op)]
     fn basic_gas_tracker() -> Result<()> {
         let t = GasTracker::new(Gas::new(20), Gas::new(10), false);
-        let _ = t.apply_charge(GasCharge::new("", Gas::new(5), Gas::zero()))?;
+        t.apply_charge(GasCharge::new("", Gas::new(5), Gas::zero()))?;
         assert_eq!(t.gas_used(), Gas::new(15));
-        let _ = t.apply_charge(GasCharge::new("", Gas::new(5), Gas::zero()))?;
+        t.apply_charge(GasCharge::new("", Gas::new(5), Gas::zero()))?;
         assert_eq!(t.gas_used(), Gas::new(20));
         assert!(t
             .apply_charge(GasCharge::new("", Gas::new(1), Gas::zero()))

fvm/src/gas/timer.rs

@@ -27,7 +27,6 @@ pub type GasInstant = Instant;
 
 /// A handle returned by `charge_gas` which must be used to mark the end of
 /// the execution associated with that gas.
-#[must_use]
 #[derive(Debug)]
 pub struct GasTimer(Option<GasTimerInner>);
 

fvm/src/kernel/default.rs

@@ -264,8 +264,7 @@ where
     fn block_open(&mut self, cid: &Cid) -> Result<(BlockId, BlockStat)> {
         // TODO(M2): Check for reachability here.
 
-        let _ = self
-            .call_manager
+        self.call_manager
             .charge_gas(self.call_manager.price_list().on_block_open_base())?;
 
         let start = GasTimer::start();
@@ -737,9 +736,8 @@ where
             );
         }
 
-        let _ = self
-            .call_manager
-            .charge_gas(self.call_manager.price_list().on_tipset_cid(offset > 1));
+        self.call_manager
+            .charge_gas(self.call_manager.price_list().on_tipset_cid(offset > 1))?;
 
         self.call_manager.externs().get_tipset_cid(epoch).or_fatal()
     }

fvm/src/syscalls/bind.rs

@@ -97,7 +97,7 @@ fn memory_and_data<'a, K: Kernel>(
 macro_rules! charge_syscall_gas {
     ($kernel:expr) => {
         let charge = $kernel.price_list().on_syscall();
-        let _ = $kernel
+        $kernel
             .charge_gas(&charge.name, charge.compute_gas)
             .map_err(Abort::from_error_as_fatal)?;
     };

fvm/src/syscalls/mod.rs

@@ -151,8 +151,7 @@ pub fn charge_for_exec<K: Kernel>(
         // Only recording time for the execution, not for the memory part, which is unknown. But we
         // could perform stomething like a multi-variate linear regression to see if the amount of
         // memory explains any of the exectuion time.
-        let _ = data
-            .kernel
+        data.kernel
             .charge_gas("wasm_memory_grow", memory_gas_charge)
             .map_err(Abort::from_error_as_fatal)?;
     }
@@ -179,7 +178,7 @@ pub fn charge_for_init<K: Kernel>(
 
     if let Some(min_table_elements) = min_table_elements(module) {
         let table_gas = data.kernel.price_list().init_table_gas(min_table_elements);
-        let _ = data.kernel.charge_gas("wasm_table_init", table_gas)?;
+        data.kernel.charge_gas("wasm_table_init", table_gas)?;
     }
 
     data.kernel.charge_gas("wasm_memory_init", memory_gas)