pallet_revive_dev_node: Always increment the timestamp by at least one second

prdoc/pr_10160.prdoc

@@ -0,0 +1,12 @@
+title: 'pallet_revive_dev_node: Always increment the timestamp by at least one second'
+doc:
+- audience: Runtime User
+  description: |-
+    Fixes https://github.com/paritytech/contract-issues/issues/191
+
+    The instant seal introduces a race condition. Blocks can be build faster than the timestamp resolution of Ethereum. Eth timestamps are only one second granularity. If we build blocks faster it can happen that the timestamp delta between them is zero. This is not allowed. We have to make sure that in instant seal two blocks don't return the same timestamp.
+
+    This PR does that by always incrementing the timestamp by at least one second. Note that this is a dev-node only change. Production chains won't have this problem as long as the block time is larger than 1 second.
+
+    Yes, it will produce timestamps in the future. But this seems to be the lesser evil for this dev node. Time is subjective. But the rule to not return duplicate timestamps is dependent on.
+crates: []

substrate/frame/revive/dev-node/node/src/service.rs

@@ -18,15 +18,15 @@
 use crate::cli::Consensus;
 use futures::FutureExt;
 use polkadot_sdk::{
-	sc_client_api::backend::Backend,
+	sc_client_api::{backend::Backend, StorageProvider},
 	sc_executor::WasmExecutor,
 	sc_service::{error::Error as ServiceError, Configuration, TaskManager},
 	sc_telemetry::{Telemetry, TelemetryWorker},
 	sc_transaction_pool_api::OffchainTransactionPoolFactory,
 	sp_runtime::traits::Block as BlockT,
 	*,
 };
-use revive_dev_runtime::{OpaqueBlock as Block, RuntimeApi};
+use revive_dev_runtime::{OpaqueBlock as Block, Runtime, RuntimeApi};
 use std::sync::Arc;
 
 type HostFunctions = sp_io::SubstrateHostFunctions;
@@ -198,6 +198,40 @@ pub fn new_full<Network: sc_network::NetworkBackend<Block, <Block as BlockT>::Ha
 		telemetry.as_ref().map(|x| x.handle()),
 	);
 
+	// Due to instant seal or low block time multiple blocks can have the same timestamp.
+	// This is because Etereum only uses second granularity (as opposed to ms).
+	// Here we make sure that we increment by at least a second from the last block.
+	//
+	// # Warning
+	//
+	// This will lead to blocks with timestamps in the future. This might cause other issues
+	// when dealing with off chain data. But for a development node it is more important to not
+	// have duplicate timestamps. The only way to not have timestamps in the future and no
+	// duplicates is to set the block time to at least one second (`--consensus manual-seal-1000`).
+	let timestamp_provider = {
+		let client = client.clone();
+		move |parent, ()| {
+			let client = client.clone();
+			async move {
+				let key = sp_core::storage::StorageKey(
+					polkadot_sdk::pallet_timestamp::Now::<Runtime>::hashed_key().to_vec(),
+				);
+				let current = sp_timestamp::Timestamp::current();
+				let next = client
+					.storage(parent, &key)
+					.ok()
+					.flatten()
+					.and_then(|data| data.0.try_into().ok())
+					.map(|data| {
+						let last = u64::from_le_bytes(data) / 1000;
+						sp_timestamp::Timestamp::new((last + 1) * 1000)
+					})
+					.unwrap_or(current);
+				Ok(sp_timestamp::InherentDataProvider::new(current.max(next)))
+			}
+		}
+	};
+
 	match consensus {
 		Consensus::InstantSeal => {
 			let params = sc_consensus_manual_seal::InstantSealParams {
@@ -207,9 +241,7 @@ pub fn new_full<Network: sc_network::NetworkBackend<Block, <Block as BlockT>::Ha
 				pool: transaction_pool,
 				select_chain,
 				consensus_data_provider: None,
-				create_inherent_data_providers: move |_, ()| async move {
-					Ok(sp_timestamp::InherentDataProvider::from_system_time())
-				},
+				create_inherent_data_providers: timestamp_provider,
 			};
 
 			let authorship_future = sc_consensus_manual_seal::run_instant_seal(params);
@@ -243,9 +275,7 @@ pub fn new_full<Network: sc_network::NetworkBackend<Block, <Block as BlockT>::Ha
 				select_chain,
 				commands_stream: Box::pin(commands_stream),
 				consensus_data_provider: None,
-				create_inherent_data_providers: move |_, ()| async move {
-					Ok(sp_timestamp::InherentDataProvider::from_system_time())
-				},
+				create_inherent_data_providers: timestamp_provider,
 			};
 			let authorship_future = sc_consensus_manual_seal::run_manual_seal(params);