diff --git a/contracts/callcenter/src/lib.rs b/contracts/callcenter/src/lib.rs index 6f084168..953a701b 100644 --- a/contracts/callcenter/src/lib.rs +++ b/contracts/callcenter/src/lib.rs @@ -92,6 +92,11 @@ impl Callcenter { call_with_limit(contract, "spend", &(), points_limit)?; Ok(()) } + + /// Just panic. + pub fn panik(&self) { + panic!("panik"); + } } /// Expose `Callcenter::query_counter()` to the host @@ -159,3 +164,9 @@ unsafe fn delegate_transaction(arg_len: u32) -> u32 { STATE.delegate_transaction(mod_id, rt) }) } + +/// Expose `Callcenter::panik()` to the host +#[no_mangle] +unsafe fn panik(arg_len: u32) -> u32 { + wrap_call(arg_len, |()| STATE.panik()) +} diff --git a/crumbles/Cargo.toml b/crumbles/Cargo.toml index 3e819fa1..e8319fa4 100644 --- a/crumbles/Cargo.toml +++ b/crumbles/Cargo.toml @@ -15,3 +15,8 @@ license = "MPL-2.0" [dependencies] libc = "0.2" rangemap = "1" + +[dev-dependencies] +blake3 = "1" +hex = "0.4" +rand = "0.8" diff --git a/crumbles/src/lib.rs b/crumbles/src/lib.rs index baedbedd..19f93218 100644 --- a/crumbles/src/lib.rs +++ b/crumbles/src/lib.rs @@ -831,6 +831,8 @@ unsafe fn segfault_handler( mod tests { use super::*; + use rand::rngs::StdRng; + use rand::{Rng, SeedableRng}; use std::thread; const N_PAGES: usize = 65536; @@ -969,4 +971,82 @@ mod tests { "Slice should be the zeros written afterwards" ); } + + #[test] + fn apply_revert_apply() { + const N_WRITES: usize = 64; + let mut rng = StdRng::seed_from_u64(0xDEAD_BEEF); + + let mut mem = Mmap::new(N_PAGES, PAGE_SIZE) + .expect("Instantiating new memory should succeed"); + let mut mem_alt = Mmap::new(N_PAGES, PAGE_SIZE) + .expect("Instantiating new memory should succeed"); + + // Snapshot both, make the same changes on both, and apply the changes. + mem.snap().expect("Snapshotting should succeed"); + mem_alt.snap().expect("Snapshotting should succeed"); + + for _ in 0..N_WRITES { + let i = rng.gen_range(0..N_PAGES); + let byte = rng.gen(); + + mem[i] = byte; + mem_alt[i] = byte; + } + + mem.apply().expect("Applying should succeed"); + mem_alt.apply().expect("Applying should succeed"); + + // Snapshot one, make some changes, and revert it. + mem.snap().expect("Snapshotting should succeed"); + for _ in 0..N_WRITES { + let i = rng.gen_range(0..N_PAGES); + let byte = rng.gen(); + mem[i] = byte; + } + mem.revert().expect("Reverting should succeed"); + + // Snapshot both, make the same changes on both, and apply the changes. + mem.snap().expect("Snapshotting should succeed"); + mem_alt.snap().expect("Snapshotting should succeed"); + + for _ in 0..N_WRITES { + let i = rng.gen_range(0..N_PAGES); + let byte = rng.gen(); + + mem[i] = byte; + mem_alt[i] = byte; + } + + mem.apply().expect("Applying should succeed"); + mem_alt.apply().expect("Applying should succeed"); + + mem.dirty_pages().zip(mem_alt.dirty_pages()).for_each( + |((dirty, clean, index), (alt_dirty, alt_clean, alt_index))| { + let hash_dirty = blake3::hash(dirty); + let hash_alt_dirty = blake3::hash(alt_dirty); + + let hash_dirty = hex::encode(hash_dirty.as_bytes()); + let hash_alt_dirty = hex::encode(hash_alt_dirty.as_bytes()); + + assert_eq!( + hash_dirty, hash_alt_dirty, + "Dirty state should be the same" + ); + + let hash_clean = blake3::hash(clean); + let hash_alt_clean = blake3::hash(alt_clean); + + let hash_clean = hex::encode(hash_clean.as_bytes()); + let hash_alt_clean = hex::encode(hash_alt_clean.as_bytes()); + + assert_eq!( + hash_clean, hash_alt_clean, + "Clean state should be the same" + ); + + assert_eq!(index, alt_index, "Index should be the same"); + }, + ); + } } diff --git a/piecrust/CHANGELOG.md b/piecrust/CHANGELOG.md index c7ef59d2..e1b7dd37 100644 --- a/piecrust/CHANGELOG.md +++ b/piecrust/CHANGELOG.md @@ -7,6 +7,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- De-instantiate modules after call [#296] +- Change `Session::memory_len` to return `Result>`, and not + require a contract to be instantiated [#296] + +### Fixed + +- Fix inconsistent state root after erroring call [#296] + ## [0.13.0] - 2023-11-22 ## Added @@ -304,6 +314,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 [#234]: https://github.com/dusk-network/piecrust/pull/234 +[#296]: https://github.com/dusk-network/piecrust/issues/296 [#287]: https://github.com/dusk-network/piecrust/issues/287 [#281]: https://github.com/dusk-network/piecrust/issues/281 [#273]: https://github.com/dusk-network/piecrust/issues/273 diff --git a/piecrust/Cargo.toml b/piecrust/Cargo.toml index 46289e6a..e1430f83 100644 --- a/piecrust/Cargo.toml +++ b/piecrust/Cargo.toml @@ -46,6 +46,11 @@ name = "crossover" path = "tests/crossover.rs" required-features = ["debug"] +[[test]] +name = "commit" +path = "tests/commit.rs" +required-features = ["debug"] + [[test]] name = "debugger" path = "tests/debugger.rs" diff --git a/piecrust/src/instance.rs b/piecrust/src/instance.rs index e99adf1f..93cf3a8e 100644 --- a/piecrust/src/instance.rs +++ b/piecrust/src/instance.rs @@ -88,6 +88,7 @@ impl WrappedInstance { contract: &WrappedContract, memory: Memory, ) -> Result { + let mut memory = memory; let engine = session.engine().clone(); let env = Env { @@ -173,6 +174,9 @@ impl WrappedInstance { return Err(Error::InvalidArgumentBuffer); } + // A memory is no longer new after one instantiation + memory.is_new = false; + let wrapped = WrappedInstance { store, instance, diff --git a/piecrust/src/session.rs b/piecrust/src/session.rs index f7f0af0a..b58afebb 100644 --- a/piecrust/src/session.rs +++ b/piecrust/src/session.rs @@ -5,7 +5,6 @@ // Copyright (c) DUSK NETWORK. All rights reserved. use std::borrow::Cow; -use std::collections::btree_map::Entry; use std::collections::BTreeMap; use std::fmt::{Debug, Formatter}; use std::mem; @@ -92,7 +91,7 @@ struct SessionInner { current: ContractId, call_tree: CallTree, - instance_map: BTreeMap, + instances: BTreeMap, debug: Vec, data: SessionData, @@ -146,7 +145,7 @@ impl Session { let inner = SessionInner { current: ContractId::uninitialized(), call_tree: CallTree::new(), - instance_map: BTreeMap::new(), + instances: BTreeMap::new(), debug: vec![], data, contract_session, @@ -451,47 +450,35 @@ impl Session { /// Returns the current length of the memory of the given contract. /// - /// If the contract does not exist, or is otherwise not instantiated in this - /// session, it will return `None`. - pub fn memory_len(&self, contract_id: &ContractId) -> Option { - self.instance(contract_id) - .map(|instance| instance.mem_len()) + /// If the contract does not exist, it will return `None`. + pub fn memory_len( + &mut self, + contract_id: ContractId, + ) -> Result, Error> { + Ok(self + .inner + .contract_session + .contract(contract_id) + .map_err(|err| Error::PersistenceError(Arc::new(err)))? + .map(|data| data.memory.current_len)) } pub(crate) fn instance<'a>( &self, contract_id: &ContractId, ) -> Option<&'a mut WrappedInstance> { - self.inner - .instance_map - .get(contract_id) - .map(|(instance, _)| { - // SAFETY: We guarantee that the instance exists since we're in - // control over if it is dropped in `pop` - unsafe { &mut **instance } - }) - } - - fn update_instance_count(&mut self, contract_id: ContractId, inc: bool) { - match self.inner.instance_map.entry(contract_id) { - Entry::Occupied(mut entry) => { - let (_, count) = entry.get_mut(); - if inc { - *count += 1 - } else { - *count -= 1 - }; - } - _ => unreachable!("map must have an instance here"), - }; + self.inner.instances.get(contract_id).map(|instance| { + // SAFETY: We guarantee that the instance exists since we're in + // control over if it is dropped with the session. + unsafe { &mut **instance } + }) } fn clear_stack_and_instances(&mut self) { self.inner.call_tree.clear(); - while !self.inner.instance_map.is_empty() { - let (_, (instance, _)) = - self.inner.instance_map.pop_first().unwrap(); + while !self.inner.instances.is_empty() { + let (_, instance) = self.inner.instances.pop_first().unwrap(); unsafe { let _ = Box::from_raw(instance); }; @@ -582,7 +569,7 @@ impl Session { contract: ContractId, ) -> Result { let instance = self.new_instance(contract)?; - if self.inner.instance_map.get(&contract).is_some() { + if self.inner.instances.get(&contract).is_some() { panic!("Contract already in the stack: {contract:?}"); } @@ -591,7 +578,7 @@ impl Session { let instance = Box::new(instance); let instance = Box::leak(instance) as *mut WrappedInstance; - self.inner.instance_map.insert(contract, (instance, 1)); + self.inner.instances.insert(contract, instance); Ok(mem_len) } @@ -604,7 +591,6 @@ impl Session { match instance { Some(instance) => { - self.update_instance_count(contract_id, true); self.inner.call_tree.push(CallTreeElem { contract_id, limit, @@ -631,15 +617,11 @@ impl Session { } pub(crate) fn move_up_call_tree(&mut self, spent: u64) { - if let Some(element) = self.inner.call_tree.move_up(spent) { - self.update_instance_count(element.contract_id, false); - } + self.inner.call_tree.move_up(spent); } pub(crate) fn move_up_prune_call_tree(&mut self) { - if let Some(element) = self.inner.call_tree.move_up_prune() { - self.update_instance_count(element.contract_id, false); - } + self.inner.call_tree.move_up_prune(); } pub(crate) fn revert_callstack(&mut self) -> Result<(), std::io::Error> { @@ -730,6 +712,7 @@ impl Session { }; } self.move_up_prune_call_tree(); + self.clear_stack_and_instances(); err }) .map_err(Error::normalize)?; @@ -748,6 +731,7 @@ impl Session { io: Arc::new(err), })?; } + self.clear_stack_and_instances(); let mut call_tree = CallTree::new(); mem::swap(&mut self.inner.call_tree, &mut call_tree); diff --git a/piecrust/tests/commit.rs b/piecrust/tests/commit.rs index 9ddb979b..a8e2837f 100644 --- a/piecrust/tests/commit.rs +++ b/piecrust/tests/commit.rs @@ -232,6 +232,67 @@ fn multiple_commits() -> Result<(), Error> { Ok(()) } +#[test] +fn root_equal_on_err() -> Result<(), Error> { + let vm = VM::ephemeral()?; + + let mut session = vm.session(SessionData::builder())?; + + let callcenter_id = session.deploy( + contract_bytecode!("callcenter"), + ContractData::builder(OWNER), + LIMIT, + )?; + let counter_id = session.deploy( + contract_bytecode!("counter"), + ContractData::builder(OWNER), + LIMIT, + )?; + + let root = session.commit()?; + + let mut session_after = vm.session(SessionData::builder().base(root))?; + let mut session_after_alt = + vm.session(SessionData::builder().base(root))?; + + assert_eq!( + session_after.root(), + session_after_alt.root(), + "Roots should be equal at the beginning" + ); + + session_after + .call::<_, ()>(callcenter_id, "panik", &counter_id, LIMIT) + .expect_err("Calling with too little gas should error"); + + assert_eq!( + session_after.root(), + session_after_alt.root(), + "Roots should be equal immediately after erroring call" + ); + + session_after.call::<_, ()>( + callcenter_id, + "increment_counter", + &counter_id, + LIMIT, + )?; + session_after_alt.call::<_, ()>( + callcenter_id, + "increment_counter", + &counter_id, + LIMIT, + )?; + + assert_eq!( + session_after.root(), + session_after_alt.root(), + "Roots should be equal after call" + ); + + Ok(()) +} + fn increment_counter_and_commit( mut session: Session, id: ContractId, diff --git a/piecrust/tests/growth.rs b/piecrust/tests/growth.rs index e6389801..0a064dbf 100644 --- a/piecrust/tests/growth.rs +++ b/piecrust/tests/growth.rs @@ -71,8 +71,8 @@ fn error_reverts_growth() -> Result<(), Error> { )?; let initial_len = session - .memory_len(&id) - .expect("The contract should be instantiated in this session"); + .memory_len(id) + .expect("The contract should exist in this session"); // The first call to `append_error` will result in a call to memory.grow, // which must be reverted in both the state, and the size of the memory. @@ -94,11 +94,11 @@ fn error_reverts_growth() -> Result<(), Error> { assert_eq!(len, 0, "There should be nothing appended to the state"); let final_len = session - .memory_len(&id) - .expect("The contract should be instantiated in this session"); + .memory_len(id) + .expect("The contract should exist in this session"); let final_len_err = session_err - .memory_len(&id) - .expect("The contract should be instantiated in this session"); + .memory_len(id) + .expect("The contract should exist in this session"); assert!(initial_len < final_len); assert_eq!(