-
Notifications
You must be signed in to change notification settings - Fork 11.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
[Consensus] avoid subtracting from current Instant #18939
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
Let me add a bit of context. On Windows, the So when spinning up a local network on Windows with |
consensus/core/src/context.rs
Outdated
) | ||
}), | ||
) | ||
.unwrap(); |
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.
I assume this is safer than before, but what do you think about handling this unwrap
better? In my mind it might entail that we need to either change the return type, or have a default?
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.
When this happens, this is an invariant violation (system time overflow) which I don't think there is a reasonable way to fallback or recover. There is nothing the client can do to handle this either, when current time is unavailable. So crash is necessary here. Added a comment to turn the unwrap() into expect().
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.
This looks good to me, but would love if @akichidis and @arun-koshy can have a look.
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.
Thanks for the fix @mwtian!
## Description On Windows, `Instant::now()` can be close to 0 and we cannot subtract from that. ## Test plan CI. Verified on a Windows machine. --- ## Release notes Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required. For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates. - [ ] Protocol: - [ ] Nodes (Validators and Full nodes): - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [ ] CLI: - [ ] Rust SDK: - [ ] REST API:
Description
On Windows,
Instant::now()
can be close to 0 and we cannot subtract from that.Test plan
CI. Verified on a Windows machine.
Release notes
Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.