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.

Sign up for GitHub

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

Jump to bottom

Update gas metering for calls to empty accounts #1774

Merged
merged 4 commits into from
Aug 25, 2020
Merged

Update gas metering for calls to empty accounts #1774

merged 4 commits into from
Aug 25, 2020

Conversation

wolflo
Copy link
Contributor

@wolflo wolflo commented Jul 12, 2020

Addresses #1773, but is in need of some tests still.

Also, it brings up the related question of determining account existence. EIP-161 defines an account as "empty when it has no code and zero nonce and zero balance." I'm not sure if existence in EVM.world.accounts ends up having the same meaning or not.

@ehennenfent ehennenfent linked an issue Jul 14, 2020 that may be closed by this pull request
Incorrect gas metering for CALL #1773
Closed
@ehennenfent
Copy link
Contributor

Thanks for the PR! Just as an update, we're interested in merging this, but we've introduced a test regression that we're trying to eliminate before merging other PRs. You can track the progress of that here: #1776

@wolflo
Copy link
Contributor Author

wolflo commented Jul 21, 2020

@ehennenfent thanks for the update. I can write some tests for it in the meantime, but it looks like most of the evm tests rely on the Ethereum VM test suite. My assumption is this isn't covered because the vm tests replace call execution with the callcreates item.

So, are python tests preferred for EVM changes, or additional GST-style tests?

@ehennenfent
Copy link
Contributor

The particular style tests are written in doesn't matter so much as the time it takes to run them - in particular, we don't want to push the time for one build step above ~40m. Assuming that Python tests are easier, you could put them in tests/ethereum, which already has several miscellaneous EVM tests that fall under it.

@ehennenfent
Copy link
Contributor

Hey there! We've fixed #1776 so I wanted to check in again. It looks like all this needs is a small regression test to ensure that gas is calculated correctly for the CALL instruction. I think ethereum/test_general.py and ethereum/test_regressions.py both have some similar tests that should be easily modifiable.

@wolflo
Copy link
Contributor Author

wolflo commented Aug 12, 2020

Sorry I lost track of this a bit, but will add a test this week.

@wolflo
Copy link
Contributor Author

wolflo commented Aug 17, 2020

@ehennenfent happy to make any changes or updates you can suggest. It doesn't seem like this branch is the source of the CI failures, but I can rebase or dig into the issue as well.

ehennenfent
ehennenfent approved these changes Aug 25, 2020
View reviewed changes
Copy link
Contributor

@ehennenfent ehennenfent left a comment

Choose a reason for hiding this comment

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

Updating from master fixed the CI failures, so I suspect it was the coveralls permissions issue we encountered last week. This looks good to me. Thanks for the PR!

@ehennenfent ehennenfent merged commit 6d590e1 into trailofbits:master Aug 25, 2020
@ehennenfent ehennenfent added this to the Manticore 0.3.5 milestone Sep 3, 2020
ekilmer added a commit that referenced this pull request Sep 28, 2020
@ekilmer
Merge branch 'master' into capstone-5-dev
3e646f7 
* master:
  Change types.FunctionType=<class 'function'> (#1803)
  Fix test regressions (#1804)
  State Introspection API (#1775)
  Fix EVM account existence checks for selfdestruct and call (#1801)
  Add partial implementation of sendto syscall (#1791)
  crytic-compile: use latest release (#1795)
  Update gas metering for calls to empty accounts (#1774)
  Fix BitVec with symbolic offset and fix TranslatorSmtlib.unique thread safety (#1792)
  Fix Coveralls for external PRs (#1794)
  Convert plugin list to dict (#1781)
  Symbolic-length reads from symbolic sockets (#1786)
  Removing Thread unsafe global caching (#1788)
  Add Manticore native State-specific hooks (#1777)
@ehennenfent ehennenfent mentioned this pull request Oct 6, 2020
Merged
ekilmer added a commit that referenced this pull request Feb 27, 2021
@ekilmer
Merge branch 'master'
040bd6f 
* master: (43 commits)
  Syscall specific hooks (#2389)
  TUI Support Infrastructure (#1620)
  Fix coveralls upload (#2387)
  docs: fix simple typo, straigth -> straight (#2381)
  Attempt to allow symbolic balances from the start (#1818)
  Fix state.cpu.PC member (#1825)
  Bump black and mypy (#1824)
  Manticore 0.3.5 (#1808)
  Fix yices timeout argument (#1817)
  Detect default solver (#1820)
  Ignore Gas Calculations by Default (#1816)
  native/cpu/x86: Add support for CPUID EAX=80000000h (#1811)
  Change types.FunctionType=<class 'function'> (#1803)
  Fix test regressions (#1804)
  State Introspection API (#1775)
  Fix EVM account existence checks for selfdestruct and call (#1801)
  Add partial implementation of sendto syscall (#1791)
  crytic-compile: use latest release (#1795)
  Update gas metering for calls to empty accounts (#1774)
  Fix BitVec with symbolic offset and fix TranslatorSmtlib.unique thread safety (#1792)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ehennenfent ehennenfent ehennenfent approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Incorrect gas metering for CALL
2 participants
@wolflo @ehennenfent