Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

trie/pathdb: decode verkle node in loadLayers #30870

Closed
wants to merge 4 commits into from

Conversation

gballet
Copy link
Member

@gballet gballet commented Dec 6, 2024

In loadLayers, the root is computed by hashing (keccak) the node. This is not going to work for a verkle node, so make sure the correct root node derivation is used.

@gballet gballet requested a review from rjl493456442 as a code owner December 6, 2024 20:18
@rjl493456442 rjl493456442 self-assigned this Dec 7, 2024
@gballet gballet requested a review from rjl493456442 December 9, 2024 08:22
Comment on lines +155 to +169
if !isVerkle {
if len(blob) == 0 {
return types.EmptyRootHash, nil
}
return crypto.Keccak256Hash(blob), nil
}
// Compute the node hash in verkle tree manner
if len(blob) == 0 {
return types.EmptyVerkleHash, nil
}
n, err := verkle.ParseNode(blob, 0)
if err != nil {
return common.Hash{}, err
}
return n.Commit().Bytes(), nil
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the "default" code path should be non-verkle. So instead of having a if !isVerkle special case, make it if isVerkle into the special case

Suggested change
if !isVerkle {
if len(blob) == 0 {
return types.EmptyRootHash, nil
}
return crypto.Keccak256Hash(blob), nil
}
// Compute the node hash in verkle tree manner
if len(blob) == 0 {
return types.EmptyVerkleHash, nil
}
n, err := verkle.ParseNode(blob, 0)
if err != nil {
return common.Hash{}, err
}
return n.Commit().Bytes(), nil
if isVerkle { // Compute the node hash in verkle tree manner
if len(blob) == 0 {
return types.EmptyVerkleHash, nil
}
if n, err := verkle.ParseNode(blob, 0); err != nil {
return common.Hash{}, err
} else {
return n.Commit().Bytes(), nil
}
}
if len(blob) == 0 {
return types.EmptyRootHash, nil
}
return crypto.Keccak256Hash(blob), nil

Copy link
Contributor

Choose a reason for hiding this comment

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

Another way to do this would be to define two hashers:

 func mptHash(blob []byte) (common.Hash, error) {
	if len(blob) == 0 {
			return types.EmptyRootHash, nil
		}
		return crypto.Keccak256Hash(blob), nil
}
func verkleHash(blob []byte) (common.Hash, error) {
	if len(blob) == 0 {
		return types.EmptyVerkleHash, nil
	}
	n, err := verkle.ParseNode(blob, 0)
	if err != nil {
		return common.Hash{}, err
	}
	return n.Commit().Bytes(), nil
}

And then, in e.g Enalbe, you do

stored = hasher(rawdb.ReadAccountTrieNode(db.diskdb, nil))

you'd just configure the hasher to use, to be one of either mptHash or verkleHash. So whenever you set db.isVerkle, you can also set db.hasher to use the correct one.

Just an idea...

@rjl493456442
Copy link
Member

@holiman

Yep, I encounter another big issue, is about the EmptyRootHash.

The empty root hash is different in Merkle Tree and Verkle Tree. We use EmptyRootHash (the root of empty Merkle Tree) everywhere. We should fix it first.

@rjl493456442
Copy link
Member

Please check #30896 instead

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.

3 participants