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

Handle balance overflow as evm exception #720

Merged
merged 3 commits into from
Apr 16, 2020

Conversation

cgewecke
Copy link
Contributor

Revival of #676

Includes review fixes by @evertonfraga

Addresses #672.

Have made a minimal reproduction of #672 at cgewecke/checkpoint-repro. It runs two transactions vs. ganache-core:

  • the first overflows an account balance and errors as expected with Value overflow
  • the second fails with Cannot get state root with uncommitted checkpoints, indicating that the previous tx has left the vm in an unresolved state.

This happens because the balance overflow error is thrown directly from evm._executeCall and evm._executeCreate without being handled via the revert/commit steps in the method which calls them (evm.executeMessage)

PR

  • returns Value overflow exceptions as part of the vm result (instead of throwing), similar to the way contract creation address collisions are handled
  • makes Value overflow a VMError
  • modifies the existing overflow test to inspect the result error and verify that the checkpoint count is 0 when runTx completes
  • adds a balance overflow test for contract creation txs

NB: Looks like invalid precompiles also trigger an uncontrolled throw (here). Haven't addressed that but if this PR is the correct approach, perhaps that could be resolved here as well.

Copy link
Contributor

@evertonfraga evertonfraga left a comment

Choose a reason for hiding this comment

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

Thanks! That's approved.

@cgewecke
Copy link
Contributor Author

codecov is erroring on a negative diff change but I don't see one. Not sure what's happening there.

codecov/project — 90.98% (+-0.01%) compared to 7eb3217

@evertonfraga
Copy link
Contributor

evertonfraga commented Apr 16, 2020

Codecov uses a quite confusing notation for coverage. There are two metrics:

The codecov/patch check refers only to the changes we made, and that's fully covered:

image

The codecov/project check compares to the parent commit, which is usually master, but in this case, your original commit.

image

I assume that was just a rounding error, the change looks 👌 and there are tests for it.

@evertonfraga evertonfraga merged commit 9cbf19b into master Apr 16, 2020
@evertonfraga evertonfraga deleted the fix/balance-overflow-checkpoints branch April 16, 2020 22:03
@holgerd77
Copy link
Member

@evertonfraga if possible we should also add some pragmatic thresholds here for CodeCov (this was at least possible for Coveralls), so to allow for minor decreases (like 0.5% or so, eventually a bit less) together with some absolute threshold not be allowed to fall below. These little decreases occur all the time (e.g. on 1-2 liner PRs without tests, rewriting little things/splitting stuff up for clarity,...). This gets annoying without some basic flexibility.

@evertonfraga
Copy link
Contributor

Yes! I have just described tweaking config in this issue: #723

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants