core/state: fix prefetcher for verkle #29760

rjl493456442 · 2024-05-13T06:08:49Z

This pull request fixes a flaw in verkle.

Inside of the statedb, there is a tool named "prefetcher". It will pull the necessary data for state hashing in the background to accelerate the state hashing performance. Unfortunately, it's broken in the verkle context.

The root cause is once verkle is activated, there is only a single trie used for state hashing(instead of two-layer tree structure in merkle). And storage trie commits are schedule before the account trie for performance consideration. If the verkle trie is resolved from the prefetcher after committing the storage changes , the changes caused by storage will be silently dropped.

This pull request disables the prefetching in verkle as a quick fix.

holiman · 2024-05-13T06:36:02Z

core/state/statedb.go

+	// Don't check prefetcher if Verkle Trie has been used. In the context of Verkle,
+	// only a single trie is used for state hashing. Replacing a non-nil Verkle tree
+	// here could result in losing uncommitted changes from storage.
+	if prefetcher != nil && (s.trie == nil || !s.trie.IsVerkle()) {


This change does not only affect verkle, but also non-verkle, if prefetcher != nil, s.trie != nil would previously enter this clause, but not after this change.
Please elaborate

s.trie != nil && !s.trie.IsVerkle() will still entry the clause
only s.trie != nil && s.trie.IsVerkle will be rejected

I'm talking about non-verkle (so s.trie.IsVerkle() == false).

So, for prefetcher != nil, s.trie != nil

currently , on master: we enter the clause

in this PR: we do not enter the clause

for the case you mentioned, non-verkle, trie is not nil, we will still enter the clause isn't it?

(s.trie == nil || !s.trie.IsVerkle()), the case will satisfy the second condition !s.trie.IsVerkle()

Oh right, yes, my bad!

gballet · 2024-05-13T06:42:40Z

core/state/statedb.go

+	// Don't check prefetcher if Verkle Trie has been used. In the context of Verkle,
+	// only a single trie is used for state hashing. Replacing a non-nil Verkle tree
+	// here could result in losing uncommitted changes from storage.
+	if prefetcher != nil && (s.trie == nil || !s.trie.IsVerkle()) {


I am quite surprised that you need this, because in verkle mode the prefetcher should be nil. But in any case, it doesn't hurt to add that check.

yeah, i have some changes locally and enable the prefetcher for verkle. Tests are broken immediately. I can imagine prefetcher will enabled for verkle as well.

gballet

LGTM

karalabe · 2024-05-13T12:24:45Z

I'd appreciate getting the prefetcher PR in first because I don't want to rebase that for a 1 liner change.

rjl493456442 requested review from karalabe and holiman as code owners May 13, 2024 06:08

holiman reviewed May 13, 2024

View reviewed changes

gballet reviewed May 13, 2024

View reviewed changes

gballet approved these changes May 13, 2024

View reviewed changes

core/state: fix prefetcher for verkle

cede7f0

rjl493456442 force-pushed the fix-prefetcher-verkle branch from 7600bb9 to cede7f0 Compare May 13, 2024 12:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core/state: fix prefetcher for verkle #29760

core/state: fix prefetcher for verkle #29760

rjl493456442 commented May 13, 2024 •

edited

holiman May 13, 2024

rjl493456442 May 13, 2024

holiman May 13, 2024

rjl493456442 May 13, 2024

holiman May 13, 2024

gballet May 13, 2024

rjl493456442 May 13, 2024

gballet left a comment

karalabe commented May 13, 2024

core/state: fix prefetcher for verkle #29760

Are you sure you want to change the base?

core/state: fix prefetcher for verkle #29760

Conversation

rjl493456442 commented May 13, 2024 • edited

holiman May 13, 2024

Choose a reason for hiding this comment

rjl493456442 May 13, 2024

Choose a reason for hiding this comment

holiman May 13, 2024

Choose a reason for hiding this comment

rjl493456442 May 13, 2024

Choose a reason for hiding this comment

holiman May 13, 2024

Choose a reason for hiding this comment

gballet May 13, 2024

Choose a reason for hiding this comment

rjl493456442 May 13, 2024

Choose a reason for hiding this comment

gballet left a comment

Choose a reason for hiding this comment

karalabe commented May 13, 2024

rjl493456442 commented May 13, 2024 •

edited