Skip to content

Commit

Permalink
Reimplement transactions as a stack and not through revert operations (…
Browse files Browse the repository at this point in the history 
…#335)

* Reimplement transactions as a stack and not through revert operations

* CHANGELOG
  • Loading branch information
@tomaka
tomaka authored Mar 22, 2023
1 parent d3891d6 commit 4c376f1
Show file tree
Hide file tree
Showing 3 changed files with 77 additions and 89 deletions.
159 changes: 71 additions & 88 deletions lib/src/executor/runtime_host.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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<HashMap<Vec<u8>, Option<Option<Vec<u8>>>, 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.
Expand Down Expand Up @@ -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(), ());
}
}

Expand All @@ -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 {
Expand Down Expand Up @@ -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();
Expand Down
3 changes: 2 additions & 1 deletion lib/src/trie/calculate_root.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,14 +75,15 @@ 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.
structure: Option<trie_structure::TrieStructure<CacheEntry>>,
}

/// Custom data stored in each node in [`CalculationCache::structure`].
#[derive(Default)]
#[derive(Default, Clone)]
struct CacheEntry {
merkle_value: Option<trie_node::MerkleValueOutput>,
}
Expand Down
4 changes: 4 additions & 0 deletions wasm-node/CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 4c376f1

Please sign in to comment.