Skip to content

triedb/pathdb: implement trienode history indexing scheme#33551

Merged
rjl493456442 merged 1 commit into
ethereum:masterfrom
rjl493456442:trienode-history-index
Jan 17, 2026
Merged

triedb/pathdb: implement trienode history indexing scheme#33551
rjl493456442 merged 1 commit into
ethereum:masterfrom
rjl493456442:trienode-history-index

Conversation

@rjl493456442

@rjl493456442 rjl493456442 commented Jan 8, 2026

Copy link
Copy Markdown
Member

This PR implements the indexing scheme for trie node history. Check #33399 for more details

@rjl493456442 rjl493456442 force-pushed the trienode-history-index branch from 1792b79 to 8d78283 Compare January 8, 2026 09:15
@rjl493456442 rjl493456442 force-pushed the trienode-history-index branch from 8d78283 to 7be05ef Compare January 12, 2026 05:36
@fjl fjl added this to the 1.17.0 milestone Jan 15, 2026

@MariusVanDerWijden MariusVanDerWijden left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, I like the chunking scheme, took me a bit to understand, but I think we discussed it in a call before. Added a few suggestions, but more like nitpicks

b.pending = 0
b.index = make(map[stateIdent][]uint64)
maps.Clear(b.index)
maps.Clear(b.ext)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We can use the builtin clear(b.index) clear(b.ext) here, then we can use the builtin "maps" package instead of the /x/exp/maps package

@rjl493456442 rjl493456442 Jan 17, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I can change it in the following PR, just don't want to lose your approval.

}

// TestTrienodeHistoryReaderIterator tests the iterator functionality
func TestTrienodeHistoryReaderIterator(t *testing.T) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it would be nice to have a test for the iterator function, just to make sure that the correct amount of elements etc are returned

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It's a bit hard to come up with a solid test to verify the elements yielded by iterator, as we only expect the "touched chunks" by a set of dirty nodes.

I will definitely add it back, it's a valuable test case.

Comment on lines +77 to +78
// The numeric node ID is represented by the uint16 with the assumption the length
// of path won't be greater than 3.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why can the path not be longer than 3?
Wouldn't it be easier to hardcode it then?
e.g.
switch len(path)
case 3:
offset *= 16
value = value *16 + bytes[2]
fallthrough
case 2:
offset = 16
value = value
16 + bytes[1]
fallthrough
case 1:
offset = 16
value = value
16 + bytes[0]

or something like this? Maybe its a bit easier to understand

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Or maybe something like this:

// The numeric node ID is represented by the uint16 with the assumption the length
// of path won't be greater than 3.
func hexPathNodeID(path string) uint16 {
	bytes := []byte(path)
	n := len(bytes)

	// Pre-computed offsets for paths of length 0, 1, 2, 3:
	// length 0: 0
	// length 1: 1
	// length 2: 1 + 16 = 17
	// length 3: 1 + 16 + 256 = 273
	var offset uint16
	switch n {
	case 3:
		offset += 256
		fallthrough
	case 2:
		offset += 16
		fallthrough
	case 1:
		offset += 1
	default:
	}

	// Compute value from hex digits
	var value uint16
	for i := 0; i < n; i++ {
		value = value*16 + uint16(bytes[i])
	}

	return offset + value
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The path here refers to the "inner" path suffix within the chunk. As I only select the consecutive 3 levels as a chunk, with at most 273 nodes in a chunk, so the assumption is implicitly held.

@rjl493456442 rjl493456442 merged commit 588dd94 into ethereum:master Jan 17, 2026
9 of 10 checks passed
gballet pushed a commit to BZO95/go-ethereum that referenced this pull request May 21, 2026
…3551)

This PR implements the indexing scheme for trie node history. Check
ethereum#33399 for more details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants