all: unify the trie database and snapshot in path mode #30159

rjl493456442 · 2024-07-15T07:36:07Z

No description provided.

core/state/database.go

core/state/snapshot/generate.go

holiman · 2024-10-14T13:44:09Z

eth/protocols/snap/handler.go

+	if chain.TrieDB().Scheme() == rawdb.HashScheme {
+		it, err = chain.Snapshots().AccountIterator(req.Root, req.Origin)
+	} else {
+		it, err = chain.TrieDB().AccountIterator(req.Root, req.Origin)


I don't understand this. Why are you using triedb account iterator in pbss-mode? (also below)

In the path mode, the legacy state snapshot is not existent anymore. The original state snapshot data will be over taken by the pathdb (the states inside of pathdb).

Therefore, constructing the iterator from pathdb is the only efficient/equivalent way to serve the snap state requests

holiman · 2024-10-14T14:17:03Z

trie/sync_test.go

@@ -704,7 +704,7 @@ func syncWithHookWriter(t *testing.T, root common.Hash, db ethdb.Database, srcDb
 			syncPath: NewSyncPath([]byte(paths[i])),
 		})
 	}
-	reader, err := srcDb.Reader(root)


If we want to make the change smaller, how about first a pure refactoring/renaming-PR, changing e.g. this Reader->NodeReader, and Database to NodeDatabase, and other similar pure changes?

sounds good to me!

holiman · 2024-10-14T14:20:46Z

triedb/pathdb/buffer.go

+	"github.com/ethereum/go-ethereum/trie/trienode"
+)
+
+// buffer is a collection of modified states along with the modified trie nodes.


Is there any relation between this buffer implementation, and the one PR:ed in #30464 ? Or rather, what is the relation between the two? Orthogonal, or one is an evolution of the other?

The buffer here contains (a) trie node data (b) state data, it's still a singleton structure for aggregating db writes;

The #30464 introduces an notion called "frozen" buffer. There are two buffers (totally equal) in the pathdb. If the first buffer is full, it will be scheduled for flushing in the background and once it's fully flushed. In the same time, the new db writes will be accumulated in the second buffer to not block the system.

holiman · 2024-10-14T14:43:27Z

triedb/pathdb/iterator_fast.go

+// newFastIterator creates a new hierarchical account or storage iterator with one
+// element per diff layer. The returned combo iterator can be used to walk over
+// the entire snapshot diff stack simultaneously.
+func newFastIterator(db *Database, root common.Hash, account common.Hash, seek common.Hash, accountIterator bool) (*fastIterator, error) {


is this an exact replica of iterator_fast.go in core/state/snapshot? If so, couldn't we put it into some shared (possibly internal) package?

Ah right I guess it cannot be exactly identical, they use different iterators under the hood :/

Yeah, there are some tiny differences unfortunately

holiman · 2024-10-14T14:45:31Z

triedb/pathdb/iterator_fast.go

+		root:    root,
+		account: accountIterator,
+	}
+loop:


when I see labels , I usually think that it ought to be a closure with returns instead of goto label.

gballet

2nd pass, left a few nitpicks. I am yet to start trying to implement verkle on top of it to see if I have more comments. I don't see how this PR could be broken into smaller pieces, unfortunately, at least in a significant way.

gballet · 2024-10-14T10:33:19Z

tests/block_test_util.go

