From 52b241977522fe21d6f8073a23520872de4930c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Tue, 5 May 2020 15:25:20 +0200 Subject: [PATCH 01/12] Hide internal structure of OverlayChanges --- .../state-machine/src/changes_trie/build.rs | 41 ++++-------- primitives/state-machine/src/ext.rs | 23 +++---- primitives/state-machine/src/lib.rs | 13 ++-- .../state-machine/src/overlayed_changes.rs | 64 ++++++++++++++++--- primitives/state-machine/src/testing.rs | 24 ++++--- 5 files changed, 92 insertions(+), 73 deletions(-) diff --git a/primitives/state-machine/src/changes_trie/build.rs b/primitives/state-machine/src/changes_trie/build.rs index f8eabfce9b70d..715a9d7a4e10c 100644 --- a/primitives/state-machine/src/changes_trie/build.rs +++ b/primitives/state-machine/src/changes_trie/build.rs @@ -17,7 +17,7 @@ //! Structures and functions required to build changes trie for given block. -use std::collections::{BTreeMap, BTreeSet}; +use std::collections::BTreeMap; use std::collections::btree_map::Entry; use codec::{Decode, Encode}; use hash_db::Hasher; @@ -33,7 +33,7 @@ use crate::{ input::{InputKey, InputPair, DigestIndex, ExtrinsicIndex, ChildIndex}, }, }; -use sp_core::storage::{ChildInfo, ChildType, PrefixedStorageKey}; +use sp_core::storage::{ChildInfo, PrefixedStorageKey}; /// Prepare input pairs for building a changes trie of given block. /// @@ -106,20 +106,18 @@ fn prepare_extrinsics_input<'a, B, H, Number>( H: Hasher + 'a, Number: BlockNumber, { - - let mut children_info = BTreeSet::::new(); let mut children_result = BTreeMap::new(); - for (_storage_key, (_map, child_info)) in changes.prospective.children_default.iter() - .chain(changes.committed.children_default.iter()) { - children_info.insert(child_info.clone()); - } - for child_info in children_info { + + for child_info in changes.child_infos() { let child_index = ChildIndex:: { block: block.clone(), storage_key: child_info.prefixed_storage_key(), }; - let iter = prepare_extrinsics_input_inner(backend, block, changes, Some(child_info))?; + let iter = prepare_extrinsics_input_inner( + backend, block, changes, + Some(child_info.clone()) + )?; children_result.insert(child_index, iter); } @@ -139,19 +137,8 @@ fn prepare_extrinsics_input_inner<'a, B, H, Number>( H: Hasher, Number: BlockNumber, { - let (committed, prospective) = if let Some(child_info) = child_info.as_ref() { - match child_info.child_type() { - ChildType::ParentKeyId => ( - changes.committed.children_default.get(child_info.storage_key()).map(|c| &c.0), - changes.prospective.children_default.get(child_info.storage_key()).map(|c| &c.0), - ), - } - } else { - (Some(&changes.committed.top), Some(&changes.prospective.top)) - }; - committed.iter().flat_map(|c| c.iter()) - .chain(prospective.iter().flat_map(|c| c.iter())) - .filter(|( _, v)| v.extrinsics.is_some()) + changes.changes(child_info.as_ref()) + .filter(|( _, v)| v.extrinsics().is_some()) .try_fold(BTreeMap::new(), |mut map: BTreeMap<&[u8], (ExtrinsicIndex, Vec)>, (k, v)| { match map.entry(k) { Entry::Vacant(entry) => { @@ -172,9 +159,10 @@ fn prepare_extrinsics_input_inner<'a, B, H, Number>( } }; - let extrinsics = v.extrinsics.as_ref() + let extrinsics = v.extrinsics() .expect("filtered by filter() call above; qed") - .iter().cloned().collect(); + .cloned() + .collect(); entry.insert((ExtrinsicIndex { block: block.clone(), key: k.to_vec(), @@ -185,9 +173,8 @@ 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.as_ref() + v.extrinsics() .expect("filtered by filter() call above; qed") - .iter() .cloned() ); extrinsics.sort_unstable(); diff --git a/primitives/state-machine/src/ext.rs b/primitives/state-machine/src/ext.rs index e4c619efe3078..7ae616001c3f5 100644 --- a/primitives/state-machine/src/ext.rs +++ b/primitives/state-machine/src/ext.rs @@ -147,8 +147,7 @@ where self.backend.pairs().iter() .map(|&(ref k, ref v)| (k.to_vec(), Some(v.to_vec()))) - .chain(self.overlay.committed.top.clone().into_iter().map(|(k, v)| (k, v.value))) - .chain(self.overlay.prospective.top.clone().into_iter().map(|(k, v)| (k, v.value))) + .chain(self.overlay.changes(None).map(|(k, v)| (k.clone(), v.value().cloned()))) .collect::>() .into_iter() .filter_map(|(k, maybe_val)| maybe_val.map(|val| (k, val))) @@ -293,7 +292,7 @@ where match (next_backend_key, next_overlay_key_change) { (Some(backend_key), Some(overlay_key)) if &backend_key[..] < overlay_key.0 => Some(backend_key), (backend_key, None) => backend_key, - (_, Some(overlay_key)) => if overlay_key.1.value.is_some() { + (_, Some(overlay_key)) => if overlay_key.1.value().is_some() { Some(overlay_key.0.to_vec()) } else { self.next_storage_key(&overlay_key.0[..]) @@ -317,7 +316,7 @@ where match (next_backend_key, next_overlay_key_change) { (Some(backend_key), Some(overlay_key)) if &backend_key[..] < overlay_key.0 => Some(backend_key), (backend_key, None) => backend_key, - (_, Some(overlay_key)) => if overlay_key.1.value.is_some() { + (_, Some(overlay_key)) => if overlay_key.1.value().is_some() { Some(overlay_key.0.to_vec()) } else { self.next_child_storage_key( @@ -479,18 +478,12 @@ where root.encode() } else { - if let Some(child_info) = self.overlay.default_child_info(storage_key).cloned() { + if let Some(child_info) = self.overlay.default_child_info(storage_key) { let (root, is_empty, _) = { - let delta = self.overlay.committed.children_default.get(storage_key) - .into_iter() - .flat_map(|(map, _)| map.clone().into_iter().map(|(k, v)| (k, v.value))) - .chain( - self.overlay.prospective.children_default.get(storage_key) - .into_iter() - .flat_map(|(map, _)| map.clone().into_iter().map(|(k, v)| (k, v.value))) - ); - - self.backend.child_storage_root(&child_info, delta) + let delta = self.overlay.changes(Some(child_info)) + .map(|(k, v)| (k.clone(), v.value().cloned())); + + self.backend.child_storage_root(child_info, delta) }; let root = root.encode(); diff --git a/primitives/state-machine/src/lib.rs b/primitives/state-machine/src/lib.rs index e9367cbec539c..733a7ee1d6e45 100644 --- a/primitives/state-machine/src/lib.rs +++ b/primitives/state-machine/src/lib.rs @@ -28,7 +28,6 @@ use sp_core::{ storage::ChildInfo, NativeOrEncoded, NeverNativeValue, hexdisplay::HexDisplay, traits::{CodeExecutor, CallInWasmExt, RuntimeCode}, }; -use overlayed_changes::OverlayedChangeSet; use sp_externalities::Extensions; pub mod backend; @@ -336,7 +335,6 @@ impl<'a, B, H, N, Exec> StateMachine<'a, B, H, N, Exec> where fn execute_call_with_both_strategy( &mut self, mut native_call: Option, - orig_prospective: OverlayedChangeSet, on_consensus_failure: Handler, ) -> CallResult where @@ -347,10 +345,11 @@ impl<'a, B, H, N, Exec> StateMachine<'a, B, H, N, Exec> where CallResult, ) -> CallResult { + let pending_changes = self.overlay.clone_pending(); let (result, was_native) = self.execute_aux(true, native_call.take()); if was_native { - self.overlay.prospective = orig_prospective.clone(); + self.overlay.set_pending(pending_changes); let (wasm_result, _) = self.execute_aux( false, native_call, @@ -372,12 +371,12 @@ impl<'a, B, H, N, Exec> StateMachine<'a, B, H, N, Exec> where fn execute_call_with_native_else_wasm_strategy( &mut self, mut native_call: Option, - orig_prospective: OverlayedChangeSet, ) -> CallResult where R: Decode + Encode + PartialEq, NC: FnOnce() -> result::Result + UnwindSafe, { + let pending_changes = self.overlay.clone_pending(); let (result, was_native) = self.execute_aux( true, native_call.take(), @@ -386,7 +385,7 @@ impl<'a, B, H, N, Exec> StateMachine<'a, B, H, N, Exec> where if !was_native || result.is_ok() { result } else { - self.overlay.prospective = orig_prospective.clone(); + self.overlay.set_pending(pending_changes); let (wasm_result, _) = self.execute_aux( false, native_call, @@ -421,20 +420,16 @@ impl<'a, B, H, N, Exec> StateMachine<'a, B, H, N, Exec> where self.overlay.set_collect_extrinsics(changes_tries_enabled); let result = { - let orig_prospective = self.overlay.prospective.clone(); - match manager { ExecutionManager::Both(on_consensus_failure) => { self.execute_call_with_both_strategy( native_call.take(), - orig_prospective, on_consensus_failure, ) }, ExecutionManager::NativeElseWasm => { self.execute_call_with_native_else_wasm_strategy( native_call.take(), - orig_prospective, ) }, ExecutionManager::AlwaysWasm(trust_level) => { diff --git a/primitives/state-machine/src/overlayed_changes.rs b/primitives/state-machine/src/overlayed_changes.rs index 55d4e8db880fa..c0a244fe9c8c2 100644 --- a/primitives/state-machine/src/overlayed_changes.rs +++ b/primitives/state-machine/src/overlayed_changes.rs @@ -30,7 +30,7 @@ use crate::{ use std::iter::FromIterator; use std::collections::{HashMap, BTreeMap, BTreeSet}; use codec::{Decode, Encode}; -use sp_core::storage::{well_known_keys::EXTRINSIC_INDEX, ChildInfo}; +use sp_core::storage::{well_known_keys::EXTRINSIC_INDEX, ChildInfo, ChildType}; use sp_core::offchain::storage::OffchainOverlayedChanges; use std::{mem, ops}; @@ -55,13 +55,13 @@ pub type ChildStorageCollection = Vec<(StorageKey, StorageCollection)>; #[derive(Debug, Default, Clone)] pub struct OverlayedChanges { /// Changes that are not yet committed. - pub(crate) prospective: OverlayedChangeSet, + prospective: OverlayedChangeSet, /// Committed changes. - pub(crate) committed: OverlayedChangeSet, + committed: OverlayedChangeSet, /// True if extrinsics stats must be collected. - pub(crate) collect_extrinsics: bool, + collect_extrinsics: bool, /// Collect statistic on this execution. - pub(crate) stats: StateMachineStats, + stats: StateMachineStats, } /// The storage value, used inside OverlayedChanges. @@ -69,10 +69,10 @@ pub struct OverlayedChanges { #[cfg_attr(test, derive(PartialEq))] pub struct OverlayedValue { /// Current value. None if value has been deleted. - pub value: Option, + value: Option, /// The set of extrinsic indices where the values has been changed. /// Is filled only if runtime has announced changes trie support. - pub extrinsics: Option>, + extrinsics: Option>, } /// Prospective or committed overlayed change set. @@ -80,9 +80,9 @@ pub struct OverlayedValue { #[cfg_attr(test, derive(PartialEq))] pub struct OverlayedChangeSet { /// Top level storage changes. - pub top: BTreeMap, + top: BTreeMap, /// Child storage changes. The map key is the child storage key without the common prefix. - pub children_default: HashMap, ChildInfo)>, + children_default: HashMap, ChildInfo)>, } /// A storage changes structure that can be generated by the data collected in [`OverlayedChanges`]. @@ -187,6 +187,16 @@ impl FromIterator<(StorageKey, OverlayedValue)> for OverlayedChangeSet { } } +impl OverlayedValue { + pub fn value(&self) -> Option<&StorageValue> { + self.value.as_ref() + } + + 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 { @@ -499,6 +509,42 @@ impl OverlayedChanges { ) } + /// Get an iterator over all child trees that are stored in the overlay. + pub fn child_infos(&self) -> impl Iterator { + self.prospective.children_default.iter() + .chain(self.committed.children_default.iter()) + .map(|(_, v)| &v.1) + } + + /// Get an iterator over all pending and committed changes. + /// + /// Supplying `None` for `child_info` will only return changes that are in the top + /// tree. Specifying some `child_info` will return only the changes in that + /// child tree. + 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()) + } + + /// Return a clone of the currently pending changes. + pub fn clone_pending(&self) -> OverlayedChangeSet { + self.prospective.clone() + } + + /// Replace the currently pending changes. + pub fn set_pending(&mut self, pending: OverlayedChangeSet) { + self.prospective = pending; + } + /// Convert this instance with all changes into a [`StorageChanges`] instance. pub fn into_storage_changes< B: Backend, H: Hasher, N: BlockNumber diff --git a/primitives/state-machine/src/testing.rs b/primitives/state-machine/src/testing.rs index 47108b884a836..71124a68bb5cf 100644 --- a/primitives/state-machine/src/testing.rs +++ b/primitives/state-machine/src/testing.rs @@ -136,21 +136,19 @@ impl TestExternalities /// Return a new backend with all pending value. pub fn commit_all(&self) -> InMemoryBackend { - let top: Vec<_> = self.overlay.committed.top.clone().into_iter() - .chain(self.overlay.prospective.top.clone().into_iter()) - .map(|(k, v)| (k, v.value)).collect(); + let top: Vec<_> = self.overlay.changes(None) + .map(|(k, v)| (k.clone(), v.value().cloned())) + .collect(); let mut transaction = vec![(None, top)]; - self.overlay.committed.children_default.clone().into_iter() - .chain(self.overlay.prospective.children_default.clone().into_iter()) - .for_each(|(_storage_key, (map, child_info))| { - transaction.push(( - Some(child_info), - map.into_iter() - .map(|(k, v)| (k, v.value)) - .collect::>(), - )) - }); + for child_info in self.overlay.child_infos() { + transaction.push(( + Some(child_info.clone()), + self.overlay.changes(Some(child_info)) + .map(|(k, v)| (k.clone(), v.value().cloned())) + .collect(), + )) + } self.backend.update(transaction) } From 8874a8ed8df5fa8f1da75e53d9339f92593b7c32 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Mon, 11 May 2020 15:24:30 +0200 Subject: [PATCH 02/12] Fix tests for OverlayChanges refactor --- .../state-machine/src/changes_trie/build.rs | 95 +++++++------------ primitives/state-machine/src/ext.rs | 29 ++---- primitives/state-machine/src/lib.rs | 23 ++--- 3 files changed, 52 insertions(+), 95 deletions(-) diff --git a/primitives/state-machine/src/changes_trie/build.rs b/primitives/state-machine/src/changes_trie/build.rs index 715a9d7a4e10c..17435bf994c59 100644 --- a/primitives/state-machine/src/changes_trie/build.rs +++ b/primitives/state-machine/src/changes_trie/build.rs @@ -328,13 +328,10 @@ fn prepare_digest_input<'a, H, Number>( #[cfg(test)] mod test { - use codec::Encode; use sp_core::Blake2Hasher; - use sp_core::storage::well_known_keys::EXTRINSIC_INDEX; use crate::InMemoryBackend; use crate::changes_trie::{RootsStorage, Configuration, storage::InMemoryStorage}; use crate::changes_trie::build_cache::{IncompleteCacheAction, IncompleteCachedBuildData}; - use crate::overlayed_changes::{OverlayedValue, OverlayedChangeSet}; use super::*; fn prepare_for_build(zero: u64) -> ( @@ -354,8 +351,6 @@ mod test { (vec![105], vec![255]), ].into_iter().collect::>().into(); let prefixed_child_trie_key1 = child_info_1.prefixed_storage_key(); - let child_trie_key1 = child_info_1.storage_key().to_vec(); - let child_trie_key2 = child_info_2.storage_key().to_vec(); let storage = InMemoryStorage::with_inputs(vec![ (zero + 1, vec![ InputPair::ExtrinsicIndex(ExtrinsicIndex { block: zero + 1, key: vec![100] }, vec![1, 3]), @@ -405,58 +400,39 @@ mod test { ]), ]), ]); - let changes = OverlayedChanges { - prospective: OverlayedChangeSet { top: vec![ - (vec![100], OverlayedValue { - value: Some(vec![200]), - extrinsics: Some(vec![0, 2].into_iter().collect()) - }), - (vec![103], OverlayedValue { - value: None, - extrinsics: Some(vec![0, 1].into_iter().collect()) - }), - ].into_iter().collect(), - children_default: vec![ - (child_trie_key1.clone(), (vec![ - (vec![100], OverlayedValue { - value: Some(vec![200]), - extrinsics: Some(vec![0, 2].into_iter().collect()) - }) - ].into_iter().collect(), child_info_1.to_owned())), - (child_trie_key2, (vec![ - (vec![100], OverlayedValue { - value: Some(vec![200]), - extrinsics: Some(vec![0, 2].into_iter().collect()) - }) - ].into_iter().collect(), child_info_2.to_owned())), - ].into_iter().collect() - }, - committed: OverlayedChangeSet { top: vec![ - (EXTRINSIC_INDEX.to_vec(), OverlayedValue { - value: Some(3u32.encode()), - extrinsics: None, - }), - (vec![100], OverlayedValue { - value: Some(vec![202]), - extrinsics: Some(vec![3].into_iter().collect()) - }), - (vec![101], OverlayedValue { - value: Some(vec![203]), - extrinsics: Some(vec![1].into_iter().collect()) - }), - ].into_iter().collect(), - children_default: vec![ - (child_trie_key1, (vec![ - (vec![100], OverlayedValue { - value: Some(vec![202]), - extrinsics: Some(vec![3].into_iter().collect()) - }) - ].into_iter().collect(), child_info_1.to_owned())), - ].into_iter().collect(), - }, - collect_extrinsics: true, - stats: Default::default(), - }; + + let mut changes = OverlayedChanges::default(); + changes.set_collect_extrinsics(true); + + changes.set_extrinsic_index(1); + changes.set_storage(vec![101], Some(vec![203])); + + changes.set_extrinsic_index(3); + changes.set_storage(vec![100], Some(vec![202])); + changes.set_child_storage(&child_info_1, vec![100], Some(vec![202])); + + changes.commit_prospective(); + + changes.set_extrinsic_index(0); + changes.set_storage(vec![100], Some(vec![0])); + changes.set_extrinsic_index(2); + changes.set_storage(vec![100], Some(vec![200])); + + changes.set_extrinsic_index(0); + changes.set_storage(vec![103], Some(vec![0])); + changes.set_extrinsic_index(1); + changes.set_storage(vec![103], None); + + changes.set_extrinsic_index(0); + changes.set_child_storage(&child_info_1, vec![100], Some(vec![0])); + changes.set_extrinsic_index(2); + changes.set_child_storage(&child_info_1, vec![100], Some(vec![200])); + + changes.set_extrinsic_index(0); + changes.set_child_storage(&child_info_2, vec![100], Some(vec![0])); + changes.set_extrinsic_index(2); + changes.set_child_storage(&child_info_2, vec![100], Some(vec![200])); + let config = Configuration { digest_interval: 4, digest_levels: 2 }; (backend, storage, changes, config) @@ -654,10 +630,7 @@ mod test { let (backend, storage, mut changes, config) = prepare_for_build(zero); // 110: missing from backend, set to None in overlay - changes.prospective.top.insert(vec![110], OverlayedValue { - value: None, - extrinsics: Some(vec![1].into_iter().collect()) - }); + changes.set_storage(vec![110], None); let parent = AnchorBlockId { hash: Default::default(), number: zero + 3 }; let changes_trie_nodes = prepare_input( diff --git a/primitives/state-machine/src/ext.rs b/primitives/state-machine/src/ext.rs index 7ae616001c3f5..7bf64444ca929 100644 --- a/primitives/state-machine/src/ext.rs +++ b/primitives/state-machine/src/ext.rs @@ -663,35 +663,24 @@ mod tests { storage::{ Storage, StorageChild, - well_known_keys::EXTRINSIC_INDEX, }, }; use crate::{ changes_trie::{ Configuration as ChangesTrieConfiguration, InMemoryStorage as TestChangesTrieStorage, - }, InMemoryBackend, overlayed_changes::OverlayedValue, + }, InMemoryBackend, }; type TestBackend = InMemoryBackend; type TestExt<'a> = Ext<'a, Blake2Hasher, u64, TestBackend>; fn prepare_overlay_with_changes() -> OverlayedChanges { - OverlayedChanges { - prospective: vec![ - (EXTRINSIC_INDEX.to_vec(), OverlayedValue { - value: Some(3u32.encode()), - extrinsics: Some(vec![1].into_iter().collect()) - }), - (vec![1], OverlayedValue { - value: Some(vec![100].into_iter().collect()), - extrinsics: Some(vec![1].into_iter().collect()) - }), - ].into_iter().collect(), - committed: Default::default(), - collect_extrinsics: true, - stats: Default::default(), - } + let mut changes = OverlayedChanges::default(); + changes.set_collect_extrinsics(true); + changes.set_extrinsic_index(3); + changes.set_storage(vec![1], Some(vec![100])); + changes } fn prepare_offchain_overlay_with_changes() -> OffchainOverlayedChanges { @@ -739,7 +728,7 @@ mod tests { let mut ext = TestExt::new(&mut overlay, &mut offchain_overlay, &mut cache, &backend, state, None); assert_eq!( ext.storage_changes_root(&H256::default().encode()).unwrap(), - Some(hex!("bb0c2ef6e1d36d5490f9766cfcc7dfe2a6ca804504c3bb206053890d6dd02376").to_vec()), + Some(hex!("dfb314e4d5ddfaed1b6ef8d98d9898971bffb303a5d50e0b9c9a7bfd42ccdb90").to_vec()), ); } @@ -748,14 +737,14 @@ mod tests { let mut overlay = prepare_overlay_with_changes(); let mut offchain_overlay = prepare_offchain_overlay_with_changes(); let mut cache = StorageTransactionCache::default(); - overlay.prospective.top.get_mut(&vec![1]).unwrap().value = None; + overlay.set_storage(vec![1], None); let storage = TestChangesTrieStorage::with_blocks(vec![(99, Default::default())]); let state = Some(ChangesTrieState::new(changes_trie_config(), Zero::zero(), &storage)); let backend = TestBackend::default(); let mut ext = TestExt::new(&mut overlay, &mut offchain_overlay, &mut cache, &backend, state, None); assert_eq!( ext.storage_changes_root(&H256::default().encode()).unwrap(), - Some(hex!("96f5aae4690e7302737b6f9b7f8567d5bbb9eac1c315f80101235a92d9ec27f4").to_vec()), + Some(hex!("03170a2e7597b7b7e3d84c05391d139a62b157e78786d8c082f29dcf4c111314").to_vec()), ); } diff --git a/primitives/state-machine/src/lib.rs b/primitives/state-machine/src/lib.rs index 733a7ee1d6e45..1c981da07c248 100644 --- a/primitives/state-machine/src/lib.rs +++ b/primitives/state-machine/src/lib.rs @@ -749,7 +749,6 @@ where mod tests { use std::collections::BTreeMap; use codec::Encode; - use overlayed_changes::OverlayedValue; use super::*; use super::ext::Ext; use super::changes_trie::Configuration as ChangesTrieConfig; @@ -972,21 +971,16 @@ mod tests { ]; let mut state = InMemoryBackend::::from(initial); let backend = state.as_trie_backend().unwrap(); - let mut overlay = OverlayedChanges { - committed: map![ - b"aba".to_vec() => OverlayedValue::from(Some(b"1312".to_vec())), - b"bab".to_vec() => OverlayedValue::from(Some(b"228".to_vec())) - ], - prospective: map![ - b"abd".to_vec() => OverlayedValue::from(Some(b"69".to_vec())), - b"bbd".to_vec() => OverlayedValue::from(Some(b"42".to_vec())) - ], - ..Default::default() - }; - let mut offchain_overlay = Default::default(); + 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.set_storage(b"abd".to_vec(), Some(b"69".to_vec())); + overlay.set_storage(b"bbd".to_vec(), Some(b"42".to_vec())); { + let mut offchain_overlay = Default::default(); let mut cache = StorageTransactionCache::default(); let mut ext = Ext::new( &mut overlay, @@ -1001,7 +995,8 @@ mod tests { overlay.commit_prospective(); assert_eq!( - overlay.committed, + overlay.changes(None).map(|(k, v)| (k.clone(), v.value().cloned())) + .collect::>(), map![ b"abc".to_vec() => None.into(), b"abb".to_vec() => None.into(), From 4fd4427f8adc69d379ea520ee30ffe7f8141535d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Tue, 12 May 2020 17:36:48 +0200 Subject: [PATCH 03/12] Do not clone pending changes Discarding prospective changes should be equivalent as a state machine is not to be called with peding changes. This will be replaced by a storage transaction that is rolled back before executing the call the second time removing this constraint. --- primitives/state-machine/src/lib.rs | 6 ++---- primitives/state-machine/src/overlayed_changes.rs | 10 ---------- 2 files changed, 2 insertions(+), 14 deletions(-) diff --git a/primitives/state-machine/src/lib.rs b/primitives/state-machine/src/lib.rs index 1c981da07c248..7915c53885a57 100644 --- a/primitives/state-machine/src/lib.rs +++ b/primitives/state-machine/src/lib.rs @@ -345,11 +345,10 @@ impl<'a, B, H, N, Exec> StateMachine<'a, B, H, N, Exec> where CallResult, ) -> CallResult { - let pending_changes = self.overlay.clone_pending(); let (result, was_native) = self.execute_aux(true, native_call.take()); if was_native { - self.overlay.set_pending(pending_changes); + self.overlay.discard_prospective(); let (wasm_result, _) = self.execute_aux( false, native_call, @@ -376,7 +375,6 @@ 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(); let (result, was_native) = self.execute_aux( true, native_call.take(), @@ -385,7 +383,7 @@ impl<'a, B, H, N, Exec> StateMachine<'a, B, H, N, Exec> where if !was_native || result.is_ok() { result } else { - self.overlay.set_pending(pending_changes); + self.overlay.discard_prospective(); let (wasm_result, _) = self.execute_aux( false, native_call, diff --git a/primitives/state-machine/src/overlayed_changes.rs b/primitives/state-machine/src/overlayed_changes.rs index c0a244fe9c8c2..45c818e01f75d 100644 --- a/primitives/state-machine/src/overlayed_changes.rs +++ b/primitives/state-machine/src/overlayed_changes.rs @@ -535,16 +535,6 @@ impl OverlayedChanges { committed.into_iter().flatten().chain(prospective.into_iter().flatten()) } - /// Return a clone of the currently pending changes. - pub fn clone_pending(&self) -> OverlayedChangeSet { - self.prospective.clone() - } - - /// Replace the currently pending changes. - pub fn set_pending(&mut self, pending: OverlayedChangeSet) { - self.prospective = pending; - } - /// Convert this instance with all changes into a [`StorageChanges`] instance. pub fn into_storage_changes< B: Backend, H: Hasher, N: BlockNumber From 079b58ce6d4bc3ea18ea58a3fdbc31b9019d4170 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Tue, 12 May 2020 18:46:37 +0200 Subject: [PATCH 04/12] Doc fixes --- primitives/state-machine/src/overlayed_changes.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/primitives/state-machine/src/overlayed_changes.rs b/primitives/state-machine/src/overlayed_changes.rs index 45c818e01f75d..2ec18035cda97 100644 --- a/primitives/state-machine/src/overlayed_changes.rs +++ b/primitives/state-machine/src/overlayed_changes.rs @@ -188,10 +188,12 @@ impl FromIterator<(StorageKey, OverlayedValue)> for OverlayedChangeSet { } 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()) } @@ -509,7 +511,7 @@ impl OverlayedChanges { ) } - /// Get an iterator over all child trees that are stored in the overlay. + /// Get an iterator over all child tries that are stored in the overlay. pub fn child_infos(&self) -> impl Iterator { self.prospective.children_default.iter() .chain(self.committed.children_default.iter()) @@ -519,8 +521,8 @@ impl OverlayedChanges { /// Get an iterator over all pending and committed changes. /// /// Supplying `None` for `child_info` will only return changes that are in the top - /// tree. Specifying some `child_info` will return only the changes in that - /// child tree. + /// 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() { From 1fef5b107d40f96197f888f98932a00ed411582c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Tue, 12 May 2020 18:47:32 +0200 Subject: [PATCH 05/12] Remove overlong line --- primitives/state-machine/src/overlayed_changes.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/primitives/state-machine/src/overlayed_changes.rs b/primitives/state-machine/src/overlayed_changes.rs index 2ec18035cda97..768e734f2799d 100644 --- a/primitives/state-machine/src/overlayed_changes.rs +++ b/primitives/state-machine/src/overlayed_changes.rs @@ -523,7 +523,9 @@ impl OverlayedChanges { /// 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 { + 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 => ( From 531cded10101c8c4fcdd8f271ef6937af323c604 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Wed, 13 May 2020 12:45:19 +0200 Subject: [PATCH 06/12] Revert "Do not clone pending changes" This reverts commit 4799491f4ac16f8517287a0fcf4a3f84ad56f46e. --- primitives/state-machine/src/lib.rs | 6 ++++-- primitives/state-machine/src/overlayed_changes.rs | 10 ++++++++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/primitives/state-machine/src/lib.rs b/primitives/state-machine/src/lib.rs index 7915c53885a57..1c981da07c248 100644 --- a/primitives/state-machine/src/lib.rs +++ b/primitives/state-machine/src/lib.rs @@ -345,10 +345,11 @@ impl<'a, B, H, N, Exec> StateMachine<'a, B, H, N, Exec> where CallResult, ) -> CallResult { + let pending_changes = self.overlay.clone_pending(); let (result, was_native) = self.execute_aux(true, native_call.take()); if was_native { - self.overlay.discard_prospective(); + self.overlay.set_pending(pending_changes); let (wasm_result, _) = self.execute_aux( false, native_call, @@ -375,6 +376,7 @@ 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(); let (result, was_native) = self.execute_aux( true, native_call.take(), @@ -383,7 +385,7 @@ impl<'a, B, H, N, Exec> StateMachine<'a, B, H, N, Exec> where if !was_native || result.is_ok() { result } else { - self.overlay.discard_prospective(); + self.overlay.set_pending(pending_changes); let (wasm_result, _) = self.execute_aux( false, native_call, diff --git a/primitives/state-machine/src/overlayed_changes.rs b/primitives/state-machine/src/overlayed_changes.rs index 768e734f2799d..a0363529130b1 100644 --- a/primitives/state-machine/src/overlayed_changes.rs +++ b/primitives/state-machine/src/overlayed_changes.rs @@ -539,6 +539,16 @@ impl OverlayedChanges { committed.into_iter().flatten().chain(prospective.into_iter().flatten()) } + /// Return a clone of the currently pending changes. + pub fn clone_pending(&self) -> OverlayedChangeSet { + self.prospective.clone() + } + + /// Replace the currently pending changes. + pub fn set_pending(&mut self, pending: OverlayedChangeSet) { + self.prospective = pending; + } + /// Convert this instance with all changes into a [`StorageChanges`] instance. pub fn into_storage_changes< B: Backend, H: Hasher, N: BlockNumber From 8e60e6eae5bb24a09db8066a26c1df4087c8f728 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Wed, 13 May 2020 13:51:19 +0200 Subject: [PATCH 07/12] Deduplicate chield tries returned from child_infos() --- primitives/state-machine/src/overlayed_changes.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/primitives/state-machine/src/overlayed_changes.rs b/primitives/state-machine/src/overlayed_changes.rs index a0363529130b1..7a25e365a5ff8 100644 --- a/primitives/state-machine/src/overlayed_changes.rs +++ b/primitives/state-machine/src/overlayed_changes.rs @@ -511,11 +511,11 @@ impl OverlayedChanges { ) } - /// Get an iterator over all child tries that are stored in the overlay. - pub fn child_infos(&self) -> impl Iterator { + /// Get an iterator over all pending and committed child tries in the overlay. + pub fn child_infos(&self) -> impl IntoIterator { self.prospective.children_default.iter() .chain(self.committed.children_default.iter()) - .map(|(_, v)| &v.1) + .map(|(_, v)| &v.1).collect::>() } /// Get an iterator over all pending and committed changes. From 02c8def9f05233b8f67ca028d6a2db911a1f4862 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Wed, 13 May 2020 13:56:26 +0200 Subject: [PATCH 08/12] Remove redundant type annotation --- 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 7a25e365a5ff8..bbfaa3c40141e 100644 --- a/primitives/state-machine/src/overlayed_changes.rs +++ b/primitives/state-machine/src/overlayed_changes.rs @@ -515,7 +515,7 @@ impl OverlayedChanges { pub fn child_infos(&self) -> impl IntoIterator { self.prospective.children_default.iter() .chain(self.committed.children_default.iter()) - .map(|(_, v)| &v.1).collect::>() + .map(|(_, v)| &v.1).collect::>() } /// Get an iterator over all pending and committed changes. From 2a6ab5d1cfce7f8b691c896de0df541a2407faea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Thu, 14 May 2020 10:52:02 +0200 Subject: [PATCH 09/12] Avoid changing the storage root in tests --- primitives/state-machine/src/ext.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/primitives/state-machine/src/ext.rs b/primitives/state-machine/src/ext.rs index 7bf64444ca929..25c20644f779c 100644 --- a/primitives/state-machine/src/ext.rs +++ b/primitives/state-machine/src/ext.rs @@ -663,6 +663,7 @@ mod tests { storage::{ Storage, StorageChild, + well_known_keys::EXTRINSIC_INDEX, }, }; use crate::{ @@ -678,8 +679,9 @@ mod tests { fn prepare_overlay_with_changes() -> OverlayedChanges { let mut changes = OverlayedChanges::default(); changes.set_collect_extrinsics(true); - changes.set_extrinsic_index(3); + changes.set_extrinsic_index(1); changes.set_storage(vec![1], Some(vec![100])); + changes.set_storage(EXTRINSIC_INDEX.to_vec(), Some(3u32.encode())); changes } @@ -728,7 +730,7 @@ mod tests { let mut ext = TestExt::new(&mut overlay, &mut offchain_overlay, &mut cache, &backend, state, None); assert_eq!( ext.storage_changes_root(&H256::default().encode()).unwrap(), - Some(hex!("dfb314e4d5ddfaed1b6ef8d98d9898971bffb303a5d50e0b9c9a7bfd42ccdb90").to_vec()), + Some(hex!("bb0c2ef6e1d36d5490f9766cfcc7dfe2a6ca804504c3bb206053890d6dd02376").to_vec()), ); } @@ -737,6 +739,7 @@ mod tests { let mut overlay = prepare_overlay_with_changes(); let mut offchain_overlay = prepare_offchain_overlay_with_changes(); let mut cache = StorageTransactionCache::default(); + overlay.set_collect_extrinsics(false); overlay.set_storage(vec![1], None); let storage = TestChangesTrieStorage::with_blocks(vec![(99, Default::default())]); let state = Some(ChangesTrieState::new(changes_trie_config(), Zero::zero(), &storage)); @@ -744,7 +747,7 @@ mod tests { let mut ext = TestExt::new(&mut overlay, &mut offchain_overlay, &mut cache, &backend, state, None); assert_eq!( ext.storage_changes_root(&H256::default().encode()).unwrap(), - Some(hex!("03170a2e7597b7b7e3d84c05391d139a62b157e78786d8c082f29dcf4c111314").to_vec()), + Some(hex!("96f5aae4690e7302737b6f9b7f8567d5bbb9eac1c315f80101235a92d9ec27f4").to_vec()), ); } From 069f1d55cb7e789447ff0d08b0445448984ca772 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Thu, 14 May 2020 10:56:38 +0200 Subject: [PATCH 10/12] Preserve extrinsic indices in trie build test --- primitives/state-machine/src/changes_trie/build.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/primitives/state-machine/src/changes_trie/build.rs b/primitives/state-machine/src/changes_trie/build.rs index 17435bf994c59..f9698f1a31dbf 100644 --- a/primitives/state-machine/src/changes_trie/build.rs +++ b/primitives/state-machine/src/changes_trie/build.rs @@ -433,6 +433,8 @@ mod test { changes.set_extrinsic_index(2); changes.set_child_storage(&child_info_2, vec![100], Some(vec![200])); + changes.set_extrinsic_index(1); + let config = Configuration { digest_interval: 4, digest_levels: 2 }; (backend, storage, changes, config) From fe48580c4ca0576cbd7f9ade6d24d7664d73b7c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Thu, 14 May 2020 10:59:31 +0200 Subject: [PATCH 11/12] Swap order of comitted and prospective in fn child_infos This is only for consistency and does not impact the result. --- 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 bbfaa3c40141e..dcf61eae7d4db 100644 --- a/primitives/state-machine/src/overlayed_changes.rs +++ b/primitives/state-machine/src/overlayed_changes.rs @@ -513,8 +513,8 @@ impl OverlayedChanges { /// Get an iterator over all pending and committed child tries in the overlay. pub fn child_infos(&self) -> impl IntoIterator { - self.prospective.children_default.iter() - .chain(self.committed.children_default.iter()) + self.committed.children_default.iter() + .chain(self.prospective.children_default.iter()) .map(|(_, v)| &v.1).collect::>() } From 58fb712a9db654ceff114cb2d39926fadd3f5567 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Thu, 14 May 2020 11:01:05 +0200 Subject: [PATCH 12/12] Rename set_pending to replace_pending for clearity --- primitives/state-machine/src/lib.rs | 4 ++-- primitives/state-machine/src/overlayed_changes.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/primitives/state-machine/src/lib.rs b/primitives/state-machine/src/lib.rs index 1c981da07c248..3a54ef08f32b1 100644 --- a/primitives/state-machine/src/lib.rs +++ b/primitives/state-machine/src/lib.rs @@ -349,7 +349,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.set_pending(pending_changes); + self.overlay.replace_pending(pending_changes); let (wasm_result, _) = self.execute_aux( false, native_call, @@ -385,7 +385,7 @@ impl<'a, B, H, N, Exec> StateMachine<'a, B, H, N, Exec> where if !was_native || result.is_ok() { result } else { - self.overlay.set_pending(pending_changes); + self.overlay.replace_pending(pending_changes); let (wasm_result, _) = self.execute_aux( false, native_call, diff --git a/primitives/state-machine/src/overlayed_changes.rs b/primitives/state-machine/src/overlayed_changes.rs index dcf61eae7d4db..2da063c96e52f 100644 --- a/primitives/state-machine/src/overlayed_changes.rs +++ b/primitives/state-machine/src/overlayed_changes.rs @@ -545,7 +545,7 @@ impl OverlayedChanges { } /// Replace the currently pending changes. - pub fn set_pending(&mut self, pending: OverlayedChangeSet) { + pub fn replace_pending(&mut self, pending: OverlayedChangeSet) { self.prospective = pending; }