bugfix: Produce correct receipts with debugger breakpoints enabled

CHANGELOG.md

@@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 ## [Unreleased]
 
+### Fixed
+- [889](https://github.com/FuelLabs/fuel-vm/pull/889): Debugger breakpoint caused receipts to be produced incorrectly.
+
 ## [Version 0.59.1]
 
 ### Fixed

fuel-vm/src/interpreter/executors/main.rs

@@ -944,82 +944,9 @@ where
             )?;
             ProgramState::Return(1)
         } else {
-            let gas_limit;
-            let is_empty_script;
-            if let Some(script) = self.transaction().as_script() {
-                gas_limit = *script.script_gas_limit();
-                is_empty_script = script.script().is_empty();
-            } else {
-                unreachable!(
-                    "Only `Script` transactions can be executed inside of the VM"
-                )
-            }
-
-            // TODO set tree balance
-
             // `Interpreter` supports only `Create` and `Script` transactions. It is not
             // `Create` -> it is `Script`.
-            let program = if !is_empty_script {
-                self.run_program()
-            } else {
-                // Return `1` as successful execution.
-                let return_val = 1;
-                self.ret(return_val)?;
-                Ok(ProgramState::Return(return_val))
-            };
-
-            let gas_used = gas_limit
-                .checked_sub(self.remaining_gas())
-                .ok_or_else(|| Bug::new(BugVariant::GlobalGasUnderflow))?;
-
-            // Catch VM panic and don't propagate, generating a receipt
-            let (status, program) = match program {
-                Ok(s) => {
-                    // either a revert or success
-                    let res = if let ProgramState::Revert(_) = &s {
-                        ScriptExecutionResult::Revert
-                    } else {
-                        ScriptExecutionResult::Success
-                    };
-                    (res, s)
-                }
-
-                Err(e) => match e.instruction_result() {
-                    Some(result) => {
-                        self.append_panic_receipt(result);
-
-                        (ScriptExecutionResult::Panic, ProgramState::Revert(0))
-                    }
-
-                    // This isn't a specified case of an erroneous program and should be
-                    // propagated. If applicable, OS errors will fall into this category.
-                    None => return Err(e),
-                },
-            };
-
-            let receipt = Receipt::script_result(status, gas_used);
-
-            self.receipts.push(receipt)?;
-
-            if program.is_debug() {
-                self.debugger_set_last_state(program);
-            }
-
-            let revert = matches!(program, ProgramState::Revert(_));
-            let gas_price = self.gas_price();
-            Self::finalize_outputs(
-                &mut self.tx,
-                &gas_costs,
-                &fee_params,
-                &base_asset_id,
-                revert,
-                gas_used,
-                &self.initial_balances,
-                &self.balances,
-                gas_price,
-            )?;
-
-            program
+            self.run_program()?
         };
         self.update_transaction_outputs()?;
 
