all: re-implement state overriding

[rjl493456442]

No description provided.

[s1na]

Can we keep the StateOverride type alias? more cumbersome to type map[common.Address]OverrideAccount 😄

[holiman]

Ah, so this one is really a follow-up on #29761 ?

[s1na]

We had a triage discussion about this. As it stands the PR doesn't let us compute the state root off of the overridden state. This feature is desired for #27720.

[rjl493456442]

But I think this branch is still better than the existing solution for state override. e.g. we can use this feature to override the states for "read-only" purposes.

For multicall, it's a bit different. It feels like a call simulation which may have the state mutations (but they are never persisted). In this case, we can use the existing solution to apply the overrides as mutation.

[holiman]

Needs a rebase

[holiman]

I think the approach here is good.

the PR doesn't let us compute the state root off of the overridden state.

I think that's a pretty hard criteria to judge by. That sounds like a pretty difficult thing to achieve, IMO.

[rjl493456442]

I can rebase it

[holiman]

Why pass *map instead of just a map?

[holiman]

Getting back to this. So, in my branch layerd_override, I started banging out how a layered-statedb implementation of this would look. I mean, if, instead of adding the overrides in the Reader-layer below the StateDB, we instead layer it like the hookedStateDB, on top of the StateDB.

First of all, that approach requires the defnition of a statedb interface, basically a copy of vm.StateDB inside the state package, so that the layers can be OverrideStateDB <> HookedState <> StateDB OR OverrideStateDB <> StateDB both. Unfortunately, that makes it so none of the hooks for modifications in the overridden layers are invoked. In order to make it possible to do HookedState <> OverrideStateDB <>StateDB, we need to add some extra paths to work around the spot where hooked reaches into statedb s.inner.journal.dirties to handle the selfdestruct burns. It can be worked out, somehow.

A larger problem is that all journalling is implemented in side *StateDB struct. And the journal operates on a *StateDB. Separating the journal from the StateDB, again, operating on some form of interface, but without involving recursion...
Because, consider balance changes - a revert happens directly on the state object:

func (ch nonceChange) revert(s *StateDB) {
	s.getStateObject(ch.account).setNonce(ch.prev)
}

Because the public SetNonce method journal the reversion, if it was used:

func (s *StateDB) SetNonce(addr common.Address, nonce uint64) {
	stateObject := s.getOrNewStateObject(addr)
	if stateObject != nil {
		stateObject.SetNonce(nonce)
	}
}
// --> invokes
func (s *stateObject) SetNonce(nonce uint64) {
	s.db.journal.nonceChange(s.address, s.data.Nonce)
	s.setNonce(nonce)
}

All in all, my idea about putting overrides as a layer-on-top doesn't seem to be very good. Please rebase and I'm all for getting this PR merged.

[holiman]

It was a bit tricky to rebase this. I made an attempt, here: https://github.com/ethereum/go-ethereum/compare/master...holiman:go-ethereum:override-reader-3?expand=1 , but I didn't want to force-push on your branch, because it's pretty raw. It compiles, but I haven't verified any functionality. YMMV, up to you if you want to use it or not :)

[rjl493456442]

@holiman

I am reconsidering our direction here.

A while ago, we implemented eth_simulateV1 with support for state overrides. However, this implementation differs slightly from the one in eth_call or eth_estimateGas. In eth_simulateV1, it’s possible to override states and generate a block hash that includes all overridden states as if they were standard state mutations. This means the overridden states are hashed as well.

During initial discussions around eth_simulateV1, we agreed that the block hash should indeed be an output of this API, as it would be useful for result comparison across different clients. In this context, a read-only state override is insufficient, as producing a hash requires the ability to include state mutations.

The limitation of the new implementation is that it only supports read-only state overrides. Given this, I would lean toward using our existing approach: applying mutations on top of overrides, as it is compatible with both use cases.

[holiman]

The limitation of the new implementation is that it only supports read-only state overrides. Given this, I would lean toward using our existing approach: applying mutations on top of overrides, as it is compatible with both use cases.

Yep, makes sense to me. Let's close this then