trie/pathdb: state iterator (snapshot integration pt 4) #30654

rjl493456442 · 2024-10-22T11:19:03Z

It's highly recommended to review the changes by comparing the files directly.

In this pull request, the state iterator is implemented. It's mostly a copy-paste from
the original state snapshot package, but still has some important changes to highlight
here:

(a) The iterator for the disk layer consists of a diff iterator and a disk iterator.

Originally, the disk layer in the state snapshot was a wrapper around the disk, and
its corresponding iterator was also a wrapper around the disk iterator. However,
due to structural differences, the disk layer iterator is divided into two parts:

The disk iterator, which traverses the content stored on disk.
The diff iterator, which traverses the aggregated state buffer.

Checkout BinaryIterator and FastIterator for more details.

(b) The staleness management is improved in the diffAccountIterator and diffStorageIterator

Originally, in the diffAccountIterator, the layer’s staleness had to be checked within the Next
function to ensure the iterator remained usable. Additionally, a read lock on the associated diff
layer was required to first retrieve the account blob. This read lock protection is essential to
prevent concurrent map read/write. Afterward, a staleness check was performed to ensure the
retrieved data was not outdated.

The entire logic can be simplified as follows: a loadAccount callback is provided to retrieve
account data. If the corresponding state is immutable (e.g., diff layers in the path database),
the staleness check can be skipped, and a single account data retrieval is sufficient. However,
if the corresponding state is mutable (e.g., the disk layer in the path database), the callback
can operate as follows:

func(hash common.Hash) ([]byte, error) {
    dl.lock.RLock()
    defer dl.lock.RUnlock()

    if dl.stale {
        return nil, errSnapshotStale
    }
    return dl.buffer.states.mustAccount(hash)
}

The callback solution can eliminate the complexity for managing concurrency with the read
lock for atomic operation.

triedb/pathdb/iterator_fast.go

triedb/pathdb/database.go

gballet · 2024-12-09T14:23:07Z

triedb/pathdb/holdable_iterator.go

+// holdableIterator is a wrapper of underlying database iterator. It extends
+// the basic iterator interface by adding Hold which can hold the element
+// locally where the iterator is currently located and serve it up next time.
+type holdableIterator struct {


this isn't a great name; how about cachingIterator ? the elements that are "held" are basically cached so that you don't have to iterate the 2nd time.

or "replayableIterator", since the iteration is "replayable"

I am open for renaming it, for sure. But I don't want to change it in this pull request.

It's a copy-paste from the state snapshot and it's easier to not change it for review.

gballet · 2024-12-10T11:19:53Z

triedb/pathdb/holdable_iterator.go

+	}
+	it.key = common.CopyBytes(it.it.Key())
+	it.val = common.CopyBytes(it.it.Value())
+	it.atHeld = false


it's a bit weird that a function called Hold will set atHeld to false.

Let's polish it in the following PRs, this pr is already huge enough and let's focus on the core changes.

gballet · 2024-12-10T11:21:16Z

triedb/pathdb/holdable_iterator_test.go

+		if !bytes.Equal(it.Value(), []byte(content[order[idx]])) {
+			t.Errorf("item %d: value mismatch: have %s, want %s", idx, string(it.Value()), content[order[idx]])
+		}
+		// Should be safe to call discard multiple times


do you mean "hold" instead of "discard" ?

gballet · 2024-12-10T11:27:26Z

triedb/pathdb/iterator_binary.go

+	a     Iterator
+	b     Iterator


can we have more descriptive names, e.g. left and right ? Same thing with k. Is that a key ?

This is about a primary and a secondary iterator. IMO a and b is slightly better than left and right. And yes, k is the current key. I guess it could be renmed

I would prefer to rename in a following PR.

gballet · 2024-12-10T11:29:07Z

triedb/pathdb/iterator_binary.go

+// newBinaryAccountIterator creates a simplistic account iterator to step over
+// all the accounts in a slow, but easily verifiable way.
+//
+// nolint:all


why do you need this nolint here ? I don't see anything obviously wrong.

gballet · 2024-12-10T11:30:53Z

triedb/pathdb/iterator_fast.go

+	switch bytes.Compare(hashI[:], hashJ[:]) {
+	case -1:
+		return -1
+	case 1:
+		return 1
+	}


Suggested change

switch bytes.Compare(hashI[:], hashJ[:]) {

case -1:

return -1

case 1:

return 1

}

comparison := bytes.Compare(hashI[:], hashJ[:])

if comparison != 0 {

return comparison

}

I would prefer to change it in a following PR.

gballet · 2024-12-10T11:33:46Z

triedb/pathdb/states.go

@@ -98,6 +98,17 @@ func (s *stateSet) account(hash common.Hash) ([]byte, bool) {
 	return nil, false // account is unknown in this set
 }

+// mustAccount returns the account data associated with the specified address
+// hash. The difference is this function will return an error if the account
+// is not found.


isn't it the wrong name then ? For example, we used to have MustCommit which would panic if it couldn't commit the block.

Yeah maybe tryAccount?

yeah, let me find a more reasonable name

gballet · 2024-12-10T11:33:58Z

triedb/pathdb/states.go

@@ -110,6 +121,19 @@ func (s *stateSet) storage(accountHash, storageHash common.Hash) ([]byte, bool)
 	return nil, false // storage is unknown in this set
 }

+// mustStorage returns the storage slot associated with the specified address
+// hash and storage key hash. The difference is this function will return an
+// error if the storage slot is not found.


holiman

LGTM, most code is is copied. However

if the corresponding state is mutable (e.g., the disk layer in the path database), the callback can operate as follows:

Where is that located?

holiman · 2024-12-11T13:24:21Z

triedb/pathdb/iterator_binary.go

+	a     Iterator
+	b     Iterator


This is about a primary and a secondary iterator. IMO a and b is slightly better than left and right. And yes, k is the current key. I guess it could be renmed

holiman · 2024-12-11T13:27:37Z

triedb/pathdb/states.go

@@ -98,6 +98,17 @@ func (s *stateSet) account(hash common.Hash) ([]byte, bool) {
 	return nil, false // account is unknown in this set
 }

+// mustAccount returns the account data associated with the specified address
+// hash. The difference is this function will return an error if the account
+// is not found.


Yeah maybe tryAccount?

holiman · 2024-12-11T13:48:27Z

triedb/pathdb/iterator_fast.go

+					priority: depth + 1,
+				})
+			case *diffLayer:
+				// The state set in diff layer is immutable and will never be stale,


Do we never merge into layers?

For diff layers in the pathdb, yes they are immutable and we never modify the content of them.

The only mutable container is the buffer in the disk layer, which will accept the state set from the diff layers on top.

The only mutable container is the buffer in the disk layer, which will accept the state set from the diff layers on top.

That's nice. We could do the same in the snap layers. Instead of having n mutable layers, where only the bottom-most difflayer is the one that is actually mutable, we could treat them differently.

rjl493456442 · 2024-12-12T08:52:04Z

@holiman This one could be an example.
https://github.com/rjl493456442/go-ethereum/blob/snapshot-integration-p4/triedb/pathdb/iterator_fast.go#L93

holiman

LGTM

gballet

Went over your responses. LGTM

holiman

LGTM

rjl493456442 requested review from karalabe and holiman as code owners October 22, 2024 11:19

rjl493456442 force-pushed the snapshot-integration-p4 branch 2 times, most recently from 0c0db5a to 409da23 Compare October 23, 2024 06:10

holiman changed the title ~~Snapshot integration p4~~ trie/pathdb: state iterator (snapshot integration pt 2) Oct 23, 2024

holiman changed the title ~~trie/pathdb: state iterator (snapshot integration pt 2)~~ trie/pathdb: state iterator (snapshot integration pt 4) Oct 23, 2024

holiman mentioned this pull request Nov 8, 2024

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

Closed

fjl added the pbss-archive label Nov 28, 2024

MariusVanDerWijden reviewed Nov 29, 2024

View reviewed changes

triedb/pathdb/iterator_fast.go Outdated Show resolved Hide resolved

rjl493456442 force-pushed the snapshot-integration-p4 branch 4 times, most recently from aac2c48 to 352e348 Compare December 2, 2024 02:12

rjl493456442 force-pushed the snapshot-integration-p4 branch from 352e348 to 6d1581a Compare December 10, 2024 02:12

gballet added the status:triage label Dec 10, 2024

gballet reviewed Dec 10, 2024

View reviewed changes

fjl assigned gballet, fjl and holiman Dec 10, 2024

holiman reviewed Dec 11, 2024

View reviewed changes

holiman previously approved these changes Dec 13, 2024

View reviewed changes

gballet previously approved these changes Dec 13, 2024

View reviewed changes

triedb/pathdb: implement flat state iterator in pathdb

a037ad4

rjl493456442 dismissed stale reviews from gballet and holiman via 5a5a70e December 16, 2024 07:01

rjl493456442 force-pushed the snapshot-integration-p4 branch 3 times, most recently from d5fbbb8 to 6cef4a6 Compare December 16, 2024 07:14

triedb/pathdb: polish code, improve code comment

4ab8eef

rjl493456442 force-pushed the snapshot-integration-p4 branch from 6cef4a6 to 4ab8eef Compare December 16, 2024 07:27

holiman approved these changes Dec 16, 2024

View reviewed changes

rjl493456442 added this to the 1.14.13 milestone Dec 16, 2024

rjl493456442 merged commit bc1ec69 into ethereum:master Dec 16, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

trie/pathdb: state iterator (snapshot integration pt 4) #30654

trie/pathdb: state iterator (snapshot integration pt 4) #30654

rjl493456442 commented Oct 22, 2024 •

edited

Loading

gballet Dec 9, 2024

gballet Dec 9, 2024

gballet Dec 10, 2024

rjl493456442 Dec 11, 2024

gballet Dec 10, 2024

rjl493456442 Dec 11, 2024

gballet Dec 10, 2024

gballet Dec 10, 2024

holiman Dec 11, 2024

rjl493456442 Dec 12, 2024

gballet Dec 10, 2024

gballet Dec 10, 2024

rjl493456442 Dec 12, 2024

gballet Dec 10, 2024

holiman Dec 11, 2024

rjl493456442 Dec 12, 2024

gballet Dec 10, 2024

holiman left a comment

holiman Dec 11, 2024

holiman Dec 11, 2024

holiman Dec 11, 2024

rjl493456442 Dec 12, 2024

holiman Dec 12, 2024

rjl493456442 commented Dec 12, 2024

holiman left a comment

gballet left a comment

holiman left a comment

trie/pathdb: state iterator (snapshot integration pt 4) #30654

trie/pathdb: state iterator (snapshot integration pt 4) #30654

Conversation

rjl493456442 commented Oct 22, 2024 • edited Loading

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

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

holiman left a 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

rjl493456442 commented Dec 12, 2024

holiman left a comment

Choose a reason for hiding this comment

gballet left a comment

Choose a reason for hiding this comment

holiman left a comment

Choose a reason for hiding this comment

rjl493456442 commented Oct 22, 2024 •

edited

Loading