From c1032aeb09571769988d1e7a02182121c640e4fe Mon Sep 17 00:00:00 2001
From: Bernhard Schuster <bernhard@parity.io>
Date: Mon, 11 May 2020 18:25:27 +0200
Subject: [PATCH 01/44] feat/offchain/storage: add remove interface method

---
 client/db/src/offchain.rs               |  8 ++++++++
 client/offchain/src/api.rs              |  7 +++++++
 primitives/core/src/offchain/mod.rs     | 18 ++++++++++++++++++
 primitives/core/src/offchain/storage.rs |  5 +++++
 primitives/core/src/offchain/testing.rs |  8 ++++++++
 primitives/io/src/lib.rs                | 10 ++++++++++
 6 files changed, 56 insertions(+)

diff --git a/client/db/src/offchain.rs b/client/db/src/offchain.rs
index 8c58d5f42c391..6e5122dd36a38 100644
--- a/client/db/src/offchain.rs
+++ b/client/db/src/offchain.rs
@@ -65,6 +65,14 @@ impl sp_core::offchain::OffchainStorage for LocalStorage {
 		self.db.commit(tx);
 	}
 
+	fn remove(&mut self, prefix: &[u8], key: &[u8]) {
+		let key: Vec<u8> = prefix.iter().chain(key).cloned().collect();
+		let mut tx = Transaction::new();
+		tx.remove(columns::OFFCHAIN, &key);
+
+		self.db.commit(tx);
+	}
+
 	fn get(&self, prefix: &[u8], key: &[u8]) -> Option<Vec<u8>> {
 		let key: Vec<u8> = prefix.iter().chain(key).cloned().collect();
 		self.db.get(columns::OFFCHAIN, &key)
diff --git a/client/offchain/src/api.rs b/client/offchain/src/api.rs
index 45a82d230c11a..f7d91c7aba95d 100644
--- a/client/offchain/src/api.rs
+++ b/client/offchain/src/api.rs
@@ -100,6 +100,13 @@ impl<Storage: OffchainStorage> OffchainExt for Api<Storage> {
 		}
 	}
 
