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

EIP-2200: Structured Definitions for Net Gas Metering #2200

Open
wants to merge 15 commits into
base: master
from

Conversation

@sorpaas
Copy link
Contributor

commented Jul 18, 2019

A repricing efforts for several trie-size-dependent opcodes are being carried out at EIP-1884. This EIP also reprices net-metered SSTORE opcode for EIP-1283.

sorpaas added 2 commits Jul 18, 2019
EIPS/eip-2200.md Outdated Show resolved Hide resolved

## Rationale

The same as EIP-1283's rationale.

This comment has been minimized.

Copy link
@nicksavers

nicksavers Jul 18, 2019

Collaborator

Unless it's competing with another proposal, I doubt that it has the same rationale.

In the Gitter chat the comment was:

If we apply 1884, then we do need to change the SLOAD_GAS related to SSTORE opcode -- that means, it's a spec change of EIP-1283 which will require another EIP. So here's the new one.

Maybe with some extra clarification this can be turned into a rationale?

EIPS/eip-2200.md Outdated Show resolved Hide resolved
sorpaas added 2 commits Jul 18, 2019
EIPS/eip-2200.md Outdated Show resolved Hide resolved
EIPS/eip-2200.md Outdated Show resolved Hide resolved

## Rationale

The same as EIP-1283's rationale. If EIP-1884 is applied, then `SLOAD_GAS`

This comment has been minimized.

Copy link
@nicksavers

nicksavers Jul 18, 2019

Collaborator

The same as EIP-1283's rationale.

This already builds on EIP-1283, so it inherits the same rationale. Could you explain what would happen if Ethereum didn't include this change and decides to go ahead with just EIP-1283?

This comment has been minimized.

Copy link
@chfast

chfast Aug 8, 2019

Contributor

The EIP-1702 has a Rationale secion: https://github.com/ethereum/EIPs/blob/master/EIPS/eip-1706.md#rationale. Can you copy the content here?

This comment has been minimized.

Copy link
@holiman

holiman Aug 15, 2019

Contributor

👍 for copying the content here

@axic axic added the type: Core label Jul 19, 2019

@axic

This comment has been minimized.

Copy link
Member

commented Jul 26, 2019

@sorpaas do you plan to make the clarifications @nicksavers requested?

@sorpaas

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2019

Will do. Sorry for the delay.

@gumb0

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

I re-ran test cases from 1283 with these new prices, here are the results sorpaas#1 (comment)

refund counter.
* Otherwise, add `SSTORE_RESET_GAS - SLOAD_GAS` gas to refund
counter.
* If *gasleft* is less than or equal to 2300, fail the current call

This comment has been minimized.

Copy link
@gumb0

gumb0 Aug 1, 2019

Member

Should this be rather moved to the top of this list of clauses (before If *current value* equals *new value*)? Otherwise here it looks like it applies to the gas left after SSTORE cost deduction, which is not the intention, right?

This comment has been minimized.

Copy link
@nevillegrech

nevillegrech Aug 14, 2019

If the 2300 gas is to guard against reentrancy following a transfer, then 1600 will also suffice as the underlying CALL will cost at least 700.

* If *original value* is not 0
* If *current value* is 0 (also means that *new value* is not
0), remove `SSTORE_CLEARS_SCHEDULE` gas from refund
counter. We can prove that refund counter will never go below

This comment has been minimized.

Copy link
@gumb0

gumb0 Aug 1, 2019

Member

Maybe the statement about counter not going below zero should be removed, it's not quite accurate, and you already have the clarification in EIP-1283 about how it can go negative in some implementations.

@sorpaas

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

I haven't got time to update this. I'm on vacation and got caught on some other stuff I'm working on. If people feel it's not ready, I'm okay to postpone this. However, personally I think there's only documentation work remain and everything else is already finished.

@karalabe karalabe referenced this pull request Aug 6, 2019
9 of 10 tasks complete
@gumb0 gumb0 referenced this pull request Aug 7, 2019
11 of 27 tasks complete
@chfast

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

@AlexeyAkhunov's proposal

https://gitter.im/ethereum/AllCoreDevs?at=5d31988ae2d1aa6688d09f39

Instead of doing the check "gasleft < 2300" at the end of the operation, we could instead make a mandatory (unconditional) charge of 2300 in the beginning, and adjust other cases (by issuing more refunds or reducing refunds) to accommodate that. In that way, "gasleft" is not explicitly accessed, but the semantics is the same

@sorpaas @AlexeyAkhunov I did quick analysis of @AlexeyAkhunov alternative proposal to charge min 2300 for SSTORE but add 1500 refund more. None of the EIPs list use cases for it, so I took a simple lock/mutex which is

SLOAD  # check the lock status
SSTORE 0 -> 1 # lock
... # do something, e.g. make a call
SSTORE 1 -> 0 # unlock

