MX-17468 Update sync speed for supernova#7670
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the synchronization speed calculation logic for the supernova release. The implementation changes from storing speed deltas to storing absolute round numbers in the history, simplifying the calculation algorithm.
Changes:
- Refactored
CalculateSynchronizationSpeedto store absolute round numbers instead of speed deltas - Extracted speed calculation logic into a new helper method
calculateSpeedFromSpeedHistory - Updated
CalculateTimeToSynchronizeto return "Estimating..." instead of empty string when speed is unknown - Updated test to reflect the new data structure (storing round numbers instead of deltas)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cmd/termui/presenter/chainInfoGetters.go | Refactored synchronization speed calculation to use absolute round numbers and simplified the algorithm by extracting calculation logic into a new helper method |
| cmd/termui/presenter/chainInfoGetters_test.go | Updated test setup to populate history with absolute round numbers instead of speed deltas |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| if cumulatedTime == 0 || numSyncedBlocks == 0 { | ||
|
|
||
| numSyncedBlocks := psh.synchronizationSpeedHistory[lastIndex] - psh.synchronizationSpeedHistory[firstIndex] |
There was a problem hiding this comment.
Potential integer underflow issue: If the synchronized round goes backwards (e.g., during a blockchain rollback), this subtraction of uint64 values will underflow and wrap around to a very large positive number, causing incorrect speed calculations. The old implementation explicitly handled this case by checking if the difference was negative and setting it to 0. Consider adding a similar check here to handle the case where synchronizationSpeedHistory[lastIndex] < synchronizationSpeedHistory[firstIndex].
| numSyncedBlocks := psh.synchronizationSpeedHistory[lastIndex] - psh.synchronizationSpeedHistory[firstIndex] | |
| firstRound := psh.synchronizationSpeedHistory[firstIndex] | |
| lastRound := psh.synchronizationSpeedHistory[lastIndex] | |
| if lastRound < firstRound { | |
| return 0 | |
| } | |
| numSyncedBlocks := lastRound - firstRound |
| speed := (float64(numSyncedBlocks) / float64(cumulatedTimeMs)) * numMillisecondsInASecond | ||
|
|
||
| return uint64(speed) | ||
|
|
There was a problem hiding this comment.
There is an extra blank line before the closing brace that is inconsistent with the rest of the functions in this file. Remove the blank line at line 132 for consistency.
d3fde09
into
feat/supernova-async-exec
Reasoning behind the pull request
Proposed changes
Testing procedure
Pre-requisites
Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:
featbranch created?featbranch merging, do all satellite projects have a proper tag insidego.mod?