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: refactor tracer #581

Merged

Conversation

Francesco4203
Copy link
Collaborator

@Francesco4203 Francesco4203 commented Sep 20, 2024

Reference: ethereum/go-ethereum#26763

Also applied some related changes from this PR: ethereum/go-ethereum#27160

Changes:

  • Rename trie/utils.go to trie/tracer.go
  • Rename origin field in tracer to accessList, still the same function
  • Merge updated nodes and deleted nodes to the same list of dirty nodes, still keep track of number of deleted and updated nodes. List of dirty nodes is map from path to node, an empty node meaning a deleted node. Same logic for merging markUpdated and markDeleted into addNode
  • committer: move logic to mark deleted nodes to committer.store for consistency with the mark update logic

order = append(order, owner)
}
if _, ok := nodes.sets[common.Hash{}]; ok {
order = append(order, common.Hash{})
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here, the trie with owner empty representing the account trie is pushed to the back of the array so that all the storage dirty nodes are flushed first.
Reference: ethereum/go-ethereum@5758d1f

Copy link
Collaborator

Choose a reason for hiding this comment

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

Add this like comment in the source code too ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is a comment like this above

 	// Note, the storage tries must be flushed before the account trie to
	// retain the invariant that children go into the dirty cache first.

Do you think this is clear enough?

Copy link
Collaborator

Choose a reason for hiding this comment

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

the trie with owner empty representing the account trie is pushed to the back of the array so that all the storage dirty nodes are flushed first. I think u can add those blocks for showing why we use common.Hash{} here.

@@ -628,16 +630,16 @@ func (t *Trie) Commit(collectLeaf bool) (common.Hash, *NodeSet, error) {
}

// hashRoot calculates the root hash of the given trie
func (t *Trie) hashRoot() (node, node, error) {
func (t *Trie) hashRoot() (node, node) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No error will ever be returned, so just not return error here.

@Francesco4203 Francesco4203 changed the title trie: rework tracer trie: refactor tracer Sep 20, 2024
@Francesco4203 Francesco4203 force-pushed the rework-tracer branch 3 times, most recently from b58387d to defda57 Compare September 26, 2024 03:51
@huyngopt1994
Copy link
Collaborator

One thing note here in PR is tracer is enabled by default

	trie := &Trie{
		owner:  id.Owner,
		reader: reader,
		//tracer: newTracer(),
		tracer: newTracer(),
	}

@huyngopt1994
Copy link
Collaborator

huyngopt1994 commented Sep 26, 2024

I think u should spit one commit in the PR into more commits for easily reviewing, merging into one 's hard for understanding the purpose of changes.

// but not present in database previously), for example the node was
// embedded in the parent and now deleted from the trie. In this case
// it's noop from database's perspective.
val := c.tracer.getPrev(path)
Copy link
Collaborator

Choose a reason for hiding this comment

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

hm that's interested, so they don't need to track the deleted nodes any more from trie in tracer, and mark it deleted 🤔

trie/committer.go Outdated Show resolved Hide resolved
@huyngopt1994 huyngopt1994 merged commit 45ee924 into axieinfinity:path-base-implementing Sep 26, 2024
1 check passed
Francesco4203 added a commit that referenced this pull request Oct 17, 2024
* trie: refactor tracer

* fix: add description
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.

2 participants