From 4c376f1d07696c5238db2d25e739acc938cef3f6 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Wed, 22 Mar 2023 15:40:37 +0000 Subject: [PATCH] Reimplement transactions as a stack and not through revert operations (#335) * Reimplement transactions as a stack and not through revert operations * CHANGELOG --- lib/src/executor/runtime_host.rs | 159 ++++++++++++++----------------- lib/src/trie/calculate_root.rs | 3 +- wasm-node/CHANGELOG.md | 4 + 3 files changed, 77 insertions(+), 89 deletions(-) diff --git a/lib/src/executor/runtime_host.rs b/lib/src/executor/runtime_host.rs index edda418dec..1545f237c7 100644 --- a/lib/src/executor/runtime_host.rs +++ b/lib/src/executor/runtime_host.rs @@ -45,7 +45,7 @@ use crate::{ use alloc::{borrow::ToOwned as _, string::String, vec::Vec}; use core::fmt; -use hashbrown::{hash_map::Entry, HashMap, HashSet}; +use hashbrown::HashSet; pub use trie::TrieEntryVersion; @@ -100,7 +100,7 @@ pub fn run( .into(), main_trie_changes: config.storage_main_trie_changes, state_trie_version, - main_trie_transaction_revert: Vec::new(), + main_trie_transaction: Vec::new(), offchain_storage_changes: config.offchain_storage_changes, main_trie_root_calculation_cache: Some( config.main_trie_root_calculation_cache.unwrap_or_default(), @@ -384,26 +384,22 @@ impl PrefixKeys { drop(after_overlay); - for key in keys_to_remove { - self.inner - .main_trie_root_calculation_cache - .as_mut() - .unwrap() - .storage_value_update(&key, false); + let (diff, calculation_cache) = self + .inner + .main_trie_transaction + .last_mut() + .map(|(a, b)| (a, b)) + .unwrap_or(( + &mut self.inner.main_trie_changes, + self.inner + .main_trie_root_calculation_cache + .as_mut() + .unwrap(), + )); - let previous_value = self - .inner - .main_trie_changes - .diff_insert_erase(key.clone(), ()) - .map(|(v, _)| v); - - if let Some(main_trie_transaction_revert) = - self.inner.main_trie_transaction_revert.last_mut() - { - if let Entry::Vacant(entry) = main_trie_transaction_revert.entry(key) { - entry.insert(previous_value); - } - } + for key in keys_to_remove { + calculation_cache.storage_value_update(&key, false); + diff.diff_insert_erase(key.clone(), ()); } self.inner.vm = req.resume(keys_removed_so_far, some_keys_remain); @@ -615,14 +611,13 @@ struct Inner { /// Pending changes to the top storage trie that this execution performs. main_trie_changes: storage_diff::TrieDiff, - /// Contains pending storage reverts if and only if we're within a storage transaction. - /// When changes are applied to [`Inner::main_trie_changes`], the reverse operation is - /// added here. + /// Contains pending storage changes that haven't been applied to [`Inner::main_trie_changes`] + /// yet. + /// When the storage transaction ends, either this is applied to [`Inner::main_trie_changes`], + /// or is entirely discarded (to revert changes). /// - /// When the storage transaction ends, either this hash map is entirely discarded (to commit - /// changes), or applied to [`Inner::main_trie_changes`] (to revert). - main_trie_transaction_revert: - Vec, Option>>, fnv::FnvBuildHasher>>, + /// Contains a `Vec` in case transactions are stacked. + main_trie_transaction: Vec<(storage_diff::TrieDiff, calculate_root::CalculationCache)>, /// State trie version indicated by the runtime. All the storage changes that are performed /// use this version. @@ -700,27 +695,20 @@ impl Inner { host::HostVm::ExternalStorageSet(req) => { // TODO: this is a dummy implementation and child tries are not implemented properly if let host::StorageKey::MainTrie { key } = req.key() { - self.main_trie_root_calculation_cache - .as_mut() - .unwrap() - .storage_value_update(key.as_ref(), req.value().is_some()); - - let previous_value = if let Some(value) = req.value() { - self.main_trie_changes - .diff_insert(key.as_ref(), value.as_ref(), ()) + let (diff, calculation_cache) = self + .main_trie_transaction + .last_mut() + .map(|(a, b)| (a, b)) + .unwrap_or(( + &mut self.main_trie_changes, + self.main_trie_root_calculation_cache.as_mut().unwrap(), + )); + + calculation_cache.storage_value_update(key.as_ref(), req.value().is_some()); + if let Some(value) = req.value() { + diff.diff_insert(key.as_ref(), value.as_ref(), ()); } else { - self.main_trie_changes.diff_insert_erase(key.as_ref(), ()) - } - .map(|(v, ())| v); - - if let Some(main_trie_transaction_revert) = - self.main_trie_transaction_revert.last_mut() - { - if let Entry::Vacant(entry) = - main_trie_transaction_revert.entry(key.as_ref().to_vec()) - { - entry.insert(previous_value); - } + diff.diff_insert_erase(key.as_ref(), ()); } } @@ -738,31 +726,22 @@ impl Inner { } }; - self.main_trie_root_calculation_cache - .as_mut() - .unwrap() - .storage_value_update(key.as_ref(), true); + let (diff, calculation_cache) = self + .main_trie_transaction + .last_mut() + .map(|(a, b)| (a, b)) + .unwrap_or(( + &mut self.main_trie_changes, + self.main_trie_root_calculation_cache.as_mut().unwrap(), + )); - let current_value = self - .main_trie_changes - .diff_get(key.as_ref()) - .map(|(v, _)| v); + calculation_cache.storage_value_update(key.as_ref(), true); + + let current_value = diff.diff_get(key.as_ref()).map(|(v, _)| v); if let Some(current_value) = current_value { let mut current_value = current_value.unwrap_or_default().to_vec(); append_to_storage_value(&mut current_value, req.value().as_ref()); - let previous_value = self - .main_trie_changes - .diff_insert(key.as_ref().to_vec(), current_value, ()) - .map(|(v, _)| v); - if let Some(main_trie_transaction_revert) = - self.main_trie_transaction_revert.last_mut() - { - if let Entry::Vacant(entry) = - main_trie_transaction_revert.entry(key.as_ref().to_vec()) - { - entry.insert(previous_value); - } - } + diff.diff_insert(key.as_ref().to_vec(), current_value, ()); drop(key); self.vm = req.resume(); } else { @@ -900,31 +879,35 @@ impl Inner { } host::HostVm::StartStorageTransaction(tx) => { - self.main_trie_transaction_revert.push(Default::default()); + self.main_trie_transaction.push(( + storage_diff::TrieDiff::default(), + self.main_trie_root_calculation_cache + .as_ref() + .unwrap() + .clone(), + )); + self.vm = tx.resume(); } host::HostVm::EndStorageTransaction { resume, rollback } => { // The inner implementation guarantees that a storage transaction can only // end if it has earlier been started. - debug_assert!(!self.main_trie_transaction_revert.is_empty()); - let last = self.main_trie_transaction_revert.pop().unwrap(); - - if rollback { - for (key, value) in last { - if let Some(value) = value { - if let Some(value) = value { - let _ = self.main_trie_changes.diff_insert(key, value, ()); - } else { - let _ = self.main_trie_changes.diff_insert_erase(key, ()); - } - } else { - let _ = self.main_trie_changes.diff_remove(&key); - } - } - - // TODO: very slow; do this properly - self.main_trie_root_calculation_cache = Some(Default::default()); + debug_assert!(!self.main_trie_transaction.is_empty()); + let (diff, cache_update) = self.main_trie_transaction.pop().unwrap(); + + if !rollback { + let (previous_diff, previous_calculation_cache) = self + .main_trie_transaction + .last_mut() + .map(|(a, b)| (a, b)) + .unwrap_or(( + &mut self.main_trie_changes, + self.main_trie_root_calculation_cache.as_mut().unwrap(), + )); + + previous_diff.merge(&diff); + *previous_calculation_cache = cache_update; } self.vm = resume.resume(); diff --git a/lib/src/trie/calculate_root.rs b/lib/src/trie/calculate_root.rs index 9124483212..fbd55fdefb 100644 --- a/lib/src/trie/calculate_root.rs +++ b/lib/src/trie/calculate_root.rs @@ -75,6 +75,7 @@ use core::{fmt, iter}; /// /// If the storage's content is modified, you **must** call the appropriate methods to invalidate /// entries. Otherwise, the trie root calculation will yield an incorrect result. +#[derive(Clone)] pub struct CalculationCache { /// Structure of the trie. /// If `Some`, the structure is either fully conforming to the trie. @@ -82,7 +83,7 @@ pub struct CalculationCache { } /// Custom data stored in each node in [`CalculationCache::structure`]. -#[derive(Default)] +#[derive(Default, Clone)] struct CacheEntry { merkle_value: Option, } diff --git a/wasm-node/CHANGELOG.md b/wasm-node/CHANGELOG.md index a47bc79896..f18557522f 100644 --- a/wasm-node/CHANGELOG.md +++ b/wasm-node/CHANGELOG.md @@ -8,6 +8,10 @@ - Add an arbitrary limit to the size of unprocessed networking packets, in order to avoid DoS attacks. This limit is necessary in order to bypass limitations in the networking APIs exposed by browsers. ([#312](https://github.com/smol-dot/smoldot/pull/312)) - Rename `/webrtc` to `/webrtc-direct` in multiaddresses, in accordance with the rest of the libp2p ecosystem. ([#326](https://github.com/smol-dot/smoldot/pull/326)) +### Fixed + +- Fix runtime transactions not being handled properly when multiple transactions are stacked. ([#335](https://github.com/smol-dot/smoldot/pull/335)) + ## 1.0.0 - 2022-03-12 ### Fixed