+	fn local_storage_remove(&mut self, kind: StorageKind, key: &[u8]) {
+		match kind {
+			StorageKind::PERSISTENT => self.db.remove(STORAGE_PREFIX, key),
+			StorageKind::LOCAL => unavailable_yet(LOCAL_DB),
+		}
+	}
+
 	fn local_storage_compare_and_set(
 		&mut self,
 		kind: StorageKind,
diff --git a/primitives/core/src/offchain/mod.rs b/primitives/core/src/offchain/mod.rs
index e792d71afca3b..01bf9998f0dad 100644
--- a/primitives/core/src/offchain/mod.rs
+++ b/primitives/core/src/offchain/mod.rs
@@ -36,6 +36,9 @@ pub trait OffchainStorage: Clone + Send + Sync {
 	/// Persist a value in storage under given key and prefix.
 	fn set(&mut self, prefix: &[u8], key: &[u8], value: &[u8]);
 
+	/// Remove a perviously pin storage ersisted value under given key and prefix.
+	fn remove(&mut self, prefix: &[u8], key: &[u8]);
+
 	/// Retrieve a value from storage under given key and prefix.
 	fn get(&self, prefix: &[u8], key: &[u8]) -> Option<Vec<u8>>;
 
@@ -348,6 +351,12 @@ pub trait Externalities: Send {
 	/// offchain worker tasks running on the same machine. It IS persisted between runs.
 	fn local_storage_set(&mut self, kind: StorageKind, key: &[u8], value: &[u8]);
 
+	/// Removes a value in the local storage.
+	///
+	/// Note this storage is not part of the consensus, it's only accessible by
+	/// offchain worker tasks running on the same machine. It IS persisted between runs.
+	fn local_storage_remove(&mut self, kind: StorageKind, key: &[u8]);
+
 	/// Sets a value in the local storage if it matches current value.
 	///
 	/// Since multiple offchain workers may be running concurrently, to prevent
@@ -512,6 +521,10 @@ impl<T: Externalities + ?Sized> Externalities for Box<T> {
 		(&mut **self).local_storage_set(kind, key, value)
 	}
 
+	fn local_storage_remove(&mut self, kind: StorageKind, key: &[u8]) {
+		(&mut **self).local_storage_remove(kind, key)
+	}
+
 	fn local_storage_compare_and_set(
 		&mut self,
 		kind: StorageKind,
@@ -617,6 +630,11 @@ impl<T: Externalities> Externalities for LimitedExternalities<T> {
 		self.externalities.local_storage_set(kind, key, value)
 	}
 
+	fn local_storage_remove(&mut self, kind: StorageKind, key: &[u8]) {
+		self.check(Capability::OffchainWorkerDbWrite, "local_storage_remove");
+		self.externalities.local_storage_remove(kind, key)
+	}
+
 	fn local_storage_compare_and_set(
 		&mut self,
 		kind: StorageKind,
diff --git a/primitives/core/src/offchain/storage.rs b/primitives/core/src/offchain/storage.rs
index 830c25392b742..627e931ac8271 100644
--- a/primitives/core/src/offchain/storage.rs
+++ b/primitives/core/src/offchain/storage.rs
@@ -50,6 +50,11 @@ impl OffchainStorage for InMemOffchainStorage {
 		self.storage.insert(key, value.to_vec());
 	}
 
+	fn remove(&mut self, prefix: &[u8], key: &[u8]) {
+		let key: Vec<u8> = prefix.iter().chain(key).cloned().collect();
+		self.storage.remove(&key);
+	}
+
 	fn get(&self, prefix: &[u8], key: &[u8]) -> Option<Vec<u8>> {
 		let key: Vec<u8> = prefix.iter().chain(key).cloned().collect();
 		self.storage.get(&key).cloned()
diff --git a/primitives/core/src/offchain/testing.rs b/primitives/core/src/offchain/testing.rs
index b889374a47c2f..36d9f15a55348 100644
--- a/primitives/core/src/offchain/testing.rs
+++ b/primitives/core/src/offchain/testing.rs
@@ -178,6 +178,14 @@ impl offchain::Externalities for TestOffchainExt {
 		}.set(b"", key, value);
 	}
 
+	fn local_storage_remove(&mut self, kind: StorageKind, key: &[u8]) {
+		let mut state = self.0.write();
+		match kind {
+			StorageKind::LOCAL => &mut state.local_storage,
+			StorageKind::PERSISTENT => &mut state.persistent_storage,
+		}.remove(b"", key);
+	}
+
 	fn local_storage_compare_and_set(
 		&mut self,
 		kind: StorageKind,
diff --git a/primitives/io/src/lib.rs b/primitives/io/src/lib.rs
index f5d692469f0a8..b6ab757c57803 100644
--- a/primitives/io/src/lib.rs
+++ b/primitives/io/src/lib.rs
@@ -711,6 +711,16 @@ pub trait Offchain {
 			.local_storage_set(kind, key, value)
 	}
 
+	/// Remove a value from the local storage.
+	///
+	/// Note this storage is not part of the consensus, it's only accessible by
+	/// offchain worker tasks running on the same machine. It IS persisted between runs.
+	fn local_storage_remove(&mut self, kind: StorageKind, key: &[u8]) {
+		self.extension::<OffchainExt>()
+			.expect("local_storage_remove can be called only in the offchain worker context")
+			.local_storage_remove(kind, key)
+	}
+
 	/// Sets a value in the local storage if it matches current value.
 	///
 	/// Since multiple offchain workers may be running concurrently, to prevent

From 3fa59ee0bf5c36671d8d463def83e02b3b5ae7f5 Mon Sep 17 00:00:00 2001
From: Bernhard Schuster <bernhard@parity.io>
Date: Wed, 13 May 2020 12:51:59 +0200
Subject: [PATCH 02/44] feat/offchain/storeage: add remote to StorageValueRef

---
 primitives/runtime/src/offchain/storage.rs | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/primitives/runtime/src/offchain/storage.rs b/primitives/runtime/src/offchain/storage.rs
index 681bc14451e11..9801e63c191cf 100644
--- a/primitives/runtime/src/offchain/storage.rs
+++ b/primitives/runtime/src/offchain/storage.rs
@@ -49,6 +49,11 @@ impl<'a> StorageValueRef<'a> {
 		})
 	}
 
+	/// Remove the associated value from the storage.
+	pub fn remove(&self) {
+		sp_io::offchain::local_storage_remove(self.kind, self.key)
+	}
+
 	/// Retrieve & decode the value from storage.
 	///
 	/// Note that if you want to do some checks based on the value

From 4557bd7e8b150265f28216e0b820763fb36e5c11 Mon Sep 17 00:00:00 2001
From: Bernhard Schuster <bernhard@parity.io>
Date: Wed, 13 May 2020 12:51:53 +0200
Subject: [PATCH 03/44] feat/offchain/storage: add storage lock

---
 primitives/runtime/src/offchain/mod.rs        |   1 +
 .../runtime/src/offchain/storage_lock.rs      | 184 ++++++++++++++++++
 2 files changed, 185 insertions(+)
 create mode 100644 primitives/runtime/src/offchain/storage_lock.rs

diff --git a/primitives/runtime/src/offchain/mod.rs b/primitives/runtime/src/offchain/mod.rs
index 9f0f949eaeb8d..cee0e6f1a58bb 100644
--- a/primitives/runtime/src/offchain/mod.rs
+++ b/primitives/runtime/src/offchain/mod.rs
@@ -18,5 +18,6 @@
 
 pub mod http;
 pub mod storage;
+pub mod storage_lock;
 
 pub use sp_core::offchain::*;
diff --git a/primitives/runtime/src/offchain/storage_lock.rs b/primitives/runtime/src/offchain/storage_lock.rs
new file mode 100644
index 0000000000000..8412af0206e8b
--- /dev/null
+++ b/primitives/runtime/src/offchain/storage_lock.rs
@@ -0,0 +1,184 @@
+// Copyright 2019-2020 Parity Technologies (UK) Ltd.
+// This file is part of Substrate.
+
+// Substrate is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+
+// Substrate is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License
+// along with Substrate.  If not, see <http://www.gnu.org/licenses/>.
+
+//! # Offchain Storage Lock
+//!
+//! A persistent storage lock with a defined expire time.
+//!
+//! One use case would be collective updates of multiple data items
+//! or append / remove of i.e. sets, vectors which are stored in
+//! the offchain storage DB.
+//!
+//! ## Example:
+//!
+//! ```rust
+//! // in your off-chain worker code
+//!
+//! fn append_to_in_storage_vec<'k, T>(key: &'k [u8], T) where T: Encode {
+//!    let mut lock = StorageLock::new(b"x::lock");
+//!    if let Ok(_guard) =  lock.spin_lock() {
+//!         let acc = StorageValueRef::persistent(key);
+//!         let v: Vec<T> = acc.get::<Vec<T>>().unwrap().unwrap();
+//!         // modify `v` as desired
+//!         acc.set(v);
+//!    } else {
+//!         // the lock duration expired
+//!    }
+//! }
+//! ```
+
+use crate::offchain::storage::StorageValueRef;
+use sp_core::offchain::{Duration, Timestamp};
+use sp_io::offchain;
+
+/// Default expirey duration in milliseconds.
+const STORAGE_LOCK_DEFAULT_EXPIREY_DURATION: u64 = 30_000;
+
+/// Snooze duration before attempting to lock again in ms.
+const STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE: u64 = 100;
+
+/// A persisted guard state.
+///
+/// An in DB persistent mutex for multi access items which are modified
+/// i.e. vecs or sets.
+pub struct StorageLock<'a> {
+	// A storage value ref which defines the DB entry representing the lock.
+	value_ref: StorageValueRef<'a>,
+	// `None` implies it was already released by `fn unlock(..)`
+	locked_until: Option<Timestamp>,
+}
+
+impl<'a> StorageLock<'a> {
+	/// Create a new storage lock with [default expirey duration](Self::STORAGE_LOCK_DEFAULT_EXPIREY_DURATION).
+	pub fn new<'k>(key: &'k [u8]) -> Self
+	where
+		'k: 'a,
+	{
+		Self {
+			value_ref: StorageValueRef::<'a>::persistent(key),
+			locked_until: None,
+		}
+	}
+
+	#[inline]
+	fn try_lock_inner(&mut self, duration: Duration) -> Result<(), Option<Timestamp>> {
+		let now = offchain::timestamp();
+		let expires_at = now.add(duration);
+		let res = self.value_ref.mutate(
+			|s: Option<Option<Timestamp>>| -> Result<Timestamp, Option<Timestamp>> {
+				match s {
+					// no lock set, we can safely acquire it
+					None => Ok(expires_at),
+					// lock is set, but it's old. We can re-acquire it.
+					Some(Some(current_good_until)) if current_good_until < now => Ok(expires_at),
+					// lock is present and is still active
+					Some(Some(current_good_until)) => Err(Some(current_good_until)),
+					_ => Err(None),
+				}
+			},
+		);
+		match res {
+			Ok(Ok(_)) => {
+				self.locked_until = Some(expires_at);
+				Ok(())
+			}
+			Ok(Err(timestamp)) => Err(Some(timestamp)), // failed to set the new value, but could read the current
+			Err(e) => Err(e),                           // forward the remaining value
+		}
+	}
+
+	/// Attempt to lock the storage entry.
+	///
+	/// Returns a lock guard on success, otherwise an error containing `None` in
+	/// case the mutex was already unlocked before, or if the lock is still held
+	/// by another process `Err(Some(expiration_timestamp))`.
+	pub fn try_lock<'b>(&'b mut self) -> Result<StorageLockGuard<'a, 'b>, Option<Timestamp>>
+	where
+		'a: 'b,
+	{
+		if self.locked_until.is_none() {
+			match self.try_lock_inner(Duration::from_millis(STORAGE_LOCK_DEFAULT_EXPIREY_DURATION))
+			{
+				Ok(_) => Ok(StorageLockGuard::<'a, 'b> { lock: Some(self) }),
+				Err(e) => Err(e),
+			}
+		} else {
+			Err(self.locked_until)
+		}
+	}
+
+	/// Try grabbing the lock until its expiry is reached.
+	///
+	/// Returns an error if the lock expired before it could be cought
+	pub fn spin_lock<'b, 'c>(&'b mut self) -> Result<StorageLockGuard<'a, 'c>, ()>
+	where
+		'a: 'b,
+		'b: 'c,
+	{
+		if self.locked_until.is_none() {
+			loop {
+				// blind attempt on locking
+				let expires_at = match self
+					.try_lock_inner(Duration::from_millis(STORAGE_LOCK_DEFAULT_EXPIREY_DURATION))
+				{
+					Ok(_) => return Ok(StorageLockGuard::<'a, 'b> { lock: Some(self) }),
+					Err(None) => return Err(()),
+					Err(Some(expires_at)) => expires_at,
+				};
+				let remainder: Duration = offchain::timestamp().diff(&expires_at);
+				// do not snooze the full duration, but instead snooze max 100ms
+				// it might get unlocked in another thread
+				// consider adding some additive jitter here
+				let snooze =
+					core::cmp::min(remainder.millis(), STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE);
+				sp_io::offchain::sleep_until(
+					offchain::timestamp().add(Duration::from_millis(snooze)),
+				);
+			}
+		} else {
+			Err(())
+		}
+	}
+
+	/// Explicitly unlock the lock.
+	///
+	/// Does nothing if the lock was alrady unlocked before.
+	pub fn unlock(&mut self) {
+		if let Some(_) = self.locked_until.take() {
+			self.value_ref.remove();
+		}
+	}
+}
+
+/// RAII style guard for a lock.
+pub struct StorageLockGuard<'a, 'b> {
+	lock: Option<&'b mut StorageLock<'a>>,
+}
+
+impl<'a, 'b> StorageLockGuard<'a, 'b> {
+	/// Consume the guard and unlock the underlying lock.
+	pub fn forget(mut self) {
+		let _ = self.lock.take();
+	}
+}
+
+impl<'a, 'b> Drop for StorageLockGuard<'a, 'b> {
+	fn drop(&mut self) {
+		if let Some(lock) = self.lock.take() {
+			lock.unlock();
+		}
+	}
+}

From be1988feafc4341bc5d7d06ae77d80d11206c379 Mon Sep 17 00:00:00 2001
From: Bernhard Schuster <bernhard@ahoi.io>
Date: Wed, 13 May 2020 20:46:56 +0200
Subject: [PATCH 04/44] fix/review: Apply suggestions from code review
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com>
Co-authored-by: Peter Goodspeed-Niklaus <coriolinus@users.noreply.github.com>
---
 client/offchain/src/api.rs                    |  2 +-
 primitives/core/src/offchain/mod.rs           |  2 +-
 primitives/runtime/src/offchain/storage.rs    |  2 +-
 .../runtime/src/offchain/storage_lock.rs      | 24 +++++++++++++------
 4 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/client/offchain/src/api.rs b/client/offchain/src/api.rs
index f7d91c7aba95d..a7f4ecbc5825e 100644
--- a/client/offchain/src/api.rs
+++ b/client/offchain/src/api.rs
@@ -100,7 +100,7 @@ impl<Storage: OffchainStorage> OffchainExt for Api<Storage> {
 		}
 	}
 
-	fn local_storage_remove(&mut self, kind: StorageKind, key: &[u8]) {
+	fn local_storage_clear(&mut self, kind: StorageKind, key: &[u8]) {
 		match kind {
 			StorageKind::PERSISTENT => self.db.remove(STORAGE_PREFIX, key),
 			StorageKind::LOCAL => unavailable_yet(LOCAL_DB),
diff --git a/primitives/core/src/offchain/mod.rs b/primitives/core/src/offchain/mod.rs
index 01bf9998f0dad..14688881b09bc 100644
--- a/primitives/core/src/offchain/mod.rs
+++ b/primitives/core/src/offchain/mod.rs
@@ -36,7 +36,7 @@ pub trait OffchainStorage: Clone + Send + Sync {
 	/// Persist a value in storage under given key and prefix.
 	fn set(&mut self, prefix: &[u8], key: &[u8], value: &[u8]);
 
-	/// Remove a perviously pin storage ersisted value under given key and prefix.
+	/// Clear a storage entry under given key and prefix.
 	fn remove(&mut self, prefix: &[u8], key: &[u8]);
 
 	/// Retrieve a value from storage under given key and prefix.
diff --git a/primitives/runtime/src/offchain/storage.rs b/primitives/runtime/src/offchain/storage.rs
index 9801e63c191cf..fd93e59fd3231 100644
--- a/primitives/runtime/src/offchain/storage.rs
+++ b/primitives/runtime/src/offchain/storage.rs
@@ -50,7 +50,7 @@ impl<'a> StorageValueRef<'a> {
 	}
 
 	/// Remove the associated value from the storage.
-	pub fn remove(&self) {
+	pub fn clear(&mut self) {
 		sp_io::offchain::local_storage_remove(self.kind, self.key)
 	}
 
diff --git a/primitives/runtime/src/offchain/storage_lock.rs b/primitives/runtime/src/offchain/storage_lock.rs
index 8412af0206e8b..20889f9daa866 100644
--- a/primitives/runtime/src/offchain/storage_lock.rs
+++ b/primitives/runtime/src/offchain/storage_lock.rs
@@ -16,7 +16,15 @@
 
 //! # Offchain Storage Lock
 //!
-//! A persistent storage lock with a defined expire time.
+//! A storage-based lock with a defined expiry time.
+//!
+//! The lock is using Local Storage and allows synchronizing
+//! access to critical section of your code for concurrently running Off-chain Workers.
+//! Usage of `PERSISTENT` variant of the storage persists the lock value even in case of re-orgs.
+//!
+//! A use case for the lock is to make sure that a particular section of the code is only run
+//! by one Off-chain Worker at the time. This may include performing a side-effect (i.e. an HTTP call)
+//! or alteration of single or multiple Local Storage entries.
 //!
 //! One use case would be collective updates of multiple data items
 //! or append / remove of i.e. sets, vectors which are stored in
@@ -32,7 +40,7 @@
 //!    if let Ok(_guard) =  lock.spin_lock() {
 //!         let acc = StorageValueRef::persistent(key);
 //!         let v: Vec<T> = acc.get::<Vec<T>>().unwrap().unwrap();
-//!         // modify `v` as desired
+//!         // modify `v` as desired - i.e. perform some heavy computation or side effects that should only be done once.
 //!         acc.set(v);
 //!    } else {
 //!         // the lock duration expired
@@ -45,7 +53,7 @@ use sp_core::offchain::{Duration, Timestamp};
 use sp_io::offchain;
 
 /// Default expirey duration in milliseconds.
-const STORAGE_LOCK_DEFAULT_EXPIREY_DURATION: u64 = 30_000;
+const STORAGE_LOCK_DEFAULT_EXPIRY_DURATION_MS: u64 = 30_000;
 
 /// Snooze duration before attempting to lock again in ms.
 const STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE: u64 = 100;
@@ -62,7 +70,7 @@ pub struct StorageLock<'a> {
 }
 
 impl<'a> StorageLock<'a> {
-	/// Create a new storage lock with [default expirey duration](Self::STORAGE_LOCK_DEFAULT_EXPIREY_DURATION).
+	/// Create a new storage lock with [default expiry duration](Self::STORAGE_LOCK_DEFAULT_EXPIRY_DURATION).
 	pub fn new<'k>(key: &'k [u8]) -> Self
 	where
 		'k: 'a,
@@ -122,7 +130,7 @@ impl<'a> StorageLock<'a> {
 
 	/// Try grabbing the lock until its expiry is reached.
 	///
-	/// Returns an error if the lock expired before it could be cought
+	/// Returns an error if the lock expired before it could be caught
 	pub fn spin_lock<'b, 'c>(&'b mut self) -> Result<StorageLockGuard<'a, 'c>, ()>
 	where
 		'a: 'b,
@@ -156,7 +164,7 @@ impl<'a> StorageLock<'a> {
 	/// Explicitly unlock the lock.
 	///
 	/// Does nothing if the lock was alrady unlocked before.
-	pub fn unlock(&mut self) {
+	fn unlock(&mut self) {
 		if let Some(_) = self.locked_until.take() {
 			self.value_ref.remove();
 		}
@@ -169,7 +177,9 @@ pub struct StorageLockGuard<'a, 'b> {
 }
 
 impl<'a, 'b> StorageLockGuard<'a, 'b> {
-	/// Consume the guard and unlock the underlying lock.
+	/// Consume the guard but DON'T unlock the underlying lock.
+	/// 
+	/// This can be used to finish off-chain worker execution while keeping the lock for it's desired expiry time.
 	pub fn forget(mut self) {
 		let _ = self.lock.take();
 	}

From ce606550a9e118772858307020edbf7bcdf0daa1 Mon Sep 17 00:00:00 2001
From: Bernhard Schuster <bernhard@parity.io>
Date: Wed, 13 May 2020 20:47:28 +0200
Subject: [PATCH 05/44] refactor/offchain/storage/lock: introduce `Lockable`
 trait part 1 of 2

---
 .../runtime/src/offchain/storage_lock.rs      | 325 ++++++++++++------
 1 file changed, 215 insertions(+), 110 deletions(-)

diff --git a/primitives/runtime/src/offchain/storage_lock.rs b/primitives/runtime/src/offchain/storage_lock.rs
index 20889f9daa866..591ea8d07bf1e 100644
--- a/primitives/runtime/src/offchain/storage_lock.rs
+++ b/primitives/runtime/src/offchain/storage_lock.rs
@@ -49,146 +49,251 @@
 //! ```
 
 use crate::offchain::storage::StorageValueRef;
+use codec::Codec;
 use sp_core::offchain::{Duration, Timestamp};
 use sp_io::offchain;
 
-/// Default expirey duration in milliseconds.
+//use frame_system how?
+
+/// Default expiry duration in milliseconds.
 const STORAGE_LOCK_DEFAULT_EXPIRY_DURATION_MS: u64 = 30_000;
 
 /// Snooze duration before attempting to lock again in ms.
 const STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE: u64 = 100;
 
+pub trait Lockable: Sized + Codec + Copy {
+    /// Get the current value of lockable.
+    fn current() -> Self;
+
+    /// Acquire a new deadline based on `Self::current()`
+    fn deadline() -> Self;
+
+    /// Verify the current value of `self` and `other`
+    /// to determine if the lock has expired.
+    fn expired(&self, other: &Self) -> bool;
+
+    /// Snooze the thread for time determined by `self` and `other`.
+    ///
+    /// Only called if not expired just yet.
+    /// Note that the deadline is only passed to allow some optimizations
+    /// for some `L` types.
+    fn snooze(&self, _deadline: &Self) {
+        sp_io::offchain::sleep_until(offchain::timestamp().add(Duration::from_millis(
+            STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE,
+        )));
+    }
+}
+
+impl Lockable for Timestamp {
+    fn current() -> Self {
+        offchain::timestamp()
+    }
+
+    fn deadline() -> Self {
+        Self::current().add(Duration::from_millis(STORAGE_LOCK_DEFAULT_EXPIRY_DURATION_MS))
+    }
+
+    fn expired(&self, reference: &Self) -> bool {
+        <Self as Lockable>::current() > *reference
+    }
+
+    fn snooze(&self, deadline: &Self) {
+        let remainder: Duration = self.diff(&deadline);
+        // do not snooze the full duration, but instead snooze max 100ms
+        // it might get unlocked in another thread
+        // consider adding some additive jitter here
+        let snooze = core::cmp::min(remainder.millis(), STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE);
+        sp_io::offchain::sleep_until(self.add(Duration::from_millis(snooze)));
+    }
+}
+
 /// A persisted guard state.
 ///
 /// An in DB persistent mutex for multi access items which are modified
 /// i.e. vecs or sets.
-pub struct StorageLock<'a> {
-	// A storage value ref which defines the DB entry representing the lock.
-	value_ref: StorageValueRef<'a>,
-	// `None` implies it was already released by `fn unlock(..)`
-	locked_until: Option<Timestamp>,
+pub struct StorageLock<'a, L>
+where
+    L: Sized + Lockable,
+{
+    // A storage value ref which defines the DB entry representing the lock.
+    value_ref: StorageValueRef<'a>,
+    deadline: L,
 }
 
-impl<'a> StorageLock<'a> {
-	/// Create a new storage lock with [default expiry duration](Self::STORAGE_LOCK_DEFAULT_EXPIRY_DURATION).
-	pub fn new<'k>(key: &'k [u8]) -> Self
-	where
-		'k: 'a,
-	{
-		Self {
-			value_ref: StorageValueRef::<'a>::persistent(key),
-			locked_until: None,
-		}
-	}
-
-	#[inline]
-	fn try_lock_inner(&mut self, duration: Duration) -> Result<(), Option<Timestamp>> {
-		let now = offchain::timestamp();
-		let expires_at = now.add(duration);
-		let res = self.value_ref.mutate(
-			|s: Option<Option<Timestamp>>| -> Result<Timestamp, Option<Timestamp>> {
-				match s {
-					// no lock set, we can safely acquire it
-					None => Ok(expires_at),
-					// lock is set, but it's old. We can re-acquire it.
-					Some(Some(current_good_until)) if current_good_until < now => Ok(expires_at),
-					// lock is present and is still active
-					Some(Some(current_good_until)) => Err(Some(current_good_until)),
-					_ => Err(None),
-				}
-			},
-		);
-		match res {
-			Ok(Ok(_)) => {
-				self.locked_until = Some(expires_at);
-				Ok(())
-			}
-			Ok(Err(timestamp)) => Err(Some(timestamp)), // failed to set the new value, but could read the current
-			Err(e) => Err(e),                           // forward the remaining value
-		}
-	}
+impl<'a, L> StorageLock<'a, L>
+where
+    L: Lockable,
+{
+	/// Create a new storage lock with [default expiry duration](Self::STORAGE_LOCK_DEFAULT_EXPIRY_DURATION_MS).
+    pub fn new<'k>(key: &'k [u8]) -> Self
+    where
+        'k: 'a,
+    {
+        Self {
+            value_ref: StorageValueRef::<'a>::persistent(key),
+            deadline: L::deadline(),
+        }
+    }
+
+    #[inline]
+    fn try_lock_inner(&mut self, new_deadline: L) -> Result<(), Option<L>> {
+        let now = L::current();
+        let res = self
+            .value_ref
+            .mutate(|s: Option<Option<L>>| -> Result<L, Option<L>> {
+                match s {
+                    // no lock set, we can safely acquire it
+                    None => Ok(new_deadline),
+                    // lock is set, but it's old. We can re-acquire it.
+                    Some(Some(deadline)) if now.expired(&deadline) => Ok(new_deadline),
+                    // lock is present and is still active
+                    Some(Some(deadline)) => Err(Some(deadline)),
+                    _ => Err(None),
+                }
+            });
+        match res {
+            Ok(Ok(_)) => Ok(()),
+            Ok(Err(_deadline)) => Err(None),
+            Err(e) => Err(e),
+        }
+    }
+
 
 	/// Attempt to lock the storage entry.
 	///
 	/// Returns a lock guard on success, otherwise an error containing `None` in
 	/// case the mutex was already unlocked before, or if the lock is still held
 	/// by another process `Err(Some(expiration_timestamp))`.
-	pub fn try_lock<'b>(&'b mut self) -> Result<StorageLockGuard<'a, 'b>, Option<Timestamp>>
-	where
-		'a: 'b,
-	{
-		if self.locked_until.is_none() {
-			match self.try_lock_inner(Duration::from_millis(STORAGE_LOCK_DEFAULT_EXPIREY_DURATION))
-			{
-				Ok(_) => Ok(StorageLockGuard::<'a, 'b> { lock: Some(self) }),
-				Err(e) => Err(e),
-			}
-		} else {
-			Err(self.locked_until)
-		}
-	}
+    pub fn try_lock<'b>(&'b mut self) -> Result<StorageLockGuard<'a, 'b, L>, Option<L>>
+    where
+        'a: 'b,
+    {
+        match self.try_lock_inner(self.deadline) {
+            Ok(_) => Ok(StorageLockGuard::<'a, 'b> { lock: Some(self) }),
+            Err(e) => Err(e),
+        }
+    }
 
 	/// Try grabbing the lock until its expiry is reached.
 	///
-	/// Returns an error if the lock expired before it could be caught
-	pub fn spin_lock<'b, 'c>(&'b mut self) -> Result<StorageLockGuard<'a, 'c>, ()>
-	where
-		'a: 'b,
-		'b: 'c,
-	{
-		if self.locked_until.is_none() {
-			loop {
-				// blind attempt on locking
-				let expires_at = match self
-					.try_lock_inner(Duration::from_millis(STORAGE_LOCK_DEFAULT_EXPIREY_DURATION))
-				{
-					Ok(_) => return Ok(StorageLockGuard::<'a, 'b> { lock: Some(self) }),
-					Err(None) => return Err(()),
-					Err(Some(expires_at)) => expires_at,
-				};
-				let remainder: Duration = offchain::timestamp().diff(&expires_at);
-				// do not snooze the full duration, but instead snooze max 100ms
-				// it might get unlocked in another thread
-				// consider adding some additive jitter here
-				let snooze =
-					core::cmp::min(remainder.millis(), STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE);
-				sp_io::offchain::sleep_until(
-					offchain::timestamp().add(Duration::from_millis(snooze)),
-				);
-			}
-		} else {
-			Err(())
-		}
-	}
+	/// Returns an error if the lock expired before it could be caught.
+    pub fn spin_lock<'b, 'c>(&'b mut self) -> StorageLockGuard<'a, 'b, L>
+    where
+        'a: 'b,
+    {
+        loop {
+            // blind attempt on locking
+            let deadline = match self.try_lock_inner(self.deadline) {
+                Ok(_) => return StorageLockGuard::<'a, 'b, L> { lock: Some(self) },
+                Err(Some(other_locks_deadline)) => other_locks_deadline,
+                _ => L::deadline(), // use the default
+            };
+            L::current().snooze(&deadline);
+        }
+    }
 
 	/// Explicitly unlock the lock.
-	///
-	/// Does nothing if the lock was alrady unlocked before.
-	fn unlock(&mut self) {
-		if let Some(_) = self.locked_until.take() {
-			self.value_ref.remove();
-		}
-	}
+    pub fn unlock(&mut self) {
+        self.value_ref.clear();
+    }
+}
+
+pub trait TimeLock<'a>: Sized {
+    fn with_deadline<'k>(key: &'k [u8], lock_deadline: Timestamp) -> Self
+    where
+        'k: 'a;
 }
 
+impl<'a> TimeLock<'a> for StorageLock<'a, Timestamp> {
+    fn with_deadline<'k>(key: &'k [u8], lock_deadline: Timestamp) -> Self
+    where
+        'k: 'a,
+    {
+        Self {
+            value_ref: StorageValueRef::<'a>::persistent(key),
+            deadline: lock_deadline,
+        }
+    }
+}
+
+// trait BlockAndTimeLock<T>: Sized where T::BlockNumber: Codec, T: frame_system::Trait {
+// 	fn with_blocks_and_deadline<'k>(key: &'k [u8], block_deadline: T::BlockNumber, time_deadline: Timestamp) -> Self;
+// }
+
+// impl<T: frame_system::Trait> BlockAndTimeLock<T> for StorageLock<BlockAndTime<T>>  where T::BlockNumber: Codec, T: frame_system::Trait {
+// 	fn with_blocks_and_time_deadline<'k>(key: &'k [u8], block_deadline: T::BlockNumber, time_deadline: Timestamp) -> Self {
+// 		unimplemented!()
+// 	}
+// }
+
 /// RAII style guard for a lock.
-pub struct StorageLockGuard<'a, 'b> {
-	lock: Option<&'b mut StorageLock<'a>>,
+pub struct StorageLockGuard<'a, 'b, L>
+where
+    L: Lockable,
+{
+    lock: Option<&'b mut StorageLock<'a, L>>,
 }
 
-impl<'a, 'b> StorageLockGuard<'a, 'b> {
-	/// Consume the guard but DON'T unlock the underlying lock.
-	/// 
-	/// This can be used to finish off-chain worker execution while keeping the lock for it's desired expiry time.
-	pub fn forget(mut self) {
-		let _ = self.lock.take();
-	}
+impl<'a, 'b, L> StorageLockGuard<'a, 'b, L>
+where
+    L: Lockable,
+{
+    /// Consume the guard but DO NOT unlock the underlying lock.
+    pub fn forget(mut self) {
+        let _ = self.lock.take();
+    }
 }
 
-impl<'a, 'b> Drop for StorageLockGuard<'a, 'b> {
-	fn drop(&mut self) {
-		if let Some(lock) = self.lock.take() {
-			lock.unlock();
-		}
-	}
+impl<'a, 'b, L> Drop for StorageLockGuard<'a, 'b, L>
+where
+    L: Lockable,
+{
+    fn drop(&mut self) {
+        if let Some(lock) = self.lock.take() {
+            lock.unlock();
+        }
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use sp_core::offchain::{testing, OffchainExt, OffchainStorage};
+    use sp_io::TestExternalities;
+
+    #[test]
+    fn simple_lock_write_unlock_lock_read_unlock() {
+        let (offchain, state) = testing::TestOffchainExt::new();
+        let mut t = TestExternalities::default();
+        t.register_extension(OffchainExt::new(offchain));
+
+        t.execute_with(|| {
+            let val1 = 0u32;
+            let val2 = 0xFFFF_FFFFu32;
+
+            let mut lock = StorageLock::<'_, Timestamp>::new(b"lock");
+
+            let val = StorageValueRef::persistent(b"protected_value");
+
+            {
+                let _guard = lock.spin_lock();
+
+                val.set(&val1);
+
+                assert_eq!(val.get::<u32>(), Some(Some(val1)));
+            }
+
+            {
+                let _guard = lock.spin_lock();
+                val.set(&val2);
+
+                assert_eq!(val.get::<u32>(), Some(Some(val2)));
+            }
+        });
+        // lock must have been cleared at this point
+        assert_eq!(
+            state.read().persistent_storage.get(b"Storage", b"lock"),
+            None
+        );
+    }
 }

From c058ad92a60bb22225db039c65281d293771beec Mon Sep 17 00:00:00 2001
From: Bernhard Schuster <bernhard@parity.io>
Date: Thu, 14 May 2020 10:33:58 +0200
Subject: [PATCH 06/44] chore/offchain/rename: _remove -> clean

---
 primitives/core/src/offchain/mod.rs        | 12 ++++++------
 primitives/core/src/offchain/testing.rs    |  2 +-
 primitives/io/src/lib.rs                   |  6 +++---
 primitives/runtime/src/offchain/storage.rs |  2 +-
 4 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/primitives/core/src/offchain/mod.rs b/primitives/core/src/offchain/mod.rs
index 14688881b09bc..09b2add5c28c5 100644
--- a/primitives/core/src/offchain/mod.rs
+++ b/primitives/core/src/offchain/mod.rs
@@ -355,7 +355,7 @@ pub trait Externalities: Send {
 	///
 	/// Note this storage is not part of the consensus, it's only accessible by
 	/// offchain worker tasks running on the same machine. It IS persisted between runs.
-	fn local_storage_remove(&mut self, kind: StorageKind, key: &[u8]);
+	fn local_storage_clear(&mut self, kind: StorageKind, key: &[u8]);
 
 	/// Sets a value in the local storage if it matches current value.
 	///
@@ -521,8 +521,8 @@ impl<T: Externalities + ?Sized> Externalities for Box<T> {
 		(&mut **self).local_storage_set(kind, key, value)
 	}
 
-	fn local_storage_remove(&mut self, kind: StorageKind, key: &[u8]) {
-		(&mut **self).local_storage_remove(kind, key)
+	fn local_storage_clear(&mut self, kind: StorageKind, key: &[u8]) {
+		(&mut **self).local_storage_clear(kind, key)
 	}
 
 	fn local_storage_compare_and_set(
@@ -630,9 +630,9 @@ impl<T: Externalities> Externalities for LimitedExternalities<T> {
 		self.externalities.local_storage_set(kind, key, value)
 	}
 
-	fn local_storage_remove(&mut self, kind: StorageKind, key: &[u8]) {
-		self.check(Capability::OffchainWorkerDbWrite, "local_storage_remove");
-		self.externalities.local_storage_remove(kind, key)
+	fn local_storage_clear(&mut self, kind: StorageKind, key: &[u8]) {
+		self.check(Capability::OffchainWorkerDbWrite, "local_storage_clear");
+		self.externalities.local_storage_clear(kind, key)
 	}
 
 	fn local_storage_compare_and_set(
diff --git a/primitives/core/src/offchain/testing.rs b/primitives/core/src/offchain/testing.rs
index 36d9f15a55348..3875a97b72360 100644
--- a/primitives/core/src/offchain/testing.rs
+++ b/primitives/core/src/offchain/testing.rs
@@ -178,7 +178,7 @@ impl offchain::Externalities for TestOffchainExt {
 		}.set(b"", key, value);
 	}
 
-	fn local_storage_remove(&mut self, kind: StorageKind, key: &[u8]) {
+	fn local_storage_clear(&mut self, kind: StorageKind, key: &[u8]) {
 		let mut state = self.0.write();
 		match kind {
 			StorageKind::LOCAL => &mut state.local_storage,
diff --git a/primitives/io/src/lib.rs b/primitives/io/src/lib.rs
index b6ab757c57803..2c216993a7d05 100644
--- a/primitives/io/src/lib.rs
+++ b/primitives/io/src/lib.rs
@@ -715,10 +715,10 @@ pub trait Offchain {
 	///
 	/// Note this storage is not part of the consensus, it's only accessible by
 	/// offchain worker tasks running on the same machine. It IS persisted between runs.
-	fn local_storage_remove(&mut self, kind: StorageKind, key: &[u8]) {
+	fn local_storage_clear(&mut self, kind: StorageKind, key: &[u8]) {
 		self.extension::<OffchainExt>()
-			.expect("local_storage_remove can be called only in the offchain worker context")
-			.local_storage_remove(kind, key)
+			.expect("local_storage_clear can be called only in the offchain worker context")
+			.local_storage_clear(kind, key)
 	}
 
 	/// Sets a value in the local storage if it matches current value.
diff --git a/primitives/runtime/src/offchain/storage.rs b/primitives/runtime/src/offchain/storage.rs
index fd93e59fd3231..ce260e8b1b6c7 100644
--- a/primitives/runtime/src/offchain/storage.rs
+++ b/primitives/runtime/src/offchain/storage.rs
@@ -51,7 +51,7 @@ impl<'a> StorageValueRef<'a> {
 
 	/// Remove the associated value from the storage.
 	pub fn clear(&mut self) {
-		sp_io::offchain::local_storage_remove(self.kind, self.key)
+		sp_io::offchain::local_storage_clear(self.kind, self.key)
 	}
 
 	/// Retrieve & decode the value from storage.

From 1cb99095421e532135174d63fc1d31606851829b Mon Sep 17 00:00:00 2001
From: Bernhard Schuster <bernhard@parity.io>
Date: Thu, 14 May 2020 10:34:24 +0200
Subject: [PATCH 07/44] feat/offchain/storage/lock: add TimeAndBlock based part
 2 of 2

---
 .../runtime/src/offchain/storage_lock.rs      | 433 +++++++++++-------
 1 file changed, 263 insertions(+), 170 deletions(-)

diff --git a/primitives/runtime/src/offchain/storage_lock.rs b/primitives/runtime/src/offchain/storage_lock.rs
index 591ea8d07bf1e..42d13ccff9fea 100644
--- a/primitives/runtime/src/offchain/storage_lock.rs
+++ b/primitives/runtime/src/offchain/storage_lock.rs
@@ -37,19 +37,20 @@
 //!
 //! fn append_to_in_storage_vec<'k, T>(key: &'k [u8], T) where T: Encode {
 //!    let mut lock = StorageLock::new(b"x::lock");
-//!    if let Ok(_guard) =  lock.spin_lock() {
+//!    {
+//!         let _guard = lock.spin_lock();
 //!         let acc = StorageValueRef::persistent(key);
 //!         let v: Vec<T> = acc.get::<Vec<T>>().unwrap().unwrap();
 //!         // modify `v` as desired - i.e. perform some heavy computation or side effects that should only be done once.
 //!         acc.set(v);
-//!    } else {
-//!         // the lock duration expired
+//!         // drop `_guard` implicitly at end of scope
 //!    }
 //! }
 //! ```
 
 use crate::offchain::storage::StorageValueRef;
 use codec::Codec;
+use codec::{Encode, Decode};
 use sp_core::offchain::{Duration, Timestamp};
 use sp_io::offchain;
 
@@ -61,50 +62,91 @@ const STORAGE_LOCK_DEFAULT_EXPIRY_DURATION_MS: u64 = 30_000;
 /// Snooze duration before attempting to lock again in ms.
 const STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE: u64 = 100;
 
+
+/// Lockable item for use with a persisted storage lock.
+///
+/// Bound for an item that has a stateful ordered meaning
+/// without explicitly requiring `Ord` trait in general.
 pub trait Lockable: Sized + Codec + Copy {
-    /// Get the current value of lockable.
-    fn current() -> Self;
-
-    /// Acquire a new deadline based on `Self::current()`
-    fn deadline() -> Self;
-
-    /// Verify the current value of `self` and `other`
-    /// to determine if the lock has expired.
-    fn expired(&self, other: &Self) -> bool;
-
-    /// Snooze the thread for time determined by `self` and `other`.
-    ///
-    /// Only called if not expired just yet.
-    /// Note that the deadline is only passed to allow some optimizations
-    /// for some `L` types.
-    fn snooze(&self, _deadline: &Self) {
-        sp_io::offchain::sleep_until(offchain::timestamp().add(Duration::from_millis(
-            STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE,
-        )));
-    }
+	/// Get the current value of lockable.
+	fn current() -> Self;
+
+	/// Acquire a new deadline based on `Self::current()`
+	fn deadline() -> Self;
+
+	/// Verify the current value of `self` against `deadline`
+	/// to determine if the lock has expired.
+	fn expired(&self, deadline: &Self) -> bool;
+
+	/// Snooze the thread for time determined by `self` and `other`.
+	///
+	/// Only called if not expired just yet.
+	/// Note that the deadline is only passed to allow some optimizations
+	/// for some `L` types.
+	fn snooze(&self, _deadline: &Self) {
+		sp_io::offchain::sleep_until(offchain::timestamp().add(Duration::from_millis(
+			STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE,
+		)));
+	}
 }
 
 impl Lockable for Timestamp {
-    fn current() -> Self {
-        offchain::timestamp()
-    }
-
-    fn deadline() -> Self {
-        Self::current().add(Duration::from_millis(STORAGE_LOCK_DEFAULT_EXPIRY_DURATION_MS))
-    }
-
-    fn expired(&self, reference: &Self) -> bool {
-        <Self as Lockable>::current() > *reference
-    }
-
-    fn snooze(&self, deadline: &Self) {
-        let remainder: Duration = self.diff(&deadline);
-        // do not snooze the full duration, but instead snooze max 100ms
-        // it might get unlocked in another thread
-        // consider adding some additive jitter here
-        let snooze = core::cmp::min(remainder.millis(), STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE);
-        sp_io::offchain::sleep_until(self.add(Duration::from_millis(snooze)));
-    }
+	fn current() -> Self {
+		offchain::timestamp()
+	}
+
+	fn deadline() -> Self {
+		Self::current().add(Duration::from_millis(
+			STORAGE_LOCK_DEFAULT_EXPIRY_DURATION_MS,
+		))
+	}
+
+	fn expired(&self, reference: &Self) -> bool {
+		<Self as Lockable>::current() > *reference
+	}
+
+	fn snooze(&self, deadline: &Self) {
+		let remainder: Duration = self.diff(&deadline);
+		// do not snooze the full duration, but instead snooze max 100ms
+		// it might get unlocked in another thread
+		// consider adding some additive jitter here
+		let snooze = core::cmp::min(remainder.millis(), STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE);
+		sp_io::offchain::sleep_until(self.add(Duration::from_millis(snooze)));
+	}
+}
+
+
+/// Lockable based on the current block number and a timestamp based deadline.
+#[derive(Debug, Copy, Clone, Encode, Decode)]
+pub struct BlockAndTime<B> where B: BlockNumberTrait {
+	pub block_number: B,
+	pub timestamp: Timestamp,
+}
+
+impl<B> BlockAndTime<B> where B: BlockNumberTrait {}
+
+impl<B> Lockable for BlockAndTime<B> where B: BlockNumberTrait {
+	fn current() -> Self {
+		Self {
+			block_number: B::current(),
+			timestamp: offchain::timestamp(),
+		}
+	}
+
+	fn deadline() -> Self {
+		Self::current()
+	}
+
+	fn expired(&self, reference: &Self) -> bool {
+		let current = <Self as Lockable>::current();
+		current.timestamp > reference.timestamp || current.block_number > reference.block_number
+	}
+
+	fn snooze(&self, deadline: &Self) {
+		let remainder: Duration = self.timestamp.diff(&(deadline.timestamp));
+		let snooze = core::cmp::min(remainder.millis(), STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE);
+		sp_io::offchain::sleep_until(self.timestamp.add(Duration::from_millis(snooze)));
+	}
 }
 
 /// A persisted guard state.
@@ -113,187 +155,238 @@ impl Lockable for Timestamp {
 /// i.e. vecs or sets.
 pub struct StorageLock<'a, L>
 where
-    L: Sized + Lockable,
+	L: Sized + Lockable,
 {
-    // A storage value ref which defines the DB entry representing the lock.
-    value_ref: StorageValueRef<'a>,
-    deadline: L,
+	// A storage value ref which defines the DB entry representing the lock.
+	value_ref: StorageValueRef<'a>,
+	deadline: L,
 }
 
 impl<'a, L> StorageLock<'a, L>
 where
-    L: Lockable,
+	L: Lockable,
 {
 	/// Create a new storage lock with [default expiry duration](Self::STORAGE_LOCK_DEFAULT_EXPIRY_DURATION_MS).
-    pub fn new<'k>(key: &'k [u8]) -> Self
-    where
-        'k: 'a,
-    {
-        Self {
-            value_ref: StorageValueRef::<'a>::persistent(key),
-            deadline: L::deadline(),
-        }
-    }
-
-    #[inline]
-    fn try_lock_inner(&mut self, new_deadline: L) -> Result<(), Option<L>> {
-        let now = L::current();
-        let res = self
-            .value_ref
-            .mutate(|s: Option<Option<L>>| -> Result<L, Option<L>> {
-                match s {
-                    // no lock set, we can safely acquire it
-                    None => Ok(new_deadline),
-                    // lock is set, but it's old. We can re-acquire it.
-                    Some(Some(deadline)) if now.expired(&deadline) => Ok(new_deadline),
-                    // lock is present and is still active
-                    Some(Some(deadline)) => Err(Some(deadline)),
-                    _ => Err(None),
-                }
-            });
-        match res {
-            Ok(Ok(_)) => Ok(()),
-            Ok(Err(_deadline)) => Err(None),
-            Err(e) => Err(e),
-        }
-    }
-
+	pub fn new<'k>(key: &'k [u8]) -> Self
+	where
+		'k: 'a,
+	{
+		Self {
+			value_ref: StorageValueRef::<'a>::persistent(key),
+			deadline: L::deadline(),
+		}
+	}
+
+	#[inline]
+	fn try_lock_inner(&mut self, new_deadline: L) -> Result<(), Option<L>> {
+		let now = L::current();
+		let res = self
+			.value_ref
+			.mutate(|s: Option<Option<L>>| -> Result<L, Option<L>> {
+				match s {
+					// no lock set, we can safely acquire it
+					None => Ok(new_deadline),
+					// lock is set, but it's old. We can re-acquire it.
+					Some(Some(deadline)) if now.expired(&deadline) => Ok(new_deadline),
+					// lock is present and is still active
+					Some(Some(deadline)) => Err(Some(deadline)),
+					_ => Err(None),
+				}
+			});
+		match res {
+			Ok(Ok(_)) => Ok(()),
+			Ok(Err(_deadline)) => Err(None),
+			Err(e) => Err(e),
+		}
+	}
 
 	/// Attempt to lock the storage entry.
 	///
 	/// Returns a lock guard on success, otherwise an error containing `None` in
 	/// case the mutex was already unlocked before, or if the lock is still held
 	/// by another process `Err(Some(expiration_timestamp))`.
-    pub fn try_lock<'b>(&'b mut self) -> Result<StorageLockGuard<'a, 'b, L>, Option<L>>
-    where
-        'a: 'b,
-    {
-        match self.try_lock_inner(self.deadline) {
-            Ok(_) => Ok(StorageLockGuard::<'a, 'b> { lock: Some(self) }),
-            Err(e) => Err(e),
-        }
-    }
+	pub fn try_lock<'b>(&'b mut self) -> Result<StorageLockGuard<'a, 'b, L>, Option<L>>
+	where
+		'a: 'b,
+	{
+		match self.try_lock_inner(self.deadline) {
+			Ok(_) => Ok(StorageLockGuard::<'a, 'b> { lock: Some(self) }),
+			Err(e) => Err(e),
+		}
+	}
 
 	/// Try grabbing the lock until its expiry is reached.
 	///
 	/// Returns an error if the lock expired before it could be caught.
-    pub fn spin_lock<'b, 'c>(&'b mut self) -> StorageLockGuard<'a, 'b, L>
-    where
-        'a: 'b,
-    {
-        loop {
-            // blind attempt on locking
-            let deadline = match self.try_lock_inner(self.deadline) {
-                Ok(_) => return StorageLockGuard::<'a, 'b, L> { lock: Some(self) },
-                Err(Some(other_locks_deadline)) => other_locks_deadline,
-                _ => L::deadline(), // use the default
-            };
-            L::current().snooze(&deadline);
-        }
-    }
+	pub fn spin_lock<'b, 'c>(&'b mut self) -> StorageLockGuard<'a, 'b, L>
+	where
+		'a: 'b,
+	{
+		loop {
+			// blind attempt on locking
+			let deadline = match self.try_lock_inner(self.deadline) {
+				Ok(_) => return StorageLockGuard::<'a, 'b, L> { lock: Some(self) },
+				Err(Some(other_locks_deadline)) => other_locks_deadline,
+				_ => L::deadline(), // use the default
+			};
+			L::current().snooze(&deadline);
+		}
+	}
 
 	/// Explicitly unlock the lock.
-    pub fn unlock(&mut self) {
-        self.value_ref.clear();
-    }
+	pub fn unlock(&mut self) {
+		self.value_ref.clear();
+	}
 }
 
+
+/// Extension trait for locks based on [`Timestamp`](::sp_core::offchain::Timestamp).
+///
+/// Allows explicity setting the timeout on construction
+/// instead of using the implicit default timeout of
+/// [`STORAGE_LOCK_DEFAULT_EXPIRY_DURATION_MS`](Self::STORAGE_LOCK_DEFAULT_EXPIRY_DURATION_MS).
 pub trait TimeLock<'a>: Sized {
-    fn with_deadline<'k>(key: &'k [u8], lock_deadline: Timestamp) -> Self
-    where
-        'k: 'a;
+	/// Provide an explicit deadline timestamp at which the locked state of the lock
+	/// becomes stale and may be dismissed by `fn try_lock(..)`, `fn spin_lock(..)` and others.
+	fn with_deadline<'k>(key: &'k [u8], lock_deadline: Timestamp) -> Self
+	where
+		'k: 'a;
 }
 
 impl<'a> TimeLock<'a> for StorageLock<'a, Timestamp> {
-    fn with_deadline<'k>(key: &'k [u8], lock_deadline: Timestamp) -> Self
-    where
-        'k: 'a,
-    {
-        Self {
-            value_ref: StorageValueRef::<'a>::persistent(key),
-            deadline: lock_deadline,
-        }
-    }
+	fn with_deadline<'k>(key: &'k [u8], lock_deadline: Timestamp) -> Self
+	where
+		'k: 'a,
+	{
+		Self {
+			value_ref: StorageValueRef::<'a>::persistent(key),
+			deadline: lock_deadline,
+		}
+	}
+}
+
+/// Bound for block numbers which commonly will be implemented by the `frame_system::Trait::BlockNumber`.
+///
+/// This trait has no intrinsic meaning and exists only to decouple `frame_system`
+/// from `runtime` crate and avoid a circular dependency.
+pub trait BlockNumberTrait: Codec + Copy + Clone + Ord + Eq {
+	/// Returns the current block number.
+	///
+	/// Commonly this will be implemented as
+	/// ```rust
+	/// fn current() -> Self {
+	///     frame_system::Module<Trait>::block_number()
+	/// }
+	/// ```
+	/// but note that the definition of current is
+	/// application specific.
+	fn current() -> Self;
 }
 
-// trait BlockAndTimeLock<T>: Sized where T::BlockNumber: Codec, T: frame_system::Trait {
-// 	fn with_blocks_and_deadline<'k>(key: &'k [u8], block_deadline: T::BlockNumber, time_deadline: Timestamp) -> Self;
-// }
+/// Extension for lor storage locks which are based on blocks in addition to a timestamp.
+trait BlockAndTimeLock<'a, B>: Sized
+where
+	B: BlockNumberTrait,
+{
+	fn with_block_and_time_deadline<'k>(
+		key: &'k [u8],
+		block_deadline: B,
+		time_deadline: Timestamp,
+	) -> Self
+	where
+		'k: 'a;
+}
 
-// impl<T: frame_system::Trait> BlockAndTimeLock<T> for StorageLock<BlockAndTime<T>>  where T::BlockNumber: Codec, T: frame_system::Trait {
-// 	fn with_blocks_and_time_deadline<'k>(key: &'k [u8], block_deadline: T::BlockNumber, time_deadline: Timestamp) -> Self {
-// 		unimplemented!()
-// 	}
-// }
+impl<'a, B> BlockAndTimeLock<'a, B> for StorageLock<'a, BlockAndTime<B>>
+where
+	B: BlockNumberTrait,
+{
+	fn with_block_and_time_deadline<'k>(
+		key: &'k [u8],
+		block_deadline: B,
+		time_deadline: Timestamp,
+	) -> Self
+	where
+		'k: 'a,
+	{
+		Self {
+			value_ref: StorageValueRef::<'a>::persistent(key),
+			deadline: BlockAndTime {
+				block_number: block_deadline,
+				timestamp: time_deadline,
+			},
+		}
+	}
+}
 
 /// RAII style guard for a lock.
 pub struct StorageLockGuard<'a, 'b, L>
 where
-    L: Lockable,
+	L: Lockable,
 {
-    lock: Option<&'b mut StorageLock<'a, L>>,
+	lock: Option<&'b mut StorageLock<'a, L>>,
 }
 
 impl<'a, 'b, L> StorageLockGuard<'a, 'b, L>
 where
-    L: Lockable,
+	L: Lockable,
 {
-    /// Consume the guard but DO NOT unlock the underlying lock.
-    pub fn forget(mut self) {
-        let _ = self.lock.take();
-    }
+	/// Consume the guard but DO NOT unlock the underlying lock.
+	pub fn forget(mut self) {
+		let _ = self.lock.take();
+	}
 }
 
 impl<'a, 'b, L> Drop for StorageLockGuard<'a, 'b, L>
 where
-    L: Lockable,
+	L: Lockable,
 {
-    fn drop(&mut self) {
-        if let Some(lock) = self.lock.take() {
-            lock.unlock();
-        }
-    }
+	fn drop(&mut self) {
+		if let Some(lock) = self.lock.take() {
+			lock.unlock();
+		}
+	}
 }
 
 #[cfg(test)]
 mod tests {
-    use super::*;
-    use sp_core::offchain::{testing, OffchainExt, OffchainStorage};
-    use sp_io::TestExternalities;
+	use super::*;
+	use sp_core::offchain::{testing, OffchainExt, OffchainStorage};
+	use sp_io::TestExternalities;
 
-    #[test]
-    fn simple_lock_write_unlock_lock_read_unlock() {
-        let (offchain, state) = testing::TestOffchainExt::new();
-        let mut t = TestExternalities::default();
-        t.register_extension(OffchainExt::new(offchain));
+	#[test]
+	fn simple_lock_write_unlock_lock_read_unlock() {
+		let (offchain, state) = testing::TestOffchainExt::new();
+		let mut t = TestExternalities::default();
+		t.register_extension(OffchainExt::new(offchain));
 
-        t.execute_with(|| {
-            let val1 = 0u32;
-            let val2 = 0xFFFF_FFFFu32;
+		t.execute_with(|| {
+			let val1 = 0u32;
+			let val2 = 0xFFFF_FFFFu32;
 
-            let mut lock = StorageLock::<'_, Timestamp>::new(b"lock");
+			let mut lock = StorageLock::<'_, Timestamp>::new(b"lock");
 
-            let val = StorageValueRef::persistent(b"protected_value");
+			let val = StorageValueRef::persistent(b"protected_value");
 
-            {
-                let _guard = lock.spin_lock();
+			{
+				let _guard = lock.spin_lock();
 
-                val.set(&val1);
+				val.set(&val1);
 
-                assert_eq!(val.get::<u32>(), Some(Some(val1)));
-            }
+				assert_eq!(val.get::<u32>(), Some(Some(val1)));
+			}
 
-            {
-                let _guard = lock.spin_lock();
-                val.set(&val2);
+			{
+				let _guard = lock.spin_lock();
+				val.set(&val2);
 
-                assert_eq!(val.get::<u32>(), Some(Some(val2)));
-            }
-        });
-        // lock must have been cleared at this point
-        assert_eq!(
-            state.read().persistent_storage.get(b"Storage", b"lock"),
-            None
-        );
-    }
+				assert_eq!(val.get::<u32>(), Some(Some(val2)));
+			}
+		});
+		// lock must have been cleared at this point
+		assert_eq!(
+			state.read().persistent_storage.get(b"Storage", b"lock"),
+			None
+		);
+	}
 }

From c6f60c7390365de85f4eb5cae0326491d2835f17 Mon Sep 17 00:00:00 2001
From: Bernhard Schuster <bernhard@parity.io>
Date: Thu, 14 May 2020 10:41:48 +0200
Subject: [PATCH 08/44] fix/offchain/storage/lock: block and time expiry must
 be && not ||

---
 primitives/runtime/src/offchain/storage_lock.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/primitives/runtime/src/offchain/storage_lock.rs b/primitives/runtime/src/offchain/storage_lock.rs
index 42d13ccff9fea..c1db85f856ad0 100644
--- a/primitives/runtime/src/offchain/storage_lock.rs
+++ b/primitives/runtime/src/offchain/storage_lock.rs
@@ -139,7 +139,7 @@ impl<B> Lockable for BlockAndTime<B> where B: BlockNumberTrait {
 
 	fn expired(&self, reference: &Self) -> bool {
 		let current = <Self as Lockable>::current();
-		current.timestamp > reference.timestamp || current.block_number > reference.block_number
+		current.timestamp > reference.timestamp && current.block_number > reference.block_number
 	}
 
 	fn snooze(&self, deadline: &Self) {

From 09e0c2cb8f8d60169cbdfe233bf304f8c775040b Mon Sep 17 00:00:00 2001
From: Bernhard Schuster <bernhard@parity.io>
Date: Thu, 14 May 2020 10:54:22 +0200
Subject: [PATCH 09/44] chore/offchain/storage: minor fmt doc comments

---
 .../runtime/src/offchain/storage_lock.rs      | 21 ++++++++++++-------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/primitives/runtime/src/offchain/storage_lock.rs b/primitives/runtime/src/offchain/storage_lock.rs
index c1db85f856ad0..d3ac746b37fdb 100644
--- a/primitives/runtime/src/offchain/storage_lock.rs
+++ b/primitives/runtime/src/offchain/storage_lock.rs
@@ -50,7 +50,7 @@
 
 use crate::offchain::storage::StorageValueRef;
 use codec::Codec;
-use codec::{Encode, Decode};
+use codec::{Decode, Encode};
 use sp_core::offchain::{Duration, Timestamp};
 use sp_io::offchain;
 
@@ -62,7 +62,6 @@ const STORAGE_LOCK_DEFAULT_EXPIRY_DURATION_MS: u64 = 30_000;
 /// Snooze duration before attempting to lock again in ms.
 const STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE: u64 = 100;
 
-
 /// Lockable item for use with a persisted storage lock.
 ///
 /// Bound for an item that has a stateful ordered meaning
@@ -115,17 +114,24 @@ impl Lockable for Timestamp {
 	}
 }
 
-
 /// Lockable based on the current block number and a timestamp based deadline.
 #[derive(Debug, Copy, Clone, Encode, Decode)]
-pub struct BlockAndTime<B> where B: BlockNumberTrait {
+pub struct BlockAndTime<B>
+where
+	B: BlockNumberTrait,
+{
+	/// The block number that has to be reached in order
+	/// for the lock to be considered stale.
 	pub block_number: B,
+	/// Additional timestamp based deadline which has to be
+	/// reached in order for the lock to become stale.
 	pub timestamp: Timestamp,
 }
 
-impl<B> BlockAndTime<B> where B: BlockNumberTrait {}
-
-impl<B> Lockable for BlockAndTime<B> where B: BlockNumberTrait {
+impl<B> Lockable for BlockAndTime<B>
+where
+	B: BlockNumberTrait,
+{
 	fn current() -> Self {
 		Self {
 			block_number: B::current(),
@@ -239,7 +245,6 @@ where
 	}
 }
 
-
 /// Extension trait for locks based on [`Timestamp`](::sp_core::offchain::Timestamp).
 ///
 /// Allows explicity setting the timeout on construction

From 9d6716ccb0b535a7329de2038a4307de0163896f Mon Sep 17 00:00:00 2001
From: Bernhard Schuster <bernhard@parity.io>
Date: Mon, 18 May 2020 08:59:17 +0200
Subject: [PATCH 10/44] doc/comment: prefer markdown emphasis over CAPS

---
 primitives/core/src/offchain/mod.rs | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/primitives/core/src/offchain/mod.rs b/primitives/core/src/offchain/mod.rs
index 09b2add5c28c5..64adc33aff923 100644
--- a/primitives/core/src/offchain/mod.rs
+++ b/primitives/core/src/offchain/mod.rs
@@ -348,13 +348,13 @@ pub trait Externalities: Send {
 	/// Sets a value in the local storage.
 	///
 	/// Note this storage is not part of the consensus, it's only accessible by
-	/// offchain worker tasks running on the same machine. It IS persisted between runs.
+	/// offchain worker tasks running on the same machine. It _is_ persisted between runs.
 	fn local_storage_set(&mut self, kind: StorageKind, key: &[u8], value: &[u8]);
 
 	/// Removes a value in the local storage.
 	///
 	/// Note this storage is not part of the consensus, it's only accessible by
-	/// offchain worker tasks running on the same machine. It IS persisted between runs.
+	/// offchain worker tasks running on the same machine. It _is_ persisted between runs.
 	fn local_storage_clear(&mut self, kind: StorageKind, key: &[u8]);
 
 	/// Sets a value in the local storage if it matches current value.
@@ -365,7 +365,7 @@ pub trait Externalities: Send {
 	/// Returns `true` if the value has been set, `false` otherwise.
 	///
 	/// Note this storage is not part of the consensus, it's only accessible by
-	/// offchain worker tasks running on the same machine. It IS persisted between runs.
+	/// offchain worker tasks running on the same machine. It _is_ persisted between runs.
 	fn local_storage_compare_and_set(
 		&mut self,
 		kind: StorageKind,
@@ -378,7 +378,7 @@ pub trait Externalities: Send {
 	///
 	/// If the value does not exist in the storage `None` will be returned.
 	/// Note this storage is not part of the consensus, it's only accessible by
-	/// offchain worker tasks running on the same machine. It IS persisted between runs.
+	/// offchain worker tasks running on the same machine. It _is_ persisted between runs.
 	fn local_storage_get(&mut self, kind: StorageKind, key: &[u8]) -> Option<Vec<u8>>;
 
 	/// Initiates a http request given HTTP verb and the URL.

From 74e2e8ca282a1860224c3faf8a79927de6919b60 Mon Sep 17 00:00:00 2001
From: Bernhard Schuster <bernhard@parity.io>
Date: Mon, 18 May 2020 08:59:59 +0200
Subject: [PATCH 11/44] doc/comment: rewrap multiline module level docs

---
 primitives/runtime/src/offchain/storage.rs    |  3 ++-
 .../runtime/src/offchain/storage_lock.rs      | 19 ++++++++++---------
 2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/primitives/runtime/src/offchain/storage.rs b/primitives/runtime/src/offchain/storage.rs
index ce260e8b1b6c7..21deac64ec5b5 100644
--- a/primitives/runtime/src/offchain/storage.rs
+++ b/primitives/runtime/src/offchain/storage.rs
@@ -71,7 +71,8 @@ impl<'a> StorageValueRef<'a> {
 	/// Function `f` should return a new value that we should attempt to write to storage.
 	/// This function returns:
 	/// 1. `Ok(Ok(T))` in case the value has been successfully set.
-	/// 2. `Ok(Err(T))` in case the value was returned, but it couldn't have been set.
+	/// 2. `Ok(Err(T))` in case the value was calculated by the passed closure `f`,
+	///    but it could not be stored.
 	/// 3. `Err(_)` in case `f` returns an error.
 	pub fn mutate<T, E, F>(&self, f: F) -> Result<Result<T, T>, E> where
 		T: codec::Codec,
diff --git a/primitives/runtime/src/offchain/storage_lock.rs b/primitives/runtime/src/offchain/storage_lock.rs
index d3ac746b37fdb..556a29b32dfe5 100644
--- a/primitives/runtime/src/offchain/storage_lock.rs
+++ b/primitives/runtime/src/offchain/storage_lock.rs
@@ -18,17 +18,18 @@
 //!
 //! A storage-based lock with a defined expiry time.
 //!
-//! The lock is using Local Storage and allows synchronizing
-//! access to critical section of your code for concurrently running Off-chain Workers.
-//! Usage of `PERSISTENT` variant of the storage persists the lock value even in case of re-orgs.
+//! The lock is using Local Storage and allows synchronizing access to critical
+//! section of your code for concurrently running Off-chain Workers. Usage of
+//! `PERSISTENT` variant of the storage persists the lock value even in case of
+//! re-orgs.
 //!
-//! A use case for the lock is to make sure that a particular section of the code is only run
-//! by one Off-chain Worker at the time. This may include performing a side-effect (i.e. an HTTP call)
-//! or alteration of single or multiple Local Storage entries.
+//! A use case for the lock is to make sure that a particular section of the
+//! code is only run by one Off-chain Worker at the time. This may include
+//! performing a side-effect (i.e. an HTTP call) or alteration of single or
+//! multiple Local Storage entries.
 //!
-//! One use case would be collective updates of multiple data items
-//! or append / remove of i.e. sets, vectors which are stored in
-//! the offchain storage DB.
+//! One use case would be collective updates of multiple data items or append /
+//! remove of i.e. sets, vectors which are stored in the offchain storage DB.
 //!
 //! ## Example:
 //!

From 05d7e54ef321e287fc05ead206824d7a6d45f0b7 Mon Sep 17 00:00:00 2001
From: Bernhard Schuster <bernhard@parity.io>
Date: Mon, 18 May 2020 09:00:23 +0200
Subject: [PATCH 12/44] doc/comment: rephrase

---
 primitives/runtime/src/offchain/storage_lock.rs | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/primitives/runtime/src/offchain/storage_lock.rs b/primitives/runtime/src/offchain/storage_lock.rs
index 556a29b32dfe5..1edeb9beea0c8 100644
--- a/primitives/runtime/src/offchain/storage_lock.rs
+++ b/primitives/runtime/src/offchain/storage_lock.rs
@@ -156,10 +156,11 @@ where
 	}
 }
 
-/// A persisted guard state.
+/// Storage based lock.
 ///
-/// An in DB persistent mutex for multi access items which are modified
-/// i.e. vecs or sets.
+/// A lock that is persisted in the DB and provides a mutex behaviour
+/// with a defined safety expirey deadline based on a [`Lockable`](Self::Lockable)
+/// implementation.
 pub struct StorageLock<'a, L>
 where
 	L: Sized + Lockable,

From 47d492f6c111ec6ff36f2391baa10211dc91efef Mon Sep 17 00:00:00 2001
From: Bernhard Schuster <bernhard@parity.io>
Date: Mon, 18 May 2020 11:26:17 +0200
Subject: [PATCH 13/44] impl sleep_until and use the actual time for the test
 env

---
 primitives/core/src/offchain/testing.rs | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/primitives/core/src/offchain/testing.rs b/primitives/core/src/offchain/testing.rs
index 3875a97b72360..573fd823e7aad 100644
--- a/primitives/core/src/offchain/testing.rs
+++ b/primitives/core/src/offchain/testing.rs
@@ -72,8 +72,6 @@ pub struct OffchainState {
 	pub persistent_storage: InMemOffchainStorage,
 	/// Local storage
 	pub local_storage: InMemOffchainStorage,
-	/// Current timestamp (unix millis)
-	pub timestamp: u64,
 	/// A supposedly random seed.
 	pub seed: [u8; 32],
 }
@@ -159,11 +157,21 @@ impl offchain::Externalities for TestOffchainExt {
 	}
 
 	fn timestamp(&mut self) -> Timestamp {
-		Timestamp::from_unix_millis(self.0.read().timestamp)
+		use std::convert::TryInto;
+		let millis = std::time::SystemTime::now()
+			.duration_since(std::time::UNIX_EPOCH)
+			.expect("Calculating unix epoch in millis for tests must succeed. qed")
+			.as_millis().try_into()
+			.expect("Calculating unix epoch in millis exceeded u64. qed");
+		Timestamp(millis)
 	}
 
-	fn sleep_until(&mut self, _deadline: Timestamp) {
-		unimplemented!("not needed in tests so far")
+	fn sleep_until(&mut self, deadline: Timestamp) {
+		let now = self.timestamp();
+		if deadline > now {
+			let dur = deadline.diff(&now);
+			std::thread::sleep(std::time::Duration::from_millis(dur.millis()));
+		}
 	}
 
 	fn random_seed(&mut self) -> [u8; 32] {

From 48d15ec7c66207fcca7b0f7d9e53ccaa1da8b2aa Mon Sep 17 00:00:00 2001
From: Bernhard Schuster <bernhard@parity.io>
Date: Mon, 18 May 2020 11:27:07 +0200
Subject: [PATCH 14/44] feat/test: add more tests, ignore some sample impl
 doctests

---
 .../runtime/src/offchain/storage_lock.rs      | 88 +++++++++++++++----
 1 file changed, 71 insertions(+), 17 deletions(-)

diff --git a/primitives/runtime/src/offchain/storage_lock.rs b/primitives/runtime/src/offchain/storage_lock.rs
index 1edeb9beea0c8..130321a8d19d0 100644
--- a/primitives/runtime/src/offchain/storage_lock.rs
+++ b/primitives/runtime/src/offchain/storage_lock.rs
@@ -34,9 +34,11 @@
 //! ## Example:
 //!
 //! ```rust
+//! # use crate::offchain::storage::StorageValueRef;
+//! # use codec::{Decode, Encode, Codec};
 //! // in your off-chain worker code
 //!
-//! fn append_to_in_storage_vec<'k, T>(key: &'k [u8], T) where T: Encode {
+//! fn append_to_in_storage_vec<'k, T>(key: &'k [u8], _: T) where T: Encode {
 //!    let mut lock = StorageLock::new(b"x::lock");
 //!    {
 //!         let _guard = lock.spin_lock();
@@ -50,8 +52,7 @@
 //! ```
 
 use crate::offchain::storage::StorageValueRef;
-use codec::Codec;
-use codec::{Decode, Encode};
+use codec::{Codec, Decode, Encode};
 use sp_core::offchain::{Duration, Timestamp};
 use sp_io::offchain;
 
@@ -280,7 +281,7 @@ pub trait BlockNumberTrait: Codec + Copy + Clone + Ord + Eq {
 	/// Returns the current block number.
 	///
 	/// Commonly this will be implemented as
-	/// ```rust
+	/// ```ignore
 	/// fn current() -> Self {
 	///     frame_system::Module<Trait>::block_number()
 	/// }
@@ -361,39 +362,92 @@ mod tests {
 	use sp_core::offchain::{testing, OffchainExt, OffchainStorage};
 	use sp_io::TestExternalities;
 
+	const VAL_1: u32 = 0u32;
+	const VAL_2: u32 = 0xFFFF_FFFFu32;
+
 	#[test]
-	fn simple_lock_write_unlock_lock_read_unlock() {
+	fn storage_lock_write_unlock_lock_read_unlock() {
 		let (offchain, state) = testing::TestOffchainExt::new();
 		let mut t = TestExternalities::default();
 		t.register_extension(OffchainExt::new(offchain));
 
 		t.execute_with(|| {
-			let val1 = 0u32;
-			let val2 = 0xFFFF_FFFFu32;
-
-			let mut lock = StorageLock::<'_, Timestamp>::new(b"lock");
+			let mut lock = StorageLock::<'_, Timestamp>::new(b"lock_1");
 
 			let val = StorageValueRef::persistent(b"protected_value");
 
 			{
 				let _guard = lock.spin_lock();
 
-				val.set(&val1);
+				val.set(&VAL_1);
 
-				assert_eq!(val.get::<u32>(), Some(Some(val1)));
+				assert_eq!(val.get::<u32>(), Some(Some(VAL_1)));
 			}
 
 			{
 				let _guard = lock.spin_lock();
-				val.set(&val2);
+				val.set(&VAL_2);
 
-				assert_eq!(val.get::<u32>(), Some(Some(val2)));
+				assert_eq!(val.get::<u32>(), Some(Some(VAL_2)));
 			}
 		});
 		// lock must have been cleared at this point
-		assert_eq!(
-			state.read().persistent_storage.get(b"Storage", b"lock"),
-			None
-		);
+		assert_eq!(state.read().persistent_storage.get(b"", b"lock_1"), None);
+	}
+
+	#[test]
+	fn storage_lock_and_forget() {
+		let (offchain, state) = testing::TestOffchainExt::new();
+		let mut t = TestExternalities::default();
+		t.register_extension(OffchainExt::new(offchain));
+
+		t.execute_with(|| {
+			let mut lock = StorageLock::<'_, Timestamp>::new(b"lock_2");
+
+			let val = StorageValueRef::persistent(b"protected_value");
+
+			let guard = lock.spin_lock();
+
+			val.set(&VAL_1);
+
+			assert_eq!(val.get::<u32>(), Some(Some(VAL_1)));
+
+			guard.forget();
+		});
+		// lock must have been cleared at this point
+		let opt = state.read().persistent_storage.get(b"", b"lock_2");
+		assert!(opt.is_some());
+	}
+
+	#[test]
+	fn storage_lock_and_let_expire_and_lock_again() {
+		let (offchain, state) = testing::TestOffchainExt::new();
+		let mut t = TestExternalities::default();
+		t.register_extension(OffchainExt::new(offchain));
+
+		t.execute_with(|| {
+			let sleep_until = Timestamp::current().add(Duration::from_millis(500));
+			let lock_expiration = Timestamp::current().add(Duration::from_millis(200));
+
+			let mut lock = StorageLock::<'_, Timestamp>::with_deadline(b"lock_3", lock_expiration);
+
+			{
+				let guard = lock.spin_lock();
+				guard.forget();
+			}
+
+			// assure the lock expires
+			offchain::sleep_until(sleep_until);
+
+			let mut lock = StorageLock::<'_, Timestamp>::new(b"lock_3");
+			let res = lock.try_lock();
+			assert!(res.is_ok());
+			let guard = res.unwrap();
+			guard.forget();
+		});
+
+		// lock must have been cleared at this point
+		let opt = state.read().persistent_storage.get(b"", b"lock_3");
+		assert!(opt.is_some());
 	}
 }

From add98ae0932fd26753e291401654b3e1924bfa5f Mon Sep 17 00:00:00 2001
From: Bernhard Schuster <bernhard@ahoi.io>
Date: Mon, 18 May 2020 11:41:28 +0200
Subject: [PATCH 15/44] fix/review: Apply suggestions from code review

Co-authored-by: Nikolay Volf <nikvolf@gmail.com>
---
 primitives/runtime/src/offchain/storage_lock.rs | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/primitives/runtime/src/offchain/storage_lock.rs b/primitives/runtime/src/offchain/storage_lock.rs
index 130321a8d19d0..17bbf560b5fc8 100644
--- a/primitives/runtime/src/offchain/storage_lock.rs
+++ b/primitives/runtime/src/offchain/storage_lock.rs
@@ -176,9 +176,7 @@ where
 	L: Lockable,
 {
 	/// Create a new storage lock with [default expiry duration](Self::STORAGE_LOCK_DEFAULT_EXPIRY_DURATION_MS).
-	pub fn new<'k>(key: &'k [u8]) -> Self
-	where
-		'k: 'a,
+	pub fn new(key: &'a [u8]) -> Self
 	{
 		Self {
 			value_ref: StorageValueRef::<'a>::persistent(key),
@@ -186,7 +184,6 @@ where
 		}
 	}
 
-	#[inline]
 	fn try_lock_inner(&mut self, new_deadline: L) -> Result<(), Option<L>> {
 		let now = L::current();
 		let res = self
@@ -227,7 +224,7 @@ where
 	/// Try grabbing the lock until its expiry is reached.
 	///
 	/// Returns an error if the lock expired before it could be caught.
-	pub fn spin_lock<'b, 'c>(&'b mut self) -> StorageLockGuard<'a, 'b, L>
+	pub fn spin_lock<'b>(&'b mut self) -> StorageLockGuard<'a, 'b, L>
 	where
 		'a: 'b,
 	{
@@ -256,16 +253,11 @@ where
 pub trait TimeLock<'a>: Sized {
 	/// Provide an explicit deadline timestamp at which the locked state of the lock
 	/// becomes stale and may be dismissed by `fn try_lock(..)`, `fn spin_lock(..)` and others.
-	fn with_deadline<'k>(key: &'k [u8], lock_deadline: Timestamp) -> Self
-	where
-		'k: 'a;
+	fn with_deadline(key: &'a [u8], lock_deadline: Timestamp) -> Self;
 }
 
 impl<'a> TimeLock<'a> for StorageLock<'a, Timestamp> {
-	fn with_deadline<'k>(key: &'k [u8], lock_deadline: Timestamp) -> Self
-	where
-		'k: 'a,
-	{
+	fn with_deadline(key: &'a [u8], lock_deadline: Timestamp) -> Self {
 		Self {
 			value_ref: StorageValueRef::<'a>::persistent(key),
 			deadline: lock_deadline,

From 9d4ef5db96814a9bcf3bb308c6ea125369aedcf1 Mon Sep 17 00:00:00 2001
From: Bernhard Schuster <bernhard@parity.io>
Date: Mon, 18 May 2020 13:49:05 +0200
Subject: [PATCH 16/44] doc/comment: better description

---
 primitives/runtime/src/offchain/storage_lock.rs | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/primitives/runtime/src/offchain/storage_lock.rs b/primitives/runtime/src/offchain/storage_lock.rs
index 17bbf560b5fc8..a9c804d787d30 100644
--- a/primitives/runtime/src/offchain/storage_lock.rs
+++ b/primitives/runtime/src/offchain/storage_lock.rs
@@ -39,7 +39,10 @@
 //! // in your off-chain worker code
 //!
 //! fn append_to_in_storage_vec<'k, T>(key: &'k [u8], _: T) where T: Encode {
-//!    let mut lock = StorageLock::new(b"x::lock");
+//!    // `access::lock` defines the storage entry which is used for
+//!    // persisting the lock in the underlying database.
+//!    // The entry name _must_ be unique and can be seen as mutex instance reference.
+//!    let mut lock = StorageLock::new(b"access::lock");
 //!    {
 //!         let _guard = lock.spin_lock();
 //!         let acc = StorageValueRef::persistent(key);

From 1bc3b6a582fa8cc7cd4ff2572b96595b8940426a Mon Sep 17 00:00:00 2001
From: Bernhard Schuster <bernhard@ahoi.io>
Date: Mon, 18 May 2020 14:12:48 +0200
Subject: [PATCH 17/44] fix/review: Apply suggestions from code review

Co-authored-by: Nikolay Volf <nikvolf@gmail.com>
---
 .../runtime/src/offchain/storage_lock.rs      | 21 +++++--------------
 1 file changed, 5 insertions(+), 16 deletions(-)

diff --git a/primitives/runtime/src/offchain/storage_lock.rs b/primitives/runtime/src/offchain/storage_lock.rs
index a9c804d787d30..82ad8b4fbfe9c 100644
--- a/primitives/runtime/src/offchain/storage_lock.rs
+++ b/primitives/runtime/src/offchain/storage_lock.rs
@@ -121,10 +121,7 @@ impl Lockable for Timestamp {
 
 /// Lockable based on the current block number and a timestamp based deadline.
 #[derive(Debug, Copy, Clone, Encode, Decode)]
-pub struct BlockAndTime<B>
-where
-	B: BlockNumberTrait,
-{
+pub struct BlockAndTime<B: BlockNumberTrait> {
 	/// The block number that has to be reached in order
 	/// for the lock to be considered stale.
 	pub block_number: B,
@@ -304,13 +301,11 @@ impl<'a, B> BlockAndTimeLock<'a, B> for StorageLock<'a, BlockAndTime<B>>
 where
 	B: BlockNumberTrait,
 {
-	fn with_block_and_time_deadline<'k>(
-		key: &'k [u8],
+	fn with_block_and_time_deadline(
+		key: &'a [u8],
 		block_deadline: B,
 		time_deadline: Timestamp,
 	) -> Self
-	where
-		'k: 'a,
 	{
 		Self {
 			value_ref: StorageValueRef::<'a>::persistent(key),
@@ -323,17 +318,11 @@ where
 }
 
 /// RAII style guard for a lock.
-pub struct StorageLockGuard<'a, 'b, L>
-where
-	L: Lockable,
-{
+pub struct StorageLockGuard<'a, 'b, L: Lockable> {
 	lock: Option<&'b mut StorageLock<'a, L>>,
 }
 
-impl<'a, 'b, L> StorageLockGuard<'a, 'b, L>
-where
-	L: Lockable,
-{
+impl<'a, 'b, L: Lockable> StorageLockGuard<'a, 'b, L> {
 	/// Consume the guard but DO NOT unlock the underlying lock.
 	pub fn forget(mut self) {
 		let _ = self.lock.take();

From debe87dbb62a00f311225b8f7cf8fd477d7c3f57 Mon Sep 17 00:00:00 2001
From: Bernhard Schuster <bernhard@parity.io>
Date: Mon, 18 May 2020 17:27:24 +0200
Subject: [PATCH 18/44] chore/storage: lifetime cleanup

---
 primitives/runtime/src/offchain/storage_lock.rs | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/primitives/runtime/src/offchain/storage_lock.rs b/primitives/runtime/src/offchain/storage_lock.rs
index 82ad8b4fbfe9c..2b6566a6e9c9e 100644
--- a/primitives/runtime/src/offchain/storage_lock.rs
+++ b/primitives/runtime/src/offchain/storage_lock.rs
@@ -38,7 +38,7 @@
 //! # use codec::{Decode, Encode, Codec};
 //! // in your off-chain worker code
 //!
-//! fn append_to_in_storage_vec<'k, T>(key: &'k [u8], _: T) where T: Encode {
+//! fn append_to_in_storage_vec<'a, T>(key: &'a [u8], _: T) where T: Encode {
 //!    // `access::lock` defines the storage entry which is used for
 //!    // persisting the lock in the underlying database.
 //!    // The entry name _must_ be unique and can be seen as mutex instance reference.
@@ -288,13 +288,11 @@ trait BlockAndTimeLock<'a, B>: Sized
 where
 	B: BlockNumberTrait,
 {
-	fn with_block_and_time_deadline<'k>(
-		key: &'k [u8],
+	fn with_block_and_time_deadline(
+		key: &'a [u8],
 		block_deadline: B,
 		time_deadline: Timestamp,
-	) -> Self
-	where
-		'k: 'a;
+	) -> Self;
 }
 
 impl<'a, B> BlockAndTimeLock<'a, B> for StorageLock<'a, BlockAndTime<B>>

From d6a53065ef26488f640ea193b3d883710200baf8 Mon Sep 17 00:00:00 2001
From: Bernhard Schuster <bernhard@parity.io>
Date: Wed, 20 May 2020 16:40:12 +0200
Subject: [PATCH 19/44] fix/cleanup: trait bounds, cargo-spellcheck + extra
 explanations

---
 frame/system/src/lib.rs                       |  21 +++
 .../runtime/src/offchain/storage_lock.rs      | 151 +++++++++++-------
 2 files changed, 113 insertions(+), 59 deletions(-)

diff --git a/frame/system/src/lib.rs b/frame/system/src/lib.rs
index ff6893d6290e8..91d6a10255364 100644
--- a/frame/system/src/lib.rs
+++ b/frame/system/src/lib.rs
@@ -111,6 +111,7 @@ use sp_runtime::{
 		MaybeSerialize, MaybeSerializeDeserialize, MaybeMallocSizeOf, StaticLookup, One, Bounded,
 		Dispatchable, DispatchInfoOf, PostDispatchInfoOf,
 	},
+	offchain::storage_lock::BlockNumberProvider,
 };
 
 use sp_core::{ChangesTrieConfiguration, storage::well_known_keys};
@@ -1260,6 +1261,26 @@ impl<T: Trait> Happened<T::AccountId> for CallKillAccount<T> {
 	}
 }
 
+/// Number of blocks until a `BlockNumberProvider` based
+/// storage lock will expire from the point it was instantiated.
+const BLOCK_NUMBER_EXPIRATION_DELTA : u32 = 4u32;
+
+impl<T> BlockNumberProvider for Module<T>
+where
+	T: crate::Trait,
+	<T as Trait>::BlockNumber: AtLeast32Bit,
+{
+	type BlockNumber = <T as Trait>::BlockNumber;
+
+	fn current_block_number() -> Self::BlockNumber {
+		Module::<T>::block_number()
+	}
+
+	fn deadline_block_number() -> Self::BlockNumber {
+		Self::current_block_number() + Self::BlockNumber::from(BLOCK_NUMBER_EXPIRATION_DELTA)
+	}
+}
+
 // Implement StoredMap for a simple single-item, kill-account-on-remove system. This works fine for
 // storing a single item which is required to not be empty/default for the account to exist.
 // Anything more complex will need more sophisticated logic.
diff --git a/primitives/runtime/src/offchain/storage_lock.rs b/primitives/runtime/src/offchain/storage_lock.rs
index 2b6566a6e9c9e..89cdc9d6db2d3 100644
--- a/primitives/runtime/src/offchain/storage_lock.rs
+++ b/primitives/runtime/src/offchain/storage_lock.rs
@@ -20,8 +20,8 @@
 //!
 //! The lock is using Local Storage and allows synchronizing access to critical
 //! section of your code for concurrently running Off-chain Workers. Usage of
-//! `PERSISTENT` variant of the storage persists the lock value even in case of
-//! re-orgs.
+//! `PERSISTENT` variant of the storage persists the lock value accross a full node
+//! restart or re-orgs.
 //!
 //! A use case for the lock is to make sure that a particular section of the
 //! code is only run by one Off-chain Worker at the time. This may include
@@ -47,7 +47,9 @@
 //!         let _guard = lock.spin_lock();
 //!         let acc = StorageValueRef::persistent(key);
 //!         let v: Vec<T> = acc.get::<Vec<T>>().unwrap().unwrap();
-//!         // modify `v` as desired - i.e. perform some heavy computation or side effects that should only be done once.
+//!         // modify `v` as desired
+//!         // i.e. perform some heavy computation
+//!         // side effects that should only be done once.
 //!         acc.set(v);
 //!         // drop `_guard` implicitly at end of scope
 //!    }
@@ -120,29 +122,55 @@ impl Lockable for Timestamp {
 }
 
 /// Lockable based on the current block number and a timestamp based deadline.
-#[derive(Debug, Copy, Clone, Encode, Decode)]
-pub struct BlockAndTime<B: BlockNumberTrait> {
+#[derive(Encode, Decode)]
+pub struct BlockAndTime<B>
+where
+	B: BlockNumberProvider,
+	<B as BlockNumberProvider>::BlockNumber: Copy,
+{
 	/// The block number that has to be reached in order
 	/// for the lock to be considered stale.
-	pub block_number: B,
+	pub block_number: <B as BlockNumberProvider>::BlockNumber,
 	/// Additional timestamp based deadline which has to be
 	/// reached in order for the lock to become stale.
 	pub timestamp: Timestamp,
 }
 
-impl<B> Lockable for BlockAndTime<B>
+// derive not possible, since `B` does not necessarily implement `trait Clone`.
+impl<B> Clone for BlockAndTime<B>
 where
-	B: BlockNumberTrait,
+	B: BlockNumberProvider,
+	<B as BlockNumberProvider>::BlockNumber: Clone,
+{
+	fn clone(&self) -> Self {
+		Self {
+			block_number: self.block_number,
+			timestamp: self.timestamp,
+		}
+	}
+}
+
+// derive not possible, since `B` does not necessarily implement `trait Copy`.
+impl<B: BlockNumberProvider> Copy for BlockAndTime<B>
+{
+}
+
+impl<B: BlockNumberProvider> Lockable for BlockAndTime<B>
 {
 	fn current() -> Self {
 		Self {
-			block_number: B::current(),
+			block_number: B::current_block_number(),
 			timestamp: offchain::timestamp(),
 		}
 	}
 
 	fn deadline() -> Self {
-		Self::current()
+		Self {
+			block_number: B::deadline_block_number(),
+			timestamp: offchain::timestamp().add(Duration::from_millis(
+				STORAGE_LOCK_DEFAULT_EXPIRY_DURATION_MS,
+			)),
+		}
 	}
 
 	fn expired(&self, reference: &Self) -> bool {
@@ -159,12 +187,12 @@ where
 
 /// Storage based lock.
 ///
-/// A lock that is persisted in the DB and provides a mutex behaviour
+/// A lock that is persisted in the DB and provides a mutex behavior
 /// with a defined safety expirey deadline based on a [`Lockable`](Self::Lockable)
 /// implementation.
 pub struct StorageLock<'a, L>
 where
-	L: Sized + Lockable,
+	L: Lockable,
 {
 	// A storage value ref which defines the DB entry representing the lock.
 	value_ref: StorageValueRef<'a>,
@@ -176,8 +204,7 @@ where
 	L: Lockable,
 {
 	/// Create a new storage lock with [default expiry duration](Self::STORAGE_LOCK_DEFAULT_EXPIRY_DURATION_MS).
-	pub fn new(key: &'a [u8]) -> Self
-	{
+	pub fn new(key: &'a [u8]) -> Self {
 		Self {
 			value_ref: StorageValueRef::<'a>::persistent(key),
 			deadline: L::deadline(),
@@ -245,9 +272,30 @@ where
 	}
 }
 
+/// RAII style guard for a lock.
+pub struct StorageLockGuard<'a, 'b, L: Lockable> {
+	lock: Option<&'b mut StorageLock<'a, L>>,
+}
+
+impl<'a, 'b, L: Lockable> StorageLockGuard<'a, 'b, L> {
+	/// Consume the guard but DO NOT unlock the underlying lock.
+	pub fn forget(mut self) {
+		let _ = self.lock.take();
+	}
+}
+
+impl<'a, 'b, L: Lockable> Drop for StorageLockGuard<'a, 'b, L>
+{
+	fn drop(&mut self) {
+		if let Some(lock) = self.lock.take() {
+			lock.unlock();
+		}
+	}
+}
+
 /// Extension trait for locks based on [`Timestamp`](::sp_core::offchain::Timestamp).
 ///
-/// Allows explicity setting the timeout on construction
+/// Allows explicitly setting the timeout on construction
 /// instead of using the implicit default timeout of
 /// [`STORAGE_LOCK_DEFAULT_EXPIRY_DURATION_MS`](Self::STORAGE_LOCK_DEFAULT_EXPIRY_DURATION_MS).
 pub trait TimeLock<'a>: Sized {
@@ -265,46 +313,30 @@ impl<'a> TimeLock<'a> for StorageLock<'a, Timestamp> {
 	}
 }
 
-/// Bound for block numbers which commonly will be implemented by the `frame_system::Trait::BlockNumber`.
+/// Extension for storage locks which are based on blocks in addition to a timestamp.
 ///
-/// This trait has no intrinsic meaning and exists only to decouple `frame_system`
-/// from `runtime` crate and avoid a circular dependency.
-pub trait BlockNumberTrait: Codec + Copy + Clone + Ord + Eq {
-	/// Returns the current block number.
-	///
-	/// Commonly this will be implemented as
-	/// ```ignore
-	/// fn current() -> Self {
-	///     frame_system::Module<Trait>::block_number()
-	/// }
-	/// ```
-	/// but note that the definition of current is
-	/// application specific.
-	fn current() -> Self;
-}
-
-/// Extension for lor storage locks which are based on blocks in addition to a timestamp.
+/// The provider `B` does provide means of obtaining a default deadline as well as obtaining
+/// the current block number. Expiry and others are actually checked.
 trait BlockAndTimeLock<'a, B>: Sized
 where
-	B: BlockNumberTrait,
+	B: BlockNumberProvider,
 {
 	fn with_block_and_time_deadline(
 		key: &'a [u8],
-		block_deadline: B,
+		block_deadline: B::BlockNumber,
 		time_deadline: Timestamp,
 	) -> Self;
 }
 
 impl<'a, B> BlockAndTimeLock<'a, B> for StorageLock<'a, BlockAndTime<B>>
 where
-	B: BlockNumberTrait,
+	B: BlockNumberProvider,
 {
 	fn with_block_and_time_deadline(
 		key: &'a [u8],
-		block_deadline: B,
+		block_deadline: B::BlockNumber,
 		time_deadline: Timestamp,
-	) -> Self
-	{
+	) -> Self {
 		Self {
 			value_ref: StorageValueRef::<'a>::persistent(key),
 			deadline: BlockAndTime {
@@ -315,27 +347,28 @@ where
 	}
 }
 
-/// RAII style guard for a lock.
-pub struct StorageLockGuard<'a, 'b, L: Lockable> {
-	lock: Option<&'b mut StorageLock<'a, L>>,
-}
-
-impl<'a, 'b, L: Lockable> StorageLockGuard<'a, 'b, L> {
-	/// Consume the guard but DO NOT unlock the underlying lock.
-	pub fn forget(mut self) {
-		let _ = self.lock.take();
-	}
-}
+/// Bound for block numbers which commonly will be implemented by the `frame_system::Trait::BlockNumber`.
+///
+/// This trait has no intrinsic meaning and exists only to decouple `frame_system`
+/// from `runtime` crate and avoid a circular dependency.
+pub trait BlockNumberProvider {
+	/// Type of `BlockNumber` the provider is going to provide
+	/// with `deadline()` and `current()`.
+	type BlockNumber: Codec + Copy + Ord + Eq;
+	/// Returns the current block number.
+	///
+	/// Commonly this will be implemented as
+	/// ```ignore
+	/// fn current() -> Self {
+	///     frame_system::Module<Trait>::block_number()
+	/// }
+	/// ```
+	/// but note that the definition of current is
+	/// application specific.
+	fn current_block_number() -> Self::BlockNumber;
 
-impl<'a, 'b, L> Drop for StorageLockGuard<'a, 'b, L>
-where
-	L: Lockable,
-{
-	fn drop(&mut self) {
-		if let Some(lock) = self.lock.take() {
-			lock.unlock();
-		}
-	}
+	/// Provide a default deadline as `BlockNumber`.
+	fn deadline_block_number() -> Self::BlockNumber;
 }
 
 #[cfg(test)]

From 50e5ef219353b56a8af0d70a262677ec98a6320a Mon Sep 17 00:00:00 2001
From: Bernhard Schuster <bernhard@parity.io>
Date: Wed, 20 May 2020 17:44:43 +0200
Subject: [PATCH 20/44] fix/doc: periods +-

---
 primitives/runtime/src/offchain/storage_lock.rs | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/primitives/runtime/src/offchain/storage_lock.rs b/primitives/runtime/src/offchain/storage_lock.rs
index 89cdc9d6db2d3..282794edaa357 100644
--- a/primitives/runtime/src/offchain/storage_lock.rs
+++ b/primitives/runtime/src/offchain/storage_lock.rs
@@ -77,10 +77,10 @@ pub trait Lockable: Sized + Codec + Copy {
 	/// Get the current value of lockable.
 	fn current() -> Self;
 
-	/// Acquire a new deadline based on `Self::current()`
+	/// Acquire a new deadline based on `Self::current()`.
 	fn deadline() -> Self;
 
-	/// Verify the current value of `self` against `deadline`
+	/// Verify the current value of `self` against `deadline`.
 	/// to determine if the lock has expired.
 	fn expired(&self, deadline: &Self) -> bool;
 
@@ -136,7 +136,7 @@ where
 	pub timestamp: Timestamp,
 }
 
-// derive not possible, since `B` does not necessarily implement `trait Clone`.
+// derive not possible, since `B` does not necessarily implement `trait Clone`
 impl<B> Clone for BlockAndTime<B>
 where
 	B: BlockNumberProvider,
@@ -150,7 +150,7 @@ where
 	}
 }
 
-// derive not possible, since `B` does not necessarily implement `trait Copy`.
+// derive not possible, since `B` does not necessarily implement `trait Copy`
 impl<B: BlockNumberProvider> Copy for BlockAndTime<B>
 {
 }

From 0bc2047e5ec7e450c48e9b1cdc1e4531534baa23 Mon Sep 17 00:00:00 2001
From: Bernhard Schuster <bernhard@ahoi.io>
Date: Fri, 22 May 2020 08:12:47 +0200
Subject: [PATCH 21/44] fix/review: Apply suggestions from code review
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com>
---
 .../runtime/src/offchain/storage_lock.rs      | 28 +++++--------------
 1 file changed, 7 insertions(+), 21 deletions(-)

diff --git a/primitives/runtime/src/offchain/storage_lock.rs b/primitives/runtime/src/offchain/storage_lock.rs
index 282794edaa357..fa9a0d364772b 100644
--- a/primitives/runtime/src/offchain/storage_lock.rs
+++ b/primitives/runtime/src/offchain/storage_lock.rs
@@ -61,7 +61,6 @@ use codec::{Codec, Decode, Encode};
 use sp_core::offchain::{Duration, Timestamp};
 use sp_io::offchain;
 
-//use frame_system how?
 
 /// Default expiry duration in milliseconds.
 const STORAGE_LOCK_DEFAULT_EXPIRY_DURATION_MS: u64 = 30_000;
@@ -123,11 +122,7 @@ impl Lockable for Timestamp {
 
 /// Lockable based on the current block number and a timestamp based deadline.
 #[derive(Encode, Decode)]
-pub struct BlockAndTime<B>
-where
-	B: BlockNumberProvider,
-	<B as BlockNumberProvider>::BlockNumber: Copy,
-{
+pub struct BlockAndTime<B: BlockNumberProvider> {
 	/// The block number that has to be reached in order
 	/// for the lock to be considered stale.
 	pub block_number: <B as BlockNumberProvider>::BlockNumber,
@@ -137,14 +132,10 @@ where
 }
 
 // derive not possible, since `B` does not necessarily implement `trait Clone`
-impl<B> Clone for BlockAndTime<B>
-where
-	B: BlockNumberProvider,
-	<B as BlockNumberProvider>::BlockNumber: Clone,
-{
+impl<B: BlockNumberProvider> Clone for BlockAndTime<B> {
 	fn clone(&self) -> Self {
 		Self {
-			block_number: self.block_number,
+			block_number: self.block_number.clone(),
 			timestamp: self.timestamp,
 		}
 	}
@@ -188,12 +179,9 @@ impl<B: BlockNumberProvider> Lockable for BlockAndTime<B>
 /// Storage based lock.
 ///
 /// A lock that is persisted in the DB and provides a mutex behavior
-/// with a defined safety expirey deadline based on a [`Lockable`](Self::Lockable)
+/// with a defined safety expiry deadline based on a [`Lockable`](Self::Lockable)
 /// implementation.
-pub struct StorageLock<'a, L>
-where
-	L: Lockable,
-{
+pub struct StorageLock<'a, L> {
 	// A storage value ref which defines the DB entry representing the lock.
 	value_ref: StorageValueRef<'a>,
 	deadline: L,
@@ -242,10 +230,8 @@ where
 	where
 		'a: 'b,
 	{
-		match self.try_lock_inner(self.deadline) {
-			Ok(_) => Ok(StorageLockGuard::<'a, 'b> { lock: Some(self) }),
-			Err(e) => Err(e),
-		}
+	let _ = self.try_lock_inner(self.deadline)?;
+	Ok(StorageLockGuard::<'a, 'b> { lock: Some(self) })
 	}
 
 	/// Try grabbing the lock until its expiry is reached.

From cc859333ebfa1a1ead9297101098c9f8b4f998c4 Mon Sep 17 00:00:00 2001
From: Bernhard Schuster <bernhard@parity.io>
Date: Fri, 22 May 2020 13:10:37 +0200
Subject: [PATCH 22/44] cleanup: remove explicit lifetime bound, copy -> clone

---
 .../runtime/src/offchain/storage_lock.rs      | 20 +++++++------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/primitives/runtime/src/offchain/storage_lock.rs b/primitives/runtime/src/offchain/storage_lock.rs
index fa9a0d364772b..83fe028c6b2b0 100644
--- a/primitives/runtime/src/offchain/storage_lock.rs
+++ b/primitives/runtime/src/offchain/storage_lock.rs
@@ -72,7 +72,7 @@ const STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE: u64 = 100;
 ///
 /// Bound for an item that has a stateful ordered meaning
 /// without explicitly requiring `Ord` trait in general.
-pub trait Lockable: Sized + Codec + Copy {
+pub trait Lockable: Sized + Codec + Clone {
 	/// Get the current value of lockable.
 	fn current() -> Self;
 
@@ -141,10 +141,6 @@ impl<B: BlockNumberProvider> Clone for BlockAndTime<B> {
 	}
 }
 
-// derive not possible, since `B` does not necessarily implement `trait Copy`
-impl<B: BlockNumberProvider> Copy for BlockAndTime<B>
-{
-}
 
 impl<B: BlockNumberProvider> Lockable for BlockAndTime<B>
 {
@@ -230,21 +226,19 @@ where
 	where
 		'a: 'b,
 	{
-	let _ = self.try_lock_inner(self.deadline)?;
+	let _ = self.try_lock_inner(self.deadline.clone())?;
 	Ok(StorageLockGuard::<'a, 'b> { lock: Some(self) })
 	}
 
 	/// Try grabbing the lock until its expiry is reached.
 	///
 	/// Returns an error if the lock expired before it could be caught.
-	pub fn spin_lock<'b>(&'b mut self) -> StorageLockGuard<'a, 'b, L>
-	where
-		'a: 'b,
+	pub fn spin_lock(&mut self) -> StorageLockGuard<'a, '_, L>
 	{
 		loop {
 			// blind attempt on locking
-			let deadline = match self.try_lock_inner(self.deadline) {
-				Ok(_) => return StorageLockGuard::<'a, 'b, L> { lock: Some(self) },
+			let deadline = match self.try_lock_inner(self.deadline.clone()) {
+				Ok(_) => return StorageLockGuard::<'a, '_, L> { lock: Some(self) },
 				Err(Some(other_locks_deadline)) => other_locks_deadline,
 				_ => L::deadline(), // use the default
 			};
@@ -253,7 +247,7 @@ where
 	}
 
 	/// Explicitly unlock the lock.
-	pub fn unlock(&mut self) {
+	fn unlock(&mut self) {
 		self.value_ref.clear();
 	}
 }
@@ -340,7 +334,7 @@ where
 pub trait BlockNumberProvider {
 	/// Type of `BlockNumber` the provider is going to provide
 	/// with `deadline()` and `current()`.
-	type BlockNumber: Codec + Copy + Ord + Eq;
+	type BlockNumber: Codec + Clone + Ord + Eq;
 	/// Returns the current block number.
 	///
 	/// Commonly this will be implemented as

From 8e14a1290402f74a7849ce959b54425ff7a6be39 Mon Sep 17 00:00:00 2001
From: Bernhard Schuster <bernhard@parity.io>
Date: Fri, 22 May 2020 14:01:14 +0200
Subject: [PATCH 23/44] fix/review: make trait Lockable contain only static,
 try_lock should not return Err(Option<L>),

---
 .../runtime/src/offchain/storage_lock.rs      | 44 +++++++++----------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/primitives/runtime/src/offchain/storage_lock.rs b/primitives/runtime/src/offchain/storage_lock.rs
index 83fe028c6b2b0..aafc8e087d2ac 100644
--- a/primitives/runtime/src/offchain/storage_lock.rs
+++ b/primitives/runtime/src/offchain/storage_lock.rs
@@ -81,14 +81,14 @@ pub trait Lockable: Sized + Codec + Clone {
 
 	/// Verify the current value of `self` against `deadline`.
 	/// to determine if the lock has expired.
-	fn expired(&self, deadline: &Self) -> bool;
+	fn expired(deadline: &Self) -> bool;
 
 	/// Snooze the thread for time determined by `self` and `other`.
 	///
 	/// Only called if not expired just yet.
 	/// Note that the deadline is only passed to allow some optimizations
 	/// for some `L` types.
-	fn snooze(&self, _deadline: &Self) {
+	fn snooze(_deadline: &Self) {
 		sp_io::offchain::sleep_until(offchain::timestamp().add(Duration::from_millis(
 			STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE,
 		)));
@@ -106,17 +106,18 @@ impl Lockable for Timestamp {
 		))
 	}
 
-	fn expired(&self, reference: &Self) -> bool {
-		<Self as Lockable>::current() > *reference
+	fn expired(deadline: &Self) -> bool {
+		<Self as Lockable>::current() > *deadline
 	}
 
-	fn snooze(&self, deadline: &Self) {
-		let remainder: Duration = self.diff(&deadline);
+	fn snooze(deadline: &Self) {
+		let now = Self::current();
+		let remainder: Duration = now.diff(&deadline);
 		// do not snooze the full duration, but instead snooze max 100ms
 		// it might get unlocked in another thread
 		// consider adding some additive jitter here
 		let snooze = core::cmp::min(remainder.millis(), STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE);
-		sp_io::offchain::sleep_until(self.add(Duration::from_millis(snooze)));
+		sp_io::offchain::sleep_until(now.add(Duration::from_millis(snooze)));
 	}
 }
 
@@ -160,15 +161,16 @@ impl<B: BlockNumberProvider> Lockable for BlockAndTime<B>
 		}
 	}
 
-	fn expired(&self, reference: &Self) -> bool {
+	fn expired(reference: &Self) -> bool {
 		let current = <Self as Lockable>::current();
 		current.timestamp > reference.timestamp && current.block_number > reference.block_number
 	}
 
-	fn snooze(&self, deadline: &Self) {
-		let remainder: Duration = self.timestamp.diff(&(deadline.timestamp));
+	fn snooze(deadline: &Self) {
+		let timestamp = Self::current().timestamp;
+		let remainder: Duration = timestamp.diff(&(deadline.timestamp));
 		let snooze = core::cmp::min(remainder.millis(), STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE);
-		sp_io::offchain::sleep_until(self.timestamp.add(Duration::from_millis(snooze)));
+		sp_io::offchain::sleep_until(timestamp.add(Duration::from_millis(snooze)));
 	}
 }
 
@@ -183,9 +185,7 @@ pub struct StorageLock<'a, L> {
 	deadline: L,
 }
 
-impl<'a, L> StorageLock<'a, L>
-where
-	L: Lockable,
+impl<'a, L: Lockable> StorageLock<'a, L>
 {
 	/// Create a new storage lock with [default expiry duration](Self::STORAGE_LOCK_DEFAULT_EXPIRY_DURATION_MS).
 	pub fn new(key: &'a [u8]) -> Self {
@@ -196,18 +196,18 @@ where
 	}
 
 	fn try_lock_inner(&mut self, new_deadline: L) -> Result<(), Option<L>> {
-		let now = L::current();
 		let res = self
 			.value_ref
 			.mutate(|s: Option<Option<L>>| -> Result<L, Option<L>> {
 				match s {
 					// no lock set, we can safely acquire it
 					None => Ok(new_deadline),
+					// write was good, bur read failed
+					Some(None) => Ok(new_deadline),
 					// lock is set, but it's old. We can re-acquire it.
-					Some(Some(deadline)) if now.expired(&deadline) => Ok(new_deadline),
+					Some(Some(deadline)) if <L as Lockable>::expired(&deadline) => Ok(new_deadline),
 					// lock is present and is still active
 					Some(Some(deadline)) => Err(Some(deadline)),
-					_ => Err(None),
 				}
 			});
 		match res {
@@ -221,13 +221,13 @@ where
 	///
 	/// Returns a lock guard on success, otherwise an error containing `None` in
 	/// case the mutex was already unlocked before, or if the lock is still held
-	/// by another process `Err(Some(expiration_timestamp))`.
-	pub fn try_lock<'b>(&'b mut self) -> Result<StorageLockGuard<'a, 'b, L>, Option<L>>
+	/// by another process `Err(())`.
+	pub fn try_lock<'b>(&'b mut self) -> Result<StorageLockGuard<'a, 'b, L>, ()>
 	where
 		'a: 'b,
 	{
-	let _ = self.try_lock_inner(self.deadline.clone())?;
-	Ok(StorageLockGuard::<'a, 'b> { lock: Some(self) })
+		let _ = self.try_lock_inner(self.deadline.clone()).map_err(|_opt| { () })?;
+		Ok(StorageLockGuard::<'a, 'b> { lock: Some(self) })
 	}
 
 	/// Try grabbing the lock until its expiry is reached.
@@ -242,7 +242,7 @@ where
 				Err(Some(other_locks_deadline)) => other_locks_deadline,
 				_ => L::deadline(), // use the default
 			};
-			L::current().snooze(&deadline);
+			L::snooze(&deadline);
 		}
 	}
 

From 828e5b9ccabdb9270613a89f043a93a265eb7c0a Mon Sep 17 00:00:00 2001
From: Bernhard Schuster <bernhard@parity.io>
Date: Fri, 22 May 2020 14:11:48 +0200
Subject: [PATCH 24/44] chore/lifetimes: remove a couple of lifetime bounds
 which the compiler can figure out

---
 primitives/runtime/src/offchain/storage_lock.rs | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/primitives/runtime/src/offchain/storage_lock.rs b/primitives/runtime/src/offchain/storage_lock.rs
index aafc8e087d2ac..ce2bcfdfd8442 100644
--- a/primitives/runtime/src/offchain/storage_lock.rs
+++ b/primitives/runtime/src/offchain/storage_lock.rs
@@ -222,12 +222,10 @@ impl<'a, L: Lockable> StorageLock<'a, L>
 	/// Returns a lock guard on success, otherwise an error containing `None` in
 	/// case the mutex was already unlocked before, or if the lock is still held
 	/// by another process `Err(())`.
-	pub fn try_lock<'b>(&'b mut self) -> Result<StorageLockGuard<'a, 'b, L>, ()>
-	where
-		'a: 'b,
+	pub fn try_lock(&'_ mut self) -> Result<StorageLockGuard<'a, '_, L>, ()>
 	{
 		let _ = self.try_lock_inner(self.deadline.clone()).map_err(|_opt| { () })?;
-		Ok(StorageLockGuard::<'a, 'b> { lock: Some(self) })
+		Ok(StorageLockGuard::<'a, '_> { lock: Some(self) })
 	}
 
 	/// Try grabbing the lock until its expiry is reached.

From 2a9919151cd56cdd93f036f4c0331032913a3490 Mon Sep 17 00:00:00 2001
From: Bernhard Schuster <bernhard@parity.io>
Date: Fri, 22 May 2020 17:55:02 +0200
Subject: [PATCH 25/44] refactor: migrate to an instant based

---
 frame/system/src/lib.rs                       |   8 -
 .../runtime/src/offchain/storage_lock.rs      | 253 +++++++++++-------
 2 files changed, 149 insertions(+), 112 deletions(-)

diff --git a/frame/system/src/lib.rs b/frame/system/src/lib.rs
index 0231d71464a51..bb062dd6d2657 100644
--- a/frame/system/src/lib.rs
+++ b/frame/system/src/lib.rs
@@ -1271,10 +1271,6 @@ impl<T: Trait> Happened<T::AccountId> for CallKillAccount<T> {
 	}
 }
 
-/// Number of blocks until a `BlockNumberProvider` based
-/// storage lock will expire from the point it was instantiated.
-const BLOCK_NUMBER_EXPIRATION_DELTA : u32 = 4u32;
-
 impl<T> BlockNumberProvider for Module<T>
 where
 	T: crate::Trait,
@@ -1285,10 +1281,6 @@ where
 	fn current_block_number() -> Self::BlockNumber {
 		Module::<T>::block_number()
 	}
-
-	fn deadline_block_number() -> Self::BlockNumber {
-		Self::current_block_number() + Self::BlockNumber::from(BLOCK_NUMBER_EXPIRATION_DELTA)
-	}
 }
 
 // Implement StoredMap for a simple single-item, kill-account-on-remove system. This works fine for
diff --git a/primitives/runtime/src/offchain/storage_lock.rs b/primitives/runtime/src/offchain/storage_lock.rs
index ce2bcfdfd8442..653f18d437516 100644
--- a/primitives/runtime/src/offchain/storage_lock.rs
+++ b/primitives/runtime/src/offchain/storage_lock.rs
@@ -44,7 +44,7 @@
 //!    // The entry name _must_ be unique and can be seen as mutex instance reference.
 //!    let mut lock = StorageLock::new(b"access::lock");
 //!    {
-//!         let _guard = lock.spin_lock();
+//!         let _guard = lock.lock();
 //!         let acc = StorageValueRef::persistent(key);
 //!         let v: Vec<T> = acc.get::<Vec<T>>().unwrap().unwrap();
 //!         // modify `v` as desired
@@ -57,11 +57,11 @@
 //! ```
 
 use crate::offchain::storage::StorageValueRef;
+use crate::traits::AtLeast32Bit;
 use codec::{Codec, Decode, Encode};
 use sp_core::offchain::{Duration, Timestamp};
 use sp_io::offchain;
 
-
 /// Default expiry duration in milliseconds.
 const STORAGE_LOCK_DEFAULT_EXPIRY_DURATION_MS: u64 = 30_000;
 
@@ -72,45 +72,67 @@ const STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE: u64 = 100;
 ///
 /// Bound for an item that has a stateful ordered meaning
 /// without explicitly requiring `Ord` trait in general.
-pub trait Lockable: Sized + Codec + Clone {
+pub trait Lockable: Sized {
+	/// The instant type
+	type Instant: Sized + Codec + Clone;
+
 	/// Get the current value of lockable.
-	fn current() -> Self;
+	fn current() -> Self::Instant;
 
 	/// Acquire a new deadline based on `Self::current()`.
-	fn deadline() -> Self;
+	fn deadline(&self) -> Self::Instant;
 
 	/// Verify the current value of `self` against `deadline`.
 	/// to determine if the lock has expired.
-	fn expired(deadline: &Self) -> bool;
+	fn expired(deadline: &Self::Instant) -> bool;
 
 	/// Snooze the thread for time determined by `self` and `other`.
 	///
 	/// Only called if not expired just yet.
 	/// Note that the deadline is only passed to allow some optimizations
 	/// for some `L` types.
-	fn snooze(_deadline: &Self) {
+	fn snooze(_deadline: &Self::Instant) {
 		sp_io::offchain::sleep_until(offchain::timestamp().add(Duration::from_millis(
 			STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE,
 		)));
 	}
 }
 
-impl Lockable for Timestamp {
-	fn current() -> Self {
+/// Lockable based on the current timestamp with a configurable expiration time.
+#[derive(Encode, Decode)]
+pub struct Time {
+	/// Time of calling `fn lock(..)`.
+	timestamp: Timestamp,
+	/// How long the lock will stay valid once `fn lock(..)` is called.
+	expiration_duration: Duration,
+}
+
+impl Default for Time {
+	fn default() -> Self {
+		let timestamp = offchain::timestamp();
+		Self {
+			timestamp,
+			expiration_duration: Duration::from_millis(STORAGE_LOCK_DEFAULT_EXPIRY_DURATION_MS),
+		}
+	}
+}
+
+impl Lockable for Time {
+	type Instant = Timestamp;
+
+	fn current() -> Self::Instant {
 		offchain::timestamp()
 	}
 
-	fn deadline() -> Self {
-		Self::current().add(Duration::from_millis(
-			STORAGE_LOCK_DEFAULT_EXPIRY_DURATION_MS,
-		))
+	fn deadline(&self) -> Self::Instant {
+		self.timestamp.add(self.expiration_duration)
 	}
 
-	fn expired(deadline: &Self) -> bool {
+	fn expired(deadline: &Self::Instant) -> bool {
 		<Self as Lockable>::current() > *deadline
 	}
 
-	fn snooze(deadline: &Self) {
+	fn snooze(deadline: &Self::Instant) {
 		let now = Self::current();
 		let remainder: Duration = now.diff(&deadline);
 		// do not snooze the full duration, but instead snooze max 100ms
@@ -121,52 +143,85 @@ impl Lockable for Timestamp {
 	}
 }
 
-/// Lockable based on the current block number and a timestamp based deadline.
-#[derive(Encode, Decode)]
-pub struct BlockAndTime<B: BlockNumberProvider> {
-	/// The block number that has to be reached in order
-	/// for the lock to be considered stale.
+/// An instant based on block and time.
+#[derive(Encode, Decode, Eq, PartialEq)]
+pub struct BlockAndTimeInstant<B: BlockNumberProvider> {
 	pub block_number: <B as BlockNumberProvider>::BlockNumber,
-	/// Additional timestamp based deadline which has to be
-	/// reached in order for the lock to become stale.
 	pub timestamp: Timestamp,
 }
 
-// derive not possible, since `B` does not necessarily implement `trait Clone`
-impl<B: BlockNumberProvider> Clone for BlockAndTime<B> {
+impl<B: BlockNumberProvider> Clone for BlockAndTimeInstant<B> {
 	fn clone(&self) -> Self {
 		Self {
 			block_number: self.block_number.clone(),
-			timestamp: self.timestamp,
+			timestamp: self.timestamp.clone(),
 		}
 	}
 }
 
-
-impl<B: BlockNumberProvider> Lockable for BlockAndTime<B>
-{
+impl<B: BlockNumberProvider> BlockAndTimeInstant<B> {
+	/// Provide the current state of block number and time.
 	fn current() -> Self {
 		Self {
 			block_number: B::current_block_number(),
 			timestamp: offchain::timestamp(),
 		}
 	}
+}
+
+/// Lockable based on the current block number and a timestamp based deadline.
+pub struct BlockAndTime<B: BlockNumberProvider> {
+	/// The instant when calling `fn lock(..)`.
+	lock_instant: BlockAndTimeInstant<B>,
+	/// The block number offset from the time of locking
+	/// when the lock is considered stale.
+	expiration_block_number_offset: u32,
+	/// Additional timestamp based deadline, which, once
+	/// reached, renders the lock stale.
+	expiration_duration: Duration,
+}
 
-	fn deadline() -> Self {
+impl<B: BlockNumberProvider> Default for BlockAndTime<B> {
+	fn default() -> Self {
 		Self {
-			block_number: B::deadline_block_number(),
-			timestamp: offchain::timestamp().add(Duration::from_millis(
-				STORAGE_LOCK_DEFAULT_EXPIRY_DURATION_MS,
-			)),
+			lock_instant: BlockAndTimeInstant::current(),
+			expiration_block_number_offset: 3u32,
+			expiration_duration: Duration::from_millis(STORAGE_LOCK_DEFAULT_EXPIRY_DURATION_MS),
 		}
 	}
+}
 
-	fn expired(reference: &Self) -> bool {
+// derive not possible, since `B` does not necessarily implement `trait Clone`
+impl<B: BlockNumberProvider> Clone for BlockAndTime<B> {
+	fn clone(&self) -> Self {
+		Self {
+			lock_instant: self.lock_instant.clone(),
+			expiration_block_number_offset: self.expiration_block_number_offset.clone(),
+			expiration_duration: self.expiration_duration,
+		}
+	}
+}
+
+impl<B: BlockNumberProvider> Lockable for BlockAndTime<B> {
+	type Instant = BlockAndTimeInstant<B>;
+
+	fn current() -> Self::Instant {
+		Self::Instant::current()
+	}
+
+	fn deadline(&self) -> Self::Instant {
+		let mut current = Self::current();
+		current.block_number += self.expiration_block_number_offset.into();
+		current.timestamp.add(self.expiration_duration);
+		current
+	}
+
+	fn expired(deadline: &Self::Instant) -> bool {
 		let current = <Self as Lockable>::current();
-		current.timestamp > reference.timestamp && current.block_number > reference.block_number
+		current.timestamp > deadline.timestamp && current.block_number > deadline.block_number
 	}
 
-	fn snooze(deadline: &Self) {
+	fn snooze(deadline: &Self::Instant) {
 		let timestamp = Self::current().timestamp;
 		let remainder: Duration = timestamp.diff(&(deadline.timestamp));
 		let snooze = core::cmp::min(remainder.millis(), STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE);
@@ -179,26 +234,32 @@ impl<B: BlockNumberProvider> Lockable for BlockAndTime<B>
 /// A lock that is persisted in the DB and provides a mutex behavior
 /// with a defined safety expiry deadline based on a [`Lockable`](Self::Lockable)
 /// implementation.
-pub struct StorageLock<'a, L> {
+pub struct StorageLock<'a, L = Time> {
 	// A storage value ref which defines the DB entry representing the lock.
 	value_ref: StorageValueRef<'a>,
-	deadline: L,
+	lockable: L,
 }
 
-impl<'a, L: Lockable> StorageLock<'a, L>
-{
-	/// Create a new storage lock with [default expiry duration](Self::STORAGE_LOCK_DEFAULT_EXPIRY_DURATION_MS).
+impl<'a, L: Lockable + Default> StorageLock<'a, L> {
+	/// Create a new storage lock with a `default()` instance of type `L`.
 	pub fn new(key: &'a [u8]) -> Self {
+		Self::with_lockable(key, Default::default())
+	}
+}
+
+impl<'a, L: Lockable> StorageLock<'a, L> {
+	/// Create a new storage lock with an explicit instance of a lockable `L`.
+	pub fn with_lockable(key: &'a [u8], lockable: L) -> Self {
 		Self {
 			value_ref: StorageValueRef::<'a>::persistent(key),
-			deadline: L::deadline(),
+			lockable,
 		}
 	}
 
-	fn try_lock_inner(&mut self, new_deadline: L) -> Result<(), Option<L>> {
-		let res = self
-			.value_ref
-			.mutate(|s: Option<Option<L>>| -> Result<L, Option<L>> {
+	/// Internal lock helper to avoid lifetime conflicts.
+	fn try_lock_inner(&mut self, new_deadline: L::Instant) -> Result<(), Option<L::Instant>> {
+		let res = self.value_ref.mutate(
+			|s: Option<Option<L::Instant>>| -> Result<L::Instant, Option<L::Instant>> {
 				match s {
 					// no lock set, we can safely acquire it
 					None => Ok(new_deadline),
@@ -209,7 +270,8 @@ impl<'a, L: Lockable> StorageLock<'a, L>
 					// lock is present and is still active
 					Some(Some(deadline)) => Err(Some(deadline)),
 				}
-			});
+			},
+		);
 		match res {
 			Ok(Ok(_)) => Ok(()),
 			Ok(Err(_deadline)) => Err(None),
@@ -222,23 +284,23 @@ impl<'a, L: Lockable> StorageLock<'a, L>
 	/// Returns a lock guard on success, otherwise an error containing `None` in
 	/// case the mutex was already unlocked before, or if the lock is still held
 	/// by another process `Err(())`.
-	pub fn try_lock(&'_ mut self) -> Result<StorageLockGuard<'a, '_, L>, ()>
-	{
-		let _ = self.try_lock_inner(self.deadline.clone()).map_err(|_opt| { () })?;
+	pub fn try_lock(&mut self) -> Result<StorageLockGuard<'a, '_, L>, ()> {
+		let _ = self
+			.try_lock_inner(self.lockable.deadline())
+			.map_err(|_opt| ())?;
 		Ok(StorageLockGuard::<'a, '_> { lock: Some(self) })
 	}
 
 	/// Try grabbing the lock until its expiry is reached.
 	///
 	/// Returns an error if the lock expired before it could be caught.
-	pub fn spin_lock(&mut self) -> StorageLockGuard<'a, '_, L>
-	{
+	pub fn lock(&mut self) -> StorageLockGuard<'a, '_, L> {
 		loop {
 			// blind attempt on locking
-			let deadline = match self.try_lock_inner(self.deadline.clone()) {
+			let deadline = match self.try_lock_inner(self.lockable.deadline()) {
 				Ok(_) => return StorageLockGuard::<'a, '_, L> { lock: Some(self) },
 				Err(Some(other_locks_deadline)) => other_locks_deadline,
-				_ => L::deadline(), // use the default
+				_ => self.lockable.deadline(), // use the default
 			};
 			L::snooze(&deadline);
 		}
@@ -257,13 +319,18 @@ pub struct StorageLockGuard<'a, 'b, L: Lockable> {
 
 impl<'a, 'b, L: Lockable> StorageLockGuard<'a, 'b, L> {
 	/// Consume the guard but DO NOT unlock the underlying lock.
+	///
+	/// Can be used to implement a grace period after doing some
+	/// heavy computations and sending a transaction to be included
+	/// on-chain. By forgetting the lock, it will stay locked until
+	/// its expiration deadline is reached while the off-chain worker
+	/// can already complete.
 	pub fn forget(mut self) {
 		let _ = self.lock.take();
 	}
 }
 
-impl<'a, 'b, L: Lockable> Drop for StorageLockGuard<'a, 'b, L>
-{
+impl<'a, 'b, L: Lockable> Drop for StorageLockGuard<'a, 'b, L> {
 	fn drop(&mut self) {
 		if let Some(lock) = self.lock.take() {
 			lock.unlock();
@@ -271,55 +338,36 @@ impl<'a, 'b, L: Lockable> Drop for StorageLockGuard<'a, 'b, L>
 	}
 }
 
-/// Extension trait for locks based on [`Timestamp`](::sp_core::offchain::Timestamp).
-///
 /// Allows explicitly setting the timeout on construction
 /// instead of using the implicit default timeout of
 /// [`STORAGE_LOCK_DEFAULT_EXPIRY_DURATION_MS`](Self::STORAGE_LOCK_DEFAULT_EXPIRY_DURATION_MS).
-pub trait TimeLock<'a>: Sized {
-	/// Provide an explicit deadline timestamp at which the locked state of the lock
-	/// becomes stale and may be dismissed by `fn try_lock(..)`, `fn spin_lock(..)` and others.
-	fn with_deadline(key: &'a [u8], lock_deadline: Timestamp) -> Self;
-}
-
-impl<'a> TimeLock<'a> for StorageLock<'a, Timestamp> {
-	fn with_deadline(key: &'a [u8], lock_deadline: Timestamp) -> Self {
+impl<'a> StorageLock<'a, Time> {
+	pub fn with_deadline(key: &'a [u8], expiration_duration: Duration) -> Self {
 		Self {
 			value_ref: StorageValueRef::<'a>::persistent(key),
-			deadline: lock_deadline,
+			lockable: Time {
+				timestamp: offchain::timestamp(),
+				expiration_duration: expiration_duration,
+			},
 		}
 	}
 }
 
-/// Extension for storage locks which are based on blocks in addition to a timestamp.
-///
-/// The provider `B` does provide means of obtaining a default deadline as well as obtaining
-/// the current block number. Expiry and others are actually checked.
-trait BlockAndTimeLock<'a, B>: Sized
-where
-	B: BlockNumberProvider,
-{
-	fn with_block_and_time_deadline(
-		key: &'a [u8],
-		block_deadline: B::BlockNumber,
-		time_deadline: Timestamp,
-	) -> Self;
-}
-
-impl<'a, B> BlockAndTimeLock<'a, B> for StorageLock<'a, BlockAndTime<B>>
+impl<'a, B> StorageLock<'a, BlockAndTime<B>>
 where
 	B: BlockNumberProvider,
 {
-	fn with_block_and_time_deadline(
+	pub fn with_block_and_time_deadline(
 		key: &'a [u8],
-		block_deadline: B::BlockNumber,
-		time_deadline: Timestamp,
+		expiration_block_number_offset: u32,
+		expiration_duration: Duration,
 	) -> Self {
 		Self {
 			value_ref: StorageValueRef::<'a>::persistent(key),
-			deadline: BlockAndTime {
-				block_number: block_deadline,
-				timestamp: time_deadline,
+			lockable: BlockAndTime::<B> {
+				lock_instant: BlockAndTimeInstant::<B>::current(),
+				expiration_block_number_offset,
+				expiration_duration,
 			},
 		}
 	}
@@ -332,21 +380,18 @@ where
 pub trait BlockNumberProvider {
 	/// Type of `BlockNumber` the provider is going to provide
 	/// with `deadline()` and `current()`.
-	type BlockNumber: Codec + Clone + Ord + Eq;
+	type BlockNumber: Codec + Clone + Ord + Eq + AtLeast32Bit;
 	/// Returns the current block number.
 	///
 	/// Commonly this will be implemented as
 	/// ```ignore
-	/// fn current() -> Self {
+	/// fn current_block_number() -> Self {
 	///     frame_system::Module<Trait>::block_number()
 	/// }
 	/// ```
 	/// but note that the definition of current is
 	/// application specific.
 	fn current_block_number() -> Self::BlockNumber;
-
-	/// Provide a default deadline as `BlockNumber`.
-	fn deadline_block_number() -> Self::BlockNumber;
 }
 
 #[cfg(test)]
@@ -365,12 +410,12 @@ mod tests {
 		t.register_extension(OffchainExt::new(offchain));
 
 		t.execute_with(|| {
-			let mut lock = StorageLock::<'_, Timestamp>::new(b"lock_1");
+			let mut lock = StorageLock::<'_, Time>::new(b"lock_1");
 
 			let val = StorageValueRef::persistent(b"protected_value");
 
 			{
-				let _guard = lock.spin_lock();
+				let _guard = lock.lock();
 
 				val.set(&VAL_1);
 
@@ -378,7 +423,7 @@ mod tests {
 			}
 
 			{
-				let _guard = lock.spin_lock();
+				let _guard = lock.lock();
 				val.set(&VAL_2);
 
 				assert_eq!(val.get::<u32>(), Some(Some(VAL_2)));
@@ -395,11 +440,11 @@ mod tests {
 		t.register_extension(OffchainExt::new(offchain));
 
 		t.execute_with(|| {
-			let mut lock = StorageLock::<'_, Timestamp>::new(b"lock_2");
+			let mut lock = StorageLock::<'_, Time>::new(b"lock_2");
 
 			let val = StorageValueRef::persistent(b"protected_value");
 
-			let guard = lock.spin_lock();
+			let guard = lock.lock();
 
 			val.set(&VAL_1);
 
@@ -419,20 +464,20 @@ mod tests {
 		t.register_extension(OffchainExt::new(offchain));
 
 		t.execute_with(|| {
-			let sleep_until = Timestamp::current().add(Duration::from_millis(500));
-			let lock_expiration = Timestamp::current().add(Duration::from_millis(200));
+			let sleep_until = offchain::timestamp().add(Duration::from_millis(500));
+			let lock_expiration = Duration::from_millis(200);
 
-			let mut lock = StorageLock::<'_, Timestamp>::with_deadline(b"lock_3", lock_expiration);
+			let mut lock = StorageLock::<'_, Time>::with_deadline(b"lock_3", lock_expiration);
 
 			{
-				let guard = lock.spin_lock();
+				let guard = lock.lock();
 				guard.forget();
 			}
 
 			// assure the lock expires
 			offchain::sleep_until(sleep_until);
 
-			let mut lock = StorageLock::<'_, Timestamp>::new(b"lock_3");
+			let mut lock = StorageLock::<'_, Time>::new(b"lock_3");
 			let res = lock.try_lock();
 			assert!(res.is_ok());
 			let guard = res.unwrap();

From fb38887946b6dc3eecb608c1fa2f20139d876ffb Mon Sep 17 00:00:00 2001
From: Bernhard Schuster <bernhard@parity.io>
Date: Mon, 25 May 2020 15:18:59 +0200
Subject: [PATCH 26/44] fix/feedback: fix, reduce, rename, docs update pending

---
 .../runtime/src/offchain/storage_lock.rs      | 104 ++++++++----------
 1 file changed, 46 insertions(+), 58 deletions(-)

diff --git a/primitives/runtime/src/offchain/storage_lock.rs b/primitives/runtime/src/offchain/storage_lock.rs
index 653f18d437516..71a6f8d7f98d1 100644
--- a/primitives/runtime/src/offchain/storage_lock.rs
+++ b/primitives/runtime/src/offchain/storage_lock.rs
@@ -41,14 +41,15 @@
 //! fn append_to_in_storage_vec<'a, T>(key: &'a [u8], _: T) where T: Encode {
 //!    // `access::lock` defines the storage entry which is used for
 //!    // persisting the lock in the underlying database.
-//!    // The entry name _must_ be unique and can be seen as mutex instance reference.
-//!    let mut lock = StorageLock::new(b"access::lock");
+//!    // The entry name _must_ be unique and can be interpreted as a
+//!    // unique mutex instance reference tag.
+//!    let mut lock = StorageLock::<Time>::new(b"access::lock");
 //!    {
 //!         let _guard = lock.lock();
 //!         let acc = StorageValueRef::persistent(key);
 //!         let v: Vec<T> = acc.get::<Vec<T>>().unwrap().unwrap();
 //!         // modify `v` as desired
-//!         // i.e. perform some heavy computation
+//!         // i.e. perform some heavy computation with
 //!         // side effects that should only be done once.
 //!         acc.set(v);
 //!         // drop `_guard` implicitly at end of scope
@@ -65,6 +66,9 @@ use sp_io::offchain;
 /// Default expiry duration in milliseconds.
 const STORAGE_LOCK_DEFAULT_EXPIRY_DURATION_MS: u64 = 30_000;
 
+/// Default expiry duration in milliseconds.
+const STORAGE_LOCK_DEFAULT_EXPIRY_BLOCKS: u32 = 4;
+
 /// Snooze duration before attempting to lock again in ms.
 const STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE: u64 = 100;
 
@@ -74,24 +78,21 @@ const STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE: u64 = 100;
 /// without explicitly requiring `Ord` trait in general.
 pub trait Lockable: Sized {
 	/// The instant type
-	type Instant: Sized + Codec + Clone;
-
-	/// Get the current value of lockable.
-	fn current() -> Self::Instant;
+	type Deadline: Sized + Codec + Clone;
 
 	/// Acquire a new deadline based on `Self::current()`.
-	fn deadline(&self) -> Self::Instant;
+	fn deadline(&self) -> Self::Deadline;
 
 	/// Verify the current value of `self` against `deadline`.
 	/// to determine if the lock has expired.
-	fn expired(deadline: &Self::Instant) -> bool;
+	fn has_expired(deadline: &Self::Deadline) -> bool;
 
 	/// Snooze the thread for time determined by `self` and `other`.
 	///
 	/// Only called if not expired just yet.
 	/// Note that the deadline is only passed to allow some optimizations
 	/// for some `L` types.
-	fn snooze(_deadline: &Self::Instant) {
+	fn snooze(_deadline: &Self::Deadline) {
 		sp_io::offchain::sleep_until(offchain::timestamp().add(Duration::from_millis(
 			STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE,
 		)));
@@ -101,39 +102,31 @@ pub trait Lockable: Sized {
 /// Lockable based on the current timestamp with a configurable expiration time.
 #[derive(Encode, Decode)]
 pub struct Time {
-	/// Time of calling `fn lock(..)`.
-	timestamp: Timestamp,
-	/// How long the lock will stay valid once `fn lock(..)` is called.
+	/// How long the lock will stay valid once `fn lock(..)` successfully acquired a lock.
 	expiration_duration: Duration,
 }
 
 impl Default for Time {
 	fn default() -> Self {
-		let timestamp = offchain::timestamp();
 		Self {
-			timestamp,
 			expiration_duration: Duration::from_millis(STORAGE_LOCK_DEFAULT_EXPIRY_DURATION_MS),
 		}
 	}
 }
 
 impl Lockable for Time {
-	type Instant = Timestamp;
+	type Deadline = Timestamp;
 
-	fn current() -> Self::Instant {
-		offchain::timestamp()
+	fn deadline(&self) -> Self::Deadline {
+		offchain::timestamp().add(self.expiration_duration)
 	}
 
-	fn deadline(&self) -> Self::Instant {
-		self.timestamp.add(self.expiration_duration)
+	fn has_expired(deadline: &Self::Deadline) -> bool {
+		offchain::timestamp() > *deadline
 	}
 
-	fn expired(deadline: &Self::Instant) -> bool {
-		<Self as Lockable>::current() > *deadline
-	}
-
-	fn snooze(deadline: &Self::Instant) {
-		let now = Self::current();
+	fn snooze(deadline: &Self::Deadline) {
+		let now = offchain::timestamp();
 		let remainder: Duration = now.diff(&deadline);
 		// do not snooze the full duration, but instead snooze max 100ms
 		// it might get unlocked in another thread
@@ -145,12 +138,12 @@ impl Lockable for Time {
 
 /// An instant based on block and time.
 #[derive(Encode, Decode, Eq, PartialEq)]
-pub struct BlockAndTimeInstant<B: BlockNumberProvider> {
+pub struct BlockAndTimeDeadline<B: BlockNumberProvider> {
 	pub block_number: <B as BlockNumberProvider>::BlockNumber,
 	pub timestamp: Timestamp,
 }
 
-impl<B: BlockNumberProvider> Clone for BlockAndTimeInstant<B> {
+impl<B: BlockNumberProvider> Clone for BlockAndTimeDeadline<B> {
 	fn clone(&self) -> Self {
 		Self {
 			block_number: self.block_number.clone(),
@@ -159,34 +152,35 @@ impl<B: BlockNumberProvider> Clone for BlockAndTimeInstant<B> {
 	}
 }
 
-impl<B: BlockNumberProvider> BlockAndTimeInstant<B> {
+impl<B: BlockNumberProvider> Default for BlockAndTimeDeadline<B> {
 	/// Provide the current state of block number and time.
-	fn current() -> Self {
+	fn default() -> Self {
 		Self {
-			block_number: B::current_block_number(),
-			timestamp: offchain::timestamp(),
+			block_number: B::current_block_number() + STORAGE_LOCK_DEFAULT_EXPIRY_BLOCKS.into(),
+			timestamp: offchain::timestamp().add(Duration::from_millis(STORAGE_LOCK_DEFAULT_EXPIRY_DURATION_MS)),
 		}
 	}
 }
 
 /// Lockable based on the current block number and a timestamp based deadline.
 pub struct BlockAndTime<B: BlockNumberProvider> {
-	/// The instant when calling `fn lock(..)`.
-	lock_instant: BlockAndTimeInstant<B>,
 	/// The block number offset from the time of locking
 	/// when the lock is considered stale.
 	expiration_block_number_offset: u32,
 	/// Additional timestamp based deadline, which, once
 	/// reached, renders the lock stale.
 	expiration_duration: Duration,
+
+	#[doc(hidden)]
+	_phantom: core::marker::PhantomData<B>,
 }
 
 impl<B: BlockNumberProvider> Default for BlockAndTime<B> {
 	fn default() -> Self {
 		Self {
-			lock_instant: BlockAndTimeInstant::current(),
 			expiration_block_number_offset: 3u32,
 			expiration_duration: Duration::from_millis(STORAGE_LOCK_DEFAULT_EXPIRY_DURATION_MS),
+			_phantom: core::marker::PhantomData::<B>,
 		}
 	}
 }
@@ -195,37 +189,32 @@ impl<B: BlockNumberProvider> Default for BlockAndTime<B> {
 impl<B: BlockNumberProvider> Clone for BlockAndTime<B> {
 	fn clone(&self) -> Self {
 		Self {
-			lock_instant: self.lock_instant.clone(),
 			expiration_block_number_offset: self.expiration_block_number_offset.clone(),
 			expiration_duration: self.expiration_duration,
+			_phantom: core::marker::PhantomData::<B>,
 		}
 	}
 }
 
 impl<B: BlockNumberProvider> Lockable for BlockAndTime<B> {
-	type Instant = BlockAndTimeInstant<B>;
-
-	fn current() -> Self::Instant {
-		Self::Instant::current()
-	}
+	type Deadline = BlockAndTimeDeadline<B>;
 
-	fn deadline(&self) -> Self::Instant {
-		let mut current = Self::current();
-		current.block_number += self.expiration_block_number_offset.into();
-		current.timestamp.add(self.expiration_duration);
-		current
+	fn deadline(&self) -> Self::Deadline {
+		BlockAndTimeDeadline {
+			timestamp: offchain::timestamp().add(self.expiration_duration),
+			block_number: <B as BlockNumberProvider>::current_block_number() + self.expiration_block_number_offset.into(),
+		}
 	}
 
-	fn expired(deadline: &Self::Instant) -> bool {
-		let current = <Self as Lockable>::current();
-		current.timestamp > deadline.timestamp && current.block_number > deadline.block_number
+	fn has_expired(deadline: &Self::Deadline) -> bool {
+		offchain::timestamp() > deadline.timestamp && <B as BlockNumberProvider>::current_block_number() > deadline.block_number
 	}
 
-	fn snooze(deadline: &Self::Instant) {
-		let timestamp = Self::current().timestamp;
-		let remainder: Duration = timestamp.diff(&(deadline.timestamp));
+	fn snooze(deadline: &Self::Deadline) {
+		let now = offchain::timestamp();
+		let remainder: Duration = now.diff(&(deadline.timestamp));
 		let snooze = core::cmp::min(remainder.millis(), STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE);
-		sp_io::offchain::sleep_until(timestamp.add(Duration::from_millis(snooze)));
+		sp_io::offchain::sleep_until(now.add(Duration::from_millis(snooze)));
 	}
 }
 
@@ -257,16 +246,16 @@ impl<'a, L: Lockable> StorageLock<'a, L> {
 	}
 
 	/// Internal lock helper to avoid lifetime conflicts.
-	fn try_lock_inner(&mut self, new_deadline: L::Instant) -> Result<(), Option<L::Instant>> {
+	fn try_lock_inner(&mut self, new_deadline: L::Deadline) -> Result<(), Option<L::Deadline>> {
 		let res = self.value_ref.mutate(
-			|s: Option<Option<L::Instant>>| -> Result<L::Instant, Option<L::Instant>> {
+			|s: Option<Option<L::Deadline>>| -> Result<L::Deadline, Option<L::Deadline>> {
 				match s {
 					// no lock set, we can safely acquire it
 					None => Ok(new_deadline),
 					// write was good, bur read failed
 					Some(None) => Ok(new_deadline),
 					// lock is set, but it's old. We can re-acquire it.
-					Some(Some(deadline)) if <L as Lockable>::expired(&deadline) => Ok(new_deadline),
+					Some(Some(deadline)) if <L as Lockable>::has_expired(&deadline) => Ok(new_deadline),
 					// lock is present and is still active
 					Some(Some(deadline)) => Err(Some(deadline)),
 				}
@@ -346,7 +335,6 @@ impl<'a> StorageLock<'a, Time> {
 		Self {
 			value_ref: StorageValueRef::<'a>::persistent(key),
 			lockable: Time {
-				timestamp: offchain::timestamp(),
 				expiration_duration: expiration_duration,
 			},
 		}
@@ -365,9 +353,9 @@ where
 		Self {
 			value_ref: StorageValueRef::<'a>::persistent(key),
 			lockable: BlockAndTime::<B> {
-				lock_instant: BlockAndTimeInstant::<B>::current(),
 				expiration_block_number_offset,
 				expiration_duration,
+				_phantom: core::marker::PhantomData,
 			},
 		}
 	}

From c3dd254b780a3170e566b4f0f628d50e54196af6 Mon Sep 17 00:00:00 2001
From: Bernhard Schuster <bernhard@parity.io>
Date: Tue, 26 May 2020 11:39:52 +0200
Subject: [PATCH 27/44] docs/reword: adjust to changed code

---
 .../runtime/src/offchain/storage_lock.rs      | 80 ++++++++++---------
 1 file changed, 42 insertions(+), 38 deletions(-)

diff --git a/primitives/runtime/src/offchain/storage_lock.rs b/primitives/runtime/src/offchain/storage_lock.rs
index 71a6f8d7f98d1..bb53107fd98aa 100644
--- a/primitives/runtime/src/offchain/storage_lock.rs
+++ b/primitives/runtime/src/offchain/storage_lock.rs
@@ -20,7 +20,7 @@
 //!
 //! The lock is using Local Storage and allows synchronizing access to critical
 //! section of your code for concurrently running Off-chain Workers. Usage of
-//! `PERSISTENT` variant of the storage persists the lock value accross a full node
+//! `PERSISTENT` variant of the storage persists the lock value across a full node
 //! restart or re-orgs.
 //!
 //! A use case for the lock is to make sure that a particular section of the
@@ -63,13 +63,13 @@ use codec::{Codec, Decode, Encode};
 use sp_core::offchain::{Duration, Timestamp};
 use sp_io::offchain;
 
-/// Default expiry duration in milliseconds.
+/// Default expiry duration for time based locks in milliseconds.
 const STORAGE_LOCK_DEFAULT_EXPIRY_DURATION_MS: u64 = 30_000;
 
-/// Default expiry duration in milliseconds.
+/// Default expiry duration for block based locks in blocks.
 const STORAGE_LOCK_DEFAULT_EXPIRY_BLOCKS: u32 = 4;
 
-/// Snooze duration before attempting to lock again in ms.
+/// Time between checks if the lock is still being held in ms.
 const STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE: u64 = 100;
 
 /// Lockable item for use with a persisted storage lock.
@@ -77,21 +77,21 @@ const STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE: u64 = 100;
 /// Bound for an item that has a stateful ordered meaning
 /// without explicitly requiring `Ord` trait in general.
 pub trait Lockable: Sized {
-	/// The instant type
+	/// An instant type specifying i.e. a point in time.
 	type Deadline: Sized + Codec + Clone;
 
-	/// Acquire a new deadline based on `Self::current()`.
+	/// Calculate the deadline based on a current state
+	/// such as time or block number and derives the deadline.
 	fn deadline(&self) -> Self::Deadline;
 
-	/// Verify the current value of `self` against `deadline`.
-	/// to determine if the lock has expired.
+	/// Verify the deadline has not expired compared to the
+	/// current state, i.e. time or block number.
 	fn has_expired(deadline: &Self::Deadline) -> bool;
 
-	/// Snooze the thread for time determined by `self` and `other`.
+	/// Snooze at least until `deadline` is reached.
 	///
-	/// Only called if not expired just yet.
-	/// Note that the deadline is only passed to allow some optimizations
-	/// for some `L` types.
+	/// Note that `deadline` is only passed to allow optimizations
+	/// for `Lockables` which have a time based component.
 	fn snooze(_deadline: &Self::Deadline) {
 		sp_io::offchain::sleep_until(offchain::timestamp().add(Duration::from_millis(
 			STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE,
@@ -102,7 +102,8 @@ pub trait Lockable: Sized {
 /// Lockable based on the current timestamp with a configurable expiration time.
 #[derive(Encode, Decode)]
 pub struct Time {
-	/// How long the lock will stay valid once `fn lock(..)` successfully acquired a lock.
+	/// How long the lock will stay valid once `fn lock(..)` or
+	/// `fn try_lock(..)` successfully acquire a lock.
 	expiration_duration: Duration,
 }
 
@@ -130,13 +131,12 @@ impl Lockable for Time {
 		let remainder: Duration = now.diff(&deadline);
 		// do not snooze the full duration, but instead snooze max 100ms
 		// it might get unlocked in another thread
-		// consider adding some additive jitter here
 		let snooze = core::cmp::min(remainder.millis(), STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE);
 		sp_io::offchain::sleep_until(now.add(Duration::from_millis(snooze)));
 	}
 }
 
-/// An instant based on block and time.
+/// A deadline based on block number and time.
 #[derive(Encode, Decode, Eq, PartialEq)]
 pub struct BlockAndTimeDeadline<B: BlockNumberProvider> {
 	pub block_number: <B as BlockNumberProvider>::BlockNumber,
@@ -162,13 +162,16 @@ impl<B: BlockNumberProvider> Default for BlockAndTimeDeadline<B> {
 	}
 }
 
-/// Lockable based on the current block number and a timestamp based deadline.
+/// Lockable based on block number and timestamp.
+///
+/// Expiration is defined if both, block number _and_ timestamp
+/// expire.
 pub struct BlockAndTime<B: BlockNumberProvider> {
-	/// The block number offset from the time of locking
-	/// when the lock is considered stale.
+	/// Relative block number offset, which is used to determine
+	/// the block number part of the deadline.
 	expiration_block_number_offset: u32,
-	/// Additional timestamp based deadline, which, once
-	/// reached, renders the lock stale.
+	/// Relative duration, used to derive the time based part of
+	/// the deadline.
 	expiration_duration: Duration,
 
 	#[doc(hidden)]
@@ -178,7 +181,7 @@ pub struct BlockAndTime<B: BlockNumberProvider> {
 impl<B: BlockNumberProvider> Default for BlockAndTime<B> {
 	fn default() -> Self {
 		Self {
-			expiration_block_number_offset: 3u32,
+			expiration_block_number_offset: STORAGE_LOCK_DEFAULT_EXPIRY_BLOCKS,
 			expiration_duration: Duration::from_millis(STORAGE_LOCK_DEFAULT_EXPIRY_DURATION_MS),
 			_phantom: core::marker::PhantomData::<B>,
 		}
@@ -220,9 +223,9 @@ impl<B: BlockNumberProvider> Lockable for BlockAndTime<B> {
 
 /// Storage based lock.
 ///
-/// A lock that is persisted in the DB and provides a mutex behavior
-/// with a defined safety expiry deadline based on a [`Lockable`](Self::Lockable)
-/// implementation.
+/// A lock that is persisted in the DB and provides the ability to guard against
+/// concurrent access in an off-chain worker, with a defined expiry deadline
+/// based on the concrete [`Lockable`](Self::Lockable) implementation.
 pub struct StorageLock<'a, L = Time> {
 	// A storage value ref which defines the DB entry representing the lock.
 	value_ref: StorageValueRef<'a>,
@@ -268,7 +271,7 @@ impl<'a, L: Lockable> StorageLock<'a, L> {
 		}
 	}
 
-	/// Attempt to lock the storage entry.
+	/// Attempt to lock using the storage entry.
 	///
 	/// Returns a lock guard on success, otherwise an error containing `None` in
 	/// case the mutex was already unlocked before, or if the lock is still held
@@ -307,13 +310,13 @@ pub struct StorageLockGuard<'a, 'b, L: Lockable> {
 }
 
 impl<'a, 'b, L: Lockable> StorageLockGuard<'a, 'b, L> {
-	/// Consume the guard but DO NOT unlock the underlying lock.
+	/// Consume the guard but **do not** unlock the underlying lock.
 	///
 	/// Can be used to implement a grace period after doing some
 	/// heavy computations and sending a transaction to be included
 	/// on-chain. By forgetting the lock, it will stay locked until
 	/// its expiration deadline is reached while the off-chain worker
-	/// can already complete.
+	/// can already exit.
 	pub fn forget(mut self) {
 		let _ = self.lock.take();
 	}
@@ -327,10 +330,9 @@ impl<'a, 'b, L: Lockable> Drop for StorageLockGuard<'a, 'b, L> {
 	}
 }
 
-/// Allows explicitly setting the timeout on construction
-/// instead of using the implicit default timeout of
-/// [`STORAGE_LOCK_DEFAULT_EXPIRY_DURATION_MS`](Self::STORAGE_LOCK_DEFAULT_EXPIRY_DURATION_MS).
 impl<'a> StorageLock<'a, Time> {
+	/// Explicitly create a time based storage lock with a non-default
+	/// expiration timeout.
 	pub fn with_deadline(key: &'a [u8], expiration_duration: Duration) -> Self {
 		Self {
 			value_ref: StorageValueRef::<'a>::persistent(key),
@@ -345,6 +347,8 @@ impl<'a, B> StorageLock<'a, BlockAndTime<B>>
 where
 	B: BlockNumberProvider,
 {
+	/// Explicitly create a time and block number based storage lock with
+	/// a non-default expiration duration and block number offset.
 	pub fn with_block_and_time_deadline(
 		key: &'a [u8],
 		expiration_block_number_offset: u32,
@@ -361,24 +365,24 @@ where
 	}
 }
 
-/// Bound for block numbers which commonly will be implemented by the `frame_system::Trait::BlockNumber`.
+/// Bound for block number sources which commonly is implemented
+/// by `frame_system::Module<T: Trait>`.
 ///
-/// This trait has no intrinsic meaning and exists only to decouple `frame_system`
-/// from `runtime` crate and avoid a circular dependency.
+/// `BlockNumberProvider` has no intrinsic meaning and exists only to decouple `frame_system`
+/// from `runtime` crate and avoid a circular dependency issues.
 pub trait BlockNumberProvider {
-	/// Type of `BlockNumber` the provider is going to provide
-	/// with `deadline()` and `current()`.
+	/// Type of `BlockNumber` which is going to be provided.
 	type BlockNumber: Codec + Clone + Ord + Eq + AtLeast32Bit;
 	/// Returns the current block number.
 	///
-	/// Commonly this will be implemented as
+	/// For `frame_system::Module<T: Trait>` it is implemented as
+	///
 	/// ```ignore
 	/// fn current_block_number() -> Self {
 	///     frame_system::Module<Trait>::block_number()
 	/// }
 	/// ```
-	/// but note that the definition of current is
-	/// application specific.
+	/// .
 	fn current_block_number() -> Self::BlockNumber;
 }
 

From b8995833144a3b4332640776bb7a0d6b46226020 Mon Sep 17 00:00:00 2001
From: Bernhard Schuster <bernhard@parity.io>
Date: Tue, 26 May 2020 15:14:04 +0200
Subject: [PATCH 28/44] fix/offchain/testing: timestamp and sleep_until shall
 not block

---
 primitives/core/src/offchain/testing.rs | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/primitives/core/src/offchain/testing.rs b/primitives/core/src/offchain/testing.rs
index 23d0f14c23321..76cf8915f2054 100644
--- a/primitives/core/src/offchain/testing.rs
+++ b/primitives/core/src/offchain/testing.rs
@@ -75,6 +75,8 @@ pub struct OffchainState {
 	pub local_storage: InMemOffchainStorage,
 	/// A supposedly random seed.
 	pub seed: [u8; 32],
+	/// A timestamp simulating the current time.
+	pub timestamp: Timestamp,
 }
 
 impl OffchainState {
@@ -158,21 +160,11 @@ impl offchain::Externalities for TestOffchainExt {
 	}
 
 	fn timestamp(&mut self) -> Timestamp {
-		use std::convert::TryInto;
-		let millis = std::time::SystemTime::now()
-			.duration_since(std::time::UNIX_EPOCH)
-			.expect("Calculating unix epoch in millis for tests must succeed. qed")
-			.as_millis().try_into()
-			.expect("Calculating unix epoch in millis exceeded u64. qed");
-		Timestamp(millis)
+		self.0.read().timestamp
 	}
 
 	fn sleep_until(&mut self, deadline: Timestamp) {
-		let now = self.timestamp();
-		if deadline > now {
-			let dur = deadline.diff(&now);
-			std::thread::sleep(std::time::Duration::from_millis(dur.millis()));
-		}
+		self.0.write().timestamp = deadline;
 	}
 
 	fn random_seed(&mut self) -> [u8; 32] {

From 7dddd76791c126bc716c1f68be42a42e89d2cb98 Mon Sep 17 00:00:00 2001
From: Bernhard Schuster <bernhard@parity.io>
Date: Tue, 26 May 2020 15:14:29 +0200
Subject: [PATCH 29/44] chore/lines: lines must < 100 chars

---
 primitives/runtime/src/offchain/storage_lock.rs | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/primitives/runtime/src/offchain/storage_lock.rs b/primitives/runtime/src/offchain/storage_lock.rs
index bb53107fd98aa..42afd37d24fef 100644
--- a/primitives/runtime/src/offchain/storage_lock.rs
+++ b/primitives/runtime/src/offchain/storage_lock.rs
@@ -157,7 +157,10 @@ impl<B: BlockNumberProvider> Default for BlockAndTimeDeadline<B> {
 	fn default() -> Self {
 		Self {
 			block_number: B::current_block_number() + STORAGE_LOCK_DEFAULT_EXPIRY_BLOCKS.into(),
-			timestamp: offchain::timestamp().add(Duration::from_millis(STORAGE_LOCK_DEFAULT_EXPIRY_DURATION_MS)),
+			timestamp: offchain::timestamp()
+				.add(
+					Duration::from_millis(STORAGE_LOCK_DEFAULT_EXPIRY_DURATION_MS)
+				),
 		}
 	}
 }
@@ -203,14 +206,17 @@ impl<B: BlockNumberProvider> Lockable for BlockAndTime<B> {
 	type Deadline = BlockAndTimeDeadline<B>;
 
 	fn deadline(&self) -> Self::Deadline {
+		let block_number = <B as BlockNumberProvider>::current_block_number()
+			+ self.expiration_block_number_offset.into();
 		BlockAndTimeDeadline {
 			timestamp: offchain::timestamp().add(self.expiration_duration),
-			block_number: <B as BlockNumberProvider>::current_block_number() + self.expiration_block_number_offset.into(),
+			block_number,
 		}
 	}
 
 	fn has_expired(deadline: &Self::Deadline) -> bool {
-		offchain::timestamp() > deadline.timestamp && <B as BlockNumberProvider>::current_block_number() > deadline.block_number
+		offchain::timestamp() > deadline.timestamp
+		&& <B as BlockNumberProvider>::current_block_number() > deadline.block_number
 	}
 
 	fn snooze(deadline: &Self::Deadline) {

From cf1a87a208981f49efd11414891f0c0fb9346d6f Mon Sep 17 00:00:00 2001
From: Bernhard Schuster <bernhard@parity.io>
Date: Tue, 26 May 2020 15:38:31 +0200
Subject: [PATCH 30/44] fix/docs: add missing pub field doc comments

---
 primitives/runtime/src/offchain/storage_lock.rs | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/primitives/runtime/src/offchain/storage_lock.rs b/primitives/runtime/src/offchain/storage_lock.rs
index 42afd37d24fef..e79a149c76243 100644
--- a/primitives/runtime/src/offchain/storage_lock.rs
+++ b/primitives/runtime/src/offchain/storage_lock.rs
@@ -139,7 +139,9 @@ impl Lockable for Time {
 /// A deadline based on block number and time.
 #[derive(Encode, Decode, Eq, PartialEq)]
 pub struct BlockAndTimeDeadline<B: BlockNumberProvider> {
+	/// The block number until which the lock is still valid _at least_.
 	pub block_number: <B as BlockNumberProvider>::BlockNumber,
+	/// The timestamp until which the lock is still valid _at least_.
 	pub timestamp: Timestamp,
 }
 

From e6e6ec2a249053428f6364cec0f2e5ea7c316666 Mon Sep 17 00:00:00 2001
From: Bernhard Schuster <bernhard@parity.io>
Date: Tue, 26 May 2020 16:29:11 +0200
Subject: [PATCH 31/44] refactor/x: try_lock does not need to return an
 Option<_>

---
 .../runtime/src/offchain/storage_lock.rs      | 27 ++++++++++---------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/primitives/runtime/src/offchain/storage_lock.rs b/primitives/runtime/src/offchain/storage_lock.rs
index e79a149c76243..f40d85a0a1851 100644
--- a/primitives/runtime/src/offchain/storage_lock.rs
+++ b/primitives/runtime/src/offchain/storage_lock.rs
@@ -257,9 +257,9 @@ impl<'a, L: Lockable> StorageLock<'a, L> {
 	}
 
 	/// Internal lock helper to avoid lifetime conflicts.
-	fn try_lock_inner(&mut self, new_deadline: L::Deadline) -> Result<(), Option<L::Deadline>> {
+	fn try_lock_inner(&mut self, new_deadline: L::Deadline) -> Result<(), <L as Lockable>::Deadline> {
 		let res = self.value_ref.mutate(
-			|s: Option<Option<L::Deadline>>| -> Result<L::Deadline, Option<L::Deadline>> {
+			|s: Option<Option<L::Deadline>>| -> Result<<L as Lockable>::Deadline, <L as Lockable>::Deadline> {
 				match s {
 					// no lock set, we can safely acquire it
 					None => Ok(new_deadline),
@@ -268,39 +268,40 @@ impl<'a, L: Lockable> StorageLock<'a, L> {
 					// lock is set, but it's old. We can re-acquire it.
 					Some(Some(deadline)) if <L as Lockable>::has_expired(&deadline) => Ok(new_deadline),
 					// lock is present and is still active
-					Some(Some(deadline)) => Err(Some(deadline)),
+					Some(Some(deadline)) => Err(deadline),
 				}
 			},
 		);
 		match res {
 			Ok(Ok(_)) => Ok(()),
-			Ok(Err(_deadline)) => Err(None),
+			Ok(Err(deadline)) => Err(deadline),
 			Err(e) => Err(e),
 		}
 	}
 
 	/// Attempt to lock using the storage entry.
 	///
-	/// Returns a lock guard on success, otherwise an error containing `None` in
-	/// case the mutex was already unlocked before, or if the lock is still held
-	/// by another process `Err(())`.
-	pub fn try_lock(&mut self) -> Result<StorageLockGuard<'a, '_, L>, ()> {
-		let _ = self
-			.try_lock_inner(self.lockable.deadline())
-			.map_err(|_opt| ())?;
+	/// Returns a lock guard on success, otherwise an error containing the
+	/// `<Self::Lockable>::Deadline` in for the currently locked lock
+	/// by another process: `Err(<L as Lockable>::Deadline)`.
+	pub fn try_lock(&mut self) -> Result<StorageLockGuard<'a, '_, L>, <L as Lockable>::Deadline> {
+		self.try_lock_inner(self.lockable.deadline())?;
 		Ok(StorageLockGuard::<'a, '_> { lock: Some(self) })
 	}
 
 	/// Try grabbing the lock until its expiry is reached.
 	///
 	/// Returns an error if the lock expired before it could be caught.
+	/// If one uses `fn forget(..)`, it is highly likely `fn try_lock(..)`
+	/// is the correct API to use instead of `fn lock(..)`, since that might
+	/// never unlock in the anticipated span i.e. when used with `BlockAndTime` during a certain
+	/// block number span.
 	pub fn lock(&mut self) -> StorageLockGuard<'a, '_, L> {
 		loop {
 			// blind attempt on locking
 			let deadline = match self.try_lock_inner(self.lockable.deadline()) {
 				Ok(_) => return StorageLockGuard::<'a, '_, L> { lock: Some(self) },
-				Err(Some(other_locks_deadline)) => other_locks_deadline,
-				_ => self.lockable.deadline(), // use the default
+				Err(other_locks_deadline) => other_locks_deadline,
 			};
 			L::snooze(&deadline);
 		}

From 78bf5015875105253a301cda9860f8dd8a7e7d50 Mon Sep 17 00:00:00 2001
From: Bernhard Schuster <bernhard@parity.io>
Date: Tue, 26 May 2020 16:42:07 +0200
Subject: [PATCH 32/44] refactor/simplify: a better way of waiting for a lock
 to resolve

---
 primitives/runtime/src/offchain/storage_lock.rs | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/primitives/runtime/src/offchain/storage_lock.rs b/primitives/runtime/src/offchain/storage_lock.rs
index f40d85a0a1851..8a33a8b884718 100644
--- a/primitives/runtime/src/offchain/storage_lock.rs
+++ b/primitives/runtime/src/offchain/storage_lock.rs
@@ -297,14 +297,10 @@ impl<'a, L: Lockable> StorageLock<'a, L> {
 	/// never unlock in the anticipated span i.e. when used with `BlockAndTime` during a certain
 	/// block number span.
 	pub fn lock(&mut self) -> StorageLockGuard<'a, '_, L> {
-		loop {
-			// blind attempt on locking
-			let deadline = match self.try_lock_inner(self.lockable.deadline()) {
-				Ok(_) => return StorageLockGuard::<'a, '_, L> { lock: Some(self) },
-				Err(other_locks_deadline) => other_locks_deadline,
-			};
+		while let Err(deadline) = self.try_lock_inner(self.lockable.deadline()) {
 			L::snooze(&deadline);
 		}
+		StorageLockGuard::<'a, '_, L> { lock: Some(self) }
 	}
 
 	/// Explicitly unlock the lock.

From 2d6fcba7fcc52b9c2704094fa19a2bb111cb3666 Mon Sep 17 00:00:00 2001
From: Bernhard Schuster <bernhard@parity.io>
Date: Wed, 27 May 2020 08:08:28 +0200
Subject: [PATCH 33/44] docs: consistency

---
 .../runtime/src/offchain/storage_lock.rs      | 23 +++++++++----------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/primitives/runtime/src/offchain/storage_lock.rs b/primitives/runtime/src/offchain/storage_lock.rs
index 8a33a8b884718..181e1d21a3975 100644
--- a/primitives/runtime/src/offchain/storage_lock.rs
+++ b/primitives/runtime/src/offchain/storage_lock.rs
@@ -14,7 +14,7 @@
 // You should have received a copy of the GNU General Public License
 // along with Substrate.  If not, see <http://www.gnu.org/licenses/>.
 
-//! # Offchain Storage Lock
+//! # Off-chain Storage Lock
 //!
 //! A storage-based lock with a defined expiry time.
 //!
@@ -29,7 +29,7 @@
 //! multiple Local Storage entries.
 //!
 //! One use case would be collective updates of multiple data items or append /
-//! remove of i.e. sets, vectors which are stored in the offchain storage DB.
+//! remove of i.e. sets, vectors which are stored in the off-chain storage DB.
 //!
 //! ## Example:
 //!
@@ -69,7 +69,7 @@ const STORAGE_LOCK_DEFAULT_EXPIRY_DURATION_MS: u64 = 30_000;
 /// Default expiry duration for block based locks in blocks.
 const STORAGE_LOCK_DEFAULT_EXPIRY_BLOCKS: u32 = 4;
 
-/// Time between checks if the lock is still being held in ms.
+/// Time between checks if the lock is still being held in milliseconds.
 const STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE: u64 = 100;
 
 /// Lockable item for use with a persisted storage lock.
@@ -263,9 +263,9 @@ impl<'a, L: Lockable> StorageLock<'a, L> {
 				match s {
 					// no lock set, we can safely acquire it
 					None => Ok(new_deadline),
-					// write was good, bur read failed
+					// write was good, but read failed
 					Some(None) => Ok(new_deadline),
-					// lock is set, but it's old. We can re-acquire it.
+					// lock is set, but it is expired. We can re-acquire it.
 					Some(Some(deadline)) if <L as Lockable>::has_expired(&deadline) => Ok(new_deadline),
 					// lock is present and is still active
 					Some(Some(deadline)) => Err(deadline),
@@ -279,23 +279,22 @@ impl<'a, L: Lockable> StorageLock<'a, L> {
 		}
 	}
 
-	/// Attempt to lock using the storage entry.
+	/// A single attempt to lock using the storage entry.
 	///
 	/// Returns a lock guard on success, otherwise an error containing the
-	/// `<Self::Lockable>::Deadline` in for the currently locked lock
-	/// by another process: `Err(<L as Lockable>::Deadline)`.
+	/// `<Self::Lockable>::Deadline` in for the currently active lock
+	/// by another task `Err(<L as Lockable>::Deadline)`.
 	pub fn try_lock(&mut self) -> Result<StorageLockGuard<'a, '_, L>, <L as Lockable>::Deadline> {
 		self.try_lock_inner(self.lockable.deadline())?;
 		Ok(StorageLockGuard::<'a, '_> { lock: Some(self) })
 	}
 
-	/// Try grabbing the lock until its expiry is reached.
+	/// Repeated lock attempts until the lock is successfully acquired.
 	///
-	/// Returns an error if the lock expired before it could be caught.
 	/// If one uses `fn forget(..)`, it is highly likely `fn try_lock(..)`
 	/// is the correct API to use instead of `fn lock(..)`, since that might
-	/// never unlock in the anticipated span i.e. when used with `BlockAndTime` during a certain
-	/// block number span.
+	/// never unlock in the anticipated span i.e. when used with `BlockAndTime`
+	/// during a certain block number span.
 	pub fn lock(&mut self) -> StorageLockGuard<'a, '_, L> {
 		while let Err(deadline) = self.try_lock_inner(self.lockable.deadline()) {
 			L::snooze(&deadline);

From 7febfe96352be1bb3bd8a1e19707357a50a98da9 Mon Sep 17 00:00:00 2001
From: Bernhard Schuster <bernhard@parity.io>
Date: Wed, 27 May 2020 08:23:38 +0200
Subject: [PATCH 34/44] fix/line: < 100

---
 .../runtime/src/offchain/storage_lock.rs      | 20 +++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/primitives/runtime/src/offchain/storage_lock.rs b/primitives/runtime/src/offchain/storage_lock.rs
index 181e1d21a3975..4c58824c77105 100644
--- a/primitives/runtime/src/offchain/storage_lock.rs
+++ b/primitives/runtime/src/offchain/storage_lock.rs
@@ -159,10 +159,9 @@ impl<B: BlockNumberProvider> Default for BlockAndTimeDeadline<B> {
 	fn default() -> Self {
 		Self {
 			block_number: B::current_block_number() + STORAGE_LOCK_DEFAULT_EXPIRY_BLOCKS.into(),
-			timestamp: offchain::timestamp()
-				.add(
-					Duration::from_millis(STORAGE_LOCK_DEFAULT_EXPIRY_DURATION_MS)
-				),
+			timestamp: offchain::timestamp().add(Duration::from_millis(
+				STORAGE_LOCK_DEFAULT_EXPIRY_DURATION_MS,
+			)),
 		}
 	}
 }
@@ -218,7 +217,7 @@ impl<B: BlockNumberProvider> Lockable for BlockAndTime<B> {
 
 	fn has_expired(deadline: &Self::Deadline) -> bool {
 		offchain::timestamp() > deadline.timestamp
-		&& <B as BlockNumberProvider>::current_block_number() > deadline.block_number
+			&& <B as BlockNumberProvider>::current_block_number() > deadline.block_number
 	}
 
 	fn snooze(deadline: &Self::Deadline) {
@@ -257,16 +256,21 @@ impl<'a, L: Lockable> StorageLock<'a, L> {
 	}
 
 	/// Internal lock helper to avoid lifetime conflicts.
-	fn try_lock_inner(&mut self, new_deadline: L::Deadline) -> Result<(), <L as Lockable>::Deadline> {
+	fn try_lock_inner(
+		&mut self,
+		new_deadline: L::Deadline,
+	) -> Result<(), <L as Lockable>::Deadline> {
 		let res = self.value_ref.mutate(
-			|s: Option<Option<L::Deadline>>| -> Result<<L as Lockable>::Deadline, <L as Lockable>::Deadline> {
+			|s: Option<Option<L::Deadline>>|
+			-> Result<<L as Lockable>::Deadline, <L as Lockable>::Deadline> {
 				match s {
 					// no lock set, we can safely acquire it
 					None => Ok(new_deadline),
 					// write was good, but read failed
 					Some(None) => Ok(new_deadline),
 					// lock is set, but it is expired. We can re-acquire it.
-					Some(Some(deadline)) if <L as Lockable>::has_expired(&deadline) => Ok(new_deadline),
+					Some(Some(deadline)) if <L as Lockable>::has_expired(&deadline) =>
+						Ok(new_deadline),
 					// lock is present and is still active
 					Some(Some(deadline)) => Err(deadline),
 				}

From 230ac43684969500ba4ecac62d0912d434afa09b Mon Sep 17 00:00:00 2001
From: Bernhard Schuster <bernhard@parity.io>
Date: Wed, 27 May 2020 09:09:39 +0200
Subject: [PATCH 35/44] fix/doctest/use: avoid crate::

---
 primitives/runtime/src/offchain/storage_lock.rs | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/primitives/runtime/src/offchain/storage_lock.rs b/primitives/runtime/src/offchain/storage_lock.rs
index 4c58824c77105..87682fba48dc6 100644
--- a/primitives/runtime/src/offchain/storage_lock.rs
+++ b/primitives/runtime/src/offchain/storage_lock.rs
@@ -34,7 +34,8 @@
 //! ## Example:
 //!
 //! ```rust
-//! # use crate::offchain::storage::StorageValueRef;
+//! # use sp_core::offchain::storage::StorageValueRef;
+//! # use sp_runtime::offchain::storage_lock::Time;
 //! # use codec::{Decode, Encode, Codec};
 //! // in your off-chain worker code
 //!

From 973747330673073f95a4b785756024afb07d2995 Mon Sep 17 00:00:00 2001
From: Bernhard Schuster <bernhard@parity.io>
Date: Wed, 27 May 2020 10:00:18 +0200
Subject: [PATCH 36/44] fix/doctest: *

---
 primitives/runtime/src/offchain/storage_lock.rs | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/primitives/runtime/src/offchain/storage_lock.rs b/primitives/runtime/src/offchain/storage_lock.rs
index 87682fba48dc6..5b4fa9b79d23d 100644
--- a/primitives/runtime/src/offchain/storage_lock.rs
+++ b/primitives/runtime/src/offchain/storage_lock.rs
@@ -34,12 +34,14 @@
 //! ## Example:
 //!
 //! ```rust
-//! # use sp_core::offchain::storage::StorageValueRef;
-//! # use sp_runtime::offchain::storage_lock::Time;
 //! # use codec::{Decode, Encode, Codec};
 //! // in your off-chain worker code
+//! use sp_runtime::offchain::{
+//!		storage::StorageValueRef,
+//!		storage_lock::{StorageLock, Time},
+//! };
 //!
-//! fn append_to_in_storage_vec<'a, T>(key: &'a [u8], _: T) where T: Encode {
+//! fn append_to_in_storage_vec<'a, T>(key: &'a [u8], _: T) where T: Codec {
 //!    // `access::lock` defines the storage entry which is used for
 //!    // persisting the lock in the underlying database.
 //!    // The entry name _must_ be unique and can be interpreted as a
@@ -52,7 +54,7 @@
 //!         // modify `v` as desired
 //!         // i.e. perform some heavy computation with
 //!         // side effects that should only be done once.
-//!         acc.set(v);
+//!         acc.set(&v);
 //!         // drop `_guard` implicitly at end of scope
 //!    }
 //! }

From 2155ad86c5379af238c604f902933bc8cbab06c3 Mon Sep 17 00:00:00 2001
From: Bernhard Schuster <bernhard@parity.io>
Date: Thu, 28 May 2020 08:48:39 +0200
Subject: [PATCH 37/44] fix/review: remove unused trait bound

---
 frame/system/src/lib.rs | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/frame/system/src/lib.rs b/frame/system/src/lib.rs
index 0366880e639c5..b4f438ce03422 100644
--- a/frame/system/src/lib.rs
+++ b/frame/system/src/lib.rs
@@ -1269,10 +1269,7 @@ impl<T: Trait> Happened<T::AccountId> for CallKillAccount<T> {
 	}
 }
 
-impl<T> BlockNumberProvider for Module<T>
-where
-	T: crate::Trait,
-	<T as Trait>::BlockNumber: AtLeast32Bit,
+impl<T: Trait> BlockNumberProvider for Module<T>
 {
 	type BlockNumber = <T as Trait>::BlockNumber;
 

From 192df5707db47a5a6d678753450a55e68e46e796 Mon Sep 17 00:00:00 2001
From: Bernhard Schuster <bernhard@parity.io>
Date: Thu, 28 May 2020 08:48:58 +0200
Subject: [PATCH 38/44] fix/review: pretty by const fn

---
 primitives/core/src/offchain/mod.rs           |  2 +-
 .../runtime/src/offchain/storage_lock.rs      | 35 +++++++++++--------
 2 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/primitives/core/src/offchain/mod.rs b/primitives/core/src/offchain/mod.rs
index 8040c485222b4..b2ff3552135ce 100644
--- a/primitives/core/src/offchain/mod.rs
+++ b/primitives/core/src/offchain/mod.rs
@@ -222,7 +222,7 @@ pub struct Duration(u64);
 
 impl Duration {
 	/// Create new duration representing given number of milliseconds.
-	pub fn from_millis(millis: u64) -> Self {
+	pub const fn from_millis(millis: u64) -> Self {
 		Duration(millis)
 	}
 
diff --git a/primitives/runtime/src/offchain/storage_lock.rs b/primitives/runtime/src/offchain/storage_lock.rs
index 5b4fa9b79d23d..2debfe5c83927 100644
--- a/primitives/runtime/src/offchain/storage_lock.rs
+++ b/primitives/runtime/src/offchain/storage_lock.rs
@@ -67,13 +67,14 @@ use sp_core::offchain::{Duration, Timestamp};
 use sp_io::offchain;
 
 /// Default expiry duration for time based locks in milliseconds.
-const STORAGE_LOCK_DEFAULT_EXPIRY_DURATION_MS: u64 = 30_000;
+const STORAGE_LOCK_DEFAULT_EXPIRY_DURATION: Duration = Duration::from_millis(30_000);
 
 /// Default expiry duration for block based locks in blocks.
 const STORAGE_LOCK_DEFAULT_EXPIRY_BLOCKS: u32 = 4;
 
 /// Time between checks if the lock is still being held in milliseconds.
-const STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE: u64 = 100;
+const STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE_MIN: Duration = Duration::from_millis(100);
+const STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE_MAX: Duration = Duration::from_millis(10);
 
 /// Lockable item for use with a persisted storage lock.
 ///
@@ -96,9 +97,9 @@ pub trait Lockable: Sized {
 	/// Note that `deadline` is only passed to allow optimizations
 	/// for `Lockables` which have a time based component.
 	fn snooze(_deadline: &Self::Deadline) {
-		sp_io::offchain::sleep_until(offchain::timestamp().add(Duration::from_millis(
-			STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE,
-		)));
+		sp_io::offchain::sleep_until(offchain::timestamp().add(
+			STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE_MAX,
+		));
 	}
 }
 
@@ -113,7 +114,7 @@ pub struct Time {
 impl Default for Time {
 	fn default() -> Self {
 		Self {
-			expiration_duration: Duration::from_millis(STORAGE_LOCK_DEFAULT_EXPIRY_DURATION_MS),
+			expiration_duration: STORAGE_LOCK_DEFAULT_EXPIRY_DURATION,
 		}
 	}
 }
@@ -134,8 +135,12 @@ impl Lockable for Time {
 		let remainder: Duration = now.diff(&deadline);
 		// do not snooze the full duration, but instead snooze max 100ms
 		// it might get unlocked in another thread
-		let snooze = core::cmp::min(remainder.millis(), STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE);
-		sp_io::offchain::sleep_until(now.add(Duration::from_millis(snooze)));
+		use core::cmp::{min,max};
+		let snooze = max(
+			min(remainder, STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE_MAX),
+			STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE_MIN
+		);
+		sp_io::offchain::sleep_until(now.add(snooze));
 	}
 }
 
@@ -162,9 +167,7 @@ impl<B: BlockNumberProvider> Default for BlockAndTimeDeadline<B> {
 	fn default() -> Self {
 		Self {
 			block_number: B::current_block_number() + STORAGE_LOCK_DEFAULT_EXPIRY_BLOCKS.into(),
-			timestamp: offchain::timestamp().add(Duration::from_millis(
-				STORAGE_LOCK_DEFAULT_EXPIRY_DURATION_MS,
-			)),
+			timestamp: offchain::timestamp().add(STORAGE_LOCK_DEFAULT_EXPIRY_DURATION),
 		}
 	}
 }
@@ -189,7 +192,7 @@ impl<B: BlockNumberProvider> Default for BlockAndTime<B> {
 	fn default() -> Self {
 		Self {
 			expiration_block_number_offset: STORAGE_LOCK_DEFAULT_EXPIRY_BLOCKS,
-			expiration_duration: Duration::from_millis(STORAGE_LOCK_DEFAULT_EXPIRY_DURATION_MS),
+			expiration_duration: STORAGE_LOCK_DEFAULT_EXPIRY_DURATION,
 			_phantom: core::marker::PhantomData::<B>,
 		}
 	}
@@ -226,8 +229,12 @@ impl<B: BlockNumberProvider> Lockable for BlockAndTime<B> {
 	fn snooze(deadline: &Self::Deadline) {
 		let now = offchain::timestamp();
 		let remainder: Duration = now.diff(&(deadline.timestamp));
-		let snooze = core::cmp::min(remainder.millis(), STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE);
-		sp_io::offchain::sleep_until(now.add(Duration::from_millis(snooze)));
+		use core::cmp::{min,max};
+		let snooze = max(
+			min(remainder, STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE_MAX),
+			STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE_MIN
+		);
+		sp_io::offchain::sleep_until(now.add(snooze));
 	}
 }
 

From ef160b9dfa81e5402daa46d6c555fbc17b3536a3 Mon Sep 17 00:00:00 2001
From: Bernhard Schuster <bernhard@parity.io>
Date: Thu, 28 May 2020 08:51:16 +0200
Subject: [PATCH 39/44] fix/review: reduce default timeout to 20s

---
 primitives/runtime/src/offchain/storage_lock.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/primitives/runtime/src/offchain/storage_lock.rs b/primitives/runtime/src/offchain/storage_lock.rs
index 2debfe5c83927..37856928880d8 100644
--- a/primitives/runtime/src/offchain/storage_lock.rs
+++ b/primitives/runtime/src/offchain/storage_lock.rs
@@ -67,7 +67,7 @@ use sp_core::offchain::{Duration, Timestamp};
 use sp_io::offchain;
 
 /// Default expiry duration for time based locks in milliseconds.
-const STORAGE_LOCK_DEFAULT_EXPIRY_DURATION: Duration = Duration::from_millis(30_000);
+const STORAGE_LOCK_DEFAULT_EXPIRY_DURATION: Duration = Duration::from_millis(20_000);
 
 /// Default expiry duration for block based locks in blocks.
 const STORAGE_LOCK_DEFAULT_EXPIRY_BLOCKS: u32 = 4;

From 80abc9d4312985bd316a3ff3724e722e4a73fe57 Mon Sep 17 00:00:00 2001
From: Bernhard Schuster <bernhard@parity.io>
Date: Thu, 28 May 2020 08:51:30 +0200
Subject: [PATCH 40/44] docs: grammar

---
 primitives/runtime/src/offchain/storage_lock.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/primitives/runtime/src/offchain/storage_lock.rs b/primitives/runtime/src/offchain/storage_lock.rs
index 37856928880d8..d9c63e933610d 100644
--- a/primitives/runtime/src/offchain/storage_lock.rs
+++ b/primitives/runtime/src/offchain/storage_lock.rs
@@ -24,7 +24,7 @@
 //! restart or re-orgs.
 //!
 //! A use case for the lock is to make sure that a particular section of the
-//! code is only run by one Off-chain Worker at the time. This may include
+//! code is only run by one Off-chain Worker at a time. This may include
 //! performing a side-effect (i.e. an HTTP call) or alteration of single or
 //! multiple Local Storage entries.
 //!

From efab9cf858c8bc715314f6aca05ffeeae6d74356 Mon Sep 17 00:00:00 2001
From: Bernhard Schuster <bernhard@parity.io>
Date: Thu, 28 May 2020 08:51:41 +0200
Subject: [PATCH 41/44] fix/review: add with_block_deadline

---
 primitives/runtime/src/offchain/storage_lock.rs | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/primitives/runtime/src/offchain/storage_lock.rs b/primitives/runtime/src/offchain/storage_lock.rs
index d9c63e933610d..d2649dbc28ee1 100644
--- a/primitives/runtime/src/offchain/storage_lock.rs
+++ b/primitives/runtime/src/offchain/storage_lock.rs
@@ -381,6 +381,20 @@ where
 			},
 		}
 	}
+
+	pub fn with_block_deadline(
+		key: &'a [u8],
+		expiration_block_number_offset: u32,
+	) -> Self {
+		Self {
+			value_ref: StorageValueRef::<'a>::persistent(key),
+			lockable: BlockAndTime::<B> {
+				expiration_block_number_offset,
+				expiration_duration: STORAGE_LOCK_DEFAULT_EXPIRY_DURATION,
+				_phantom: core::marker::PhantomData,
+			},
+		}
+	}
 }
 
 /// Bound for block number sources which commonly is implemented

From ae0f8dc0791a352b8722f14f51ef51145c2edb4f Mon Sep 17 00:00:00 2001
From: Bernhard Schuster <bernhard@parity.io>
Date: Thu, 28 May 2020 09:02:30 +0200
Subject: [PATCH 42/44] doc: revamp BlockNumberProvider documentation to be
 less frame centric

---
 primitives/runtime/src/offchain/storage_lock.rs | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/primitives/runtime/src/offchain/storage_lock.rs b/primitives/runtime/src/offchain/storage_lock.rs
index d2649dbc28ee1..0ddadd6289119 100644
--- a/primitives/runtime/src/offchain/storage_lock.rs
+++ b/primitives/runtime/src/offchain/storage_lock.rs
@@ -397,17 +397,19 @@ where
 	}
 }
 
-/// Bound for block number sources which commonly is implemented
-/// by `frame_system::Module<T: Trait>`.
-///
-/// `BlockNumberProvider` has no intrinsic meaning and exists only to decouple `frame_system`
-/// from `runtime` crate and avoid a circular dependency issues.
+/// Bound for a block number source
+/// used with [`BlockAndTime<BlockNumberProvider>`](Self::BlockAndTime).
 pub trait BlockNumberProvider {
-	/// Type of `BlockNumber` which is going to be provided.
+	/// Type of `BlockNumber` to provide.
 	type BlockNumber: Codec + Clone + Ord + Eq + AtLeast32Bit;
 	/// Returns the current block number.
 	///
-	/// For `frame_system::Module<T: Trait>` it is implemented as
+	/// Provides an abstraction over an arbitrary way of providing the
+	/// current block number.
+	///
+	/// In case of using crate `sp_runtime` without the crate `frame`
+	/// system, it is already implemented for
+	/// `frame_system::Module<T: Trait>` as:
 	///
 	/// ```ignore
 	/// fn current_block_number() -> Self {

From 230c0d9aa7b0ef756fc35086c725e9871ea3a10d Mon Sep 17 00:00:00 2001
From: Bernhard Schuster <bernhard@parity.io>
Date: Thu, 28 May 2020 09:03:19 +0200
Subject: [PATCH 43/44] chore: fmt

---
 primitives/runtime/src/offchain/storage_lock.rs | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/primitives/runtime/src/offchain/storage_lock.rs b/primitives/runtime/src/offchain/storage_lock.rs
index 0ddadd6289119..a687323195641 100644
--- a/primitives/runtime/src/offchain/storage_lock.rs
+++ b/primitives/runtime/src/offchain/storage_lock.rs
@@ -97,9 +97,9 @@ pub trait Lockable: Sized {
 	/// Note that `deadline` is only passed to allow optimizations
 	/// for `Lockables` which have a time based component.
 	fn snooze(_deadline: &Self::Deadline) {
-		sp_io::offchain::sleep_until(offchain::timestamp().add(
-			STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE_MAX,
-		));
+		sp_io::offchain::sleep_until(
+			offchain::timestamp().add(STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE_MAX),
+		);
 	}
 }
 
@@ -135,10 +135,10 @@ impl Lockable for Time {
 		let remainder: Duration = now.diff(&deadline);
 		// do not snooze the full duration, but instead snooze max 100ms
 		// it might get unlocked in another thread
-		use core::cmp::{min,max};
+		use core::cmp::{max, min};
 		let snooze = max(
 			min(remainder, STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE_MAX),
-			STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE_MIN
+			STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE_MIN,
 		);
 		sp_io::offchain::sleep_until(now.add(snooze));
 	}
@@ -229,10 +229,10 @@ impl<B: BlockNumberProvider> Lockable for BlockAndTime<B> {
 	fn snooze(deadline: &Self::Deadline) {
 		let now = offchain::timestamp();
 		let remainder: Duration = now.diff(&(deadline.timestamp));
-		use core::cmp::{min,max};
+		use core::cmp::{max, min};
 		let snooze = max(
 			min(remainder, STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE_MAX),
-			STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE_MIN
+			STORAGE_LOCK_PER_CHECK_ITERATION_SNOOZE_MIN,
 		);
 		sp_io::offchain::sleep_until(now.add(snooze));
 	}

From 8e425ac16b7abe3b87953c7d45d2cf515d2fd47c Mon Sep 17 00:00:00 2001
From: Bernhard Schuster <bernhard@parity.io>
Date: Thu, 28 May 2020 09:46:21 +0200
Subject: [PATCH 44/44] docs: add missing doc comment

---
 primitives/runtime/src/offchain/storage_lock.rs | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/primitives/runtime/src/offchain/storage_lock.rs b/primitives/runtime/src/offchain/storage_lock.rs
index a687323195641..60bf9f04772dd 100644
--- a/primitives/runtime/src/offchain/storage_lock.rs
+++ b/primitives/runtime/src/offchain/storage_lock.rs
@@ -382,10 +382,9 @@ where
 		}
 	}
 
-	pub fn with_block_deadline(
-		key: &'a [u8],
-		expiration_block_number_offset: u32,
-	) -> Self {
+	/// Explicitly create a time and block number based storage lock with
+	/// the default expiration duration and a non-default block number offset.
+	pub fn with_block_deadline(key: &'a [u8], expiration_block_number_offset: u32) -> Self {
 		Self {
 			value_ref: StorageValueRef::<'a>::persistent(key),
 			lockable: BlockAndTime::<B> {