[fix] #3262: Don't prematurely execute time-triggers before their `st…

…art` timestamp

Signed-off-by: Daniil Polyakov <arjentix@gmail.com>

client/tests/integration/triggers/time_trigger.rs

@@ -88,6 +88,8 @@ fn change_asset_metadata_after_1_sec() -> Result<()> {
 
     let (_rt, _peer, mut test_client) = <PeerBuilder>::new().with_port(10_660).start_with_runtime();
     wait_for_genesis_committed(&vec![test_client.clone()], 0);
+    // Sleep to certainly bypass time interval analyzed by genesis
+    std::thread::sleep(DEFAULT_CONSENSUS_ESTIMATION);
     let start_time = current_time();
 
     // Start listening BEFORE submitting any transaction not to miss any block committed event
@@ -115,7 +117,7 @@ fn change_asset_metadata_after_1_sec() -> Result<()> {
         &mut test_client,
         &account_id,
         Duration::from_secs(1),
-        usize::try_from(PERIOD.as_millis() / DEFAULT_CONSENSUS_ESTIMATION.as_millis() + 1)?,
+        2, // One block for trigger registration and one for execution
     )?;
 
     let value = test_client

core/src/smartcontracts/isi/triggers/mod.rs

@@ -34,6 +34,10 @@ pub mod isi {
                 }
             }
 
+            let last_block_estimation = wsv
+                .latest_block_ref()
+                .map(|block| block.header().timestamp() + block.header().consensus_estimation());
+
             let engine = wsv.engine.clone(); // Cloning engine is cheap
             let triggers = wsv.triggers_mut();
             let trigger_id = new_trigger.id().clone();
@@ -50,12 +54,32 @@ pub mod isi {
                         .try_into()
                         .map_err(|e: &str| Error::Conversion(e.to_owned()))?,
                 ),
-                TriggeringFilterBox::Time(_) => triggers.add_time_trigger(
-                    &engine,
-                    new_trigger
-                        .try_into()
-                        .map_err(|e: &str| Error::Conversion(e.to_owned()))?,
-                ),
+                TriggeringFilterBox::Time(time_filter) => {
+                    if let ExecutionTime::Schedule(schedule) = time_filter.0 {
+                        match last_block_estimation {
+                            // We're in genesis
+                            None => {
+                                return Err(Error::InvalidParameter(
+                                    InvalidParameterError::TimeTriggerInThePast,
+                                ));
+                            }
+                            Some(latest_block_estimation)
+                                if schedule.start < latest_block_estimation =>
+                            {
+                                return Err(Error::InvalidParameter(
+                                    InvalidParameterError::TimeTriggerInThePast,
+                                ));
+                            }
+                            Some(_) => (),
+                        }
+                    }
+                    triggers.add_time_trigger(
+                        &engine,
+                        new_trigger
+                            .try_into()
+                            .map_err(|e: &str| Error::Conversion(e.to_owned()))?,
+                    )
+                }
                 TriggeringFilterBox::ExecuteTrigger(_) => triggers.add_by_call_trigger(
                     &engine,
                     new_trigger

data_model/Cargo.toml

@@ -21,7 +21,7 @@ default = ["std"]
 # Enable static linkage of the rust standard library.
 # Disabled for WASM interoperability, to reduce the binary size.
 # Please refer to https://docs.rust-embedded.org/book/intro/no-std.html
-std = ["iroha_macro/std", "iroha_version/std", "iroha_crypto/std", "iroha_primitives/std", "thiserror", "displaydoc/std", "strum/std", "once_cell"]
+std = ["rand", "iroha_macro/std", "iroha_version/std", "iroha_crypto/std", "iroha_primitives/std", "thiserror", "displaydoc/std", "strum/std", "once_cell"]
 # Enable API for HTTP requests. Should be activated for HTTP clients
 http = ["std", "warp", "iroha_version/http"]
 # Replace structures and methods with FFI equivalents to facilitate dynamic linkage (mainly used in smartcontracts)
@@ -31,6 +31,8 @@ http = ["std", "warp", "iroha_version/http"]
 ffi_export = ["std", "iroha_ffi", "iroha_primitives/ffi_export", "iroha_crypto/ffi_export"]
 # Expose API for mutating structures (Internal use only)
 transparent_api = []
+# Activate random key generation
+rand = ["iroha_crypto/rand"]
 
 [dependencies]
 iroha_primitives = { workspace = true }

data_model/src/account.rs

@@ -440,6 +440,7 @@ pub mod prelude {
 }
 
 #[cfg(test)]
+#[cfg(feature = "rand")]
 mod tests {
     use core::cmp::Ordering;
 

data_model/src/events/time.rs

@@ -58,7 +58,7 @@ pub mod model {
     )]
     #[serde(transparent)]
     #[repr(transparent)]
-    pub struct TimeEventFilter(pub(super) ExecutionTime);
+    pub struct TimeEventFilter(pub ExecutionTime);
 
     /// Trigger execution time
     #[derive(
@@ -144,16 +144,29 @@ impl Filter for TimeEventFilter {
         match &self.0 {
             ExecutionTime::PreCommit => 1,
             ExecutionTime::Schedule(schedule) => {
-                let current_interval = event.prev_interval.map_or(event.interval, |prev| {
+                let mut current_interval = event.prev_interval.map_or(event.interval, |prev| {
                     let estimation = event.interval.since + event.interval.length;
-                    let prev_estimation = prev.since + prev.length;
+                    let prev_estimation = if Range::from(prev).contains(&schedule.start) {
+                        schedule.start
+                    } else {
+                        prev.since + prev.length
+                    };
 
                     TimeInterval {
                         since: prev_estimation,
                         length: estimation.saturating_sub(prev_estimation),
                     }
                 });
 
+                if schedule.start > event.interval.since {
+                    current_interval.length = std::cmp::min(
+                        current_interval.length,
+                        schedule.start.checked_sub(current_interval.since).expect(
+                            "`current_interval.since` is always smaller than `schedule.start`",
+                        ),
+                    )
+                }
+
                 count_matches_in_interval(schedule, &current_interval)
             }
         }
@@ -213,7 +226,7 @@ fn multiply_duration_by_u128(duration: Duration, n: u128) -> Duration {
 }
 
 impl Schedule {
-    /// Create new `Schedule` starting at `start` and without period
+    /// Create new [`Schedule`] starting at `start` and without period
     #[must_use]
     #[inline]
     pub const fn starting_at(start: Duration) -> Self {
@@ -326,10 +339,10 @@ mod tests {
             // ----|------[-----)----*----
             //     p     i1    i2
 
-            let schedule = Schedule::starting_at(Duration::from_secs(TIMESTAMP))
-                .with_period(Duration::from_secs(10));
-            let since = Duration::from_secs(TIMESTAMP + 35);
-            let length = Duration::from_secs(4);
+            let schedule = Schedule::starting_at(Duration::from_secs(TIMESTAMP - 5))
+                .with_period(Duration::from_secs(20));
+            let since = Duration::from_secs(TIMESTAMP);
+            let length = Duration::from_secs(5);
             let interval = TimeInterval { since, length };
             assert_eq!(count_matches_in_interval(&schedule, &interval), 0);
         }

data_model/src/isi.rs

@@ -1495,6 +1495,8 @@ pub mod error {
             ///
             /// i.e. too long [`AccountId`]
             NameLength,
+            /// Attempt to register a time-trigger with `start` point in the past
+            TimeTriggerInThePast,
         }
 
         /// Repetition of of `{instruction_type}` for id `{id}`

docs/source/references/schema.json

@@ -2320,6 +2320,10 @@
       {
         "tag": "NameLength",
         "discriminant": 1
+      },
+      {
+        "tag": "TimeTriggerInThePast",
+        "discriminant": 2
       }
     ]
   },