Skip to content
This repository has been archived by the owner on Oct 28, 2021. It is now read-only.

Implement EIP-2200: Structured Definitions for Net Gas Metering in aleth-interpreter #5728

Merged
merged 4 commits into from
Sep 24, 2019

Conversation

gumb0
Copy link
Member

@gumb0 gumb0 commented Aug 28, 2019

@gumb0 gumb0 added this to the Istanbul milestone Aug 28, 2019
@gumb0 gumb0 mentioned this pull request Aug 28, 2019
18 tasks
@codecov-io
Copy link

codecov-io commented Aug 28, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@0f41bb1). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #5728   +/-   ##
=========================================
  Coverage          ?   63.79%           
=========================================
  Files             ?      358           
  Lines             ?    30564           
  Branches          ?     3405           
=========================================
  Hits              ?    19499           
  Misses            ?     9835           
  Partials          ?     1230

@gumb0 gumb0 force-pushed the eip-2200-interpreter branch 2 times, most recently from 6948ef2 to 6c0fbf3 Compare September 3, 2019 13:06
@gumb0 gumb0 changed the title Implement EIP-2200: Rebalance net-metered SSTORE gas cost with consideration of SLOAD gas cost change in aleth-interpreter Implement EIP-2200: Structured Definitions for Net Gas Metering in aleth-interpreter Sep 23, 2019
@winsvega
Copy link
Contributor

waiting this PR to update the blockchain tests and check aleth consensus with geth and besu

@gumb0
Copy link
Member Author

gumb0 commented Sep 24, 2019

@winsvega you rather need this one #5709 (it's default aleth's VM implementation and it's going to be merged first)

@gumb0 gumb0 force-pushed the eip-2200-interpreter branch 2 times, most recently from 530da2d to 0271da8 Compare September 24, 2019 16:52
@gumb0 gumb0 merged commit 41c94e1 into master Sep 24, 2019
@gumb0 gumb0 deleted the eip-2200-interpreter branch September 24, 2019 18:09
@payvint
Copy link

payvint commented Sep 26, 2019

Hi guys, could you please explain me where is this constants are located?
https://gist.github.com/karalabe/adc43c07db9f03be82093cd5466562b0#specification
I have a little bit got lost in code

@gumb0
Copy link
Member Author

gumb0 commented Sep 26, 2019

@payvint there might be not exact one-to-one correspondence between the constants in the spec and constants defined in our code, but for LegacyVM they are in https://github.com/ethereum/aleth/blob/master/libethcore/EVMSchedule.h
and for aleth-interpreter they are in https://github.com/ethereum/aleth/blob/master/libaleth-interpreter/VM.h

@gumb0
Copy link
Member Author

gumb0 commented Sep 26, 2019

@payvint Our implementation I think is closer to the spec definition in the current version of the EIP at ethereum/EIPs#2200, than to the one at your link (but I guess they are effectively equivalent)

@payvint
Copy link

payvint commented Sep 26, 2019

Thanks @gumb0, I will take a look

@payvint
Copy link

payvint commented Sep 26, 2019

So, @gumb0 , thank you for links very much. I 've just figured it out. But I have one question:
https://gist.github.com/karalabe/adc43c07db9f03be82093cd5466562b0#specification
ethereum/EIPs#2200
In last version - constants are changed and optimized from 8 new to 1 changes in existing implementation, but you changed a constant named SLOAD_GAS - does it related to existence one?
SLOAD_GAS was 200 and became 800 - does it mean that opcode sload - costs 4 times more?
if yes - it is not mentioned in EIP, as I read. if no - please show me where could I find proofs?:)
Thanks

@gumb0
Copy link
Member Author

gumb0 commented Sep 26, 2019

@payvint that's right, SLOAD cost has changed, and it's the subject of another Istanbul EIP https://eips.ethereum.org/EIPS/eip-1884

@payvint
Copy link

payvint commented Sep 26, 2019

@gumb0 Thank you very much. Figured out everything!!!

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

Successfully merging this pull request may close these issues.

5 participants