build: add storage trait and FilledTree::serialize

[amosStarkware]

This change is 

[amosStarkware]

+reviewer:@dorimedini-starkware

Reviewable status: 0 of 7 files reviewed, all discussions resolved (waiting on @dorimedini-starkware)

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (0c13cad) to head (e423f49).

Additional details and impacted files

@@          Coverage Diff          @@
##            main     #26   +/-   ##
=====================================
  Coverage   0.00%   0.00%           
=====================================
  Files          4       4           
  Lines         34      34           
  Branches      34      34           
=====================================
  Misses        34      34           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[amosStarkware]

Reviewable status: 0 of 7 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware)

---

crates/committer/src/patricia_merkle_tree/current_skeleton_tree.rs line 4 at r1 (raw file):

use crate::hash::types::HashFunction;
use crate::patricia_merkle_tree::errors::SkeletonTreeError;

I notice I forgot to split SkeletonTreeError into CurrentSkeletonTreeError, UpdatedSkeletonTreeError

Code quote:

use crate::patricia_merkle_tree::errors::SkeletonTreeError;

[dorimedini-starkware]

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @amosStarkware)

---

crates/committer/src/patricia_merkle_tree/filled_tree.rs line 13 at r1 (raw file):

    /// Serializes the tree into storage. Returns hash set of keys of the serialized nodes,
    /// if successful.
    fn serialize(&self, storage: &dyn Storage) -> Result<HashSet<&[u8]>, FilledTreeError>;

prefer generic type (compile time) to dynamic type (runtime)

Suggestion:

pub(crate) trait FilledTree<L: LeafDataTrait, S: Storage> {
    /// Serializes the tree into storage. Returns hash set of keys of the serialized nodes,
    /// if successful.
    fn serialize(&self, storage: &S) -> Result<HashSet<&[u8]>, FilledTreeError>;

---

crates/committer/src/patricia_merkle_tree/filled_tree.rs line 14 at r1 (raw file):

    /// if successful.
    fn serialize(&self, storage: &dyn Storage) -> Result<HashSet<&[u8]>, FilledTreeError>;
}

@aner-starkware note this change

Code quote:

pub(crate) trait FilledTree<L: LeafDataTrait> {
    /// Serializes the tree into storage. Returns hash set of keys of the serialized nodes,
    /// if successful.
    fn serialize(&self, storage: &dyn Storage) -> Result<HashSet<&[u8]>, FilledTreeError>;
}

---

crates/committer/src/storage/storage_trait.rs line 13 at r1 (raw file):

    /// Deletes value from storage.
    fn delete(&mut self, key: &[u8]);
}

1. please add @Itay-Tsabary-Starkware as a reviewer for this
2. are you sure we want an Option type for storage values? do we want to differentiate between 0 and None?
3. wrap the type of a storage value in a named type (not &[u8])
4. ditto for storage key

Code quote:

pub(crate) trait Storage {
    /// Returns value from storage, if it exists.
    fn get(&self, key: &[u8]) -> Option<&[u8]>;
    /// Sets value in storage.
    fn set(&mut self, key: &[u8], value: &[u8]);
    /// Returns values from storage in same order of given keys. If key does not exist,
    /// value is None.
    fn mget(&self, keys: Vec<&[u8]>) -> Vec<Option<&[u8]>>;
    /// Sets values in storage.
    fn mset(&mut self, key: Vec<&[u8]>, value: Vec<&[u8]>);
    /// Deletes value from storage.
    fn delete(&mut self, key: &[u8]);
}

[amosStarkware]

+reviewer:@Itay-Tsabary-Starkware

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Itay-Tsabary-Starkware)

[dorimedini-starkware]

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @amosStarkware and @Itay-Tsabary-Starkware)

---

crates/committer/src/storage/storage_trait.rs line 7 at r2 (raw file):

#[allow(dead_code)]
pub(crate) struct StorageValue<'a>(&'a [u8]);

@nimrod-starkware are you defining something similar?

Code quote:

#[allow(dead_code)]
pub(crate) struct StorageKey<'a>(&'a [u8]);

#[allow(dead_code)]
pub(crate) struct StorageValue<'a>(&'a [u8]);

---

crates/committer/src/storage/storage_trait.rs line 7 at r2 (raw file):

#[allow(dead_code)]
pub(crate) struct StorageValue<'a>(&'a [u8]);

@amosStarkware any reason these types shouldn't own their internal data? (asking, not sure if this is possible with slices)

Suggestion:

#[allow(dead_code)]
pub(crate) struct StorageKey([u8]);

#[allow(dead_code)]
pub(crate) struct StorageValue([u8]);

---

