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

Errata: EIP-2200 should require EIP-1884 #2514

Closed
wants to merge 2 commits into from

Conversation

sorpaas
Copy link
Contributor

@sorpaas sorpaas commented Feb 10, 2020

This PR add clause requires: 1884 for EIP-2200, otherwise the variable name SLOAD_GAS in EIP-2200 can be a point of confusion.

@sorpaas sorpaas changed the title Errata: EIP-2200 change variable name SLOAD_GAS to SSTORE_DIRTY_GAS Errata: EIP-2200 should require EIP-1884 Feb 20, 2020
@sorpaas
Copy link
Contributor Author

sorpaas commented Feb 20, 2020

Changed this to be a much simpler way -- add clause requires: 1884 so no point of confusion is possible.

@GregTheGreek
Copy link
Contributor

GregTheGreek commented Feb 20, 2020

@soc1c To answer your question, no it wouldn't. The way client teams implemented it was that they will take the SLOAD_GAS from EIP-1884, and use that number as the subtraction in the formula when calculating the SSTORE_DIRTY_GAS. Its a case where some clients implemented it as a bundle, and just didn't create a separate variable, since the two were identical in numerical value.

My thoughts:

  • If you go with #86437e you haven't cleared up anything, its still confusing and misleading to have SLOAD_GAS a parameter when it is in fact not. I would suggest the original changes you made in 44feb9 that rename SLOAD_GAS to SSTORE_DIRTY_GAS ontop of this parameter.

@sorpaas
Copy link
Contributor Author

sorpaas commented Feb 20, 2020

I wouldn't mind either 86437ec or 86437ec + 44feb98 but I disagree for the point that the former doesn't clear up things. The only confusion point is if EIP-2200 is applied without EIP-1884. Applying 2200 without 1884 was never intended and is in fact buggy (SLOAD/SSTORE gas costs are quite messed up in that situation). Users who wish to do that should use EIP-1283 + EIP-1706 instead.

@meowsbits
Copy link
Contributor

meowsbits commented Feb 20, 2020

Applying 2200 without 1884 was never intended and is in fact buggy

But... (and I hate to be nitpicky here ☹️ ) if the EIP was Finalized without a specification for EIP1884 dependence (and has no mention of it in the document), then... maybe the EIP2200 is just buggy? And the intention would need a new EIP to establish a complete and un-buggy specification?

Or...? I'm not very familiar with the EIP process specs... can one simply amend a Final spec? Is that allowed? 😕

@sorpaas
Copy link
Contributor Author

sorpaas commented Feb 20, 2020

@meowsbits See #2388 which is an errata for EIP-1052. Your route works as well, but given there're precedents of errata, I do not plan to create new specifications at this moment.

And just FYI, EIP-1884 is indeed mentioned in the summary section of 2200:

This is an EIP that implements net gas metering. It's a combined version of EIP-1283 and EIP-1706, with a structured definition so as to make it interoperable with other gas changes such as EIP-1884.

I think it's over-exaggerate to call it "buggy". No changes mentioned here will require implementations to change anything or to cause consensus bugs in Ethereum. Rather, it's a "potential of misuse". Not specify 1884 can indeed lead to people from other chains trying to apply this specifications without it, and result in problematic chains.

@meowsbits
Copy link
Contributor

@sorpaas Thanks for the clarification! (And sorry I missed the 1884 reference - my search went down, but not up all the way... oops).

I have one more question - does this specification (EIP2200) actually specify SLOAD gas cost to be 800, or was that an "assumption" (potentially soon-to-be-made-explicit) taken from the context of EIP1884? (I'm still confused with the SLOAD_GAS meaning 😕 ).

... And if SLOAD_GAS (== SSTORE_DIRTY_GAS?) was presumed to be "inherited" from the 1884 context, then maybe the spec can be revised from

- `SLOAD_GAS`: changed from `200` to `800`.

to

- `SLOAD_GAS`: unchanged from `800` (stays at `800`).

@sorpaas
Copy link
Contributor Author

sorpaas commented Feb 20, 2020

Thinking again I don't think this is needed at all. The so-called "bug" only affects ETC and has nothing to do with Ethereum or EIPs anyway. I don't think we should spend too much time dealing with unintended use cases like this.

I'm closing this PR for now. We can re-open it if it's really needed in the future.

@sorpaas sorpaas closed this Feb 20, 2020
@shanejonas
Copy link
Contributor

why would you close this??

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

Successfully merging this pull request may close these issues.

5 participants