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

EIP1283 - Net gas metering #1105

Closed
pipermerriam opened this issue Jul 27, 2018 · 8 comments
Closed

EIP1283 - Net gas metering #1105

pipermerriam opened this issue Jul 27, 2018 · 8 comments

Comments

@pipermerriam
Copy link
Member

pipermerriam commented Jul 27, 2018

What is wrong?

ttps://eips.ethereum.org/EIPS/eip-1087
https://eips.ethereum.org/EIPS/eip-1283 is coming in Constantinople

How can it be fixed

Implement the changes to how SSTORE costs are computed.

@veox
Copy link
Contributor

veox commented Aug 11, 2018

There are now two incompatible proposals (the latter a counter-proposal by Parity Tech due to their implementation specifics):



A copy-paste from gitter/ethereum/AllCoreDevs:

@veox

(...)

EIP-1087 introduces a map (of bools) for marking "dirty" storage, but doesn't make comparisons to the original values until the post-processing step. Post-processing can be delayed until a transaction's execution is complete, so that the refund logic can be applied outside the EVM.

EIP-1283 needs a "cache" (a map, most likely - of uint256s) of original storage values, so that it can determine gas_used when doing a write (20000 or 5000). This cache must now necessarily reside within the EVM. In other words:

EIP-1283 introduces a map (of uint256s) for "original" value look-ups.

(The above might not make sense depending on how your favourite implementation delineates "inside the EVM". Or maybe I'm battling a chimera here, to understand the trade-offs. Sorry if noise.)


To which the author of EIP-1283 responds:

@sorpaas

Not quite the case, actually. EIP-1283's "map" is needed to be kept for every client right now -- all clients already have this information because they need to handle it in case a transaction reverts. Just like a storage slot's current value and new value, most clients won't need any new extra structs to fetch that info.

The issue with EIP-1087 is that the "dirty map" concept is in conflict with one of the optimization we do after EIP-658, so for parity-ethereum, we can't have that without getting an extra struct.

So you may say:

  • EIP-1087 needs a map (of uint256s) for "original" value look-ups, and introduces a map (of bools) for marking "dirty" storage.
  • EIP-1283 needs a map (of uint256s) for "original" value look-ups.

The first is needed any way by other operations as well.



A few questions, somewhat abstract:

  • Which of the two (if either) would more closely describe the situation in py-evm?
  • Would implementing either require pushing the "cache for original values of storage items" deeper? Or is that already available at the point where gas-use metering is done?

@pipermerriam
Copy link
Member Author

pipermerriam commented Aug 13, 2018

Looking at our JournalDB class, I believe 1283 is going to be easiest to implement. We'll have to figure out how to expose the original/current values to the SSTORE opcode but that shouldn't be too difficult.

To answer your specific questions.

Which of the two (if either) would more closely describe the situation in py-evm?

I believe that our architecture is closest to 1283 since we keep an in-memory map of current values.

Would implementing either require pushing the "cache for original values of storage items" deeper? Or is that already available at the point where gas-use metering is done?

Implementing 1283 will require exposing information about the cache (the journal) deeper as to make it available within the SSTORE opcode function. I suspect this will be best done as a combination of a formal API on the AccountDB for checking storage slot status. I believe this will involve simply querying the self._trie which should return the original value, and self._journaltrie which should return the current value.

@pipermerriam
Copy link
Member Author

@veox just in case you read these in your email, I update my response with explicit answers to your questions.

@Bhargavasomu
Copy link
Contributor

@pipermerriam I would like to work on this issue. I might need help in between.

@pipermerriam
Copy link
Member Author

@Bhargavasomu Same as #1346 (comment) probably best to stick to "Good First Issue" tagged issues until you have a better understanding of the codebase.

@5chdn
Copy link

5chdn commented Nov 14, 2018

This is done in #1410, isn't it?

@cburgdorf
Copy link
Contributor

@5chdn correct. That said, we aren't passing all state tests yet (#1181) which means there's a high chance that our constantinople implementations (all of them) may still have bugs.

That said, Trinity is still in alpha and isn't a reliable client to sync the mainnet yet so nobody should be surprised to hit serious bugs 😅

@carver carver changed the title EIP1087 - Net gas metering EIP1283 - Net gas metering Dec 10, 2018
@carver
Copy link
Contributor

carver commented Dec 13, 2018

Confirmed finished in #1579

@carver carver closed this as completed Dec 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants