Merge pull request #5497 from onflow/janez/charge-for-storage-check-2

Meter computation in the account storage check

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.
@@ -1477,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) {

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