From f6ba7c96d9161628538c65577b32a5f9776edb05 Mon Sep 17 00:00:00 2001 From: Janez Podhostnik Date: Tue, 20 Aug 2024 17:20:17 +0200 Subject: [PATCH 1/3] Fix metering invalidation --- fvm/environment/derived_data_invalidator.go | 50 ++++++++----------- .../derived_data_invalidator_test.go | 23 +++++---- fvm/executionParameters.go | 9 ++-- fvm/script.go | 2 +- fvm/storage/derived/derived_block_data.go | 6 ++- fvm/storage/derived/table.go | 31 ++++++++++-- fvm/transactionInvoker.go | 23 +++++++-- 7 files changed, 91 insertions(+), 53 deletions(-) diff --git a/fvm/environment/derived_data_invalidator.go b/fvm/environment/derived_data_invalidator.go index 5aa4bf05808..8427ffe2f76 100644 --- a/fvm/environment/derived_data_invalidator.go +++ b/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 diff --git a/fvm/environment/derived_data_invalidator_test.go b/fvm/environment/derived_data_invalidator_test.go index 026f0dd119a..50ce6dca4e3 100644 --- a/fvm/environment/derived_data_invalidator_test.go +++ b/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) } diff --git a/fvm/executionParameters.go b/fvm/executionParameters.go index 0173210545b..454440e406d 100644 --- a/fvm/executionParameters.go +++ b/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 { diff --git a/fvm/script.go b/fvm/script.go index 1b5d247b640..01e8ea15044 100644 --- a/fvm/script.go +++ b/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) diff --git a/fvm/storage/derived/derived_block_data.go b/fvm/storage/derived/derived_block_data.go index 69a8b0990d0..450d09cd9d5 100644 --- a/fvm/storage/derived/derived_block_data.go +++ b/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) diff --git a/fvm/storage/derived/table.go b/fvm/storage/derived/table.go index 6698f7a6db1..2c9a70631ea 100644 --- a/fvm/storage/derived/table.go +++ b/fvm/storage/derived/table.go @@ -466,6 +466,27 @@ 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 @@ -473,17 +494,17 @@ func (txn *TableTransaction[TKey, TVal]) GetOrCompute( 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( diff --git a/fvm/transactionInvoker.go b/fvm/transactionInvoker.go index 20a68727c81..ef3d34b658b 100644 --- a/fvm/transactionInvoker.go +++ b/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 // From 24ea53d13d3fde7bd3a8107ba2a72c5bdc0ee5f6 Mon Sep 17 00:00:00 2001 From: Janez Podhostnik Date: Fri, 23 Aug 2024 12:50:16 +0200 Subject: [PATCH 2/3] prevent no-op writes --- fvm/environment/value_store.go | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/fvm/environment/value_store.go b/fvm/environment/value_store.go index 27fa80c6385..39ece06bcb7 100644 --- a/fvm/environment/value_store.go +++ b/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.GetValue(owner, keyBytes) + 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 { From b5da6631b04c0e8cd224dabf7c4561a5c7eec900 Mon Sep 17 00:00:00 2001 From: Janez Podhostnik Date: Thu, 12 Sep 2024 21:01:45 +0200 Subject: [PATCH 3/3] change pre write read to not meter --- fvm/environment/value_store.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fvm/environment/value_store.go b/fvm/environment/value_store.go index 39ece06bcb7..441f23f9f96 100644 --- a/fvm/environment/value_store.go +++ b/fvm/environment/value_store.go @@ -153,7 +153,7 @@ func (store *valueStore) SetValue( return errors.NewInvalidInternalStateAccessError(id, "modify") } - oldValue, err := store.GetValue(owner, keyBytes) + oldValue, err := store.accounts.GetValue(id) if err != nil { return fmt.Errorf("get value failed: %w", err) }