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

add DeleteAtStem command for state rollbacks #446

Merged
merged 3 commits into from
Jul 30, 2024
Merged

Conversation

gballet
Copy link
Member

@gballet gballet commented Jul 24, 2024

Adds a method to delete a whole subgroup instead of individual keys. This is necessary for a more efficient deletion of an account when doing a rollback, which is used by the PBSS archive node.

TODO

  • write local test
  • test in geth

@gballet gballet requested a review from jsign July 24, 2024 15:40
tree.go Show resolved Hide resolved
Comment on lines +679 to +694
// delete the entire child if instructed to by
// the recursive algorigthm.
if del {
n.children[nChild] = Empty{}

// Check if all children are gone, if so
// signal that this node should be deleted
// as well.
for _, c := range n.children {
if _, ok := c.(Empty); !ok {
return false, nil
}
}

return true, nil
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know this is a copy/paste from the Delete(...) API, but I guess we could try extracting the logic to a private method?

And BTW, there's a typo (that already existed before). L680, "algorigthm"

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to do that right off the bat, but it's making the interface more complicated. I'd like to do that too, but I want to ensure correctness.

One of the differences, as you pointed out, is what happens when you delete something that doesn't exist. Since this is meant to be called from something that knows of reverse-diffs, so if the value does not exist, there is an issue somewhere.

@gballet gballet marked this pull request as ready for review July 30, 2024 09:36
@gballet gballet merged commit 217a0b6 into master Jul 30, 2024
7 checks passed
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