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

Return an error when attempting to invalidate the finalized checkpoint #14740

Closed
wants to merge 3 commits into from

Conversation

potuz
Copy link
Contributor

@potuz potuz commented Dec 19, 2024

return an error instead of panicking if the finalized checkpoint is being marked as invalid.

(h/t @nalepae for finding this)

@potuz potuz requested a review from a team as a code owner December 19, 2024 13:49
@@ -23,6 +23,9 @@ func (s *Store) setOptimisticToInvalid(ctx context.Context, root, parentRoot, la
if node == nil {
return invalidRoots, errors.Wrap(ErrNilNode, "could not set node to invalid")
}
if node.parent == nil {
return invalidRoots, errors.New("attempting to set the root node to invalid")
Copy link
Contributor

Choose a reason for hiding this comment

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

I find this error confusing, makes it sound like there is an ongoing attempt to set the root node to invalid. This follows suit with the phrasing above too

Suggested change
return invalidRoots, errors.New("attempting to set the root node to invalid")
return invalidRoots, errors.New("could not set the root node to invalid")

Copy link
Contributor Author

@potuz potuz left a comment

Choose a reason for hiding this comment

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

I would rather fix the kurtosis error at genesis, or case genesis rather than returning an error here. There is a chance that the node will revert finality if we go this route.

@potuz potuz added the Blocked Blocked by research or external factors label Feb 3, 2025
@potuz potuz closed this Feb 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocked Blocked by research or external factors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants