EIP-2929: Gas cost increases for state access opcodes #889

cgewecke · 2020-09-23T03:09:35Z

EIP-2929
Test cases
Discord discussion of implementation
Ethereum Magicians thread
hyperledger/besu PR
geth PR

PR passes the initial tests written by holiman. May need some additional work when run against the YoloV2 state tests (which remain WIP as of Oct 20, 2020). Where tests are identified as missing below we can defer to the state tests which should include them for free.

Background

"A short-term security improvement to reduce the effectiveness of what is currently the most effective DoS strategy, reducing the theoretical max processing time of a block by ~3x, and also has the effect of being a stepping stone toward bounding stateless witness sizes"

Specification Details

Initialization

Maintain two sets
- accessedAddresses: Set[Address]
- accessedStorage: Map[Address --> Set[Bytes32]] .
- The sets are transaction-context-wide, implemented identically to other transaction-scoped constructs such as the self-destruct-list and global refund counter.
- If a scope reverts, the access lists should be in the state they were in before that scope was entered.
- ⚠️ Scoping needs tests / clarification ⚠️
When a transaction execution begins,
- accessedStorage is initialized to empty
- accessedAddresses is initialized to include:
  - tx.sender,
  - tx.to (or the address being created if it is a contract creation transaction)
  - the set of all precompiles.
- ⚠️ Needs test ⚠️

Address Read Costs

Opcodes	Tested
EXTCODESIZE (0x3B)	✅
EXTCODECOPY (0x3C)	✅
EXTCODEHASH (0x3F)	✅
BALANCE (0x31))	✅
CALL (0xF1)	✅
CALLCODE (0xF2)	❌
DELEGATECALL (0xF4)	❌
STATICCALL (0xFA))	✅

When an address is the target of the opcodes listed above, the gas costs are computed as follows:

If the target is not in accessedAddresses
- charge COLD_ACCOUNT_ACCESS gas (instead of opcode's current base fee)
- add the address to accessedAddresses
Otherwise, charge WARM_STORAGE_READ gas (instead of current base fee)

Opcodes	Tested
CREATE	❌
CREATE2	❌
SELFDESTRUCT	❌

For CREATE/2

Immediately (ie. before checking if the address is unclaimed)
- charge no additional fees
- add the address being created to accessedAddresses

For SELFDESTRUCT

If the ETH recipient address is not in accessedAddresses
- charge an additional COLD_ACCOUNT_ACCESS (on top of the existing gas costs),
- add the address to accessedAddresses

Storage Read and Store Costs

Opcodes	Tested
SLOAD	✅
SSTORE	✅

For SLOAD,

If the storage location is not yet registered in accessedStorage
- charge COLD_SLOAD gas (instead of current base fee)
- add the location to accessedStorage
If storage location has already been registered, charge WARM_STORAGE_READ gas (instead of current base fee)

For SSTORE

If the storage location is not yet registered in accessedStorage
- charge an additional COLD_SLOAD gas (on top of the existing gas costs)
- add the location to accessedStorage
- modify the parameters defined in EIP 2200 as follows:

Parameter	New value	Tested
SLOAD_GAS	WARM_STORAGE_READ	✅
SSTORE_RESET_GAS	5000 - COLD_SLOAD

codecov · 2020-09-23T03:10:32Z

Codecov Report

Merging #889 into master will decrease coverage by 0.22%.
The diff coverage is 100.00%.

Flag	Coverage Δ
#block	`75.79% <ø> (-0.48%)`	⬇️
#blockchain	`80.33% <ø> (-0.39%)`	⬇️
#common	`91.79% <ø> (ø)`
#ethash	`82.08% <ø> (ø)`
#tx	`88.18% <ø> (-0.19%)`	⬇️
#vm	`86.96% <100.00%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

holgerd77 · 2020-09-24T14:44:22Z

packages/vm/lib/evm/opFns.ts

+
+  return defaultCost
+}
+


Not a concrete suggestion yet, was just wondering since the list of additional helper functions is getting so long:

Should we restructure opcode-related functionality a bit, a first suggestion would be:

opcodes.ts -> opcodes/(op)codes.ts

opFns.ts (opcode implementations) -> opcodes/functions.ts

opFns.ts (all helper functions) -> opcodes/util.ts

Eventually the latter also split up:

opFns.ts (EIP2929 helper functions) -> opcodes/EIP2929.ts

What do you guys think 🤔 ?

