-
Notifications
You must be signed in to change notification settings - Fork 772
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
EIP150 - Part II #129
EIP150 - Part II #129
Conversation
Chatted with @cdetrio - we need to use the finalized spec doc, not the eip issue. |
@cdetrio -
I think this is ready to be merged into |
When changing fork config to EIP150, running master's stateTests fails 951 tests.
After these changes: we're at
FAQ/WalkthroughThis file will be used to answer any common questions from the Gitter or elsewhere. We'll add to separate documentation/the README later, but using hackmd for easy collab/editing for now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments for checkOutOfGas
.
@cdetrio - left you a message on Gitter about this.
lib/opFns.js
Outdated
|
||
function checkOutOfGas (runState, callOptions) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving checkOutOfGas logic into a new function helps reduce the number of responsibilities forcheckCallMemCost
. This will help with maintainability, and implementation of EIP150 (so that the gas required for a call can be calculated separate from memCost
and OOG Error
logic).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update on EIP150 progress. I've cleaned it up: only 359 tests are failing (down from 900+ before #129).
1..2412
# tests 2412
# pass 2053
# fail 359
I think this will be a more complicated refactor that originally expected because checkCallGas
(and others) are used by many op code functions (not just the ones that need a limit applied). This means means we can't simply change the functions. We must refactor the logic that does not apply to CALL
and CREATE
.
Do you have a moment to chat later this week? I'd love your guidance on what to do next. I have some ideas about how we can move forward quickly, especially now that we have more contributors.
For example, I think these following EIP150 requirements can be split into separate issues that different contributors can grab.
- If SELFDESTRUCT hits a newly created account, it triggers an additional gas cost of 25000 (similar to CALLs)
- If a call asks for more gas than the maximum allowed amount (ie. total amount of gas remaining in the parent after subtracting the gas cost of the call and memory expansion), do not return an OOG error; instead, if a call asks for more gas than all but one 64th of the maximum allowed amount, call with all but one 64th of the maximum allowed amount of gas (this is equivalent to a version of #90 plus #114). CREATE only provides all but one 64th of the parent gas to the child call.
I can help with this by creating issues and EIP-150 specific instructions. I can also lead on putting the PRs back together into a cohesive feature that passes CI. Let me know what you think..
Thanks!
|
quick note for @inclu-media : Test output before I pushed:
Latest test failure: Thanks for the help! |
Moved refund handling in the SUICIDE op to after subGas for non existing toAccount otherwise a refund could be given followed by an OOG resulting in wrong gas.
|
we are though! |
tests/hooked.js
Outdated
@@ -32,7 +32,8 @@ tape('hooked-vm', function (test) { | |||
} | |||
|
|||
var vm = createHookedVm({ | |||
enableHomestead: true | |||
enableHomestead: true, | |||
enableHomsteadReprice: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: Homstead
Upgrade ethereumjs-util to v7.0.2
(Edit: updated to included a quick walkthrough from @cdetrio in the last comment.)
Small PR. Note that the PR is being made to
ethereumjs/eip150
, on top of @axic 's work. Merging this will complete that branch, for a final merge into master.eip150
, including "do not return an OOG error" requirement, by callingcalcCallLimit
inCREATE
opFn.runTx.js
./cc @axic in case I did anything incorrectly.
Thanks in advance for any feedback / tips!
Remaining Tasks:
(old list, using outdated EIP)
new list, using up-to-date EIP:
Complete
If
block.number >= FORK_BLKNUM
, then:N
asN - floor(N / 64)
Remaining: