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

[Flow EVM] update error handling #5357

Merged
merged 36 commits into from
Feb 16, 2024
Merged

[Flow EVM] update error handling #5357

merged 36 commits into from
Feb 16, 2024

Conversation

ramtinms
Copy link
Contributor

@ramtinms ramtinms commented Feb 6, 2024

closes #5225
This PR

  • refactors the handler to support the functionality needed for methods like tryRun
  • updates the logic on the tx run and direct call to not panic on VMErrors (the ones in the case of tx failed execution)
  • introduces an FVM environment wrapper providing a backend that handles the error and lets the handler know if the errors are because of the FVM environments or not.

Some context:
In the world of EVM, there are multiple possible outcomes for running a transaction

  • it fails at the validation phase (e.g. Nonce doesn't match), it rejects the transaction and doesn't include the transaction in the block
  • it passes the validation but fails during the execution (e.g. running out of gas), it includes the transaction in the block and forms the receipt with the status Failed for the transaction. [The error in this case is called VMError]
  • it passes the validation and also successes at the execution step, this tx then is also included in the block and a receipt is formed.

For us there are 4 cases:

  • EVM hits a backend or fatal error (e.g. hitting interaction limit)
    • this reverts the outer Flow transaction and returns the control to the FVM by calling panic
  • Tx hits a validation error (either the ones on the Geth code or the extra checks we have like (computation check))
    • for run, it reverts the outer Flow transaction, and for tryRun it returns the status as Invalid, we also charge some computation.
  • Tx runs and hits a execution error,
    • for both run and tryrun don't error our and we form a block and generate a receipt. For tryRun we set the status to failed. we charge the full computations that are used.
  • Tx runs successfully a block and a receipt is formed.

@codecov-commenter
Copy link

codecov-commenter commented Feb 6, 2024

Codecov Report

Attention: 234 lines in your changes are missing coverage. Please review.

Comparison is base (db3d671) 55.95% compared to head (ad735a5) 55.88%.

Files Patch % Lines
fvm/evm/handler/handler.go 56.92% 82 Missing and 33 partials ⚠️
fvm/evm/handler/codeFinder.go 21.42% 66 Missing ⚠️
fvm/evm/emulator/emulator.go 40.62% 17 Missing and 2 partials ⚠️
fvm/evm/types/errors.go 0.00% 10 Missing ⚠️
fvm/evm/types/result.go 0.00% 8 Missing ⚠️
fvm/errors/failures.go 0.00% 5 Missing ⚠️
fvm/evm/handler/blockstore.go 72.72% 2 Missing and 1 partial ⚠️
fvm/evm/types/events.go 0.00% 3 Missing ⚠️
fvm/evm/emulator/state/stateDB.go 0.00% 1 Missing and 1 partial ⚠️
fvm/evm/types/call.go 0.00% 2 Missing ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5357      +/-   ##
==========================================
- Coverage   55.95%   55.88%   -0.08%     
==========================================
  Files        1023     1024       +1     
  Lines       98930    99160     +230     
==========================================
+ Hits        55360    55413      +53     
- Misses      39364    39510     +146     
- Partials     4206     4237      +31     
Flag Coverage Δ
unittests 55.88% <45.83%> (-0.08%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ramtinms ramtinms changed the title [WIP] [Flow EVM] tryRun functionality [WIP] [Flow EVM] update error handling Feb 8, 2024
Comment on lines -36 to -40
// EVMExecutionError is a non-fatal error, returned when execution of
// an evm transaction or direct call has failed.
type EVMExecutionError struct {
err error
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was never used.

Comment on lines +76 to +81
// ResultSummary summerizes the outcome of a EVM call or tx run
type ResultSummary struct {
Status Status
ErrorCode ErrorCode
GasConsumed uint64
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what is returned for try methods.

@ramtinms ramtinms marked this pull request as ready for review February 10, 2024 00:10

"github.com/onflow/flow-go/fvm/evm/types"
"github.com/onflow/flow-go/model/flow"
)

const (
BlockHashListCapacity = 256
BlockHashListCapacity = 16
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@janezpodhostnik I reduced this to 16 for now, until the new changes are ready.

// returns true if error passes the checks
type checkError func(error) bool

var isFatal = func(err error) bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should keep the naming of failure and not mix the two otherwise it gets more confusing


var _ types.Backend = &WrappedEnvironment{}

func (we *WrappedEnvironment) GetValue(owner, key []byte) ([]byte, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these don't need to be pointer receivers.

@ramtinms ramtinms enabled auto-merge February 16, 2024 18:30
@ramtinms ramtinms added this pull request to the merge queue Feb 16, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 16, 2024
@ramtinms ramtinms added this pull request to the merge queue Feb 16, 2024
@ramtinms ramtinms removed this pull request from the merge queue due to a manual request Feb 16, 2024
@ramtinms ramtinms added this pull request to the merge queue Feb 16, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 16, 2024
@ramtinms ramtinms enabled auto-merge February 16, 2024 21:11
@ramtinms ramtinms added this pull request to the merge queue Feb 16, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 16, 2024
@ramtinms ramtinms added this pull request to the merge queue Feb 16, 2024
Merged via the queue into master with commit 8fe638a Feb 16, 2024
51 checks passed
@ramtinms ramtinms deleted the ramtin/5225-returning-error branch February 16, 2024 23:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Flow EVM] investigate ways to allow the cadence env handle evm errors
5 participants