Istanbul: EIP-2200 net gas metering for SSTORE #590
Conversation
|
Also merged in master. Had to update the |
| { original: new BN(1), code: '600060005560016000556000600055', used: 10818, refund: 19200 }, // 1 -> 0 -> 1 -> 0 | ||
| { original: new BN(1), gas: new BN(2306), code: '6001600055', used: 2306, refund: 0, err: ERROR.OUT_OF_GAS }, // 1 -> 1 (2300 sentry + 2xPUSH) | ||
| { original: new BN(1), gas: new BN(2307), code: '6001600055', used: 806, refund: 0 } // 1 -> 1 (2301 sentry + 2xPUSH) | ||
| ] |
There was a problem hiding this comment.
Hmm, the numbers in the test cases differ to what is listed in the EIP? 🤔
There was a problem hiding this comment.
Hm, these test cases have been added after I created the PR: ethereum/EIPs@01b66af
It seems the EIP document has also been modified. Will have to look into it
There was a problem hiding this comment.
The parameters in the EIP are also named very much differently than here (and in Go), did this also change? Or is this an intentional deviation to be in line with the old Constantinople naming?
If this changed we should likely do the extra round sigh on the Common library and change the parameter names (and eventually delete some?).
There was a problem hiding this comment.
So the test vectors that were recently added to the EIP are for an older version of the EIP which assumed cost of SLOAD to be 200 instead of 800. See comment: ethereum/EIPs#2200 (comment)
About the param names, as long as we're conforming to test cases it should be fine. I don't think every client uses the same param names. The EIP is also somewhat under-specified. I'm more confident in geth's code as that's what eventually makes it into mainnet. But we can wait until the EIP's PR is merged, hopefully by then things will be more clear.
|
See also #591. |
|
@s1na Ok, I think I would approve here. Can you update the branch and remove the update to common |
|
Update: ah, just seeing that the last 2 commits this branch is lacking are not from the Common update. Will then update the branch here via GitHub and approve. |
Reference for implementation and tests: ethereum/go-ethereum#19964