Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FVM] Short Circuit Transactions with insufficient balance #2767

Closed
1 task
janezpodhostnik opened this issue Jul 8, 2022 · 0 comments
Closed
1 task

[FVM] Short Circuit Transactions with insufficient balance #2767

janezpodhostnik opened this issue Jul 8, 2022 · 0 comments
Assignees
Labels
Execution Cadence Execution Team Performance

Comments

@janezpodhostnik
Copy link
Contributor

janezpodhostnik commented Jul 8, 2022

Context

Fee deduction currently happens as the last step of the transaction execution. This means that the users transaction is fully executed before failing with insufficient balance on the payers account. This means it is possible to send computationally heavy transactions without having the funds to cover them.

This however also allows the use case where the payer does not have enough funds to pay for the transaction, but withdraws those funds from a different source (withdraw staking rewards) during the transaction, so by the end of the transaction its balance is OK.

Solution

Main point: Transactions where the payer has insufficient balance should fail earlier.

Define "earlier"

Ideally the payers balance would be checked even before the transaction gets included in a collection, so that it doesn't cost any resources on the execution/verification side. This is the next step and not in the scope of this issue.

Even if the transaction's payer's balance is checked before hitting the execution node, the execution node still has to re-check each transaction.

The current transaction execution steps:

  • check all signatures
  • check sequence number
  • regular path:
    • invoke users transactions
    • deduct transaction fees
    • check storage
  • failure path (in case of error during the regular path):
    • deduct transaction fees

The transaction execution steps would Ideally look like this:

  • Phase 1.
    • check payers signature
    • check sequence number
  • Phase 2.
    • regular path
      • check payers balance
      • check other signatures
      • invoke users transactions
      • deduct transaction fees
      • check storage
    • failure path (in case of error during the regular path):
      • deduct transaction fees

A failure in the Phase 1. would mean we cannot charge the payer at all, because this isn't a validly signed transaction and someone might be impersonating the payer or re-sending old transactions in order to drain the payers balance.

A failure in the regular path of Phase 2. means fees will be deducted from the payer even if the payer is below the minimum account balance. This might lead to the payer eventually dropping to 0 balance if the payer keeps sending transactions.

For this issue just the first building block will be added which is the check payers balance step, and the transaction execution steps will look like this:

  • check all signatures
  • check sequence number
  • regular path:
    • check payers balance
    • invoke users transactions
    • deduct transaction fees
    • check storage
  • failure path (in case of error during the regular path):
    • deduct transaction fees

Define "insufficient balance"

Insufficient balance means the payers balance is less then:

  • Minimum account balance (MAB)
  • PLUS transaction inclusion fees
  • PLUS maximum transaction execution fees (in respect to the max execution effort (gas) defined by the transaction itself)

Alternatives considered:

  • balance is less than inclusion fees => insufficient condition, execution costs could potentially not be charged
  • balance is less than inclusion fees + maximum transaction execution fees => if this is lower then the MAB the transaction will fail anyway
  • MAB => fees will always be > 0 so transaction will fail if the payers balance is MAB before the transaction

Related changes

There is another connected issues this might tie into. Currently the regular path fee deduction happens before the storage check. The reason why we currently cannot switch to this order is that the payers storage capacity depends on the payers balance. so fees must be deducted first. This also means that we cannot meter and charge fees for the storage check, which is something the user has control over, since touching more account means the storage check runs for longer.

However if we change the check for storage so that we can tell it that the payers balance is actually lower by the fee amount, the ordering can be switched.If we then also say that the payers balance is actually lower by the maximum fee amount during the storage check, then we can meter and charge fees for the storage check.

Definition of Done

  • Transactions where payer cannot pay, fail before the transaction body is invoked
@janezpodhostnik janezpodhostnik added the Execution Cadence Execution Team label Jul 8, 2022
@janezpodhostnik janezpodhostnik self-assigned this Jul 8, 2022
@janezpodhostnik janezpodhostnik changed the title [FVM] Short Circuit Transactions with no coverage [FVM] Short Circuit Transactions with with insufficient balance Aug 10, 2022
@janezpodhostnik janezpodhostnik changed the title [FVM] Short Circuit Transactions with with insufficient balance [FVM] Short Circuit Transactions with insufficient balance Aug 10, 2022
bors bot added a commit that referenced this issue Oct 31, 2022
3471: [FVM] Move inclusion effort to transaction r=janezpodhostnik a=janezpodhostnik

prerequisite for: #2767

Co-authored-by: Janez Podhostnik <[email protected]>
bors bot added a commit that referenced this issue Oct 31, 2022
3472: [FVM] Use empty programs during bootstrapping r=janezpodhostnik a=janezpodhostnik

prerequisite for: #2767

It doesn't make sense to use the programs cache during bootstrapping.
Avery transaction is a new deploy, and the speed isn't needed.

3474: Disable limits for all of tx seq num checker / tx verifier r=pattyshack a=pattyshack



Co-authored-by: Janez Podhostnik <[email protected]>
Co-authored-by: Patrick Lee <[email protected]>
bors bot added a commit that referenced this issue Dec 7, 2022
3473: [FVM] Fail fast when payer cannot pay r=janezpodhostnik a=janezpodhostnik

This is the first part of #2767

Payer must have sufficient balance for the transaction execution to happen.
The required balance is defined by the smart contract.


related to: onflow/flow-emulator#231 and onflow/flow-core-contracts#318

Co-authored-by: Janez Podhostnik <[email protected]>
bors bot added a commit that referenced this issue Dec 9, 2022
3634: [FVM] Check account storage before deducting fees r=janezpodhostnik a=janezpodhostnik

ref: #2767

uses: onflow/flow-core-contracts#323

Invert payer check  and fee deduction. This is possible by assuming that during the storage check the balance of the payer, is `maxFees` lower then it actually is.

The next step (PR) is metering the storage check and including it into the fees.

Co-authored-by: Janez Podhostnik <[email protected]>
bors bot added a commit that referenced this issue Dec 9, 2022
3621: Optimize state migration r=zhangchiqing a=zhangchiqing

For #3614

This PR implemented a template for running state migration concurrently based on account.

It first group all payloads by account, and then migrate the payloads of different account concurrently.

The example migration reads the account storage usage and update the register in account status with new storage usage.

Since after migration, the storage usage is mostly likely changed, this example migration could be reused in the real migration.

3634: [FVM] Check account storage before deducting fees r=SaveTheRbtz a=janezpodhostnik

ref: #2767

uses: onflow/flow-core-contracts#323

Invert payer check  and fee deduction. This is possible by assuming that during the storage check the balance of the payer, is `maxFees` lower then it actually is.

The next step (PR) is metering the storage check and including it into the fees.

Co-authored-by: Leo Zhang (zhangchiqing) <[email protected]>
Co-authored-by: Janez Podhostnik <[email protected]>
bors bot added a commit that referenced this issue Dec 9, 2022
3634: [FVM] Check account storage before deducting fees r=SaveTheRbtz a=janezpodhostnik

ref: #2767

uses: onflow/flow-core-contracts#323

Invert payer check  and fee deduction. This is possible by assuming that during the storage check the balance of the payer, is `maxFees` lower then it actually is.

The next step (PR) is metering the storage check and including it into the fees.

Co-authored-by: Janez Podhostnik <[email protected]>
bors bot added a commit that referenced this issue Dec 10, 2022
3634: [FVM] Check account storage before deducting fees r=SaveTheRbtz a=janezpodhostnik

ref: #2767

uses: onflow/flow-core-contracts#323

Invert payer check  and fee deduction. This is possible by assuming that during the storage check the balance of the payer, is `maxFees` lower then it actually is.

The next step (PR) is metering the storage check and including it into the fees.

Co-authored-by: Janez Podhostnik <[email protected]>
bors bot added a commit that referenced this issue Dec 12, 2022
3634: [FVM] Check account storage before deducting fees r=janezpodhostnik a=janezpodhostnik

ref: #2767

uses: onflow/flow-core-contracts#323

Invert payer check  and fee deduction. This is possible by assuming that during the storage check the balance of the payer, is `maxFees` lower then it actually is.

The next step (PR) is metering the storage check and including it into the fees.

Co-authored-by: Janez Podhostnik <[email protected]>
bors bot added a commit that referenced this issue Dec 13, 2022
3634: [FVM] Check account storage before deducting fees r=janezpodhostnik a=janezpodhostnik

ref: #2767

uses: onflow/flow-core-contracts#323

Invert payer check  and fee deduction. This is possible by assuming that during the storage check the balance of the payer, is `maxFees` lower then it actually is.

The next step (PR) is metering the storage check and including it into the fees.

Co-authored-by: Janez Podhostnik <[email protected]>
bors bot added a commit that referenced this issue Dec 20, 2022
3726: Don't meter the payer balance check r=janezpodhostnik a=janezpodhostnik

ref: #2767

I realised that checking the payers account balance should not be metered because of 2 reasons:

1. It should be part of inclusion fees, not execution fees. All transactions need to have this check, and the check is of a constant complexity.
2. Adding additional computation cost to transactions will potentially break any daps that have fine tuned their transaction gas limits. Which we will eventually have to do, but that needs a longer communication change, and we can batch more changes like that together.

Co-authored-by: Janez Podhostnik <[email protected]>
@j1010001 j1010001 closed this as completed Jan 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Execution Cadence Execution Team Performance
Projects
None yet
Development

No branches or pull requests

2 participants