-
Notifications
You must be signed in to change notification settings - Fork 1
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-1283: Net gas metering for SSTORE without dirty maps #1
Comments
A small style note: gas in this case is an uncountable noun, if used in a "vehicle fuel" metaphor. I.e.,
|
Thanks. Fixed! |
Here is my complete review on the current proposal draft. ComplicatedThe state diagram and explanation are too complicated. This table may or may not help: A is any non-zero value and B is any-zero value that is not equal to A.
Review alternatives consideredThere are several obvious alternatives to consider:
Please explain the level of review that was performed to evaluate the fitness of the proposed solution versus these options and other considered options. Review economicsThe value 200 is a new constant. Please explain this choice. The price to store a word and then erase it from storage is a fixed 400 gas including refunds. At some point this will be cheaper than the memory expansion function, which is variable priced based on the amount of memory used. This means optimizers will do well to instantiate dummy storage variables as overflow memory registers. Before we encourage this perverse behavior, I would like to see this proposal provide an analysis of the break even point where this optimization saves gas. |
Hi @fulldecent, thanks for the review! Regarding the state diagram, I would encourage you to look deeper into the state diagram provided by Nick Johnson in the explanation section. By separating what the original value of the storage is, we can make everything look much clearer. Regarding "review alternatives considered":
Check local cache and the storage state is the current behavior. The reason is that checking storage state is much more expensive compared with checking cache. That's why we have cache in the first place!
The reason we have 200 is because it aligns with SLOAD gas cost. That fits current assumption of how much gas a storage cache read/write would take.
That's EIP-1087, if you want to take a look. You can also see EIP-1283's motivation section for more explanation of why it doesn't work for some clients. Regarding "review economics":
This "economics" is present right now on mainnet, and either EIP-1283 or EIP-1087 does not affect it. An initial SSTORE operation which creates a new cache slot on execution would cost just as much as it currently does. So I think this discussion would fit more on topics of "storage rent", but not probably not here. |
Isn't the local cache is currently an implementation detail? This proposal creates a read/write storage layer. So I believe this is additive. Regarding 1087, any notes leading to the decisions in this EIP should be discussed here so that this document can stand on its own. I am not concerned with creating new cache slots. I am concerned with a contract that initializes with 60016fff55 60016ffe55 ... and then uses (and resets) these storage locations as an alternative to using memory. Presently this approach would cost 5000 per word stored. But this proposal reduces that to 200 per access plus 200 to reset. So I'd like to understand the breakeven. New issue. Was there a consideration to make STATICCALL compatible with storage where a word is set and then reset before the transaction finishes? This might be valuable to people doing formal verification where they use *CALL more than the average bear. ping @jacqueswww |
Many things in EVM can be argued to be implementation detail. That's true. The goal for having gas cost is to make it matches the most common implementation structure's actual cost.
We have some explanations in the Motivation section that we don't introduce any unnecessary new concepts compared with what all current client implementations have. The only thing added is to make refund counter able to be reduced. All other structures discussed are needed to be kept by clients anyway due to reasons like transaction reverts, etc.
Yeah sure. Right now most of the explanations of this are in EIP-1283's motivation section, if you want to take a look. I would also appreciate help fetching relevant link to this discussion issue.
Memory cost is only calculated when the memory is expanded, meaning only the first time when
That would change the semantic of |
I don't disagree with your points, but I believe that at least some of this analysis belongs in the EIP proper. Regarding memory cost. The current cost of memory at large quantities is quadratic. But the proposed SSTORE cost is linear and much lower than before. Abusing SSTORE (with pre-paid dirty blocks) allows SSTORE to work effectively like memory. SSTORE usage is linear. Therefore this EIP reduces some large memory store costs from quadratic to linear. I do not know if this leads to any practical problems. However this topic should be studied to its logical conclusion and presented inside the EIP. |
With this EIP, whenever we're allocating memory for storage cache, the cost is always the same as current. Yes, indeed later we can add more gas to refund counter compared with current, but that doesn't present attack vector -- you cannot use gas from refund counter in the current transaction. I think if we want to provide any analysis, it's basically this: Given a block gas limit, it is not possible to allocate more memory using SSTORE comparing EIP-1283 and current scheme. It is, however, possible to have (potentially a lot of) transaction fee reduction comparing EIP-1283 and current scheme. |
* Proposed EIP for address and ERC20 transfer rules * Update eip-X.md Updating creation date * Update eip-X.md (#1) * Update eip-X.md * Update eip-X.md * Update eip-X.md Rule -> IRule consistently fix missing links improve abstract * Update eip-X.md typos small improvements adds implementation section
I've created a pull request to modify the proposal with regard to reentrancy issue, but I think this is a correct place for a discussion. Here is a link: In short, I propose to revert any attempt to perform SSTORE if gasleft <= 2300, which makes an assumption that 'gas stipend' is not enough to modify storage more explicit. |
FYI, unless extra precaution is taken for And see general proposal for fixing |
Sorry false alert, the issues was discovered in ganache and I assumed it was the case everywhere. Geth and parity use binary search to find the minimum gas amount and will work with 1706 keeping backward compatibility |
@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
@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). https://docs.google.com/spreadsheets/d/1l6HHVAEcmJyb76J-trNQQFD1lipjkBkl5-QYx3ME2mc/edit?usp=sharing |
Here's the updated test cases table repriced according to EIP-2200
|
I have reviewed this EIP. Just one comment but otherwise everything is great. SLOAD repricing is not necessary or explainedThis EIP changes the price of SLOAD from 200 to 800. I suppose the reason why is because EIP-1884 (DRAFT) and EIP-2200 (DRAFT) are both dependencies of EIP-1679 (DRAFT) and the former changes G_SLOAD from 200 to 800. The reasoning in EIP-2200 (DRAFT) is opaque and is:
I request that the specification remove the requirement that SLOAD_GAS be changed from 200 to 800. Or alternatively, include arguments of why such a change is necessary, using rigor comparable to EIP-1884 (DRAFT). |
Abstract: This EIP proposes net gas metering changes for SSTORE opcode. Rendered
Note on Undefined Behavior
In this specification, we assert that contract creations are always on empty account with empty storage. This is true for Ethereum and all Ethereum-based networks. As a result, contract creations on empty account with non-empty storage is considered undefined behavior.
The text was updated successfully, but these errors were encountered: