From 8c751320ce935a6715661e6f10e523e05cf19eb9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bastian=20K=C3=B6cher?= <git@kchr.de>
Date: Tue, 14 Feb 2023 16:17:37 +0100
Subject: [PATCH] pallet-timestamp: Remove `ValidAtTimestamp` error variant
 (#13346)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* pallet-timestamp: Remove `ValidAtTimestamp` error variant

The error variant wasn't that useful and it was also used wrongly in the code. In the code we
returned this variant when the `timestamp < minimum`. The problem of this is that we waited on the
node side some time, but then `set` function rejects the timestamp because of the same check (the
timestamp in the block stays the same). We ensure that the timestamp isn't drifting too much in the
future, but waiting for the timestamp to be "valid" would open some attack vector. The consensus
protocols also compare the slots in the blocks to ensure that there isn't a block from the future
and in the runtime we then ensure that `slot = timestamp / slot_duration`. So, we can just remove
this variant and replace it with a new variant `TimeBetweenBlocksTooShort` to not even try importing
a block which uses a too short delay since the last block.

* Update primitives/timestamp/src/lib.rs

Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com>

* Rename to `TooEarly`

* FMT

---------

Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com>
---
 frame/timestamp/src/lib.rs      |  2 +-
 primitives/timestamp/src/lib.rs | 44 ++++++++-------------------------
 2 files changed, 11 insertions(+), 35 deletions(-)

diff --git a/frame/timestamp/src/lib.rs b/frame/timestamp/src/lib.rs
index 6e9c887824d17..61a46125a423b 100644
--- a/frame/timestamp/src/lib.rs
+++ b/frame/timestamp/src/lib.rs
@@ -255,7 +255,7 @@ pub mod pallet {
 			if t > *(data + MAX_TIMESTAMP_DRIFT_MILLIS) {
 				Err(InherentError::TooFarInFuture)
 			} else if t < minimum {
-				Err(InherentError::ValidAtTimestamp(minimum.into()))
+				Err(InherentError::TooEarly)
 			} else {
 				Ok(())
 			}
diff --git a/primitives/timestamp/src/lib.rs b/primitives/timestamp/src/lib.rs
index 14b06779340f2..0ec079816c10a 100644
--- a/primitives/timestamp/src/lib.rs
+++ b/primitives/timestamp/src/lib.rs
@@ -134,10 +134,12 @@ impl From<Duration> for Timestamp {
 #[derive(Encode, sp_runtime::RuntimeDebug)]
 #[cfg_attr(feature = "std", derive(Decode, thiserror::Error))]
 pub enum InherentError {
-	/// The timestamp is valid in the future.
-	/// This is a non-fatal-error and will not stop checking the inherents.
-	#[cfg_attr(feature = "std", error("Block will be valid at {0}."))]
-	ValidAtTimestamp(InherentType),
+	/// The time between the blocks is too short.
+	#[cfg_attr(
+		feature = "std",
+		error("The time since the last timestamp is lower than the minimum period.")
+	)]
+	TooEarly,
 	/// The block timestamp is too far in the future
 	#[cfg_attr(feature = "std", error("The timestamp of the block is too far in the future."))]
 	TooFarInFuture,
@@ -146,7 +148,7 @@ pub enum InherentError {
 impl IsFatalError for InherentError {
 	fn is_fatal_error(&self) -> bool {
 		match self {
-			InherentError::ValidAtTimestamp(_) => false,
+			InherentError::TooEarly => true,
 			InherentError::TooFarInFuture => true,
 		}
 	}
@@ -240,34 +242,8 @@ impl sp_inherents::InherentDataProvider for InherentDataProvider {
 		identifier: &InherentIdentifier,
 		error: &[u8],
 	) -> Option<Result<(), sp_inherents::Error>> {
-		if *identifier != INHERENT_IDENTIFIER {
-			return None
-		}
-
-		match InherentError::try_from(&INHERENT_IDENTIFIER, error)? {
-			InherentError::ValidAtTimestamp(valid) => {
-				let max_drift = self.max_drift;
-				let timestamp = self.timestamp;
-				// halt import until timestamp is valid.
-				// reject when too far ahead.
-				if valid > timestamp + max_drift {
-					return Some(Err(sp_inherents::Error::Application(Box::from(
-						InherentError::TooFarInFuture,
-					))))
-				}
-
-				let diff = valid.checked_sub(timestamp).unwrap_or_default();
-				log::info!(
-					target: "timestamp",
-					"halting for block {} milliseconds in the future",
-					diff.0,
-				);
-
-				futures_timer::Delay::new(diff.as_duration()).await;
-
-				Some(Ok(()))
-			},
-			o => Some(Err(sp_inherents::Error::Application(Box::from(o)))),
-		}
+		Some(Err(sp_inherents::Error::Application(Box::from(InherentError::try_from(
+			identifier, error,
+		)?))))
 	}
 }