From 8047698fe6786ae51e6d0fe811203a6551c82d98 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Thu, 9 Feb 2023 12:03:11 +0100 Subject: [PATCH 1/4] 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. --- frame/timestamp/src/lib.rs | 2 +- primitives/timestamp/src/lib.rs | 41 ++++++--------------------------- 2 files changed, 8 insertions(+), 35 deletions(-) diff --git a/frame/timestamp/src/lib.rs b/frame/timestamp/src/lib.rs index e859474c2cb9e..656b2bd9493b3 100644 --- a/frame/timestamp/src/lib.rs +++ b/frame/timestamp/src/lib.rs @@ -258,7 +258,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::TimeBetweenBlocksTooShort) } else { Ok(()) } diff --git a/primitives/timestamp/src/lib.rs b/primitives/timestamp/src/lib.rs index 14b06779340f2..aba9308a03af8 100644 --- a/primitives/timestamp/src/lib.rs +++ b/primitives/timestamp/src/lib.rs @@ -134,10 +134,9 @@ impl From 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 between the blocks is too short."))] + TimeBetweenBlocksTooShort, /// 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 +145,7 @@ pub enum InherentError { impl IsFatalError for InherentError { fn is_fatal_error(&self) -> bool { match self { - InherentError::ValidAtTimestamp(_) => false, + InherentError::TimeBetweenBlocksTooShort => true, InherentError::TooFarInFuture => true, } } @@ -240,34 +239,8 @@ impl sp_inherents::InherentDataProvider for InherentDataProvider { identifier: &InherentIdentifier, error: &[u8], ) -> Option> { - 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, + )?)))) } } From 4d46477dc2e59b6c3c036eaf1fb2225e779de8d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Tue, 14 Feb 2023 09:38:24 +0100 Subject: [PATCH 2/4] Update primitives/timestamp/src/lib.rs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com> --- primitives/timestamp/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/primitives/timestamp/src/lib.rs b/primitives/timestamp/src/lib.rs index aba9308a03af8..e893be5784b63 100644 --- a/primitives/timestamp/src/lib.rs +++ b/primitives/timestamp/src/lib.rs @@ -135,7 +135,7 @@ impl From for Timestamp { #[cfg_attr(feature = "std", derive(Decode, thiserror::Error))] pub enum InherentError { /// The time between the blocks is too short. - #[cfg_attr(feature = "std", error("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."))] TimeBetweenBlocksTooShort, /// 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."))] From 75a631af129c760da824a86d7a2040932ac5ba37 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Tue, 14 Feb 2023 09:47:49 +0100 Subject: [PATCH 3/4] Rename to `TooEarly` --- frame/timestamp/src/lib.rs | 2 +- primitives/timestamp/src/lib.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/frame/timestamp/src/lib.rs b/frame/timestamp/src/lib.rs index 656b2bd9493b3..22c7c1620c661 100644 --- a/frame/timestamp/src/lib.rs +++ b/frame/timestamp/src/lib.rs @@ -258,7 +258,7 @@ pub mod pallet { if t > *(data + MAX_TIMESTAMP_DRIFT_MILLIS) { Err(InherentError::TooFarInFuture) } else if t < minimum { - Err(InherentError::TimeBetweenBlocksTooShort) + Err(InherentError::TooEarly) } else { Ok(()) } diff --git a/primitives/timestamp/src/lib.rs b/primitives/timestamp/src/lib.rs index e893be5784b63..e652b2cc8d6a3 100644 --- a/primitives/timestamp/src/lib.rs +++ b/primitives/timestamp/src/lib.rs @@ -136,7 +136,7 @@ impl From for Timestamp { pub enum InherentError { /// 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."))] - TimeBetweenBlocksTooShort, + 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, @@ -145,7 +145,7 @@ pub enum InherentError { impl IsFatalError for InherentError { fn is_fatal_error(&self) -> bool { match self { - InherentError::TimeBetweenBlocksTooShort => true, + InherentError::TooEarly => true, InherentError::TooFarInFuture => true, } } From 576a2e78a30d0f79ae90ad05194302c20d14773d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Tue, 14 Feb 2023 09:49:19 +0100 Subject: [PATCH 4/4] FMT --- primitives/timestamp/src/lib.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/primitives/timestamp/src/lib.rs b/primitives/timestamp/src/lib.rs index e652b2cc8d6a3..0ec079816c10a 100644 --- a/primitives/timestamp/src/lib.rs +++ b/primitives/timestamp/src/lib.rs @@ -135,7 +135,10 @@ impl From for Timestamp { #[cfg_attr(feature = "std", derive(Decode, thiserror::Error))] pub enum InherentError { /// 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."))] + #[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."))]