evm: verkle updates #3832

gabrocheleau · 2025-01-12T00:42:49Z

This PR extracts some Verkle-related updates and fixes from #3716:

Adds a systemVerkleAccessWiness to the EVM class.
Adds a codeHash read access when creating a contract (for collision checking)
Adds conditions that skips some accesses when interacting with system contracts and precompiles, as specified by the EIP-4762 EIP.
Clarifies and fixes code chunk handling in the statefulVerkleStateManager
Adjusts some miner/account reward logic so that it uses the systemVerkleAccessWitness and not the tx-level verkleAccessWitness, and skips access witness for miner when reward is 0

codecov · 2025-01-12T00:45:19Z

Codecov Report

Attention: Patch coverage is 29.03226% with 44 lines in your changes missing coverage. Please review.

Project coverage is 75.65%. Comparing base (e15b73b) to head (dd2e850).
Report is 1 commits behind head on master.

Additional details and impacted files

Flag	Coverage Δ
block	`73.74% <ø> (ø)`
blockchain	`83.23% <ø> (ø)`
client	`73.70% <ø> (?)`
common	`89.82% <0.00%> (ø)`
devp2p	`71.95% <ø> (ø)`
evm	`64.95% <15.00%> (-0.09%)`	⬇️
genesis	`100.00% <ø> (ø)`
mpt	`52.30% <ø> (+0.18%)`	⬆️
rlp	`95.11% <ø> (ø)`
statemanager	`67.64% <37.50%> (-0.13%)`	⬇️
tx	`76.56% <ø> (ø)`
util	`72.81% <ø> (ø)`
vm	`56.97% <36.00%> (-0.21%)`	⬇️
wallet	`79.67% <ø> (ø)`

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

acolytec3

Can you be more explicit about what these changes are targeting? If I understand it correctly, you're dealing with the following items:

Checking to see if an accessed address is a system contract/precompile so we don't charge AccessWitness read for the account header fields
Verifying that we only add the miner to the write portion of the System Verkle Witness if the miner actually get a reward for the transaction

Is that correct or is there more being covered here?

gabrocheleau · 2025-01-12T16:18:38Z

Can you be more explicit about what these changes are targeting? If I understand it correctly, you're dealing with the following items:

Checking to see if an accessed address is a system contract/precompile so we don't charge AccessWitness read for the account header fields

Verifying that we only add the miner to the write portion of the System Verkle Witness if the miner actually get a reward for the transaction

Is that correct or is there more being covered here?

Just clarified the description to be more explicit about the changes

acolytec3 · 2025-01-13T14:15:07Z

packages/evm/src/opcodes/gas.ts

@@ -166,7 +166,8 @@ export const dynamicGasHandlers: Map<number, AsyncDynamicGasHandler | SyncDynami
        let charge2929Gas = true
        if (
          common.isActivatedEIP(6800) &&
-          runState.interpreter._evm.getPrecompile(address) === undefined
+          runState.interpreter._evm.getPrecompile(address) === undefined &&
+          !address.equals(createAddressFromStackBigInt(common.param('systemAddress')))


My only concern with this is that should consider the larger picture, mentioned by @g11tech, that it makes sense to store system contract address, codehash, and code, in memory the way we do for precompiles. I think we should go ahead and merge this as is but then do some work to make the system contract (plus any future ones) available in memory so we don't need to read code from storage for them.

yea we need to address this both for system calls which will have address equal to system address as well as normal contract calls into system contracts

(same comment for similar handling in below changes)

acolytec3 · 2025-01-13T14:28:13Z

packages/vm/src/runBlock.ts

+    if (common.isActivatedEIP(6800) === true && reward !== BIGINT_0) {
+      if (evm.systemVerkleAccessWitness === undefined) {
        throw Error(`verkleAccessWitness required if verkle (EIP-6800) is activated`)
      }
-      evm.verkleAccessWitness.readAccountHeader(address)
+      evm.systemVerkleAccessWitness.writeAccountHeader(address)
    }
    account = new Account()
  }
  account.balance += reward
  await evm.journal.putAccount(address, account)

-  if (common.isActivatedEIP(6800) === true) {
-    if (evm.verkleAccessWitness === undefined) {
+  if (common.isActivatedEIP(6800) === true && reward !== BIGINT_0) {
+    if (evm.systemVerkleAccessWitness === undefined) {
      throw Error(`verkleAccessWitness required if verkle (EIP-6800) is activated`)
    }
    // use vm utility to build access but the computed gas is not charged and hence free
-    evm.verkleAccessWitness.writeAccountBasicData(address)
-    evm.verkleAccessWitness.readAccountCodeHash(address)
+    evm.systemVerkleAccessWitness.writeAccountBasicData(address)
+    evm.systemVerkleAccessWitness.readAccountCodeHash(address)


Can we consolidate all this down? Instead of having a whole code block to check to write the account header when we have account non-existence and then a second check, can we just do (this is pseudo code but you get the idea)

let accountExists = true if (account === undefined) { accountExists = false account = new Account() } if (6800 is active and reward > 0) accountExists ? writeAccountData/readCodHash : writeAccountHeader

Yeah thanks, this makes total sense and is simpler + more efficient

acolytec3

This looks good. I'm going to test out my proposed simplification of the miner address AW in runTx logic locally and it the tests are still passing the same way, will add a commit so we can merge.

acolytec3

LGTM. Please review my last commit before merging

g11tech · 2025-01-13T16:26:50Z

packages/evm/src/opcodes/EIP2929.ts

+    } else if (chargeGas && common.isActivatedEIP(6800)) {
+      // If Verkle is active, then the warmstoragereadGas should still be charged
+      // This is because otherwise opcodes will have cost 0 (this is thus the base fee)
+      return common.param('warmstoragereadGas')


we have a check for warm in L26, so this should just be cold gas segment no?

g11tech · 2025-01-13T16:32:32Z

packages/statemanager/src/statefulVerkleStateManager.ts

@@ -318,7 +318,7 @@ export class StatefulVerkleStateManager implements StateManagerInterface {
      chunkStems[0],
      chunkSuffixes.slice(
        0,
-        codeChunks.length <= VERKLE_CODE_OFFSET ? codeChunks.length : VERKLE_CODE_OFFSET,
+        chunkSuffixes.length <= VERKLE_CODE_OFFSET ? chunkSuffixes.length : VERKLE_CODE_OFFSET,


is there a difference in code chunks and suffixes length?

No, but it seemed more correct to actually use chunkSuffixes for this slice rather than implicitly relying that codeChunks.length === codeSuffixes.length

evm: verkle updates

019b6b9

gabrocheleau added PR state: needs review package: vm package: evm labels Jan 12, 2025

vm: uncomment runBlock

086d0c1

acolytec3 reviewed Jan 12, 2025

View reviewed changes

vm: fix historical blockhash handling

0e4c162

gabrocheleau requested a review from acolytec3 January 13, 2025 11:54

acolytec3 reviewed Jan 13, 2025

View reviewed changes

verkle stuffs

dd2e850

acolytec3 approved these changes Jan 13, 2025

View reviewed changes

g11tech reviewed Jan 13, 2025

View reviewed changes

gabrocheleau merged commit 15994b9 into master Jan 13, 2025
40 of 41 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

evm: verkle updates #3832

evm: verkle updates #3832

gabrocheleau commented Jan 12, 2025 •

edited

Loading

codecov bot commented Jan 12, 2025 •

edited

Loading

acolytec3 left a comment

gabrocheleau commented Jan 12, 2025

acolytec3 Jan 13, 2025

g11tech Jan 13, 2025 •

edited

Loading

acolytec3 Jan 13, 2025

gabrocheleau Jan 13, 2025

acolytec3 left a comment

acolytec3 left a comment

g11tech Jan 13, 2025

g11tech Jan 13, 2025

gabrocheleau Jan 13, 2025

evm: verkle updates #3832

evm: verkle updates #3832

Conversation

gabrocheleau commented Jan 12, 2025 • edited Loading

codecov bot commented Jan 12, 2025 • edited Loading

Codecov Report

acolytec3 left a comment

Choose a reason for hiding this comment

gabrocheleau commented Jan 12, 2025

acolytec3 Jan 13, 2025

Choose a reason for hiding this comment

g11tech Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

acolytec3 Jan 13, 2025

Choose a reason for hiding this comment

gabrocheleau Jan 13, 2025

Choose a reason for hiding this comment

acolytec3 left a comment

Choose a reason for hiding this comment

acolytec3 left a comment

Choose a reason for hiding this comment

g11tech Jan 13, 2025

Choose a reason for hiding this comment

g11tech Jan 13, 2025

Choose a reason for hiding this comment

gabrocheleau Jan 13, 2025

Choose a reason for hiding this comment

gabrocheleau commented Jan 12, 2025 •

edited

Loading

codecov bot commented Jan 12, 2025 •

edited

Loading

g11tech Jan 13, 2025 •

edited

Loading