@holgerd77 Agree, the helpers alone are starting to be a few hundred lines...

Happy to open that as an issue and self-assign if there's agreement this is the way to go...

Would be a separate PR which we rebase this against eventually?

I agree to split up this file 😄 , but should be a seperate PR 👍

Yeah, makes sense to keep this separate

This is rebased against the file re-org now...

Above it's suggested that:

Eventually the latter also split up:

opFns.ts (EIP2929 helper functions) -> opcodes/EIP2929.ts

In 8cdc9b0 have extended this idea and moved updateSstoreGas out of util splitting it into it's own EIP files.

Idk - this might be more than was necessary - idea is just to make util generic and order dynamic gas pricing logic consistently.

cgewecke · 2020-09-28T04:09:24Z

Some refunds spec'd by EIP-2200 needed additional adjustments...

Made in 62eb4b8, mirroring these geth commits:

There's a PR updating state tests for YOLO V2 at ethereum/tests 723

holgerd77 · 2020-09-30T11:31:34Z

@cgewecke thanks for all the updates here 😃 , make sure to remove DRAFT status and move to PR state: needs review once you consider this ready so that others will now when to start on a review.

holgerd77 · 2020-09-30T11:33:01Z

(and optimally also drop a short note, "just" these status changes also have a tendency to be overlooked for some time since there is not notification associated (I would say at least, or is there some associated with this new explicit "Ready for review" feature baked into GitHub))

cgewecke · 2020-09-30T15:01:05Z

Next step might be to run against the Berlin/YoloV2 tests when they're merged (and see if there are bugs here...)

holgerd77 · 2020-10-20T13:32:19Z

What is this the status here? Any updates on the state of agreement regarding this EIP? It might also be good to give this PR a rebase to not loose track with the code base too much.

cgewecke · 2020-10-20T14:37:53Z

@holgerd77

Status

PR to update general state tests for YoloV2 is still being worked on at ethereum/tests 723
Geth's PR remains open and was actively reviewed today...(looks like might be close to merge?)
No additional discussion about logic changes that I can see...

Will rebase rn.

holgerd77 · 2020-10-20T15:00:52Z

packages/vm/lib/index.ts

