Skip to content

Conversation

rjl493456442
Copy link

In this pull request, several changes have been made:

(a) Replaced CurrentAccountAddress *common.Address with CurrentAccountHash common.Hash

Managing a nullable pointer adds unnecessary complexity. This change simplifies the
logic by replacing the pointer with a value based flag:

  • Previously: CurrentAccountAddress == nil indicated that no account had been migrated yet.
  • Now: CurrentAccountHash == common.Hash{} serves the same purpose.

This approach is already used in CurrentSlotHash, so this change aligns the handling of both fields.

(b) Removed LoadTransitionState call sites from the trie reader

The current usage of LoadTransitionState in the trie reader is not correct. We should consider
five different scenarios:

(a) TransitionState(root) is not found:
Verkle is not activated yet => use merkle tree

(b) TransitionState(root) is not found and ChainConfig.IsVerkle(blockNumber, blockTime) is true:
The first Verkle block at the boundary => use overlay tree

(c) ChainConfig.IsVerkle(blockNumber, blockTime) is true and TransitionState(root).Started is false:
The verkle block before the  migration => use overlay tree

(d) ChainConfig.IsVerkle(blockNumber, blockTime) is true and TransitionState(root).Started is true:
The verkle block during the migration => use overlay tree

(e)  ChainConfig.IsVerkle(blockNumber, blockTime) is true and TransitionState(root).Ended is true
Verkle transition completes => use verkle tree

However, in your implementation, e.g., in the code below, it's wrong. As you mentioned,
the migration is only started if the base merkle state if finalized. !ts.InTransition() && !ts.Transitioned()
can refer to the situation that Verkle has been activated but the migration is not. In this
case, Overlay tree should be used instead of Merkle here. We should separate this part
of logic into a following PR.

// OpenStorageTrie opens the storage trie of an account.
func (db *CachingDB) OpenStorageTrie(stateRoot common.Hash, address common.Address, root common.Hash, self Trie) (Trie, error) {
	ts := db.LoadTransitionState(stateRoot)
	// In the verkle case, there is only one tree. But the two-tree structure
	// is hardcoded in the codebase. So we need to return the same trie in this
	// case.
	if ts.InTransition() || ts.Transitioned() {
		return self, nil
	}
	tr, err := trie.NewStateTrie(trie.StorageTrieID(stateRoot, crypto.Keccak256Hash(address.Bytes()), root), db.triedb)
	if err != nil {
		return nil, err
	}
	return tr, nil
}

@rjl493456442 rjl493456442 requested a review from holiman as a code owner June 3, 2025 07:59
@@ -382,8 +381,6 @@ func (beacon *Beacon) FinalizeAndAssemble(chain consensus.ChainHeaderReader, hea
// Create the block witness and attach to block.
// This step needs to happen as late as possible to catch all access events.
if chain.Config().IsVerkle(header.Number, header.Time) {
// TODO(gballet) move this to the end of the overlay conversion function in a subsequent PR
statedb.Database().(*state.CachingDB).SaveTransitionState(header.Root, &overlay.TransitionState{Ended: true})
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

&overlay.TransitionState{Ended: true}) is apparently wrong here. Let's remove it for now.

@@ -577,7 +577,6 @@ func GenerateVerkleChain(config *params.ChainConfig, parent *types.Block, engine

func GenerateVerkleChainWithGenesis(genesis *Genesis, engine consensus.Engine, n int, gen func(int, *BlockGen)) (common.Hash, ethdb.Database, []*types.Block, []types.Receipts, []*verkle.VerkleProof, []verkle.StateDiff) {
db := rawdb.NewMemoryDatabase()
saveVerkleTransitionStatusAtVerlkeGenesis(db)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicated with the one in genesis.Commit(db, triedb). It's also not correct to persist a transition-state with zero root.

core/genesis.go Outdated
@@ -146,9 +146,6 @@ func hashAlloc(ga *types.GenesisAlloc, isVerkle bool) (common.Hash, error) {
emptyRoot = types.EmptyVerkleHash
}
db := rawdb.NewMemoryDatabase()
if isVerkle {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unnecessary to persist any transition flag, as this function is supposed to calculate the state root of genesis only.

core/genesis.go Outdated
@@ -319,11 +300,6 @@ func SetupGenesisBlockWithOverride(db ethdb.Database, triedb *triedb.Database, g
if genesis != nil && genesis.Config == nil {
return nil, common.Hash{}, nil, errGenesisNoConfig
}
// In case of verkle-at-genesis, we need to ensure that the conversion
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicated with the one in genesis.Commit(db, triedb). It's also not correct to persist a transition-state with zero root.

@gballet gballet force-pushed the rebase-master-add-transition-state branch from c38f272 to c0578c1 Compare July 11, 2025 15:16
@gballet gballet self-requested a review as a July 11, 2025 15:16
@gballet gballet force-pushed the rebase-master-add-transition-state branch from e3f7af8 to 86183a8 Compare July 15, 2025 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants