Fixing TrackingCopy caching mechanism so it doesn't return Some(_) when a key was pruned. Fixing Vector implementation.

Author: zajko

executor/wasm/tests/collections.rs

@@ -152,11 +152,11 @@ fn inserting_into_non_existing_vec_index_fails() {
         .expect("should build");
     let res = run_wasm_session(&mut executor, &global_state, state_root_hash, run_method);
     assert!(res.is_ok());
+
     if let Ok(res) = res {
-        let host_error = res.host_error;
-        assert!(host_error.is_some());
-        if let Some(err) = host_error {
-            println!("XXX {:?}", err);
-        }
+        assert!(matches!(
+            res.host_error,
+            Some(casper_executor_wasm_common::error::CallError::NotCallable)
+        ));
     }
 }

executor/wasm_host/src/host/global_state.rs

@@ -354,6 +354,7 @@ pub(crate) fn host_write<S: GlobalStateReader + 'static>(
         Keyspace::EntryPoint(_) => return Ok(HOST_ERROR_INVALID_INPUT),
     };
 
+    error!("XXXX2");
     metered_write(caller, global_state_key, stored_value)?;
 
     Ok(HOST_ERROR_SUCCESS)

smart_contracts/contracts/vm2/vm2-collections-test/src/vector/assert.rs

@@ -1,4 +1,4 @@
-use core::{panic, ptr::NonNull};
+use core::ptr::NonNull;
 
 use alloc::{vec, vec::Vec};
 use casper_contract_sdk::{
@@ -54,7 +54,21 @@ pub(crate) fn should_retain_assert(vec: &mut Vector<u64>) {
     assert_eq!(vec, vec![2, 4]);
 }
 
-pub(crate) fn test_vec_assert(vec: &mut Vector<u64>) {
+// Assert for scenario in which deletes happen in previous
+// state hash
+pub(crate) fn test_vec_1_assert() {
+    assert_eq!(
+        get_vec_elements_from_storage("test_vec_1"),
+        vec![41, 43, 42, 111, 222, 333]
+    );
+
+    let vec2 = Vector::<u64>::new("test1");
+    assert_eq!(vec2.get(0), None);
+
+    assert_eq!(get_vec_elements_from_storage("test1"), Vec::<u64>::new());
+}
+
+pub(crate) fn test_vec_2_assert(vec: &mut Vector<u64>) {
     assert_eq!(vec.remove(5), Some(334));
     assert_eq!(vec.remove(55), None);
 
@@ -81,7 +95,7 @@ pub(crate) fn test_vec_assert(vec: &mut Vector<u64>) {
     }
 
     assert_eq!(
-        get_vec_elements_from_storage("test_vec"),
+        get_vec_elements_from_storage("test_vec_2"),
         vec![41, 43, 42, 111, 222, 333]
     );
 

smart_contracts/contracts/vm2/vm2-collections-test/src/vector/mod.rs

@@ -9,9 +9,10 @@ use prepare::*;
 
 #[casper]
 pub(crate) struct VectorTestData {
-    should_not_panic_with_empty: Vector<u64>,
     should_retain: Vector<u64>,
-    test_vec: Vector<u64>,
+    should_not_panic_with_empty: Vector<u64>,
+    test_vec_1: Vector<u64>,
+    test_vec_2: Vector<u64>,
     test_pop: Vector<u64>,
     test_contains: Vector<u64>,
     test_clear: Vector<u64>,
@@ -26,9 +27,10 @@ pub(crate) struct VectorTestData {
 impl VectorTestData {
     pub(crate) fn new() -> Self {
         Self {
-            should_not_panic_with_empty: should_not_panic_with_empty_prepare(),
             should_retain: should_retain_prepare(),
-            test_vec: test_vec_prepare(),
+            should_not_panic_with_empty: should_not_panic_with_empty_prepare(),
+            test_vec_1: test_vec_1_prepare(),
+            test_vec_2: test_vec_2_prepare(),
             test_pop: test_pop_prepare(),
             test_contains: test_contains_prepare(),
             test_clear: test_clear_prepare(),
@@ -44,7 +46,7 @@ impl VectorTestData {
     pub(crate) fn do_assertions(&mut self) {
         let should_not_panic_with_empty = &mut self.should_not_panic_with_empty;
         let should_retain = &mut self.should_retain;
-        let test_vec = &mut self.test_vec;
+        let test_vec_2 = &mut self.test_vec_2;
         let test_pop = &mut self.test_pop;
         let test_contains = &mut self.test_contains;
         let test_clear = &mut self.test_clear;
@@ -56,7 +58,8 @@ impl VectorTestData {
         let test_remove_invalid_index = &mut self.test_remove_invalid_index;
         should_not_panic_with_empty_assert(should_not_panic_with_empty);
         should_retain_assert(should_retain);
-        test_vec_assert(test_vec);
+        test_vec_1_assert();
+        test_vec_2_assert(test_vec_2);
         test_pop_assert(test_pop);
         test_contains_assert(test_contains);
         test_clear_assert(test_clear);

smart_contracts/contracts/vm2/vm2-collections-test/src/vector/prepare.rs

@@ -1,4 +1,4 @@
-use casper_contract_sdk::{casper, collections::Vector};
+use casper_contract_sdk::collections::Vector;
 
 use crate::types::VectorTestStruct;
 
@@ -22,8 +22,51 @@ pub(crate) fn should_retain_prepare() -> Vector<u64> {
     vec
 }
 
-pub(crate) fn test_vec_prepare() -> Vector<u64> {
-    let mut vec = Vector::new("test_vec");
+pub(crate) fn test_vec_1_prepare() -> Vector<u64> {
+    let mut vec = Vector::new("test_vec_1");
+
+    assert!(vec.get(0).is_none());
+    vec.push(111);
+    assert_eq!(vec.get(0), Some(111));
+    vec.push(222);
+    assert_eq!(vec.get(1), Some(222));
+
+    vec.insert(0, 42);
+    vec.insert(0, 41);
+    vec.insert(1, 43);
+    vec.insert(5, 333);
+    vec.insert(5, 334);
+    assert_eq!(vec.remove(5), Some(334));
+    assert_eq!(vec.remove(55), None);
+
+    /*let to_return = vec.clone();
+        let mut iter = (&vec).iter();
+        assert_eq!(iter.next(), Some(41));
+        assert_eq!(iter.next(), Some(43));
+        assert_eq!(iter.next(), Some(42));
+        assert_eq!(iter.next(), Some(111));
+        assert_eq!(iter.next(), Some(222));
+        assert_eq!(iter.next(), Some(333));
+        assert_eq!(iter.next(), None);
+    */
+    {
+        let ser = borsh::to_vec(&vec).unwrap();
+        let deser: Vector<u64> = borsh::from_slice(&ser).unwrap();
+        let mut iter = deser.iter();
+        assert_eq!(iter.next(), Some(41));
+        assert_eq!(iter.next(), Some(43));
+        assert_eq!(iter.next(), Some(42));
+        assert_eq!(iter.next(), Some(111));
+        assert_eq!(iter.next(), Some(222));
+        assert_eq!(iter.next(), Some(333));
+        assert_eq!(iter.next(), None);
+    }
+
+    vec
+}
+
+pub(crate) fn test_vec_2_prepare() -> Vector<u64> {
+    let mut vec = Vector::new("test_vec_2");
 
     assert!(vec.get(0).is_none());
     vec.push(111);

smart_contracts/vm2/sdk/src/collections/vector.rs

@@ -14,7 +14,7 @@ use const_fnv1a_hash::fnv1a_hash_str_64;
 #[cfg(all(not(target_arch = "wasm32"), feature = "std"))]
 use crate::abi::{AbiDeclaration, CasperABI, Definition, StructField};
 
-#[derive(BorshSerialize, BorshDeserialize, Debug, Clone)]
+#[derive(BorshSerialize, BorshDeserialize, Debug)]
 #[borsh(crate = "crate::serializers::borsh")]
 pub struct Vector<T> {
     pub(crate) prefix: String,
@@ -71,19 +71,8 @@ where
 
     /// Appends an element to the back of a collection.
     pub fn push(&mut self, value: T) {
-        let prefix_bytes = self.compute_prefix_bytes_for_index(self.length);
-        let collection_prefix = fnv1a_hash_str_64(self.prefix.as_str()).to_le_bytes();
-        let addr = CollectionAddrInner::new(
-            *casper::get_callee().address(),
-            CollectionTypeTag::Vector,
-            collection_prefix,
-            casper::generic_hash(&prefix_bytes, crate::types::HashAlgorithm::Blake2b).unwrap(),
-        );
-        casper::write(
-            Keyspace::Context(ContextAddr::from(addr)),
-            &borsh::to_vec(&value).unwrap(),
-        )
-        .unwrap();
+        let addr = self.calculate_addr(self.length);
+        casper::write(Keyspace::Context(addr), &borsh::to_vec(&value).unwrap()).unwrap();
         self.length += 1;
     }
 
@@ -107,15 +96,8 @@ where
 
     /// Returns an element at index, deserialized.
     pub fn get(&self, index: u64) -> Option<T> {
-        let prefix = self.compute_prefix_bytes_for_index(index);
-        let collection_prefix = fnv1a_hash_str_64(self.prefix.as_str()).to_le_bytes();
-        let addr = CollectionAddrInner::new(
-            *casper::get_callee().address(),
-            CollectionTypeTag::Vector,
-            collection_prefix,
-            casper::generic_hash(&prefix, crate::types::HashAlgorithm::Blake2b).unwrap(),
-        );
-        let item_keyspace = Keyspace::Context(ContextAddr::from(addr));
+        let addr = self.calculate_addr(index);
+        let item_keyspace = Keyspace::Context(addr);
         read_into_vec(item_keyspace)
             .unwrap()
             .map(|vec| borsh::from_slice(&vec).unwrap())
@@ -149,15 +131,8 @@ where
     /// from the global state.
     pub fn clear(&mut self) {
         for i in 0..self.length {
-            let prefix_bytes = self.compute_prefix_bytes_for_index(i);
-            let collection_prefix = fnv1a_hash_str_64(self.prefix.as_str()).to_le_bytes();
-            let addr = CollectionAddrInner::new(
-                *casper::get_callee().address(),
-                CollectionTypeTag::Vector,
-                collection_prefix,
-                casper::generic_hash(&prefix_bytes, crate::types::HashAlgorithm::Blake2b).unwrap(),
-            );
-            casper::remove(Keyspace::Context(ContextAddr::from(addr))).unwrap();
+            let addr = self.calculate_addr(i);
+            casper::remove(Keyspace::Context(addr)).unwrap();
         }
         self.length = 0;
     }
@@ -239,7 +214,6 @@ where
         if index >= self.length {
             return None;
         }
-
         let value_to_remove = self.get(index).unwrap();
 
         // Shift elements to the left
@@ -248,20 +222,10 @@ where
                 self.write(i, next_value);
             }
         }
-
         // Remove the last element from storage
         self.length -= 1;
-        let addr = CollectionAddrInner::new(
-            *casper::get_callee().address(),
-            CollectionTypeTag::Vector,
-            [0u8; 8],
-            casper::generic_hash(
-                &self.compute_prefix_bytes_for_index(self.length),
-                crate::types::HashAlgorithm::Blake2b,
-            )
-            .unwrap(),
-        );
-        casper::remove(Keyspace::Context(ContextAddr::from(addr))).unwrap();
+        let addr = self.calculate_addr(self.length);
+        casper::remove(Keyspace::Context(addr)).unwrap();
 
         Some(value_to_remove)
     }
@@ -283,17 +247,8 @@ where
         }
 
         self.length -= 1;
-        let addr = CollectionAddrInner::new(
-            *casper::get_callee().address(),
-            CollectionTypeTag::Vector,
-            [0u8; 8],
-            casper::generic_hash(
-                &self.compute_prefix_bytes_for_index(self.length),
-                crate::types::HashAlgorithm::Blake2b,
-            )
-            .unwrap(),
-        );
-        casper::remove(Keyspace::Context(ContextAddr::from(addr))).unwrap();
+        let addr = self.calculate_addr(self.length);
+        casper::remove(Keyspace::Context(addr)).unwrap();
 
         Some(value_to_remove)
     }
@@ -319,6 +274,11 @@ where
     }
 
     fn write(&self, index: u64, value: T) {
+        let addr = self.calculate_addr(index);
+        casper::write(Keyspace::Context(addr), &borsh::to_vec(&value).unwrap()).unwrap();
+    }
+
+    fn calculate_addr(&self, index: u64) -> ContextAddr {
         let prefix_bytes = self.compute_prefix_bytes_for_index(index);
         let collection_prefix = fnv1a_hash_str_64(self.prefix.as_str()).to_le_bytes();
         let addr = CollectionAddrInner::new(
@@ -327,11 +287,7 @@ where
             collection_prefix,
             casper::generic_hash(&prefix_bytes, crate::types::HashAlgorithm::Blake2b).unwrap(),
         );
-        casper::write(
-            Keyspace::Context(ContextAddr::from(addr)),
-            &borsh::to_vec(&value).unwrap(),
-        )
-        .unwrap();
+        ContextAddr::from(addr)
     }
 }
 

storage/src/tracking_copy/mod.rs

@@ -14,7 +14,7 @@ use std::{
     borrow::Borrow,
     collections::{BTreeMap, BTreeSet, HashSet, VecDeque},
     convert::{From, TryInto},
-    fmt::Debug,
+    fmt::{Debug, Display},
     sync::Arc,
 };
 
@@ -101,6 +101,27 @@ impl TrackingCopyQueryResult {
     }
 }
 
+#[derive(Error, Debug, PartialEq, Eq)]
+/// Enum encapsulating data returned by the cache
+pub enum CacheEntry<'a> {
+    /// This entry was pruned
+    Pruned,
+    /// Cache is not aware of this element
+    NotFound,
+    /// Cache contains an unpruned version of this entry
+    Exists(&'a StoredValue),
+}
+
+impl Display for CacheEntry<'_> {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            CacheEntry::Pruned => write!(f, "TrackingCopyEntry::Pruned"),
+            CacheEntry::NotFound => write!(f, "TrackingCopyEntry::NotFound"),
+            CacheEntry::Exists(_) => write!(f, "TrackingCopyEntry::Exists"),
+        }
+    }
+}
+
 /// Struct containing state relating to a given query.
 struct Query {
     /// The key from where the search starts.
@@ -238,22 +259,27 @@ impl<M: Meter<Key, StoredValue> + Copy + Default> GenericTrackingCopyCache<M> {
 
     /// Inserts `key` and `value` pair to Write/Add cache.
     pub fn insert_prune(&mut self, key: Key) {
+        let kb = KeyWithByteRepr::new(key.clone());
+        self.muts_cached.remove(&kb);
         self.prunes_cached.insert(key);
     }
 
     /// Gets value from `key` in the cache.
-    pub fn get(&mut self, key: &Key) -> Option<&StoredValue> {
+    pub fn get<'a>(&'a mut self, key: &Key) -> CacheEntry<'a> {
         if self.prunes_cached.contains(key) {
             // the item is marked for pruning and therefore
             // is no longer accessible.
-            return None;
+            return CacheEntry::Pruned;
         }
         let kb = KeyWithByteRepr::new(*key);
         if let Some(value) = self.muts_cached.get(&kb) {
-            return Some(value);
+            return CacheEntry::Exists(value);
         };
 
-        self.reads_cached.get_refresh(key).map(|v| &*v)
+        match self.reads_cached.get_refresh(key).map(|v| &*v) {
+            Some(v) => CacheEntry::Exists(v),
+            None => CacheEntry::NotFound,
+        }
     }
 
     /// Get cached items by prefix.
@@ -485,9 +511,13 @@ where
 
     /// Get record by key.
     pub fn get(&mut self, key: &Key) -> Result<Option<StoredValue>, TrackingCopyError> {
-        if let Some(value) = self.cache.get(key) {
+        let get = self.cache.get(key);
+        if let CacheEntry::Exists(value) = get {
             return Ok(Some(value.to_owned()));
         }
+        if let CacheEntry::Pruned = get {
+            return Ok(None);
+        }
         match self.reader.read(key) {
             Ok(ret) => {
                 if let Some(value) = ret {