cmd, core, params, trie: Add verkle access witness #29338

gballet · 2024-03-25T20:21:26Z

This is a rewrite of #29234 that uses the simplified approach suggested by @holiman

gballet · 2024-03-27T10:35:07Z

core/state_processor.go

@@ -78,7 +78,7 @@ func (p *StateProcessor) Process(block *types.Block, statedb *state.StateDB, cfg
 		signer  = types.MakeSigner(p.config, header.Number, header.Time)
 	)
 	if beaconRoot := block.BeaconRoot(); beaconRoot != nil {
-		ProcessBeaconBlockRoot(*beaconRoot, vmenv, statedb)
+		ProcessBeaconBlockRoot(*beaconRoot, vmenv, statedb, header.Number, header.Time)


vmenv actually has an unexported rules field that I would like to be able to access instead of having to pass extra headers. Would that be an option?

Yeah could either make the chainRules accessible, or use a vmenv.ChainConfig().Rules(header.Number, header.Time) new instance

core/state/access_witness.go

holiman · 2024-03-27T10:46:18Z

core/state/access_witness.go

+	for i := utils.VersionLeafKey; i <= utils.CodeSizeLeafKey; i++ {
+		gas += aw.touchAddressAndChargeGas(addr, zeroTreeIndex, byte(i), isWrite)
+	}


This little thing iterates form VersionLeafKey to CodeSizeLeafKey, it's not apparent to me why this does so.In TouchAndChageMessageCall, we only use those two, not the things in between. Please add some comments

core/state/access_witness.go

holiman · 2024-03-27T10:50:00Z

consensus/beacon/consensus.go

@@ -360,6 +360,10 @@ func (beacon *Beacon) Finalize(chain consensus.ChainHeaderReader, header *types.
 		amount := new(uint256.Int).SetUint64(w.Amount)
 		amount = amount.Mul(amount, uint256.NewInt(params.GWei))
 		state.AddBalance(w.Address, amount, tracing.BalanceIncreaseWithdrawal)
+
+		if chain.Config().IsEIP4762(header.Number, header.Time) {
+			state.Witness().TouchFullAccount(w.Address[:], true)


WHy, nonce is not needed here, it's just a balance increase?

Have the same feeling.

As account fields are stored in different position in leafNode. The state access granularity is actually a field. It brings lots of complexity for gas metering.

Nothing to against the implementation, just want to mention that gas metering could be wrong very easy.

holiman · 2024-03-27T10:56:31Z

core/state/access_witness.go

+	return gas
+}
+
+func (aw *AccessWitness) TouchTxOriginAndComputeGas(originAddr []byte) uint64 {


All of these implicitly ComputeGas, no? But some have it in the name and some don't, so just drop it imo

indeed some don't, I changed their names and the API to reflect that.

holiman · 2024-03-27T10:59:53Z

core/vm/instructions.go

@@ -382,8 +390,23 @@ func opExtCodeCopy(pc *uint64, interpreter *EVMInterpreter, scope *ScopeContext)
 		uint64CodeOffset = math.MaxUint64
 	}
 	addr := common.Address(a.Bytes20())
-	codeCopy := getData(interpreter.evm.StateDB.GetCode(addr), uint64CodeOffset, length.Uint64())
-	scope.Memory.Set(memOffset.Uint64(), length.Uint64(), codeCopy)
+	if interpreter.evm.chainRules.IsEIP4762 {


I'd prefer if you make opExtCodeCopyVerkle and put in eips. That way we know that the verkle-things do not interfere with pre-verkle, not even adding an extra if-clause check. Same for other changed ops in this file

it's not great, because if any operation is changed between the time I copy the code and the time the Osaka fork is activated, there is a regression risk. I would rather try to move it to a dynamic gas function if I can.

and who knows how these functions, especially the introspecting ones, are going to change when/if EOF is added to Prague and the spec finalized.

right, I guess I could just call the previous handler and just add the extra gas costs, to avoid regressions.

fjl · 2024-04-02T12:54:00Z

core/vm/evm.go

 		} else {
-			err = ErrCodeStoreOutOfGas
+			// Contract creation completed, touch the missing fields in the contract
+			if !contract.UseGas(evm.Accesses.TouchFullAccount(address.Bytes()[:], true), evm.Config.Tracer, tracing.GasChangeUnspecified) {


Probably needs a new GasChange reason here

core/state/access_witness.go

rjl493456442 · 2024-04-03T07:00:07Z

consensus/beacon/consensus.go

@@ -360,6 +360,10 @@ func (beacon *Beacon) Finalize(chain consensus.ChainHeaderReader, header *types.
 		amount := new(uint256.Int).SetUint64(w.Amount)
 		amount = amount.Mul(amount, uint256.NewInt(params.GWei))
 		state.AddBalance(w.Address, amount, tracing.BalanceIncreaseWithdrawal)
+
+		if chain.Config().IsEIP4762(header.Number, header.Time) {
+			state.Witness().TouchFullAccount(w.Address[:], true)


Have the same feeling.

As account fields are stored in different position in leafNode. The state access granularity is actually a field. It brings lots of complexity for gas metering.

Nothing to against the implementation, just want to mention that gas metering could be wrong very easy.

rjl493456442 · 2024-04-03T07:01:05Z

core/chain_makers.go

@@ -102,7 +102,7 @@ func (b *BlockGen) SetParentBeaconRoot(root common.Hash) {
 		blockContext = NewEVMBlockContext(b.header, b.cm, &b.header.Coinbase)
 		vmenv        = vm.NewEVM(blockContext, vm.TxContext{}, b.statedb, b.cm.config, vm.Config{})
 	)
-	ProcessBeaconBlockRoot(root, vmenv, b.statedb)
+	ProcessBeaconBlockRoot(root, vmenv, b.statedb, b.Number(), b.Timestamp())


Please replace it with github.com/ethereum/go-verkle

not sure what you mean here, this is what is already happening line 35? 🤔

core/state/access_witness.go

core/state/statedb.go

rjl493456442 · 2024-04-03T07:05:45Z

core/state_processor.go

-	statedb.AddAddressToAccessList(params.BeaconRootsAddress)
+	txctx := NewEVMTxContext(msg)
+	vmenv.Reset(txctx, statedb)
+	if vmenv.ChainConfig().Rules(num, true, time).IsEIP2929 {


Shouldn't it be IsEIP4762?

nope, access lists are a concept of 2929 that is deactivated with 4762

Is there any particular reason why you are placing all these calls to AccessList within isEIP2929 clauses?
I mean, you could just let the call happen, couldn't you?

I could, but it's not supposed to happen (even if it has no obvious impact afaict)

rjl493456442 · 2024-04-03T07:08:09Z

core/state_transition.go

+
+		// add the coinbase to the witness iff the fee is greater than 0
+		if rules.IsEIP4762 && fee.Sign() != 0 {
+			st.evm.Accesses.TouchFullAccount(st.evm.Context.Coinbase[:], true)


Shouldn't we only mutate the balance field?

core/vm/evm.go

holiman · 2024-04-25T13:29:14Z

core/state_processor.go

@@ -185,7 +190,7 @@ func ApplyTransaction(config *params.ChainConfig, bc ChainContext, author *commo

 // ProcessBeaconBlockRoot applies the EIP-4788 system call to the beacon block root
 // contract. This method is exported to be used in tests.
-func ProcessBeaconBlockRoot(beaconRoot common.Hash, vmenv *vm.EVM, statedb *state.StateDB) {
+func ProcessBeaconBlockRoot(beaconRoot common.Hash, vmenv *vm.EVM, statedb *state.StateDB, num *big.Int, time uint64) {


And this could just take the params.Rules, couldn't it?

holiman · 2024-04-25T13:34:35Z

core/state_transition.go

+		if msg.To != nil {
+			st.evm.Accesses.AddTxDestination(targetAddr.Bytes(), msg.Value.Sign() != 0)


Why is this needed? We're going to Call into that address, shouldn't that add it later on?

because when it comes to witnesses, these costs are paid for the 21000 gas. All other calls are paid for. If this is moved to the Call, I have to add an if somewhere to distinguish between the top-level call, and any other, nested call.

params/config.go

Co-authored-by: Ignacio Hagopian <jsign.uy@gmail.com>

Co-authored-by: Martin HS <martin@swende.se>

core/vm/evm.go

holiman · 2024-04-29T12:51:05Z

core/state/access_events.go

+func (aw *AccessEvents) VersionGas(addr []byte, isWrite bool) uint64 {
+	return aw.touchAddressAndChargeGas(addr, zeroTreeIndex, utils.VersionLeafKey, isWrite)
+}
+
+func (aw *AccessEvents) BalanceGas(addr []byte, isWrite bool) uint64 {
+	return aw.touchAddressAndChargeGas(addr, zeroTreeIndex, utils.BalanceLeafKey, isWrite)
+}
+
+func (aw *AccessEvents) NonceGas(addr []byte, isWrite bool) uint64 {
+	return aw.touchAddressAndChargeGas(addr, zeroTreeIndex, utils.NonceLeafKey, isWrite)
+}
+
+func (aw *AccessEvents) CodeSizeGas(addr []byte, isWrite bool) uint64 {
+	return aw.touchAddressAndChargeGas(addr, zeroTreeIndex, utils.CodeSizeLeafKey, isWrite)
+}
+
+func (aw *AccessEvents) CodeHashGas(addr []byte, isWrite bool) uint64 {
+	return aw.touchAddressAndChargeGas(addr, zeroTreeIndex, utils.CodeKeccakLeafKey, isWrite)
+}


All of these needs docstrings. What assumptions are used here, regarding the caller? For example, is it "ok" if the caller first invokes BalanceGas(.., false) then BalanceGas(.., true)?
What are the side-effects?

For example. in beacon consensus engine, this method is invoked:

state.AccessEvents().BalanceGas(w.Address[:], true)

It's not clear to me why. What are the side-effects of that invocation? Because clearly we're not concerned about the gas cost.

All of these needs docstrings. What assumptions are used here, regarding the caller? For example, is it "ok" if the caller first invokes BalanceGas(.., false) then BalanceGas(.., true)? What are the side-effects?

The previous version of this PR was more explicit, it defined ReadWriteMode and so this is what things are: if a location is accessed in "read" mode, the price is cheaper than if they are accessed in "write" mode.

core/state/access_events.go

holiman · 2024-04-29T12:55:23Z

core/state/access_events.go

+	naw := &AccessEvents{
+		branches:   make(map[branchAccessKey]mode),
+		chunks:     make(map[chunkAccessKey]mode),
+		pointCache: aw.pointCache,


Whoa, you just copy the pointcache so the original and the copy now uses the same instance? Is that safe?

core/state/access_events.go

holiman · 2024-04-29T13:00:00Z

core/state_processor.go

@@ -120,6 +120,7 @@ func ApplyTransactionWithEVM(msg *Message, config *params.ChainConfig, gp *GasPo
 	}
 	// Create a new context to be used in the EVM environment.
 	txContext := NewEVMTxContext(msg)
+	txContext.AccessEvents = statedb.NewAccessEvents()


Shouldn't this only be set if verkle is active?

Also, inside of evm.Reset, the txContext.AccessEvents will also be initialized. Is it a bit redundant? Or some mistakes here?

it's not a mistake, one needs to start with an empty access event list when starting a conversion.

core/state_transition.go

core/state/access_events.go

core/vm/operations_verkle.go

core/state_processor.go

rjl493456442 · 2024-04-29T13:56:44Z

core/state_processor.go

@@ -120,6 +120,7 @@ func ApplyTransactionWithEVM(msg *Message, config *params.ChainConfig, gp *GasPo
 	}
 	// Create a new context to be used in the EVM environment.
 	txContext := NewEVMTxContext(msg)
+	txContext.AccessEvents = statedb.NewAccessEvents()


Also, inside of evm.Reset, the txContext.AccessEvents will also be initialized. Is it a bit redundant? Or some mistakes here?

core/state_transition.go

core/vm/evm.go

Co-authored-by: Martin HS <martin@swende.se>

* params: fix chainconfig rules * core, core/state: init accessEvent for each transactin * core/state: reuse point cache between blocks

This reverts commit aa96a9c.

Co-authored-by: Gary Rong <garyrong0905@gmail.com>

core/vm/jump_table.go

core/vm/interpreter.go

rjl493456442 · 2024-05-06T06:45:59Z

core/vm/gas_table.go

@@ -402,6 +402,15 @@ func gasCall(evm *EVM, contract *Contract, stack *Stack, mem *Memory, memorySize
 	if gas, overflow = math.SafeAdd(gas, evm.callGasTemp); overflow {
 		return 0, ErrGasUintOverflow
 	}
+	if evm.chainRules.IsEIP4762 {


I think it's wrong here.

In EIp158, we introduce a rule that at most 63/64 gas could be used in a inner call, therefore the gas for CALL opcode should be fully computed, and then 1/64 remaining gas will be deducted for this rule.

However in this pull request, the verkle specific gas is deducted after this rule. It's a behavioral change.

suggested change:

- if transfersValue && !evm.chainRules.IsEIP4762 { + if transfersValue { gas += params.CallValueTransferGas } memoryGas, err := memoryGasCost(mem, memorySize) @@ -394,7 +394,14 @@ func gasCall(evm *EVM, contract *Contract, stack *Stack, mem *Memory, memorySize if gas, overflow = math.SafeAdd(gas, memoryGas); overflow { return 0, ErrGasUintOverflow } - + if evm.chainRules.IsEIP4762 && transfersValue { + // Deduct the gas cost for value transfers according to the legacy rules. + gas -= params.CallValueTransferGas + gas, overflow = math.SafeAdd(gas, evm.AccessEvents.ValueTransferGas(contract.Address().Bytes()[:], address.Bytes()[:])) + if overflow { + return 0, ErrGasUintOverflow + } + } evm.callGasTemp, err = callGas(evm.chainRules.IsEIP150, contract.Gas, gas, stack.Back(0)) if err != nil { return 0, err @@ -402,15 +409,6 @@ func gasCall(evm *EVM, contract *Contract, stack *Stack, mem *Memory, memorySize if gas, overflow = math.SafeAdd(gas, evm.callGasTemp); overflow { return 0, ErrGasUintOverflow } - if evm.chainRules.IsEIP4762 { - if transfersValue { - gas, overflow = math.SafeAdd(gas, evm.AccessEvents.ValueTransferGas(contract.Address().Bytes()[:], address.Bytes()[:])) - if overflow { - return 0, ErrGasUintOverflow - } - } - }

so for the part where the sent value is concerned, this is the correct behavior:

I had to think about the other part a little, but I am now convinced that you are correct.

core/vm/gas_table.go

core/vm/eips.go

core/state/access_events.go

core/state_processor.go

Co-authored-by: Gary Rong <garyrong0905@gmail.com>

holiman

One thing that bothers me a bit, with how the access events are designed: you do e.g. BalanceGas, which does two things:

Adds it to the access events
Returns how much it cost

This means that the evm cannot first check if the cost is covered, and then proceed (or not). As it is, the effective gas cost is the "available gas", not the cost.

This is problematic in the following case.
1.Y: Call contract X with near-zero gas
2. X: load slot 1
3. X: The load operation fails because insufficient gas for adding to witness
4.Y: Call contract X with near-zero gas
5. X: load slot 2
6. X: The load operation fails because insufficient gas for adding to witness
7. repeat

The end result is that we've added N slots to the witness, bloating it, at the cost of ~100 (the cost of a call to a warm address, plus a little more). Whereas the 'true' cost would have been something like N x ( (WitnessBranchReadCost @ 1900) + (WitnessChunkReadCost @ 200))

Now, the same is true for a few other ops, notably the access lists. However, the access lists are journalled, which makes this attack moot. This can be fixed, if the design is changed so that either

The methods to "add and return cost" takes a availableGas parameter, and aborts if the cost is higher than that
The methods to "add and return cost" is split into two: return cost and add, where the first is pure, the second just executes the change without any checks.

core/state/access_events.go

holiman

All in all, I'm fairly confident that this does not break existing code. Did you run it on any of the benchmarkers in prod at all? Or somewhere else.. ?

holiman · 2024-05-07T05:36:37Z

core/vm/evm.go

-		createDataGas := uint64(len(ret)) * params.CreateDataGas
-		if contract.UseGas(createDataGas, evm.Config.Tracer, tracing.GasChangeCallCodeStorage) {
-			evm.StateDB.SetCode(address, ret)
+		if !evm.chainRules.IsEIP4762 {


IMO we should rename IsEIP4762 to IsVerkle, to make things easier for a casual reader

I could, but here's the thing: EIP4762 and verkle are two separate things, eip4762 pertains to statelessness while verkle is a specific implementation of statelessness (i.e. if for whatever reason we switch to another state tree later, eip4762 will still apply).

Co-authored-by: Martin HS <martin@swende.se>

gballet requested review from karalabe, holiman and rjl493456442 as code owners March 25, 2024 20:21

gballet added the status:triage label Mar 26, 2024

gballet force-pushed the rewritten-eip4762-into-master branch 2 times, most recently from 8373191 to d4019ed Compare March 27, 2024 08:36

gballet commented Mar 27, 2024

View reviewed changes

holiman reviewed Mar 27, 2024

View reviewed changes

fjl reviewed Apr 2, 2024

View reviewed changes

rjl493456442 reviewed Apr 3, 2024

View reviewed changes

gballet mentioned this pull request Apr 15, 2024

List of verkle PRs to be reviewed #29537

Open

2 tasks

gballet force-pushed the rewritten-eip4762-into-master branch from 2044918 to 7de652d Compare April 24, 2024 19:58

gballet requested a review from s1na as a code owner April 24, 2024 19:58

holiman reviewed Apr 25, 2024

View reviewed changes

params/config.go Outdated Show resolved Hide resolved

gballet and others added 12 commits April 26, 2024 15:44

cmd, core, params, trie: Add verkle access witness

9ad03c9

Co-authored-by: Ignacio Hagopian <jsign.uy@gmail.com>

fix failing tests

bcc6da8

fix linter issue

2b3391e

add beacon block hash to the witness

a21c021

fix reference error

e34ac0f

Update core/state/access_witness.go

54bdfb4

Co-authored-by: Martin HS <martin@swende.se>

partial review feedback

f534a4d

update based on fixes added to the main branch

1bdae10

add more fixes

fea8553

rename access witness functions

2334cb7

fix rebase issues

811ebfe

review feedback from @holiman

d4f2768

gballet force-pushed the rewritten-eip4762-into-master branch from 788d16c to d4f2768 Compare April 26, 2024 13:44

add verkle op handlers

c65d117

holiman reviewed Apr 29, 2024

View reviewed changes

core/vm/evm.go Outdated Show resolved Hide resolved

gballet commented Apr 29, 2024

View reviewed changes

core/vm/evm.go Outdated Show resolved Hide resolved

gballet added 4 commits April 29, 2024 11:24

rename AccessWitness to AccessEvents

4030cdd

improve comment for eip4762 PUSH1 handler

3a5e041

remove unnecessary creation field

c04b64c

address last review comments

0620c5c

holiman reviewed Apr 29, 2024

View reviewed changes

rjl493456442 reviewed Apr 29, 2024

View reviewed changes

gballet and others added 6 commits April 29, 2024 16:18

Apply suggestions from code review

026ebc9

Co-authored-by: Martin HS <martin@swende.se>

Several fixes (#425)

aa96a9c

* params: fix chainconfig rules * core, core/state: init accessEvent for each transactin * core/state: reuse point cache between blocks

Revert "Several fixes (#425)"

d5699fb

This reverts commit aa96a9c.

more review feedback

04a4fe1

Focus on gas charges and remove witness-building parts

3777222

Co-authored-by: Gary Rong <garyrong0905@gmail.com>

move point cache to cachingDB and remove more witness stuff

72efd71

Co-authored-by: Gary Rong <garyrong0905@gmail.com>