Merge #3204

3204: [FVM] Handle cadence runtime error early r=janezpodhostnik a=janezpodhostnik

Handle cadence errors as early as possible, so we don't forget to do it later.

Co-authored-by: Janez Podhostnik <janez.podhostnik@gmail.com>

fvm/environment/system_contracts.go

@@ -6,7 +6,6 @@ import (
 	"github.com/onflow/cadence/runtime/sema"
 	"go.opentelemetry.io/otel/attribute"
 
-	"github.com/onflow/flow-go/fvm/errors"
 	"github.com/onflow/flow-go/fvm/systemcontracts"
 	"github.com/onflow/flow-go/model/flow"
 	"github.com/onflow/flow-go/module/trace"
@@ -64,8 +63,6 @@ func (sys *SystemContracts) Invoke(
 		spec.ArgumentTypes,
 	)
 	if err != nil {
-		// this is an error coming from Cadendce runtime, so it must be handled first.
-		err = errors.HandleRuntimeError(err)
 		sys.env.Logger().
 			Info().
 			Err(err).

fvm/environment/system_contracts_test.go

@@ -1,21 +1,18 @@
 package environment_test
 
 import (
-	"fmt"
 	"testing"
 
 	"github.com/onflow/cadence"
 	"github.com/onflow/cadence/runtime"
 	"github.com/onflow/cadence/runtime/common"
-	runtimeerrors "github.com/onflow/cadence/runtime/errors"
 	"github.com/onflow/cadence/runtime/sema"
 	"github.com/rs/zerolog"
 	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
 
 	"github.com/onflow/flow-go/fvm/environment"
 	fvmMock "github.com/onflow/flow-go/fvm/environment/mock"
-	"github.com/onflow/flow-go/fvm/errors"
 	reusableRuntime "github.com/onflow/flow-go/fvm/runtime"
 	"github.com/onflow/flow-go/fvm/runtime/testutil"
 	"github.com/onflow/flow-go/model/flow"
@@ -56,54 +53,6 @@ func TestSystemContractsInvoke(t *testing.T) {
 				require.Equal(t, cadence.String("value"), v)
 			},
 		},
-		{
-			name: "unknown error from contract function is handled",
-			contractFunction: func(a common.AddressLocation, s string, values []cadence.Value, types []sema.Type, ctx runtime.Context) (cadence.Value, error) {
-				return nil, fmt.Errorf("non-runtime error")
-			},
-			require: func(t *testing.T, v cadence.Value, err error) {
-				require.Error(t, err)
-
-				var failure errors.Failure
-				require.ErrorAs(t, err, &failure)
-
-				errors.As(err, &failure)
-
-				require.Equal(t, errors.FailureCodeUnknownFailure, failure.FailureCode())
-			},
-		},
-		{
-			name: "known error from contract function is handled",
-			contractFunction: func(a common.AddressLocation, s string, values []cadence.Value, types []sema.Type, ctx runtime.Context) (cadence.Value, error) {
-				return nil, runtime.Error{
-					Err:      fmt.Errorf("some error"),
-					Location: common.AddressLocation{},
-				}
-			},
-			require: func(t *testing.T, v cadence.Value, err error) {
-				require.Error(t, err)
-
-				var rterr *errors.CadenceRuntimeError
-				require.ErrorAs(t, err, &rterr)
-			},
-		},
-		{
-			name: "external error from contract function is unwrapped",
-			contractFunction: func(a common.AddressLocation, s string, values []cadence.Value, types []sema.Type, ctx runtime.Context) (cadence.Value, error) {
-				return nil, runtime.Error{
-					Err: runtimeerrors.ExternalError{
-						Recovered: &errors.ContractNotFoundError{},
-					},
-					Location: common.AddressLocation{},
-				}
-			},
-			require: func(t *testing.T, v cadence.Value, err error) {
-				require.Error(t, err)
-
-				var someFVMerr *errors.ContractNotFoundError
-				require.ErrorAs(t, err, &someFVMerr)
-			},
-		},
 	}
 
 	for _, tc := range testCases {

fvm/errors/errors.go

@@ -63,6 +63,10 @@ func SplitErrorTypes(inp error) (err Error, failure Failure) {
 // HandleRuntimeError handles runtime errors and separates
 // errors generated by runtime from fvm errors (e.g. environment errors)
 func HandleRuntimeError(err error) error {
+	if err == nil {
+		return nil
+	}
+
 	// if is not a runtime error return as vm error
 	// this should never happen unless a bug in the code
 	runErr, ok := err.(runtime.Error)

fvm/fvm.go

@@ -101,12 +101,7 @@ func (vm *VirtualMachine) RunV2(
 			WithMaxInteractionSizeAllowed(interactionLimit),
 	)
 
-	err = proc.Run(vm, ctx, stTxn, blockPrograms)
-	if err != nil {
-		return err
-	}
-
-	return nil
+	return proc.Run(vm, ctx, stTxn, blockPrograms)
 }
 
 // GetAccount returns an account by address or an error if none exists.

fvm/fvm_blockcontext_test.go

@@ -1128,7 +1128,8 @@ func TestBlockContext_ExecuteTransaction_InteractionLimitReached(t *testing.T) {
 				err = vm.RunV2(ctx, tx, view)
 				require.NoError(t, err)
 				require.Error(t, tx.Err)
-				assert.Equal(t, (&errors.LedgerInteractionLimitExceededError{}).Code(), tx.Err.Code())
+
+				assert.Equal(t, (&errors.CadenceRuntimeError{}).Code(), tx.Err.Code())
 			}))
 }
 

fvm/runtime/reusable_cadence_runtime.go

@@ -225,7 +225,10 @@ func (pool ReusableCadenceRuntimePool) Borrow(
 	case reusable = <-pool.pool:
 		// Do nothing.
 	default:
-		reusable = NewReusableCadenceRuntime(pool.newRuntime())
+		reusable = NewReusableCadenceRuntime(
+			WrappedCadenceRuntime{
+				pool.newRuntime(),
+			})
 	}
 
 	reusable.SetFvmEnvironment(fvmEnv)

fvm/runtime/wrapped_cadence_runtime.go

@@ -0,0 +1,95 @@
+package runtime
+
+import (
+	"github.com/onflow/cadence"
+	"github.com/onflow/cadence/runtime"
+	"github.com/onflow/cadence/runtime/common"
+	"github.com/onflow/cadence/runtime/interpreter"
+	"github.com/onflow/cadence/runtime/sema"
+
+	"github.com/onflow/flow-go/fvm/errors"
+)
+
+// WrappedCadenceRuntime wraps cadence runtime to handle errors.
+// Errors from cadence runtime should be handled with `errors.HandleRuntimeError` before the FVM can understand them.
+// Handling all possible locations, where the error could be coming from, here,
+// makes it impossible to forget to handle the error.
+type WrappedCadenceRuntime struct {
+	runtime.Runtime
+}
+
+func (wr WrappedCadenceRuntime) NewScriptExecutor(s runtime.Script, c runtime.Context) runtime.Executor {
+	return WrappedCadenceExecutor{wr.Runtime.NewScriptExecutor(s, c)}
+}
+
+func (wr WrappedCadenceRuntime) ExecuteScript(s runtime.Script, c runtime.Context) (cadence.Value, error) {
+	v, err := wr.Runtime.ExecuteScript(s, c)
+	return v, errors.HandleRuntimeError(err)
+}
+
+func (wr WrappedCadenceRuntime) NewTransactionExecutor(s runtime.Script, c runtime.Context) runtime.Executor {
+	return WrappedCadenceExecutor{wr.Runtime.NewTransactionExecutor(s, c)}
+}
+
+func (wr WrappedCadenceRuntime) ExecuteTransaction(s runtime.Script, c runtime.Context) error {
+	err := wr.Runtime.ExecuteTransaction(s, c)
+	return errors.HandleRuntimeError(err)
+}
+
+func (wr WrappedCadenceRuntime) NewContractFunctionExecutor(
+	contractLocation common.AddressLocation,
+	functionName string,
+	arguments []cadence.Value,
+	argumentTypes []sema.Type,
+	context runtime.Context,
+) runtime.Executor {
+	return WrappedCadenceExecutor{wr.Runtime.NewContractFunctionExecutor(contractLocation, functionName, arguments, argumentTypes, context)}
+}
+
+func (wr WrappedCadenceRuntime) InvokeContractFunction(
+	contractLocation common.AddressLocation,
+	functionName string,
+	arguments []cadence.Value,
+	argumentTypes []sema.Type,
+	context runtime.Context,
+) (cadence.Value, error) {
+	v, err := wr.Runtime.InvokeContractFunction(contractLocation, functionName, arguments, argumentTypes, context)
+	return v, errors.HandleRuntimeError(err)
+}
+
+func (wr WrappedCadenceRuntime) ParseAndCheckProgram(source []byte, context runtime.Context) (*interpreter.Program, error) {
+	p, err := wr.Runtime.ParseAndCheckProgram(source, context)
+	return p, errors.HandleRuntimeError(err)
+}
+
+func (wr WrappedCadenceRuntime) ReadStored(address common.Address, path cadence.Path, context runtime.Context) (cadence.Value, error) {
+	v, err := wr.Runtime.ReadStored(address, path, context)
+	return v, errors.HandleRuntimeError(err)
+}
+
+func (wr WrappedCadenceRuntime) ReadLinked(address common.Address, path cadence.Path, context runtime.Context) (cadence.Value, error) {
+	v, err := wr.Runtime.ReadLinked(address, path, context)
+	return v, errors.HandleRuntimeError(err)
+}
+
+func (wr WrappedCadenceRuntime) Storage(context runtime.Context) (*runtime.Storage, *interpreter.Interpreter, error) {
+	s, i, err := wr.Runtime.Storage(context)
+	return s, i, errors.HandleRuntimeError(err)
+}
+
+type WrappedCadenceExecutor struct {
+	runtime.Executor
+}
+
+func (we WrappedCadenceExecutor) Preprocess() error {
+	return errors.HandleRuntimeError(we.Executor.Preprocess())
+}
+
+func (we WrappedCadenceExecutor) Execute() error {
+	return errors.HandleRuntimeError(we.Executor.Execute())
+}
+
+func (we WrappedCadenceExecutor) Result() (cadence.Value, error) {
+	v, err := we.Executor.Result()
+	return v, errors.HandleRuntimeError(err)
+}

