Allow transaction for offchain indexing (#7290)

* Moving offchain change set to state machine overlay change set,
preparing use of change set internally.

* Make change set generic over key and value, and use it for offchain
indexing.

* test ui change

* remaining delta

* generating with standard method

* Remove 'drain_committed' function, and documentation.

* Default constructor for enabling offchain indexing.

* Remove offchain change specific iterators.

* remove pub accessor

* keep previous hierarchy, just expose iterator instead.

* Update primitives/state-machine/src/overlayed_changes/mod.rs

Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com>

* fix line break

* missing renamings

* fix import

* fix new state-machine tests.

* Don't expose InnerValue type.

* Add test similar to set_storage.

* Remove conditional offchain storage (hard to instantiate correctly).

* fix

* offchain as children cannot fail if top doesn't

Co-authored-by: Addie Wagenknecht <addie@nortd.com>
Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com>

Author: 3 people

client/api/src/backend.rs

@@ -21,12 +21,12 @@
 use std::sync::Arc;
 use std::collections::{HashMap, HashSet};
 use sp_core::ChangesTrieConfigurationRange;
-use sp_core::offchain::{OffchainStorage,storage::OffchainOverlayedChanges};
+use sp_core::offchain::OffchainStorage;
 use sp_runtime::{generic::BlockId, Justification, Storage};
 use sp_runtime::traits::{Block as BlockT, NumberFor, HashFor};
 use sp_state_machine::{
 	ChangesTrieState, ChangesTrieStorage as StateChangesTrieStorage, ChangesTrieTransaction,
-	StorageCollection, ChildStorageCollection,
+	StorageCollection, ChildStorageCollection, OffchainChangesCollection,
 };
 use sp_storage::{StorageData, StorageKey, PrefixedStorageKey, ChildInfo};
 use crate::{
@@ -174,7 +174,7 @@ pub trait BlockImportOperation<Block: BlockT> {
 	/// Write offchain storage changes to the database.
 	fn update_offchain_storage(
 		&mut self,
-		_offchain_update: OffchainOverlayedChanges,
+		_offchain_update: OffchainChangesCollection,
 	) -> sp_blockchain::Result<()> {
 		 Ok(())
 	}

client/db/src/lib.rs

@@ -67,7 +67,7 @@ use hash_db::Prefix;
 use sp_trie::{MemoryDB, PrefixedMemoryDB, prefixed_key};
 use sp_database::Transaction;
 use sp_core::{Hasher, ChangesTrieConfiguration};
-use sp_core::offchain::storage::{OffchainOverlayedChange, OffchainOverlayedChanges};
+use sp_core::offchain::OffchainOverlayedChange;
 use sp_core::storage::{well_known_keys, ChildInfo};
 use sp_arithmetic::traits::Saturating;
 use sp_runtime::{generic::{DigestItem, BlockId}, Justification, Storage};
@@ -76,7 +76,7 @@ use sp_runtime::traits::{
 };
 use sp_state_machine::{
 	DBValue, ChangesTrieTransaction, ChangesTrieCacheAction, UsageInfo as StateUsageInfo,
-	StorageCollection, ChildStorageCollection,
+	StorageCollection, ChildStorageCollection, OffchainChangesCollection,
 	backend::Backend as StateBackend, StateMachineStats,
 };
 use crate::utils::{DatabaseType, Meta, meta_keys, read_db, read_meta};
@@ -667,7 +667,7 @@ pub struct BlockImportOperation<Block: BlockT> {
 	db_updates: PrefixedMemoryDB<HashFor<Block>>,
 	storage_updates: StorageCollection,
 	child_storage_updates: ChildStorageCollection,
-	offchain_storage_updates: OffchainOverlayedChanges,
+	offchain_storage_updates: OffchainChangesCollection,
 	changes_trie_updates: MemoryDB<HashFor<Block>>,
 	changes_trie_build_cache_update: Option<ChangesTrieCacheAction<Block::Hash, NumberFor<Block>>>,
 	changes_trie_config_update: Option<Option<ChangesTrieConfiguration>>,
@@ -680,7 +680,7 @@ pub struct BlockImportOperation<Block: BlockT> {
 
 impl<Block: BlockT> BlockImportOperation<Block> {
 	fn apply_offchain(&mut self, transaction: &mut Transaction<DbHash>) {
-		for ((prefix, key), value_operation) in self.offchain_storage_updates.drain() {
+		for ((prefix, key), value_operation) in self.offchain_storage_updates.drain(..) {
 			let key = crate::offchain::concatenate_prefix_and_key(&prefix, &key);
 			match value_operation {
 				OffchainOverlayedChange::SetValue(val) =>
@@ -798,7 +798,7 @@ impl<Block: BlockT> sc_client_api::backend::BlockImportOperation<Block> for Bloc
 
 	fn update_offchain_storage(
 		&mut self,
-		offchain_update: OffchainOverlayedChanges,
+		offchain_update: OffchainChangesCollection,
 	) -> ClientResult<()> {
 		self.offchain_storage_updates = offchain_update;
 		Ok(())

client/executor/src/integration_tests/mod.rs

@@ -475,13 +475,11 @@ fn offchain_index(wasm_method: WasmExecutionMethod) {
 		&mut ext.ext(),
 	).unwrap();
 
-	use sp_core::offchain::storage::OffchainOverlayedChange;
-	assert_eq!(
-		ext.overlayed_changes()
-			.offchain_overlay()
-			.get(sp_core::offchain::STORAGE_PREFIX, b"k"),
-		Some(OffchainOverlayedChange::SetValue(b"v".to_vec()))
-	);
+	use sp_core::offchain::OffchainOverlayedChange;
+	let data = ext.overlayed_changes().clone().offchain_drain_committed().find(|(k, _v)| {
+		k == &(sp_core::offchain::STORAGE_PREFIX.to_vec(), b"k".to_vec())
+	});
+	assert_eq!(data.map(|data| data.1), Some(OffchainOverlayedChange::SetValue(b"v".to_vec())));
 }
 
 test_wasm_execution!(offchain_local_storage_should_work);

primitives/core/src/offchain/mod.rs

@@ -746,6 +746,14 @@ impl TransactionPoolExt {
 	}
 }
 
+/// Change to be applied to the offchain worker db in regards to a key.
+#[derive(Debug, Clone, Hash, Eq, PartialEq)]
+pub enum OffchainOverlayedChange {
+	/// Remove the data associated with the key
+	Remove,
+	/// Overwrite the value of an associated key
+	SetValue(Vec<u8>),
+}
 
 #[cfg(test)]
 mod tests {

primitives/core/src/offchain/storage.rs

@@ -83,98 +83,3 @@ impl OffchainStorage for InMemOffchainStorage {
 		}
 	}
 }
-
-/// Change to be applied to the offchain worker db in regards to a key.
-#[derive(Debug,Clone,Hash,Eq,PartialEq)]
-pub enum OffchainOverlayedChange {
-	/// Remove the data associated with the key
-	Remove,
-	/// Overwrite the value of an associated key
-	SetValue(Vec<u8>),
-}
-
-/// In-memory storage for offchain workers recoding changes for the actual offchain storage implementation.
-#[derive(Debug, Clone, Default)]
-pub struct OffchainOverlayedChanges {
-	changes: HashMap<(Vec<u8>, Vec<u8>), OffchainOverlayedChange>,
-}
-
-impl OffchainOverlayedChanges {
-	/// Consume the offchain storage and iterate over all key value pairs.
-	pub fn into_iter(self) -> impl Iterator<Item = ((Vec<u8>, Vec<u8>), OffchainOverlayedChange)> {
-		self.changes.into_iter()
-	}
-
-	/// Iterate over all key value pairs by reference.
-	pub fn iter(&self) -> impl Iterator<Item = (&(Vec<u8>, Vec<u8>), &OffchainOverlayedChange)> {
-		self.changes.iter()
-	}
-
-	/// Drain all elements of changeset.
-	pub fn drain(&mut self) -> impl Iterator<Item = ((Vec<u8>, Vec<u8>), OffchainOverlayedChange)> + '_ {
-		self.changes.drain()
-	}
-
-	/// Remove a key and its associated value from the offchain database.
-	pub fn remove(&mut self, prefix: &[u8], key: &[u8]) {
-		self.changes.insert((prefix.to_vec(), key.to_vec()), OffchainOverlayedChange::Remove);
-	}
-
-	/// Set the value associated with a key under a prefix to the value provided.
-	pub fn set(&mut self, prefix: &[u8], key: &[u8], value: &[u8]) {
-		self.changes.insert(
-			(prefix.to_vec(), key.to_vec()),
-			OffchainOverlayedChange::SetValue(value.to_vec()),
-		);
-	}
-
-	/// Obtain a associated value to the given key in storage with prefix.
-	pub fn get(&self, prefix: &[u8], key: &[u8]) -> Option<OffchainOverlayedChange> {
-		let key = (prefix.to_vec(), key.to_vec());
-		self.changes.get(&key).cloned()
-	}
-}
-
-#[cfg(test)]
-mod test {
-	use super::*;
-	use super::super::STORAGE_PREFIX;
-
-	#[test]
-	fn test_drain() {
-		let mut ooc = OffchainOverlayedChanges::default();
-		ooc.set(STORAGE_PREFIX,b"kkk", b"vvv");
-		let drained = ooc.drain().count();
-		assert_eq!(drained, 1);
-		let leftover = ooc.iter().count();
-		assert_eq!(leftover, 0);
-
-		ooc.set(STORAGE_PREFIX, b"a", b"v");
-		ooc.set(STORAGE_PREFIX, b"b", b"v");
-		ooc.set(STORAGE_PREFIX, b"c", b"v");
-		ooc.set(STORAGE_PREFIX, b"d", b"v");
-		ooc.set(STORAGE_PREFIX, b"e", b"v");
-		assert_eq!(ooc.iter().count(), 5);
-	}
-
-	#[test]
-	fn test_accumulated_set_remove_set() {
-		let mut ooc = OffchainOverlayedChanges::default();
-		ooc.set(STORAGE_PREFIX, b"ppp", b"qqq");
-		ooc.remove(STORAGE_PREFIX, b"ppp");
-		// keys are equiv, so it will overwrite the value and the overlay will contain
-		// one item
-		assert_eq!(ooc.iter().count(), 1);
-
-		ooc.set(STORAGE_PREFIX, b"ppp", b"rrr");
-		let mut iter = ooc.into_iter();
-		assert_eq!(
-			iter.next(),
-			Some(
-				((STORAGE_PREFIX.to_vec(), b"ppp".to_vec()),
-				OffchainOverlayedChange::SetValue(b"rrr".to_vec()))
-			)
-		);
-		assert_eq!(iter.next(), None);
-	}
-}

primitives/core/src/offchain/testing.rs

@@ -27,7 +27,8 @@ use std::{
 use crate::OpaquePeerId;
 use crate::offchain::{
 	self,
-	storage::{InMemOffchainStorage, OffchainOverlayedChange, OffchainOverlayedChanges},
+	OffchainOverlayedChange,
+	storage::InMemOffchainStorage,
 	HttpError,
 	HttpRequestId as RequestId,
 	HttpRequestStatus as RequestStatus,
@@ -80,9 +81,12 @@ impl TestPersistentOffchainDB {
 	}
 
 	/// Apply a set of off-chain changes directly to the test backend
-	pub fn apply_offchain_changes(&mut self, changes: &mut OffchainOverlayedChanges) {
+	pub fn apply_offchain_changes(
+		&mut self,
+		changes: impl Iterator<Item = ((Vec<u8>, Vec<u8>), OffchainOverlayedChange)>,
+	) {
 		let mut me = self.persistent.write();
-		for ((_prefix, key), value_operation) in changes.drain() {
+		for ((_prefix, key), value_operation) in changes {
 			match value_operation {
 				OffchainOverlayedChange::SetValue(val) => me.set(Self::PREFIX, key.as_slice(), val.as_slice()),
 				OffchainOverlayedChange::Remove => me.remove(Self::PREFIX, key.as_slice()),

primitives/offchain/src/lib.rs

@@ -21,7 +21,7 @@
 #![warn(missing_docs)]
 
 /// Re-export of parent module scope storage prefix.
-pub use sp_core::offchain::STORAGE_PREFIX as STORAGE_PREFIX;
+pub use sp_core::offchain::STORAGE_PREFIX;
 
 sp_api::decl_runtime_apis! {
 	/// The offchain worker api.

primitives/state-machine/src/ext.rs

@@ -193,18 +193,10 @@ where
 	B: Backend<H>,
 	N: crate::changes_trie::BlockNumber,
 {
-	#[cfg(feature = "std")]
 	fn set_offchain_storage(&mut self, key: &[u8], value: Option<&[u8]>) {
-		use sp_core::offchain::STORAGE_PREFIX;
-		match value {
-			Some(value) => self.overlay.offchain_set_storage(STORAGE_PREFIX, key, value),
-			None => self.overlay.offchain_remove_storage(STORAGE_PREFIX, key),
-		}
+		self.overlay.set_offchain_storage(key, value)
 	}
 
-	#[cfg(not(feature = "std"))]
-	fn set_offchain_storage(&mut self, _key: &[u8], _value: Option<&[u8]>) {}
-
 	fn storage(&self, key: &[u8]) -> Option<StorageValue> {
 		let _guard = guard();
 		let result = self.overlay.storage(key).map(|x| x.map(|x| x.to_vec())).unwrap_or_else(||
@@ -790,7 +782,6 @@ mod tests {
 		H256,
 		Blake2Hasher,
 		map,
-		offchain,
 		storage::{
 			Storage,
 			StorageChild,
@@ -813,14 +804,11 @@ mod tests {
 		changes.set_extrinsic_index(1);
 		changes.set_storage(vec![1], Some(vec![100]));
 		changes.set_storage(EXTRINSIC_INDEX.to_vec(), Some(3u32.encode()));
+		changes.set_offchain_storage(b"k1", Some(b"v1"));
+		changes.set_offchain_storage(b"k2", Some(b"v2"));
 		changes
 	}
 
-	fn prepare_offchain_overlay_with_changes(overlay: &mut OverlayedChanges) {
-		overlay.offchain_set_storage(offchain::STORAGE_PREFIX, b"k1", b"v1");
-		overlay.offchain_set_storage(offchain::STORAGE_PREFIX, b"k2", b"v2");
-	}
-
 	fn changes_trie_config() -> ChangesTrieConfiguration {
 		ChangesTrieConfiguration {
 			digest_interval: 0,
@@ -849,7 +837,6 @@ mod tests {
 	#[test]
 	fn storage_changes_root_is_some_when_extrinsic_changes_are_non_empty() {
 		let mut overlay = prepare_overlay_with_changes();
-		prepare_offchain_overlay_with_changes(&mut overlay);
 		let mut cache = StorageTransactionCache::default();
 		let storage = TestChangesTrieStorage::with_blocks(vec![(99, Default::default())]);
 		let state = Some(ChangesTrieState::new(changes_trie_config(), Zero::zero(), &storage));
@@ -864,7 +851,6 @@ mod tests {
 	#[test]
 	fn storage_changes_root_is_some_when_extrinsic_changes_are_empty() {
 		let mut overlay = prepare_overlay_with_changes();
-		prepare_offchain_overlay_with_changes(&mut overlay);
 		let mut cache = StorageTransactionCache::default();
 		overlay.set_collect_extrinsics(false);
 		overlay.set_storage(vec![1], None);
@@ -884,7 +870,6 @@ mod tests {
 		let mut overlay = OverlayedChanges::default();
 		overlay.set_storage(vec![20], None);
 		overlay.set_storage(vec![30], Some(vec![31]));
-		prepare_offchain_overlay_with_changes(&mut overlay);
 		let backend = Storage {
 			top: map![
 				vec![10] => vec![10],
@@ -939,8 +924,6 @@ mod tests {
 			],
 		}.into();
 
-		prepare_offchain_overlay_with_changes(&mut overlay);
-
 		let ext = TestExt::new(&mut overlay, &mut cache, &backend, None, None);
 
 		// next_backend < next_overlay
@@ -971,7 +954,6 @@ mod tests {
 		let mut overlay = OverlayedChanges::default();
 		overlay.set_child_storage(child_info, vec![20], None);
 		overlay.set_child_storage(child_info, vec![30], Some(vec![31]));
-		prepare_offchain_overlay_with_changes(&mut overlay);
 		let backend = Storage {
 			top: map![],
 			children_default: map![
@@ -1013,7 +995,6 @@ mod tests {
 		let child_info = &child_info;
 		let mut cache = StorageTransactionCache::default();
 		let mut overlay = OverlayedChanges::default();
-		prepare_offchain_overlay_with_changes(&mut overlay);
 		let backend = Storage {
 			top: map![],
 			children_default: map![

primitives/state-machine/src/lib.rs

@@ -119,6 +119,8 @@ pub use crate::overlayed_changes::{
 	OverlayedChanges, StorageKey, StorageValue,
 	StorageCollection, ChildStorageCollection,
 	StorageChanges, StorageTransactionCache,
+	OffchainChangesCollection,
+	OffchainOverlayedChanges,
 };
 pub use crate::backend::Backend;
 pub use crate::trie_backend_essence::{TrieBackendStorage, Storage};