crates/committer/src/storage/storage_trait.rs line 16 at r2 (raw file):

    /// Returns values from storage in same order of given keys. If key does not exist,
    /// value is None.
    fn mget(&self, keys: Vec<StorageKey<'_>>) -> Vec<Option<StorageValue<'_>>>;

this should be better than constructing vectors, right?
is this possible without unreasonable lifetime syntax?

Suggestion:

&[StorageKey<'_>]

[amosStarkware]

Reviewable status: 4 of 8 files reviewed, 5 unresolved discussions (waiting on @dorimedini-starkware and @Itay-Tsabary-Starkware)

---

crates/committer/src/patricia_merkle_tree/filled_tree.rs line 13 at r1 (raw file):

Previously, dorimedini-starkware wrote…

prefer generic type (compile time) to dynamic type (runtime)

Changed to impl Storage (type is determened at compile time)

---

crates/committer/src/storage/storage_trait.rs line 13 at r1 (raw file):

Previously, dorimedini-starkware wrote…

1. please add @Itay-Tsabary-Starkware as a reviewer for this
2. are you sure we want an Option type for storage values? do we want to differentiate between 0 and None?
3. wrap the type of a storage value in a named type (not &[u8])
4. ditto for storage key

1. Done
2. IMO yes - there should be a difference between non existent and 0.
3. • 4. • done.

---

crates/committer/src/storage/storage_trait.rs line 7 at r2 (raw file):

Previously, dorimedini-starkware wrote…

@amosStarkware any reason these types shouldn't own their internal data? (asking, not sure if this is possible with slices)

Just noticed that too, thanks. fixed

---

crates/committer/src/storage/storage_trait.rs line 16 at r2 (raw file):

Previously, dorimedini-starkware wrote…

this should be better than constructing vectors, right?
is this possible without unreasonable lifetime syntax?

Yes
I think it's better to return the actual value (and not the reference)

[dorimedini-starkware]

Reviewed 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @amosStarkware and @Itay-Tsabary-Starkware)

---

crates/committer/src/patricia_merkle_tree/current_skeleton_tree.rs line 18 at r3 (raw file):

pub(crate) trait CurrentSkeletonTree<L: LeafDataTrait, H: HashFunction, TH: TreeHashFunction<L, H>>
{
    fn compute_current_skeleton_tree(

no computation being done, just storage traversal

Suggestion:

construct_current_skeleton_tree

---

crates/committer/src/patricia_merkle_tree/current_skeleton_tree.rs line 23 at r3 (raw file):

        root_hash: HashOutput,
        tree_height: TreeHeight,
    ) -> CurrentSkeletonTreeResult<Box<Self>>;

why Box?

Suggestion:

CurrentSkeletonTreeResult<Self>

---

crates/committer/src/storage/storage_trait.rs line 11 at r3 (raw file):

pub(crate) trait Storage {
    /// Returns value from storage, if it exists.
    fn get(&self, key: &StorageKey) -> Option<Box<StorageValue>>;

ah...
if we have a choice between using struct StorageValue(Vec<u8>),
or defining StorageValue([u8]) and having to use Box,
then I prefer the Vector route...
better to allocate vectors than to unwrap boxes

Code quote:

Box<

---

crates/committer/src/storage/storage_trait.rs line 16 at r3 (raw file):

    /// Returns values from storage in same order of given keys. If key does not exist,
    /// value is None.
    fn mget(&self, keys: [&StorageKey]) -> [Option<Box<StorageValue>>];

subtle difference

Suggestion:

&[StorageKey]

[amosStarkware]

Reviewable status: 6 of 9 files reviewed, 4 unresolved discussions (waiting on @dorimedini-starkware and @Itay-Tsabary-Starkware)

---

crates/committer/src/patricia_merkle_tree/current_skeleton_tree.rs line 18 at r3 (raw file):

Previously, dorimedini-starkware wrote…

no computation being done, just storage traversal

It's computed in the same sense that compute_updated_skeleton_tree computes the updated skeleton tree. should I change both to construct?

---

crates/committer/src/patricia_merkle_tree/current_skeleton_tree.rs line 23 at r3 (raw file):

Previously, dorimedini-starkware wrote…

why Box?

its size isn't known at compile time, so I think it makes sense to allocate it on the heap. but maybe there's a better solution?

---

crates/committer/src/storage/storage_trait.rs line 11 at r3 (raw file):

Previously, dorimedini-starkware wrote…

ah...
if we have a choice between using struct StorageValue(Vec<u8>),
or defining StorageValue([u8]) and having to use Box,
then I prefer the Vector route...
better to allocate vectors than to unwrap boxes

Done.

---

crates/committer/src/storage/storage_trait.rs line 16 at r3 (raw file):

Previously, dorimedini-starkware wrote…

subtle difference

Done.

[dorimedini-starkware]

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Itay-Tsabary-Starkware)

---

crates/committer/src/patricia_merkle_tree/current_skeleton_tree.rs line 18 at r3 (raw file):

Previously, amosStarkware wrote…

It's computed in the same sense that compute_updated_skeleton_tree computes the updated skeleton tree. should I change both to construct?

no, current->updated includes computation (merge nodes, split notes etc).
constructing an instance of the current tree from storage requires no computation, just traversal.
unblocking anyway, it's not important