fix: Don't prematurely execute time-triggers before their start tim…

…estamp

Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
Signed-off-by: Shanin Roman <shanin1000@yandex.ru>

client/tests/integration/triggers/time_trigger.rs

@@ -3,10 +3,15 @@ use std::{str::FromStr as _, time::Duration};
 use eyre::Result;
 use iroha_client::{
     client::{self, Client, QueryResult},
-    data_model::{prelude::*, transaction::WasmSmartContract},
+    data_model::{
+        asset::AssetId,
+        events::pipeline::{BlockEventFilter, BlockStatus},
+        prelude::*,
+        transaction::WasmSmartContract,
+        Level,
+    },
 };
 use iroha_config::parameters::defaults::chain_wide::DEFAULT_CONSENSUS_ESTIMATION;
-use iroha_data_model::events::pipeline::{BlockEventFilter, BlockStatus};
 use iroha_logger::info;
 use test_network::*;
 use test_samples::{gen_account_in, ALICE_ID};
@@ -95,46 +100,54 @@ fn time_trigger_execution_count_error_should_be_less_than_15_percent() -> Result
 }
 
 #[test]
-fn change_asset_metadata_after_1_sec() -> Result<()> {
-    const PERIOD: Duration = Duration::from_secs(1);
-
-    let (_rt, _peer, mut test_client) = <PeerBuilder>::new().with_port(10_660).start_with_runtime();
+fn mint_asset_after_3_sec() -> Result<()> {
+    let (_rt, _peer, test_client) = <PeerBuilder>::new().with_port(10_660).start_with_runtime();
     wait_for_genesis_committed(&vec![test_client.clone()], 0);
-    let start_time = curr_time();
-
-    // Start listening BEFORE submitting any transaction not to miss any block committed event
-    let event_listener = get_block_committed_event_listener(&test_client)?;
+    // Sleep to certainly bypass time interval analyzed by genesis
+    std::thread::sleep(DEFAULT_CONSENSUS_ESTIMATION);
 
     let asset_definition_id = AssetDefinitionId::from_str("rose#wonderland").expect("Valid");
     let account_id = ALICE_ID.clone();
-    let key = Name::from_str("petal")?;
+    let asset_id = AssetId::new(asset_definition_id.clone(), account_id.clone());
 
-    let schedule = TimeSchedule::starting_at(start_time + PERIOD);
-    let instruction =
-        SetKeyValue::asset_definition(asset_definition_id.clone(), key.clone(), 3_u32);
+    let init_quantity = test_client.request(FindAssetQuantityById {
+        id: asset_id.clone(),
+    })?;
+
+    let start_time = curr_time();
+    // Create trigger with schedule which is in the future to the new block but within block estimation time
+    let schedule = TimeSchedule::starting_at(start_time + Duration::from_secs(3));
+    let instruction = Mint::asset_numeric(1_u32, asset_id.clone());
     let register_trigger = Register::trigger(Trigger::new(
-        "change_rose_metadata".parse().expect("Valid"),
+        "mint_rose".parse().expect("Valid"),
         Action::new(
             vec![instruction],
             Repeats::from(1_u32),
             account_id.clone(),
             TimeEventFilter::new(ExecutionTime::Schedule(schedule)),
         ),
     ));
-    test_client.submit(register_trigger)?;
-    submit_sample_isi_on_every_block_commit(
-        event_listener,
-        &mut test_client,
-        &account_id,
-        Duration::from_secs(1),
-        usize::try_from(PERIOD.as_millis() / DEFAULT_CONSENSUS_ESTIMATION.as_millis() + 1)?,
-    )?;
+    test_client.submit_blocking(register_trigger)?;
 
-    let value = test_client.request(FindAssetDefinitionKeyValueByIdAndKey {
-        id: asset_definition_id,
-        key,
+    // Schedule start is in the future so trigger isn't executed after creating a new block
+    test_client.submit_blocking(Log::new(Level::DEBUG, "Just to create block".to_string()))?;
+    let after_registration_quantity = test_client.request(FindAssetQuantityById {
+        id: asset_id.clone(),
     })?;
-    assert_eq!(value, numeric!(3).into());
+    assert_eq!(init_quantity, after_registration_quantity);
+
+    // Sleep long enough that trigger start is in the past
+    std::thread::sleep(DEFAULT_CONSENSUS_ESTIMATION);
+    test_client.submit_blocking(Log::new(Level::DEBUG, "Just to create block".to_string()))?;
+
+    let after_wait_quantity = test_client.request(FindAssetQuantityById {
+        id: asset_id.clone(),
+    })?;
+    // Schedule is in the past now so trigger is executed
+    assert_eq!(
+        init_quantity.checked_add(1u32.into()).unwrap(),
+        after_wait_quantity
+    );
 
     Ok(())
 }

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

@@ -13,6 +13,8 @@ pub mod specialized;
 /// - TODO: authorities.
 /// - TODO: authority permissions.
 pub mod isi {
+    use std::time::Duration;
+
     use iroha_data_model::{
         events::EventFilter,
         isi::error::{InvalidParameterError, RepetitionError},
@@ -39,6 +41,11 @@ pub mod isi {
                 }
             }
 
+            let last_block_estimation = state_transaction.latest_block_ref().map(|block| {
+                block.header().timestamp()
+                    + Duration::from_millis(block.header().consensus_estimation_ms)
+            });
+
             let engine = state_transaction.engine.clone(); // Cloning engine is cheap
             let triggers = &mut state_transaction.world.triggers;
             let trigger_id = new_trigger.id().clone();
@@ -55,12 +62,32 @@ pub mod isi {
                         .try_into()
                         .map_err(|e: &str| Error::Conversion(e.to_owned()))?,
                 ),
-                TriggeringEventFilterBox::Time(_) => triggers.add_time_trigger(
-                    &engine,
-                    new_trigger
-                        .try_into()
-                        .map_err(|e: &str| Error::Conversion(e.to_owned()))?,
-                ),
+                TriggeringEventFilterBox::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()))?,
+                    )
+                }
                 TriggeringEventFilterBox::ExecuteTrigger(_) => triggers.add_by_call_trigger(
                     &engine,
                     new_trigger

data_model/src/account.rs

@@ -244,6 +244,7 @@ pub mod prelude {
 mod tests {
     use super::*;
 
+    #[cfg(feature = "transparent_api")]
     #[test]
     fn parse_account_id() {
         const SIGNATORY: &str =

data_model/src/events/time.rs

@@ -58,7 +58,7 @@ mod model {
     )]
     #[serde(transparent)]
     #[repr(transparent)]
-    pub struct TimeEventFilter(pub(super) ExecutionTime);
+    pub struct TimeEventFilter(pub ExecutionTime);
 
     /// Trigger execution time
     #[derive(
@@ -144,14 +144,46 @@ impl EventFilter for TimeEventFilter {
         match &self.0 {
             ExecutionTime::PreCommit => 1,
             ExecutionTime::Schedule(schedule) => {
+                // Prevent matching in the future it will be handled by the next block
+                if schedule.start > event.interval.since {
+                    return 0;
+                }
+
                 let current_interval = event.prev_interval.map_or(event.interval, |prev| {
+                    // Case 1:
+                    // ----|-----[--[--)--)-----
+                    //     s    p1 c1  p2 c2
+                    //
+                    // Schedule start was before previous block (p1).
+                    // In this case we only care about interval [p2, c2)
+                    // Because everything up to p2 (excluding) was processed in the previous blocks.
+                    //
+                    // Case 2:
+                    // ---------[-|-[--)--)-----
+                    //         p1 s c1 p2 c2
+                    //
+                    // ---------[--)--|--[--)---
+                    //          p1 p2 s  c1 c2
+                    //
+                    // Schedule start is between previous block (p1) and current block (c1).
+                    // In this case we care about either interval [s, c2) if (s) is in [p1, p2) or [p2, c2) if (s) is after (p2).
+                    // Because in the previous block [p1, p2) event won't match since (s) was in the future.
+                    //
+                    // Case 3:
+                    // ---------[--[-|-)--)-----
+                    //         p1  c1 s p2 c2
+                    //
+                    // Schedule start is after current block (c1).
+                    // In this case event won't match and it will be handled in the next block.
+                    let since = if Range::from(prev).contains(&schedule.start) {
+                        schedule.start
+                    } else {
+                        prev.since + prev.length
+                    };
                     let estimation = event.interval.since + event.interval.length;
-                    let prev_estimation = prev.since + prev.length;
+                    let length = estimation - since;
 
-                    TimeInterval {
-                        since: prev_estimation,
-                        length: estimation.saturating_sub(prev_estimation),
-                    }
+                    TimeInterval { since, length }
                 });
 
                 count_matches_in_interval(schedule, &current_interval)
@@ -213,7 +245,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 {
@@ -251,13 +283,13 @@ pub mod prelude {
 mod tests {
     use super::*;
 
+    /// Sample timestamp
+    const TIMESTAMP: u64 = 1_647_443_386;
+
     /// Tests for `count_matches_in_interval()`
     mod count_matches_in_interval {
         use super::*;
 
-        /// Sample timestamp
-        const TIMESTAMP: u64 = 1_647_443_386;
-
         #[test]
         fn test_no_period_before_left_border() {
             // ----|-----[-----)-------
@@ -429,4 +461,139 @@ mod tests {
             assert_eq!(count_matches_in_interval(&schedule, &interval), 7);
         }
     }
+
+    // Tests for [`TimeEventFilter`]
+    mod time_event_filter {
+        use super::*;
+
+        #[test]
+        fn test_schedule_start_before_prev_interval() {
+            //
+            // ----|---[--*--)--*--[--*--)----
+            //     s   p1    p2   c1    c2
+
+            let schedule = Schedule::starting_at(Duration::from_secs(TIMESTAMP))
+                .with_period(Duration::from_secs(10));
+            let filter = TimeEventFilter(ExecutionTime::Schedule(schedule));
+
+            let since = Duration::from_secs(TIMESTAMP + 5);
+            let length = Duration::from_secs(10);
+            let prev_interval = TimeInterval { since, length };
+
+            let since = Duration::from_secs(TIMESTAMP + 25);
+            let length = Duration::from_secs(10);
+            let interval = TimeInterval { since, length };
+
+            let event = TimeEvent {
+                prev_interval: Some(prev_interval),
+                interval,
+            };
+
+            assert_eq!(filter.count_matches(&event), 2);
+        }
+
+        #[test]
+        fn test_schedule_start_inside_prev_interval() {
+            //
+            // -------[--|--)--*--[--*--)----
+            //        p1 s  p2   c1    c2
+
+            let schedule = Schedule::starting_at(Duration::from_secs(TIMESTAMP + 5))
+                .with_period(Duration::from_secs(10));
+            let filter = TimeEventFilter(ExecutionTime::Schedule(schedule));
+
+            let since = Duration::from_secs(TIMESTAMP);
+            let length = Duration::from_secs(10);
+            let prev_interval = TimeInterval { since, length };
+
+            let since = Duration::from_secs(TIMESTAMP + 20);
+            let length = Duration::from_secs(10);
+            let interval = TimeInterval { since, length };
+
+            let event = TimeEvent {
+                prev_interval: Some(prev_interval),
+                interval,
+            };
+
+            assert_eq!(filter.count_matches(&event), 3);
+        }
+
+        #[test]
+        fn test_schedule_start_between_intervals() {
+            //
+            // -------[----)--|--[--*--)----
+            //        p1   p2 s  c1    c2
+
+            let schedule = Schedule::starting_at(Duration::from_secs(TIMESTAMP + 15))
+                .with_period(Duration::from_secs(10));
+            let filter = TimeEventFilter(ExecutionTime::Schedule(schedule));
+
+            let since = Duration::from_secs(TIMESTAMP);
+            let length = Duration::from_secs(10);
+            let prev_interval = TimeInterval { since, length };
+
+            let since = Duration::from_secs(TIMESTAMP + 20);
+            let length = Duration::from_secs(10);
+            let interval = TimeInterval { since, length };
+
+            let event = TimeEvent {
+                prev_interval: Some(prev_interval),
+                interval,
+            };
+
+            assert_eq!(filter.count_matches(&event), 2);
+        }
+
+        #[test]
+        fn test_schedule_start_inside_current_interval() {
+            //
+            // -------[----)----[--|--)----
+            //        p1   p2   c1 s  c2
+
+            let schedule = Schedule::starting_at(Duration::from_secs(TIMESTAMP + 25))
+                .with_period(Duration::from_secs(10));
+            let filter = TimeEventFilter(ExecutionTime::Schedule(schedule));
+
+            let since = Duration::from_secs(TIMESTAMP);
+            let length = Duration::from_secs(10);
+            let prev_interval = TimeInterval { since, length };
+
+            let since = Duration::from_secs(TIMESTAMP + 20);
+            let length = Duration::from_secs(10);
+            let interval = TimeInterval { since, length };
+
+            let event = TimeEvent {
+                prev_interval: Some(prev_interval),
+                interval,
+            };
+
+            assert_eq!(filter.count_matches(&event), 0);
+        }
+
+        #[test]
+        fn test_schedule_start_after_current_interval() {
+            //
+            // -------[----)----[----)--|--
+            //        p1   p2   c1   c2 s
+
+            let schedule = Schedule::starting_at(Duration::from_secs(TIMESTAMP + 35))
+                .with_period(Duration::from_secs(10));
+            let filter = TimeEventFilter(ExecutionTime::Schedule(schedule));
+
+            let since = Duration::from_secs(TIMESTAMP);
+            let length = Duration::from_secs(10);
+            let prev_interval = TimeInterval { since, length };
+
+            let since = Duration::from_secs(TIMESTAMP + 20);
+            let length = Duration::from_secs(10);
+            let interval = TimeInterval { since, length };
+
+            let event = TimeEvent {
+                prev_interval: Some(prev_interval),
+                interval,
+            };
+
+            assert_eq!(filter.count_matches(&event), 0);
+        }
+    }
 }