From 655dfeba85fc5e68cb1cde17ccf8561839a98a39 Mon Sep 17 00:00:00 2001 From: Daniyar Itegulov Date: Mon, 18 Nov 2024 16:23:30 +1100 Subject: [PATCH 1/3] refactor time management into its own component --- e2e-tests/test/evm-apis.test.ts | 2 +- src/node/config_api.rs | 10 +--- src/node/eth.rs | 15 +++-- src/node/in_memory.rs | 29 +++++---- src/node/in_memory_ext.rs | 103 ++++++++++++++------------------ src/node/mod.rs | 1 + src/node/time.rs | 58 ++++++++++++++++++ src/utils.rs | 4 +- 8 files changed, 133 insertions(+), 89 deletions(-) create mode 100644 src/node/time.rs diff --git a/e2e-tests/test/evm-apis.test.ts b/e2e-tests/test/evm-apis.test.ts index e0db9bb4..d1a9a181 100644 --- a/e2e-tests/test/evm-apis.test.ts +++ b/e2e-tests/test/evm-apis.test.ts @@ -44,7 +44,7 @@ describe("evm_increaseTime", function () { const wallet = new Wallet(RichAccounts[0].PrivateKey, provider); const userWallet = Wallet.createRandom().connect(provider); let expectedTimestamp: number = await provider.send("config_getCurrentTimestamp", []); - expectedTimestamp += timeIncreaseInSeconds * 1000; + expectedTimestamp += timeIncreaseInSeconds; // Act await provider.send("evm_increaseTime", [timeIncreaseInSeconds]); diff --git a/src/node/config_api.rs b/src/node/config_api.rs index 3051a998..f0313e6d 100644 --- a/src/node/config_api.rs +++ b/src/node/config_api.rs @@ -37,15 +37,7 @@ impl Configurat } fn config_get_current_timestamp(&self) -> Result { - self.get_inner() - .read() - .map_err(|err| { - tracing::error!("failed acquiring lock: {:?}", err); - into_jsrpc_error(Web3Error::InternalError(anyhow::Error::msg( - "Failed to acquire read lock for inner node state.", - ))) - }) - .map(|reader| reader.current_timestamp) + Ok(self.time.last_timestamp()) } fn config_set_show_calls(&self, value: String) -> Result { diff --git a/src/node/eth.rs b/src/node/eth.rs index af76bd44..180d2d83 100644 --- a/src/node/eth.rs +++ b/src/node/eth.rs @@ -2954,7 +2954,7 @@ mod tests { inner.current_batch = 1; inner.current_miniblock = 1; inner.current_miniblock_hash = H256::repeat_byte(0x1); - inner.current_timestamp = 1; + inner.time.set_last_timestamp(1); inner .filters .add_block_filter() @@ -2971,7 +2971,7 @@ mod tests { let storage = inner.fork_storage.inner.read().unwrap(); let expected_snapshot = Snapshot { - current_timestamp: inner.current_timestamp, + current_timestamp: inner.time.last_timestamp(), current_batch: inner.current_batch, current_miniblock: inner.current_miniblock, current_miniblock_hash: inner.current_miniblock_hash, @@ -3060,7 +3060,7 @@ mod tests { inner.current_batch = 1; inner.current_miniblock = 1; inner.current_miniblock_hash = H256::repeat_byte(0x1); - inner.current_timestamp = 1; + inner.time.set_last_timestamp(1); inner .filters .add_block_filter() @@ -3078,7 +3078,7 @@ mod tests { let expected_snapshot = { let storage = inner.fork_storage.inner.read().unwrap(); Snapshot { - current_timestamp: inner.current_timestamp, + current_timestamp: inner.time.last_timestamp(), current_batch: inner.current_batch, current_miniblock: inner.current_miniblock, current_miniblock_hash: inner.current_miniblock_hash, @@ -3113,7 +3113,7 @@ mod tests { inner.current_batch = 2; inner.current_miniblock = 2; inner.current_miniblock_hash = H256::repeat_byte(0x2); - inner.current_timestamp = 2; + inner.time.set_last_timestamp(2); inner .filters .add_pending_transaction_filter() @@ -3134,7 +3134,10 @@ mod tests { .expect("failed restoring snapshot"); let storage = inner.fork_storage.inner.read().unwrap(); - assert_eq!(expected_snapshot.current_timestamp, inner.current_timestamp); + assert_eq!( + expected_snapshot.current_timestamp, + inner.time.last_timestamp() + ); assert_eq!(expected_snapshot.current_batch, inner.current_batch); assert_eq!(expected_snapshot.current_miniblock, inner.current_miniblock); assert_eq!( diff --git a/src/node/in_memory.rs b/src/node/in_memory.rs index 479cb9ea..cb81d5b3 100644 --- a/src/node/in_memory.rs +++ b/src/node/in_memory.rs @@ -46,6 +46,7 @@ use zksync_types::{ use zksync_utils::{bytecode::hash_bytecode, h256_to_account_address, h256_to_u256, u256_to_h256}; use zksync_web3_decl::error::Web3Error; +use crate::node::time::TimestampManager; use crate::{ bootloader_debug::{BootloaderDebug, BootloaderDebugTracer}, config::{ @@ -149,9 +150,8 @@ impl TransactionResult { /// S - is the Source of the Fork. #[derive(Clone)] pub struct InMemoryNodeInner { - /// The latest timestamp that was already generated. - /// Next block will be current_timestamp + 1 - pub current_timestamp: u64, + /// Supplies timestamps that are unique across the system. + pub time: TimestampManager, /// The latest batch number that was already generated. /// Next block will be current_batch + 1 pub current_batch: u32, @@ -223,7 +223,7 @@ impl InMemoryNodeInner { }; InMemoryNodeInner { - current_timestamp: f.block_timestamp, + time: TimestampManager::new(f.block_timestamp), current_batch: f.l1_block.0, current_miniblock: f.l2_miniblock, current_miniblock_hash: f.l2_miniblock_hash, @@ -262,7 +262,7 @@ impl InMemoryNodeInner { let fee_input_provider = TestNodeFeeInputProvider::default().with_overrides(gas_overrides); InMemoryNodeInner { - current_timestamp: NON_FORK_FIRST_BLOCK_TIMESTAMP, + time: TimestampManager::new(NON_FORK_FIRST_BLOCK_TIMESTAMP), current_batch: 0, current_miniblock: 0, current_miniblock_hash: block_hash, @@ -304,15 +304,15 @@ impl InMemoryNodeInner { let (last_l1_block_num, last_l1_block_ts) = load_last_l1_batch(storage.clone()) .map(|(num, ts)| (num as u32, ts)) - .unwrap_or_else(|| (self.current_batch, self.current_timestamp)); + .unwrap_or_else(|| (self.current_batch, self.time.last_timestamp())); let last_l2_block = load_last_l2_block(&storage).unwrap_or_else(|| L2Block { number: self.current_miniblock as u32, hash: L2BlockHasher::legacy_hash(L2BlockNumber(self.current_miniblock as u32)), - timestamp: self.current_timestamp, + timestamp: self.time.last_timestamp(), }); let latest_timestamp = std::cmp::max( std::cmp::max(last_l1_block_ts, last_l2_block.timestamp), - self.current_timestamp, + self.time.last_timestamp(), ); let block_ctx = BlockContext::from_current( @@ -769,7 +769,7 @@ impl InMemoryNodeInner { .map_err(|err| format!("failed acquiring read lock on storage: {:?}", err))?; Ok(Snapshot { - current_timestamp: self.current_timestamp, + current_timestamp: self.time.last_timestamp(), current_batch: self.current_batch, current_miniblock: self.current_miniblock, current_miniblock_hash: self.current_miniblock_hash, @@ -795,7 +795,7 @@ impl InMemoryNodeInner { .write() .map_err(|err| format!("failed acquiring write lock on storage: {:?}", err))?; - self.current_timestamp = snapshot.current_timestamp; + self.time.set_last_timestamp(snapshot.current_timestamp); self.current_batch = snapshot.current_batch; self.current_miniblock = snapshot.current_miniblock; self.current_miniblock_hash = snapshot.current_miniblock_hash; @@ -850,6 +850,7 @@ pub struct InMemoryNode { /// Configuration option that survives reset. #[allow(dead_code)] pub(crate) system_contracts_options: system_contracts::Options, + pub(crate) time: TimestampManager, } fn contract_address_from_tx_result(execution_result: &VmExecutionResultAndLogs) -> Option { @@ -876,10 +877,12 @@ impl InMemoryNode { ) -> Self { let system_contracts_options = config.system_contracts_options; let inner = InMemoryNodeInner::new(fork, observability, config, gas_overrides); + let time = inner.time.clone(); InMemoryNode { inner: Arc::new(RwLock::new(inner)), snapshots: Default::default(), system_contracts_options, + time, } } @@ -1694,7 +1697,7 @@ impl InMemoryNode { } inner.current_miniblock = inner.current_miniblock.saturating_add(1); - inner.current_timestamp = inner.current_timestamp.saturating_add(1); + let expected_timestamp = inner.time.next_timestamp(); let actual_l1_batch_number = block .l1_batch_number @@ -1715,10 +1718,10 @@ impl InMemoryNode { ); } - if block.timestamp.as_u64() != inner.current_timestamp { + if block.timestamp.as_u64() != expected_timestamp { panic!( "expected next block to have timestamp {}, got {} | {i}", - inner.current_timestamp, + expected_timestamp, block.timestamp.as_u64() ); } diff --git a/src/node/in_memory_ext.rs b/src/node/in_memory_ext.rs index 0cbf207c..1f400a6e 100644 --- a/src/node/in_memory_ext.rs +++ b/src/node/in_memory_ext.rs @@ -29,18 +29,11 @@ impl InMemoryNo /// # Returns /// The applied time delta to `current_timestamp` value for the InMemoryNodeInner. pub fn increase_time(&self, time_delta_seconds: u64) -> Result { - self.get_inner() - .write() - .map_err(|err| anyhow!("failed acquiring lock: {:?}", err)) - .map(|mut writer| { - if time_delta_seconds == 0 { - return time_delta_seconds; - } - - let time_delta = time_delta_seconds.saturating_mul(1000); - writer.current_timestamp = writer.current_timestamp.saturating_add(time_delta); - time_delta_seconds - }) + if time_delta_seconds == 0 { + return Ok(time_delta_seconds); + } + self.time.increase_time(time_delta_seconds); + Ok(time_delta_seconds) } /// Set the current timestamp for the node. The timestamp must be in future. @@ -51,22 +44,18 @@ impl InMemoryNo /// # Returns /// The new timestamp value for the InMemoryNodeInner. pub fn set_next_block_timestamp(&self, timestamp: U64) -> Result { - self.get_inner() - .write() - .map_err(|err| anyhow!("failed acquiring lock: {:?}", err)) - .and_then(|mut writer| { - let ts = timestamp.as_u64(); - if ts <= writer.current_timestamp { - Err(anyhow!( - "timestamp ({}) must be greater than current timestamp ({})", - ts, - writer.current_timestamp - )) - } else { - writer.current_timestamp = ts - 1; - Ok(timestamp) - } - }) + let ts = timestamp.as_u64(); + let last_timestamp = self.time.last_timestamp(); + if ts <= last_timestamp { + Err(anyhow!( + "timestamp ({}) must be greater than current timestamp ({})", + ts, + last_timestamp + )) + } else { + self.time.set_last_timestamp(ts - 1); + Ok(timestamp) + } } /// Set the current timestamp for the node. @@ -79,14 +68,9 @@ impl InMemoryNo /// # Returns /// The difference between the `current_timestamp` and the new timestamp for the InMemoryNodeInner. pub fn set_time(&self, time: u64) -> Result { - self.get_inner() - .write() - .map_err(|err| anyhow!("failed acquiring lock: {:?}", err)) - .map(|mut writer| { - let time_diff = (time as i128).saturating_sub(writer.current_timestamp as i128); - writer.current_timestamp = time; - time_diff - }) + let time_diff = (time as i128).saturating_sub(self.time.last_timestamp() as i128); + self.time.set_last_timestamp(time); + Ok(time_diff) } /// Force a single block to be mined. @@ -402,6 +386,7 @@ mod tests { use super::*; use crate::fork::ForkStorage; use crate::namespaces::EthNamespaceT; + use crate::node::time::TimestampManager; use crate::node::{InMemoryNodeInner, Snapshot}; use crate::{http_fork_source::HttpForkSource, node::InMemoryNode}; use std::str::FromStr; @@ -514,7 +499,7 @@ mod tests { let old_snapshots = Arc::new(RwLock::new(vec![Snapshot::default()])); let old_system_contracts_options = Default::default(); let old_inner = InMemoryNodeInner:: { - current_timestamp: 123, + time: TimestampManager::new(123), current_batch: 100, current_miniblock: 300, current_miniblock_hash: H256::random(), @@ -532,11 +517,13 @@ mod tests { previous_states: Default::default(), observability: None, }; + let time = old_inner.time.clone(); let node = InMemoryNode:: { inner: Arc::new(RwLock::new(old_inner)), snapshots: old_snapshots, system_contracts_options: old_system_contracts_options, + time, }; let address = Address::from_str("0x36615Cf349d7F6344891B1e7CA7C72883F5dc049").unwrap(); @@ -554,7 +541,7 @@ mod tests { assert_eq!(node.snapshots.read().unwrap().len(), 0); let inner = node.inner.read().unwrap(); - assert_eq!(inner.current_timestamp, 1000); + assert_eq!(inner.time.last_timestamp(), 1000); assert_eq!(inner.current_batch, 0); assert_eq!(inner.current_miniblock, 0); assert_ne!(inner.current_miniblock_hash, H256::random()); @@ -691,7 +678,7 @@ mod tests { let timestamp_before = node .get_inner() .read() - .map(|inner| inner.current_timestamp) + .map(|inner| inner.time.last_timestamp()) .expect("failed reading timestamp"); let expected_response = increase_value_seconds; @@ -701,7 +688,7 @@ mod tests { let timestamp_after = node .get_inner() .read() - .map(|inner| inner.current_timestamp) + .map(|inner| inner.time.last_timestamp()) .expect("failed reading timestamp"); assert_eq!(expected_response, actual_response, "erroneous response"); @@ -720,7 +707,7 @@ mod tests { let timestamp_before = node .get_inner() .read() - .map(|inner| inner.current_timestamp) + .map(|inner| inner.time.last_timestamp()) .expect("failed reading timestamp"); assert_ne!(0, timestamp_before, "initial timestamp must be non zero",); let expected_response = increase_value_seconds; @@ -731,7 +718,7 @@ mod tests { let timestamp_after = node .get_inner() .read() - .map(|inner| inner.current_timestamp) + .map(|inner| inner.time.last_timestamp()) .expect("failed reading timestamp"); assert_eq!(expected_response, actual_response, "erroneous response"); @@ -750,7 +737,7 @@ mod tests { let timestamp_before = node .get_inner() .read() - .map(|inner| inner.current_timestamp) + .map(|inner| inner.time.last_timestamp()) .expect("failed reading timestamp"); let expected_response = increase_value_seconds; @@ -760,12 +747,12 @@ mod tests { let timestamp_after = node .get_inner() .read() - .map(|inner| inner.current_timestamp) + .map(|inner| inner.time.last_timestamp()) .expect("failed reading timestamp"); assert_eq!(expected_response, actual_response, "erroneous response"); assert_eq!( - increase_value_seconds.saturating_mul(1000u64), + increase_value_seconds, timestamp_after.saturating_sub(timestamp_before), "timestamp did not increase by the specified amount", ); @@ -779,7 +766,7 @@ mod tests { let timestamp_before = node .get_inner() .read() - .map(|inner| inner.current_timestamp) + .map(|inner| inner.time.last_timestamp()) .expect("failed reading timestamp"); assert_ne!( timestamp_before, new_timestamp, @@ -793,7 +780,7 @@ mod tests { let timestamp_after = node .get_inner() .read() - .map(|inner| inner.current_timestamp) + .map(|inner| inner.time.last_timestamp()) .expect("failed reading timestamp"); assert_eq!( @@ -815,7 +802,7 @@ mod tests { let timestamp_before = node .get_inner() .read() - .map(|inner| inner.current_timestamp) + .map(|inner| inner.time.last_timestamp()) .expect("failed reading timestamp"); let new_timestamp = timestamp_before + 500; @@ -835,7 +822,7 @@ mod tests { let timestamp_before = node .get_inner() .read() - .map(|inner| inner.current_timestamp) + .map(|inner| inner.time.last_timestamp()) .expect("failed reading timestamp"); assert_eq!(timestamp_before, new_timestamp, "timestamps must be same"); @@ -845,7 +832,7 @@ mod tests { let timestamp_after = node .get_inner() .read() - .map(|inner| inner.current_timestamp) + .map(|inner| inner.time.last_timestamp()) .expect("failed reading timestamp"); assert_eq!( timestamp_before, timestamp_after, @@ -861,7 +848,7 @@ mod tests { let timestamp_before = node .get_inner() .read() - .map(|inner| inner.current_timestamp) + .map(|inner| inner.time.last_timestamp()) .expect("failed reading timestamp"); assert_ne!(timestamp_before, new_time, "timestamps must be different"); let expected_response = 9000; @@ -870,7 +857,7 @@ mod tests { let timestamp_after = node .get_inner() .read() - .map(|inner| inner.current_timestamp) + .map(|inner| inner.time.last_timestamp()) .expect("failed reading timestamp"); assert_eq!(expected_response, actual_response, "erroneous response"); @@ -885,7 +872,7 @@ mod tests { let timestamp_before = node .get_inner() .read() - .map(|inner| inner.current_timestamp) + .map(|inner| inner.time.last_timestamp()) .expect("failed reading timestamp"); assert_ne!(timestamp_before, new_time, "timestamps must be different"); let expected_response = -990; @@ -894,7 +881,7 @@ mod tests { let timestamp_after = node .get_inner() .read() - .map(|inner| inner.current_timestamp) + .map(|inner| inner.time.last_timestamp()) .expect("failed reading timestamp"); assert_eq!(expected_response, actual_response, "erroneous response"); @@ -909,7 +896,7 @@ mod tests { let timestamp_before = node .get_inner() .read() - .map(|inner| inner.current_timestamp) + .map(|inner| inner.time.last_timestamp()) .expect("failed reading timestamp"); assert_eq!(timestamp_before, new_time, "timestamps must be same"); let expected_response = 0; @@ -918,7 +905,7 @@ mod tests { let timestamp_after = node .get_inner() .read() - .map(|inner| inner.current_timestamp) + .map(|inner| inner.time.last_timestamp()) .expect("failed reading timestamp"); assert_eq!(expected_response, actual_response, "erroneous response"); @@ -936,7 +923,7 @@ mod tests { let timestamp_before = node .get_inner() .read() - .map(|inner| inner.current_timestamp) + .map(|inner| inner.time.last_timestamp()) .unwrap_or_else(|_| panic!("case {}: failed reading timestamp", new_time)); assert_ne!( timestamp_before, new_time, @@ -948,7 +935,7 @@ mod tests { let timestamp_after = node .get_inner() .read() - .map(|inner| inner.current_timestamp) + .map(|inner| inner.time.last_timestamp()) .unwrap_or_else(|_| panic!("case {}: failed reading timestamp", new_time)); assert_eq!( diff --git a/src/node/mod.rs b/src/node/mod.rs index 9dccd2dd..bf8a00d7 100644 --- a/src/node/mod.rs +++ b/src/node/mod.rs @@ -12,6 +12,7 @@ mod in_memory; mod in_memory_ext; mod net; mod storage_logs; +mod time; mod web3; mod zks; diff --git a/src/node/time.rs b/src/node/time.rs new file mode 100644 index 00000000..52b0053f --- /dev/null +++ b/src/node/time.rs @@ -0,0 +1,58 @@ +use std::sync::{Arc, RwLock}; + +/// Manages timestamps across the system. +/// +/// Clones always agree on the underlying timestamp and updating one affects all other instances. +#[derive(Clone, Debug)] +pub struct TimestampManager { + /// The latest timestamp that has already been used. + last_timestamp: Arc>, +} + +impl TimestampManager { + pub fn new(last_timestamp: u64) -> TimestampManager { + TimestampManager { + last_timestamp: Arc::new(RwLock::new(last_timestamp)), + } + } + + /// Returns the last timestamp that has already been used. + pub fn last_timestamp(&self) -> u64 { + *self + .last_timestamp + .read() + .expect("TimestampManager lock is poisoned") + } + + /// Returns the next unique timestamp to be used. + pub fn next_timestamp(&self) -> u64 { + let mut guard = self + .last_timestamp + .write() + .expect("TimestampManager lock is poisoned"); + let next_timestamp = *guard + 1; + *guard = next_timestamp; + + next_timestamp + } + + /// Sets last used timestamp to the provided value. + pub fn set_last_timestamp(&self, timestamp: u64) { + let mut guard = self + .last_timestamp + .write() + .expect("TimestampManager lock is poisoned"); + *guard = timestamp; + } + + /// Fast-forwards time by the given amount of seconds. + pub fn increase_time(&self, seconds: u64) -> u64 { + let mut guard = self + .last_timestamp + .write() + .expect("TimestampManager lock is poisoned"); + let next = guard.saturating_add(seconds); + *guard = next; + next + } +} diff --git a/src/utils.rs b/src/utils.rs index 0ad074ea..e28c7cf3 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -89,7 +89,7 @@ pub fn mine_empty_blocks( let (batch_env, mut block_ctx) = node.create_l1_batch_env(storage.clone()); // override the next block's timestamp to match up with interval for subsequent blocks if i != 0 { - block_ctx.timestamp = node.current_timestamp.saturating_add(interval_sec); + block_ctx.timestamp = node.time.increase_time(interval_sec); } // init vm @@ -124,7 +124,7 @@ pub fn mine_empty_blocks( // leave node state ready for next interaction node.current_batch = block_ctx.batch; node.current_miniblock = block_ctx.miniblock; - node.current_timestamp = block_ctx.timestamp; + node.time.set_last_timestamp(block_ctx.timestamp); } Ok(()) From ec179611cd969189946eff6b207266b4724961ae Mon Sep 17 00:00:00 2001 From: Daniyar Itegulov Date: Tue, 19 Nov 2024 01:07:05 +1100 Subject: [PATCH 2/3] clarify units of time --- src/node/time.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/node/time.rs b/src/node/time.rs index 52b0053f..ecacf0a9 100644 --- a/src/node/time.rs +++ b/src/node/time.rs @@ -1,11 +1,11 @@ use std::sync::{Arc, RwLock}; -/// Manages timestamps across the system. +/// Manages timestamps (in seconds) across the system. /// /// Clones always agree on the underlying timestamp and updating one affects all other instances. #[derive(Clone, Debug)] pub struct TimestampManager { - /// The latest timestamp that has already been used. + /// The latest timestamp (in seconds) that has already been used. last_timestamp: Arc>, } @@ -16,7 +16,7 @@ impl TimestampManager { } } - /// Returns the last timestamp that has already been used. + /// Returns the last timestamp (in seconds) that has already been used. pub fn last_timestamp(&self) -> u64 { *self .last_timestamp @@ -24,7 +24,7 @@ impl TimestampManager { .expect("TimestampManager lock is poisoned") } - /// Returns the next unique timestamp to be used. + /// Returns the next unique timestamp (in seconds) to be used. pub fn next_timestamp(&self) -> u64 { let mut guard = self .last_timestamp @@ -36,7 +36,7 @@ impl TimestampManager { next_timestamp } - /// Sets last used timestamp to the provided value. + /// Sets last used timestamp (in seconds) to the provided value. pub fn set_last_timestamp(&self, timestamp: u64) { let mut guard = self .last_timestamp From ccaecdee71a4b1f4cecdba950bc0197b4052d8eb Mon Sep 17 00:00:00 2001 From: Daniyar Itegulov Date: Tue, 19 Nov 2024 01:23:20 +1100 Subject: [PATCH 3/3] fix TOCTOUs --- src/node/eth.rs | 6 +++--- src/node/in_memory.rs | 3 ++- src/node/in_memory_ext.rs | 21 +++------------------ src/node/time.rs | 28 ++++++++++++++++++++++++++-- src/utils.rs | 2 +- 5 files changed, 35 insertions(+), 25 deletions(-) diff --git a/src/node/eth.rs b/src/node/eth.rs index 180d2d83..aee4c714 100644 --- a/src/node/eth.rs +++ b/src/node/eth.rs @@ -2954,7 +2954,7 @@ mod tests { inner.current_batch = 1; inner.current_miniblock = 1; inner.current_miniblock_hash = H256::repeat_byte(0x1); - inner.time.set_last_timestamp(1); + inner.time.set_last_timestamp_unchecked(1); inner .filters .add_block_filter() @@ -3060,7 +3060,7 @@ mod tests { inner.current_batch = 1; inner.current_miniblock = 1; inner.current_miniblock_hash = H256::repeat_byte(0x1); - inner.time.set_last_timestamp(1); + inner.time.set_last_timestamp_unchecked(1); inner .filters .add_block_filter() @@ -3113,7 +3113,7 @@ mod tests { inner.current_batch = 2; inner.current_miniblock = 2; inner.current_miniblock_hash = H256::repeat_byte(0x2); - inner.time.set_last_timestamp(2); + inner.time.set_last_timestamp_unchecked(2); inner .filters .add_pending_transaction_filter() diff --git a/src/node/in_memory.rs b/src/node/in_memory.rs index cb81d5b3..21b4d05b 100644 --- a/src/node/in_memory.rs +++ b/src/node/in_memory.rs @@ -795,7 +795,8 @@ impl InMemoryNodeInner { .write() .map_err(|err| format!("failed acquiring write lock on storage: {:?}", err))?; - self.time.set_last_timestamp(snapshot.current_timestamp); + self.time + .set_last_timestamp_unchecked(snapshot.current_timestamp); self.current_batch = snapshot.current_batch; self.current_miniblock = snapshot.current_miniblock; self.current_miniblock_hash = snapshot.current_miniblock_hash; diff --git a/src/node/in_memory_ext.rs b/src/node/in_memory_ext.rs index 1f400a6e..62a4fc1c 100644 --- a/src/node/in_memory_ext.rs +++ b/src/node/in_memory_ext.rs @@ -29,9 +29,6 @@ impl InMemoryNo /// # Returns /// The applied time delta to `current_timestamp` value for the InMemoryNodeInner. pub fn increase_time(&self, time_delta_seconds: u64) -> Result { - if time_delta_seconds == 0 { - return Ok(time_delta_seconds); - } self.time.increase_time(time_delta_seconds); Ok(time_delta_seconds) } @@ -44,18 +41,8 @@ impl InMemoryNo /// # Returns /// The new timestamp value for the InMemoryNodeInner. pub fn set_next_block_timestamp(&self, timestamp: U64) -> Result { - let ts = timestamp.as_u64(); - let last_timestamp = self.time.last_timestamp(); - if ts <= last_timestamp { - Err(anyhow!( - "timestamp ({}) must be greater than current timestamp ({})", - ts, - last_timestamp - )) - } else { - self.time.set_last_timestamp(ts - 1); - Ok(timestamp) - } + self.time.advance_timestamp(timestamp.as_u64() - 1)?; + Ok(timestamp) } /// Set the current timestamp for the node. @@ -68,9 +55,7 @@ impl InMemoryNo /// # Returns /// The difference between the `current_timestamp` and the new timestamp for the InMemoryNodeInner. pub fn set_time(&self, time: u64) -> Result { - let time_diff = (time as i128).saturating_sub(self.time.last_timestamp() as i128); - self.time.set_last_timestamp(time); - Ok(time_diff) + Ok(self.time.set_last_timestamp_unchecked(time)) } /// Force a single block to be mined. diff --git a/src/node/time.rs b/src/node/time.rs index ecacf0a9..57a1817b 100644 --- a/src/node/time.rs +++ b/src/node/time.rs @@ -1,3 +1,4 @@ +use anyhow::anyhow; use std::sync::{Arc, RwLock}; /// Manages timestamps (in seconds) across the system. @@ -36,13 +37,36 @@ impl TimestampManager { next_timestamp } - /// Sets last used timestamp (in seconds) to the provided value. - pub fn set_last_timestamp(&self, timestamp: u64) { + /// Sets last used timestamp (in seconds) to the provided value and returns the difference + /// between new value and old value (represented as a signed number of seconds). + pub fn set_last_timestamp_unchecked(&self, timestamp: u64) -> i128 { let mut guard = self .last_timestamp .write() .expect("TimestampManager lock is poisoned"); + let diff = (timestamp as i128).saturating_sub(*guard as i128); *guard = timestamp; + diff + } + + /// Advances internal timestamp (in seconds) to the provided value. + /// + /// Expects provided timestamp to be in the future, returns error otherwise. + pub fn advance_timestamp(&self, timestamp: u64) -> anyhow::Result<()> { + let mut guard = self + .last_timestamp + .write() + .expect("TimestampManager lock is poisoned"); + if timestamp < *guard { + Err(anyhow!( + "timestamp ({}) must be greater or equal than current timestamp ({})", + timestamp, + *guard + )) + } else { + *guard = timestamp; + Ok(()) + } } /// Fast-forwards time by the given amount of seconds. diff --git a/src/utils.rs b/src/utils.rs index e28c7cf3..e27ceda8 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -124,7 +124,7 @@ pub fn mine_empty_blocks( // leave node state ready for next interaction node.current_batch = block_ctx.batch; node.current_miniblock = block_ctx.miniblock; - node.time.set_last_timestamp(block_ctx.timestamp); + node.time.set_last_timestamp_unchecked(block_ctx.timestamp); } Ok(())