@AlexeyAkhunov's proposal (let's call it variant 2) looks pretty solid because SSTORE / SLOAD will cost 800 now (I still have 200 in my mind).
In simplest operation of a CALL costing 700 the variant 2 is less than 7% more expensive in the end.
For the case where in variant 1 we get full refund and the protected operation cost ~17k the difference is less than 4%.
In the final case where variant 2 also get full refund the protected operation cost ~20k (and the end costs are the same obviously).

https://docs.google.com/spreadsheets/d/1l6HHVAEcmJyb76J-trNQQFD1lipjkBkl5-QYx3ME2mc/edit?usp=sharing

This was previously sent to official discussion place for this EIP: sorpaas#1 (comment), but has received no feedback.

@holiman

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

I personally think that @AlexeyAkhunov 's proposal is the most elegant. So the variants are

  • Go into some kind of semi-readonly-mode when entry-gas is below 2300. Increases complexity needlessly.
  • Perform a check that we're above 2300 on every SSTORE. Also very quirky -- require N gas but does not use N gas, for some obscure reason that people will forget in a year or two.
  • Increase both SSTORE cost and refund size, thus requiring a larger runtime gas margin which is returned post-facto. Only downside that I can see is that we cannot reuse Constantinople gas formula, but need to have one formula for Rinkeby and another for Mainnet.

All in all, I prefer the latter one, mainly since it's self-contained and does not cause new abstractions or quirks to be added in other places.

@chfast

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

* Go into some kind of semi-readonly-mode when entry-gas is below `2300`. Increases complexity needlessly.

I don't think anyone want to go with this one any more. As the original author I propose to cross it out for clarity. Others can always object and bring it back.

type: Standards Track
category: Core
created: 2019-07-18
requires: 1283, 1706, 1884

This comment has been minimized.

Copy link
@axic

axic Aug 12, 2019

Member

So is this requiring 1283/1706 or replacing them?

This comment has been minimized.

Copy link
@holiman

holiman Sep 17, 2019

Contributor

I don't see how it requires 1283 ?
And I certainly don't see how it requires 1706... ?

This comment has been minimized.

Copy link
@sorpaas

sorpaas Sep 19, 2019

Author Contributor

It requires all of the three EIPs to be functional.

@karalabe

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

Where is this proposal from @AlexeyAkhunov? Everyone's mentioning it, but I have no clue what it is.

@chfast

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

Where is this proposal from @AlexeyAkhunov? Everyone's mentioning it, but I have no clue what it is.

There is no formal spec. But summary is this:

proposal to charge min 2300 for SSTORE but add 1500 refund more

I dug up the original message from gitter and put it in #2200 (comment).

@karalabe

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

@chfast The original text of the EIP is If gasleft is less than or equal to 2300, fail the current call frame with 'out of gas' exception. (i.e. it fails on equal too), whereas the suggestion from @AlexeyAkhunov allows equal. Please elaborate

@chfast

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

@chfast The original text of the EIP is If gasleft is less than or equal to 2300, fail the current call frame with 'out of gas' exception. (i.e. it fails on equal too), whereas the suggestion from @AlexeyAkhunov allows equal. Please elaborate

Because you also need other instructions to provide arguments for SSTORE, you still will not be able to squeeze SSTORE in a call providing only 2300 gas. Just to keep the set of magic numbers in EVM small, I believe the value 2300 is ok this case.

@karalabe

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

That proposal makes things quite funky. I've tried to integrate it into the EIP, someone please double check everything. We'll end up duplicating quite a few constants because now we need to take that +1500 gas into consideration on all code paths and compensate.

My attempt(!) at updating the spec

Definitions of constants are as below:

  • SLOAD_GAS: 800
  • SSTORE_GAS: 2300
  • SSTORE_SET_GAS: 20000
  • SSTORE_SET_REFUND: 20700
  • SSTORE_RESET_GAS: 5000
  • SSTORE_RESET_REFUND: 5700
  • SSTORE_CLEAR_REFUND: 15000
  • SSTORE_CLEAR_REFUND_REVERT: 13500

Replace SSTORE opcode gas cost calculation (including refunds) with the following logic:

  • If current value equals new value (this is a no-op), SLOAD_GAS gas is deducted.
  • If current value does not equal new value:
    • If original value equals current value (this storage slot has not been changed by the current execution context):
      • If original value is 0, SSTORE_SET_GAS gas is deducted.
      • Otherwise, SSTORE_RESET_GAS gas is deducted. If new value is 0, add SSTORE_CLEAR_REFUND to refund counter.
    • If original value does not equal current value (this storage slot is dirty), SSTORE_GAS gas is deducted. Apply both of the following clauses:
      • If original value is not 0:
        • If current value is 0 (also means that new value is not 0), subtract SSTORE_CLEAR_REFUND_REVERT gas from refund counter. We can prove that refund counter will never go below 0.
        • If new value is 0 (also means that current value is not 0), add SSTORE_CLEAR_REFUND gas to refund counter.
      • If original value equals new value (this storage slot is reset):
        • If original value is 0, add SSTORE_SET_REFUND to refund counter.
        • Otherwise, add SSTORE_RESET_REFUND gas to refund counter.