@@ -135,7 +135,7 @@ export default class VM extends AsyncEventEmitter {

    if (opts.common) {
      //EIPs
-      const supportedEIPs = [2537]
+      const supportedEIPs = [2537, 2929]


Can you also add a note on the support of this EIP in the code docs above (line 43, so annoying that GitHub doesn't allow comments on non-changed code parts)?

Fixed in e922e1b

holgerd77 · 2020-10-20T15:04:30Z

Great and thanks for the update! 😄

I would suggest that we review here and merge this in also for the dev release and eventually see if we can harden the implementation during the dev release feedback period. Is this ok? Support for this EIP will be still experimental anyhow.

cgewecke · 2020-10-20T15:10:12Z

I would suggest that we review here and merge this in also for the dev release and eventually see if we can harden the implementation during the dev release feedback period. Is this ok? Support for this EIP will be still experimental anyhow.

Yeah! That's sounds good to me. The other implementations have been stable for a few weeks and we can fix any problems when we update state tests here.

… fixes

holgerd77

Have rebased this and integrated the new Address type.

This looks good now, thanks Chris for tackling this and bringing this to the latest state and over the finish line! 😄

Will merge.

jochem-brouwer · 2020-10-23T11:49:37Z

Can we close this? #874

Also the READMEs do not yet reflect that we have 2929 support. Linking #905

holgerd77 · 2020-10-23T12:00:25Z

@jochem-brouwer yes, definitely, feel free to just do on such cases, always some continued challenge to keep up here with this housekeeping stuff. If people disagree they will likely reopen (doesn't happen too often though anyhow).

holgerd77 · 2021-02-20T22:27:27Z

packages/vm/lib/evm/opcodes/EIP2929.ts

+ * @param {BN}       address
+ */
+export function accessAddressEIP2929(runState: RunState, address: Address, baseFee?: number) {
+  if (!runState._common.eips().includes(2929)) return


Just seeing here that this implementation has also these common.eips().includes() calls. We should really replace this soon by a method in common also taking the HFs into account. This will otherwise fall onto our feet.

(so "soon" means: in the next 1-2 weeks)

I wanted to introduce this in the EIP2718 PR, but won't do it as it will get too bloated otherwise. Can you open an issue for this so we don't "forget" it? 😄

I will just prioritize this as the first task I'll do on Monday, so we won't forget then. 😄

holgerd77 reviewed Sep 24, 2020

View reviewed changes

cgewecke added package: common package: vm PR state: WIP type: feature labels Sep 24, 2020

cgewecke marked this pull request as draft September 24, 2020 15:03

cgewecke mentioned this pull request Sep 24, 2020

Refactor opFns.ts #893

Closed

cgewecke force-pushed the eip-2929 branch from 4f1a28d to 1959de5 Compare September 28, 2020 17:53

holgerd77 reviewed Oct 20, 2020

View reviewed changes

cgewecke force-pushed the eip-2929 branch from 8cdc9b0 to 85e8e89 Compare October 20, 2020 17:14

cgewecke marked this pull request as ready for review October 20, 2020 17:15

cgewecke added the PR state: needs review label Oct 20, 2020

cgewecke requested a review from holgerd77 October 20, 2020 17:31

cgewecke added 8 commits October 21, 2020 11:01

Initial draft

cca37a6

Adjust EIP-2200 refunds

582d625

Add charges for selfdestruct ETH recipient address reads

52c7f96

Fix errors from file reorg rebase

c93eef7

Move dynamic gas pricing logic into own EIP files

2e06aaa

Fix tape import (for config 2.0 rebase)

0ef9686

Update supported EIPs comment in lib/index

f82db4a

Lint fixes (config 2.0)

20e845d

vm -> EIP2929: updated accessAddressEIP2929 to use Address type, test…

c186e33

… fixes

holgerd77 force-pushed the eip-2929 branch from 85e8e89 to c186e33 Compare October 21, 2020 09:56

holgerd77 approved these changes Oct 21, 2020

View reviewed changes

holgerd77 merged commit 1afe8db into master Oct 21, 2020

holgerd77 deleted the eip-2929 branch October 21, 2020 10:09

evertonfraga mentioned this pull request Oct 28, 2020

Failing nightly build #928

Closed

holgerd77 reviewed Feb 20, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EIP-2929: Gas cost increases for state access opcodes #889

EIP-2929: Gas cost increases for state access opcodes #889

cgewecke commented Sep 23, 2020 •

edited

Loading

codecov bot commented Sep 23, 2020 •

edited

Loading

holgerd77 Sep 24, 2020

cgewecke Sep 24, 2020 •

edited

Loading

jochem-brouwer Sep 24, 2020

holgerd77 Sep 24, 2020

cgewecke Sep 28, 2020

cgewecke commented Sep 28, 2020 •

edited

Loading

holgerd77 commented Sep 30, 2020

holgerd77 commented Sep 30, 2020

cgewecke commented Sep 30, 2020

holgerd77 commented Oct 20, 2020

cgewecke commented Oct 20, 2020

holgerd77 Oct 20, 2020

cgewecke Oct 20, 2020

holgerd77 commented Oct 20, 2020

cgewecke commented Oct 20, 2020

holgerd77 left a comment

jochem-brouwer commented Oct 23, 2020

holgerd77 commented Oct 23, 2020

holgerd77 Feb 20, 2021

holgerd77 Feb 20, 2021

jochem-brouwer Feb 20, 2021 •

edited

Loading

holgerd77 Feb 20, 2021

EIP-2929: Gas cost increases for state access opcodes #889

EIP-2929: Gas cost increases for state access opcodes #889

Conversation

cgewecke commented Sep 23, 2020 • edited Loading

Background

Specification Details

Initialization

Address Read Costs

Storage Read and Store Costs

codecov bot commented Sep 23, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

cgewecke Sep 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgewecke commented Sep 28, 2020 • edited Loading

holgerd77 commented Sep 30, 2020

holgerd77 commented Sep 30, 2020

cgewecke commented Sep 30, 2020

holgerd77 commented Oct 20, 2020

cgewecke commented Oct 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holgerd77 commented Oct 20, 2020

cgewecke commented Oct 20, 2020

holgerd77 left a comment

Choose a reason for hiding this comment

jochem-brouwer commented Oct 23, 2020

holgerd77 commented Oct 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jochem-brouwer Feb 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgewecke commented Sep 23, 2020 •

edited

Loading

codecov bot commented Sep 23, 2020 •

edited

Loading

cgewecke Sep 24, 2020 •

edited

Loading

cgewecke commented Sep 28, 2020 •

edited

Loading

jochem-brouwer Feb 20, 2021 •

edited

Loading