fvm/script.go

@@ -139,7 +139,7 @@ func (i ScriptInvoker) Process(
 		common.ScriptLocation(proc.ID))
 
 	if err != nil {
-		return errors.HandleRuntimeError(err)
+		return err
 	}
 
 	proc.Value = value

fvm/transactionInvoker.go

@@ -102,7 +102,7 @@ func (i TransactionInvoker) Process(
 			// Otherwise, do what we use to do
 			txError = fmt.Errorf(
 				"transaction invocation failed when executing transaction: %w",
-				errors.HandleRuntimeError(err),
+				err,
 			)
 		}
 	}

fvm/transaction_test.go

@@ -495,11 +495,13 @@ func TestAccountFreezing(t *testing.T) {
 		proc := fvm.Transaction(tx, 0)
 
 		txInvoker := fvm.NewTransactionInvoker()
-		err := txInvoker.Process(vm, &context, proc, st, programsStorage)
+		var err error
+		err = txInvoker.Process(vm, &context, proc, st, programsStorage)
 		require.NoError(t, err)
 
 		// make sure freeze status is correct
-		frozen, err := accounts.GetAccountFrozen(frozenAddress)
+		var frozen bool
+		frozen, err = accounts.GetAccountFrozen(frozenAddress)
 		require.NoError(t, err)
 		require.True(t, frozen)