VM: Generate Access Lists in runTx() #1170

holgerd77 · 2021-03-27T12:36:54Z

Ok, here is the access list generation PR fixing #1152. This needs significantly more tests, but this should generally do what is expected. I just tried to generate the following super-simple access list with this:

{
  gasUsed: <BN: 52f8>,
  execResult: {
    gasUsed: <BN: 0>,
    exceptionError: undefined,
    returnValue: <Buffer >,
    gasRefund: <BN: 0>
  },
  bloom: Bloom {
    bitvector: <Buffer 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ... 206 more bytes>
  },
  amountSpent: <BN: 2068e0>,
  accessList: [
    {
      address: 'be862ad9abfe6f22bcb087716c7d89a26051f74c',
      storageKeys: []
    }
  ]
}

Which obviously is not enough yet. 😄 Functionality for more complex AL is there but should be manually tested or optimally by adding additional test cases.

For now I've added the functionality to StateManger.clearWarmedAccounts() in a non-breaking way. On a second thought along pushing I rather think it might be better and more flexible to just add a distinct method for that which can be called separately (which then still can be called somewhat along the clearing-call in our context).

I also added a new helper function Address.isPrecompileOrSystemAddress() to Util which checks an address to be a precompile or system address following EIP-1352.

codecov · 2021-03-27T12:38:51Z

Codecov Report

Merging #1170 (95c9522) into master (71c54f6) will decrease coverage by 0.07%.
The diff coverage is 60.00%.

Flag	Coverage Δ
block	`81.98% <ø> (+0.17%)`	⬆️
blockchain	`84.16% <ø> (ø)`
client	`84.05% <ø> (+0.01%)`	⬆️
common	`86.87% <ø> (ø)`
devp2p	`83.88% <ø> (-0.25%)`	⬇️
ethash	`82.08% <ø> (ø)`
tx	`84.25% <100.00%> (+0.19%)`	⬆️
vm	`81.69% <55.55%> (-0.66%)`	⬇️

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

…ss() function

jochem-brouwer · 2021-03-27T14:18:57Z

packages/util/test/address.spec.ts

@@ -75,6 +75,14 @@ tape('Address', (t) => {
    st.end()
  })

