-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Rollback Block During Processing #14554
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -76,6 +76,7 @@ func (s *Service) postBlockProcess(cfg *postBlockProcessConfig) error { | |||
|
|||
err := s.cfg.ForkChoiceStore.InsertNode(ctx, cfg.postState, cfg.roblock) | |||
if err != nil { | |||
s.rollbackBlock(ctx, cfg.roblock.Root()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have a unit test triggering this?
require.NoError(t, err) | ||
|
||
// Rollback block insertion into db and caches. | ||
require.ErrorContains(t, "invalid parent root", service.postBlockProcess(&postBlockProcessConfig{ctx, roblock, [32]byte{}, postState, false})) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you check for "could not insert block" and "to fork choice store" instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TSM
What type of PR is this?
Bug Fix
What does this PR do? Why is it needed?
In the event that there are issues during the processing of a block we rollback the block insertion so as to ensure that the node is not left in an inconsistent state. This will allow the node to be able to process the block quickly again via req/resp .
Which issues(s) does this PR fix?
N.A
Other notes for review
Acknowledgements