Implement cache deletes #1029

iFrostizz · 2024-06-07T12:54:18Z

Cache deletes in the program state so that we now avoid cache misses when it comes to data deletion.
Also added a cache insertion when calling get.
The new cache now tells the caller wether the data is here Data(Vec<u8>), has been deleted Deleted or ToDelete if it's scheduled to be deleted when the cache is flushed.
There is a caveat though which is that ZST Vecs are completely disabled by borsh-rs. That's because they want to avoid a potential DOS since huge vecs can easily be crafted using ZST (they are free memory-wise!) with vec![(), !0].
See near/borsh-rs#52
We need to make sure that we are not exposed to that, or that fuel is correctly metering to avoid abuses.

Closes #867
Closes #868

* Add ability to add programs to the runtime and reuse this function in the program initialization flow

iFrostizz · 2024-06-10T11:52:14Z

x/programs/rust/sdk-macros/src/lib.rs

@@ -257,7 +257,7 @@ pub fn state_keys(_attr: TokenStream, item: TokenStream) -> TokenStream {

    // add default attributes
    item_enum.attrs.push(parse_quote! {
-         #[derive(Clone, Copy, Debug, Eq, PartialEq, Hash)]


a tab was off 😄

You can take this change and land it in a separate PR if you want it in earlier/immediately.

It's fine, it's only catching the eye when writing another aligned stuff just below

iFrostizz · 2024-06-10T11:53:43Z

x/programs/rust/wasmlanche-sdk/src/state.rs

+#[derive(Debug, PartialEq)]
+pub enum CachedData {
+    Data(Vec<u8>),
+    Deleted,
+    ToDelete,
+}


Here, Deleted means "we know that there is no value here", it could be a bit unclear since it may mean that some value was here before, while it just means that we checked in the actual storage and there is no value.

Also, having a value at a key K in the cache means "we know the state of the value here". If it's not here that means that it's unknown.

🤔

Since we know that the empty Vec is not something that we store, would it make more sense to simply have:

#[derive] #[non_exhaustive] // should include this in case we decide to extend or change the behaviour here pub enum CacheValue { Data(Vec<u8>), ToDelete, }

Then you can match on the value as follows:

match cache_value { Data(bytes) if !bytes.is_empty() => borsh::from_slice(bytes), ToDetele => ... }

iFrostizz · 2024-06-10T11:56:26Z

x/programs/rust/wasmlanche-sdk/src/state.rs

+        match self.cache.borrow_mut().entry(key) {
+            Entry::Occupied(entry) => match entry.get() {
+                CachedData::Data(data) => from_slice::<V>(data)
+                    .map_err(|_| StateError::Deserialization)
+                    .map(Some),
+                CachedData::ToDelete | CachedData::Deleted => Ok(None),
+            },
+            Entry::Vacant(entry) => {
+                if let Some(bytes) = Self::get_from_host(key)? {
+                    let value = from_slice::<V>(&bytes)
+                        .map_err(|_| StateError::Deserialization)
+                        .map(Some);
+                    entry.insert(CachedData::Data(bytes));
+                    value
+                } else {
+                    entry.insert(CachedData::Deleted);
+                    Ok(None)
+                }
+            }


I'm not sure how to avoid the repetition here, in one branch the data is borrowed because it comes from the cache. On the other, it has been copied from host so it's owned.

It's working quite well though because we need an owned value to insert in the cache.

Why can't you follow the same pattern as before but use a match instead?

let val_bytes = match cache.get(&key) { Some(CacheValue::Data(val)) => val, Some(CacheValue::ToDelete | CacheValue::Deleted) => { return Ok(None), }, None => // same as the else from before }

Okay, this actually compiles

pub fn get<V>(self, key: K) -> Result<Option<V>, Error> where V: BorshDeserialize, { #[link(wasm_import_module = "state")] extern "C" { #[link_name = "get_bytes"] fn get_bytes(ptr: *const u8, len: usize) -> HostPtr; } let mut cache = self.cache.borrow_mut(); let cached_data = match cache.get(&key) { Some(cached_data) => cached_data, None => { let args_bytes = borsh::to_vec(&key).map_err(|_| StateError::Serialization)?; let ptr = unsafe { get_bytes(args_bytes.as_ptr(), args_bytes.len()) }; if ptr.is_null() { cache.insert(key, CachedData::Deleted); return Ok(None); } cache.entry(key).or_insert(CachedData::Data(ptr.into())) } }; match cached_data { CachedData::Data(data) => from_slice::<V>(data) .map_err(|_| StateError::Deserialization) .map(Some), CachedData::ToDelete | CachedData::Deleted => Ok(None), } }

x/programs/runtime/import_state.go

richardpringle

Okay, sorry for taking so long with this review.

I want to check with @dboehm-avalabs to see if he also things it would make sense before you launch into it, but what I would like you to do is centralize the API first. What I mean by that is a single

       #[link(wasm_import_module = "state")]
        extern "C" {
            #[link_name = "put_bytes"]
            fn put_bytes(ptr: *const u8, len: usize);
        }

That actually takes a something like Vec<(Vec<u8>, Vec<u8>)> as the serialized data. The first value in the tuple is the key, the second is the value. If the value is empty, that should signify a delete. I'm not 100% sure what the best way is to represent this in Go, maybe [][2][]byte or something along those lines?

You should be able to do that with only minor changes to the Rust (nothing to the actual cache).

Once we merge that code, we can come back and do the CachedData enum.

Off the top of my head, I think the cache would probably be HashMap<K, Option<Vec<u8>>>.

richardpringle · 2024-06-10T12:43:10Z

x/programs/rust/wasmlanche-sdk/src/state.rs

+#[derive(Debug, PartialEq)]
+pub enum CachedData {
+    Data(Vec<u8>),
+    Deleted,
+    ToDelete,
+}


🤔

Since we know that the empty Vec is not something that we store, would it make more sense to simply have:

#[derive] #[non_exhaustive] // should include this in case we decide to extend or change the behaviour here pub enum CacheValue { Data(Vec<u8>), ToDelete, }

Then you can match on the value as follows:

match cache_value { Data(bytes) if !bytes.is_empty() => borsh::from_slice(bytes), ToDetele => ... }

richardpringle · 2024-06-10T13:04:55Z

x/programs/rust/wasmlanche-sdk/src/state.rs

+        match self.cache.borrow_mut().entry(key) {
+            Entry::Occupied(entry) => match entry.get() {
+                CachedData::Data(data) => from_slice::<V>(data)
+                    .map_err(|_| StateError::Deserialization)
+                    .map(Some),
+                CachedData::ToDelete | CachedData::Deleted => Ok(None),
+            },
+            Entry::Vacant(entry) => {
+                if let Some(bytes) = Self::get_from_host(key)? {
+                    let value = from_slice::<V>(&bytes)
+                        .map_err(|_| StateError::Deserialization)
+                        .map(Some);
+                    entry.insert(CachedData::Data(bytes));
+                    value
+                } else {
+                    entry.insert(CachedData::Deleted);
+                    Ok(None)
+                }
+            }


Why can't you follow the same pattern as before but use a match instead?

let val_bytes = match cache.get(&key) { Some(CacheValue::Data(val)) => val, Some(CacheValue::ToDelete | CacheValue::Deleted) => { return Ok(None), }, None => // same as the else from before }

richardpringle · 2024-06-10T14:02:25Z

x/programs/runtime/import_state.go

+					}
+				}
+				return nil
+			})},


There should be a single API for delete and delete_many. It'll be easier to review the changes if you consolidate the API and fix the call signature on the Rust side before you start using it.

I would argue that it would probably even be better to have a single put operation such that put-ing an empty Vec/byte-slice is the same as delete.

I'm not 100% sure if that makes sense with how state should work though. I'm only assuming that storing an empty slice isn't valid.

Opened #1057 in order to address #1029 (comment)

richardpringle · 2024-06-11T16:26:34Z

x/programs/rust/sdk-macros/src/lib.rs

@@ -257,7 +257,7 @@ pub fn state_keys(_attr: TokenStream, item: TokenStream) -> TokenStream {

    // add default attributes
    item_enum.attrs.push(parse_quote! {
-         #[derive(Clone, Copy, Debug, Eq, PartialEq, Hash)]


You can take this change and land it in a separate PR if you want it in earlier/immediately.

richardpringle · 2024-06-11T19:36:26Z

x/programs/rust/wasmlanche-sdk/src/state.rs

+        match self.cache.borrow_mut().entry(key) {
+            Entry::Occupied(entry) => match entry.get() {
+                CachedData::Data(data) => from_slice::<V>(data)
+                    .map_err(|_| StateError::Deserialization)
+                    .map(Some),
+                CachedData::ToDelete | CachedData::Deleted => Ok(None),
+            },
+            Entry::Vacant(entry) => {
+                if let Some(bytes) = Self::get_from_host(key)? {
+                    let value = from_slice::<V>(&bytes)
+                        .map_err(|_| StateError::Deserialization)
+                        .map(Some);
+                    entry.insert(CachedData::Data(bytes));
+                    value
+                } else {
+                    entry.insert(CachedData::Deleted);
+                    Ok(None)
+                }
+            }


Okay, this actually compiles

pub fn get<V>(self, key: K) -> Result<Option<V>, Error> where V: BorshDeserialize, { #[link(wasm_import_module = "state")] extern "C" { #[link_name = "get_bytes"] fn get_bytes(ptr: *const u8, len: usize) -> HostPtr; } let mut cache = self.cache.borrow_mut(); let cached_data = match cache.get(&key) { Some(cached_data) => cached_data, None => { let args_bytes = borsh::to_vec(&key).map_err(|_| StateError::Serialization)?; let ptr = unsafe { get_bytes(args_bytes.as_ptr(), args_bytes.len()) }; if ptr.is_null() { cache.insert(key, CachedData::Deleted); return Ok(None); } cache.entry(key).or_insert(CachedData::Data(ptr.into())) } }; match cached_data { CachedData::Data(data) => from_slice::<V>(data) .map_err(|_| StateError::Deserialization) .map(Some), CachedData::ToDelete | CachedData::Deleted => Ok(None), } }

richardpringle · 2024-06-11T19:37:10Z

x/programs/rust/wasmlanche-sdk/src/state.rs

+                } else {
+                    entry.insert(CachedData::Deleted);
+                    Ok(None)
+                }


Same as the comment above, I don't think you need all the repetition. There's a lot of surface area for bugs here.

richardpringle · 2024-06-11T19:38:06Z

x/programs/rust/wasmlanche-sdk/src/state.rs

-    /// Returns an [Error] if the key cannot be serialized
-    /// or if the host fails to delete the key and the associated value
-    pub fn delete<T: BorshDeserialize>(self, key: K) -> Result<Option<T>, Error> {
+    fn get_from_host(key: K) -> Result<Option<Vec<u8>>, Error> {


I don't think you need this static method.

richardpringle · 2024-06-11T19:40:35Z

x/programs/rust/wasmlanche-sdk/src/state.rs

+                CachedData::Data(value) => puts.push(PutArgs { key, value }),
+                CachedData::ToDelete => deletes.push(key),


Yeah, this tells me that a single API here would be better. I can't remember where I said it (could have been in this review or elsewhere), but I think that putting an empty Vec<u8> isn't valid. So you can actually eliminate all invalid state (minimizing the runtime checks) by making empty Vec puts the equivalent of delete.

@dboehm-avalabs, thoughts?

richardpringle · 2024-06-11T19:42:52Z

x/programs/rust/wasmlanche-sdk/src/state.rs

+        if !puts.is_empty() {
+            let serialized_args = borsh::to_vec(&puts).expect("failed to serialize");
+            unsafe { put_many_bytes(serialized_args.as_ptr(), serialized_args.len()) };
+        }
+
+        if !deletes.is_empty() {
+            let serialized_args = borsh::to_vec(&deletes.as_slice()).expect("failed to serialize");
+            unsafe { delete_many_bytes(serialized_args.as_ptr(), serialized_args.len()) };
+        }


Haha, I just keep seeing good reason to centralize the API into a single call. If there's only one call, you can do a simple if cache.is_empty() { return } for Drop

iFrostizz · 2024-06-13T19:48:56Z

Closing in favor of #1059

iFrostizz requested review from dboehm-avalabs and richardpringle as code owners June 7, 2024 12:54

iFrostizz force-pushed the cache_delete branch from a9c521a to 97b1f06 Compare June 7, 2024 14:13

iFrostizz requested a review from patrick-ogrady as a code owner June 7, 2024 14:13

iFrostizz changed the base branch from call_add_program to main June 7, 2024 14:14

iFrostizz self-assigned this Jun 7, 2024

iFrostizz force-pushed the cache_delete branch from a28cfae to e2e1e32 Compare June 10, 2024 11:47

iFrostizz and others added 7 commits June 10, 2024 13:48

return program module from AddProgram

396af78

return program module from AddProgram (#1024)

95de5a3

* Add ability to add programs to the runtime and reuse this function in the program initialization flow

return program module from AddProgram

4e0d47c

implement cache deletes

aae3245

cleanup things and nits

ff9aef8

implement Debug

26fe015

avoid the null-pointer optimization

96e7f61

iFrostizz force-pushed the cache_delete branch from e2e1e32 to 96e7f61 Compare June 10, 2024 11:49

iFrostizz commented Jun 10, 2024

View reviewed changes

fix nits

efd3196

dboehm-avalabs reviewed Jun 10, 2024

View reviewed changes

x/programs/runtime/import_state.go Outdated Show resolved Hide resolved

use [][]byte

5e66938

iFrostizz requested a review from dboehm-avalabs June 10, 2024 14:22

circumvent borsh ZST check with a slice

ea6cd79

dboehm-avalabs approved these changes Jun 11, 2024

View reviewed changes

richardpringle requested changes Jun 11, 2024

View reviewed changes

This was referenced Jun 13, 2024

Remove host delete and put functions #1057

Merged

Implement cache deletes #1059

Merged

iFrostizz closed this Jun 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement cache deletes #1029

Implement cache deletes #1029

iFrostizz commented Jun 7, 2024 •

edited

Loading

iFrostizz Jun 10, 2024

richardpringle Jun 11, 2024

iFrostizz Jun 11, 2024

iFrostizz Jun 10, 2024

iFrostizz Jun 10, 2024

richardpringle Jun 10, 2024

iFrostizz Jun 10, 2024 •

edited

Loading

iFrostizz Jun 10, 2024

richardpringle Jun 10, 2024

richardpringle Jun 11, 2024

richardpringle left a comment

richardpringle Jun 10, 2024

richardpringle Jun 10, 2024

richardpringle Jun 10, 2024

richardpringle Jun 11, 2024

iFrostizz Jun 13, 2024

richardpringle Jun 11, 2024

richardpringle Jun 11, 2024

richardpringle Jun 11, 2024

richardpringle Jun 11, 2024

richardpringle Jun 11, 2024

richardpringle Jun 11, 2024

iFrostizz commented Jun 13, 2024

		CachedData::Data(value) => puts.push(PutArgs { key, value }),
		CachedData::ToDelete => deletes.push(key),

Implement cache deletes #1029

Implement cache deletes #1029

Conversation

iFrostizz commented Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iFrostizz Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardpringle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iFrostizz commented Jun 13, 2024

iFrostizz commented Jun 7, 2024 •

edited

Loading

iFrostizz Jun 10, 2024 •

edited

Loading