-
Notifications
You must be signed in to change notification settings - Fork 135
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
Check for int64 overflow when computing uint64 diff #2181
base: main
Are you sure you want to change the base?
Conversation
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2181 +/- ##
==========================================
+ Coverage 26.56% 26.65% +0.08%
==========================================
Files 363 363
Lines 16493 16520 +27
Branches 12 12
==========================================
+ Hits 4382 4403 +21
- Misses 11829 11833 +4
- Partials 282 284 +2
|
// add telemetry for the diff between payloadTimestamp and | ||
// consensusTimestamp | ||
err := sp.metrics.gaugePayloadConsensusTimestampDiff( | ||
payloadTimestamp, consensusTimestamp) |
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.
How about instead of all of this we go though time.Duration?
Turn the timestamp into a time.Time objects and get diff as TimeA.Sub(TimeB)?
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.
That I think would still result in us needing to deal with uint64 to int64 casting which would trigger gosec, so not any better than what's on main
branch already.
In #2171 we added the
beacon_kit.state.payload_consensus_timestamp_diff
metric for tracking the diff between the payload and consensus timestamps.In that PR we ignored int64 overflow which this PR addresses. Here we explicitly check ahead of time if an overflow will happen and log an error if that is the case.