all: distinguish empty state root in merkle and verkle #30896
Conversation
rjl493456442
requested review from
fjl,
s1na,
lightclient,
holiman and
MariusVanDerWijden
as code owners
rjl493456442
force-pushed
the
empty-tree-root
branch
3 times, most recently
from
c3d84dd to
b758ae9
Compare
There was a problem hiding this comment.
I get the whole point of the PR, I would recommend making the following changes though:
- Keeping the name
EmptyRootHashfor now, in order to reduce the footprint of that PR - Actually implementing the related verkle code, e.g. in the snapshot. In the past, it already happened that some changes were made that greatly broke the verkle model, but we merged them anyway thinking it was going to help.
- don't bother making a difference between an account's state root being an MPT or a verkle root, verkle accounts don't have state roots.
| @@ -427,7 +427,7 @@ func (pre *Prestate) Apply(vmConfig vm.Config, chainConfig *params.ChainConfig, | |||
| func MakePreState(db ethdb.Database, accounts types.GenesisAlloc) *state.StateDB { | |||
| tdb := triedb.NewDatabase(db, &triedb.Config{Preimages: true}) | |||
| sdb := state.NewDatabase(tdb, nil) | |||
| statedb, _ := state.New(types.EmptyRootHash, sdb) | |||
| statedb, _ := state.New(types.EmptyMerkleHash, sdb) // TODO support verkle node in t8n | |||
There was a problem hiding this comment.
you don't have to worry about that, we have an upcoming PR for it
| {DefaultSepoliaGenesisBlock(), params.SepoliaGenesisHash}, | ||
|
|
||
| // TODO (rjl493456442, gballet) add verkle genesis tests |
There was a problem hiding this comment.
I can help you with that, but if all these changes are meant for verkle, we should include verkle tests as well. Otherwise we get in the same situation, when the base branch drifted and broke a ton of verkle use cases.
core/types/state_account.go
Outdated
| @@ -89,7 +97,7 @@ func SlimAccountRLP(account StateAccount) []byte { | |||
|
|
|||
| // FullAccount decodes the data on the 'slim RLP' format and returns | |||
| // the consensus format account. | |||
| func FullAccount(data []byte) (*StateAccount, error) { | |||
| func FullAccount(data []byte, isVerkle bool) (*StateAccount, error) { | |||
There was a problem hiding this comment.
I don't think that this is necessary: verkle trees don't have the concept of Root so if it's set to EmptyMerkleRoot it doesn't matter, this code should never be used in the first place. And it would greatly reduce the footprint of this PR,
There was a problem hiding this comment.
I've managed quite far without having to do this change, I think we should do the same - it should be very easy to circumvent.
triedb/pathdb/execute.go
Outdated
| @@ -171,7 +171,7 @@ func deleteAccount(ctx *context, db database.NodeDatabase, addr common.Address) | |||
| } | |||
| } | |||
| root, result := st.Commit(false) | |||
| if root != types.EmptyRootHash { | |||
| if root != types.EmptyMerkleHash { // TODO (rjl493456442) support verkle | |||
There was a problem hiding this comment.
you're not supposed to delete an account in verkle anyway, regardless of whether or not state deletions are possible. So we should never get to this point, and so this comment could be removed.
core/vm/evm.go
Outdated
| @@ -467,7 +467,7 @@ func (evm *EVM) create(caller ContractRef, codeAndHash *codeAndHash, gas uint64, | |||
| storageRoot := evm.StateDB.GetStorageRoot(address) | |||
| if evm.StateDB.GetNonce(address) != 0 || | |||
| (contractHash != (common.Hash{}) && contractHash != types.EmptyCodeHash) || // non-empty code | |||
| (storageRoot != (common.Hash{}) && storageRoot != types.EmptyRootHash) { // non-empty storage | |||
| (storageRoot != (common.Hash{}) && storageRoot != types.EmptyMerkleHash) { // non-empty storage TODO (rjl493456442) support verkle | |||
There was a problem hiding this comment.
verkle here is already supported: in trie/verkle.go, it will return 0 as its root. So we can remove this TODO
rjl493456442
force-pushed
the
empty-tree-root
branch
2 times, most recently
from
1c30900 to
b1d43c6
Compare
rjl493456442
force-pushed
the
empty-tree-root
branch
from
b1d43c6 to
f2e8f67
Compare
|
Deployed it on 7 (full sync) and 8 (snap sync) to quickly validate the changes.
|
This pull request distinguishes the different empty tree hash in merkle tree and verkle tree.