From 95d73994c1484b601819de651d1537303431912a Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Thu, 28 May 2020 14:26:11 +0200 Subject: [PATCH] offchain storage lock (#6010) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat/offchain/storage: add remove interface method * feat/offchain/storeage: add remote to StorageValueRef * feat/offchain/storage: add storage lock * fix/review: Apply suggestions from code review Co-authored-by: Tomasz Drwięga Co-authored-by: Peter Goodspeed-Niklaus * refactor/offchain/storage/lock: introduce `Lockable` trait part 1 of 2 * chore/offchain/rename: _remove -> clean * feat/offchain/storage/lock: add TimeAndBlock based part 2 of 2 * fix/offchain/storage/lock: block and time expiry must be && not || * chore/offchain/storage: minor fmt doc comments * doc/comment: prefer markdown emphasis over CAPS * doc/comment: rewrap multiline module level docs * doc/comment: rephrase * impl sleep_until and use the actual time for the test env * feat/test: add more tests, ignore some sample impl doctests * fix/review: Apply suggestions from code review Co-authored-by: Nikolay Volf * doc/comment: better description * fix/review: Apply suggestions from code review Co-authored-by: Nikolay Volf * chore/storage: lifetime cleanup * fix/cleanup: trait bounds, cargo-spellcheck + extra explanations * fix/doc: periods +- * fix/review: Apply suggestions from code review Co-authored-by: Tomasz Drwięga * cleanup: remove explicit lifetime bound, copy -> clone * fix/review: make trait Lockable contain only static, try_lock should not return Err(Option), * chore/lifetimes: remove a couple of lifetime bounds which the compiler can figure out * refactor: migrate to an instant based * fix/feedback: fix, reduce, rename, docs update pending * docs/reword: adjust to changed code * fix/offchain/testing: timestamp and sleep_until shall not block * chore/lines: lines must < 100 chars * fix/docs: add missing pub field doc comments * refactor/x: try_lock does not need to return an Option<_> * refactor/simplify: a better way of waiting for a lock to resolve * docs: consistency * fix/line: < 100 * fix/doctest/use: avoid crate:: * fix/doctest: * * fix/review: remove unused trait bound * fix/review: pretty by const fn * fix/review: reduce default timeout to 20s * docs: grammar * fix/review: add with_block_deadline * doc: revamp BlockNumberProvider documentation to be less frame centric * chore: fmt * docs: add missing doc comment Co-authored-by: Bernhard Schuster Co-authored-by: Tomasz Drwięga Co-authored-by: Peter Goodspeed-Niklaus Co-authored-by: Nikolay Volf --- substrate/client/db/src/offchain.rs | 8 + substrate/client/offchain/src/api.rs | 7 + substrate/frame/system/src/lib.rs | 10 + substrate/primitives/core/src/offchain/mod.rs | 26 +- .../primitives/core/src/offchain/storage.rs | 5 + .../primitives/core/src/offchain/testing.rs | 18 +- substrate/primitives/io/src/lib.rs | 10 + .../primitives/runtime/src/offchain/mod.rs | 1 + .../runtime/src/offchain/storage.rs | 8 +- .../runtime/src/offchain/storage_lock.rs | 516 ++++++++++++++++++ 10 files changed, 599 insertions(+), 10 deletions(-) create mode 100644 substrate/primitives/runtime/src/offchain/storage_lock.rs diff --git a/substrate/client/db/src/offchain.rs b/substrate/client/db/src/offchain.rs index 651510d6e8..f6a0925a08 100644 --- a/substrate/client/db/src/offchain.rs +++ b/substrate/client/db/src/offchain.rs @@ -67,6 +67,14 @@ impl sp_core::offchain::OffchainStorage for LocalStorage { self.db.commit(tx); } + fn remove(&mut self, prefix: &[u8], key: &[u8]) { + let key: Vec = 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> { let key: Vec = prefix.iter().chain(key).cloned().collect(); self.db.get(columns::OFFCHAIN, &key) diff --git a/substrate/client/offchain/src/api.rs b/substrate/client/offchain/src/api.rs index 45a82d230c..a7f4ecbc58 100644 --- a/substrate/client/offchain/src/api.rs +++ b/substrate/client/offchain/src/api.rs @@ -100,6 +100,13 @@ impl OffchainExt for Api { } } + 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), + } + } + fn local_storage_compare_and_set( &mut self, kind: StorageKind, diff --git a/substrate/frame/system/src/lib.rs b/substrate/frame/system/src/lib.rs index 8360f6c4cb..2221b0591d 100644 --- a/substrate/frame/system/src/lib.rs +++ b/substrate/frame/system/src/lib.rs @@ -114,6 +114,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}; @@ -1268,6 +1269,15 @@ impl Happened for CallKillAccount { } } +impl BlockNumberProvider for Module +{ + type BlockNumber = ::BlockNumber; + + fn current_block_number() -> Self::BlockNumber { + Module::::block_number() + } +} + // 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/substrate/primitives/core/src/offchain/mod.rs b/substrate/primitives/core/src/offchain/mod.rs index 1d77e10f59..b2ff355213 100644 --- a/substrate/primitives/core/src/offchain/mod.rs +++ b/substrate/primitives/core/src/offchain/mod.rs @@ -37,6 +37,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]); + /// 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. fn get(&self, prefix: &[u8], key: &[u8]) -> Option>; @@ -219,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) } @@ -346,9 +349,15 @@ 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. + fn local_storage_clear(&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 @@ -357,7 +366,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, @@ -370,7 +379,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>; /// Initiates a http request given HTTP verb and the URL. @@ -513,6 +522,10 @@ impl Externalities for Box { (&mut **self).local_storage_set(kind, key, value) } + fn local_storage_clear(&mut self, kind: StorageKind, key: &[u8]) { + (&mut **self).local_storage_clear(kind, key) + } + fn local_storage_compare_and_set( &mut self, kind: StorageKind, @@ -618,6 +631,11 @@ impl Externalities for LimitedExternalities { self.externalities.local_storage_set(kind, key, value) } + 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( &mut self, kind: StorageKind, diff --git a/substrate/primitives/core/src/offchain/storage.rs b/substrate/primitives/core/src/offchain/storage.rs index 1826015b0d..52a7bbe857 100644 --- a/substrate/primitives/core/src/offchain/storage.rs +++ b/substrate/primitives/core/src/offchain/storage.rs @@ -51,6 +51,11 @@ impl OffchainStorage for InMemOffchainStorage { self.storage.insert(key, value.to_vec()); } + fn remove(&mut self, prefix: &[u8], key: &[u8]) { + let key: Vec = prefix.iter().chain(key).cloned().collect(); + self.storage.remove(&key); + } + fn get(&self, prefix: &[u8], key: &[u8]) -> Option> { let key: Vec = prefix.iter().chain(key).cloned().collect(); self.storage.get(&key).cloned() diff --git a/substrate/primitives/core/src/offchain/testing.rs b/substrate/primitives/core/src/offchain/testing.rs index 5e25e433a3..76cf8915f2 100644 --- a/substrate/primitives/core/src/offchain/testing.rs +++ b/substrate/primitives/core/src/offchain/testing.rs @@ -73,10 +73,10 @@ 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], + /// A timestamp simulating the current time. + pub timestamp: Timestamp, } impl OffchainState { @@ -160,11 +160,11 @@ impl offchain::Externalities for TestOffchainExt { } fn timestamp(&mut self) -> Timestamp { - Timestamp::from_unix_millis(self.0.read().timestamp) + self.0.read().timestamp } - fn sleep_until(&mut self, _deadline: Timestamp) { - unimplemented!("not needed in tests so far") + fn sleep_until(&mut self, deadline: Timestamp) { + self.0.write().timestamp = deadline; } fn random_seed(&mut self) -> [u8; 32] { @@ -179,6 +179,14 @@ impl offchain::Externalities for TestOffchainExt { }.set(b"", key, value); } + fn local_storage_clear(&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/substrate/primitives/io/src/lib.rs b/substrate/primitives/io/src/lib.rs index f28f3e2c95..8d81a84c4c 100644 --- a/substrate/primitives/io/src/lib.rs +++ b/substrate/primitives/io/src/lib.rs @@ -796,6 +796,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_clear(&mut self, kind: StorageKind, key: &[u8]) { + self.extension::() + .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. /// /// Since multiple offchain workers may be running concurrently, to prevent diff --git a/substrate/primitives/runtime/src/offchain/mod.rs b/substrate/primitives/runtime/src/offchain/mod.rs index 427b54468f..fe5844ce30 100644 --- a/substrate/primitives/runtime/src/offchain/mod.rs +++ b/substrate/primitives/runtime/src/offchain/mod.rs @@ -19,5 +19,6 @@ pub mod http; pub mod storage; +pub mod storage_lock; pub use sp_core::offchain::*; diff --git a/substrate/primitives/runtime/src/offchain/storage.rs b/substrate/primitives/runtime/src/offchain/storage.rs index f8dcd73fa2..2f62d400c0 100644 --- a/substrate/primitives/runtime/src/offchain/storage.rs +++ b/substrate/primitives/runtime/src/offchain/storage.rs @@ -50,6 +50,11 @@ impl<'a> StorageValueRef<'a> { }) } + /// Remove the associated value from the storage. + pub fn clear(&mut self) { + sp_io::offchain::local_storage_clear(self.kind, self.key) + } + /// Retrieve & decode the value from storage. /// /// Note that if you want to do some checks based on the value @@ -67,7 +72,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(&self, f: F) -> Result, E> where T: codec::Codec, diff --git a/substrate/primitives/runtime/src/offchain/storage_lock.rs b/substrate/primitives/runtime/src/offchain/storage_lock.rs new file mode 100644 index 0000000000..60bf9f0477 --- /dev/null +++ b/substrate/primitives/runtime/src/offchain/storage_lock.rs @@ -0,0 +1,516 @@ +// 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 . + +//! # Off-chain Storage Lock +//! +//! 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 across 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 a 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 off-chain storage DB. +//! +//! ## Example: +//! +//! ```rust +//! # 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: 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 +//! // unique mutex instance reference tag. +//! let mut lock = StorageLock::