@@ -1029,29 +956,94 @@ where
     pub(crate) fn run_program(
         &mut self,
     ) -> Result<ProgramState, InterpreterError<S::DataError>> {
-        loop {
-            // Check whether the instruction will be executed in a call context
-            let in_call = !self.frames.is_empty();
-
-            let state = self.execute()?;
-
-            if in_call {
-                // Only reverts should terminate execution from a call context
-                match state {
-                    ExecuteState::Revert(r) => return Ok(ProgramState::Revert(r)),
-                    ExecuteState::DebugEvent(d) => return Ok(ProgramState::RunProgram(d)),
-                    _ => {}
-                }
-            } else {
-                match state {
-                    ExecuteState::Return(r) => return Ok(ProgramState::Return(r)),
-                    ExecuteState::ReturnData(d) => return Ok(ProgramState::ReturnData(d)),
-                    ExecuteState::Revert(r) => return Ok(ProgramState::Revert(r)),
-                    ExecuteState::DebugEvent(d) => return Ok(ProgramState::RunProgram(d)),
-                    ExecuteState::Proceed => {}
+        let Some(script) = self.transaction().as_script() else {
+            unreachable!("Only `Script` transactions can be executed inside of the VM")
+        };
+        let gas_limit = *script.script_gas_limit();
+
+        let (result, state) = if script.script().is_empty() {
+            // Empty script is special-cased to simply return `1` as successful execution.
+            let return_val = 1;
+            self.ret(return_val)?;
+            (
+                ScriptExecutionResult::Success,
+                ProgramState::Return(return_val),
+            )
+        } else {
+            // TODO set tree balance
+            loop {
+                // Check whether the instruction will be executed in a call context
+                let in_call = !self.frames.is_empty();
+
+                match self.execute() {
+                    // Proceeding with the execution normally
+                    Ok(ExecuteState::Proceed) => continue,
+                    // Debugger events are returned directly to the caller
+                    Ok(ExecuteState::DebugEvent(d)) => {
+                        self.debugger_set_last_state(ProgramState::RunProgram(d));
+                        return Ok(ProgramState::RunProgram(d));
+                    }
+                    // Reverting terminated execution immediately
+                    Ok(ExecuteState::Revert(r)) => {
+                        break (ScriptExecutionResult::Revert, ProgramState::Revert(r))
+                    }
+                    // Returning in call context is ignored
+                    Ok(ExecuteState::Return(_) | ExecuteState::ReturnData(_))
+                        if in_call =>
+                    {
+                        continue
+                    }
+                    // In non-call context, returning terminates the execution
+                    Ok(ExecuteState::Return(r)) => {
+                        break (ScriptExecutionResult::Success, ProgramState::Return(r))
+                    }
+                    Ok(ExecuteState::ReturnData(d)) => {
+                        break (
+                            ScriptExecutionResult::Success,
+                            ProgramState::ReturnData(d),
+                        )
+                    }
+                    // Error always terminates the execution
+                    Err(e) => match e.instruction_result() {
+                        Some(result) => {
+                            self.append_panic_receipt(result);
+                            break (ScriptExecutionResult::Panic, ProgramState::Revert(0));
+                        }
+                        // This isn't a specified case of an erroneous program and should
+                        // be propagated. If applicable, OS errors
+                        // will fall into this category.
+                        // The VM state is not finalized in this case.
+                        None => return Err(e),
+                    },
                 }
             }
-        }
+        };
+
+        // Produce result receipt
+        let gas_used = gas_limit
+            .checked_sub(self.remaining_gas())
+            .ok_or_else(|| Bug::new(BugVariant::GlobalGasUnderflow))?;
+        self.receipts
+            .push(Receipt::script_result(result, gas_used))?;
+
+        // Finalize the outputs
+        let fee_params = *self.fee_params();
+        let base_asset_id = *self.base_asset_id();
+        let gas_costs = self.gas_costs().clone();
+        let gas_price = self.gas_price();
+        Self::finalize_outputs(
+            &mut self.tx,
+            &gas_costs,
+            &fee_params,
+            &base_asset_id,
+            matches!(state, ProgramState::Revert(_)),
+            gas_used,
+            &self.initial_balances,
+            &self.balances,
+            gas_price,
+        )?;
+
+        Ok(state)
     }
 
     /// Update tx fields after execution

fuel-vm/src/tests/crypto.rs

@@ -35,7 +35,6 @@ use sha3::{
 
 use crate::{
     prelude::*,
-    storage::predicate::EmptyStorage,
     tests::test_helpers::set_full_word,
     util::test_helpers::check_expected_reason_for_instructions,
 };
@@ -237,7 +236,7 @@ async fn recover_tx_id_predicate() {
             .estimate_predicates_async::<TokioWithRayon>(
                 &check_params,
                 &DummyPool,
-                &EmptyStorage,
+                &crate::storage::predicate::EmptyStorage,
             )
             .await
             .expect("Should estimate predicate successfully");
@@ -248,8 +247,12 @@ async fn recover_tx_id_predicate() {
     }
 
     // sequential version
-    tx.estimate_predicates(&check_params, MemoryInstance::new(), &EmptyStorage)
-        .expect("Should estimate predicate successfully");
+    tx.estimate_predicates(
+        &check_params,
+        MemoryInstance::new(),
+        &crate::storage::predicate::EmptyStorage,
+    )
+    .expect("Should estimate predicate successfully");
 
     tx.into_checked(maturity, &consensus_params)
         .expect("Should check predicate successfully");

fuel-vm/src/tests/debugger.rs

@@ -0,0 +1,72 @@
+use alloc::{
+    vec,
+    vec::Vec,
+};
+
+use fuel_asm::{
+    op,
+    RegId,
+};
+use fuel_tx::{
+    ConsensusParameters,
+    Finalizable,
+    GasCosts,
+    Script,
+    TransactionBuilder,
+};
+
+use crate::{
+    prelude::{
+        Interpreter,
+        IntoChecked,
+    },
+    state::ProgramState,
+};
+
+#[test]
+fn receipts_are_produced_correctly_with_stepping() {
+    let script = vec![
+        op::movi(0x20, 1234),
+        op::log(0x20, RegId::ZERO, RegId::ZERO, RegId::ZERO),
+        op::ret(RegId::ONE),
+    ]
+    .into_iter()
+    .collect();
+
+    let params = ConsensusParameters::standard();
+    let tx = TransactionBuilder::script(script, Vec::new())
+        .script_gas_limit(1_000_000)
+        .maturity(Default::default())
+        .add_fee_input()
+        .finalize()
+        .into_checked(Default::default(), &params)
+        .expect("failed to check tx")
+        .into_ready(0, &GasCosts::default(), params.fee_params(), None)
+        .expect("failed to ready tx");
+
+    let mut vm = Interpreter::<_, _, Script>::with_memory_storage();
+    vm.transact(tx.clone()).expect("panicked");
+    let receipts_without_debugger = vm.receipts().to_vec();
+
+    let mut vm = Interpreter::<_, _, Script>::with_memory_storage();
+    vm.set_single_stepping(true);
+    let mut t = *vm.transact(tx).expect("panicked").state();
+    loop {
+        match t {
+            ProgramState::Return(_)
+            | ProgramState::ReturnData(_)
+            | ProgramState::Revert(_) => {
+                break;
+            }
+            ProgramState::RunProgram(_) => {
+                t = vm.resume().expect("panicked");
+            }
+            ProgramState::VerifyPredicate(_) => {
+                unreachable!("no predicates in this test")
+            }
+        }
+    }
+    let receipts_with_debugger = vm.receipts();
+
+    assert_eq!(receipts_without_debugger, receipts_with_debugger);
+}

fuel-vm/src/tests/mod.rs

@@ -17,6 +17,7 @@ mod code_coverage;
 mod coins;
 mod contract;
 mod crypto;
+mod debugger;
 mod encoding;
 mod external;
 mod flow;