From 0729420b80fa4c6e68c6c5617e9b58dc0e4a3f9c Mon Sep 17 00:00:00 2001 From: Shanin Roman <40040452+Erigara@users.noreply.github.com> Date: Tue, 8 Oct 2024 07:46:13 +0300 Subject: [PATCH] feat: remove consensus estimation (#5116) Signed-off-by: Shanin Roman --- .../integration/triggers/time_trigger.rs | 91 +--------- crates/iroha_core/src/block.rs | 7 - .../src/smartcontracts/isi/triggers/mod.rs | 12 +- crates/iroha_core/src/state.rs | 18 +- .../benches/time_event_filter.rs | 5 +- crates/iroha_data_model/src/block.rs | 8 - .../iroha_data_model/src/events/pipeline.rs | 1 - crates/iroha_data_model/src/events/time.rs | 170 +++--------------- crates/iroha_data_model/src/parameter.rs | 6 - crates/iroha_schema_gen/src/lib.rs | 1 - docs/source/references/schema.json | 11 -- 11 files changed, 47 insertions(+), 283 deletions(-) diff --git a/crates/iroha/tests/integration/triggers/time_trigger.rs b/crates/iroha/tests/integration/triggers/time_trigger.rs index 6a952098dd6..bc641812975 100644 --- a/crates/iroha/tests/integration/triggers/time_trigger.rs +++ b/crates/iroha/tests/integration/triggers/time_trigger.rs @@ -15,18 +15,10 @@ use iroha_test_network::*; use iroha_test_samples::{gen_account_in, load_sample_wasm, ALICE_ID}; /// Default estimation of consensus duration. -pub fn default_consensus_estimation() -> Duration { +pub fn pipeline_time() -> Duration { let default_parameters = SumeragiParameters::default(); - default_parameters - .block_time() - .checked_add( - default_parameters - .commit_time() - .checked_div(2) - .map_or_else(|| unreachable!(), |x| x), - ) - .map_or_else(|| unreachable!(), |x| x) + default_parameters.pipeline_time() } fn curr_time() -> core::time::Duration { @@ -37,87 +29,12 @@ fn curr_time() -> core::time::Duration { .unwrap() } -/// Macro to abort compilation, if `e` isn't `true` -macro_rules! const_assert { - ($e:expr) => { - #[allow(trivial_casts)] - const _: usize = ($e as bool) as usize - 1; - }; -} - -/// Time-based triggers and block commitment process depend heavily on **current time** and **CPU**, -/// so it's impossible to create fully reproducible test scenario. -/// -/// But in general it works well and this test demonstrates it -#[test] -#[allow(clippy::cast_precision_loss)] -fn time_trigger_execution_count_error_should_be_less_than_15_percent() -> Result<()> { - const PERIOD: Duration = Duration::from_millis(100); - const ACCEPTABLE_ERROR_PERCENT: u8 = 15; - assert!(PERIOD.as_millis() < default_consensus_estimation().as_millis()); - const_assert!(ACCEPTABLE_ERROR_PERCENT <= 100); - - let (_rt, _peer, mut test_client) = ::new().with_port(10_775).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)?; - - let account_id = ALICE_ID.clone(); - let asset_definition_id = "rose#wonderland".parse().expect("Valid"); - let asset_id = AssetId::new(asset_definition_id, account_id.clone()); - - let prev_value = get_asset_value(&mut test_client, asset_id.clone()); - - let schedule = TimeSchedule::starting_at(start_time).with_period(PERIOD); - let instruction = Mint::asset_numeric(1u32, asset_id.clone()); - let register_trigger = Register::trigger(Trigger::new( - "mint_rose".parse()?, - Action::new( - vec![instruction], - Repeats::Indefinitely, - 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), - 3, - )?; - std::thread::sleep(default_consensus_estimation()); - - let finish_time = curr_time(); - let average_count = finish_time.saturating_sub(start_time).as_millis() / PERIOD.as_millis(); - - let actual_value = get_asset_value(&mut test_client, asset_id); - let expected_value = prev_value - .checked_add(Numeric::new(average_count, 0)) - .unwrap(); - let acceptable_error = expected_value.to_f64() * (f64::from(ACCEPTABLE_ERROR_PERCENT) / 100.0); - let error = core::cmp::max(actual_value, expected_value) - .checked_sub(core::cmp::min(actual_value, expected_value)) - .unwrap() - .to_f64(); - assert!( - error < acceptable_error, - "error = {error}, but acceptable error = {acceptable_error}" - ); - - Ok(()) -} - #[test] fn mint_asset_after_3_sec() -> Result<()> { let (_rt, _peer, test_client) = ::new().with_port(10_665).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()); + std::thread::sleep(pipeline_time()); let asset_definition_id = "rose#wonderland" .parse::() @@ -152,7 +69,7 @@ fn mint_asset_after_3_sec() -> Result<()> { assert_eq!(init_quantity, after_registration_quantity); // Sleep long enough that trigger start is in the past - std::thread::sleep(default_consensus_estimation()); + std::thread::sleep(pipeline_time()); test_client.submit_blocking(Log::new(Level::DEBUG, "Just to create block".to_string()))?; let after_wait_quantity = test_client.query_single(FindAssetQuantityById { diff --git a/crates/iroha_core/src/block.rs b/crates/iroha_core/src/block.rs index 98ff226687b..4634c4c9bd4 100644 --- a/crates/iroha_core/src/block.rs +++ b/crates/iroha_core/src/block.rs @@ -157,7 +157,6 @@ mod pending { prev_block: Option<&SignedBlock>, view_change_index: usize, transactions: &[CommittedTransaction], - consensus_estimation: Duration, ) -> BlockHeader { let prev_block_time = prev_block.map_or(Duration::ZERO, |block| block.header().creation_time()); @@ -206,10 +205,6 @@ mod pending { view_change_index: view_change_index .try_into() .expect("View change index should fit into u32"), - consensus_estimation_ms: consensus_estimation - .as_millis() - .try_into() - .expect("INTERNAL BUG: Time should fit into u64"), } } @@ -254,7 +249,6 @@ mod pending { state.latest_block().as_deref(), view_change_index, &transactions, - state.world.parameters().sumeragi.consensus_estimation(), ), transactions, })) @@ -793,7 +787,6 @@ mod valid { )), creation_time_ms: 0, view_change_index: 0, - consensus_estimation_ms: 4_000, }, transactions: Vec::new(), }; diff --git a/crates/iroha_core/src/smartcontracts/isi/triggers/mod.rs b/crates/iroha_core/src/smartcontracts/isi/triggers/mod.rs index d729a78c5c0..10942d54c75 100644 --- a/crates/iroha_core/src/smartcontracts/isi/triggers/mod.rs +++ b/crates/iroha_core/src/smartcontracts/isi/triggers/mod.rs @@ -39,9 +39,9 @@ pub mod isi { } } - let last_block_estimation = state_transaction.latest_block().map(|block| { - block.header().creation_time() + block.header().consensus_estimation() - }); + let latest_block_time = state_transaction + .latest_block() + .map(|block| block.header().creation_time()); let engine = state_transaction.engine.clone(); // Cloning engine is cheap let genesis_creation_time_ms = state_transaction.world().genesis_creation_time_ms(); @@ -63,7 +63,7 @@ pub mod isi { ), EventFilterBox::Time(time_filter) => { if let ExecutionTime::Schedule(schedule) = time_filter.0 { - match last_block_estimation { + match latest_block_time { // Genesis block None => { let genesis_creation_time_ms = genesis_creation_time_ms @@ -75,8 +75,8 @@ pub mod isi { )); } } - Some(latest_block_estimation) => { - if schedule.start() < latest_block_estimation { + Some(latest_block_time) => { + if schedule.start() < latest_block_time { return Err(Error::InvalidParameter( InvalidParameterError::TimeTriggerInThePast, )); diff --git a/crates/iroha_core/src/state.rs b/crates/iroha_core/src/state.rs index 98411cee24b..95b3439580d 100644 --- a/crates/iroha_core/src/state.rs +++ b/crates/iroha_core/src/state.rs @@ -1455,21 +1455,17 @@ impl<'state> StateBlock<'state> { /// Create time event using previous and current blocks fn create_time_event(&self, block: &CommittedBlock) -> TimeEvent { - let prev_interval = self.latest_block().map(|latest_block| { - let header = &latest_block.as_ref().header(); + let to = block.as_ref().header().creation_time(); - TimeInterval::new(header.creation_time(), header.consensus_estimation()) + let since = self.latest_block().map_or(to, |latest_block| { + let header = latest_block.header(); + header.creation_time() }); - let interval = TimeInterval::new( - block.as_ref().header().creation_time(), - block.as_ref().header().consensus_estimation(), - ); + // NOTE: in case of genesis block only single point in time is matched + let interval = TimeInterval::new(since, to - since); - TimeEvent { - prev_interval, - interval, - } + TimeEvent { interval } } /// Process every trigger in `matched_ids` diff --git a/crates/iroha_data_model/benches/time_event_filter.rs b/crates/iroha_data_model/benches/time_event_filter.rs index 7d30f09a252..890f06cb145 100644 --- a/crates/iroha_data_model/benches/time_event_filter.rs +++ b/crates/iroha_data_model/benches/time_event_filter.rs @@ -15,10 +15,7 @@ fn schedule_from_zero_with_little_period(criterion: &mut Criterion) { let since = Duration::from_secs(TIMESTAMP); let length = Duration::from_secs(1); let interval = TimeInterval::new(since, length); - let event = TimeEvent { - prev_interval: None, - interval, - }; + let event = TimeEvent { interval }; let schedule = TimeSchedule::starting_at(Duration::ZERO).with_period(Duration::from_millis(1)); let filter = TimeEventFilter::new(ExecutionTime::Schedule(schedule)); diff --git a/crates/iroha_data_model/src/block.rs b/crates/iroha_data_model/src/block.rs index 1ffa33ff268..4306fba2188 100644 --- a/crates/iroha_data_model/src/block.rs +++ b/crates/iroha_data_model/src/block.rs @@ -65,8 +65,6 @@ mod model { /// Value of view change index. Used to resolve soft forks. #[getset(skip)] pub view_change_index: u32, - /// Estimation of consensus duration (in milliseconds). - pub consensus_estimation_ms: u64, } #[derive( @@ -135,11 +133,6 @@ impl BlockHeader { Duration::from_millis(self.creation_time_ms) } - /// Consensus estimation - pub const fn consensus_estimation(&self) -> Duration { - Duration::from_millis(self.consensus_estimation_ms) - } - /// Calculate block hash #[inline] pub fn hash(&self) -> HashOf { @@ -275,7 +268,6 @@ impl SignedBlock { transactions_hash, creation_time_ms, view_change_index: 0, - consensus_estimation_ms: 0, }; let transactions = genesis_transactions .into_iter() diff --git a/crates/iroha_data_model/src/events/pipeline.rs b/crates/iroha_data_model/src/events/pipeline.rs index 22897432501..8d593e3ce35 100644 --- a/crates/iroha_data_model/src/events/pipeline.rs +++ b/crates/iroha_data_model/src/events/pipeline.rs @@ -357,7 +357,6 @@ mod tests { )), creation_time_ms: 0, view_change_index: 0, - consensus_estimation_ms: 0, } } } diff --git a/crates/iroha_data_model/src/events/time.rs b/crates/iroha_data_model/src/events/time.rs index e01c101b464..c898b376ed4 100644 --- a/crates/iroha_data_model/src/events/time.rs +++ b/crates/iroha_data_model/src/events/time.rs @@ -33,10 +33,7 @@ mod model { #[getset(get = "pub")] #[ffi_type] pub struct TimeEvent { - /// Previous block timestamp and consensus durations estimation. - /// `None` if it's first block commit - pub prev_interval: Option, - /// Current block timestamp and consensus durations estimation + /// Time interval between creation of two blocks pub interval: TimeInterval, } @@ -142,49 +139,7 @@ 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 length = estimation - since; - - TimeInterval::new(since, length) - }); - - count_matches_in_interval(schedule, ¤t_interval) + count_matches_in_interval(schedule, &event.interval) } } } @@ -302,6 +257,12 @@ impl TimeInterval { } } + /// Create [`Self`] from since and to points + pub fn new_since_to(since: Duration, to: Duration) -> Self { + let length = to - since; + Self::new(since, length) + } + /// Instant of the previous execution pub fn since(&self) -> Duration { Duration::from_millis(self.since_ms) @@ -516,131 +477,58 @@ mod tests { use super::*; #[test] - fn test_schedule_start_before_prev_interval() { + fn test_schedule_start_before_interval() { // - // ----|---[--*--)--*--[--*--)---- - // s p1 p2 c1 c2 + // ----|---[--*----*--)--*------ + // s a b 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::new(since, length); - - let since = Duration::from_secs(TIMESTAMP + 25); - let length = Duration::from_secs(10); - let interval = TimeInterval::new(since, length); + let a = Duration::from_secs(TIMESTAMP + 5); + let b = Duration::from_secs(TIMESTAMP + 25); + let interval = TimeInterval::new_since_to(a, b); - let event = TimeEvent { - prev_interval: Some(prev_interval), - interval, - }; + let event = TimeEvent { interval }; assert_eq!(filter.count_matches(&event), 2); } #[test] - fn test_schedule_start_inside_prev_interval() { + fn test_schedule_start_inside_interval() { // - // -------[--|--)--*--[--*--)---- - // p1 s p2 c1 c2 + // -------[--|-----*--)--*------ + // a s b 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::new(since, length); - - let since = Duration::from_secs(TIMESTAMP + 20); - let length = Duration::from_secs(10); - let interval = TimeInterval::new(since, length); - - let event = TimeEvent { - prev_interval: Some(prev_interval), - interval, - }; + let a = Duration::from_secs(TIMESTAMP); + let b = Duration::from_secs(TIMESTAMP + 20); + let interval = TimeInterval::new_since_to(a, b); - 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::new(since, length); - - let since = Duration::from_secs(TIMESTAMP + 20); - let length = Duration::from_secs(10); - let interval = TimeInterval::new(since, length); - - let event = TimeEvent { - prev_interval: Some(prev_interval), - interval, - }; + let event = TimeEvent { interval }; assert_eq!(filter.count_matches(&event), 2); } #[test] - fn test_schedule_start_inside_current_interval() { + fn test_schedule_start_after_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::new(since, length); - - let since = Duration::from_secs(TIMESTAMP + 20); - let length = Duration::from_secs(10); - let interval = TimeInterval::new(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 + // -------[--------)----|-- + // a b 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::new(since, length); - - let since = Duration::from_secs(TIMESTAMP + 20); - let length = Duration::from_secs(10); - let interval = TimeInterval::new(since, length); + let a = Duration::from_secs(TIMESTAMP); + let b = Duration::from_secs(TIMESTAMP + 20); + let interval = TimeInterval::new_since_to(a, b); - let event = TimeEvent { - prev_interval: Some(prev_interval), - interval, - }; + let event = TimeEvent { interval }; assert_eq!(filter.count_matches(&event), 0); } diff --git a/crates/iroha_data_model/src/parameter.rs b/crates/iroha_data_model/src/parameter.rs index ec60939a2b2..42c37f9fa2b 100644 --- a/crates/iroha_data_model/src/parameter.rs +++ b/crates/iroha_data_model/src/parameter.rs @@ -350,12 +350,6 @@ impl SumeragiParameters { pub fn pipeline_time(&self) -> Duration { self.block_time() + self.commit_time() } - - /// Estimation of consensus duration - #[cfg(feature = "transparent_api")] - pub fn consensus_estimation(&self) -> Duration { - self.block_time() + (self.commit_time() / 2) - } } mod defaults { diff --git a/crates/iroha_schema_gen/src/lib.rs b/crates/iroha_schema_gen/src/lib.rs index a6d22807cb9..f310b40defb 100644 --- a/crates/iroha_schema_gen/src/lib.rs +++ b/crates/iroha_schema_gen/src/lib.rs @@ -323,7 +323,6 @@ types!( Option, Option, Option, - Option, Option>, Option, Option, diff --git a/docs/source/references/schema.json b/docs/source/references/schema.json index de724c14da0..c010be2906f 100644 --- a/docs/source/references/schema.json +++ b/docs/source/references/schema.json @@ -682,10 +682,6 @@ { "name": "view_change_index", "type": "u32" - }, - { - "name": "consensus_estimation_ms", - "type": "u64" } ] }, @@ -2709,9 +2705,6 @@ "Option": { "Option": "RoleId" }, - "Option": { - "Option": "TimeInterval" - }, "Option": { "Option": "TransactionRejectionReason" }, @@ -4278,10 +4271,6 @@ }, "TimeEvent": { "Struct": [ - { - "name": "prev_interval", - "type": "Option" - }, { "name": "interval", "type": "TimeInterval"