polygon/sync: Fix onMilestoneEvent crash - #11568
Conversation
|
@shohamc1 please attach more info to the PR - ie the error message in this case. This is good if we have to retrospectively go back via PRs to figure out why some changes were made etc. |
| } | ||
| if newTip == nil { | ||
| return errors.New("sync.Sync.onMilestoneEvent: unexpected to have no milestone headers since the last milestone after receiving a new milestone event") | ||
| return errors.New("sync.Sync.handleMilestoneMismatch: unexpected to have no milestone headers since the last milestone after receiving a new milestone event") |
There was a problem hiding this comment.
Maybe it would make sense to log the new milestone event here for debugging purposes.
There was a problem hiding this comment.
Agree, does not hurt to log, or wrap the error in a new error with extra info using fmt.Errorf("%w: id=%d, start=%d, end=%d", errors.New("...existing err..."), milestone.Id, milestone.Start, milestone.End, ....other info....). Happy with either if you want to do it in a follow up.
| return nil | ||
| } | ||
|
|
||
| func (s *Sync) onMilestoneEvent( |
There was a problem hiding this comment.
The function name onMilestoneEvent does not give any description about what the function does, nor does it seem to accept any function handler. Maybe it's better to rename it something like checkMilestoneEvent or verifyMilestoneEvent .
There was a problem hiding this comment.
I agree. I'm also not a fan of the onXYZ naming here. Some suggestions:
applyMilestone- it verifies milestone matches tip & prunes canonical chain builder root if we match the milestone or it unwinds our tip if we don't match the milestoneapplyNewBlock- tries to connect blocks, fill gaps, update fork choice if tip changedapplyNewBlockHashes- same as above but using block hashes event
There was a problem hiding this comment.
Probably a good follow up to do if you want @antonis19
There was a problem hiding this comment.
applyMilestoneOnTip, applyNewBlockOnTip, applyNewBlockHashesOnTip - if we want to be extra verbose (maybe not needed as it is clear? I don't mind either)
There was a problem hiding this comment.
(feel free to think of better names based on the descriptions above)
addresses follow ups from #11568 (comment)
3 participants
Crash was due to a missing early return.