core, params: implement EIP-1087, net storage gas metering

[karalabe]

Implements https://eips.ethereum.org/EIPS/eip-1087.

An important thing to note in this PR: Until now, the dirtyStorage set inside the stateObject was an optimization. It of course correctly tracked the dirtiness when something was changed, but code until now didn't much care about revertals (i.e. it still kept it dirty). This was not an issue because it was just a tiny amount of extra work during commit. With the net gas metering EIP enabled however, the SSTORE gas costs depend on accurate tracking of the dirtiness, so we need to take better care from now on (i.e. this PR starts tracking the dirtiness in the journal too).

The EIP doesn’t really specify what happens if an inner transaction makes a storage slot dirty, but then reverts. Should the slot be reverted to clean (thus incurring an additional gas hit when modifying it again), or should the slot be kept dirty? Our code currently reverts the dirtiness too but I guess we can change if needed. However this is an important subtlety to emphasize in the spec. Clarified in the EIP.

Ping @Arachnid, PTAL.

[Arachnid]

Per our earlier conversation, this probably needs rewording, to make it clear that setting something to the value it already contains only charges 200 gas and doesn't set the dirty bit.

[gumb0]

Comment should say that it returns now dirty flag, too

[karalabe]

Updated.

[AlexeyAkhunov]

default here also includes the case !dirty && y.Sign() == 0, which per specification should charge 5k, not 200

[karalabe]

Good catch! Will fix on Monday.

…

On Fri, Aug 10, 2018, 12:37 ledgerwatch ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In core/vm/gas_table.go <#17208 (comment)> : > + // marked as dirty. If the slot was previously set to 0, 20000 gas is deducted; + // otherwise, 5000 gas is deducted. + // 2. When a storage slot that is already in the dirty map is written to, 200 + // gas is deducted. + // 3. At the end of the transaction, for each slot in the dirty map: + // * If the slot was 0 before the transaction and is 0 now, refund 19800 gas. + // * If the slot was nonzero before the transaction and its value has not changed, refund 4800 gas. + // * If the slot was nonzero before the transaction and is 0 now, refund 10000 gas. + switch { + case value == common.BigToHash(y): // noop + return params.NetSstoreNoopGas, nil + case !dirty && y.Sign() != 0 && value == (common.Hash{}): // clean(0) => anything + return params.NetSstoreCleanInitgas, nil + case !dirty && y.Sign() != 0: // clean(non 0) => anything + return params.NetSstoreCleanUpdateGas, nil + default: // dirty(anything) => anything default here also include the case !dirty && y.Sign() == 0, which per specification should charge 5k, not 200 — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#17208 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAH6Gf8XatZA0ySbvWP4fpWJZiZN60drks5uPVRRgaJpZM4VWUQK> .

[AlexeyAkhunov]

I think part of this PR (replacing "cachedStorage" with "originStorage") could be merged regardless of whether EIP-1087 gets implemented. I am taking this part to Turbo-Geth, because it allows it to avoid writes of the storage values that did not change