flow-go icon indicating copy to clipboard operation
flow-go copied to clipboard

[Access] Experiment with validating a transaction payer can pay for tx before submission

Open peterargue opened this issue 1 year ago • 2 comments

Problem Definition

See https://github.com/onflow/flow-go/issues/5683 for details.

Sometimes dapps and scripted clients will send transactions using a payer account that does not have enough FLOW balance to pay for the transaction fee. In these cases, the tx will not succeed and nodes on the network spend resources handling the tx.

Now that Access nodes have data about account balances, they can be configured to screen for these tx and return a useful error to the client about why their tx is rejected.

There are limitations to how well this would protect the network from an actual malicious actor, but there is still some value protecting from the more common unintentional spam.

Proposed Solution

Add a check to the transaction validation done during SendTransaction, that ensures the payer has sufficient balance to pay for the transaction.

Access nodes currently validate a transaction here: https://github.com/onflow/flow-go/blob/6f0e33aa35f007f6b03447e359ea1c0c52780ff6/engine/access/rpc/backend/backend_transactions.go#L59

which calls this method: https://github.com/onflow/flow-go/blob/c4fe1c6bfb93aacadeaa5d5f52ec34e32bf4ac80/access/validator.go#L112

Follow the approach execution nodes use to verify a transaction payer's balance https://github.com/onflow/flow-go/blob/c56c2169ee1c9c66f71db67dc06f184aa93f82d6/fvm/transactionPayerBalanceChecker.go#L78-L116

I think a systemContracts object can be setup something like this (but double check how these params are set for ANs and ENs to see if we need something different):

	tracer := tracing.NewMockTracerSpan()
	sc := environment.NewSystemContracts(
		chain,
		tracer,
		environment.NewProgramLogger(tracer, environment.DefaultProgramLoggerParams()),
		environment.NewRuntime(environment.DefaultRuntimeParams()),
	)

then, the script execution call within the validator would look like

	resultValue, err := sc.CheckPayerBalanceAndGetMaxTxFees(
		tx.Payer,
		tx.InclusionEffort(),
		tx.GasLimit,
	)

Note:

  • the execution.ScriptExecutor should be configured in "local only" mode. We do not want scripts to failover to an execution node
  • If the lookup fails, we should fail open and allow the transaction
  • we can cache the balance lookups where balances are over some threshold. this will be important since payers are often wallets, so the majority of submitted tx will have payers with relatively larger balances.

Definition of Done

  • Access nodes can enable a feature to validate the transaction payers have sufficient balance to submit the tx
  • The feature is off by default, and configurable with a CLI flag when script execution is enabled
  • There are unit and integration tests covering the new behavior

peterargue avatar May 01 '24 00:05 peterargue

we can cache the balance lookups where balances are over some threshold. this will be important since payers are often wallets, so the majority of submitted tx will have payers with relatively larger balances.

I think caching will be hard because there is no good way to do invalidation.

If payer A drops below minimum account balance and that stat is cached A will be unable to send transactions until the cache expires, even if A's account was already fixed somehow.

We could do caching if access nodes have access to all register changes (i'm not sure if they do or not). In that case we could invalidate the cached balance of an account if any of the accounts registers change (we don't know which register holds the vault)

janezpodhostnik avatar Jun 05 '24 15:06 janezpodhostnik

we can cache the balance lookups where balances are over some threshold. this will be important since payers are often wallets, so the majority of submitted tx will have payers with relatively larger balances.

I think caching will be hard because there is no good way to do invalidation.

If payer A drops below minimum account balance and that stat is cached A will be unable to send transactions until the cache expires, even if A's account was already fixed somehow.

that's true. I was thinking more that we'd cache lookups for accounts with >N flow balance to avoid this situation. Then the worst case is we let spammy tx through for a minute or so until the entry expires. but if it was set to something reasonable like <60 second, it would still have pretty minimal impact even if it cached low balances.

We could do caching if access nodes have access to all register changes (i'm not sure if they do or not). In that case we could invalidate the cached balance of an account if any of the accounts registers change (we don't know which register holds the vault)

They do have this data and already do inspect the trie updates for the program cache integration so this shouldn't be a large lift. My preference would be to keep it simple to start though.

peterargue avatar Jun 06 '24 00:06 peterargue