@karalabe

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

Someone would really need to reproduce the entire test matrix for all the variations, otherwise we might end up with some path not being in line with the others. The +1500 addition to SSTORE means we need to compensate all the refunds and refund revertals properly.

A nasty side effect of raising the gas for SSTORE and refunding it later is that whilst the gas paid by end users might be the same, a significant portion of the block gas space might end up wasted, since the transaction still "consumes" that gas out of the block, just is not charged for. Hmm, I guess only the gas used is charged against the block space too.

@karalabe

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

cc @Arachnid Some feedback? ^

@holiman

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

since the transaction still "consumes" that gas out of the block, just is not charged fo

Hmm... Are you sure? I don't think so

@chfast

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

@holiman I have considered the option of lowering the required gas for SSTORE from 2300 to 1600 (i.e. stipend - call cost). I'm not sure this gives us a lot because the particular number is less important than the fact we introduce a weird behavior. But on theoretical level it depends on the interpretation what "calls with stipend only are safe" mean.

  • if "reentry is not possible" we can go with 1600,
  • if "the callee cannot change its state" then 2300 should stay.

In case of 1600, it should be defined as stipend_gas - call_gas so we don't have to update the spec in case we'd lake to change the call_gas in future.

Personally, I don't have strong preferences. 2300 is slightly simpler and safer, 1600 is more user friendly because chance of this exception to happen in practice is lower.

@holgerd77 holgerd77 referenced this pull request Aug 18, 2019
17 of 18 tasks complete
@sorpaas sorpaas referenced this pull request Aug 21, 2019
6 of 6 tasks complete
@tkstanczak tkstanczak referenced this pull request Aug 22, 2019
@gumb0 gumb0 referenced this pull request Sep 4, 2019
@chfast

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2019

Yes another analysis of parts of this EIP: ethereum/evmc#418.

@sorpaas sorpaas changed the title Rebalance net-metered SSTORE gas cost with consideration of SLOAD gas cost change EIP-2200: Structured Definitions for Net Gas Metering Sep 6, 2019

@sorpaas

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2019

Should be ready for merge!

sorpaas added 2 commits Sep 6, 2019
EIPS/eip-2200.md Outdated Show resolved Hide resolved
EIPS/eip-2200.md Show resolved Hide resolved
passing, etc.

The original definition of EIP-1283 created a danger of a new kind of
reentrancy attacks on existing contracts as Solidity by default grants

This comment has been minimized.

Copy link
@gumb0

gumb0 Sep 9, 2019

Member

Nit: it's not Solidity's fault, it's part of the protocol, defined in YP

This comment has been minimized.

Copy link
@axic

axic Sep 17, 2019

Member

Solidity sends the stipend if the call doesn't have value associated with it. The protocol only sets it if there's value sent.

refund counter.
* Otherwise, add `SSTORE_RESET_GAS - SLOAD_GAS` gas to refund
counter.
* If *gasleft* is less than or equal to gas stipend, fail the current

This comment has been minimized.

Copy link
@gumb0

gumb0 Sep 9, 2019

Member

Repeating my earlier suggestion, that didn't received response: this should be moved to the top of the list of clauses, otherwise it looks like it is applied after the cost deduction for the current instruction.

This comment has been minimized.

Copy link
@holiman

holiman Sep 19, 2019

Contributor

Yes, should be moved to the top, I fully agree

when a reversion happens on the *current transaction*, it's easy to
see that call frames won't interfere SSTORE gas calculation. So
although the below proof is discussed without call frames, it applies
to all situations with call frames. We will discuss the case

This comment has been minimized.

Copy link
@maurelian

maurelian Sep 12, 2019

Contributor

To disambiguate, do I understand what you mean by these terms?

  • "current transaction" mean the entire transaction initiated by an EOA
  • "call frame" means an invocation of a contract

This comment has been minimized.

Copy link
@sorpaas

sorpaas Sep 19, 2019

Author Contributor

Yes. Those are clear terms defined in EVM.

@sarawut8128

This comment has been minimized.

Copy link

commented Sep 17, 2019

ช่วยตรวจสอบและแก้ไขในส่วนนี้ให้ผมที

EIPS/eip-2200.md Show resolved Hide resolved
## Rationale

This EIP mostly achieves what a transient storage tries to do
(EIP-1087 and EIP-1153), but without the complexity of introducing the

This comment has been minimized.

Copy link
@axic

axic Sep 19, 2019

Member

Should make these two into link to the EIP.

sorpaas added 4 commits Sep 19, 2019
@abandeali1 abandeali1 referenced this pull request Sep 19, 2019
0 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.