Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions core/src/core/block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Copy link
Copy Markdown
Contributor

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?

// 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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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.
Expand Down
6 changes: 6 additions & 0 deletions core/src/ser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 {
Expand Down Expand Up @@ -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"),
}
}
}
Expand All @@ -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",
}
}
}
Expand Down
26 changes: 25 additions & 1 deletion core/tests/block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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();
Expand Down
Loading