fix: use FutureTimeLimit instead of CorruptedData for clock skew#3896
fix: use FutureTimeLimit instead of CorruptedData for clock skew#3896iho wants to merge 1 commit into
Conversation
Headers slightly beyond future_time_limit are not corrupted data — clocks differ. Return ser::Error::FutureTimeLimit so the condition is explicit and not lumped with real corruption/POW failures. Addresses mimblewimble#3360.
wiesche89
left a comment
wiesche89
left a comment
There was a problem hiding this comment.
The new error is clearer, but it remains diagnostics-only. P2P still closes the connection for every serialization error. The PR description should make that scope clear, and the test can be simplified because timestamp is checked before PoW.
| header.hash(), ftl | ||
| ); | ||
| return Err(ser::Error::CorruptedData); | ||
| return Err(ser::Error::FutureTimeLimit); |
There was a problem hiding this comment.
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?
| /// 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 |
There was a problem hiding this comment.
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?
| 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) |
There was a problem hiding this comment.
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?
| // 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. |
There was a problem hiding this comment.
Could we shorten this to describe the rejection reason and leave the issue history in the PR?
2 participants
Summary
Fixes #3360.
UntrustedBlockHeaderrejected headers pastfuture_time_limitwithser::Error::CorruptedData. That label is misleading: the bytes are well-formed; local clocks may simply disagree (miners near the FTL boundary). Maintainers already noted we need more informative deserialization errors for this case.Change
ser::Error::FutureTimeLimitwarn!) when a header timestamp is beyond FTLCorruptedDataFutureTimeLimit, notCorruptedDataP2P still treats all serialization errors as non-bannable (
Error::Serialization); this mainly improves logging/clarity and future handling if finer peer policy is added.Test plan
cargo test -p grin_core --test block deserialize_untrusted_header_future_timecargo test -p grin_core --test block -- --test-threads=1(28 passed)