-
Notifications
You must be signed in to change notification settings - Fork 58
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
node: change block timestamp validation #1242
Conversation
11898c4
to
928c860
Compare
928c860
to
3b3c678
Compare
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 is keeping a timestamp condition as part of the validity requirements, which is what we're trying to remove (because it's well-known to be error-prone).
This PR is just reducing the problem while keeping the risk of rejecting a block for no valid reason.
I might be okay with a committee member deciding to ignore the candidate if the timestamp makes no sense (by which I mean it's like a day in the future or something like that), but a block that includes all valid transactions and reached a quorum in both committees should always considered as valid by other nodes.
Imagine the node's clock has a bug or stays behind for whatever reason... it will reject a block that was accepted by the rest of the network and eclipse itself
@@ -32,3 +32,5 @@ pub const RELAX_ITERATION_THRESHOLD: u8 = 10; | |||
|
|||
pub const ROUND_BASE_TIMEOUT: Duration = Duration::from_secs(5); | |||
pub const MAX_STEP_TIMEOUT: Duration = Duration::from_secs(60); | |||
|
|||
pub const MAX_BLOCK_SECS_DRIFT: u64 = 120; |
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 would strongly suggest to make this much larger.
The whole point of this PR is to remove the timestamp from the block logic validity, since it should have no significance in it.
We are keeping this check as a sanity check to avoid absurd timestamps. A 120-second time shift is definitely not absurd and still very risky IMHO.
- Change block timestamp to `u64` - Add `config::MAX_BLOCK_SECS_DRIFT`
3b3c678
to
5c535c6
Compare
Superseded by #1244 |
This pull request aims to remove every timestamp control concerning the previous block.
I just considered that if I receive a block from the future that is obvious invalid (let's say with a date a year from now), it doesn't really make sense to accept it.
So, I started thinking about what would be a reasonable drift to set, and for now, I've chosen 120 seconds.
See also #1062 (comment)