Merge pull request #6461 from onflow/janez/fix-metering-invalidation-…

…port

[FVM] Fix metering invalidation - port

fvm/environment/derived_data_invalidator.go

@@ -5,7 +5,6 @@ import (
 
 	"github.com/onflow/flow-go/fvm/storage/derived"
 	"github.com/onflow/flow-go/fvm/storage/snapshot"
-	"github.com/onflow/flow-go/model/flow"
 )
 
 type ContractUpdate struct {
@@ -33,44 +32,37 @@ var _ derived.TransactionInvalidator = DerivedDataInvalidator{}
 
 func NewDerivedDataInvalidator(
 	contractUpdates ContractUpdates,
-	serviceAddress flow.Address,
 	executionSnapshot *snapshot.ExecutionSnapshot,
+	meterStateRead *snapshot.ExecutionSnapshot,
 ) DerivedDataInvalidator {
 	return DerivedDataInvalidator{
 		ContractUpdates: contractUpdates,
 		MeterParamOverridesUpdated: meterParamOverridesUpdated(
-			serviceAddress,
-			executionSnapshot),
+			executionSnapshot,
+			meterStateRead),
 	}
 }
 
+// meterParamOverridesUpdated returns true if the meter param overrides have been updated
+// this is done by checking if the registers needed to compute the meter param overrides
+// have been touched in the execution snapshot
 func meterParamOverridesUpdated(
-	serviceAddress flow.Address,
 	executionSnapshot *snapshot.ExecutionSnapshot,
+	meterStateRead *snapshot.ExecutionSnapshot,
 ) bool {
-	serviceAccount := string(serviceAddress.Bytes())
-	storageDomain := common.PathDomainStorage.Identifier()
-
-	for registerId := range executionSnapshot.WriteSet {
-		// The meter param override values are stored in the service account.
-		if registerId.Owner != serviceAccount {
-			continue
+	if len(executionSnapshot.WriteSet) > len(meterStateRead.ReadSet) {
+		for registerId := range meterStateRead.ReadSet {
+			_, ok := executionSnapshot.WriteSet[registerId]
+			if ok {
+				return true
+			}
 		}
-
-		// NOTE: This condition is empirically generated by running the
-		// MeterParamOverridesComputer to capture touched registers.
-		//
-		// The paramater settings are stored as regular fields in the service
-		// account.  In general, each account's regular fields are stored in
-		// ordered map known only to cadence.  Cadence encodes this map into
-		// bytes and split the bytes into slab chunks before storing the slabs
-		// into the ledger.  Hence any changes to the stabs indicate changes
-		// the ordered map.
-		//
-		// The meter param overrides use storageDomain as input, so any
-		// changes to it must also invalidate the values.
-		if registerId.Key == storageDomain || registerId.IsSlabIndex() {
-			return true
+	} else {
+		for registerId := range executionSnapshot.WriteSet {
+			_, ok := meterStateRead.ReadSet[registerId]
+			if ok {
+				return true
+			}
 		}
 	}
 
@@ -95,9 +87,9 @@ func (invalidator ProgramInvalidator) ShouldInvalidateEntries() bool {
 }
 
 func (invalidator ProgramInvalidator) ShouldInvalidateEntry(
-	location common.AddressLocation,
+	_ common.AddressLocation,
 	program *derived.Program,
-	snapshot *snapshot.ExecutionSnapshot,
+	_ *snapshot.ExecutionSnapshot,
 ) bool {
 	if invalidator.MeterParamOverridesUpdated {
 		// if meter parameters changed we need to invalidate all programs

fvm/environment/derived_data_invalidator_test.go

@@ -283,6 +283,12 @@ func TestMeterParamOverridesUpdated(t *testing.T) {
 
 	ctx.TxBody = &flow.TransactionBody{}
 
+	meterStateRead := &snapshot.ExecutionSnapshot{
+		ReadSet: map[flow.RegisterID]struct{}{
+			flow.NewRegisterID(ctx.Chain.ServiceAddress(), "meter"): {},
+		},
+	}
+
 	checkForUpdates := func(id flow.RegisterID, expected bool) {
 		snapshot := &snapshot.ExecutionSnapshot{
 			WriteSet: map[flow.RegisterID]flow.RegisterValue{
@@ -292,8 +298,8 @@ func TestMeterParamOverridesUpdated(t *testing.T) {
 
 		invalidator := environment.NewDerivedDataInvalidator(
 			environment.ContractUpdates{},
-			ctx.Chain.ServiceAddress(),
-			snapshot)
+			snapshot,
+			meterStateRead)
 		require.Equal(t, expected, invalidator.MeterParamOverridesUpdated)
 	}
 
@@ -304,17 +310,14 @@ func TestMeterParamOverridesUpdated(t *testing.T) {
 	otherOwner := unittest.RandomAddressFixtureForChain(ctx.Chain.ChainID())
 
 	for _, registerId := range executionSnapshot.AllRegisterIDs() {
-		checkForUpdates(registerId, true)
+		checkForUpdates(registerId, false)
 		checkForUpdates(
 			flow.NewRegisterID(otherOwner, registerId.Key),
 			false)
 	}
 
-	stabIndexKey := flow.NewRegisterID(owner, "$12345678")
-	require.True(t, stabIndexKey.IsSlabIndex())
-
-	checkForUpdates(stabIndexKey, true)
-	checkForUpdates(flow.NewRegisterID(owner, "other keys"), false)
-	checkForUpdates(flow.NewRegisterID(otherOwner, stabIndexKey.Key), false)
-	checkForUpdates(flow.NewRegisterID(otherOwner, "other key"), false)
+	checkForUpdates(flow.NewRegisterID(owner, "meter2"), false)
+	checkForUpdates(flow.NewRegisterID(owner, "meter"), true)
+	checkForUpdates(flow.NewRegisterID(otherOwner, "meter2"), false)
+	checkForUpdates(flow.NewRegisterID(otherOwner, "meter"), false)
 }

fvm/environment/value_store.go

@@ -1,6 +1,7 @@
 package environment
 
 import (
+	"bytes"
 	"fmt"
 
 	"github.com/onflow/atree"
@@ -140,7 +141,6 @@ func (store *valueStore) GetValue(
 	return v, nil
 }
 
-// TODO disable SetValue for scripts, right now the view changes are discarded
 func (store *valueStore) SetValue(
 	owner []byte,
 	keyBytes []byte,
@@ -153,7 +153,16 @@ func (store *valueStore) SetValue(
 		return errors.NewInvalidInternalStateAccessError(id, "modify")
 	}
 
-	err := store.meter.MeterComputation(
+	oldValue, err := store.accounts.GetValue(id)
+	if err != nil {
+		return fmt.Errorf("get value failed: %w", err)
+	}
+	// no-op write
+	if bytes.Equal(oldValue, value) {
+		return nil
+	}
+
+	err = store.meter.MeterComputation(
 		ComputationKindSetValue,
 		uint(len(value)))
 	if err != nil {

fvm/executionParameters.go

@@ -5,6 +5,8 @@ import (
 	"fmt"
 	"math"
 
+	"github.com/onflow/flow-go/fvm/storage/snapshot"
+
 	"github.com/onflow/cadence"
 	"github.com/onflow/cadence/runtime/common"
 
@@ -58,15 +60,16 @@ func getBodyMeterParameters(
 	txnState storage.TransactionPreparer,
 ) (
 	meter.MeterParameters,
+	*snapshot.ExecutionSnapshot,
 	error,
 ) {
 	procParams := getBasicMeterParameters(ctx, proc)
 
-	overrides, err := txnState.GetMeterParamOverrides(
+	overrides, meterStateRead, err := txnState.GetMeterParamOverrides(
 		txnState,
 		NewMeterParamOverridesComputer(ctx, txnState))
 	if err != nil {
-		return procParams, err
+		return procParams, nil, err
 	}
 
 	if overrides.ComputationWeights != nil {
@@ -89,7 +92,7 @@ func getBodyMeterParameters(
 			WithStorageInteractionLimit(math.MaxUint64)
 	}
 
-	return procParams, nil
+	return procParams, meterStateRead, nil
 }
 
 type MeterParamOverridesComputer struct {

fvm/script.go

@@ -173,7 +173,7 @@ func (executor *scriptExecutor) Execute() error {
 }
 
 func (executor *scriptExecutor) execute() error {
-	meterParams, err := getBodyMeterParameters(
+	meterParams, _, err := getBodyMeterParameters(
 		executor.ctx,
 		executor.proc,
 		executor.txnState)

fvm/storage/derived/derived_block_data.go

@@ -3,6 +3,8 @@ package derived
 import (
 	"fmt"
 
+	"github.com/onflow/flow-go/fvm/storage/snapshot"
+
 	"github.com/onflow/cadence/runtime/common"
 	"github.com/onflow/cadence/runtime/interpreter"
 
@@ -26,6 +28,7 @@ type DerivedTransactionPreparer interface {
 		getMeterParamOverrides ValueComputer[struct{}, MeterParamOverrides],
 	) (
 		MeterParamOverrides,
+		*snapshot.ExecutionSnapshot,
 		error,
 	)
 
@@ -184,9 +187,10 @@ func (transaction *DerivedTransactionData) GetMeterParamOverrides(
 	getMeterParamOverrides ValueComputer[struct{}, MeterParamOverrides],
 ) (
 	MeterParamOverrides,
+	*snapshot.ExecutionSnapshot,
 	error,
 ) {
-	return transaction.meterParamOverrides.GetOrCompute(
+	return transaction.meterParamOverrides.GetWithStateOrCompute(
 		txnState,
 		struct{}{},
 		getMeterParamOverrides)

fvm/storage/derived/table.go

@@ -466,24 +466,45 @@ func (txn *TableTransaction[TKey, TVal]) GetOrCompute(
 ) (
 	TVal,
 	error,
+) {
+	val, _, err := txn.GetWithStateOrCompute(txnState, key, computer)
+	return val, err
+}
+
+// GetWithStateOrCompute returns the key's value and the execution snapshot used to
+// compute it. If a pre-computed value is available,
+// then the pre-computed value is returned and the cached state is replayed on
+// txnState.  Otherwise, the value is computed using valFunc; both the value
+// and the states used to compute the value are captured.
+//
+// Note: valFunc must be an idempotent function and it must not modify
+// txnState's values.
+func (txn *TableTransaction[TKey, TVal]) GetWithStateOrCompute(
+	txnState state.NestedTransactionPreparer,
+	key TKey,
+	computer ValueComputer[TKey, TVal],
+) (
+	TVal,
+	*snapshot.ExecutionSnapshot,
+	error,
 ) {
 	var defaultVal TVal
 
 	val, state, ok := txn.get(key)
 	if ok {
 		err := txnState.AttachAndCommitNestedTransaction(state)
 		if err != nil {
-			return defaultVal, fmt.Errorf(
+			return defaultVal, nil, fmt.Errorf(
 				"failed to replay cached state: %w",
 				err)
 		}
 
-		return val, nil
+		return val, state, nil
 	}
 
 	nestedTxId, err := txnState.BeginNestedTransaction()
 	if err != nil {
-		return defaultVal, fmt.Errorf("failed to start nested txn: %w", err)
+		return defaultVal, nil, fmt.Errorf("failed to start nested txn: %w", err)
 	}
 
 	val, err = computer.Compute(txnState, key)
@@ -497,12 +518,12 @@ func (txn *TableTransaction[TKey, TVal]) GetOrCompute(
 	}
 
 	if err != nil {
-		return defaultVal, fmt.Errorf("failed to derive value: %w", err)
+		return defaultVal, nil, fmt.Errorf("failed to derive value: %w", err)
 	}
 
 	txn.set(key, val, committedState)
 
-	return val, nil
+	return val, committedState, nil
 }
 
 func (txn *TableTransaction[TKey, TVal]) AddInvalidator(

fvm/transactionInvoker.go

@@ -65,6 +65,11 @@ type transactionExecutor struct {
 	startedTransactionBodyExecution bool
 	nestedTxnId                     state.NestedTransactionId
 
+	// the state reads needed to compute the metering parameters
+	// this is used to invalidate the metering parameters if a transaction
+	// writes to any of those registers
+	meterStateRead *snapshot.ExecutionSnapshot
+
 	cadenceRuntime  *reusableRuntime.ReusableCadenceRuntime
 	txnBodyExecutor runtime.Executor
 
@@ -195,14 +200,24 @@ func (executor *transactionExecutor) preprocessTransactionBody() error {
 		}
 	}
 
-	meterParams, err := getBodyMeterParameters(
+	// get meter parameters
+	meterParams, meterStateRead, err := getBodyMeterParameters(
 		executor.ctx,
 		executor.proc,
 		executor.txnState)
 	if err != nil {
-		return fmt.Errorf("error gettng meter parameters: %w", err)
+		return fmt.Errorf("error getting meter parameters: %w", err)
+	}
+
+	if len(meterStateRead.WriteSet) != 0 {
+		// this should never happen
+		// and indicates an implementation error
+		panic("getting metering parameters should not write to registers")
 	}
 
+	// we need to save the meter state read for invalidation purposes
+	executor.meterStateRead = meterStateRead
+
 	txnId, err := executor.txnState.BeginNestedTransactionWithMeterParams(
 		meterParams)
 	if err != nil {
@@ -387,8 +402,8 @@ func (executor *transactionExecutor) normalExecution() (
 
 	invalidator = environment.NewDerivedDataInvalidator(
 		contractUpdates,
-		executor.ctx.Chain.ServiceAddress(),
-		bodySnapshot)
+		bodySnapshot,
+		executor.meterStateRead)
 
 	// Check if all account storage limits are ok
 	//