+		}
+	}
+	if scheme == rawdb.PathScheme {
+		if err := chain.TrieDB().WaitGeneration(); err != nil {


nit: WaitForGeneration

gballet · 2024-10-14T13:44:52Z

triedb/database.go

-	//
-	// The passed in maps(nodes, states) will be retained to avoid copying
-	// everything. Therefore, these maps must not be changed afterwards.
-	Update(root common.Hash, parent common.Hash, block uint64, nodes *trienode.MergedNodeSet, states *triestate.Set) error


can't you just pass nil in the case of hashdb and not remove this from the interface?

This change ports some non-important changes from #30159, including interface renaming and some trivial refactorings.

holiman · 2024-11-08T08:27:57Z

This PR has been split up into

will-2012 · 2024-11-14T08:54:35Z

Hello～ what is the purpose of this PR? Is it just simplifying the logic and making the trie code clearer?
Btw, the bloomfilter in the flat state is no longer used. Does this harm performance?

This change ports some non-important changes from #30159, including interface renaming and some trivial refactorings.

rjl493456442 requested review from karalabe, holiman, MariusVanDerWijden and lightclient as code owners July 15, 2024 07:36

rjl493456442 force-pushed the snapshot-integration branch 7 times, most recently from 3669e84 to 9634ae2 Compare July 22, 2024 06:36

rjl493456442 force-pushed the snapshot-integration branch 5 times, most recently from 3ce906c to 51812ea Compare July 30, 2024 05:46

rjl493456442 force-pushed the snapshot-integration branch from 51812ea to af54be1 Compare July 30, 2024 08:23

rjl493456442 force-pushed the snapshot-integration branch 5 times, most recently from ee78428 to ebc0454 Compare August 21, 2024 07:41

holiman reviewed Sep 24, 2024

View reviewed changes

core/state/database.go Outdated Show resolved Hide resolved

holiman reviewed Sep 24, 2024

View reviewed changes

core/state/database.go Outdated Show resolved Hide resolved

rjl493456442 force-pushed the snapshot-integration branch 5 times, most recently from 0c48fce to d5fdd5a Compare September 27, 2024 05:51

rjl493456442 changed the title ~~[Work-in-progress] Snapshot integration~~ all: unify the trie database and snapshot in path mode Sep 27, 2024

rjl493456442 force-pushed the snapshot-integration branch from d5fdd5a to ad2046a Compare September 27, 2024 07:44

rjl493456442 added the status:triage label Oct 14, 2024

holiman reviewed Oct 14, 2024

View reviewed changes

core/state/snapshot/generate.go Show resolved Hide resolved

holiman reviewed Oct 14, 2024

View reviewed changes

gballet reviewed Oct 14, 2024

View reviewed changes

all: integrate state snapshot into pathdb

8c6bf3b

rjl493456442 force-pushed the snapshot-integration branch from ad2046a to 8c6bf3b Compare October 15, 2024 06:53

holiman mentioned this pull request Oct 15, 2024

core, trie, triedb: port changes from the snapshot integration #30599

Merged

holiman pushed a commit that referenced this pull request Oct 18, 2024

core, trie, triedb: minor changes from snapshot integration (#30599)

b6c62d5

This change ports some non-important changes from #30159, including interface renaming and some trivial refactorings.

holiman pushed a commit that referenced this pull request Nov 19, 2024

core, trie, triedb: minor changes from snapshot integration (#30599)

6412bee

This change ports some non-important changes from #30159, including interface renaming and some trivial refactorings.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

all: unify the trie database and snapshot in path mode #30159

all: unify the trie database and snapshot in path mode #30159

rjl493456442 commented Jul 15, 2024

holiman Oct 14, 2024

rjl493456442 Oct 15, 2024

holiman Oct 14, 2024

rjl493456442 Oct 15, 2024

holiman Oct 14, 2024

rjl493456442 Oct 15, 2024

holiman Oct 14, 2024

holiman Oct 14, 2024

rjl493456442 Oct 15, 2024

holiman Oct 14, 2024

gballet left a comment

gballet Oct 14, 2024

gballet Oct 14, 2024

holiman commented Nov 8, 2024

will-2012 commented Nov 14, 2024

all: unify the trie database and snapshot in path mode #30159

Are you sure you want to change the base?

all: unify the trie database and snapshot in path mode #30159

Conversation

rjl493456442 commented Jul 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gballet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holiman commented Nov 8, 2024

will-2012 commented Nov 14, 2024