Skip to content

Set Cn to identity instead of nil, when deleting a 128-leaf subtree #465

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gballet
Copy link
Member

@gballet gballet commented Apr 10, 2025

Even though verkle doesn't support deletions during transaction execution (yet) we have to support deletions so that geth can rollback state during a reorg.

When either the left or right suffix subtree of a LeafNode becomes empty, the commitment must be set to 0. It used to be set to nil, but this would cause panics later down the line when inserting value and trying to differentially-update the commitment.

I can foresee that this will cause an issue for storage size, as we'd potentially be storing a 0. But this will unblock Gary for his rollback experiments.

@gballet gballet requested review from jsign and Copilot April 10, 2025 21:16
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

tree.go:1387

  • Consider adding unit tests to verify that delete operations correctly update both c1 and c2 to the identity point instead of nil, ensuring that rollback behavior works as expected.
n.c1 = new(Point).SetIdentity()

Comment on lines -1387 to +1389
n.c1 = nil
n.c1 = new(Point).SetIdentity()
} else {
n.c2 = nil
n.c2 = new(Point).SetIdentity()
Copy link
Collaborator

@jsign jsign Apr 10, 2025

Choose a reason for hiding this comment

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

The new changes are aligned on how NewLeafNode works, right? Look here.

Looks like in that else we aren't doing else if count > 0 {, which means cfg.CommitToPoly is always called (apart from the optimized case of emptyCodeHash) which never return nil.

So maybe this means the current change is a more reasonable action than what we had before this PR?

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