-
Notifications
You must be signed in to change notification settings - Fork 980
fix: use FutureTimeLimit instead of CorruptedData for clock skew #3896
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
base: staging
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -448,14 +448,16 @@ impl Readable for UntrustedBlockHeader { | |
| let header = read_block_header(reader)?; | ||
| let ftl = global::get_future_time_limit(); | ||
| if header.timestamp > Utc::now() + Duration::seconds(ftl as i64) { | ||
| // refuse blocks whose timestamp is too far in the future | ||
| // this future_time_limit (FTL) is specified in grin-server.toml | ||
| // TODO add warning in p2p code if local time is too different from peers | ||
| error!( | ||
| "block header {} validation error: block time is more than {} seconds in the future", | ||
| // Refuse blocks whose timestamp is too far in the future. | ||
| // FTL is configured in grin-server.toml. This is *not* corrupted data — | ||
| // local clocks differ; use a dedicated error so peers are not treated | ||
| // as sending garbage (#3360). | ||
| // TODO: warn in p2p if local time is systematically different from peers | ||
| warn!( | ||
| "block header {} rejected: block time is more than {} seconds in the future (clock skew?)", | ||
| header.hash(), ftl | ||
| ); | ||
| return Err(ser::Error::CorruptedData); | ||
| return Err(ser::Error::FutureTimeLimit); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This still becomes a generic P2P serialization error and closes the connection. Is this intentionally diagnostics-only, or should future timestamps be handled without dropping the peer? |
||
| } | ||
|
|
||
| // Check the block version before proceeding any further. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -72,6 +72,10 @@ pub enum Error { | |
| InvalidBlockVersion, | ||
| /// Unsupported protocol version | ||
| UnsupportedProtocolVersion, | ||
| /// Block/header timestamp is beyond the future time limit. | ||
| /// Not corrupted data — peer clocks may simply disagree; treat as non-fatal | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2P currently treats all serialization errors the same, so this variant does not change peer reputation yet. Could we keep the comment focused on the timestamp condition? |
||
| /// for peer reputation (see #3360). | ||
| FutureTimeLimit, | ||
| } | ||
|
|
||
| impl From<io::Error> for Error { | ||
|
|
@@ -102,6 +106,7 @@ impl fmt::Display for Error { | |
| Error::HexError(ref e) => write!(f, "hex error {:?}", e), | ||
| Error::InvalidBlockVersion => f.write_str("invalid block version"), | ||
| Error::UnsupportedProtocolVersion => f.write_str("unsupported protocol version"), | ||
| Error::FutureTimeLimit => f.write_str("future time limit exceeded"), | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -126,6 +131,7 @@ impl error::Error for Error { | |
| Error::HexError(_) => "hex error", | ||
| Error::InvalidBlockVersion => "invalid block version", | ||
| Error::UnsupportedProtocolVersion => "unsupported protocol version", | ||
| Error::FutureTimeLimit => "future time limit exceeded", | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,7 +26,7 @@ use crate::core::core::{Committed, CompactBlock}; | |
| use crate::core::libtx::build::{self, input, output}; | ||
| use crate::core::libtx::ProofBuilder; | ||
| use crate::core::{global, pow, ser}; | ||
| use chrono::Duration; | ||
| use chrono::{Duration, Utc}; | ||
| use grin_core as core; | ||
| use keychain::{BlindingFactor, ExtKeychain, Keychain}; | ||
| use util::{secp, ToHex}; | ||
|
|
@@ -466,6 +466,30 @@ fn deserialize_untrusted_header_weight() { | |
| assert!(res.is_ok()); | ||
| } | ||
|
|
||
| /// Future timestamps must not be reported as CorruptedData (#3360). | ||
| #[test] | ||
| fn deserialize_untrusted_header_future_time() { | ||
| test_setup(); | ||
| let keychain = ExtKeychain::from_random_seed(false).unwrap(); | ||
| let builder = ProofBuilder::new(&keychain); | ||
| let prev = BlockHeader::default(); | ||
| let key_id = ExtKeychain::derive_key_id(1, 1, 0, 0, 0); | ||
| let mut b = new_block(&[], &keychain, &builder, &prev, &key_id); | ||
|
|
||
| // Far beyond future_time_limit (default 300s for AutomatedTesting? check global) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The default is 300 seconds, so the question can go. Since timestamp is checked before PoW, could this use a plain header and avoid mining a proof? |
||
| b.header.timestamp = Utc::now() + Duration::seconds(3600); | ||
| set_pow(&mut b.header); | ||
|
|
||
| let mut vec = Vec::new(); | ||
| ser::serialize_default(&mut vec, &b.header).expect("serialization failed"); | ||
| let res: Result<UntrustedBlockHeader, _> = ser::deserialize_default(&mut &vec[..]); | ||
| assert_eq!( | ||
| res.err(), | ||
| Some(ser::Error::FutureTimeLimit), | ||
| "future time must use FutureTimeLimit, not CorruptedData" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn serialize_deserialize_block() { | ||
| test_setup(); | ||
|
|
||
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.
Could we shorten this to describe the rejection reason and leave the issue history in the PR?