[Access] Add check if tx payer has sufficient amount of flow to pay fee

[illia-malachyn]

Add an invocation of system contract to check if tx payer has enough balance to pay for tx

[illia-malachyn]

According to https://github.com/onflow/flow-go/issues/5823

the execution.ScriptExecutor should be configured in "local only" mode. We do not want scripts to failover to an execution node

Do you know if we still need to check it? We don't use script executor

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 34.55882% with 89 lines in your changes missing coverage. Please review.

Project coverage is 41.52%. Comparing base (9ea6fae) to head (aeee394). Report is 16 commits behind head on master.

Files	Patch %	Lines
access/mock/blocks.go	0.00%	59 Missing :warning:
access/validator.go	68.88%	8 Missing and 6 partials :warning:
cmd/access/node_builder/access_node_builder.go	0.00%	8 Missing :warning:
access/errors.go	50.00%	3 Missing :warning:
access/utils/cadence.go	75.00%	1 Missing and 1 partial :warning:
engine/access/rpc/backend/backend.go	71.42%	1 Missing and 1 partial :warning:
engine/access/rpc/backend/backend_transactions.go	0.00%	1 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6004      +/-   ##
==========================================
+ Coverage   41.48%   41.52%   +0.03%     
==========================================
  Files        1978     1980       +2     
  Lines      140843   140986     +143     
==========================================
+ Hits        58433    58546     +113     
- Misses      76335    76337       +2     
- Partials     6075     6103      +28     

Flag	Coverage Δ
unittests	41.52% <34.55%> (+0.03%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[illia-malachyn]

@janezpodhostnik Hey. I need my account balance to go below the default value as we did in https://github.com/onflow/flow-core-contracts/pull/435. Though this time I have to do it in the integration test. I tried to set the option you suggested but it didn't work https://github.com/onflow/flow-go/pull/6004/files#diff-7540be57ca1fb2714cfce630ad0eba7bfc97362d8e30b79ac2551dc68792e75bR1209.

I also noticed that there's an account creation fee already set. So, to not play with tx fee enabling I just decided to create a new account on behalf of my custom-created account to pay tx fee. I changed tx fee value a bit but the balance is still the same. https://github.com/onflow/flow-go/pull/6004/files#diff-f414ad868524efa32a2d6a821877f9d01b7948e26bece32df25664a8eb4709d8R26

Please, check out the transaction_validator_test.go file in the PR.

What am I missing here?

P. s. When I enable tx fees in scaffold.go the test starts to hang on waitForSealed with grpc error

[janezpodhostnik]

Enabling tx fees in scaffold.go is exactly what is missing.

The integration tests were not designed to run with fees enabled. Its likely some transactions start failing when you enable tx fees in scaffold.go, that is why you start seeing errors.

Its likely modifying the integration tests to work with fees would require a significant effort (and changes). I would suggest covering the feature with unit tests as much as possible and manually testing the feature with the emulator (maybe adding a test there since the emulator can run with fees enabled).

The integration test can be added with a separate PR. The first thing to figure out is how to change the integration tests so that they can optionally run with fees enabled.

[k1nder10]

@peterargue Does the user pay for the script executed to check whether the account has enough money to pay for a transaction? The script execution happens inside each transaction, so should such a transaction cost more when enabled?

[illia-malachyn]

@peterargue I think it is ready to be reviewed. I'll do a control test tomorrow on clean master and fix your comments if any.

[illia-malachyn]

How to test it manually:

1. Open https://github.com/onflow/flow-go/blob/master/integration/localnet/builder/bootstrap.go#L443

• set --check-payer-balance=true
• reset --script-execution-mode to local-only

2. Open https://github.com/onflow/flow-go/blob/master/cmd/scaffold.go#L1533 Add flow.Localnet to switch to enable tx fees for local net.

3. Open https://github.com/onflow/flow-go/blob/master/integration/testnet/network.go#L1208 Add fvm.WithTransactionFee(fvm.DefaultTransactionFees), to the list

4. Build & start local net

cd intergration/localnet
make bootstrap
make start

5. Set up flow project flow-c1 init (press No to skip system accounts creation as they're already deployed to the service account)

6. Open the newly created folder with flow.json

7. Update flow.json according to https://github.com/onflow/flow-go/tree/master/integration/localnet#add-service-account-address-and-private-key

8. Create a new account following https://github.com/onflow/flow-go/tree/master/integration/localnet#creating-new-account-on-the-localnet

9. Add previously created account to flow.json e.g.

		"alice": {
			"address": "your_address",
			"key": {
				"type": "hex",
				"index": 0,
				"signatureAlgorithm": "ECDSA_P256",
				"hashAlgorithm": "SHA3_256",
				"privateKey": "your_private_key"
			}
		},

10. Create simple_tx.cdc transaction { execute {} }

11. Execute transaction on behalf of Alice flow-c1 -n localnet transactions send ./simple_tx.cdc --payer alice --authorizer alice --proposer alice

The transaction should fail due to the storage limit check but the fees will be taken anyway thus the account balance will become insufficient.

12. Repeat the transaction flow-c1 -n localnet transactions send ./simple_tx.cdc --payer alice --authorizer alice --proposer alice

You should see an error that Alice doesn't have sufficient balance to pay for tx.

13. Create transfer_tokens.cdc transaction

import FungibleToken from 0xee82856bf20e2aa6
import FlowToken from 0x0ae53cb6e3f42a79

transaction(amount: UFix64, to: Address) {

    // The Vault resource that holds the tokens that are being transferred
    let sentVault: @{FungibleToken.Vault}

    prepare(signer: auth(BorrowValue) &Account) {

        // Get a reference to the signer's stored vault
        let vaultRef = signer.storage.borrow<auth(FungibleToken.Withdraw) &FlowToken.Vault>(from: /storage/flowTokenVault)
			?? panic("Could not borrow reference to the owner's Vault!")

        // Withdraw tokens from the signer's stored vault
        self.sentVault <- vaultRef.withdraw(amount: amount)
    }

    execute {

        // Get a reference to the recipient's Receiver
        let receiverRef =  getAccount(to)
            .capabilities.borrow<&{FungibleToken.Receiver}>(/public/flowTokenReceiver)
			?? panic("Could not borrow receiver reference to the recipient's Vault")

        // Deposit the withdrawn tokens in the recipient's receiver
        receiverRef.deposit(from: <-self.sentVault)
    }
}

14. Send some tokens to Alice account flow-c1 -n localnet transactions send ./transfer_tokens.cdc "0.01" {ALICE_ACCOUNT_ADDRESS} --payer localnet-service-account --authorizer localnet-service-account --proposer localnet-service-account

15. Execute transaction on behalf of Alice flow-c1 -n localnet transactions send ./simple_tx.cdc --payer alice --authorizer alice --proposer alice

You should see no errors. The transaction should be executed successfully.

[illia-malachyn]

@peterargue I'm running into such behavior during the test: when you execute an empty tx on a newly created account (with default balance), the very first error you run into is a storage limit check that comes from FVM. If you execute the tx the second time (with less balance as you paid fees for the previous tx), the access node's tx validator will reject the tx as the account balance is insufficient. This is weird and I assume we want tx validator to fail on the first run too.

Access node's tx validator uses this script to see if balance is sufficient for tx. But we do fall into an execution node when executing the first transaction. I assume that FVM has more checks than FlowFees.verifyPayersBalanceForTransactionExecution, it includes a storage limit I believe. So, we need to adjust the validator to include such checks as well. Otherwise, it won't be very useful on average.

@janezpodhostnik what do you suggest doing in such a case? Is there some cadence code for me to include storage limit check in tx validator's script?

[peterargue]

@illia-malachyn to get this PR ready to merge please:

1. merge master and fix conflicts
2. remove the replace statement for the core contracts repo, and update to use v1.3.1
3. remove the replace statement for the core contracts repo in your emulator branch
4. Update this PR to point to the new latest commit in your emulator branch
5. Update the emulator branch to point to the new latest commit in this branch

[peterargue]

Looks good. please fix the conflicts again, and we merge tomorrow