+  t.test('should provide correct precompile check', (st) => {
+    const precompile = Address.fromString('0x0000000000000000000000000000000000000009')
+    st.true(precompile.isPrecompileOrSystemAddress(), 'should detect precompile address')


Hmm this EIP is actually a little bit confusing. It does specify which address range could be assumed to be system precompiles. However, for access lists, we are only allowed to push precompiles which actually have code to the access lists. There was a bug in Besu where they also included BLS addresses to the access lists, which does not match the EIP. What is weird is that EIP2930 in combination with EIP1352 is not very consistent.

Hmm, I'm just using this to remove precompile addresses, so there should be no problem? 🤔

"which actually have code to the access lists"

What do you mean by that actually? 😀

It means that once we setup the initial access list, we add the caller and the target to the access list, but we also add every precompile which has attached code to it. So, Besu made a mistake and also added the BLS precompile addresses. However, in Berlin, we do not have BLS, so there is no precompiled code at these BLS addresses and they should thus not be added to the access lists.

jochem-brouwer · 2021-03-27T14:21:14Z

packages/vm/lib/runTx.ts

@@ -123,6 +142,10 @@ export default async function runTx(this: VM, opts: RunTxOpts): Promise<RunTxRes
    const result = await _runTx.bind(this)(opts)
    await state.commit()
    debug(`tx checkpoint committed`)
+    if (this._common.isActivatedEIP(2929) && opts.reportAccessList) {
+      result.accessList = state.clearWarmedAccounts(true)


I know this is WIP, but this currently only supports the accessed items for the root transaction right? It will not report access lists if you *CALL* into another address.

Hmm it might actually report it, but it will not report if we REVERT one of these inner calls, since that removes the accessed items from warmed storage.

Hmm, does this happen very often in practice and do you think it's an important miss?

(I can add extra logic for it though)

Well, internal reverts sometimes happen, and if you don't put these storage slots on the access lists then you are going to get charged to touch a cold account multiple times. So if we want to generate "correct" access lists (we want to be friendly and provide the complete list of the slots and accounts we are going to add in the access list), then we should also include the reverted slots.

This is actually a bit mind-boggling... There is an edge case here. What if an internal call reverts, but if we add it to the access list then it does not revert? This might also change the behavior of the call; we might now touch other slots. I think we could try to rebuild the access lists based upon whatever we just touched, but I think we might get in a flip/flop situation where each time you edit the access list, you get another access list which we already considered before 🤔 (Hope this reasoning makes sense)

I will ask on R&D

Checkout geth's implementation; ethereum/go-ethereum#22550

They cap the count of attempts to generate an access list at 10.

… own method generateAccessList (not part of interface yet)

…eration logic, unified folding logic, add note on AL generation edge case

holgerd77 · 2021-03-29T10:05:30Z

Hi @jochem-brouwer, thanks for digging in so deeply here, really cool! 😄 I won't do this edge case implementation here since I simply don't have the development resources for this given all our other tasks atm, instead I've added a note here along the implementation. Since this is not breaking anything if an access list might not be complete on some edge cases (+ also this 10 -run-limit from geth seems also conceptually be rather a practical solution than a 100%-correct implementation) and it is not yet clear how much this whole access list feature will be used, I think this should be acceptable.

I have added the additional structure though for revert(), also did some further functional cleanup on the folding logic in the StateManager and added a set of new tests dedicatedly for StateManager.createAccessList().

Ah, yes, and this is now a separate function, haven't added to the official interface yet though for compatibility reasons. Instead runTx() (with reportAccessList option activated) will throw if used with a custom state manager not implementing this method.

Ok, ready for review. 😄

holgerd77 · 2021-03-29T16:15:05Z

Hi @ryanio, could you have a look at the latest VM API test run, which is failing for the karma browser run? I could just add the examples folder to the exclude directive and also add this downlevelIteration compiler option, but I guess you are better at judging the overall situation and see why this might be happening in the first place and if I e.g. introduced some structures which are problematic from a browser perspective or something.

jochem-brouwer · 2021-03-29T16:26:38Z

packages/vm/lib/state/stateManager.ts

-    this._accessedStorage.pop()
+    const lastItem = this._accessedStorage.pop()
+    if (lastItem) {
+      this._accessedStorageReverted.push(lastItem)


This will degrade performance, right? If we don't want to get the access lists, then we also should not copy these items since it is unnecessary?

Ah, yes, you are right. With this openly usable generateAccessList() function we might nevertheless want to leave as-is, but haven't thought about this aspect yes.

jochem-brouwer · 2021-03-29T16:27:40Z

packages/vm/lib/state/stateManager.ts

+      if (!address.isPrecompileOrSystemAddress()) {
+        const storageSlots = Array.from(slots)
+          .map((s) => `0x${s}`)
+          .sort()


Sorting this is fine, but it is not really necessary, right?

I would be a fan of sorting (even if it comes to some performance costs), I think this gives a better overview if one want to manually check the outcome. And I guess in this use case performance is a bit secondary (just an unproven assumption reflecting on how I imagine people using this stuff).

jochem-brouwer

Some general remarks, looks great in general, nice tests 👍

…e test must have been removed, as one singular test takes 3 tests)

ryanio

lgtm!

jochem-brouwer · 2021-03-30T13:51:43Z

Question here, do the reported access lists sometimes also report the sender address or the to address (or a precompile)? Because these should not be included on the access lists, they are already added to the lists internally by default.

holgerd77 · 2021-03-30T15:27:52Z

@jochem-brouwer ah, good catch, yes, I assume these are still in. Will put the release PR on "blocked" and open a subsequent PR soon.

holgerd77 · 2021-03-30T20:15:15Z

@jochem-brouwer ah, actually to is likely not a good candidate to remove since there might be storage slots added as well? 🤔 Would it be a problem if we then just keep both of the addresses in?

jochem-brouwer · 2021-03-30T20:21:31Z

Ah you are right. We only don't want to put to in if the storage slots there are empty. But I do not think there is a situation where this can occur. It is not really a problem if we would put the to address in with no slots, but that means the user has to pay more gas than necessary.

holgerd77 added type: feature PR state: WIP prio: P3 important package: vm package: util labels Mar 27, 2021

holgerd77 added 2 commits March 27, 2021 13:42

util: added new EIP-1352 conformant Address.isPrecompileOrSystemAddre…

5099e3a

…ss() function

vm: added reportAccessList option to runTx(), added simple test

aaecc9b

holgerd77 force-pushed the vm-access-list-generation branch from 7547637 to aaecc9b Compare March 27, 2021 12:42

jochem-brouwer reviewed Mar 27, 2021

View reviewed changes

holgerd77 mentioned this pull request Mar 29, 2021

Breaking Releases - Meta Issue (VM v6, others) (old) #1024

Closed

50 tasks

holgerd77 added 3 commits March 29, 2021 09:42

vm: separated StateManager accessList generation functionality to its…

e8c4527

… own method generateAccessList (not part of interface yet)

vm -> access lists: added storage maps from REVERT to access list gen…

b4787ca

…eration logic, unified folding logic, add note on AL generation edge case

vm -> access lists: added StateManager.generateAccessList() API tests

e2b8f64

holgerd77 added PR state: needs review and removed PR state: WIP labels Mar 29, 2021

holgerd77 requested review from jochem-brouwer, ryanio and cgewecke March 29, 2021 09:56

vm -> access lists: add sorting to StateManager.generateAccessList()

06ff988

holgerd77 mentioned this pull request Mar 29, 2021

VM: Add vm.buildBlock() and BlockBuilder API #1158

Merged

jochem-brouwer reviewed Mar 29, 2021

View reviewed changes

ryanio and others added 3 commits March 29, 2021 09:49

karma: fix example, use tsconfig

d36e90f

update homestead test count from 7004 to 7001 to fix nightly runs (on…

a60a80b

…e test must have been removed, as one singular test takes 3 tests)

Merge branch 'master' into vm-access-list-generation

95c9522

ryanio approved these changes Mar 30, 2021

View reviewed changes

ryanio merged commit 71e0f67 into master Mar 30, 2021

ryanio deleted the vm-access-list-generation branch March 30, 2021 02:43

holgerd77 mentioned this pull request Mar 30, 2021

VM: generate EIP-2930 Access Lists #1152

Closed

holgerd77 mentioned this pull request Mar 30, 2021

VM v5.3.0 (Access List Gen & Block Builder) / Util v7.0.10 / Tx v3.1.2 Releases #1174

Merged

holgerd77 mentioned this pull request Feb 16, 2022

v6 Breaking Release Planning #1717

Closed

51 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VM: Generate Access Lists in runTx() #1170

VM: Generate Access Lists in runTx() #1170

holgerd77 commented Mar 27, 2021

codecov bot commented Mar 27, 2021 •

edited

jochem-brouwer Mar 27, 2021

holgerd77 Mar 27, 2021

holgerd77 Mar 29, 2021

jochem-brouwer Mar 29, 2021

jochem-brouwer Mar 27, 2021

jochem-brouwer Mar 27, 2021

holgerd77 Mar 27, 2021

holgerd77 Mar 27, 2021

jochem-brouwer Mar 27, 2021

jochem-brouwer Mar 27, 2021

jochem-brouwer Mar 27, 2021

holgerd77 commented Mar 29, 2021

holgerd77 commented Mar 29, 2021

jochem-brouwer Mar 29, 2021

holgerd77 Mar 29, 2021

jochem-brouwer Mar 29, 2021

holgerd77 Mar 29, 2021

jochem-brouwer left a comment

ryanio left a comment

jochem-brouwer commented Mar 30, 2021

holgerd77 commented Mar 30, 2021

holgerd77 commented Mar 30, 2021

jochem-brouwer commented Mar 30, 2021

VM: Generate Access Lists in runTx() #1170

VM: Generate Access Lists in runTx() #1170

Conversation

holgerd77 commented Mar 27, 2021

codecov bot commented Mar 27, 2021 • edited

Codecov Report

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holgerd77 commented Mar 29, 2021

holgerd77 commented Mar 29, 2021

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

jochem-brouwer left a comment

Choose a reason for hiding this comment

ryanio left a comment

Choose a reason for hiding this comment

jochem-brouwer commented Mar 30, 2021

holgerd77 commented Mar 30, 2021

holgerd77 commented Mar 30, 2021

jochem-brouwer commented Mar 30, 2021

codecov bot commented Mar 27, 2021 •

edited