From 5b3d0b5ca8450338b078a177921b83ca84270a10 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Mon, 25 May 2020 10:53:20 +0200 Subject: [PATCH 01/38] Add transactional storage functionality to OverlayChanges A collection already has a natural None state. No need to wrap it with an option. --- Cargo.lock | 28 +- .../api/proc-macro/src/impl_runtime_apis.rs | 8 +- primitives/state-machine/Cargo.toml | 3 + .../state-machine/src/changes_trie/build.rs | 33 +- primitives/state-machine/src/ext.rs | 25 +- primitives/state-machine/src/lib.rs | 26 +- .../state-machine/src/overlayed_changes.rs | 668 ++++++------------ .../src/overlayed_changes/changeset.rs | 635 +++++++++++++++++ primitives/state-machine/src/testing.rs | 6 +- 9 files changed, 917 insertions(+), 515 deletions(-) create mode 100644 primitives/state-machine/src/overlayed_changes/changeset.rs diff --git a/Cargo.lock b/Cargo.lock index 2e7fbb14cf9e9..9116586cc1dec 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -819,7 +819,7 @@ dependencies = [ "clap", "criterion-plot 0.3.1", "csv", - "itertools", + "itertools 0.8.2", "lazy_static", "libc", "num-traits 0.2.11", @@ -846,7 +846,7 @@ dependencies = [ "clap", "criterion-plot 0.4.1", "csv", - "itertools", + "itertools 0.8.2", "lazy_static", "num-traits 0.2.11", "oorandom", @@ -868,7 +868,7 @@ checksum = "76f9212ddf2f4a9eb2d401635190600656a1f88a932ef53d06e7fa4c7e02fb8e" dependencies = [ "byteorder", "cast", - "itertools", + "itertools 0.8.2", ] [[package]] @@ -878,7 +878,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a01e15e0ea58e8234f96146b1f91fa9d0e4dd7a38da93ff7a75d42c0b9d3a545" dependencies = [ "cast", - "itertools", + "itertools 0.8.2", ] [[package]] @@ -2293,6 +2293,15 @@ dependencies = [ "either", ] +[[package]] +name = "itertools" +version = "0.9.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "284f18f85651fe11e8a991b2adb42cb078325c996ed026d994719efcfca1d54b" +dependencies = [ + "either", +] + [[package]] name = "itoa" version = "0.4.5" @@ -5093,7 +5102,7 @@ checksum = "02b10678c913ecbd69350e8535c3aef91a8676c0773fc1d7b95cdd196d7f2f26" dependencies = [ "bytes 0.5.4", "heck", - "itertools", + "itertools 0.8.2", "log", "multimap", "petgraph", @@ -5110,7 +5119,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "537aa19b95acde10a12fec4301466386f757403de4cd4e5b4fa78fb5ecb18f72" dependencies = [ "anyhow", - "itertools", + "itertools 0.8.2", "proc-macro2", "quote 1.0.3", "syn 1.0.17", @@ -7675,11 +7684,14 @@ version = "0.8.0-rc2" dependencies = [ "hash-db", "hex-literal", + "itertools 0.9.0", "log", "num-traits 0.2.11", "parity-scale-codec", "parking_lot 0.10.2", + "pretty_assertions", "rand 0.7.3", + "smallvec 1.4.0", "sp-core", "sp-externalities", "sp-panic-handler", @@ -7915,7 +7927,7 @@ dependencies = [ "hex", "hex-literal", "hyper 0.12.35", - "itertools", + "itertools 0.8.2", "jsonrpc-core-client", "libp2p", "node-primitives", @@ -8140,7 +8152,7 @@ dependencies = [ "build-helper", "cargo_metadata", "fs2", - "itertools", + "itertools 0.8.2", "tempfile", "toml", "walkdir", diff --git a/primitives/api/proc-macro/src/impl_runtime_apis.rs b/primitives/api/proc-macro/src/impl_runtime_apis.rs index 2878bd2c13683..d1d2ca3b57f85 100644 --- a/primitives/api/proc-macro/src/impl_runtime_apis.rs +++ b/primitives/api/proc-macro/src/impl_runtime_apis.rs @@ -257,6 +257,7 @@ fn generate_runtime_api_base_structures() -> Result { &self, map_call: F, ) -> std::result::Result where Self: Sized { + self.changes.borrow_mut().start_transaction(); *self.commit_on_success.borrow_mut() = false; let res = map_call(self); *self.commit_on_success.borrow_mut() = true; @@ -366,6 +367,9 @@ fn generate_runtime_api_base_structures() -> Result { &self, call_api_at: F, ) -> std::result::Result<#crate_::NativeOrEncoded, E> { + if *self.commit_on_success.borrow() { + self.changes.borrow_mut().start_transaction(); + } let res = call_api_at( &self.call, self, @@ -383,9 +387,9 @@ fn generate_runtime_api_base_structures() -> Result { fn commit_on_ok(&self, res: &std::result::Result) { if *self.commit_on_success.borrow() { if res.is_err() { - self.changes.borrow_mut().discard_prospective(); + self.changes.borrow_mut().rollback_transaction(); } else { - self.changes.borrow_mut().commit_prospective(); + self.changes.borrow_mut().commit_transaction(); } } } diff --git a/primitives/state-machine/Cargo.toml b/primitives/state-machine/Cargo.toml index 6a2653bb5bf32..095ea6b37aec5 100644 --- a/primitives/state-machine/Cargo.toml +++ b/primitives/state-machine/Cargo.toml @@ -25,10 +25,13 @@ codec = { package = "parity-scale-codec", version = "1.3.0" } num-traits = "0.2.8" rand = "0.7.2" sp-externalities = { version = "0.8.0-rc2", path = "../externalities" } +itertools = "0.9" +smallvec = "1.4" [dev-dependencies] hex-literal = "0.2.1" sp-runtime = { version = "2.0.0-rc2", path = "../runtime" } +pretty_assertions = "0.6.1" [features] default = [] diff --git a/primitives/state-machine/src/changes_trie/build.rs b/primitives/state-machine/src/changes_trie/build.rs index f9698f1a31dbf..5a4db0c656033 100644 --- a/primitives/state-machine/src/changes_trie/build.rs +++ b/primitives/state-machine/src/changes_trie/build.rs @@ -25,7 +25,7 @@ use num_traits::One; use crate::{ StorageKey, backend::Backend, - overlayed_changes::OverlayedChanges, + overlayed_changes::{OverlayedChanges, OverlayedValue}, trie_backend_essence::TrieBackendEssence, changes_trie::{ AnchorBlockId, ConfigurationRange, Storage, BlockNumber, @@ -108,7 +108,7 @@ fn prepare_extrinsics_input<'a, B, H, Number>( { let mut children_result = BTreeMap::new(); - for child_info in changes.child_infos() { + for (child_changes, child_info) in changes.children() { let child_index = ChildIndex:: { block: block.clone(), storage_key: child_info.prefixed_storage_key(), @@ -116,12 +116,13 @@ fn prepare_extrinsics_input<'a, B, H, Number>( let iter = prepare_extrinsics_input_inner( backend, block, changes, - Some(child_info.clone()) + Some(child_info.clone()), + child_changes, )?; children_result.insert(child_index, iter); } - let top = prepare_extrinsics_input_inner(backend, block, changes, None)?; + let top = prepare_extrinsics_input_inner(backend, block, changes, None, changes.changes())?; Ok((top, children_result)) } @@ -129,40 +130,38 @@ fn prepare_extrinsics_input<'a, B, H, Number>( fn prepare_extrinsics_input_inner<'a, B, H, Number>( backend: &'a B, block: &Number, - changes: &'a OverlayedChanges, + overlay: &'a OverlayedChanges, child_info: Option, + changes: impl Iterator ) -> Result> + 'a, String> where B: Backend, H: Hasher, Number: BlockNumber, { - changes.changes(child_info.as_ref()) - .filter(|( _, v)| v.extrinsics().is_some()) + changes + .filter(|( _, v)| v.extrinsics().next().is_some()) .try_fold(BTreeMap::new(), |mut map: BTreeMap<&[u8], (ExtrinsicIndex, Vec)>, (k, v)| { match map.entry(k) { Entry::Vacant(entry) => { // ignore temporary values (values that have null value at the end of operation // AND are not in storage at the beginning of operation if let Some(child_info) = child_info.as_ref() { - if !changes.child_storage(child_info, k).map(|v| v.is_some()).unwrap_or_default() { + if !overlay.child_storage(child_info, k).map(|v| v.is_some()).unwrap_or_default() { if !backend.exists_child_storage(&child_info, k) .map_err(|e| format!("{}", e))? { return Ok(map); } } } else { - if !changes.storage(k).map(|v| v.is_some()).unwrap_or_default() { + if !overlay.storage(k).map(|v| v.is_some()).unwrap_or_default() { if !backend.exists_storage(k).map_err(|e| format!("{}", e))? { return Ok(map); } } }; - let extrinsics = v.extrinsics() - .expect("filtered by filter() call above; qed") - .cloned() - .collect(); + let extrinsics = v.extrinsics().cloned().collect(); entry.insert((ExtrinsicIndex { block: block.clone(), key: k.to_vec(), @@ -173,9 +172,7 @@ fn prepare_extrinsics_input_inner<'a, B, H, Number>( // AND we are checking it before insertion let extrinsics = &mut entry.get_mut().1; extrinsics.extend( - v.extrinsics() - .expect("filtered by filter() call above; qed") - .cloned() + v.extrinsics().cloned() ); extrinsics.sort_unstable(); }, @@ -404,6 +401,8 @@ mod test { let mut changes = OverlayedChanges::default(); changes.set_collect_extrinsics(true); + changes.start_transaction(); + changes.set_extrinsic_index(1); changes.set_storage(vec![101], Some(vec![203])); @@ -411,7 +410,7 @@ mod test { changes.set_storage(vec![100], Some(vec![202])); changes.set_child_storage(&child_info_1, vec![100], Some(vec![202])); - changes.commit_prospective(); + changes.commit_transaction(); changes.set_extrinsic_index(0); changes.set_storage(vec![100], Some(vec![0])); diff --git a/primitives/state-machine/src/ext.rs b/primitives/state-machine/src/ext.rs index 7e805250e726a..f98dbb707ef59 100644 --- a/primitives/state-machine/src/ext.rs +++ b/primitives/state-machine/src/ext.rs @@ -147,7 +147,7 @@ where self.backend.pairs().iter() .map(|&(ref k, ref v)| (k.to_vec(), Some(v.to_vec()))) - .chain(self.overlay.changes(None).map(|(k, v)| (k.clone(), v.value().cloned()))) + .chain(self.overlay.changes().map(|(k, v)| (k.clone(), v.value().cloned()))) .collect::>() .into_iter() .filter_map(|(k, maybe_val)| maybe_val.map(|val| (k, val))) @@ -477,15 +477,14 @@ where ); root.encode() } else { + let root = if let Some((changes, info)) = self.overlay.child_changes(storage_key) { + let delta = changes.map(|(k, v)| (k.as_ref(), v.value().map(AsRef::as_ref))); + Some(self.backend.child_storage_root(info, delta)) + } else { + None + }; - if let Some(child_info) = self.overlay.default_child_info(storage_key) { - let (root, is_empty, _) = { - let delta = self.overlay.changes(Some(child_info)) - .map(|(k, v)| (k.as_ref(), v.value().map(AsRef::as_ref))); - - self.backend.child_storage_root(child_info, delta) - }; - + if let Some((root, is_empty, _)) = root { let root = root.encode(); // We store update in the overlay in order to be able to use 'self.storage_transaction' // cache. This is brittle as it rely on Ext only querying the trie backend for @@ -548,7 +547,9 @@ where } fn wipe(&mut self) { - self.overlay.discard_prospective(); + for _ in 0..self.overlay.transaction_depth() { + self.overlay.rollback_transaction(); + } self.overlay.drain_storage_changes( &self.backend, None, @@ -560,7 +561,9 @@ where } fn commit(&mut self) { - self.overlay.commit_prospective(); + for _ in 0..self.overlay.transaction_depth() { + self.overlay.commit_transaction(); + } let changes = self.overlay.drain_storage_changes( &self.backend, None, diff --git a/primitives/state-machine/src/lib.rs b/primitives/state-machine/src/lib.rs index 693a7bc12fad0..4b52f96c364e8 100644 --- a/primitives/state-machine/src/lib.rs +++ b/primitives/state-machine/src/lib.rs @@ -347,11 +347,11 @@ impl<'a, B, H, N, Exec> StateMachine<'a, B, H, N, Exec> where CallResult, ) -> CallResult { - let pending_changes = self.overlay.clone_pending(); + self.overlay.start_transaction(); let (result, was_native) = self.execute_aux(true, native_call.take()); if was_native { - self.overlay.replace_pending(pending_changes); + self.overlay.rollback_transaction(); let (wasm_result, _) = self.execute_aux( false, native_call, @@ -366,6 +366,7 @@ impl<'a, B, H, N, Exec> StateMachine<'a, B, H, N, Exec> where on_consensus_failure(wasm_result, result) } } else { + self.overlay.commit_transaction(); result } } @@ -378,16 +379,17 @@ impl<'a, B, H, N, Exec> StateMachine<'a, B, H, N, Exec> where R: Decode + Encode + PartialEq, NC: FnOnce() -> result::Result + UnwindSafe, { - let pending_changes = self.overlay.clone_pending(); + self.overlay.start_transaction(); let (result, was_native) = self.execute_aux( true, native_call.take(), ); if !was_native || result.is_ok() { + self.overlay.commit_transaction(); result } else { - self.overlay.replace_pending(pending_changes); + self.overlay.rollback_transaction(); let (wasm_result, _) = self.execute_aux( false, native_call, @@ -977,7 +979,7 @@ mod tests { let mut overlay = OverlayedChanges::default(); overlay.set_storage(b"aba".to_vec(), Some(b"1312".to_vec())); overlay.set_storage(b"bab".to_vec(), Some(b"228".to_vec())); - overlay.commit_prospective(); + overlay.start_transaction(); overlay.set_storage(b"abd".to_vec(), Some(b"69".to_vec())); overlay.set_storage(b"bbd".to_vec(), Some(b"42".to_vec())); @@ -994,10 +996,10 @@ mod tests { ); ext.clear_prefix(b"ab"); } - overlay.commit_prospective(); + overlay.commit_transaction(); assert_eq!( - overlay.changes(None).map(|(k, v)| (k.clone(), v.value().cloned())) + overlay.changes().map(|(k, v)| (k.clone(), v.value().cloned())) .collect::>(), map![ b"abc".to_vec() => None.into(), @@ -1083,7 +1085,7 @@ mod tests { Some(vec![reference_data[0].clone()].encode()), ); } - overlay.commit_prospective(); + overlay.start_transaction(); { let mut ext = Ext::new( &mut overlay, @@ -1102,7 +1104,7 @@ mod tests { Some(reference_data.encode()), ); } - overlay.discard_prospective(); + overlay.rollback_transaction(); { let ext = Ext::new( &mut overlay, @@ -1145,7 +1147,7 @@ mod tests { ext.clear_storage(key.as_slice()); ext.storage_append(key.clone(), Item::InitializationItem.encode()); } - overlay.commit_prospective(); + overlay.start_transaction(); // For example, first transaction resulted in panic during block building { @@ -1170,7 +1172,7 @@ mod tests { Some(vec![Item::InitializationItem, Item::DiscardedItem].encode()), ); } - overlay.discard_prospective(); + overlay.rollback_transaction(); // Then we apply next transaction which is valid this time. { @@ -1196,7 +1198,7 @@ mod tests { ); } - overlay.commit_prospective(); + overlay.start_transaction(); // Then only initlaization item and second (commited) item should persist. { diff --git a/primitives/state-machine/src/overlayed_changes.rs b/primitives/state-machine/src/overlayed_changes.rs index b0259c2b8592d..a4f254929dc64 100644 --- a/primitives/state-machine/src/overlayed_changes.rs +++ b/primitives/state-machine/src/overlayed_changes.rs @@ -17,6 +17,8 @@ //! The overlayed changes to state. +mod changeset; + use crate::{ backend::Backend, ChangesTrieTransaction, changes_trie::{ @@ -25,14 +27,17 @@ use crate::{ }, stats::StateMachineStats, }; +use self::changeset::OverlayedChangeSet; -use std::{mem, ops, collections::{HashMap, BTreeMap, BTreeSet}}; +use std::collections::HashMap; use codec::{Decode, Encode}; -use sp_core::storage::{well_known_keys::EXTRINSIC_INDEX, ChildInfo, ChildType}; +use sp_core::storage::{well_known_keys::EXTRINSIC_INDEX, ChildInfo}; use sp_core::offchain::storage::OffchainOverlayedChanges; - use hash_db::Hasher; +/// Re-export of `changeset::OverlayedValue`. +pub use self::changeset::OverlayedValue; + /// Storage key. pub type StorageKey = Vec; @@ -45,43 +50,21 @@ pub type StorageCollection = Vec<(StorageKey, Option)>; /// In memory arrays of storage values for multiple child tries. pub type ChildStorageCollection = Vec<(StorageKey, StorageCollection)>; -/// The overlayed changes to state to be queried on top of the backend. +/// The set of changes that are overlaid onto the backend. /// -/// A transaction shares all prospective changes within an inner overlay -/// that can be cleared. +/// It allows changes to be modified using nestable transactions. #[derive(Debug, Default, Clone)] pub struct OverlayedChanges { - /// Changes that are not yet committed. - prospective: OverlayedChangeSet, - /// Committed changes. - committed: OverlayedChangeSet, + /// Top level storage changes. + top: OverlayedChangeSet, + /// Child storage changes. The map key is the child storage key without the common prefix. + children: HashMap, /// True if extrinsics stats must be collected. collect_extrinsics: bool, /// Collect statistic on this execution. stats: StateMachineStats, } -/// The storage value, used inside OverlayedChanges. -#[derive(Debug, Default, Clone)] -#[cfg_attr(test, derive(PartialEq))] -pub struct OverlayedValue { - /// Current value. None if value has been deleted. - value: Option, - /// The set of extrinsic indices where the values has been changed. - /// Is filled only if runtime has announced changes trie support. - extrinsics: Option>, -} - -/// Prospective or committed overlayed change set. -#[derive(Debug, Default, Clone)] -#[cfg_attr(test, derive(PartialEq))] -pub struct OverlayedChangeSet { - /// Top level storage changes. - top: BTreeMap, - /// Child storage changes. The map key is the child storage key without the common prefix. - children_default: HashMap, ChildInfo)>, -} - /// A storage changes structure that can be generated by the data collected in [`OverlayedChanges`]. /// /// This contains all the changes to the storage and transactions to apply theses changes to the @@ -174,45 +157,10 @@ impl Default for StorageChanges } } -#[cfg(test)] -impl std::iter::FromIterator<(StorageKey, OverlayedValue)> for OverlayedChangeSet { - fn from_iter>(iter: T) -> Self { - Self { - top: iter.into_iter().collect(), - children_default: Default::default(), - } - } -} - -impl OverlayedValue { - /// The most recent value contained in this overlay. - pub fn value(&self) -> Option<&StorageValue> { - self.value.as_ref() - } - - /// List of indices of extrinsics which modified the value using this overlay. - pub fn extrinsics(&self) -> Option> { - self.extrinsics.as_ref().map(|v| v.iter()) - } -} - -impl OverlayedChangeSet { - /// Whether the change set is empty. - pub fn is_empty(&self) -> bool { - self.top.is_empty() && self.children_default.is_empty() - } - - /// Clear the change set. - pub fn clear(&mut self) { - self.top.clear(); - self.children_default.clear(); - } -} - impl OverlayedChanges { - /// Whether the overlayed changes are empty. + /// Whether no changes are contained in the top nor in any of the child changes. pub fn is_empty(&self) -> bool { - self.prospective.is_empty() && self.committed.is_empty() + self.top.is_empty() && self.children.is_empty() } /// Ask to collect/not to collect extrinsics indices where key(s) has been changed. @@ -224,194 +172,112 @@ impl OverlayedChanges { /// to the backend); Some(None) if the key has been deleted. Some(Some(...)) for a key whose /// value has been set. pub fn storage(&self, key: &[u8]) -> Option> { - self.prospective.top.get(key) - .or_else(|| self.committed.top.get(key)) - .map(|x| { - let size_read = x.value.as_ref().map(|x| x.len() as u64).unwrap_or(0); - self.stats.tally_read_modified(size_read); - x.value.as_ref().map(AsRef::as_ref) - }) + self.top.get(key).map(|x| { + let value = x.value(); + let size_read = value.map(|x| x.len() as u64).unwrap_or(0); + self.stats.tally_read_modified(size_read); + value.map(AsRef::as_ref) + }) } - /// Returns mutable reference to current changed value (prospective). - /// If there is no value in the overlay, the default callback is used to initiate - /// the value. - /// Warning this function register a change, so the mutable reference MUST be modified. + /// Returns mutable reference to current value. + /// If there is no value in the overlay, the default callback is used to initiate the value. + /// Warning this function registers a change, so the mutable reference MUST be modified. + /// + /// Can be rolled back or comitted when called inside a transaction. #[must_use = "A change was registered, so this value MUST be modified."] pub fn value_mut_or_insert_with( &mut self, key: &[u8], init: impl Fn() -> StorageValue, ) -> &mut StorageValue { - let extrinsic_index = self.extrinsic_index(); - let committed = &self.committed.top; - - let mut entry = self.prospective.top.entry(key.to_vec()) - .or_insert_with(|| { - if let Some(overlay_state) = committed.get(key).cloned() { - overlay_state - } else { - OverlayedValue { value: Some(init()), ..Default::default() } - } - }); - - //if was deleted initialise back with empty vec - if entry.value.is_none() { - entry.value = Some(Default::default()); - } - if let Some(extrinsic) = extrinsic_index { - entry.extrinsics.get_or_insert_with(Default::default) - .insert(extrinsic); + let value = self.top.modify(key.to_owned(), init, self.extrinsic_index()); + + if value.is_none() { + *value = Some(Default::default()); } - entry.value.as_mut().expect("Initialized above; qed") + + value.as_mut().expect("Initialized above; qed") } /// Returns a double-Option: None if the key is unknown (i.e. and the query should be referred /// to the backend); Some(None) if the key has been deleted. Some(Some(...)) for a key whose /// value has been set. pub fn child_storage(&self, child_info: &ChildInfo, key: &[u8]) -> Option> { - if let Some(map) = self.prospective.children_default.get(child_info.storage_key()) { + if let Some(map) = self.children.get(child_info.storage_key()) { if let Some(val) = map.0.get(key) { - let size_read = val.value.as_ref().map(|x| x.len() as u64).unwrap_or(0); + let value = val.value(); + let size_read = value.map(|x| x.len() as u64).unwrap_or(0); self.stats.tally_read_modified(size_read); - return Some(val.value.as_ref().map(AsRef::as_ref)); + return Some(value.map(AsRef::as_ref)); } } - - if let Some(map) = self.committed.children_default.get(child_info.storage_key()) { - if let Some(val) = map.0.get(key) { - let size_read = val.value.as_ref().map(|x| x.len() as u64).unwrap_or(0); - self.stats.tally_read_modified(size_read); - return Some(val.value.as_ref().map(AsRef::as_ref)); - } - } - None } - /// Inserts the given key-value pair into the prospective change set. + /// Set a new value for the specified key. /// - /// `None` can be used to delete a value specified by the given key. + /// Can be rolled back or comitted when called inside a transaction. pub(crate) fn set_storage(&mut self, key: StorageKey, val: Option) { let size_write = val.as_ref().map(|x| x.len() as u64).unwrap_or(0); self.stats.tally_write_overlay(size_write); - let extrinsic_index = self.extrinsic_index(); - let entry = self.prospective.top.entry(key).or_default(); - entry.value = val; - - if let Some(extrinsic) = extrinsic_index { - entry.extrinsics.get_or_insert_with(Default::default) - .insert(extrinsic); - } + self.top.set(key, val, self.extrinsic_index()); } - /// Inserts the given key-value pair into the prospective child change set. + /// Set a new value for the specified key and child. /// /// `None` can be used to delete a value specified by the given key. + /// + /// Can be rolled back or comitted when called inside a transaction. pub(crate) fn set_child_storage( &mut self, child_info: &ChildInfo, key: StorageKey, val: Option, ) { + let extrinsic_index = self.extrinsic_index(); let size_write = val.as_ref().map(|x| x.len() as u64).unwrap_or(0); self.stats.tally_write_overlay(size_write); - let extrinsic_index = self.extrinsic_index(); let storage_key = child_info.storage_key().to_vec(); - let map_entry = self.prospective.children_default.entry(storage_key) - .or_insert_with(|| (Default::default(), child_info.to_owned())); - let updatable = map_entry.1.try_update(child_info); + let tx_depth = self.transaction_depth(); + let (overlay, info) = self.children.entry(storage_key) + .or_insert_with(|| ( + // This changeset might be created when there are already open transactions. + // We need to catch up here so it is at the same transaction depth. + OverlayedChangeSet::with_depth(tx_depth), + child_info.to_owned() + )); + let updatable = info.try_update(child_info); debug_assert!(updatable); - - let entry = map_entry.0.entry(key).or_default(); - entry.value = val; - - if let Some(extrinsic) = extrinsic_index { - entry.extrinsics.get_or_insert_with(Default::default) - .insert(extrinsic); - } + overlay.set(key, val, extrinsic_index); } /// Clear child storage of given storage key. /// - /// NOTE that this doesn't take place immediately but written into the prospective - /// change set, and still can be reverted by [`discard_prospective`]. - /// - /// [`discard_prospective`]: #method.discard_prospective + /// Can be rolled back or comitted when called inside a transaction. pub(crate) fn clear_child_storage( &mut self, child_info: &ChildInfo, ) { let extrinsic_index = self.extrinsic_index(); let storage_key = child_info.storage_key(); - let map_entry = self.prospective.children_default.entry(storage_key.to_vec()) + let (changeset, info) = self.children.entry(storage_key.to_vec()) .or_insert_with(|| (Default::default(), child_info.to_owned())); - let updatable = map_entry.1.try_update(child_info); + let updatable = info.try_update(child_info); debug_assert!(updatable); - - map_entry.0.values_mut().for_each(|e| { - if let Some(extrinsic) = extrinsic_index { - e.extrinsics.get_or_insert_with(Default::default) - .insert(extrinsic); - } - - e.value = None; - }); - - if let Some((committed_map, _child_info)) = self.committed.children_default.get(storage_key) { - for (key, value) in committed_map.iter() { - if !map_entry.0.contains_key(key) { - map_entry.0.insert(key.clone(), OverlayedValue { - value: None, - extrinsics: extrinsic_index.map(|i| { - let mut e = value.extrinsics.clone() - .unwrap_or_else(|| BTreeSet::default()); - e.insert(i); - e - }), - }); - } - } - } + changeset.clear(|_, _| true, extrinsic_index); } /// Removes all key-value pairs which keys share the given prefix. /// - /// NOTE that this doesn't take place immediately but written into the prospective - /// change set, and still can be reverted by [`discard_prospective`]. - /// - /// [`discard_prospective`]: #method.discard_prospective + /// Can be rolled back or comitted when called inside a transaction. pub(crate) fn clear_prefix(&mut self, prefix: &[u8]) { - let extrinsic_index = self.extrinsic_index(); - - // Iterate over all prospective and mark all keys that share - // the given prefix as removed (None). - for (key, entry) in self.prospective.top.iter_mut() { - if key.starts_with(prefix) { - entry.value = None; - - if let Some(extrinsic) = extrinsic_index { - entry.extrinsics.get_or_insert_with(Default::default) - .insert(extrinsic); - } - } - } - - // Then do the same with keys from committed changes. - // NOTE that we are making changes in the prospective change set. - for key in self.committed.top.keys() { - if key.starts_with(prefix) { - let entry = self.prospective.top.entry(key.clone()).or_default(); - entry.value = None; - - if let Some(extrinsic) = extrinsic_index { - entry.extrinsics.get_or_insert_with(Default::default) - .insert(extrinsic); - } - } - } + self.top.clear(|key, _| key.starts_with(prefix), self.extrinsic_index()); } + /// Removes all key-value pairs which keys share the given prefix. + /// + /// Can be rolled back or comitted when called inside a transaction pub(crate) fn clear_child_prefix( &mut self, child_info: &ChildInfo, @@ -419,131 +285,100 @@ impl OverlayedChanges { ) { let extrinsic_index = self.extrinsic_index(); let storage_key = child_info.storage_key(); - let map_entry = self.prospective.children_default.entry(storage_key.to_vec()) + let (changeset, info) = self.children.entry(storage_key.to_vec()) .or_insert_with(|| (Default::default(), child_info.to_owned())); - let updatable = map_entry.1.try_update(child_info); + let updatable = info.try_update(child_info); debug_assert!(updatable); + changeset.clear(|key, _| key.starts_with(prefix), extrinsic_index); + } - for (key, entry) in map_entry.0.iter_mut() { - if key.starts_with(prefix) { - entry.value = None; - - if let Some(extrinsic) = extrinsic_index { - entry.extrinsics.get_or_insert_with(Default::default) - .insert(extrinsic); - } - } - } + /// Returns the current nesting depth of the transaction stack. + /// + /// A value of zero means that no transaction is open and changes are comitted on write. + pub fn transaction_depth(&self) -> usize { + // The top changeset and all child changesets transact in lockstep and are + // therefore always at the same transaction depth. + self.top.transaction_depth() + } - if let Some((child_committed, _child_info)) = self.committed.children_default.get(storage_key) { - // Then do the same with keys from committed changes. - // NOTE that we are making changes in the prospective change set. - for key in child_committed.keys() { - if key.starts_with(prefix) { - let entry = map_entry.0.entry(key.clone()).or_default(); - entry.value = None; - - if let Some(extrinsic) = extrinsic_index { - entry.extrinsics.get_or_insert_with(Default::default) - .insert(extrinsic); - } - } - } + /// Start a new nested transaction. + /// + /// This allows to either commit or roll back all changes that where made while this + /// transaction was open. Any transaction must be closed by one of the aforementioned + /// functions before this overlay can be converted into storage changes. + /// + /// Changes made without any open transaction are comitted immediatly. + pub fn start_transaction(&mut self) { + self.top.start_transaction(); + for (_, (changeset, _)) in self.children.iter_mut() { + changeset.start_transaction(); } } - /// Discard prospective changes to state. - pub fn discard_prospective(&mut self) { - self.prospective.clear(); + /// Rollback the last transaction started by `start_transaction`. + /// + /// Any changes made during that transaction are discarded. + /// + /// Panics: + /// Will panic if there is no open transaction. + pub fn rollback_transaction(&mut self) { + self.top.rollback_transaction(); + self.children.retain(|_, (changeset, _)| { + changeset.rollback_transaction(); + !changeset.is_empty() + }); } - /// Commit prospective changes to state. - pub fn commit_prospective(&mut self) { - if self.committed.is_empty() { - mem::swap(&mut self.prospective, &mut self.committed); - } else { - let top_to_commit = mem::replace(&mut self.prospective.top, BTreeMap::new()); - for (key, val) in top_to_commit.into_iter() { - let entry = self.committed.top.entry(key).or_default(); - entry.value = val.value; - - if let Some(prospective_extrinsics) = val.extrinsics { - entry.extrinsics.get_or_insert_with(Default::default) - .extend(prospective_extrinsics); - } - } - for (storage_key, (map, child_info)) in self.prospective.children_default.drain() { - let child_content = self.committed.children_default.entry(storage_key) - .or_insert_with(|| (Default::default(), child_info)); - // No update to child info at this point (will be needed for deletion). - for (key, val) in map.into_iter() { - let entry = child_content.0.entry(key).or_default(); - entry.value = val.value; - - if let Some(prospective_extrinsics) = val.extrinsics { - entry.extrinsics.get_or_insert_with(Default::default) - .extend(prospective_extrinsics); - } - } - } + /// Commit the last transaction started by `start_transaction`. + /// + /// Any changes made during that transaction are committed. + /// + /// Panics: + /// Will panic if there is no open transaction. + pub fn commit_transaction(&mut self) { + self.top.commit_transaction(); + for (_, (changeset, _)) in self.children.iter_mut() { + changeset.commit_transaction(); } } - /// Consume `OverlayedChanges` and take committed set. + /// Consume all changes (top + children) and return them. + /// + /// After calling this function no more changes are contained in this changeset. /// /// Panics: - /// Will panic if there are any uncommitted prospective changes. + /// Panics if `transaction_depth() > 0` fn drain_committed(&mut self) -> ( impl Iterator)>, impl Iterator)>, ChildInfo))>, ) { - assert!(self.prospective.is_empty()); + use std::mem::take; ( - std::mem::take(&mut self.committed.top) - .into_iter() - .map(|(k, v)| (k, v.value)), - std::mem::take(&mut self.committed.children_default) - .into_iter() - .map(|(sk, (v, ci))| (sk, (v.into_iter().map(|(k, v)| (k, v.value)), ci))), + take(&mut self.top).drain_commited(), + take(&mut self.children).into_iter() + .map(|(key, (val, info))| ( + key, + (val.drain_commited(), info) + ) + ), ) } - /// Get an iterator over all pending and committed child tries in the overlay. - pub fn child_infos(&self) -> impl IntoIterator { - self.committed.children_default.iter() - .chain(self.prospective.children_default.iter()) - .map(|(_, v)| &v.1).collect::>() - } - - /// Get an iterator over all pending and committed changes. - /// - /// Supplying `None` for `child_info` will only return changes that are in the top - /// trie. Specifying some `child_info` will return only the changes in that - /// child trie. - pub fn changes(&self, child_info: Option<&ChildInfo>) - -> impl Iterator - { - let (committed, prospective) = if let Some(child_info) = child_info { - match child_info.child_type() { - ChildType::ParentKeyId => ( - self.committed.children_default.get(child_info.storage_key()).map(|c| &c.0), - self.prospective.children_default.get(child_info.storage_key()).map(|c| &c.0), - ), - } - } else { - (Some(&self.committed.top), Some(&self.prospective.top)) - }; - committed.into_iter().flatten().chain(prospective.into_iter().flatten()) + /// Get an iterator over all child changes as seen by the current transaction. + pub fn children(&self) + -> impl Iterator, &ChildInfo)> { + self.children.iter().map(|(_, v)| (v.0.changes(), &v.1)) } - /// Return a clone of the currently pending changes. - pub fn clone_pending(&self) -> OverlayedChangeSet { - self.prospective.clone() + /// Get an iterator over all top changes as been by the current transaction. + pub fn changes(&self) -> impl Iterator { + self.top.changes() } - /// Replace the currently pending changes. - pub fn replace_pending(&mut self, pending: OverlayedChangeSet) { - self.prospective = pending; + /// Get an optional iterator over all child changes stored under the supplied key. + pub fn child_changes(&self, key: &[u8] + ) -> Option<(impl Iterator, &ChildInfo)> { + self.children.get(key).map(|(overlay, info)| (overlay.changes(), info)) } /// Convert this instance with all changes into a [`StorageChanges`] instance. @@ -607,10 +442,7 @@ impl OverlayedChanges { /// Inserts storage entry responsible for current extrinsic index. #[cfg(test)] pub(crate) fn set_extrinsic_index(&mut self, extrinsic_index: u32) { - self.prospective.top.insert(EXTRINSIC_INDEX.to_vec(), OverlayedValue { - value: Some(extrinsic_index.encode()), - extrinsics: None, - }); + self.top.set(EXTRINSIC_INDEX.to_vec(), Some(extrinsic_index.encode()), None); } /// Returns current extrinsic index to use in changes trie construction. @@ -629,7 +461,8 @@ impl OverlayedChanges { } } - /// Generate the storage root using `backend` and all changes from `prospective` and `committed`. + /// Generate the storage root using `backend` and all changes + /// as seen by the current transaction. /// /// Returns the storage root and caches storage transaction in the given `cache`. pub fn storage_root>( @@ -639,35 +472,13 @@ impl OverlayedChanges { ) -> H::Out where H::Out: Ord + Encode, { - let child_storage_keys = self.prospective.children_default.keys() - .chain(self.committed.children_default.keys()); - let child_delta_iter = child_storage_keys.map(|storage_key| - ( - self.default_child_info(storage_key) - .expect("child info initialized in either committed or prospective"), - self.committed.children_default.get(storage_key) - .into_iter() - .flat_map(|(map, _)| - map.iter().map(|(k, v)| (&k[..], v.value().map(|v| &v[..]))) - ) - .chain( - self.prospective.children_default.get(storage_key) - .into_iter() - .flat_map(|(map, _)| - map.iter().map(|(k, v)| (&k[..], v.value().map(|v| &v[..]))) - ) - ), - ) - ); - - // compute and memoize - let delta = self.committed - .top - .iter() - .map(|(k, v)| (&k[..], v.value().map(|v| &v[..]))) - .chain(self.prospective.top.iter().map(|(k, v)| (&k[..], v.value().map(|v| &v[..])))); + let delta = self.changes().map(|(k, v)| (&k[..], v.value().map(|v| &v[..]))); + let child_delta = self.children() + .map(|(changes, info)| (info, changes.map( + |(k, v)| (&k[..], v.value().map(|v| &v[..])) + ))); - let (root, transaction) = backend.full_storage_root(delta, child_delta_iter); + let (root, transaction) = backend.full_storage_root(delta, child_delta); cache.transaction = Some(transaction); cache.transaction_storage_root = Some(root); @@ -704,41 +515,10 @@ impl OverlayedChanges { }) } - /// Get child info for a storage key. - /// Take the latest value so prospective first. - pub fn default_child_info(&self, storage_key: &[u8]) -> Option<&ChildInfo> { - if let Some((_, ci)) = self.prospective.children_default.get(storage_key) { - return Some(&ci); - } - if let Some((_, ci)) = self.committed.children_default.get(storage_key) { - return Some(&ci); - } - None - } - /// Returns the next (in lexicographic order) storage key in the overlayed alongside its value. /// If no value is next then `None` is returned. pub fn next_storage_key_change(&self, key: &[u8]) -> Option<(&[u8], &OverlayedValue)> { - let range = (ops::Bound::Excluded(key), ops::Bound::Unbounded); - - let next_prospective_key = self.prospective.top - .range::<[u8], _>(range) - .next() - .map(|(k, v)| (&k[..], v)); - - let next_committed_key = self.committed.top - .range::<[u8], _>(range) - .next() - .map(|(k, v)| (&k[..], v)); - - match (next_committed_key, next_prospective_key) { - // Committed is strictly less than prospective - (Some(committed_key), Some(prospective_key)) if committed_key.0 < prospective_key.0 => - Some(committed_key), - (committed_key, None) => committed_key, - // Prospective key is less or equal to committed or committed doesn't exist - (_, prospective_key) => prospective_key, - } + self.top.next_change(key) } /// Returns the next (in lexicographic order) child storage key in the overlayed alongside its @@ -748,48 +528,32 @@ impl OverlayedChanges { storage_key: &[u8], key: &[u8] ) -> Option<(&[u8], &OverlayedValue)> { - let range = (ops::Bound::Excluded(key), ops::Bound::Unbounded); - - let next_prospective_key = self.prospective.children_default.get(storage_key) - .and_then(|(map, _)| map.range::<[u8], _>(range).next().map(|(k, v)| (&k[..], v))); - - let next_committed_key = self.committed.children_default.get(storage_key) - .and_then(|(map, _)| map.range::<[u8], _>(range).next().map(|(k, v)| (&k[..], v))); - - match (next_committed_key, next_prospective_key) { - // Committed is strictly less than prospective - (Some(committed_key), Some(prospective_key)) if committed_key.0 < prospective_key.0 => - Some(committed_key), - (committed_key, None) => committed_key, - // Prospective key is less or equal to committed or committed doesn't exist - (_, prospective_key) => prospective_key, - } - } -} - -#[cfg(test)] -impl From> for OverlayedValue { - fn from(value: Option) -> OverlayedValue { - OverlayedValue { value, ..Default::default() } + self.children + .get(storage_key) + .and_then(|(overlay, _)| + overlay.next_change(key) + ) } } #[cfg(test)] mod tests { use hex_literal::hex; - use sp_core::{ - Blake2Hasher, traits::Externalities, storage::well_known_keys::EXTRINSIC_INDEX, - }; + use sp_core::{Blake2Hasher, traits::Externalities}; use crate::InMemoryBackend; use crate::ext::Ext; use super::*; + use std::collections::BTreeMap; - fn strip_extrinsic_index(map: &BTreeMap) - -> BTreeMap - { - let mut clone = map.clone(); - clone.remove(&EXTRINSIC_INDEX.to_vec()); - clone + fn assert_extrinsics( + overlay: &OverlayedChangeSet, + key: impl AsRef<[u8]>, + should: Vec + ) { + assert_eq!( + overlay.get(key.as_ref()).unwrap().extrinsics().cloned().collect::>(), + should + ) } #[test] @@ -800,23 +564,28 @@ mod tests { assert!(overlayed.storage(&key).is_none()); + overlayed.start_transaction(); + overlayed.set_storage(key.clone(), Some(vec![1, 2, 3])); assert_eq!(overlayed.storage(&key).unwrap(), Some(&[1, 2, 3][..])); - overlayed.commit_prospective(); + overlayed.commit_transaction(); + assert_eq!(overlayed.storage(&key).unwrap(), Some(&[1, 2, 3][..])); + overlayed.start_transaction(); + overlayed.set_storage(key.clone(), Some(vec![])); assert_eq!(overlayed.storage(&key).unwrap(), Some(&[][..])); overlayed.set_storage(key.clone(), None); assert!(overlayed.storage(&key).unwrap().is_none()); - overlayed.discard_prospective(); + overlayed.rollback_transaction(); + assert_eq!(overlayed.storage(&key).unwrap(), Some(&[1, 2, 3][..])); overlayed.set_storage(key.clone(), None); - overlayed.commit_prospective(); assert!(overlayed.storage(&key).unwrap().is_none()); } @@ -829,18 +598,18 @@ mod tests { (b"doug".to_vec(), b"notadog".to_vec()), ].into_iter().collect(); let backend = InMemoryBackend::::from(initial); - let mut overlay = OverlayedChanges { - committed: vec![ - (b"dog".to_vec(), Some(b"puppy".to_vec()).into()), - (b"dogglesworth".to_vec(), Some(b"catYYY".to_vec()).into()), - (b"doug".to_vec(), Some(vec![]).into()), - ].into_iter().collect(), - prospective: vec![ - (b"dogglesworth".to_vec(), Some(b"cat".to_vec()).into()), - (b"doug".to_vec(), None.into()), - ].into_iter().collect(), - ..Default::default() - }; + let mut overlay = OverlayedChanges::default(); + overlay.set_collect_extrinsics(false); + + overlay.start_transaction(); + overlay.set_storage(b"dog".to_vec(), Some(b"puppy".to_vec())); + overlay.set_storage(b"dogglesworth".to_vec(), Some(b"catYYY".to_vec())); + overlay.set_storage(b"doug".to_vec(), Some(vec![])); + overlay.commit_transaction(); + + overlay.start_transaction(); + overlay.set_storage(b"dogglesworth".to_vec(), Some(b"cat".to_vec())); + overlay.set_storage(b"doug".to_vec(), None); let mut offchain_overlay = Default::default(); let mut cache = StorageTransactionCache::default(); @@ -862,6 +631,8 @@ mod tests { let mut overlay = OverlayedChanges::default(); overlay.set_collect_extrinsics(true); + overlay.start_transaction(); + overlay.set_storage(vec![100], Some(vec![101])); overlay.set_extrinsic_index(0); @@ -873,17 +644,11 @@ mod tests { overlay.set_extrinsic_index(2); overlay.set_storage(vec![1], Some(vec![6])); - assert_eq!(strip_extrinsic_index(&overlay.prospective.top), - vec![ - (vec![1], OverlayedValue { value: Some(vec![6]), - extrinsics: Some(vec![0, 2].into_iter().collect()) }), - (vec![3], OverlayedValue { value: Some(vec![4]), - extrinsics: Some(vec![1].into_iter().collect()) }), - (vec![100], OverlayedValue { value: Some(vec![101]), - extrinsics: Some(vec![NO_EXTRINSIC_INDEX].into_iter().collect()) }), - ].into_iter().collect()); + assert_extrinsics(&overlay.top, vec![1], vec![0, 2]); + assert_extrinsics(&overlay.top, vec![3], vec![1]); + assert_extrinsics(&overlay.top, vec![100], vec![NO_EXTRINSIC_INDEX]); - overlay.commit_prospective(); + overlay.start_transaction(); overlay.set_extrinsic_index(3); overlay.set_storage(vec![3], Some(vec![7])); @@ -891,75 +656,53 @@ mod tests { overlay.set_extrinsic_index(4); overlay.set_storage(vec![1], Some(vec![8])); - assert_eq!(strip_extrinsic_index(&overlay.committed.top), - vec![ - (vec![1], OverlayedValue { value: Some(vec![6]), - extrinsics: Some(vec![0, 2].into_iter().collect()) }), - (vec![3], OverlayedValue { value: Some(vec![4]), - extrinsics: Some(vec![1].into_iter().collect()) }), - (vec![100], OverlayedValue { value: Some(vec![101]), - extrinsics: Some(vec![NO_EXTRINSIC_INDEX].into_iter().collect()) }), - ].into_iter().collect()); - - assert_eq!(strip_extrinsic_index(&overlay.prospective.top), - vec![ - (vec![1], OverlayedValue { value: Some(vec![8]), - extrinsics: Some(vec![4].into_iter().collect()) }), - (vec![3], OverlayedValue { value: Some(vec![7]), - extrinsics: Some(vec![3].into_iter().collect()) }), - ].into_iter().collect()); - - overlay.commit_prospective(); - - assert_eq!(strip_extrinsic_index(&overlay.committed.top), - vec![ - (vec![1], OverlayedValue { value: Some(vec![8]), - extrinsics: Some(vec![0, 2, 4].into_iter().collect()) }), - (vec![3], OverlayedValue { value: Some(vec![7]), - extrinsics: Some(vec![1, 3].into_iter().collect()) }), - (vec![100], OverlayedValue { value: Some(vec![101]), - extrinsics: Some(vec![NO_EXTRINSIC_INDEX].into_iter().collect()) }), - ].into_iter().collect()); - - assert_eq!(overlay.prospective, - Default::default()); + assert_extrinsics(&overlay.top, vec![1], vec![0, 2, 4]); + assert_extrinsics(&overlay.top, vec![3], vec![1, 3]); + assert_extrinsics(&overlay.top, vec![100], vec![NO_EXTRINSIC_INDEX]); + + overlay.rollback_transaction(); + + assert_extrinsics(&overlay.top, vec![1], vec![0, 2]); + assert_extrinsics(&overlay.top, vec![3], vec![1]); + assert_extrinsics(&overlay.top, vec![100], vec![NO_EXTRINSIC_INDEX]); } #[test] fn next_storage_key_change_works() { let mut overlay = OverlayedChanges::default(); + overlay.start_transaction(); overlay.set_storage(vec![20], Some(vec![20])); overlay.set_storage(vec![30], Some(vec![30])); overlay.set_storage(vec![40], Some(vec![40])); - overlay.commit_prospective(); + overlay.commit_transaction(); overlay.set_storage(vec![10], Some(vec![10])); overlay.set_storage(vec![30], None); // next_prospective < next_committed let next_to_5 = overlay.next_storage_key_change(&[5]).unwrap(); assert_eq!(next_to_5.0.to_vec(), vec![10]); - assert_eq!(next_to_5.1.value, Some(vec![10])); + assert_eq!(next_to_5.1.value(), Some(&vec![10])); // next_committed < next_prospective let next_to_10 = overlay.next_storage_key_change(&[10]).unwrap(); assert_eq!(next_to_10.0.to_vec(), vec![20]); - assert_eq!(next_to_10.1.value, Some(vec![20])); + assert_eq!(next_to_10.1.value(), Some(&vec![20])); // next_committed == next_prospective let next_to_20 = overlay.next_storage_key_change(&[20]).unwrap(); assert_eq!(next_to_20.0.to_vec(), vec![30]); - assert_eq!(next_to_20.1.value, None); + assert_eq!(next_to_20.1.value(), None); // next_committed, no next_prospective let next_to_30 = overlay.next_storage_key_change(&[30]).unwrap(); assert_eq!(next_to_30.0.to_vec(), vec![40]); - assert_eq!(next_to_30.1.value, Some(vec![40])); + assert_eq!(next_to_30.1.value(), Some(&vec![40])); overlay.set_storage(vec![50], Some(vec![50])); // next_prospective, no next_committed let next_to_40 = overlay.next_storage_key_change(&[40]).unwrap(); assert_eq!(next_to_40.0.to_vec(), vec![50]); - assert_eq!(next_to_40.1.value, Some(vec![50])); + assert_eq!(next_to_40.1.value(), Some(&vec![50])); } #[test] @@ -968,37 +711,38 @@ mod tests { let child_info = &child_info; let child = child_info.storage_key(); let mut overlay = OverlayedChanges::default(); + overlay.start_transaction(); overlay.set_child_storage(child_info, vec![20], Some(vec![20])); overlay.set_child_storage(child_info, vec![30], Some(vec![30])); overlay.set_child_storage(child_info, vec![40], Some(vec![40])); - overlay.commit_prospective(); + overlay.commit_transaction(); overlay.set_child_storage(child_info, vec![10], Some(vec![10])); overlay.set_child_storage(child_info, vec![30], None); // next_prospective < next_committed let next_to_5 = overlay.next_child_storage_key_change(child, &[5]).unwrap(); assert_eq!(next_to_5.0.to_vec(), vec![10]); - assert_eq!(next_to_5.1.value, Some(vec![10])); + assert_eq!(next_to_5.1.value(), Some(&vec![10])); // next_committed < next_prospective let next_to_10 = overlay.next_child_storage_key_change(child, &[10]).unwrap(); assert_eq!(next_to_10.0.to_vec(), vec![20]); - assert_eq!(next_to_10.1.value, Some(vec![20])); + assert_eq!(next_to_10.1.value(), Some(&vec![20])); // next_committed == next_prospective let next_to_20 = overlay.next_child_storage_key_change(child, &[20]).unwrap(); assert_eq!(next_to_20.0.to_vec(), vec![30]); - assert_eq!(next_to_20.1.value, None); + assert_eq!(next_to_20.1.value(), None); // next_committed, no next_prospective let next_to_30 = overlay.next_child_storage_key_change(child, &[30]).unwrap(); assert_eq!(next_to_30.0.to_vec(), vec![40]); - assert_eq!(next_to_30.1.value, Some(vec![40])); + assert_eq!(next_to_30.1.value(), Some(&vec![40])); overlay.set_child_storage(child_info, vec![50], Some(vec![50])); // next_prospective, no next_committed let next_to_40 = overlay.next_child_storage_key_change(child, &[40]).unwrap(); assert_eq!(next_to_40.0.to_vec(), vec![50]); - assert_eq!(next_to_40.1.value, Some(vec![50])); + assert_eq!(next_to_40.1.value(), Some(&vec![50])); } } diff --git a/primitives/state-machine/src/overlayed_changes/changeset.rs b/primitives/state-machine/src/overlayed_changes/changeset.rs new file mode 100644 index 0000000000000..328cfda34ff30 --- /dev/null +++ b/primitives/state-machine/src/overlayed_changes/changeset.rs @@ -0,0 +1,635 @@ +// This file is part of Substrate. + +// Copyright (C) 2017-2020 Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: Apache-2.0 + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License + +//! Houses the code that implements the transactional overlay storage. + +use super::{StorageKey, StorageValue}; + +use itertools::Itertools; +use std::collections::{HashSet, BTreeMap, BTreeSet}; +use smallvec::SmallVec; + +const PROOF_DIRTY_KEYS: &str = "\ + We assume transactions are balanced. Every start of a transaction created one dirty + keys element. This function is only called on transaction close. Therefore an element + created when starting the transaction must exist; qed"; + +const PROOF_DIRTY_OVERLAY_VALUE: &str = "\ + A write to an OverlayedValue is recorded in the dirty key set. Before an OverlayedValues + is removed its containing dirty set is removed. This function is only called for keys that + are in the dirty set. Therefore the entry must exist; qed"; + +const PROOF_OVERLAY_NON_EMPTY: &str = "\ + An OverlayValue is always created with at least one transaction and dropped as soon + as the last transaction is removed; qed"; + +type DirtyKeys = SmallVec<[HashSet; 5]>; +type Transactions = SmallVec<[InnerValue; 5]>; + +#[derive(Debug, Default, Clone)] +#[cfg_attr(test, derive(PartialEq))] +struct InnerValue { + /// Current value. None if value has been deleted. + value: Option, + /// The set of extrinsic indices where the values has been changed. + /// Is filled only if runtime has announced changes trie support. + extrinsics: BTreeSet, +} + +/// An overlay that contains all versions of a value for a specific key. +#[derive(Debug, Default, Clone)] +#[cfg_attr(test, derive(PartialEq))] +pub struct OverlayedValue { + // The individual versions of that value. + // One entry per transactions during that the value was actually written. + transactions: Transactions, +} + +/// Holds a set of changes with the ability modify them using nested transactions. +#[derive(Debug, Default, Clone)] +pub struct OverlayedChangeSet { + /// Stores the changes that this overlay constitutes. + changes: BTreeMap, + /// Stores which keys are dirty per transaction. Needed in order to determine which + /// values to merge into the parent transaction on commit. The length of this vector + /// therefore determines how many nested transactions are currently open (depth). + dirty_keys: DirtyKeys, +} + +impl OverlayedValue { + /// The value as seen by the current transaction. + pub fn value(&self) -> Option<&StorageValue> { + self.transactions.last().expect(PROOF_OVERLAY_NON_EMPTY).value.as_ref() + } + + /// Unique list of extrinsic indices which modified the value. + pub fn extrinsics(&self) -> impl Iterator { + self.transactions.iter().flat_map(|t| t.extrinsics.iter()).unique() + } + + // Mutable reference to the most recent version. + fn value_mut(&mut self) -> &mut Option { + &mut self.transactions.last_mut().expect(PROOF_OVERLAY_NON_EMPTY).value + } + + // Remove the last version and return it. + fn pop_transaction(&mut self) -> InnerValue { + self.transactions.pop().expect(PROOF_OVERLAY_NON_EMPTY) + } + + // Mutable reference to the set which holds the indices for the **current transaction only**. + fn transaction_extrinsics_mut(&mut self) -> &mut BTreeSet { + &mut self.transactions.last_mut().expect(PROOF_OVERLAY_NON_EMPTY).extrinsics + } + + // Writes a new version of a value. + // + // This makes sure that the old version is not overwritten and can be properly + // rolled back when required. + fn set( + &mut self, + value: Option, + first_write_in_tx: bool, + at_extrinsic: Option + ) { + if first_write_in_tx || self.transactions.is_empty() { + self.transactions.push(InnerValue { + value, + .. Default::default() + }); + } else { + *self.value_mut() = value; + } + + if let Some(extrinsic) = at_extrinsic { + self.transaction_extrinsics_mut().insert(extrinsic); + } + } +} + +// Inserts a key into the dirty set. +// +// Returns true iff we are currently have at least one open transaction and if this +// is the first write to that transaction. +fn insert_dirty(set: &mut DirtyKeys, key: StorageKey) -> bool { + if let Some(dirty_keys) = set.last_mut() { + dirty_keys.insert(key) + } else { + false + } +} + +impl OverlayedChangeSet { + /// Create a new changeset with the specified transaction depth. + pub fn with_depth(depth: usize) -> Self { + use std::iter::repeat; + Self { + dirty_keys: repeat(HashSet::new()).take(depth).collect(), + .. Default::default() + } + } + + /// True if no changes at all are contained in the change set. + pub fn is_empty(&self) -> bool { + self.changes.is_empty() + } + + /// Get an optional reference to the value stored for the specified key. + pub fn get(&self, key: &[u8]) -> Option<&OverlayedValue> { + self.changes.get(key) + } + + /// Set a new value for the specified key. + /// + /// Can be rolled back or comitted when called inside a transaction. + pub fn set( + &mut self, + key: StorageKey, + value: Option, + at_extrinsic: Option + ) { + let overlayed = self.changes.entry(key.clone()).or_insert_with(Default::default); + overlayed.set(value, insert_dirty(&mut self.dirty_keys, key), at_extrinsic); + } + + /// Get a mutable reference for a value. + /// + /// Can be rolled back or comitted when called inside a transaction. + #[must_use = "A change was registered, so this value MUST be modified."] + pub fn modify( + &mut self, + key: StorageKey, + init: impl Fn() -> StorageValue, + at_extrinsic: Option + ) -> &mut Option { + let overlayed = self.changes.entry(key.clone()).or_insert_with(Default::default); + let first_write_in_tx = insert_dirty(&mut self.dirty_keys, key); + let clone_into_new_tx = if let Some(tx) = overlayed.transactions.last() { + if first_write_in_tx { + Some(tx.value.clone()) + } else { + None + } + } else { + Some(Some(init())) + }; + + if let Some(cloned) = clone_into_new_tx { + overlayed.set(cloned, first_write_in_tx, at_extrinsic); + } + overlayed.value_mut() + } + + /// Set all values to deleted which are matched by the predicate. + /// + /// Can be rolled back or comitted when called inside a transaction. + pub fn clear( + &mut self, + predicate: impl Fn(&[u8], &OverlayedValue) -> bool, + at_extrinsic: Option + ) { + for (key, val) in self.changes.iter_mut().filter(|(k, v)| predicate(k, v)) { + val.set(None, insert_dirty(&mut self.dirty_keys, key.to_owned()), at_extrinsic); + } + } + + /// Get a list of all changes as seen by current transaction. + pub fn changes(&self) -> impl Iterator { + self.changes.iter() + } + + /// Get the change that is next to the supplied key. + pub fn next_change(&self, key: &[u8]) -> Option<(&[u8], &OverlayedValue)> { + use std::ops::Bound; + let range = (Bound::Excluded(key), Bound::Unbounded); + self.changes.range::<[u8], _>(range).next().map(|(k, v)| (&k[..], v)) + } + + /// Consume this changeset and return all committed changes. + /// + /// Panics: + /// Panics if there are open transactions: `transaction_depth() > 0` + pub fn drain_commited(self) -> impl Iterator)> { + assert!(self.transaction_depth() == 0, "Drain is not allowed with open transactions."); + self.changes.into_iter().map(|(k, mut v)| (k, v.pop_transaction().value)) + } + + /// Returns the current nesting depth of the transaction stack. + /// + /// A value of zero means that no transaction is open and changes are comitted on write. + pub fn transaction_depth(&self) -> usize { + self.dirty_keys.len() + } + + /// Start a new nested transaction. + /// + /// This allows to either commit or roll back all changes that where made while this + /// transaction was open. Any transaction must be closed by one of the aforementioned + /// functions before this overlay can be converted into storage changes. + /// + /// Changes made without any open transaction are comitted immediatly. + pub fn start_transaction(&mut self) { + self.dirty_keys.push(Default::default()); + } + + /// Rollback the last transaction started by `start_transaction`. + /// + /// Any changes made during that transaction are discarded. + /// + /// Panics: + /// Will panic if there is no open transaction. + pub fn rollback_transaction(&mut self) { + self.close_transaction(true); + } + + /// Commit the last transaction started by `start_transaction`. + /// + /// Any changes made during that transaction are committed. + /// + /// Panics: + /// Will panic if there is no open transaction + pub fn commit_transaction(&mut self) { + self.close_transaction(false); + } + + fn close_transaction(&mut self, rollback: bool) { + for key in self.dirty_keys.pop().expect(PROOF_DIRTY_KEYS) { + let value = self.changes.get_mut(&key).expect(PROOF_DIRTY_OVERLAY_VALUE); + + if rollback { + value.pop_transaction(); + + // We need to remove the key as an `OverlayValue` with no transactions + // violates its invariant of always having at least one transaction. + if value.transactions.is_empty() { + self.changes.remove(&key); + } + } else { + let has_predecessor = ! if let Some(dirty_keys) = self.dirty_keys.last_mut() { + // Not the last tx: Did the previous tx write to this key? + dirty_keys.insert(key) + } else { + // Last tx: Is there already a value in the committed set? + // Check against one rather than empty because the current tx is still + // in the list as it is popped later in this function. + value.transactions.len() == 1 + }; + + // We only need to merge if in the previous tx (or committed set) there is + // already an existing value. + if has_predecessor { + let dropped_tx = value.pop_transaction(); + *value.value_mut() = dropped_tx.value; + value.transaction_extrinsics_mut().extend(dropped_tx.extrinsics); + } + } + } + } +} + +#[cfg(test)] +mod test { + use super::*; + use pretty_assertions::assert_eq; + + type Changes<'a> = Vec<(&'a [u8], (Option<&'a [u8]>, Vec))>; + type Drained<'a> = Vec<(&'a [u8], Option<&'a [u8]>)>; + + fn assert_changes(is: &OverlayedChangeSet, should: &Changes) { + let is: Changes = is.changes().map(|(k, v)| { + (k.as_ref(), (v.value().map(AsRef::as_ref), v.extrinsics().cloned().collect())) + }).collect(); + assert_eq!(&is, should); + } + + fn assert_drained_changes(is: OverlayedChangeSet, should: Changes) { + let is = is.drain_commited().collect::>(); + let should = should + .iter() + .map(|(k, v)| (k.to_vec(), v.0.map(From::from))).collect::>(); + assert_eq!(is, should); + } + + fn assert_drained(is: OverlayedChangeSet, should: Drained) { + let is = is.drain_commited().collect::>(); + let should = should + .iter() + .map(|(k, v)| (k.to_vec(), v.map(From::from))).collect::>(); + assert_eq!(is, should); + } + + #[test] + fn no_transaction_works() { + let mut changeset = OverlayedChangeSet::default(); + assert_eq!(changeset.transaction_depth(), 0); + + changeset.set(b"key0".to_vec(), Some(b"val0".to_vec()), Some(1)); + changeset.set(b"key1".to_vec(), Some(b"val1".to_vec()), Some(2)); + changeset.set(b"key0".to_vec(), Some(b"val0-1".to_vec()), Some(9)); + + assert_drained(changeset, vec![ + (b"key0", Some(b"val0-1")), + (b"key1", Some(b"val1")), + ]); + } + + #[test] + fn transaction_works() { + let mut changeset = OverlayedChangeSet::default(); + assert_eq!(changeset.transaction_depth(), 0); + + // no transaction: committed on set + changeset.set(b"key0".to_vec(), Some(b"val0".to_vec()), Some(1)); + changeset.set(b"key1".to_vec(), Some(b"val1".to_vec()), Some(1)); + changeset.set(b"key0".to_vec(), Some(b"val0-1".to_vec()), Some(10)); + + changeset.start_transaction(); + assert_eq!(changeset.transaction_depth(), 1); + + // we will commit that later + changeset.set(b"key42".to_vec(), Some(b"val42".to_vec()), Some(42)); + changeset.set(b"key99".to_vec(), Some(b"val99".to_vec()), Some(99)); + + changeset.start_transaction(); + assert_eq!(changeset.transaction_depth(), 2); + + // we will roll that back + changeset.set(b"key42".to_vec(), Some(b"val42-rolled".to_vec()), Some(421)); + changeset.set(b"key7".to_vec(), Some(b"val7-rolled".to_vec()), Some(77)); + changeset.set(b"key0".to_vec(), Some(b"val0-rolled".to_vec()), Some(1000)); + changeset.set(b"key5".to_vec(), Some(b"val5-rolled".to_vec()), None); + + // changes contain all changes not only the commmited ones. + let all_changes: Changes = vec![ + (b"key0", (Some(b"val0-rolled"), vec![1, 10, 1000])), + (b"key1", (Some(b"val1"), vec![1])), + (b"key42", (Some(b"val42-rolled"), vec![42, 421])), + (b"key5", (Some(b"val5-rolled"), vec![])), + (b"key7", (Some(b"val7-rolled"), vec![77])), + (b"key99", (Some(b"val99"), vec![99])), + ]; + assert_changes(&changeset, &all_changes); + + // this should be no-op + changeset.start_transaction(); + assert_eq!(changeset.transaction_depth(), 3); + changeset.start_transaction(); + assert_eq!(changeset.transaction_depth(), 4); + changeset.rollback_transaction(); + assert_eq!(changeset.transaction_depth(), 3); + changeset.commit_transaction(); + assert_eq!(changeset.transaction_depth(), 2); + assert_changes(&changeset, &all_changes); + + // roll back our first transactions that actually contains something + changeset.rollback_transaction(); + assert_eq!(changeset.transaction_depth(), 1); + + let rolled_back: Changes = vec![ + (b"key0", (Some(b"val0-1"), vec![1, 10])), + (b"key1", (Some(b"val1"), vec![1])), + (b"key42", (Some(b"val42"), vec![42])), + (b"key99", (Some(b"val99"), vec![99])), + ]; + assert_changes(&changeset, &rolled_back); + + changeset.commit_transaction(); + assert_eq!(changeset.transaction_depth(), 0); + assert_changes(&changeset, &rolled_back); + + assert_drained_changes(changeset, rolled_back); + } + + #[test] + fn transaction_commit_then_rollback_works() { + let mut changeset = OverlayedChangeSet::default(); + assert_eq!(changeset.transaction_depth(), 0); + + changeset.set(b"key0".to_vec(), Some(b"val0".to_vec()), Some(1)); + changeset.set(b"key1".to_vec(), Some(b"val1".to_vec()), Some(1)); + changeset.set(b"key0".to_vec(), Some(b"val0-1".to_vec()), Some(10)); + + changeset.start_transaction(); + assert_eq!(changeset.transaction_depth(), 1); + + changeset.set(b"key42".to_vec(), Some(b"val42".to_vec()), Some(42)); + changeset.set(b"key99".to_vec(), Some(b"val99".to_vec()), Some(99)); + + changeset.start_transaction(); + assert_eq!(changeset.transaction_depth(), 2); + + changeset.set(b"key42".to_vec(), Some(b"val42-rolled".to_vec()), Some(421)); + changeset.set(b"key7".to_vec(), Some(b"val7-rolled".to_vec()), Some(77)); + changeset.set(b"key0".to_vec(), Some(b"val0-rolled".to_vec()), Some(1000)); + changeset.set(b"key5".to_vec(), Some(b"val5-rolled".to_vec()), None); + + let all_changes: Changes = vec![ + (b"key0", (Some(b"val0-rolled"), vec![1, 10, 1000])), + (b"key1", (Some(b"val1"), vec![1])), + (b"key42", (Some(b"val42-rolled"), vec![42, 421])), + (b"key5", (Some(b"val5-rolled"), vec![])), + (b"key7", (Some(b"val7-rolled"), vec![77])), + (b"key99", (Some(b"val99"), vec![99])), + ]; + assert_changes(&changeset, &all_changes); + + // this should be no-op + changeset.start_transaction(); + assert_eq!(changeset.transaction_depth(), 3); + changeset.start_transaction(); + assert_eq!(changeset.transaction_depth(), 4); + changeset.rollback_transaction(); + assert_eq!(changeset.transaction_depth(), 3); + changeset.commit_transaction(); + assert_eq!(changeset.transaction_depth(), 2); + assert_changes(&changeset, &all_changes); + + changeset.commit_transaction(); + assert_eq!(changeset.transaction_depth(), 1); + + assert_changes(&changeset, &all_changes); + + changeset.rollback_transaction(); + assert_eq!(changeset.transaction_depth(), 0); + + let rolled_back: Changes = vec![ + (b"key0", (Some(b"val0-1"), vec![1, 10])), + (b"key1", (Some(b"val1"), vec![1])), + ]; + assert_changes(&changeset, &rolled_back); + + assert_drained_changes(changeset, rolled_back); + } + + #[test] + fn modify_works() { + let mut changeset = OverlayedChangeSet::default(); + assert_eq!(changeset.transaction_depth(), 0); + let init = || b"valinit".to_vec(); + + // comitted set + changeset.set(b"key0".to_vec(), Some(b"val0".to_vec()), Some(0)); + changeset.set(b"key1".to_vec(), None, Some(1)); + let val = changeset.modify(b"key3".to_vec(), init, Some(3)); + assert_eq!(val, &Some(b"valinit".to_vec())); + val.as_mut().unwrap().extend_from_slice(b"-modified"); + + changeset.start_transaction(); + assert_eq!(changeset.transaction_depth(), 1); + changeset.start_transaction(); + assert_eq!(changeset.transaction_depth(), 2); + + // non existing value -> init value should be returned + let val = changeset.modify(b"key2".to_vec(), init, Some(2)); + assert_eq!(val, &Some(b"valinit".to_vec())); + val.as_mut().unwrap().extend_from_slice(b"-modified"); + + // existing value should be returned by modify + let val = changeset.modify(b"key0".to_vec(), init, Some(10)); + assert_eq!(val, &Some(b"val0".to_vec())); + val.as_mut().unwrap().extend_from_slice(b"-modified"); + + // should work for deleted keys + let val = changeset.modify(b"key1".to_vec(), init, Some(20)); + assert_eq!(val, &None); + *val = Some(b"deleted-modified".to_vec()); + + let all_changes: Changes = vec![ + (b"key0", (Some(b"val0-modified"), vec![0, 10])), + (b"key1", (Some(b"deleted-modified"), vec![1, 20])), + (b"key2", (Some(b"valinit-modified"), vec![2])), + (b"key3", (Some(b"valinit-modified"), vec![3])), + ]; + assert_changes(&changeset, &all_changes); + changeset.commit_transaction(); + assert_eq!(changeset.transaction_depth(), 1); + assert_changes(&changeset, &all_changes); + + changeset.rollback_transaction(); + assert_eq!(changeset.transaction_depth(), 0); + let rolled_back: Changes = vec![ + (b"key0", (Some(b"val0"), vec![0])), + (b"key1", (None, vec![1])), + (b"key3", (Some(b"valinit-modified"), vec![3])), + ]; + assert_changes(&changeset, &rolled_back); + assert_drained_changes(changeset, rolled_back); + } + + #[test] + fn clear_works() { + let mut changeset = OverlayedChangeSet::default(); + + changeset.set(b"key0".to_vec(), Some(b"val0".to_vec()), Some(1)); + changeset.set(b"key1".to_vec(), Some(b"val1".to_vec()), Some(2)); + changeset.set(b"del1".to_vec(), Some(b"delval1".to_vec()), Some(3)); + changeset.set(b"del2".to_vec(), Some(b"delval2".to_vec()), Some(4)); + + changeset.start_transaction(); + + changeset.clear(|k, _| k.starts_with(b"del"), Some(5)); + + assert_changes(&changeset, &vec![ + (b"del1", (None, vec![3, 5])), + (b"del2", (None, vec![4, 5])), + (b"key0", (Some(b"val0"), vec![1])), + (b"key1", (Some(b"val1"), vec![2])), + ]); + + changeset.rollback_transaction(); + + assert_changes(&changeset, &vec![ + (b"del1", (Some(b"delval1"), vec![3])), + (b"del2", (Some(b"delval2"), vec![4])), + (b"key0", (Some(b"val0"), vec![1])), + (b"key1", (Some(b"val1"), vec![2])), + ]); + } + + #[test] + fn next_change_works() { + let mut changeset = OverlayedChangeSet::default(); + + changeset.set(b"key0".to_vec(), Some(b"val0".to_vec()), Some(0)); + changeset.set(b"key1".to_vec(), Some(b"val1".to_vec()), Some(1)); + changeset.set(b"key2".to_vec(), Some(b"val2".to_vec()), Some(2)); + + changeset.start_transaction(); + + changeset.set(b"key3".to_vec(), Some(b"val3".to_vec()), Some(3)); + changeset.set(b"key4".to_vec(), Some(b"val4".to_vec()), Some(4)); + changeset.set(b"key11".to_vec(), Some(b"val11".to_vec()), Some(11)); + + assert_eq!(changeset.next_change(b"key0").unwrap().0, b"key1"); + assert_eq!(changeset.next_change(b"key0").unwrap().1.value(), Some(&b"val1".to_vec())); + assert_eq!(changeset.next_change(b"key1").unwrap().0, b"key11"); + assert_eq!(changeset.next_change(b"key1").unwrap().1.value(), Some(&b"val11".to_vec())); + assert_eq!(changeset.next_change(b"key11").unwrap().0, b"key2"); + assert_eq!(changeset.next_change(b"key11").unwrap().1.value(), Some(&b"val2".to_vec())); + assert_eq!(changeset.next_change(b"key2").unwrap().0, b"key3"); + assert_eq!(changeset.next_change(b"key2").unwrap().1.value(), Some(&b"val3".to_vec())); + assert_eq!(changeset.next_change(b"key3").unwrap().0, b"key4"); + assert_eq!(changeset.next_change(b"key3").unwrap().1.value(), Some(&b"val4".to_vec())); + assert_eq!(changeset.next_change(b"key4"), None); + + changeset.rollback_transaction(); + + assert_eq!(changeset.next_change(b"key0").unwrap().0, b"key1"); + assert_eq!(changeset.next_change(b"key0").unwrap().1.value(), Some(&b"val1".to_vec())); + assert_eq!(changeset.next_change(b"key1").unwrap().0, b"key2"); + assert_eq!(changeset.next_change(b"key1").unwrap().1.value(), Some(&b"val2".to_vec())); + assert_eq!(changeset.next_change(b"key11").unwrap().0, b"key2"); + assert_eq!(changeset.next_change(b"key11").unwrap().1.value(), Some(&b"val2".to_vec())); + assert_eq!(changeset.next_change(b"key2"), None); + assert_eq!(changeset.next_change(b"key3"), None); + assert_eq!(changeset.next_change(b"key4"), None); + + } + + #[test] + #[should_panic] + fn no_open_tx_commit_panics() { + let mut changeset = OverlayedChangeSet::default(); + assert_eq!(changeset.transaction_depth(), 0); + changeset.commit_transaction(); + } + + #[test] + #[should_panic] + fn no_open_tx_rollback_panics() { + let mut changeset = OverlayedChangeSet::default(); + assert_eq!(changeset.transaction_depth(), 0); + changeset.rollback_transaction(); + } + + #[test] + #[should_panic] + fn unbalanced_transactions_panics() { + let mut changeset = OverlayedChangeSet::default(); + changeset.start_transaction(); + changeset.commit_transaction(); + changeset.commit_transaction(); + } + + #[test] + #[should_panic] + fn drain_with_open_transaction_panics() { + let mut changeset = OverlayedChangeSet::default(); + changeset.start_transaction(); + let _ = changeset.drain_commited(); + } +} diff --git a/primitives/state-machine/src/testing.rs b/primitives/state-machine/src/testing.rs index 71124a68bb5cf..9d7e0a8e26fcf 100644 --- a/primitives/state-machine/src/testing.rs +++ b/primitives/state-machine/src/testing.rs @@ -136,15 +136,15 @@ impl TestExternalities /// Return a new backend with all pending value. pub fn commit_all(&self) -> InMemoryBackend { - let top: Vec<_> = self.overlay.changes(None) + let top: Vec<_> = self.overlay.changes() .map(|(k, v)| (k.clone(), v.value().cloned())) .collect(); let mut transaction = vec![(None, top)]; - for child_info in self.overlay.child_infos() { + for (child_changes, child_info) in self.overlay.children() { transaction.push(( Some(child_info.clone()), - self.overlay.changes(Some(child_info)) + child_changes .map(|(k, v)| (k.clone(), v.value().cloned())) .collect(), )) From 68e34f043c1f577fac5f97467f27757a2b4c6862 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Fri, 5 Jun 2020 12:02:01 +0200 Subject: [PATCH 02/38] Add storage transactions runtime interface --- primitives/externalities/src/lib.rs | 25 +++++++++++++++ primitives/io/src/lib.rs | 37 +++++++++++++++++++++++ primitives/state-machine/src/basic.rs | 12 ++++++++ primitives/state-machine/src/ext.rs | 12 ++++++++ primitives/state-machine/src/read_only.rs | 12 ++++++++ 5 files changed, 98 insertions(+) diff --git a/primitives/externalities/src/lib.rs b/primitives/externalities/src/lib.rs index cfb1d0878a491..4cf8690e38dcb 100644 --- a/primitives/externalities/src/lib.rs +++ b/primitives/externalities/src/lib.rs @@ -195,6 +195,31 @@ pub trait Externalities: ExtensionStore { /// The returned hash is defined by the `Block` and is SCALE encoded. fn storage_changes_root(&mut self, parent: &[u8]) -> Result>, ()>; + /// Start a new nested transaction. + /// + /// This allows to either commit or roll back all changes that are made after this call. + /// For every transaction there must be a matching call to either `storage_rollback_transaction` + /// or `storage_commit_transaction`. + /// + /// Changes made without any open transaction are comitted immediatly + fn storage_start_transaction(&mut self); + + /// Rollback the last transaction started by `start_transaction`. + /// + /// Any changes made during that transaction are discarded. + /// + /// Panics: + /// Will panic if there is no open transaction. + fn storage_rollback_transaction(&mut self); + + /// Commit the last transaction started by `start_transaction`. + /// + /// Any changes made during that transaction are committed. + /// + /// Panics: + /// Will panic if there is no open transaction. + fn storage_commit_transaction(&mut self); + /// !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! /// Benchmarking related functionality and shouldn't be used anywhere else! /// !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! diff --git a/primitives/io/src/lib.rs b/primitives/io/src/lib.rs index 8d81a84c4c88a..8b2c0dccbb2fd 100644 --- a/primitives/io/src/lib.rs +++ b/primitives/io/src/lib.rs @@ -155,6 +155,43 @@ pub trait Storage { fn next_key(&mut self, key: &[u8]) -> Option> { self.next_storage_key(&key) } + + /// Start a new nested transaction. + /// + /// This allows to either commit or roll back all changes that are made after this call. + /// For every transaction there must be a matching call to either `rollback_transaction` + /// or `commit_transaction`. This is also effective for all values manipulated using the + /// `DefaultChildStorage` API. + /// + /// # Warning + /// + /// This is a low level API that is potentially dangerous as it can easily result + /// in unbalanced transactions. FRAME users should use high level storage abstractions. + fn start_transaction(&mut self) { + self.storage_start_transaction(); + } + + /// Rollback the last transaction started by `start_transaction`. + /// + /// Any changes made during that transaction are discarded. + /// + /// # Panics + /// + /// Will panic if there is no open transaction. + fn rollback_transaction(&mut self) { + self.storage_rollback_transaction(); + } + + /// Commit the last transaction started by `start_transaction`. + /// + /// Any changes made during that transaction are committed. + /// + /// # Panics + /// + /// Will panic if there is no open transaction. + fn commit_transaction(&mut self) { + self.storage_commit_transaction(); + } } /// Interface for accessing the child storage for default child trie, diff --git a/primitives/state-machine/src/basic.rs b/primitives/state-machine/src/basic.rs index 917e41f33d78b..0366befe70cfc 100644 --- a/primitives/state-machine/src/basic.rs +++ b/primitives/state-machine/src/basic.rs @@ -307,6 +307,18 @@ impl Externalities for BasicExternalities { Ok(None) } + fn storage_start_transaction(&mut self) { + unimplemented!("Transactions are not supported by BasicExternalities"); + } + + fn storage_rollback_transaction(&mut self) { + unimplemented!("Transactions are not supported by BasicExternalities"); + } + + fn storage_commit_transaction(&mut self) { + unimplemented!("Transactions are not supported by BasicExternalities"); + } + fn wipe(&mut self) {} fn commit(&mut self) {} diff --git a/primitives/state-machine/src/ext.rs b/primitives/state-machine/src/ext.rs index f98dbb707ef59..05f82ad5bd84a 100644 --- a/primitives/state-machine/src/ext.rs +++ b/primitives/state-machine/src/ext.rs @@ -546,6 +546,18 @@ where root.map(|r| r.map(|o| o.encode())) } + fn storage_start_transaction(&mut self) { + self.overlay.start_transaction(); + } + + fn storage_rollback_transaction(&mut self) { + self.overlay.rollback_transaction(); + } + + fn storage_commit_transaction(&mut self) { + self.overlay.commit_transaction(); + } + fn wipe(&mut self) { for _ in 0..self.overlay.transaction_depth() { self.overlay.rollback_transaction(); diff --git a/primitives/state-machine/src/read_only.rs b/primitives/state-machine/src/read_only.rs index 817282f8e71a3..2c31cc672c3e8 100644 --- a/primitives/state-machine/src/read_only.rs +++ b/primitives/state-machine/src/read_only.rs @@ -170,6 +170,18 @@ impl<'a, H: Hasher, B: 'a + Backend> Externalities for ReadOnlyExternalities< unimplemented!("storage_changes_root is not supported in ReadOnlyExternalities") } + fn storage_start_transaction(&mut self) { + unimplemented!("Transactions are not supported by ReadOnlyExternalities"); + } + + fn storage_rollback_transaction(&mut self) { + unimplemented!("Transactions are not supported by ReadOnlyExternalities"); + } + + fn storage_commit_transaction(&mut self) { + unimplemented!("Transactions are not supported by ReadOnlyExternalities"); + } + fn wipe(&mut self) {} fn commit(&mut self) {} From 0a7019149286fb43e42095b09f9fc86e2bc12821 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Fri, 5 Jun 2020 12:17:55 +0200 Subject: [PATCH 03/38] Add frame support for transactions --- frame/support/src/storage/mod.rs | 18 ++ .../support/test/tests/storage_transaction.rs | 157 ++++++++++++++++++ 2 files changed, 175 insertions(+) create mode 100644 frame/support/test/tests/storage_transaction.rs diff --git a/frame/support/src/storage/mod.rs b/frame/support/src/storage/mod.rs index 6d0ef91ce1e17..94704afe2cc3d 100644 --- a/frame/support/src/storage/mod.rs +++ b/frame/support/src/storage/mod.rs @@ -29,6 +29,24 @@ pub mod child; pub mod generator; pub mod migration; +/// Execute the supplied function in a new storage transaction. +/// +/// All changes to storage performed by the supplied function are discarded if an +/// error is returned and comitted on success. +/// +/// Transactions can be nested to any depth. Commits happen to the parent transaction. +pub fn with_transaction(f: F) -> Result where + F: FnOnce() -> Result +{ + sp_io::storage::start_transaction(); + let result = f(); + match result { + Ok(_) => sp_io::storage::commit_transaction(), + Err(_) => sp_io::storage::rollback_transaction(), + } + result +} + /// A trait for working with macro-generated storage values under the substrate storage API. /// /// Details on implementation can be found at diff --git a/frame/support/test/tests/storage_transaction.rs b/frame/support/test/tests/storage_transaction.rs new file mode 100644 index 0000000000000..9d9856cce05c2 --- /dev/null +++ b/frame/support/test/tests/storage_transaction.rs @@ -0,0 +1,157 @@ +// This file is part of Substrate. + +// Copyright (C) 2019-2020 Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: Apache-2.0 + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use codec::{Encode, Decode, EncodeLike}; +use frame_support::{StorageMap, StorageValue, storage::with_transaction}; +use sp_io::TestExternalities; + +pub trait Trait { + type Origin; + type BlockNumber: Encode + Decode + EncodeLike + Default + Clone; +} + +frame_support::decl_module! { + pub struct Module for enum Call where origin: T::Origin {} +} + +frame_support::decl_storage!{ + trait Store for Module as StorageTransactions { + pub Value: u32; + pub Map: map hasher(twox_64_concat) String => u32; + } +} + + +#[test] +fn storage_transaction_basic_commit() { + TestExternalities::default().execute_with(|| { + + assert_eq!(Value::get(), 0); + assert!(!Map::contains_key("val0")); + + let _: Result<(), ()> = with_transaction(|| { + Value::set(99); + Map::insert("val0", 99); + assert_eq!(Value::get(), 99); + assert_eq!(Map::get("val0"), 99); + Ok(()) + }); + + assert_eq!(Value::get(), 99); + assert_eq!(Map::get("val0"), 99); + }); +} + +#[test] +fn storage_transaction_basic_rollback() { + TestExternalities::default().execute_with(|| { + + assert_eq!(Value::get(), 0); + assert_eq!(Map::get("val0"), 0); + + let _: Result<(), ()> = with_transaction(|| { + Value::set(99); + Map::insert("val0", 99); + assert_eq!(Value::get(), 99); + assert_eq!(Map::get("val0"), 99); + Err(()) + }); + + assert_eq!(Value::get(), 0); + assert_eq!(Map::get("val0"), 0); + }); +} + +#[test] +fn storage_transaction_rollback_then_commit() { + TestExternalities::default().execute_with(|| { + Value::set(1); + Map::insert("val1", 1); + + let _: Result<(), ()> = with_transaction(|| { + Value::set(2); + Map::insert("val1", 2); + Map::insert("val2", 2); + + let _: Result<(), ()> = with_transaction(|| { + Value::set(3); + Map::insert("val1", 3); + Map::insert("val2", 3); + Map::insert("val3", 3); + + assert_eq!(Value::get(), 3); + assert_eq!(Map::get("val1"), 3); + assert_eq!(Map::get("val2"), 3); + assert_eq!(Map::get("val3"), 3); + + Err(()) + }); + + assert_eq!(Value::get(), 2); + assert_eq!(Map::get("val1"), 2); + assert_eq!(Map::get("val2"), 2); + assert_eq!(Map::get("val3"), 0); + + Ok(()) + }); + + assert_eq!(Value::get(), 2); + assert_eq!(Map::get("val1"), 2); + assert_eq!(Map::get("val2"), 2); + assert_eq!(Map::get("val3"), 0); + }); +} + +#[test] +fn storage_transaction_commit_then_rollback() { + TestExternalities::default().execute_with(|| { + Value::set(1); + Map::insert("val1", 1); + + let _: Result<(), ()> = with_transaction(|| { + Value::set(2); + Map::insert("val1", 2); + Map::insert("val2", 2); + + let _: Result<(), ()> = with_transaction(|| { + Value::set(3); + Map::insert("val1", 3); + Map::insert("val2", 3); + Map::insert("val3", 3); + + assert_eq!(Value::get(), 3); + assert_eq!(Map::get("val1"), 3); + assert_eq!(Map::get("val2"), 3); + assert_eq!(Map::get("val3"), 3); + + Ok(()) + }); + + assert_eq!(Value::get(), 3); + assert_eq!(Map::get("val1"), 3); + assert_eq!(Map::get("val2"), 3); + assert_eq!(Map::get("val3"), 3); + + Err(()) + }); + + assert_eq!(Value::get(), 1); + assert_eq!(Map::get("val1"), 1); + assert_eq!(Map::get("val2"), 0); + assert_eq!(Map::get("val3"), 0); + }); +} From c767eea0be2f4ee50f2885ee1b2e09f56b3fe30a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Mon, 8 Jun 2020 10:39:04 +0200 Subject: [PATCH 04/38] Fix committed typo --- frame/support/src/storage/mod.rs | 2 +- primitives/externalities/src/lib.rs | 2 +- .../state-machine/src/overlayed_changes.rs | 16 ++++++++-------- .../src/overlayed_changes/changeset.rs | 12 ++++++------ 4 files changed, 16 insertions(+), 16 deletions(-) diff --git a/frame/support/src/storage/mod.rs b/frame/support/src/storage/mod.rs index 94704afe2cc3d..36dc852e83320 100644 --- a/frame/support/src/storage/mod.rs +++ b/frame/support/src/storage/mod.rs @@ -32,7 +32,7 @@ pub mod migration; /// Execute the supplied function in a new storage transaction. /// /// All changes to storage performed by the supplied function are discarded if an -/// error is returned and comitted on success. +/// error is returned and committed on success. /// /// Transactions can be nested to any depth. Commits happen to the parent transaction. pub fn with_transaction(f: F) -> Result where diff --git a/primitives/externalities/src/lib.rs b/primitives/externalities/src/lib.rs index 4cf8690e38dcb..9a55aa650de58 100644 --- a/primitives/externalities/src/lib.rs +++ b/primitives/externalities/src/lib.rs @@ -201,7 +201,7 @@ pub trait Externalities: ExtensionStore { /// For every transaction there must be a matching call to either `storage_rollback_transaction` /// or `storage_commit_transaction`. /// - /// Changes made without any open transaction are comitted immediatly + /// Changes made without any open transaction are committed immediatly fn storage_start_transaction(&mut self); /// Rollback the last transaction started by `start_transaction`. diff --git a/primitives/state-machine/src/overlayed_changes.rs b/primitives/state-machine/src/overlayed_changes.rs index a4f254929dc64..16388b9188301 100644 --- a/primitives/state-machine/src/overlayed_changes.rs +++ b/primitives/state-machine/src/overlayed_changes.rs @@ -184,7 +184,7 @@ impl OverlayedChanges { /// If there is no value in the overlay, the default callback is used to initiate the value. /// Warning this function registers a change, so the mutable reference MUST be modified. /// - /// Can be rolled back or comitted when called inside a transaction. + /// Can be rolled back or committed when called inside a transaction. #[must_use = "A change was registered, so this value MUST be modified."] pub fn value_mut_or_insert_with( &mut self, @@ -217,7 +217,7 @@ impl OverlayedChanges { /// Set a new value for the specified key. /// - /// Can be rolled back or comitted when called inside a transaction. + /// Can be rolled back or committed when called inside a transaction. pub(crate) fn set_storage(&mut self, key: StorageKey, val: Option) { let size_write = val.as_ref().map(|x| x.len() as u64).unwrap_or(0); self.stats.tally_write_overlay(size_write); @@ -228,7 +228,7 @@ impl OverlayedChanges { /// /// `None` can be used to delete a value specified by the given key. /// - /// Can be rolled back or comitted when called inside a transaction. + /// Can be rolled back or committed when called inside a transaction. pub(crate) fn set_child_storage( &mut self, child_info: &ChildInfo, @@ -254,7 +254,7 @@ impl OverlayedChanges { /// Clear child storage of given storage key. /// - /// Can be rolled back or comitted when called inside a transaction. + /// Can be rolled back or committed when called inside a transaction. pub(crate) fn clear_child_storage( &mut self, child_info: &ChildInfo, @@ -270,14 +270,14 @@ impl OverlayedChanges { /// Removes all key-value pairs which keys share the given prefix. /// - /// Can be rolled back or comitted when called inside a transaction. + /// Can be rolled back or committed when called inside a transaction. pub(crate) fn clear_prefix(&mut self, prefix: &[u8]) { self.top.clear(|key, _| key.starts_with(prefix), self.extrinsic_index()); } /// Removes all key-value pairs which keys share the given prefix. /// - /// Can be rolled back or comitted when called inside a transaction + /// Can be rolled back or committed when called inside a transaction pub(crate) fn clear_child_prefix( &mut self, child_info: &ChildInfo, @@ -294,7 +294,7 @@ impl OverlayedChanges { /// Returns the current nesting depth of the transaction stack. /// - /// A value of zero means that no transaction is open and changes are comitted on write. + /// A value of zero means that no transaction is open and changes are committed on write. pub fn transaction_depth(&self) -> usize { // The top changeset and all child changesets transact in lockstep and are // therefore always at the same transaction depth. @@ -307,7 +307,7 @@ impl OverlayedChanges { /// transaction was open. Any transaction must be closed by one of the aforementioned /// functions before this overlay can be converted into storage changes. /// - /// Changes made without any open transaction are comitted immediatly. + /// Changes made without any open transaction are committed immediatly. pub fn start_transaction(&mut self) { self.top.start_transaction(); for (_, (changeset, _)) in self.children.iter_mut() { diff --git a/primitives/state-machine/src/overlayed_changes/changeset.rs b/primitives/state-machine/src/overlayed_changes/changeset.rs index 328cfda34ff30..5cededf86bea7 100644 --- a/primitives/state-machine/src/overlayed_changes/changeset.rs +++ b/primitives/state-machine/src/overlayed_changes/changeset.rs @@ -155,7 +155,7 @@ impl OverlayedChangeSet { /// Set a new value for the specified key. /// - /// Can be rolled back or comitted when called inside a transaction. + /// Can be rolled back or committed when called inside a transaction. pub fn set( &mut self, key: StorageKey, @@ -168,7 +168,7 @@ impl OverlayedChangeSet { /// Get a mutable reference for a value. /// - /// Can be rolled back or comitted when called inside a transaction. + /// Can be rolled back or committed when called inside a transaction. #[must_use = "A change was registered, so this value MUST be modified."] pub fn modify( &mut self, @@ -196,7 +196,7 @@ impl OverlayedChangeSet { /// Set all values to deleted which are matched by the predicate. /// - /// Can be rolled back or comitted when called inside a transaction. + /// Can be rolled back or committed when called inside a transaction. pub fn clear( &mut self, predicate: impl Fn(&[u8], &OverlayedValue) -> bool, @@ -230,7 +230,7 @@ impl OverlayedChangeSet { /// Returns the current nesting depth of the transaction stack. /// - /// A value of zero means that no transaction is open and changes are comitted on write. + /// A value of zero means that no transaction is open and changes are committed on write. pub fn transaction_depth(&self) -> usize { self.dirty_keys.len() } @@ -241,7 +241,7 @@ impl OverlayedChangeSet { /// transaction was open. Any transaction must be closed by one of the aforementioned /// functions before this overlay can be converted into storage changes. /// - /// Changes made without any open transaction are comitted immediatly. + /// Changes made without any open transaction are committed immediatly. pub fn start_transaction(&mut self) { self.dirty_keys.push(Default::default()); } @@ -481,7 +481,7 @@ mod test { assert_eq!(changeset.transaction_depth(), 0); let init = || b"valinit".to_vec(); - // comitted set + // committed set changeset.set(b"key0".to_vec(), Some(b"val0".to_vec()), Some(0)); changeset.set(b"key1".to_vec(), None, Some(1)); let val = changeset.modify(b"key3".to_vec(), init, Some(3)); From c48d41734e66126ff67d37bf3d6ef746ade259c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Mon, 8 Jun 2020 10:51:21 +0200 Subject: [PATCH 05/38] Rename 'changes' variable to 'overlay' --- primitives/state-machine/src/changes_trie/build.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/primitives/state-machine/src/changes_trie/build.rs b/primitives/state-machine/src/changes_trie/build.rs index 5a4db0c656033..87ddc3b8c1086 100644 --- a/primitives/state-machine/src/changes_trie/build.rs +++ b/primitives/state-machine/src/changes_trie/build.rs @@ -43,7 +43,7 @@ pub(crate) fn prepare_input<'a, B, H, Number>( backend: &'a B, storage: &'a dyn Storage, config: ConfigurationRange<'a, Number>, - changes: &'a OverlayedChanges, + overlay: &'a OverlayedChanges, parent: &'a AnchorBlockId, ) -> Result<( impl Iterator> + 'a, @@ -60,7 +60,7 @@ pub(crate) fn prepare_input<'a, B, H, Number>( let (extrinsics_input, children_extrinsics_input) = prepare_extrinsics_input( backend, &number, - changes, + overlay, )?; let (digest_input, mut children_digest_input, digest_input_blocks) = prepare_digest_input::( parent, @@ -96,7 +96,7 @@ pub(crate) fn prepare_input<'a, B, H, Number>( fn prepare_extrinsics_input<'a, B, H, Number>( backend: &'a B, block: &Number, - changes: &'a OverlayedChanges, + overlay: &'a OverlayedChanges, ) -> Result<( impl Iterator> + 'a, BTreeMap, impl Iterator> + 'a>, @@ -108,7 +108,7 @@ fn prepare_extrinsics_input<'a, B, H, Number>( { let mut children_result = BTreeMap::new(); - for (child_changes, child_info) in changes.children() { + for (child_changes, child_info) in overlay.children() { let child_index = ChildIndex:: { block: block.clone(), storage_key: child_info.prefixed_storage_key(), @@ -122,7 +122,7 @@ fn prepare_extrinsics_input<'a, B, H, Number>( children_result.insert(child_index, iter); } - let top = prepare_extrinsics_input_inner(backend, block, changes, None, changes.changes())?; + let top = prepare_extrinsics_input_inner(backend, block, changes, None, overlay.changes())?; Ok((top, children_result)) } From 98a89da8f2f66220885d3b772c770714105c120f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Mon, 8 Jun 2020 11:06:01 +0200 Subject: [PATCH 06/38] Fix renaming change --- primitives/state-machine/src/changes_trie/build.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/primitives/state-machine/src/changes_trie/build.rs b/primitives/state-machine/src/changes_trie/build.rs index 87ddc3b8c1086..9733c1df17c87 100644 --- a/primitives/state-machine/src/changes_trie/build.rs +++ b/primitives/state-machine/src/changes_trie/build.rs @@ -115,14 +115,14 @@ fn prepare_extrinsics_input<'a, B, H, Number>( }; let iter = prepare_extrinsics_input_inner( - backend, block, changes, + backend, block, overlay, Some(child_info.clone()), child_changes, )?; children_result.insert(child_index, iter); } - let top = prepare_extrinsics_input_inner(backend, block, changes, None, overlay.changes())?; + let top = prepare_extrinsics_input_inner(backend, block, overlay, None, overlay.changes())?; Ok((top, children_result)) } From 3fb2e05a5954f72d3f9e1ee7f598642033bb445c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Mon, 8 Jun 2020 13:41:54 +0200 Subject: [PATCH 07/38] Fixed strange line break --- primitives/state-machine/src/overlayed_changes.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/primitives/state-machine/src/overlayed_changes.rs b/primitives/state-machine/src/overlayed_changes.rs index 16388b9188301..be009c93bbdc3 100644 --- a/primitives/state-machine/src/overlayed_changes.rs +++ b/primitives/state-machine/src/overlayed_changes.rs @@ -376,8 +376,8 @@ impl OverlayedChanges { } /// Get an optional iterator over all child changes stored under the supplied key. - pub fn child_changes(&self, key: &[u8] - ) -> Option<(impl Iterator, &ChildInfo)> { + pub fn child_changes(&self, key: &[u8]) + -> Option<(impl Iterator, &ChildInfo)> { self.children.get(key).map(|(overlay, info)| (overlay.changes(), info)) } From 1acf7a4246efe7a60b1533444ac69b5e72aa8d73 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Mon, 8 Jun 2020 13:46:35 +0200 Subject: [PATCH 08/38] Rename clear to clear_where --- primitives/state-machine/src/overlayed_changes.rs | 6 +++--- primitives/state-machine/src/overlayed_changes/changeset.rs | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/primitives/state-machine/src/overlayed_changes.rs b/primitives/state-machine/src/overlayed_changes.rs index be009c93bbdc3..9879301b52d8e 100644 --- a/primitives/state-machine/src/overlayed_changes.rs +++ b/primitives/state-machine/src/overlayed_changes.rs @@ -265,14 +265,14 @@ impl OverlayedChanges { .or_insert_with(|| (Default::default(), child_info.to_owned())); let updatable = info.try_update(child_info); debug_assert!(updatable); - changeset.clear(|_, _| true, extrinsic_index); + changeset.clear_where(|_, _| true, extrinsic_index); } /// Removes all key-value pairs which keys share the given prefix. /// /// Can be rolled back or committed when called inside a transaction. pub(crate) fn clear_prefix(&mut self, prefix: &[u8]) { - self.top.clear(|key, _| key.starts_with(prefix), self.extrinsic_index()); + self.top.clear_where(|key, _| key.starts_with(prefix), self.extrinsic_index()); } /// Removes all key-value pairs which keys share the given prefix. @@ -289,7 +289,7 @@ impl OverlayedChanges { .or_insert_with(|| (Default::default(), child_info.to_owned())); let updatable = info.try_update(child_info); debug_assert!(updatable); - changeset.clear(|key, _| key.starts_with(prefix), extrinsic_index); + changeset.clear_where(|key, _| key.starts_with(prefix), extrinsic_index); } /// Returns the current nesting depth of the transaction stack. diff --git a/primitives/state-machine/src/overlayed_changes/changeset.rs b/primitives/state-machine/src/overlayed_changes/changeset.rs index 5cededf86bea7..dd67e8011d150 100644 --- a/primitives/state-machine/src/overlayed_changes/changeset.rs +++ b/primitives/state-machine/src/overlayed_changes/changeset.rs @@ -197,7 +197,7 @@ impl OverlayedChangeSet { /// Set all values to deleted which are matched by the predicate. /// /// Can be rolled back or committed when called inside a transaction. - pub fn clear( + pub fn clear_where( &mut self, predicate: impl Fn(&[u8], &OverlayedValue) -> bool, at_extrinsic: Option @@ -541,7 +541,7 @@ mod test { changeset.start_transaction(); - changeset.clear(|k, _| k.starts_with(b"del"), Some(5)); + changeset.clear_where(|k, _| k.starts_with(b"del"), Some(5)); assert_changes(&changeset, &vec![ (b"del1", (None, vec![3, 5])), From d4440a401a363de263331840b49b2cee9dbe54e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Mon, 8 Jun 2020 13:48:54 +0200 Subject: [PATCH 09/38] Add comment regarding delete value on mutation --- primitives/state-machine/src/overlayed_changes.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/primitives/state-machine/src/overlayed_changes.rs b/primitives/state-machine/src/overlayed_changes.rs index 9879301b52d8e..62760aa7fdcb1 100644 --- a/primitives/state-machine/src/overlayed_changes.rs +++ b/primitives/state-machine/src/overlayed_changes.rs @@ -193,6 +193,7 @@ impl OverlayedChanges { ) -> &mut StorageValue { let value = self.top.modify(key.to_owned(), init, self.extrinsic_index()); + // if the value was deleted initialise it back with an empty vec if value.is_none() { *value = Some(Default::default()); } From 359224605bd4514cf329978f39f6354620d89c77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Mon, 8 Jun 2020 17:43:33 +0200 Subject: [PATCH 10/38] Add comment which changes are covered by a transaction --- primitives/externalities/src/lib.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/primitives/externalities/src/lib.rs b/primitives/externalities/src/lib.rs index 9a55aa650de58..983f7bada7b8a 100644 --- a/primitives/externalities/src/lib.rs +++ b/primitives/externalities/src/lib.rs @@ -197,9 +197,9 @@ pub trait Externalities: ExtensionStore { /// Start a new nested transaction. /// - /// This allows to either commit or roll back all changes that are made after this call. - /// For every transaction there must be a matching call to either `storage_rollback_transaction` - /// or `storage_commit_transaction`. + /// This allows to either commit or roll back all changes made after this call to the + /// top changes or the default child changes. For every transaction there must be a + /// matching call to either `storage_rollback_transaction` or `storage_commit_transaction`. /// /// Changes made without any open transaction are committed immediatly fn storage_start_transaction(&mut self); From 7395a03960da02d351b4768b6ec8c279c3f6763e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Mon, 8 Jun 2020 18:00:44 +0200 Subject: [PATCH 11/38] Do force the arg to with_transaction return a Result --- frame/support/src/storage/mod.rs | 31 +++++++++++++------ .../support/test/tests/storage_transaction.rs | 28 +++++++++-------- 2 files changed, 36 insertions(+), 23 deletions(-) diff --git a/frame/support/src/storage/mod.rs b/frame/support/src/storage/mod.rs index 36dc852e83320..58d1462f69fad 100644 --- a/frame/support/src/storage/mod.rs +++ b/frame/support/src/storage/mod.rs @@ -29,20 +29,31 @@ pub mod child; pub mod generator; pub mod migration; +/// Describes whether a storage transaction should be committed or rolled back. +pub enum TransactionOutcome { + /// Transaction should be committed. + Commit, + /// Transaction should be rolled back. + Rollback, +} + /// Execute the supplied function in a new storage transaction. /// -/// All changes to storage performed by the supplied function are discarded if an -/// error is returned and committed on success. +/// All changes to storage performed by the supplied function are discarded if the returned +/// outcome is `TransactionOutcome::Discard`. /// /// Transactions can be nested to any depth. Commits happen to the parent transaction. -pub fn with_transaction(f: F) -> Result where - F: FnOnce() -> Result -{ - sp_io::storage::start_transaction(); - let result = f(); - match result { - Ok(_) => sp_io::storage::commit_transaction(), - Err(_) => sp_io::storage::rollback_transaction(), +pub fn with_transaction(f: impl FnOnce() -> (R, TransactionOutcome)) -> R { + use sp_io::storage::{ + start_transaction, commit_transaction, rollback_transaction, + }; + use TransactionOutcome::*; + + start_transaction(); + let (result, outcome) = f(); + match outcome { + Commit => commit_transaction(), + Rollback => rollback_transaction(), } result } diff --git a/frame/support/test/tests/storage_transaction.rs b/frame/support/test/tests/storage_transaction.rs index 9d9856cce05c2..136e665c72af2 100644 --- a/frame/support/test/tests/storage_transaction.rs +++ b/frame/support/test/tests/storage_transaction.rs @@ -16,7 +16,9 @@ // limitations under the License. use codec::{Encode, Decode, EncodeLike}; -use frame_support::{StorageMap, StorageValue, storage::with_transaction}; +use frame_support::{ + StorageMap, StorageValue, storage::{with_transaction, TransactionOutcome::*}, +}; use sp_io::TestExternalities; pub trait Trait { @@ -43,12 +45,12 @@ fn storage_transaction_basic_commit() { assert_eq!(Value::get(), 0); assert!(!Map::contains_key("val0")); - let _: Result<(), ()> = with_transaction(|| { + with_transaction(|| { Value::set(99); Map::insert("val0", 99); assert_eq!(Value::get(), 99); assert_eq!(Map::get("val0"), 99); - Ok(()) + ((), Commit) }); assert_eq!(Value::get(), 99); @@ -63,12 +65,12 @@ fn storage_transaction_basic_rollback() { assert_eq!(Value::get(), 0); assert_eq!(Map::get("val0"), 0); - let _: Result<(), ()> = with_transaction(|| { + with_transaction(|| { Value::set(99); Map::insert("val0", 99); assert_eq!(Value::get(), 99); assert_eq!(Map::get("val0"), 99); - Err(()) + ((), Rollback) }); assert_eq!(Value::get(), 0); @@ -82,12 +84,12 @@ fn storage_transaction_rollback_then_commit() { Value::set(1); Map::insert("val1", 1); - let _: Result<(), ()> = with_transaction(|| { + with_transaction(|| { Value::set(2); Map::insert("val1", 2); Map::insert("val2", 2); - let _: Result<(), ()> = with_transaction(|| { + with_transaction(|| { Value::set(3); Map::insert("val1", 3); Map::insert("val2", 3); @@ -98,7 +100,7 @@ fn storage_transaction_rollback_then_commit() { assert_eq!(Map::get("val2"), 3); assert_eq!(Map::get("val3"), 3); - Err(()) + ((), Rollback) }); assert_eq!(Value::get(), 2); @@ -106,7 +108,7 @@ fn storage_transaction_rollback_then_commit() { assert_eq!(Map::get("val2"), 2); assert_eq!(Map::get("val3"), 0); - Ok(()) + ((), Commit) }); assert_eq!(Value::get(), 2); @@ -122,12 +124,12 @@ fn storage_transaction_commit_then_rollback() { Value::set(1); Map::insert("val1", 1); - let _: Result<(), ()> = with_transaction(|| { + with_transaction(|| { Value::set(2); Map::insert("val1", 2); Map::insert("val2", 2); - let _: Result<(), ()> = with_transaction(|| { + with_transaction(|| { Value::set(3); Map::insert("val1", 3); Map::insert("val2", 3); @@ -138,7 +140,7 @@ fn storage_transaction_commit_then_rollback() { assert_eq!(Map::get("val2"), 3); assert_eq!(Map::get("val3"), 3); - Ok(()) + ((), Commit) }); assert_eq!(Value::get(), 3); @@ -146,7 +148,7 @@ fn storage_transaction_commit_then_rollback() { assert_eq!(Map::get("val2"), 3); assert_eq!(Map::get("val3"), 3); - Err(()) + ((), Rollback) }); assert_eq!(Value::get(), 1); From 81e52417dbf0a56b5af0bfee1f9fc12d35bc7b49 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Tue, 9 Jun 2020 11:45:57 +0200 Subject: [PATCH 12/38] Use rust doc comments on every documentable place --- .../src/overlayed_changes/changeset.rs | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/primitives/state-machine/src/overlayed_changes/changeset.rs b/primitives/state-machine/src/overlayed_changes/changeset.rs index dd67e8011d150..90f0bbeca4ad0 100644 --- a/primitives/state-machine/src/overlayed_changes/changeset.rs +++ b/primitives/state-machine/src/overlayed_changes/changeset.rs @@ -54,8 +54,8 @@ struct InnerValue { #[derive(Debug, Default, Clone)] #[cfg_attr(test, derive(PartialEq))] pub struct OverlayedValue { - // The individual versions of that value. - // One entry per transactions during that the value was actually written. + /// The individual versions of that value. + /// One entry per transactions during that the value was actually written. transactions: Transactions, } @@ -81,25 +81,25 @@ impl OverlayedValue { self.transactions.iter().flat_map(|t| t.extrinsics.iter()).unique() } - // Mutable reference to the most recent version. + /// Mutable reference to the most recent version. fn value_mut(&mut self) -> &mut Option { &mut self.transactions.last_mut().expect(PROOF_OVERLAY_NON_EMPTY).value } - // Remove the last version and return it. + /// Remove the last version and return it. fn pop_transaction(&mut self) -> InnerValue { self.transactions.pop().expect(PROOF_OVERLAY_NON_EMPTY) } - // Mutable reference to the set which holds the indices for the **current transaction only**. + /// Mutable reference to the set which holds the indices for the **current transaction only**. fn transaction_extrinsics_mut(&mut self) -> &mut BTreeSet { &mut self.transactions.last_mut().expect(PROOF_OVERLAY_NON_EMPTY).extrinsics } - // Writes a new version of a value. - // - // This makes sure that the old version is not overwritten and can be properly - // rolled back when required. + /// Writes a new version of a value. + /// + /// This makes sure that the old version is not overwritten and can be properly + /// rolled back when required. fn set( &mut self, value: Option, @@ -121,10 +121,10 @@ impl OverlayedValue { } } -// Inserts a key into the dirty set. -// -// Returns true iff we are currently have at least one open transaction and if this -// is the first write to that transaction. +/// Inserts a key into the dirty set. +/// +/// Returns true iff we are currently have at least one open transaction and if this +/// is the first write to that transaction. fn insert_dirty(set: &mut DirtyKeys, key: StorageKey) -> bool { if let Some(dirty_keys) = set.last_mut() { dirty_keys.insert(key) @@ -261,7 +261,7 @@ impl OverlayedChangeSet { /// Any changes made during that transaction are committed. /// /// Panics: - /// Will panic if there is no open transaction + /// Will panic if there is no open transaction. pub fn commit_transaction(&mut self) { self.close_transaction(false); } From ff5210121efa9b68bb293251b9913c546faacc17 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Tue, 9 Jun 2020 11:47:29 +0200 Subject: [PATCH 13/38] Fix wording of insert_diry doc --- primitives/state-machine/src/overlayed_changes/changeset.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/primitives/state-machine/src/overlayed_changes/changeset.rs b/primitives/state-machine/src/overlayed_changes/changeset.rs index 90f0bbeca4ad0..7b4bf52bf0f55 100644 --- a/primitives/state-machine/src/overlayed_changes/changeset.rs +++ b/primitives/state-machine/src/overlayed_changes/changeset.rs @@ -124,7 +124,7 @@ impl OverlayedValue { /// Inserts a key into the dirty set. /// /// Returns true iff we are currently have at least one open transaction and if this -/// is the first write to that transaction. +/// is the first write to the given key that transaction. fn insert_dirty(set: &mut DirtyKeys, key: StorageKey) -> bool { if let Some(dirty_keys) = set.last_mut() { dirty_keys.insert(key) From d789aabdf4adde99195755351ca39bb9b079dcb0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Tue, 9 Jun 2020 12:06:03 +0200 Subject: [PATCH 14/38] Improve doc on start_transaction --- primitives/state-machine/src/overlayed_changes/changeset.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/primitives/state-machine/src/overlayed_changes/changeset.rs b/primitives/state-machine/src/overlayed_changes/changeset.rs index 7b4bf52bf0f55..d2e8020d7f187 100644 --- a/primitives/state-machine/src/overlayed_changes/changeset.rs +++ b/primitives/state-machine/src/overlayed_changes/changeset.rs @@ -237,9 +237,9 @@ impl OverlayedChangeSet { /// Start a new nested transaction. /// - /// This allows to either commit or roll back all changes that where made while this - /// transaction was open. Any transaction must be closed by one of the aforementioned - /// functions before this overlay can be converted into storage changes. + /// This allows to either commit or roll back all changes that were made while this + /// transaction was open. Any transaction must be closed by either `commit_transaction` + /// or `rollback_transaction` before this overlay can be converted into storage changes. /// /// Changes made without any open transaction are committed immediatly. pub fn start_transaction(&mut self) { From 5e65240aed440432a5ae1ab1e6f0554d8652c5ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Tue, 9 Jun 2020 12:09:03 +0200 Subject: [PATCH 15/38] Rename value to overlayed in close_transaction --- .../src/overlayed_changes/changeset.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/primitives/state-machine/src/overlayed_changes/changeset.rs b/primitives/state-machine/src/overlayed_changes/changeset.rs index d2e8020d7f187..860e381f0ec12 100644 --- a/primitives/state-machine/src/overlayed_changes/changeset.rs +++ b/primitives/state-machine/src/overlayed_changes/changeset.rs @@ -268,14 +268,14 @@ impl OverlayedChangeSet { fn close_transaction(&mut self, rollback: bool) { for key in self.dirty_keys.pop().expect(PROOF_DIRTY_KEYS) { - let value = self.changes.get_mut(&key).expect(PROOF_DIRTY_OVERLAY_VALUE); + let overlayed = self.changes.get_mut(&key).expect(PROOF_DIRTY_OVERLAY_VALUE); if rollback { - value.pop_transaction(); + overlayed.pop_transaction(); // We need to remove the key as an `OverlayValue` with no transactions // violates its invariant of always having at least one transaction. - if value.transactions.is_empty() { + if overlayed.transactions.is_empty() { self.changes.remove(&key); } } else { @@ -286,15 +286,15 @@ impl OverlayedChangeSet { // Last tx: Is there already a value in the committed set? // Check against one rather than empty because the current tx is still // in the list as it is popped later in this function. - value.transactions.len() == 1 + overlayed.transactions.len() == 1 }; // We only need to merge if in the previous tx (or committed set) there is // already an existing value. if has_predecessor { - let dropped_tx = value.pop_transaction(); - *value.value_mut() = dropped_tx.value; - value.transaction_extrinsics_mut().extend(dropped_tx.extrinsics); + let dropped_tx = overlayed.pop_transaction(); + *overlayed.value_mut() = dropped_tx.value; + overlayed.transaction_extrinsics_mut().extend(dropped_tx.extrinsics); } } } From 7eb90659ce89d0e731badee0f7d22e6f4a5b6deb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Tue, 9 Jun 2020 12:11:03 +0200 Subject: [PATCH 16/38] Inline negation --- primitives/state-machine/src/overlayed_changes/changeset.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/primitives/state-machine/src/overlayed_changes/changeset.rs b/primitives/state-machine/src/overlayed_changes/changeset.rs index 860e381f0ec12..41d1675770135 100644 --- a/primitives/state-machine/src/overlayed_changes/changeset.rs +++ b/primitives/state-machine/src/overlayed_changes/changeset.rs @@ -279,14 +279,14 @@ impl OverlayedChangeSet { self.changes.remove(&key); } } else { - let has_predecessor = ! if let Some(dirty_keys) = self.dirty_keys.last_mut() { + let has_predecessor = if let Some(dirty_keys) = self.dirty_keys.last_mut() { // Not the last tx: Did the previous tx write to this key? - dirty_keys.insert(key) + !dirty_keys.insert(key) } else { // Last tx: Is there already a value in the committed set? // Check against one rather than empty because the current tx is still // in the list as it is popped later in this function. - overlayed.transactions.len() == 1 + overlayed.transactions.len() != 1 }; // We only need to merge if in the previous tx (or committed set) there is From 5dd6a8574ce7f49b556122bb07ac70ae3d068b2c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Tue, 9 Jun 2020 12:13:14 +0200 Subject: [PATCH 17/38] Improve wording of close_transaction comments --- primitives/state-machine/src/overlayed_changes/changeset.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/primitives/state-machine/src/overlayed_changes/changeset.rs b/primitives/state-machine/src/overlayed_changes/changeset.rs index 41d1675770135..65ab697c78d80 100644 --- a/primitives/state-machine/src/overlayed_changes/changeset.rs +++ b/primitives/state-machine/src/overlayed_changes/changeset.rs @@ -289,8 +289,8 @@ impl OverlayedChangeSet { overlayed.transactions.len() != 1 }; - // We only need to merge if in the previous tx (or committed set) there is - // already an existing value. + // We only need to merge if there is an pre-existing value. It may be avalue from + // the previous transaction or a value committed without any open transaction. if has_predecessor { let dropped_tx = overlayed.pop_transaction(); *overlayed.value_mut() = dropped_tx.value; From 1c8e7c1a8a66116c81642cc6050bef54de507b16 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Tue, 9 Jun 2020 12:16:35 +0200 Subject: [PATCH 18/38] Get rid of an expect by using get_or_insert_with --- primitives/state-machine/src/overlayed_changes.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/primitives/state-machine/src/overlayed_changes.rs b/primitives/state-machine/src/overlayed_changes.rs index 62760aa7fdcb1..4027700513cc4 100644 --- a/primitives/state-machine/src/overlayed_changes.rs +++ b/primitives/state-machine/src/overlayed_changes.rs @@ -194,11 +194,7 @@ impl OverlayedChanges { let value = self.top.modify(key.to_owned(), init, self.extrinsic_index()); // if the value was deleted initialise it back with an empty vec - if value.is_none() { - *value = Some(Default::default()); - } - - value.as_mut().expect("Initialized above; qed") + value.get_or_insert_with(StorageValue::default) } /// Returns a double-Option: None if the key is unknown (i.e. and the query should be referred From 232b68feebb584ea7c60e551dad750fc88a8c024 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Tue, 9 Jun 2020 12:17:28 +0200 Subject: [PATCH 19/38] Remove trailing whitespace --- primitives/state-machine/src/overlayed_changes.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/primitives/state-machine/src/overlayed_changes.rs b/primitives/state-machine/src/overlayed_changes.rs index 4027700513cc4..ae63bd6d7d70a 100644 --- a/primitives/state-machine/src/overlayed_changes.rs +++ b/primitives/state-machine/src/overlayed_changes.rs @@ -58,7 +58,7 @@ pub struct OverlayedChanges { /// Top level storage changes. top: OverlayedChangeSet, /// Child storage changes. The map key is the child storage key without the common prefix. - children: HashMap, + children: HashMap, /// True if extrinsics stats must be collected. collect_extrinsics: bool, /// Collect statistic on this execution. From 00b9ff359f6bf08e55b165610110261e6a7ceef7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Tue, 9 Jun 2020 12:20:32 +0200 Subject: [PATCH 20/38] Rename should to expected in tests --- .../state-machine/src/overlayed_changes.rs | 4 ++-- .../src/overlayed_changes/changeset.rs | 16 ++++++++-------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/primitives/state-machine/src/overlayed_changes.rs b/primitives/state-machine/src/overlayed_changes.rs index ae63bd6d7d70a..d5a802016659b 100644 --- a/primitives/state-machine/src/overlayed_changes.rs +++ b/primitives/state-machine/src/overlayed_changes.rs @@ -545,11 +545,11 @@ mod tests { fn assert_extrinsics( overlay: &OverlayedChangeSet, key: impl AsRef<[u8]>, - should: Vec + expected: Vec, ) { assert_eq!( overlay.get(key.as_ref()).unwrap().extrinsics().cloned().collect::>(), - should + expected ) } diff --git a/primitives/state-machine/src/overlayed_changes/changeset.rs b/primitives/state-machine/src/overlayed_changes/changeset.rs index 65ab697c78d80..cb167cda6cb65 100644 --- a/primitives/state-machine/src/overlayed_changes/changeset.rs +++ b/primitives/state-machine/src/overlayed_changes/changeset.rs @@ -309,27 +309,27 @@ mod test { type Changes<'a> = Vec<(&'a [u8], (Option<&'a [u8]>, Vec))>; type Drained<'a> = Vec<(&'a [u8], Option<&'a [u8]>)>; - fn assert_changes(is: &OverlayedChangeSet, should: &Changes) { + fn assert_changes(is: &OverlayedChangeSet, expected: &Changes) { let is: Changes = is.changes().map(|(k, v)| { (k.as_ref(), (v.value().map(AsRef::as_ref), v.extrinsics().cloned().collect())) }).collect(); - assert_eq!(&is, should); + assert_eq!(&is, expected); } - fn assert_drained_changes(is: OverlayedChangeSet, should: Changes) { + fn assert_drained_changes(is: OverlayedChangeSet, expected: Changes) { let is = is.drain_commited().collect::>(); - let should = should + let expected = expected .iter() .map(|(k, v)| (k.to_vec(), v.0.map(From::from))).collect::>(); - assert_eq!(is, should); + assert_eq!(is, expected); } - fn assert_drained(is: OverlayedChangeSet, should: Drained) { + fn assert_drained(is: OverlayedChangeSet, expected: Drained) { let is = is.drain_commited().collect::>(); - let should = should + let expected = expected .iter() .map(|(k, v)| (k.to_vec(), v.map(From::from))).collect::>(); - assert_eq!(is, should); + assert_eq!(is, expected); } #[test] From 85617a51ed68c3dbbd9680818685f80941ae690b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Tue, 9 Jun 2020 12:31:59 +0200 Subject: [PATCH 21/38] Rolling back a transaction must mark the overlay as dirty --- primitives/state-machine/src/ext.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/primitives/state-machine/src/ext.rs b/primitives/state-machine/src/ext.rs index 05f82ad5bd84a..2102cba2449da 100644 --- a/primitives/state-machine/src/ext.rs +++ b/primitives/state-machine/src/ext.rs @@ -551,6 +551,7 @@ where } fn storage_rollback_transaction(&mut self) { + self.mark_dirty(); self.overlay.rollback_transaction(); } @@ -568,7 +569,7 @@ where Default::default(), self.storage_transaction_cache, ).expect(EXT_NOT_ALLOWED_TO_FAIL); - self.storage_transaction_cache.reset(); + self.mark_dirty(); self.backend.wipe().expect(EXT_NOT_ALLOWED_TO_FAIL) } From 4e039a1e128dccb372f78f046b7fef8b22de66cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Tue, 9 Jun 2020 14:07:10 +0200 Subject: [PATCH 22/38] Protect client initiated storage tx from being droped by runtime --- .../api/proc-macro/src/impl_runtime_apis.rs | 9 +- primitives/externalities/src/lib.rs | 16 +- primitives/io/src/lib.rs | 6 +- primitives/runtime-interface/test/src/lib.rs | 2 +- primitives/state-machine/src/basic.rs | 4 +- .../state-machine/src/changes_trie/build.rs | 2 +- primitives/state-machine/src/ext.rs | 34 ++- primitives/state-machine/src/lib.rs | 19 +- .../state-machine/src/overlayed_changes.rs | 69 ++++-- .../src/overlayed_changes/changeset.rs | 196 +++++++++++++----- primitives/state-machine/src/read_only.rs | 4 +- 11 files changed, 253 insertions(+), 108 deletions(-) diff --git a/primitives/api/proc-macro/src/impl_runtime_apis.rs b/primitives/api/proc-macro/src/impl_runtime_apis.rs index d1d2ca3b57f85..77329c67a1342 100644 --- a/primitives/api/proc-macro/src/impl_runtime_apis.rs +++ b/primitives/api/proc-macro/src/impl_runtime_apis.rs @@ -385,11 +385,16 @@ fn generate_runtime_api_base_structures() -> Result { } fn commit_on_ok(&self, res: &std::result::Result) { + let proof = "\ + We only close a transaction when we opened one ourself. + Other parts of the runtime that make use of transactions (state-machine) + also balance their transactions. The runtime cannot close client initiated + transactions. qed"; if *self.commit_on_success.borrow() { if res.is_err() { - self.changes.borrow_mut().rollback_transaction(); + self.changes.borrow_mut().rollback_transaction().expect(proof); } else { - self.changes.borrow_mut().commit_transaction(); + self.changes.borrow_mut().commit_transaction().expect(proof); } } } diff --git a/primitives/externalities/src/lib.rs b/primitives/externalities/src/lib.rs index 983f7bada7b8a..56fa8020b5f6e 100644 --- a/primitives/externalities/src/lib.rs +++ b/primitives/externalities/src/lib.rs @@ -206,19 +206,15 @@ pub trait Externalities: ExtensionStore { /// Rollback the last transaction started by `start_transaction`. /// - /// Any changes made during that transaction are discarded. - /// - /// Panics: - /// Will panic if there is no open transaction. - fn storage_rollback_transaction(&mut self); + /// Any changes made during that transaction are discarded. Returns an error when + /// no transaction is open that can be closed. + fn storage_rollback_transaction(&mut self) -> Result<(), ()>; /// Commit the last transaction started by `start_transaction`. /// - /// Any changes made during that transaction are committed. - /// - /// Panics: - /// Will panic if there is no open transaction. - fn storage_commit_transaction(&mut self); + /// Any changes made during that transaction are committed. Returns an error when + /// no transaction is open that can be closed. + fn storage_commit_transaction(&mut self) -> Result<(), ()>; /// !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! /// Benchmarking related functionality and shouldn't be used anywhere else! diff --git a/primitives/io/src/lib.rs b/primitives/io/src/lib.rs index 8b2c0dccbb2fd..4f86ce61a56d6 100644 --- a/primitives/io/src/lib.rs +++ b/primitives/io/src/lib.rs @@ -179,7 +179,8 @@ pub trait Storage { /// /// Will panic if there is no open transaction. fn rollback_transaction(&mut self) { - self.storage_rollback_transaction(); + self.storage_rollback_transaction() + .expect("No open transaction that can be rolled back."); } /// Commit the last transaction started by `start_transaction`. @@ -190,7 +191,8 @@ pub trait Storage { /// /// Will panic if there is no open transaction. fn commit_transaction(&mut self) { - self.storage_commit_transaction(); + self.storage_commit_transaction() + .expect("No open transaction that can be committed."); } } diff --git a/primitives/runtime-interface/test/src/lib.rs b/primitives/runtime-interface/test/src/lib.rs index 06bc4e8ed8d46..5600d3d2283fb 100644 --- a/primitives/runtime-interface/test/src/lib.rs +++ b/primitives/runtime-interface/test/src/lib.rs @@ -55,7 +55,7 @@ fn call_wasm_method_with_result( &mut ext_ext, sp_core::traits::MissingHostFunctions::Disallow, ).map_err(|e| format!("Failed to execute `{}`: {}", method, e))?; - + drop(ext_ext); Ok(ext) } diff --git a/primitives/state-machine/src/basic.rs b/primitives/state-machine/src/basic.rs index 0366befe70cfc..dbb4c6c2b82f2 100644 --- a/primitives/state-machine/src/basic.rs +++ b/primitives/state-machine/src/basic.rs @@ -311,11 +311,11 @@ impl Externalities for BasicExternalities { unimplemented!("Transactions are not supported by BasicExternalities"); } - fn storage_rollback_transaction(&mut self) { + fn storage_rollback_transaction(&mut self) -> Result<(), ()> { unimplemented!("Transactions are not supported by BasicExternalities"); } - fn storage_commit_transaction(&mut self) { + fn storage_commit_transaction(&mut self) -> Result<(), ()> { unimplemented!("Transactions are not supported by BasicExternalities"); } diff --git a/primitives/state-machine/src/changes_trie/build.rs b/primitives/state-machine/src/changes_trie/build.rs index 9733c1df17c87..bf910e2c4f7fb 100644 --- a/primitives/state-machine/src/changes_trie/build.rs +++ b/primitives/state-machine/src/changes_trie/build.rs @@ -410,7 +410,7 @@ mod test { changes.set_storage(vec![100], Some(vec![202])); changes.set_child_storage(&child_info_1, vec![100], Some(vec![202])); - changes.commit_transaction(); + changes.commit_transaction().unwrap(); changes.set_extrinsic_index(0); changes.set_storage(vec![100], Some(vec![0])); diff --git a/primitives/state-machine/src/ext.rs b/primitives/state-machine/src/ext.rs index 2102cba2449da..146fde3cc15ad 100644 --- a/primitives/state-machine/src/ext.rs +++ b/primitives/state-machine/src/ext.rs @@ -37,6 +37,10 @@ use std::{error, fmt, any::{Any, TypeId}}; use log::{warn, trace}; const EXT_NOT_ALLOWED_TO_FAIL: &str = "Externalities not allowed to fail within runtime"; +const BENCHMARKING_FN: &str = "\ + This is a special fn only for benchmarking where a database commit happens from the runtime. + For that reason client started transactions before calling into runtime are not allowed. + Without client transactions the loop condition garantuees the success of the tx close."; /// Errors that can occur when interacting with the externalities. #[derive(Debug, Copy, Clone)] @@ -109,6 +113,7 @@ where changes_trie_state: Option>, extensions: Option<&'a mut Extensions>, ) -> Self { + overlay.enter_runtime(); Self { overlay, offchain_overlay, @@ -155,6 +160,17 @@ where } } +impl<'a, H, B, N> Drop for Ext<'a, H, N, B> +where + H: Hasher, + B: 'a + Backend, + N: crate::changes_trie::BlockNumber +{ + fn drop(&mut self) { + self.overlay.exit_runtime(); + } +} + impl<'a, H, B, N> Externalities for Ext<'a, H, N, B> where H: Hasher, @@ -547,21 +563,21 @@ where } fn storage_start_transaction(&mut self) { - self.overlay.start_transaction(); + self.overlay.start_transaction() } - fn storage_rollback_transaction(&mut self) { + fn storage_rollback_transaction(&mut self) -> Result<(), ()> { self.mark_dirty(); - self.overlay.rollback_transaction(); + self.overlay.rollback_transaction().map_err(|_| ()) } - fn storage_commit_transaction(&mut self) { - self.overlay.commit_transaction(); + fn storage_commit_transaction(&mut self) -> Result<(), ()> { + self.overlay.commit_transaction().map_err(|_| ()) } fn wipe(&mut self) { for _ in 0..self.overlay.transaction_depth() { - self.overlay.rollback_transaction(); + self.overlay.rollback_transaction().expect(BENCHMARKING_FN); } self.overlay.drain_storage_changes( &self.backend, @@ -569,13 +585,13 @@ where Default::default(), self.storage_transaction_cache, ).expect(EXT_NOT_ALLOWED_TO_FAIL); + self.backend.wipe().expect(EXT_NOT_ALLOWED_TO_FAIL); self.mark_dirty(); - self.backend.wipe().expect(EXT_NOT_ALLOWED_TO_FAIL) } fn commit(&mut self) { for _ in 0..self.overlay.transaction_depth() { - self.overlay.commit_transaction(); + self.overlay.commit_transaction().expect(BENCHMARKING_FN); } let changes = self.overlay.drain_storage_changes( &self.backend, @@ -587,7 +603,7 @@ where changes.transaction_storage_root, changes.transaction, ).expect(EXT_NOT_ALLOWED_TO_FAIL); - self.storage_transaction_cache.reset(); + self.mark_dirty(); } } diff --git a/primitives/state-machine/src/lib.rs b/primitives/state-machine/src/lib.rs index 4b52f96c364e8..facfa7bd35ae4 100644 --- a/primitives/state-machine/src/lib.rs +++ b/primitives/state-machine/src/lib.rs @@ -79,6 +79,10 @@ pub use in_memory_backend::new_in_mem; pub use stats::{UsageInfo, UsageUnit, StateMachineStats}; pub use sp_core::traits::CloneableSpawn; +const PROOF_CLOSE_TRANSACTION: &str = "\ + Closing a transaction that was started in this function. Client initiated transactions + are protected from being closed by the runtime. qed"; + type CallResult = Result, E>; /// Default handler of the execution manager. @@ -351,7 +355,7 @@ impl<'a, B, H, N, Exec> StateMachine<'a, B, H, N, Exec> where let (result, was_native) = self.execute_aux(true, native_call.take()); if was_native { - self.overlay.rollback_transaction(); + self.overlay.rollback_transaction().expect(PROOF_CLOSE_TRANSACTION); let (wasm_result, _) = self.execute_aux( false, native_call, @@ -366,7 +370,7 @@ impl<'a, B, H, N, Exec> StateMachine<'a, B, H, N, Exec> where on_consensus_failure(wasm_result, result) } } else { - self.overlay.commit_transaction(); + self.overlay.commit_transaction().expect(PROOF_CLOSE_TRANSACTION); result } } @@ -386,10 +390,10 @@ impl<'a, B, H, N, Exec> StateMachine<'a, B, H, N, Exec> where ); if !was_native || result.is_ok() { - self.overlay.commit_transaction(); + self.overlay.commit_transaction().expect(PROOF_CLOSE_TRANSACTION); result } else { - self.overlay.rollback_transaction(); + self.overlay.rollback_transaction().expect(PROOF_CLOSE_TRANSACTION); let (wasm_result, _) = self.execute_aux( false, native_call, @@ -996,7 +1000,7 @@ mod tests { ); ext.clear_prefix(b"ab"); } - overlay.commit_transaction(); + overlay.commit_transaction().unwrap(); assert_eq!( overlay.changes().map(|(k, v)| (k.clone(), v.value().cloned())) @@ -1104,7 +1108,7 @@ mod tests { Some(reference_data.encode()), ); } - overlay.rollback_transaction(); + overlay.rollback_transaction().unwrap(); { let ext = Ext::new( &mut overlay, @@ -1172,7 +1176,7 @@ mod tests { Some(vec![Item::InitializationItem, Item::DiscardedItem].encode()), ); } - overlay.rollback_transaction(); + overlay.rollback_transaction().unwrap(); // Then we apply next transaction which is valid this time. { @@ -1296,6 +1300,7 @@ mod tests { ext.set_child_storage(&child_info_1, b"abc".to_vec(), b"def".to_vec()); ext.set_child_storage(&child_info_2, b"abc".to_vec(), b"def".to_vec()); ext.storage_root(); + drop(ext); cache.transaction.unwrap() }; let mut duplicate = false; diff --git a/primitives/state-machine/src/overlayed_changes.rs b/primitives/state-machine/src/overlayed_changes.rs index d5a802016659b..645c300a7bd10 100644 --- a/primitives/state-machine/src/overlayed_changes.rs +++ b/primitives/state-machine/src/overlayed_changes.rs @@ -38,6 +38,9 @@ use hash_db::Hasher; /// Re-export of `changeset::OverlayedValue`. pub use self::changeset::OverlayedValue; +/// Re-export of `changeset::NoOpenTransaction`. +pub use self::changeset::NoOpenTransaction; + /// Storage key. pub type StorageKey = Vec; @@ -237,11 +240,13 @@ impl OverlayedChanges { self.stats.tally_write_overlay(size_write); let storage_key = child_info.storage_key().to_vec(); let tx_depth = self.transaction_depth(); + let num_client_tx = self.top.num_client_transactions(); + let exec_mode = self.top.execution_mode(); let (overlay, info) = self.children.entry(storage_key) .or_insert_with(|| ( // This changeset might be created when there are already open transactions. // We need to catch up here so it is at the same transaction depth. - OverlayedChangeSet::with_depth(tx_depth), + OverlayedChangeSet::new(tx_depth, num_client_tx, exec_mode), child_info.to_owned() )); let updatable = info.try_update(child_info); @@ -301,8 +306,8 @@ impl OverlayedChanges { /// Start a new nested transaction. /// /// This allows to either commit or roll back all changes that where made while this - /// transaction was open. Any transaction must be closed by one of the aforementioned - /// functions before this overlay can be converted into storage changes. + /// transaction was open. Any transaction must be closed by either `rollback_transaction` or + /// `commit_transaction` before this overlay can be converted into storage changes. /// /// Changes made without any open transaction are committed immediatly. pub fn start_transaction(&mut self) { @@ -314,28 +319,48 @@ impl OverlayedChanges { /// Rollback the last transaction started by `start_transaction`. /// - /// Any changes made during that transaction are discarded. - /// - /// Panics: - /// Will panic if there is no open transaction. - pub fn rollback_transaction(&mut self) { - self.top.rollback_transaction(); + /// Any changes made during that transaction are discarded. Returns an error if + /// there is no open transaction that can be rolled back. + pub fn rollback_transaction(&mut self) -> Result<(), NoOpenTransaction> { + self.top.rollback_transaction()?; self.children.retain(|_, (changeset, _)| { - changeset.rollback_transaction(); + changeset.rollback_transaction() + .expect("Top and children changesets are started in lockstep; qed"); !changeset.is_empty() }); + Ok(()) } /// Commit the last transaction started by `start_transaction`. /// - /// Any changes made during that transaction are committed. + /// Any changes made during that transaction are committed. Returns an error if there + /// is no open transaction that can be committed. + pub fn commit_transaction(&mut self) -> Result<(), NoOpenTransaction> { + self.top.commit_transaction()?; + for (_, (changeset, _)) in self.children.iter_mut() { + changeset.commit_transaction() + .expect("Top and children changesets are started in lockstep; qed"); + } + Ok(()) + } + + /// Call this before transfering control to the runtime. /// - /// Panics: - /// Will panic if there is no open transaction. - pub fn commit_transaction(&mut self) { - self.top.commit_transaction(); + /// This protects all existing transactions from being removed by the runtime. + pub fn enter_runtime(&mut self) { + self.top.enter_runtime(); + for (_, (changeset, _)) in self.children.iter_mut() { + changeset.enter_runtime(); + } + } + + /// Call this when control returns from the runtime. + /// + /// This commits all dangeling transaction left open by the runtime. + pub fn exit_runtime(&mut self) { + self.top.exit_runtime(); for (_, (changeset, _)) in self.children.iter_mut() { - changeset.commit_transaction(); + changeset.exit_runtime(); } } @@ -566,7 +591,7 @@ mod tests { overlayed.set_storage(key.clone(), Some(vec![1, 2, 3])); assert_eq!(overlayed.storage(&key).unwrap(), Some(&[1, 2, 3][..])); - overlayed.commit_transaction(); + overlayed.commit_transaction().unwrap(); assert_eq!(overlayed.storage(&key).unwrap(), Some(&[1, 2, 3][..])); @@ -578,7 +603,7 @@ mod tests { overlayed.set_storage(key.clone(), None); assert!(overlayed.storage(&key).unwrap().is_none()); - overlayed.rollback_transaction(); + overlayed.rollback_transaction().unwrap(); assert_eq!(overlayed.storage(&key).unwrap(), Some(&[1, 2, 3][..])); @@ -602,7 +627,7 @@ mod tests { overlay.set_storage(b"dog".to_vec(), Some(b"puppy".to_vec())); overlay.set_storage(b"dogglesworth".to_vec(), Some(b"catYYY".to_vec())); overlay.set_storage(b"doug".to_vec(), Some(vec![])); - overlay.commit_transaction(); + overlay.commit_transaction().unwrap(); overlay.start_transaction(); overlay.set_storage(b"dogglesworth".to_vec(), Some(b"cat".to_vec())); @@ -657,7 +682,7 @@ mod tests { assert_extrinsics(&overlay.top, vec![3], vec![1, 3]); assert_extrinsics(&overlay.top, vec![100], vec![NO_EXTRINSIC_INDEX]); - overlay.rollback_transaction(); + overlay.rollback_transaction().unwrap(); assert_extrinsics(&overlay.top, vec![1], vec![0, 2]); assert_extrinsics(&overlay.top, vec![3], vec![1]); @@ -671,7 +696,7 @@ mod tests { overlay.set_storage(vec![20], Some(vec![20])); overlay.set_storage(vec![30], Some(vec![30])); overlay.set_storage(vec![40], Some(vec![40])); - overlay.commit_transaction(); + overlay.commit_transaction().unwrap(); overlay.set_storage(vec![10], Some(vec![10])); overlay.set_storage(vec![30], None); @@ -712,7 +737,7 @@ mod tests { overlay.set_child_storage(child_info, vec![20], Some(vec![20])); overlay.set_child_storage(child_info, vec![30], Some(vec![30])); overlay.set_child_storage(child_info, vec![40], Some(vec![40])); - overlay.commit_transaction(); + overlay.commit_transaction().unwrap(); overlay.set_child_storage(child_info, vec![10], Some(vec![10])); overlay.set_child_storage(child_info, vec![30], None); diff --git a/primitives/state-machine/src/overlayed_changes/changeset.rs b/primitives/state-machine/src/overlayed_changes/changeset.rs index cb167cda6cb65..5e761c6dada4f 100644 --- a/primitives/state-machine/src/overlayed_changes/changeset.rs +++ b/primitives/state-machine/src/overlayed_changes/changeset.rs @@ -23,16 +23,6 @@ use itertools::Itertools; use std::collections::{HashSet, BTreeMap, BTreeSet}; use smallvec::SmallVec; -const PROOF_DIRTY_KEYS: &str = "\ - We assume transactions are balanced. Every start of a transaction created one dirty - keys element. This function is only called on transaction close. Therefore an element - created when starting the transaction must exist; qed"; - -const PROOF_DIRTY_OVERLAY_VALUE: &str = "\ - A write to an OverlayedValue is recorded in the dirty key set. Before an OverlayedValues - is removed its containing dirty set is removed. This function is only called for keys that - are in the dirty set. Therefore the entry must exist; qed"; - const PROOF_OVERLAY_NON_EMPTY: &str = "\ An OverlayValue is always created with at least one transaction and dropped as soon as the last transaction is removed; qed"; @@ -40,6 +30,29 @@ const PROOF_OVERLAY_NON_EMPTY: &str = "\ type DirtyKeys = SmallVec<[HashSet; 5]>; type Transactions = SmallVec<[InnerValue; 5]>; +/// Error returned when trying to commit or rollback while no transaction is open. +#[derive(Debug)] +#[cfg_attr(test, derive(PartialEq))] +pub struct NoOpenTransaction; + +/// Describes in which mode the node is currently executing. +#[derive(Debug, Clone, Copy)] +pub enum ExecutionMode { + /// Exeuting in client mode: Removal of all transactions possible. + Client, + /// Executing in runtime mode: Transactions started by the client are protected. + Runtime, +} + +/// Describes which kind of transaction close should be performed. +#[derive(Debug, Clone, Copy)] +enum CloseType { + /// Commit the transaction. + Commit, + /// Rollback the transaction. + Rollback, +} + #[derive(Debug, Default, Clone)] #[cfg_attr(test, derive(PartialEq))] struct InnerValue { @@ -68,6 +81,18 @@ pub struct OverlayedChangeSet { /// values to merge into the parent transaction on commit. The length of this vector /// therefore determines how many nested transactions are currently open (depth). dirty_keys: DirtyKeys, + /// The number of how many transactions beginning from the first transactions are started + /// by the client. Those transactions are protected against close (commit, rollback) + /// when in runtime mode. + num_client_transactions: usize, + /// Determines whether the node is using the overlay from the client or the runtime. + execution_mode: ExecutionMode, +} + +impl Default for ExecutionMode { + fn default() -> Self { + Self::Client + } } impl OverlayedValue { @@ -134,11 +159,13 @@ fn insert_dirty(set: &mut DirtyKeys, key: StorageKey) -> bool { } impl OverlayedChangeSet { - /// Create a new changeset with the specified transaction depth. - pub fn with_depth(depth: usize) -> Self { + /// Create a new changeset. + pub fn new(depth: usize, num_client_transactions: usize, mode: ExecutionMode) -> Self { use std::iter::repeat; Self { dirty_keys: repeat(HashSet::new()).take(depth).collect(), + num_client_transactions, + execution_mode: mode, .. Default::default() } } @@ -235,6 +262,36 @@ impl OverlayedChangeSet { self.dirty_keys.len() } + /// Returns how many of the open transactions are opened by the client. + /// + /// These are alwyays the first transaction to open and the last to close. + pub fn num_client_transactions(&self) -> usize { + self.num_client_transactions + } + + /// Returns whether we are executing in client or runtime mode. + pub fn execution_mode(&self) -> ExecutionMode { + self.execution_mode + } + + /// Call this before entering runtime. + pub fn enter_runtime(&mut self) { + if let ExecutionMode::Runtime = self.execution_mode { + return; + } + self.execution_mode = ExecutionMode::Runtime; + self.num_client_transactions = self.transaction_depth(); + } + + /// Call this when returning from runtime. + pub fn exit_runtime(&mut self) { + self.execution_mode = ExecutionMode::Client; + while self.transaction_depth() > self.num_client_transactions { + self.commit_transaction() + .expect("The loop condition checks that the transaction depth is > 0; qed"); + } + } + /// Start a new nested transaction. /// /// This allows to either commit or roll back all changes that were made while this @@ -248,29 +305,36 @@ impl OverlayedChangeSet { /// Rollback the last transaction started by `start_transaction`. /// - /// Any changes made during that transaction are discarded. - /// - /// Panics: - /// Will panic if there is no open transaction. - pub fn rollback_transaction(&mut self) { - self.close_transaction(true); + /// Any changes made during that transaction are discarded. Returns an error if + /// there is no open transaction that can be rolled back. + pub fn rollback_transaction(&mut self) -> Result<(), NoOpenTransaction> { + self.close_transaction(CloseType::Rollback) } /// Commit the last transaction started by `start_transaction`. /// - /// Any changes made during that transaction are committed. - /// - /// Panics: - /// Will panic if there is no open transaction. - pub fn commit_transaction(&mut self) { - self.close_transaction(false); + /// Any changes made during that transaction are committed. Returns an error if + /// there is no open transaction that can be committed. + pub fn commit_transaction(&mut self) -> Result<(), NoOpenTransaction> { + self.close_transaction(CloseType::Commit) } - fn close_transaction(&mut self, rollback: bool) { - for key in self.dirty_keys.pop().expect(PROOF_DIRTY_KEYS) { - let overlayed = self.changes.get_mut(&key).expect(PROOF_DIRTY_OVERLAY_VALUE); + fn close_transaction(&mut self, close_type: CloseType) -> Result<(), NoOpenTransaction> { + // runtime is not allowed to close transactions started by the client + if let ExecutionMode::Runtime = self.execution_mode { + if self.num_client_transactions == self.transaction_depth() { + return Err(NoOpenTransaction) + } + } + + for key in self.dirty_keys.pop().ok_or(NoOpenTransaction)? { + let overlayed = self.changes.get_mut(&key).expect("\ + A write to an OverlayedValue is recorded in the dirty key set. Before an + OverlayedValue is removed, its containing dirty set is removed. This + function is only called for keys that are in the dirty set. qed\ + "); - if rollback { + if let CloseType::Rollback = close_type { overlayed.pop_transaction(); // We need to remove the key as an `OverlayValue` with no transactions @@ -298,6 +362,8 @@ impl OverlayedChangeSet { } } } + + Ok(()) } } @@ -389,14 +455,14 @@ mod test { assert_eq!(changeset.transaction_depth(), 3); changeset.start_transaction(); assert_eq!(changeset.transaction_depth(), 4); - changeset.rollback_transaction(); + changeset.rollback_transaction().unwrap(); assert_eq!(changeset.transaction_depth(), 3); - changeset.commit_transaction(); + changeset.commit_transaction().unwrap(); assert_eq!(changeset.transaction_depth(), 2); assert_changes(&changeset, &all_changes); // roll back our first transactions that actually contains something - changeset.rollback_transaction(); + changeset.rollback_transaction().unwrap(); assert_eq!(changeset.transaction_depth(), 1); let rolled_back: Changes = vec![ @@ -407,7 +473,7 @@ mod test { ]; assert_changes(&changeset, &rolled_back); - changeset.commit_transaction(); + changeset.commit_transaction().unwrap(); assert_eq!(changeset.transaction_depth(), 0); assert_changes(&changeset, &rolled_back); @@ -452,18 +518,18 @@ mod test { assert_eq!(changeset.transaction_depth(), 3); changeset.start_transaction(); assert_eq!(changeset.transaction_depth(), 4); - changeset.rollback_transaction(); + changeset.rollback_transaction().unwrap(); assert_eq!(changeset.transaction_depth(), 3); - changeset.commit_transaction(); + changeset.commit_transaction().unwrap(); assert_eq!(changeset.transaction_depth(), 2); assert_changes(&changeset, &all_changes); - changeset.commit_transaction(); + changeset.commit_transaction().unwrap(); assert_eq!(changeset.transaction_depth(), 1); assert_changes(&changeset, &all_changes); - changeset.rollback_transaction(); + changeset.rollback_transaction().unwrap(); assert_eq!(changeset.transaction_depth(), 0); let rolled_back: Changes = vec![ @@ -515,11 +581,11 @@ mod test { (b"key3", (Some(b"valinit-modified"), vec![3])), ]; assert_changes(&changeset, &all_changes); - changeset.commit_transaction(); + changeset.commit_transaction().unwrap(); assert_eq!(changeset.transaction_depth(), 1); assert_changes(&changeset, &all_changes); - changeset.rollback_transaction(); + changeset.rollback_transaction().unwrap(); assert_eq!(changeset.transaction_depth(), 0); let rolled_back: Changes = vec![ (b"key0", (Some(b"val0"), vec![0])), @@ -550,7 +616,7 @@ mod test { (b"key1", (Some(b"val1"), vec![2])), ]); - changeset.rollback_transaction(); + changeset.rollback_transaction().unwrap(); assert_changes(&changeset, &vec![ (b"del1", (Some(b"delval1"), vec![3])), @@ -586,7 +652,7 @@ mod test { assert_eq!(changeset.next_change(b"key3").unwrap().1.value(), Some(&b"val4".to_vec())); assert_eq!(changeset.next_change(b"key4"), None); - changeset.rollback_transaction(); + changeset.rollback_transaction().unwrap(); assert_eq!(changeset.next_change(b"key0").unwrap().0, b"key1"); assert_eq!(changeset.next_change(b"key0").unwrap().1.value(), Some(&b"val1".to_vec())); @@ -601,28 +667,25 @@ mod test { } #[test] - #[should_panic] - fn no_open_tx_commit_panics() { + fn no_open_tx_commit_errors() { let mut changeset = OverlayedChangeSet::default(); assert_eq!(changeset.transaction_depth(), 0); - changeset.commit_transaction(); + assert_eq!(changeset.commit_transaction(), Err(NoOpenTransaction)); } #[test] - #[should_panic] - fn no_open_tx_rollback_panics() { + fn no_open_tx_rollback_errors() { let mut changeset = OverlayedChangeSet::default(); assert_eq!(changeset.transaction_depth(), 0); - changeset.rollback_transaction(); + assert_eq!(changeset.rollback_transaction(), Err(NoOpenTransaction)); } #[test] - #[should_panic] - fn unbalanced_transactions_panics() { + fn unbalanced_transactions_errors() { let mut changeset = OverlayedChangeSet::default(); changeset.start_transaction(); - changeset.commit_transaction(); - changeset.commit_transaction(); + changeset.commit_transaction().unwrap(); + assert_eq!(changeset.commit_transaction(), Err(NoOpenTransaction)); } #[test] @@ -632,4 +695,37 @@ mod test { changeset.start_transaction(); let _ = changeset.drain_commited(); } + + #[test] + fn runtime_cannot_close_client_tx() { + let mut changeset = OverlayedChangeSet::default(); + changeset.start_transaction(); + changeset.enter_runtime(); + changeset.start_transaction(); + changeset.commit_transaction().unwrap(); + assert_eq!(changeset.commit_transaction(), Err(NoOpenTransaction)); + assert_eq!(changeset.rollback_transaction(), Err(NoOpenTransaction)); + } + + #[test] + fn exit_runtime_closes_runtime_tx() { + let mut changeset = OverlayedChangeSet::default(); + + changeset.start_transaction(); + + changeset.set(b"key0".to_vec(), Some(b"val0".to_vec()), Some(1)); + + changeset.enter_runtime(); + changeset.start_transaction(); + changeset.set(b"key1".to_vec(), Some(b"val1".to_vec()), Some(2)); + changeset.exit_runtime(); + + changeset.commit_transaction().unwrap(); + assert_eq!(changeset.transaction_depth(), 0); + + assert_drained(changeset, vec![ + (b"key0", Some(b"val0")), + (b"key1", Some(b"val1")), + ]); + } } diff --git a/primitives/state-machine/src/read_only.rs b/primitives/state-machine/src/read_only.rs index 2c31cc672c3e8..2a5d7fda364de 100644 --- a/primitives/state-machine/src/read_only.rs +++ b/primitives/state-machine/src/read_only.rs @@ -174,11 +174,11 @@ impl<'a, H: Hasher, B: 'a + Backend> Externalities for ReadOnlyExternalities< unimplemented!("Transactions are not supported by ReadOnlyExternalities"); } - fn storage_rollback_transaction(&mut self) { + fn storage_rollback_transaction(&mut self) -> Result<(), ()> { unimplemented!("Transactions are not supported by ReadOnlyExternalities"); } - fn storage_commit_transaction(&mut self) { + fn storage_commit_transaction(&mut self) -> Result<(), ()> { unimplemented!("Transactions are not supported by ReadOnlyExternalities"); } From 8ee36960c361e7150a5db080a06db287b2adab14 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Wed, 10 Jun 2020 14:03:14 +0200 Subject: [PATCH 23/38] Review nits --- primitives/externalities/src/lib.rs | 14 ++++++----- primitives/io/src/lib.rs | 3 ++- .../state-machine/src/overlayed_changes.rs | 4 +++- .../src/overlayed_changes/changeset.rs | 23 ++++++++++++------- 4 files changed, 28 insertions(+), 16 deletions(-) diff --git a/primitives/externalities/src/lib.rs b/primitives/externalities/src/lib.rs index 56fa8020b5f6e..210fe5b4ef009 100644 --- a/primitives/externalities/src/lib.rs +++ b/primitives/externalities/src/lib.rs @@ -198,21 +198,23 @@ pub trait Externalities: ExtensionStore { /// Start a new nested transaction. /// /// This allows to either commit or roll back all changes made after this call to the - /// top changes or the default child changes. For every transaction there must be a + /// top changes or the default child changes. For every transaction there cam be a /// matching call to either `storage_rollback_transaction` or `storage_commit_transaction`. + /// Any transactions that are still open after returning from runtime are committed + /// automatically. /// - /// Changes made without any open transaction are committed immediatly + /// Changes made without any open transaction are committed immediately. fn storage_start_transaction(&mut self); - /// Rollback the last transaction started by `start_transaction`. + /// Rollback the last transaction started by `storage_start_transaction`. /// - /// Any changes made during that transaction are discarded. Returns an error when + /// Any changes made during that storage transaction are discarded. Returns an error when /// no transaction is open that can be closed. fn storage_rollback_transaction(&mut self) -> Result<(), ()>; - /// Commit the last transaction started by `start_transaction`. + /// Commit the last transaction started by `storage_start_transaction`. /// - /// Any changes made during that transaction are committed. Returns an error when + /// Any changes made during that storage transaction are committed. Returns an error when /// no transaction is open that can be closed. fn storage_commit_transaction(&mut self) -> Result<(), ()>; diff --git a/primitives/io/src/lib.rs b/primitives/io/src/lib.rs index 4f86ce61a56d6..232101fdb344f 100644 --- a/primitives/io/src/lib.rs +++ b/primitives/io/src/lib.rs @@ -166,7 +166,8 @@ pub trait Storage { /// # Warning /// /// This is a low level API that is potentially dangerous as it can easily result - /// in unbalanced transactions. FRAME users should use high level storage abstractions. + /// in unbalanced transactions. For example, FRAME users should use high level storage + /// abstractions. fn start_transaction(&mut self) { self.storage_start_transaction(); } diff --git a/primitives/state-machine/src/overlayed_changes.rs b/primitives/state-machine/src/overlayed_changes.rs index 645c300a7bd10..673fea5cc6dfe 100644 --- a/primitives/state-machine/src/overlayed_changes.rs +++ b/primitives/state-machine/src/overlayed_changes.rs @@ -347,6 +347,7 @@ impl OverlayedChanges { /// Call this before transfering control to the runtime. /// /// This protects all existing transactions from being removed by the runtime. + /// Calling this while already inside the runtime is a no-op. pub fn enter_runtime(&mut self) { self.top.enter_runtime(); for (_, (changeset, _)) in self.children.iter_mut() { @@ -356,7 +357,8 @@ impl OverlayedChanges { /// Call this when control returns from the runtime. /// - /// This commits all dangeling transaction left open by the runtime. + /// This commits all dangling transaction left open by the runtime. + /// Calling this while already outside the runtime is a no-op. pub fn exit_runtime(&mut self) { self.top.exit_runtime(); for (_, (changeset, _)) in self.children.iter_mut() { diff --git a/primitives/state-machine/src/overlayed_changes/changeset.rs b/primitives/state-machine/src/overlayed_changes/changeset.rs index 5e761c6dada4f..79f6405afa62b 100644 --- a/primitives/state-machine/src/overlayed_changes/changeset.rs +++ b/primitives/state-machine/src/overlayed_changes/changeset.rs @@ -27,10 +27,11 @@ const PROOF_OVERLAY_NON_EMPTY: &str = "\ An OverlayValue is always created with at least one transaction and dropped as soon as the last transaction is removed; qed"; -type DirtyKeys = SmallVec<[HashSet; 5]>; +type DirtyKeysSets = SmallVec<[HashSet; 5]>; type Transactions = SmallVec<[InnerValue; 5]>; -/// Error returned when trying to commit or rollback while no transaction is open. +/// Error returned when trying to commit or rollback while no transaction is open or +/// when the runtime is trying to close a transaction started by the client. #[derive(Debug)] #[cfg_attr(test, derive(PartialEq))] pub struct NoOpenTransaction; @@ -80,7 +81,7 @@ pub struct OverlayedChangeSet { /// Stores which keys are dirty per transaction. Needed in order to determine which /// values to merge into the parent transaction on commit. The length of this vector /// therefore determines how many nested transactions are currently open (depth). - dirty_keys: DirtyKeys, + dirty_keys: DirtyKeysSets, /// The number of how many transactions beginning from the first transactions are started /// by the client. Those transactions are protected against close (commit, rollback) /// when in runtime mode. @@ -150,7 +151,7 @@ impl OverlayedValue { /// /// Returns true iff we are currently have at least one open transaction and if this /// is the first write to the given key that transaction. -fn insert_dirty(set: &mut DirtyKeys, key: StorageKey) -> bool { +fn insert_dirty(set: &mut DirtyKeysSets, key: StorageKey) -> bool { if let Some(dirty_keys) = set.last_mut() { dirty_keys.insert(key) } else { @@ -264,7 +265,7 @@ impl OverlayedChangeSet { /// Returns how many of the open transactions are opened by the client. /// - /// These are alwyays the first transaction to open and the last to close. + /// These are always the first transaction to open and the last to close. pub fn num_client_transactions(&self) -> usize { self.num_client_transactions } @@ -274,7 +275,10 @@ impl OverlayedChangeSet { self.execution_mode } - /// Call this before entering runtime. + /// Call this before transfering control to the runtime. + /// + /// This protects all existing transactions from being removed by the runtime. + /// Calling this while already inside the runtime is a no-op. pub fn enter_runtime(&mut self) { if let ExecutionMode::Runtime = self.execution_mode { return; @@ -283,7 +287,10 @@ impl OverlayedChangeSet { self.num_client_transactions = self.transaction_depth(); } - /// Call this when returning from runtime. + /// Call this when control returns from the runtime. + /// + /// This commits all dangling transaction left open by the runtime. + /// Calling this while already outside the runtime is a no-op. pub fn exit_runtime(&mut self) { self.execution_mode = ExecutionMode::Client; while self.transaction_depth() > self.num_client_transactions { @@ -298,7 +305,7 @@ impl OverlayedChangeSet { /// transaction was open. Any transaction must be closed by either `commit_transaction` /// or `rollback_transaction` before this overlay can be converted into storage changes. /// - /// Changes made without any open transaction are committed immediatly. + /// Changes made without any open transaction are committed immediately. pub fn start_transaction(&mut self) { self.dirty_keys.push(Default::default()); } From 843c84ac878a699cf47e043543e97ab5237f0429 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Wed, 10 Jun 2020 17:17:46 +0200 Subject: [PATCH 24/38] Return Err when entering or exiting runtime fails --- primitives/state-machine/src/ext.rs | 9 +++- .../state-machine/src/overlayed_changes.rs | 22 +++++++--- .../src/overlayed_changes/changeset.rs | 42 +++++++++++++++---- 3 files changed, 57 insertions(+), 16 deletions(-) diff --git a/primitives/state-machine/src/ext.rs b/primitives/state-machine/src/ext.rs index 146fde3cc15ad..60880063b8775 100644 --- a/primitives/state-machine/src/ext.rs +++ b/primitives/state-machine/src/ext.rs @@ -113,7 +113,8 @@ where changes_trie_state: Option>, extensions: Option<&'a mut Extensions>, ) -> Self { - overlay.enter_runtime(); + overlay.enter_runtime() + .expect("Instances of this tpye must be created outside of the runtime."); Self { overlay, offchain_overlay, @@ -167,7 +168,11 @@ where N: crate::changes_trie::BlockNumber { fn drop(&mut self) { - self.overlay.exit_runtime(); + self.overlay.exit_runtime().expect("\ + This instance holds an exclusive borrow of the overlay and calls enter_runtime on + creation. We do not call exit_runtime in any other place of this type. + Drop is only called once. qed + "); } } diff --git a/primitives/state-machine/src/overlayed_changes.rs b/primitives/state-machine/src/overlayed_changes.rs index 673fea5cc6dfe..eb385842e6a43 100644 --- a/primitives/state-machine/src/overlayed_changes.rs +++ b/primitives/state-machine/src/overlayed_changes.rs @@ -41,6 +41,12 @@ pub use self::changeset::OverlayedValue; /// Re-export of `changeset::NoOpenTransaction`. pub use self::changeset::NoOpenTransaction; +/// Re-export of `changeset::AlreadyInRutnime`. +pub use self::changeset::AlreadyInRuntime; + +/// Re-export of `changeset::NotInruntime`. +pub use self::changeset::NotInRuntime; + /// Storage key. pub type StorageKey = Vec; @@ -348,22 +354,26 @@ impl OverlayedChanges { /// /// This protects all existing transactions from being removed by the runtime. /// Calling this while already inside the runtime is a no-op. - pub fn enter_runtime(&mut self) { - self.top.enter_runtime(); + pub fn enter_runtime(&mut self) -> Result<(), AlreadyInRuntime> { + self.top.enter_runtime()?; for (_, (changeset, _)) in self.children.iter_mut() { - changeset.enter_runtime(); + changeset.enter_runtime() + .expect("Top and children changesets are entering runtime in lockstep; qed") } + Ok(()) } /// Call this when control returns from the runtime. /// /// This commits all dangling transaction left open by the runtime. /// Calling this while already outside the runtime is a no-op. - pub fn exit_runtime(&mut self) { - self.top.exit_runtime(); + pub fn exit_runtime(&mut self) -> Result<(), NotInRuntime> { + self.top.exit_runtime()?; for (_, (changeset, _)) in self.children.iter_mut() { - changeset.exit_runtime(); + changeset.exit_runtime() + .expect("Top and children changesets are entering runtime in lockstep; qed"); } + Ok(()) } /// Consume all changes (top + children) and return them. diff --git a/primitives/state-machine/src/overlayed_changes/changeset.rs b/primitives/state-machine/src/overlayed_changes/changeset.rs index 79f6405afa62b..82b5c5b73f566 100644 --- a/primitives/state-machine/src/overlayed_changes/changeset.rs +++ b/primitives/state-machine/src/overlayed_changes/changeset.rs @@ -36,6 +36,16 @@ type Transactions = SmallVec<[InnerValue; 5]>; #[cfg_attr(test, derive(PartialEq))] pub struct NoOpenTransaction; +/// Error when calling `enter_runtime` when already being in runtime execution mode. +#[derive(Debug)] +#[cfg_attr(test, derive(PartialEq))] +pub struct AlreadyInRuntime; + +/// Error when calling `exit_runtime` when not being in runtime exection mdde. +#[derive(Debug)] +#[cfg_attr(test, derive(PartialEq))] +pub struct NotInRuntime; + /// Describes in which mode the node is currently executing. #[derive(Debug, Clone, Copy)] pub enum ExecutionMode { @@ -278,25 +288,30 @@ impl OverlayedChangeSet { /// Call this before transfering control to the runtime. /// /// This protects all existing transactions from being removed by the runtime. - /// Calling this while already inside the runtime is a no-op. - pub fn enter_runtime(&mut self) { + /// Calling this while already inside the runtime will return an error. + pub fn enter_runtime(&mut self) -> Result<(), AlreadyInRuntime> { if let ExecutionMode::Runtime = self.execution_mode { - return; + return Err(AlreadyInRuntime); } self.execution_mode = ExecutionMode::Runtime; self.num_client_transactions = self.transaction_depth(); + Ok(()) } /// Call this when control returns from the runtime. /// /// This commits all dangling transaction left open by the runtime. - /// Calling this while already outside the runtime is a no-op. - pub fn exit_runtime(&mut self) { + /// Calling this while already outside the runtime will return an error. + pub fn exit_runtime(&mut self) -> Result<(), NotInRuntime> { + if let ExecutionMode::Client = self.execution_mode { + return Err(NotInRuntime); + } self.execution_mode = ExecutionMode::Client; while self.transaction_depth() > self.num_client_transactions { self.commit_transaction() .expect("The loop condition checks that the transaction depth is > 0; qed"); } + Ok(()) } /// Start a new nested transaction. @@ -707,7 +722,7 @@ mod test { fn runtime_cannot_close_client_tx() { let mut changeset = OverlayedChangeSet::default(); changeset.start_transaction(); - changeset.enter_runtime(); + changeset.enter_runtime().unwrap(); changeset.start_transaction(); changeset.commit_transaction().unwrap(); assert_eq!(changeset.commit_transaction(), Err(NoOpenTransaction)); @@ -722,10 +737,10 @@ mod test { changeset.set(b"key0".to_vec(), Some(b"val0".to_vec()), Some(1)); - changeset.enter_runtime(); + changeset.enter_runtime().unwrap(); changeset.start_transaction(); changeset.set(b"key1".to_vec(), Some(b"val1".to_vec()), Some(2)); - changeset.exit_runtime(); + changeset.exit_runtime().unwrap(); changeset.commit_transaction().unwrap(); assert_eq!(changeset.transaction_depth(), 0); @@ -735,4 +750,15 @@ mod test { (b"key1", Some(b"val1")), ]); } + + #[test] + fn enter_exit_runtime_fails_when_already_in_requested_mode() { + let mut changeset = OverlayedChangeSet::default(); + + assert_eq!(changeset.exit_runtime(), Err(NotInRuntime)); + assert_eq!(changeset.enter_runtime(), Ok(())); + assert_eq!(changeset.enter_runtime(), Err(AlreadyInRuntime)); + assert_eq!(changeset.exit_runtime(), Ok(())); + assert_eq!(changeset.exit_runtime(), Err(NotInRuntime)); + } } From c60a4c1e63feb4b573a39b7d940cf96d708cda4c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Wed, 10 Jun 2020 17:27:57 +0200 Subject: [PATCH 25/38] Documentation fixup --- primitives/state-machine/src/overlayed_changes.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/primitives/state-machine/src/overlayed_changes.rs b/primitives/state-machine/src/overlayed_changes.rs index eb385842e6a43..7cbc189dfeae4 100644 --- a/primitives/state-machine/src/overlayed_changes.rs +++ b/primitives/state-machine/src/overlayed_changes.rs @@ -353,7 +353,7 @@ impl OverlayedChanges { /// Call this before transfering control to the runtime. /// /// This protects all existing transactions from being removed by the runtime. - /// Calling this while already inside the runtime is a no-op. + /// Calling this while already inside the runtime will return an error. pub fn enter_runtime(&mut self) -> Result<(), AlreadyInRuntime> { self.top.enter_runtime()?; for (_, (changeset, _)) in self.children.iter_mut() { @@ -366,7 +366,7 @@ impl OverlayedChanges { /// Call this when control returns from the runtime. /// /// This commits all dangling transaction left open by the runtime. - /// Calling this while already outside the runtime is a no-op. + /// Calling this while outside the runtime will return an error. pub fn exit_runtime(&mut self) -> Result<(), NotInRuntime> { self.top.exit_runtime()?; for (_, (changeset, _)) in self.children.iter_mut() { From 1ad20777ad30bdccf13239c5211971826471e74a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Thu, 11 Jun 2020 10:37:51 +0200 Subject: [PATCH 26/38] Remove close type --- .../src/overlayed_changes/changeset.rs | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/primitives/state-machine/src/overlayed_changes/changeset.rs b/primitives/state-machine/src/overlayed_changes/changeset.rs index 82b5c5b73f566..540bde4a99c11 100644 --- a/primitives/state-machine/src/overlayed_changes/changeset.rs +++ b/primitives/state-machine/src/overlayed_changes/changeset.rs @@ -55,15 +55,6 @@ pub enum ExecutionMode { Runtime, } -/// Describes which kind of transaction close should be performed. -#[derive(Debug, Clone, Copy)] -enum CloseType { - /// Commit the transaction. - Commit, - /// Rollback the transaction. - Rollback, -} - #[derive(Debug, Default, Clone)] #[cfg_attr(test, derive(PartialEq))] struct InnerValue { @@ -330,7 +321,7 @@ impl OverlayedChangeSet { /// Any changes made during that transaction are discarded. Returns an error if /// there is no open transaction that can be rolled back. pub fn rollback_transaction(&mut self) -> Result<(), NoOpenTransaction> { - self.close_transaction(CloseType::Rollback) + self.close_transaction(true) } /// Commit the last transaction started by `start_transaction`. @@ -338,10 +329,10 @@ impl OverlayedChangeSet { /// Any changes made during that transaction are committed. Returns an error if /// there is no open transaction that can be committed. pub fn commit_transaction(&mut self) -> Result<(), NoOpenTransaction> { - self.close_transaction(CloseType::Commit) + self.close_transaction(false) } - fn close_transaction(&mut self, close_type: CloseType) -> Result<(), NoOpenTransaction> { + fn close_transaction(&mut self, rollback: bool) -> Result<(), NoOpenTransaction> { // runtime is not allowed to close transactions started by the client if let ExecutionMode::Runtime = self.execution_mode { if self.num_client_transactions == self.transaction_depth() { @@ -356,7 +347,7 @@ impl OverlayedChangeSet { function is only called for keys that are in the dirty set. qed\ "); - if let CloseType::Rollback = close_type { + if rollback { overlayed.pop_transaction(); // We need to remove the key as an `OverlayValue` with no transactions From de8af5b6920920a5409a5b92449f65901ae96aaf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Thu, 11 Jun 2020 11:39:17 +0200 Subject: [PATCH 27/38] Move enter/exit runtime to excute_aux in the state-machine --- primitives/runtime-interface/test/src/lib.rs | 1 - primitives/state-machine/src/ext.rs | 17 ----------------- primitives/state-machine/src/lib.rs | 7 ++++++- 3 files changed, 6 insertions(+), 19 deletions(-) diff --git a/primitives/runtime-interface/test/src/lib.rs b/primitives/runtime-interface/test/src/lib.rs index 5600d3d2283fb..109caab6062f8 100644 --- a/primitives/runtime-interface/test/src/lib.rs +++ b/primitives/runtime-interface/test/src/lib.rs @@ -55,7 +55,6 @@ fn call_wasm_method_with_result( &mut ext_ext, sp_core::traits::MissingHostFunctions::Disallow, ).map_err(|e| format!("Failed to execute `{}`: {}", method, e))?; - drop(ext_ext); Ok(ext) } diff --git a/primitives/state-machine/src/ext.rs b/primitives/state-machine/src/ext.rs index 60880063b8775..2cd63cde975de 100644 --- a/primitives/state-machine/src/ext.rs +++ b/primitives/state-machine/src/ext.rs @@ -113,8 +113,6 @@ where changes_trie_state: Option>, extensions: Option<&'a mut Extensions>, ) -> Self { - overlay.enter_runtime() - .expect("Instances of this tpye must be created outside of the runtime."); Self { overlay, offchain_overlay, @@ -161,21 +159,6 @@ where } } -impl<'a, H, B, N> Drop for Ext<'a, H, N, B> -where - H: Hasher, - B: 'a + Backend, - N: crate::changes_trie::BlockNumber -{ - fn drop(&mut self) { - self.overlay.exit_runtime().expect("\ - This instance holds an exclusive borrow of the overlay and calls enter_runtime on - creation. We do not call exit_runtime in any other place of this type. - Drop is only called once. qed - "); - } -} - impl<'a, H, B, N> Externalities for Ext<'a, H, N, B> where H: Hasher, diff --git a/primitives/state-machine/src/lib.rs b/primitives/state-machine/src/lib.rs index facfa7bd35ae4..b91c95402f54f 100644 --- a/primitives/state-machine/src/lib.rs +++ b/primitives/state-machine/src/lib.rs @@ -301,6 +301,9 @@ impl<'a, B, H, N, Exec> StateMachine<'a, B, H, N, Exec> where None => &mut cache, }; + self.overlay.enter_runtime() + .expect("This function is supposed to be called from the client only."); + let mut ext = Ext::new( self.overlay, self.offchain_overlay, @@ -328,6 +331,9 @@ impl<'a, B, H, N, Exec> StateMachine<'a, B, H, N, Exec> where native_call, ); + self.overlay.exit_runtime() + .expect("Runtime is not able to call this function in the overlay; qed"); + trace!( target: "state", "{:04x}: Return. Native={:?}, Result={:?}", id, @@ -1300,7 +1306,6 @@ mod tests { ext.set_child_storage(&child_info_1, b"abc".to_vec(), b"def".to_vec()); ext.set_child_storage(&child_info_2, b"abc".to_vec(), b"def".to_vec()); ext.storage_root(); - drop(ext); cache.transaction.unwrap() }; let mut duplicate = false; From 4cf20c99812f83df267eccacdaaf63ba05725c6b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Thu, 11 Jun 2020 16:05:42 +0200 Subject: [PATCH 28/38] Rename Discard -> Rollback --- frame/support/src/storage/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/support/src/storage/mod.rs b/frame/support/src/storage/mod.rs index 58d1462f69fad..146e635ad5c57 100644 --- a/frame/support/src/storage/mod.rs +++ b/frame/support/src/storage/mod.rs @@ -40,7 +40,7 @@ pub enum TransactionOutcome { /// Execute the supplied function in a new storage transaction. /// /// All changes to storage performed by the supplied function are discarded if the returned -/// outcome is `TransactionOutcome::Discard`. +/// outcome is `TransactionOutcome::Rollback`. /// /// Transactions can be nested to any depth. Commits happen to the parent transaction. pub fn with_transaction(f: impl FnOnce() -> (R, TransactionOutcome)) -> R { From 10022f6d5030f3068af5d7e4515754d092803da8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Thu, 11 Jun 2020 16:20:59 +0200 Subject: [PATCH 29/38] Move child changeset creation to constructor --- .../state-machine/src/overlayed_changes.rs | 39 +++++++++++-------- .../src/overlayed_changes/changeset.rs | 25 ++++-------- 2 files changed, 31 insertions(+), 33 deletions(-) diff --git a/primitives/state-machine/src/overlayed_changes.rs b/primitives/state-machine/src/overlayed_changes.rs index 7cbc189dfeae4..de31d01156881 100644 --- a/primitives/state-machine/src/overlayed_changes.rs +++ b/primitives/state-machine/src/overlayed_changes.rs @@ -245,19 +245,16 @@ impl OverlayedChanges { let size_write = val.as_ref().map(|x| x.len() as u64).unwrap_or(0); self.stats.tally_write_overlay(size_write); let storage_key = child_info.storage_key().to_vec(); - let tx_depth = self.transaction_depth(); - let num_client_tx = self.top.num_client_transactions(); - let exec_mode = self.top.execution_mode(); - let (overlay, info) = self.children.entry(storage_key) - .or_insert_with(|| ( - // This changeset might be created when there are already open transactions. - // We need to catch up here so it is at the same transaction depth. - OverlayedChangeSet::new(tx_depth, num_client_tx, exec_mode), + let changeset = self.top.spawn_child(); + let (changeset, info) = self.children.entry(storage_key).or_insert_with(|| + ( + changeset, child_info.to_owned() - )); + ) + ); let updatable = info.try_update(child_info); debug_assert!(updatable); - overlay.set(key, val, extrinsic_index); + changeset.set(key, val, extrinsic_index); } /// Clear child storage of given storage key. @@ -268,9 +265,14 @@ impl OverlayedChanges { child_info: &ChildInfo, ) { let extrinsic_index = self.extrinsic_index(); - let storage_key = child_info.storage_key(); - let (changeset, info) = self.children.entry(storage_key.to_vec()) - .or_insert_with(|| (Default::default(), child_info.to_owned())); + let storage_key = child_info.storage_key().to_vec(); + let changeset = self.top.spawn_child(); + let (changeset, info) = self.children.entry(storage_key).or_insert_with(|| + ( + changeset, + child_info.to_owned() + ) + ); let updatable = info.try_update(child_info); debug_assert!(updatable); changeset.clear_where(|_, _| true, extrinsic_index); @@ -292,9 +294,14 @@ impl OverlayedChanges { prefix: &[u8], ) { let extrinsic_index = self.extrinsic_index(); - let storage_key = child_info.storage_key(); - let (changeset, info) = self.children.entry(storage_key.to_vec()) - .or_insert_with(|| (Default::default(), child_info.to_owned())); + let storage_key = child_info.storage_key().to_vec(); + let changeset = self.top.spawn_child(); + let (changeset, info) = self.children.entry(storage_key).or_insert_with(|| + ( + changeset, + child_info.to_owned() + ) + ); let updatable = info.try_update(child_info); debug_assert!(updatable); changeset.clear_where(|key, _| key.starts_with(prefix), extrinsic_index); diff --git a/primitives/state-machine/src/overlayed_changes/changeset.rs b/primitives/state-machine/src/overlayed_changes/changeset.rs index 540bde4a99c11..d1b3bc2ec0d88 100644 --- a/primitives/state-machine/src/overlayed_changes/changeset.rs +++ b/primitives/state-machine/src/overlayed_changes/changeset.rs @@ -161,13 +161,16 @@ fn insert_dirty(set: &mut DirtyKeysSets, key: StorageKey) -> bool { } impl OverlayedChangeSet { - /// Create a new changeset. - pub fn new(depth: usize, num_client_transactions: usize, mode: ExecutionMode) -> Self { + /// Create a new changeset at the same transaction state but without any contents. + /// + /// This changeset might be created when there are already open transactions. + /// We need to catch up here so that the child is at the same transaction depth. + pub fn spawn_child(&self) -> Self { use std::iter::repeat; Self { - dirty_keys: repeat(HashSet::new()).take(depth).collect(), - num_client_transactions, - execution_mode: mode, + dirty_keys: repeat(HashSet::new()).take(self.transaction_depth()).collect(), + num_client_transactions: self.num_client_transactions, + execution_mode: self.execution_mode, .. Default::default() } } @@ -264,18 +267,6 @@ impl OverlayedChangeSet { self.dirty_keys.len() } - /// Returns how many of the open transactions are opened by the client. - /// - /// These are always the first transaction to open and the last to close. - pub fn num_client_transactions(&self) -> usize { - self.num_client_transactions - } - - /// Returns whether we are executing in client or runtime mode. - pub fn execution_mode(&self) -> ExecutionMode { - self.execution_mode - } - /// Call this before transfering control to the runtime. /// /// This protects all existing transactions from being removed by the runtime. From 5c0c35f66d3b4d03d00460423f80be01bad741d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Thu, 11 Jun 2020 16:51:30 +0200 Subject: [PATCH 30/38] Move child spawning into the closure --- primitives/state-machine/src/overlayed_changes.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/primitives/state-machine/src/overlayed_changes.rs b/primitives/state-machine/src/overlayed_changes.rs index de31d01156881..467e35021b9ea 100644 --- a/primitives/state-machine/src/overlayed_changes.rs +++ b/primitives/state-machine/src/overlayed_changes.rs @@ -245,10 +245,10 @@ impl OverlayedChanges { let size_write = val.as_ref().map(|x| x.len() as u64).unwrap_or(0); self.stats.tally_write_overlay(size_write); let storage_key = child_info.storage_key().to_vec(); - let changeset = self.top.spawn_child(); + let top = &self.top; let (changeset, info) = self.children.entry(storage_key).or_insert_with(|| ( - changeset, + top.spawn_child(), child_info.to_owned() ) ); @@ -266,10 +266,10 @@ impl OverlayedChanges { ) { let extrinsic_index = self.extrinsic_index(); let storage_key = child_info.storage_key().to_vec(); - let changeset = self.top.spawn_child(); + let top = &self.top; let (changeset, info) = self.children.entry(storage_key).or_insert_with(|| ( - changeset, + top.spawn_child(), child_info.to_owned() ) ); @@ -295,10 +295,10 @@ impl OverlayedChanges { ) { let extrinsic_index = self.extrinsic_index(); let storage_key = child_info.storage_key().to_vec(); - let changeset = self.top.spawn_child(); + let top = &self.top; let (changeset, info) = self.children.entry(storage_key).or_insert_with(|| ( - changeset, + top.spawn_child(), child_info.to_owned() ) ); From 76ce5cf4d30af8003cf21d4b2b90e53845ec2c21 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Fri, 12 Jun 2020 18:38:06 +0200 Subject: [PATCH 31/38] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Köcher --- .../state-machine/src/overlayed_changes.rs | 10 ++++------ .../src/overlayed_changes/changeset.rs | 18 +++++++----------- 2 files changed, 11 insertions(+), 17 deletions(-) diff --git a/primitives/state-machine/src/overlayed_changes.rs b/primitives/state-machine/src/overlayed_changes.rs index 467e35021b9ea..f65be587f41bf 100644 --- a/primitives/state-machine/src/overlayed_changes.rs +++ b/primitives/state-machine/src/overlayed_changes.rs @@ -190,7 +190,7 @@ impl OverlayedChanges { } /// Returns mutable reference to current value. - /// If there is no value in the overlay, the default callback is used to initiate the value. + /// If there is no value in the overlay, the given callback is used to initiate the value. /// Warning this function registers a change, so the mutable reference MUST be modified. /// /// Can be rolled back or committed when called inside a transaction. @@ -210,14 +210,12 @@ impl OverlayedChanges { /// to the backend); Some(None) if the key has been deleted. Some(Some(...)) for a key whose /// value has been set. pub fn child_storage(&self, child_info: &ChildInfo, key: &[u8]) -> Option> { - if let Some(map) = self.children.get(child_info.storage_key()) { - if let Some(val) = map.0.get(key) { + let map = self.children.get(child_info.storage_key())?; + let val = map.0.get(key)?; let value = val.value(); let size_read = value.map(|x| x.len() as u64).unwrap_or(0); self.stats.tally_read_modified(size_read); - return Some(value.map(AsRef::as_ref)); - } - } + Some(value.map(AsRef::as_ref)) None } diff --git a/primitives/state-machine/src/overlayed_changes/changeset.rs b/primitives/state-machine/src/overlayed_changes/changeset.rs index d1b3bc2ec0d88..46fb4f9820732 100644 --- a/primitives/state-machine/src/overlayed_changes/changeset.rs +++ b/primitives/state-machine/src/overlayed_changes/changeset.rs @@ -131,7 +131,7 @@ impl OverlayedValue { &mut self, value: Option, first_write_in_tx: bool, - at_extrinsic: Option + at_extrinsic: Option, ) { if first_write_in_tx || self.transactions.is_empty() { self.transactions.push(InnerValue { @@ -153,11 +153,7 @@ impl OverlayedValue { /// Returns true iff we are currently have at least one open transaction and if this /// is the first write to the given key that transaction. fn insert_dirty(set: &mut DirtyKeysSets, key: StorageKey) -> bool { - if let Some(dirty_keys) = set.last_mut() { - dirty_keys.insert(key) - } else { - false - } + set.last_mut().map(|dk| dk.insert(key)).unwrap_or_default() } impl OverlayedChangeSet { @@ -192,9 +188,9 @@ impl OverlayedChangeSet { &mut self, key: StorageKey, value: Option, - at_extrinsic: Option + at_extrinsic: Option, ) { - let overlayed = self.changes.entry(key.clone()).or_insert_with(Default::default); + let overlayed = self.changes.entry(key.clone()).or_default(); overlayed.set(value, insert_dirty(&mut self.dirty_keys, key), at_extrinsic); } @@ -206,9 +202,9 @@ impl OverlayedChangeSet { &mut self, key: StorageKey, init: impl Fn() -> StorageValue, - at_extrinsic: Option + at_extrinsic: Option, ) -> &mut Option { - let overlayed = self.changes.entry(key.clone()).or_insert_with(Default::default); + let overlayed = self.changes.entry(key.clone()).or_default(); let first_write_in_tx = insert_dirty(&mut self.dirty_keys, key); let clone_into_new_tx = if let Some(tx) = overlayed.transactions.last() { if first_write_in_tx { @@ -232,7 +228,7 @@ impl OverlayedChangeSet { pub fn clear_where( &mut self, predicate: impl Fn(&[u8], &OverlayedValue) -> bool, - at_extrinsic: Option + at_extrinsic: Option, ) { for (key, val) in self.changes.iter_mut().filter(|(k, v)| predicate(k, v)) { val.set(None, insert_dirty(&mut self.dirty_keys, key.to_owned()), at_extrinsic); From 3d0f16289a80f30551434f40df24ba1ace050b1e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Fri, 12 Jun 2020 18:40:37 +0200 Subject: [PATCH 32/38] Fixup for code suggestion --- primitives/state-machine/src/overlayed_changes.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/primitives/state-machine/src/overlayed_changes.rs b/primitives/state-machine/src/overlayed_changes.rs index f65be587f41bf..5346be02d81ce 100644 --- a/primitives/state-machine/src/overlayed_changes.rs +++ b/primitives/state-machine/src/overlayed_changes.rs @@ -211,12 +211,10 @@ impl OverlayedChanges { /// value has been set. pub fn child_storage(&self, child_info: &ChildInfo, key: &[u8]) -> Option> { let map = self.children.get(child_info.storage_key())?; - let val = map.0.get(key)?; - let value = val.value(); - let size_read = value.map(|x| x.len() as u64).unwrap_or(0); - self.stats.tally_read_modified(size_read); + let value = map.0.get(key)?.value(); + let size_read = value.map(|x| x.len() as u64).unwrap_or(0); + self.stats.tally_read_modified(size_read); Some(value.map(AsRef::as_ref)) - None } /// Set a new value for the specified key. From 1651f68f0c01577444687c2a434c57adfdcba6d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Fri, 12 Jun 2020 18:42:58 +0200 Subject: [PATCH 33/38] Unify re-exports --- primitives/state-machine/src/overlayed_changes.rs | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/primitives/state-machine/src/overlayed_changes.rs b/primitives/state-machine/src/overlayed_changes.rs index 5346be02d81ce..9a2b1c4197310 100644 --- a/primitives/state-machine/src/overlayed_changes.rs +++ b/primitives/state-machine/src/overlayed_changes.rs @@ -35,17 +35,7 @@ use sp_core::storage::{well_known_keys::EXTRINSIC_INDEX, ChildInfo}; use sp_core::offchain::storage::OffchainOverlayedChanges; use hash_db::Hasher; -/// Re-export of `changeset::OverlayedValue`. -pub use self::changeset::OverlayedValue; - -/// Re-export of `changeset::NoOpenTransaction`. -pub use self::changeset::NoOpenTransaction; - -/// Re-export of `changeset::AlreadyInRutnime`. -pub use self::changeset::AlreadyInRuntime; - -/// Re-export of `changeset::NotInruntime`. -pub use self::changeset::NotInRuntime; +pub use self::changeset::{OverlayedValue, NoOpenTransaction, AlreadyInRuntime, NotInRuntime}; /// Storage key. pub type StorageKey = Vec; From 64f3c90f9a160c1c07eca4cb2150d717e6933e3f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Fri, 12 Jun 2020 18:43:53 +0200 Subject: [PATCH 34/38] Rename overlay_changes to mod.rs and move into subdir --- .../src/{overlayed_changes.rs => overlayed_changes/mod.rs} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename primitives/state-machine/src/{overlayed_changes.rs => overlayed_changes/mod.rs} (100%) diff --git a/primitives/state-machine/src/overlayed_changes.rs b/primitives/state-machine/src/overlayed_changes/mod.rs similarity index 100% rename from primitives/state-machine/src/overlayed_changes.rs rename to primitives/state-machine/src/overlayed_changes/mod.rs From 1568984fd64f976dffe40420b815e70cbb77dca4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Tue, 16 Jun 2020 10:15:11 +0200 Subject: [PATCH 35/38] Change proof wording --- primitives/state-machine/src/lib.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/primitives/state-machine/src/lib.rs b/primitives/state-machine/src/lib.rs index b91c95402f54f..9f761f9661760 100644 --- a/primitives/state-machine/src/lib.rs +++ b/primitives/state-machine/src/lib.rs @@ -301,8 +301,7 @@ impl<'a, B, H, N, Exec> StateMachine<'a, B, H, N, Exec> where None => &mut cache, }; - self.overlay.enter_runtime() - .expect("This function is supposed to be called from the client only."); + self.overlay.enter_runtime().expect("StateMachine is never called from the runtime; qed"); let mut ext = Ext::new( self.overlay, From 6a8b3a79dc13a15173acb00044579c996583d95b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Fri, 19 Jun 2020 10:09:45 +0200 Subject: [PATCH 36/38] Adapt a new test from master to storage-tx --- primitives/state-machine/src/lib.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/primitives/state-machine/src/lib.rs b/primitives/state-machine/src/lib.rs index 67b34e74a927c..e5e48bc47cd48 100644 --- a/primitives/state-machine/src/lib.rs +++ b/primitives/state-machine/src/lib.rs @@ -1328,9 +1328,11 @@ mod tests { let backend = state.as_trie_backend().unwrap(); let mut overlay = OverlayedChanges::default(); + overlay.start_transaction(); overlay.set_storage(b"ccc".to_vec(), Some(b"".to_vec())); assert_eq!(overlay.storage(b"ccc"), Some(Some(&[][..]))); - overlay.commit_prospective(); + overlay.commit_transaction().unwrap(); + overlay.start_transaction(); assert_eq!(overlay.storage(b"ccc"), Some(Some(&[][..]))); assert_eq!(overlay.storage(b"bbb"), None); @@ -1350,7 +1352,7 @@ mod tests { ext.clear_storage(b"ccc"); assert_eq!(ext.storage(b"ccc"), None); } - overlay.commit_prospective(); + overlay.commit_transaction().unwrap(); assert_eq!(overlay.storage(b"ccc"), Some(None)); } } From 46a56dc257054db8a35c64a3d128cf585fa5163c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Sat, 20 Jun 2020 12:36:10 +0200 Subject: [PATCH 37/38] Suggestions from the latest round of review --- frame/support/src/storage/mod.rs | 16 ++++++------- .../support/test/tests/storage_transaction.rs | 14 +++++------ .../src/overlayed_changes/changeset.rs | 24 +++++++++++++------ 3 files changed, 31 insertions(+), 23 deletions(-) diff --git a/frame/support/src/storage/mod.rs b/frame/support/src/storage/mod.rs index 146e635ad5c57..c2d7ceef0fee6 100644 --- a/frame/support/src/storage/mod.rs +++ b/frame/support/src/storage/mod.rs @@ -30,11 +30,11 @@ pub mod generator; pub mod migration; /// Describes whether a storage transaction should be committed or rolled back. -pub enum TransactionOutcome { +pub enum TransactionOutcome { /// Transaction should be committed. - Commit, + Commit(T), /// Transaction should be rolled back. - Rollback, + Rollback(T), } /// Execute the supplied function in a new storage transaction. @@ -43,19 +43,17 @@ pub enum TransactionOutcome { /// outcome is `TransactionOutcome::Rollback`. /// /// Transactions can be nested to any depth. Commits happen to the parent transaction. -pub fn with_transaction(f: impl FnOnce() -> (R, TransactionOutcome)) -> R { +pub fn with_transaction(f: impl FnOnce() -> TransactionOutcome) -> R { use sp_io::storage::{ start_transaction, commit_transaction, rollback_transaction, }; use TransactionOutcome::*; start_transaction(); - let (result, outcome) = f(); - match outcome { - Commit => commit_transaction(), - Rollback => rollback_transaction(), + match f() { + Commit(res) => { commit_transaction(); res }, + Rollback(res) => { rollback_transaction(); res }, } - result } /// A trait for working with macro-generated storage values under the substrate storage API. diff --git a/frame/support/test/tests/storage_transaction.rs b/frame/support/test/tests/storage_transaction.rs index 136e665c72af2..bf6c70966b469 100644 --- a/frame/support/test/tests/storage_transaction.rs +++ b/frame/support/test/tests/storage_transaction.rs @@ -1,6 +1,6 @@ // This file is part of Substrate. -// Copyright (C) 2019-2020 Parity Technologies (UK) Ltd. +// Copyright (C) 2020 Parity Technologies (UK) Ltd. // SPDX-License-Identifier: Apache-2.0 // Licensed under the Apache License, Version 2.0 (the "License"); @@ -50,7 +50,7 @@ fn storage_transaction_basic_commit() { Map::insert("val0", 99); assert_eq!(Value::get(), 99); assert_eq!(Map::get("val0"), 99); - ((), Commit) + Commit(()) }); assert_eq!(Value::get(), 99); @@ -70,7 +70,7 @@ fn storage_transaction_basic_rollback() { Map::insert("val0", 99); assert_eq!(Value::get(), 99); assert_eq!(Map::get("val0"), 99); - ((), Rollback) + Rollback(()) }); assert_eq!(Value::get(), 0); @@ -100,7 +100,7 @@ fn storage_transaction_rollback_then_commit() { assert_eq!(Map::get("val2"), 3); assert_eq!(Map::get("val3"), 3); - ((), Rollback) + Rollback(()) }); assert_eq!(Value::get(), 2); @@ -108,7 +108,7 @@ fn storage_transaction_rollback_then_commit() { assert_eq!(Map::get("val2"), 2); assert_eq!(Map::get("val3"), 0); - ((), Commit) + Commit(()) }); assert_eq!(Value::get(), 2); @@ -140,7 +140,7 @@ fn storage_transaction_commit_then_rollback() { assert_eq!(Map::get("val2"), 3); assert_eq!(Map::get("val3"), 3); - ((), Commit) + Commit(()) }); assert_eq!(Value::get(), 3); @@ -148,7 +148,7 @@ fn storage_transaction_commit_then_rollback() { assert_eq!(Map::get("val2"), 3); assert_eq!(Map::get("val3"), 3); - ((), Rollback) + Rollback(()) }); assert_eq!(Value::get(), 1); diff --git a/primitives/state-machine/src/overlayed_changes/changeset.rs b/primitives/state-machine/src/overlayed_changes/changeset.rs index 46fb4f9820732..d3f1822d5fa84 100644 --- a/primitives/state-machine/src/overlayed_changes/changeset.rs +++ b/primitives/state-machine/src/overlayed_changes/changeset.rs @@ -1,6 +1,6 @@ // This file is part of Substrate. -// Copyright (C) 2017-2020 Parity Technologies (UK) Ltd. +// Copyright (C) 2020 Parity Technologies (UK) Ltd. // SPDX-License-Identifier: Apache-2.0 // Licensed under the Apache License, Version 2.0 (the "License"); @@ -22,6 +22,7 @@ use super::{StorageKey, StorageValue}; use itertools::Itertools; use std::collections::{HashSet, BTreeMap, BTreeSet}; use smallvec::SmallVec; +use log::warn; const PROOF_OVERLAY_NON_EMPTY: &str = "\ An OverlayValue is always created with at least one transaction and dropped as soon @@ -285,8 +286,14 @@ impl OverlayedChangeSet { return Err(NotInRuntime); } self.execution_mode = ExecutionMode::Client; - while self.transaction_depth() > self.num_client_transactions { - self.commit_transaction() + if self.has_open_runtime_transactions() { + warn!( + "{} storage transactions are left open by the runtime. Those will be rolled back.", + self.transaction_depth() + ); + } + while self.has_open_runtime_transactions() { + self.rollback_transaction() .expect("The loop condition checks that the transaction depth is > 0; qed"); } Ok(()) @@ -322,7 +329,7 @@ impl OverlayedChangeSet { fn close_transaction(&mut self, rollback: bool) -> Result<(), NoOpenTransaction> { // runtime is not allowed to close transactions started by the client if let ExecutionMode::Runtime = self.execution_mode { - if self.num_client_transactions == self.transaction_depth() { + if !self.has_open_runtime_transactions() { return Err(NoOpenTransaction) } } @@ -350,10 +357,10 @@ impl OverlayedChangeSet { // Last tx: Is there already a value in the committed set? // Check against one rather than empty because the current tx is still // in the list as it is popped later in this function. - overlayed.transactions.len() != 1 + overlayed.transactions.len() > 1 }; - // We only need to merge if there is an pre-existing value. It may be avalue from + // We only need to merge if there is an pre-existing value. It may be a value from // the previous transaction or a value committed without any open transaction. if has_predecessor { let dropped_tx = overlayed.pop_transaction(); @@ -365,6 +372,10 @@ impl OverlayedChangeSet { Ok(()) } + + fn has_open_runtime_transactions(&self) -> bool { + self.transaction_depth() > self.num_client_transactions + } } #[cfg(test)] @@ -725,7 +736,6 @@ mod test { assert_drained(changeset, vec![ (b"key0", Some(b"val0")), - (b"key1", Some(b"val1")), ]); } From 5145916fd19700cc88ee7dfb1cd2a5f124704ab4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Sun, 21 Jun 2020 11:47:06 +0200 Subject: [PATCH 38/38] Fix warning message --- primitives/state-machine/src/overlayed_changes/changeset.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/primitives/state-machine/src/overlayed_changes/changeset.rs b/primitives/state-machine/src/overlayed_changes/changeset.rs index d3f1822d5fa84..fe43c0ea99d89 100644 --- a/primitives/state-machine/src/overlayed_changes/changeset.rs +++ b/primitives/state-machine/src/overlayed_changes/changeset.rs @@ -289,7 +289,7 @@ impl OverlayedChangeSet { if self.has_open_runtime_transactions() { warn!( "{} storage transactions are left open by the runtime. Those will be rolled back.", - self.transaction_depth() + self.transaction_depth() - self.num_client_transactions, ); } while self.has_open_runtime_transactions() {