From f4f8a962c753202f1d3ed1ce8bea4d0e05298067 Mon Sep 17 00:00:00 2001 From: Janez Podhostnik Date: Mon, 4 Mar 2024 16:30:13 +0100 Subject: [PATCH 1/2] Include account storage check into metering --- fvm/fvm_test.go | 11 ++++++----- fvm/transactionInvoker.go | 17 ++++++----------- 2 files changed, 12 insertions(+), 16 deletions(-) diff --git a/fvm/fvm_test.go b/fvm/fvm_test.go index ceba4ed35bf..030d8765f5f 100644 --- a/fvm/fvm_test.go +++ b/fvm/fvm_test.go @@ -1404,7 +1404,8 @@ func TestSettingExecutionWeights(t *testing.T) { ).run( func(t *testing.T, vm fvm.VM, chain flow.Chain, ctx fvm.Context, snapshotTree snapshot.SnapshotTree) { // Use the maximum amount of computation so that the transaction still passes. - loops := uint64(997) + loops := uint64(996) + executionEffortNeededToCheckStorage := uint64(1) maxExecutionEffort := uint64(997) txBody := flow.NewTransactionBody(). SetScript([]byte(fmt.Sprintf(` @@ -1427,8 +1428,8 @@ func TestSettingExecutionWeights(t *testing.T) { snapshotTree = snapshotTree.Append(executionSnapshot) - // expected used is number of loops. - require.Equal(t, loops, output.ComputationUsed) + // expected computation used is number of loops + 1 (from the storage limit check). + require.Equal(t, loops+executionEffortNeededToCheckStorage, output.ComputationUsed) // increasing the number of loops should fail the transaction. loops = loops + 1 @@ -1451,8 +1452,8 @@ func TestSettingExecutionWeights(t *testing.T) { require.NoError(t, err) require.ErrorContains(t, output.Err, "computation exceeds limit (997)") - // computation used should the actual computation used. - require.Equal(t, loops, output.ComputationUsed) + // expected computation used is still number of loops + 1 (from the storage limit check). + require.Equal(t, loops+executionEffortNeededToCheckStorage, output.ComputationUsed) for _, event := range output.Events { // the fee deduction event should only contain the max gas worth of execution effort. diff --git a/fvm/transactionInvoker.go b/fvm/transactionInvoker.go index 57c0c449cbf..85d7375a0d3 100644 --- a/fvm/transactionInvoker.go +++ b/fvm/transactionInvoker.go @@ -397,21 +397,16 @@ func (executor *transactionExecutor) normalExecution() ( // Check if all account storage limits are ok // - // disable the computation/memory limit checks on storage checks, - // so we don't error from computation/memory limits on this part. - // // The storage limit check is performed for all accounts that were touched during the transaction. // The storage capacity of an account depends on its balance and should be higher than the accounts storage used. // The payer account is special cased in this check and its balance is considered max_fees lower than its // actual balance, for the purpose of calculating storage capacity, because the payer will have to pay for this tx. - executor.txnState.RunWithAllLimitsDisabled(func() { - err = executor.CheckStorageLimits( - executor.ctx, - executor.env, - bodySnapshot, - executor.proc.Transaction.Payer, - maxTxFees) - }) + err = executor.CheckStorageLimits( + executor.ctx, + executor.env, + bodySnapshot, + executor.proc.Transaction.Payer, + maxTxFees) if err != nil { return From 438bdbc32839ede0f5a34b1360b6bb4660e2f9cd Mon Sep 17 00:00:00 2001 From: Janez Podhostnik Date: Mon, 4 Mar 2024 18:09:52 +0100 Subject: [PATCH 2/2] add test --- fvm/fvm_test.go | 111 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 111 insertions(+) diff --git a/fvm/fvm_test.go b/fvm/fvm_test.go index 030d8765f5f..92245acf949 100644 --- a/fvm/fvm_test.go +++ b/fvm/fvm_test.go @@ -1478,6 +1478,117 @@ func TestSettingExecutionWeights(t *testing.T) { unittest.EnsureEventsIndexSeq(t, output.Events, chain.ChainID()) }, )) + + t.Run("transaction with more accounts touched uses more computation", newVMTest().withBootstrapProcedureOptions( + fvm.WithMinimumStorageReservation(fvm.DefaultMinimumStorageReservation), + fvm.WithAccountCreationFee(fvm.DefaultAccountCreationFee), + fvm.WithStorageMBPerFLOW(fvm.DefaultStorageMBPerFLOW), + fvm.WithTransactionFee(fvm.DefaultTransactionFees), + fvm.WithExecutionEffortWeights( + meter.ExecutionEffortWeights{ + common.ComputationKindStatement: 0, + // only count loops + // the storage check has a loop + common.ComputationKindLoop: 1 << meter.MeterExecutionInternalPrecisionBytes, + common.ComputationKindFunctionInvocation: 0, + }, + ), + ).withContextOptions( + fvm.WithAccountStorageLimit(true), + fvm.WithTransactionFeesEnabled(true), + fvm.WithMemoryLimit(math.MaxUint64), + ).run( + func(t *testing.T, vm fvm.VM, chain flow.Chain, ctx fvm.Context, snapshotTree snapshot.SnapshotTree) { + // Create an account private key. + privateKeys, err := testutil.GenerateAccountPrivateKeys(5) + require.NoError(t, err) + + // Bootstrap a ledger, creating accounts with the provided + // private keys and the root account. + snapshotTree, accounts, err := testutil.CreateAccounts( + vm, + snapshotTree, + privateKeys, + chain) + require.NoError(t, err) + + sc := systemcontracts.SystemContractsForChain(chain.ChainID()) + + // create a transaction without loops so only the looping in the storage check is counted. + txBody := flow.NewTransactionBody(). + SetScript([]byte(fmt.Sprintf(` + import FungibleToken from 0x%s + import FlowToken from 0x%s + + transaction() { + let sentVault: @FungibleToken.Vault + + prepare(signer: AuthAccount) { + let vaultRef = signer.borrow<&FlowToken.Vault>(from: /storage/flowTokenVault) + ?? panic("Could not borrow reference to the owner's Vault!") + + self.sentVault <- vaultRef.withdraw(amount: 5.0) + } + + execute { + let recipient1 = getAccount(%s) + let recipient2 = getAccount(%s) + let recipient3 = getAccount(%s) + let recipient4 = getAccount(%s) + let recipient5 = getAccount(%s) + + let receiverRef1 = recipient1.getCapability(/public/flowTokenReceiver) + .borrow<&{FungibleToken.Receiver}>() + ?? panic("Could not borrow receiver reference to the recipient's Vault") + let receiverRef2 = recipient2.getCapability(/public/flowTokenReceiver) + .borrow<&{FungibleToken.Receiver}>() + ?? panic("Could not borrow receiver reference to the recipient's Vault") + let receiverRef3 = recipient3.getCapability(/public/flowTokenReceiver) + .borrow<&{FungibleToken.Receiver}>() + ?? panic("Could not borrow receiver reference to the recipient's Vault") + let receiverRef4 = recipient4.getCapability(/public/flowTokenReceiver) + .borrow<&{FungibleToken.Receiver}>() + ?? panic("Could not borrow receiver reference to the recipient's Vault") + let receiverRef5 = recipient5.getCapability(/public/flowTokenReceiver) + .borrow<&{FungibleToken.Receiver}>() + ?? panic("Could not borrow receiver reference to the recipient's Vault") + + receiverRef1.deposit(from: <-self.sentVault.withdraw(amount: 1.0)) + receiverRef2.deposit(from: <-self.sentVault.withdraw(amount: 1.0)) + receiverRef3.deposit(from: <-self.sentVault.withdraw(amount: 1.0)) + receiverRef4.deposit(from: <-self.sentVault.withdraw(amount: 1.0)) + receiverRef5.deposit(from: <-self.sentVault.withdraw(amount: 1.0)) + + destroy self.sentVault + } + }`, + sc.FungibleToken.Address, + sc.FlowToken.Address, + accounts[0].HexWithPrefix(), + accounts[1].HexWithPrefix(), + accounts[2].HexWithPrefix(), + accounts[3].HexWithPrefix(), + accounts[4].HexWithPrefix(), + ))). + SetProposalKey(chain.ServiceAddress(), 0, 0). + AddAuthorizer(chain.ServiceAddress()). + SetPayer(chain.ServiceAddress()) + + err = testutil.SignTransactionAsServiceAccount(txBody, 0, chain) + require.NoError(t, err) + + _, output, err := vm.Run( + ctx, + fvm.Transaction(txBody, 0), + snapshotTree) + require.NoError(t, err) + require.NoError(t, output.Err) + + // The storage check should loop once for each of the five accounts created + + // once for the service account + require.Equal(t, uint64(5+1), output.ComputationUsed) + }, + )) } func TestStorageUsed(t *testing.T) {