inspection fixes

Author: janezpodhostnik

engine/execution/computation/computer/computer_test.go

@@ -352,9 +352,6 @@ func TestBlockExecutor_ExecuteBlock(t *testing.T) {
 			Return(noOpExecutor{}).
 			Once() // just system chunk
 
-		vm.On("Inspect", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).
-			Return(nil)
-
 		snapshot := storehouse.NewExecutingBlockSnapshot(
 			snapshot.MapStorageSnapshot{},
 			unittest.StateCommitmentFixture(),

engine/execution/computation/computer/transaction_coordinator.go

@@ -149,23 +149,10 @@ func (coordinator *transactionCoordinator) commit(txn *transaction) error {
 		return err
 	}
 
-	output := txn.Output()
-
-	// Run inspection on the transaction results.
-	// This is done here rather than in vm.Run() because the block computer
-	// uses NewExecutor for fine-grained transaction control.
-	output.InspectionResults = coordinator.vm.Inspect(
-		txn.request.ctx,
-		txn.request.TransactionProcedure,
-		coordinator.storageSnapshot,
-		executionSnapshot,
-		output,
-	)
-
 	coordinator.writeBehindLog.AddTransactionResult(
 		txn.request,
 		executionSnapshot,
-		output,
+		txn.Output(),
 		time.Since(txn.startedAt),
 		txn.numConflictRetries)
 

fvm/fvm.go

@@ -7,7 +7,6 @@ import (
 	"github.com/onflow/cadence"
 	"github.com/onflow/cadence/common"
 	"github.com/rs/zerolog"
-	"github.com/rs/zerolog/log"
 
 	"github.com/onflow/flow-go/fvm/inspection"
 
@@ -72,6 +71,25 @@ func (output *ProcedureOutput) PopulateEnvironmentValues(
 	return nil
 }
 
+func (output *ProcedureOutput) PopulateInspectionResults(
+	log zerolog.Logger,
+	ctx Context,
+	env environment.Environment,
+	storageSnapshot snapshot.StorageSnapshot,
+	executionSnapshot *snapshot.ExecutionSnapshot,
+) {
+
+	evts := make([]flow.Event, 0, len(output.Events)+len(output.ServiceEvents))
+	evts = append(evts, output.Events...)
+	evts = append(evts, output.ServiceEvents...)
+
+	log.Debug().Str("module", "tc-inspector").
+		Int("inspectors", len(ctx.Inspectors)).
+		Msg("populating environment values for procedure output")
+	inspectionResults := inspectProcedureResults(log, ctx, storageSnapshot, executionSnapshot, evts)
+	output.InspectionResults = inspectionResults
+}
+
 type ProcedureExecutor interface {
 	Preprocess() error
 	Execute() error
@@ -124,17 +142,6 @@ type VM interface {
 		ProcedureOutput,
 		error,
 	)
-
-	// Inspect runs configured inspectors on the procedure results.
-	// This is used by the block computer which manages transaction execution
-	// manually via NewExecutor rather than using Run.
-	Inspect(
-		ctx Context,
-		proc Procedure,
-		storageSnapshot snapshot.StorageSnapshot,
-		executionSnapshot *snapshot.ExecutionSnapshot,
-		output ProcedureOutput,
-	) []inspection.Result
 }
 
 var _ VM = (*VirtualMachine)(nil)
@@ -214,56 +221,22 @@ func (vm *VirtualMachine) Run(
 		return nil, ProcedureOutput{}, err
 	}
 
-	// This is of informative nature right now so this placement is ok
-	// In the future we will need to move this inside the procedure if we want it to affect execution
-	output := executor.Output()
-	log.Debug().Str("module", "tc-inspector").
-		Str("procedure-type", string(proc.Type())).
-		Int("inspectors", len(ctx.Inspectors)).
-		Msg("populating environment values for procedure output")
-	inspectionResults := vm.inspectProcedureResults(ctx.Logger, ctx, proc, storageSnapshot, executionSnapshot, output)
-	output.InspectionResults = inspectionResults
-
-	return executionSnapshot, output, nil
-}
-
-// Inspect runs configured inspectors on the procedure results.
-// This is used by the block computer which manages transaction execution
-// manually via NewExecutor rather than using Run.
-func (vm *VirtualMachine) Inspect(
-	ctx Context,
-	proc Procedure,
-	storageSnapshot snapshot.StorageSnapshot,
-	executionSnapshot *snapshot.ExecutionSnapshot,
-	output ProcedureOutput,
-) []inspection.Result {
-	return vm.inspectProcedureResults(ctx.Logger, ctx, proc, storageSnapshot, executionSnapshot, output)
+	return executionSnapshot, executor.Output(), nil
 }
 
-func (vm *VirtualMachine) inspectProcedureResults(
+func inspectProcedureResults(
 	logger zerolog.Logger,
 	context Context,
-	proc Procedure,
 	storageSnapshot snapshot.StorageSnapshot,
 	executionSnapshot *snapshot.ExecutionSnapshot,
-	output ProcedureOutput,
+	events []flow.Event,
 ) []inspection.Result {
-	// TODO(janezp): this should be decided by the inspector
-	if proc.Type() != TransactionProcedureType {
-		logger.Debug().Str("module", "tc-inspector").Msg("skipping inspection for non-transaction procedure")
-		return nil
-	}
-
-	// TODO(janezp): inspector should be able to receive ProcedureOutput directly
-	evts := make([]flow.Event, 0, len(output.Events)+len(output.ServiceEvents))
-	evts = append(evts, output.Events...)
-	evts = append(evts, output.ServiceEvents...)
-
 	inspectionResults := make([]inspection.Result, len(context.Inspectors))
 	logger.Debug().Str("module", "tc-inspector").Int("num_inspectors", len(context.Inspectors)).Msg("inspecting procedure results")
 	var err error
 	for i, inspector := range context.Inspectors {
-		inspectionResults[i], err = inspector.Inspect(logger, storageSnapshot, executionSnapshot, evts)
+		// TODO(janezp): inspector should be able to receive ProcedureOutput directly
+		inspectionResults[i], err = inspector.Inspect(logger, storageSnapshot, executionSnapshot, events)
 		if err != nil {
 			logger.Warn().Str("module", "tc-inspector").Err(err).Msg("failed to inspect procedure results")
 		}

fvm/fvm_test.go

@@ -12,7 +12,6 @@ import (
 	"testing"
 
 	"github.com/onflow/flow-core-contracts/lib/go/templates"
-	"github.com/rs/zerolog"
 	"github.com/stretchr/testify/assert"
 	mockery "github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
@@ -4613,6 +4612,10 @@ func TestFlowTokenChangesInspector(t *testing.T) {
 
 					differ := inspection.NewTokenChangesInspector(tc.tokenDefinitions)
 
+					// Add the inspector to the context so inspection runs
+					// as part of the transaction execution pipeline.
+					ctx = fvm.NewContextFromParent(ctx, fvm.WithInspectors([]inspection.Inspector{differ}))
+
 					// Create an account private key.
 					privateKey, err := testutil.GenerateAccountPrivateKey()
 					require.NoError(t, err)
@@ -4628,7 +4631,7 @@ func TestFlowTokenChangesInspector(t *testing.T) {
 
 					txBody := tc.txBody(t, chain, accounts)
 
-					executionSnapshot, output, err := vm.Run(
+					_, output, err := vm.Run(
 						ctx,
 						fvm.Transaction(txBody, 0),
 						snapshotTree)
@@ -4640,14 +4643,8 @@ func TestFlowTokenChangesInspector(t *testing.T) {
 						require.NoError(t, output.Err)
 					}
 
-					evts := make([]flow.Event, 0, len(output.Events)+len(output.ServiceEvents))
-					evts = append(evts, output.Events...)
-					evts = append(evts, output.ServiceEvents...)
-
-					diff, err := differ.Inspect(zerolog.Nop(), snapshotTree, executionSnapshot, evts)
-					require.NoError(t, err)
-
-					tc.resultChecker(t, diff.(inspection.TokenDiffResult))
+					require.Len(t, output.InspectionResults, 1, "expected one inspection result")
+					tc.resultChecker(t, output.InspectionResults[0].(inspection.TokenDiffResult))
 				},
 			)
 	}

fvm/mock/vm.go

@@ -131,3 +131,9 @@ func (state *storageState) interimReadSet(
 		accumulator[id] = struct{}{}
 	}
 }
+
+// BaseStorageSnapshot gives access to the storage snapshot as it was without changes
+// WARNING: this should not be read mid-transaction as reads to it are not recorded in the spocks
+func (state *storageState) BaseStorageSnapshot() snapshot.StorageSnapshot {
+	return state.baseStorage
+}

fvm/storage/state/storage_state.go

@@ -160,6 +160,10 @@ type NestedTransactionPreparer interface {
 	Get(id flow.RegisterID) (flow.RegisterValue, error)
 
 	Set(id flow.RegisterID, value flow.RegisterValue) error
+
+	// BaseStorageSnapshot gives access to the storage snapshot as it was without changes
+	// WARNING: this should not be read mid-transaction as reads to it are not recorded in the spocks
+	BaseStorageSnapshot() snapshot.StorageSnapshot
 }
 
 type nestedTransactionStackFrame struct {
@@ -450,6 +454,10 @@ func (txnState *transactionState) Set(
 	return txnState.current().Set(id, value)
 }
 
+func (txnState *transactionState) BaseStorageSnapshot() snapshot.StorageSnapshot {
+	return txnState.nestedTransactions[0].ExecutionState.BaseStorageSnapshot()
+}
+
 func (txnState *transactionState) MeterComputation(usage common.ComputationUsage) error {
 	return txnState.current().MeterComputation(usage)
 }

fvm/storage/state/transaction_state.go

@@ -465,5 +465,7 @@ func (executor *transactionExecutor) commit(
 			commitErr)
 	}
 
+	executor.output.PopulateInspectionResults(executor.ctx.Logger, executor.ctx, executor.env, executor.txnState, executor.executionStateRead)
+
 	return nil
 }