feat: implement tiered smt

[vlopes11]

closes #36

Tasks

Beta Give feedback

We're having trouble rendering your tasklist, it's most likely has one of the following formatting errors:

• indented tasks (i.e. nested tasklist; support for this is coming!)
• an empty task (i.e. - [ ] on a line by itself)
• any empty new lines (before/after the tasklist or in between tasks)
• duplicate issue/pr links
• a draft task exceeds 512 characters

Please check the tasklists documentation for more information. 🔗 Feedback Discussion

Thank you for participating in the Private Beta ❤️

1. use hash in domain for upper leaves
2. implement leaf insert
3. implement membership proof
4. implement non-membership proof
5. implement test coverage with descriptive interface

Tasks

Beta Give feedback

We're having trouble rendering your tasklist, it's most likely has one of the following formatting errors:

• indented tasks (i.e. nested tasklist; support for this is coming!)
• an empty task (i.e. - [ ] on a line by itself)
• any empty new lines (before/after the tasklist or in between tasks)
• duplicate issue/pr links
• a draft task exceeds 512 characters

Please check the tasklists documentation for more information. 🔗 Feedback Discussion

Thank you for participating in the Private Beta ❤️

1. feat: implement tiered smt remove #52
   +0
2. feat: add merkle path wrapper #53
   4 of 4
   +0
3. feat: add merkle node index #54
   5 of 6
   +0
4. define the best storage strategy for merkle trees

Tasks

Beta Give feedback

We're having trouble rendering your tasklist, it's most likely has one of the following formatting errors:

• indented tasks (i.e. nested tasklist; support for this is coming!)
• an empty task (i.e. - [ ] on a line by itself)
• any empty new lines (before/after the tasklist or in between tasks)
• duplicate issue/pr links
• a draft task exceeds 512 characters

Please check the tasklists documentation for more information. 🔗 Feedback Discussion

Thank you for participating in the Private Beta ❤️

1. use hash in domain for lower leaves

[vlopes11]

We have a couple of open questions such as:

• should storages always take self as immutable?
  I think it should because in-memory storages should be treated as exception as we will have more robust solutions in production that will be either async or mutex/lock based

• should we use as interface the serializable trait from winter-utils?
  it seems to me we should introduce a new Hashable trait that will require deterministic serialization to bytes, and the ability to use the type as input for a given hasher (i.e. Rpo256)

[hackaugusto]

Hi @vlopes11 , thanks for the PR. I did a first pass, and added a few questions.

[hackaugusto]

Why not use the ContentType::Empty here?

[vlopes11]

The idea is to leave the storage as dumb as possible. It will basically store anything the tree asks, without applying any logic or assumption. There is an equivalent method for the tree that will unwrap to empty, as it is part of the tree logic.

[bobbinth]

Thank you! I've left some comments inline - which may require some structural changes to the current implementation. A few general comments:

1. This needs to be a key-value map where the key is a word and the value is a word as well. The structure needs to be alined with what works for Miden assembly.
2. This struct has two uses: one as a data structure which can be used directly, but we also need to be able to expose it as another variant of AdviceSet enum. This will be needed to support tired SMTs in Miden assembly. We don't need to tackle the second part in this PR - but something to keep in mind.
3. I would integrate usage of merge_in_domain() in this PR.
4. For storage, I wonder if we can adapt the struct we use for simple SMT - but if it requires too many modifications, we can keep a separate implementation.

[bobbinth]

I wonder if having this BitsIterator is a bit of an overkill for our use case - at least in its current form. For example, we will never need to iterate over more than a single element. So, no need to track the current element index.

It might make sense to roll this into node index struct.

[vlopes11]

The idea was to keep the responsibility of the components separated and minimal, having the node index only providing a map for the tree, and the bits iterator with a state to keep track of the current bit.

Another point to consider is that bits iterator is an internal helper of the implementation that won't be exposed by the API, and the node index will be public. If we move the functionality from the bits iterator to the node index, we might end up increasing the complexity of the public API.

wdyt?

[hackaugusto]

I agree with @bobbinth here, the code would be simpler without the iterator. One suggestion to keep the separation is to use something like trait SMTIndex.

Edit: Just to clarify, what I agree with is that we don't need to iterate over the bits, and we can use the first word to do the indexing. That makes the code more direct and maybe more performant. Nothing against separation of concerns, it is nice to have.

Edit2: Now that I think of, it could just be a impl From<Word> for NodeIndex no?

[vlopes11]

We need to keep track of the depth as well. I'll describe in one example. Assume we have a tier depth of 3, and a node index of { depth: 6, index: 36 }.

The key for such case would be 100100. It means that we should, for the first iteration, traverse right, left, left, and for the next one, again, right, left, left.

If we From<Word>, we will take the first depth bits (that is 3), and for this case we will not be able to differentiate if we should construct a NodeIndex { depth: 3: index: 4 } or NodeIndex { depth: 6, index: 36 } because we lose the current depth iteration.

What we can do as alternative to the bits iterator is to remove it entirely and create a method traverse on NodeIndex that will take the first 3 bits of the provided word, return the shifted word, and mutate the current node index. Something like

fn traverse(&mut self, key: Word) -> Word

But then we will expose the need for the user to manage this Word, as opposed to having everything encapsulated in the bits iterator. It is a working alternative, if we want to get rid of the bits iterator.

[bobbinth]

Is the Content the right name here? It feels like this described a tree node - so, maybe should be TreeNode. Also, I have the same questions as @hackaugusto: could this be an enum?

A few comments on node types:

1. Since empty nodes at the same depth have the same value, should Empty node just track the depth? And then we could look up the actual value of an empty node at a given depth in some static array?
2. Technically, empty node is also an internal node - but I agree that it may make sense to track empty nodes separately from non-empty internal nodes.
3. We actually should have another node type: a leaf node at depth 64. This leaf node would contain a sorted list of all leaves which share a common 64-bit prefix for their keys.

Summing up, I think it may be possible to describe a node as:

pub enum TreeNode {
    Empty(u8),
    Internal(Word),
    Leaf(Entry),
    LeafList(u64, Vec<Entry>),
}

In the above, Entry could be a tuple with containing the key and the value which define a leaf.

[vlopes11]

The reason to not use an enum is to detach the need to parse the whole node from the need to peek only the type - something that we do extensively while inserting new keys.

With the current implementation, we just ask the storage for the type, and it will be able to parse a single byte to provide us the required information with the minimal possible overhead. On the other hand, if we use an enum, the storage will have to parse the whole node for every of these iterations, and in most of these cases we will end up not using the parsed information as we will just check if its internal node and traverse until empty/leaf.

wdyt?

[bobbinth]

Make sense! I don't have a good sense of what would work best here (the TreeNode enum I proposed above also has some issues). We do need to handle somehow the fact that different types will store different data (i.e., u8 vs. Word vs. Entry etc.).

[bobbinth]

I think a more correct name for this could be NodeIndex. Depth here should not exceed 64 and so we can use u8 for it.

Overall, it might make sense to combine this with BitsIterator.

[vlopes11]

NodeIndex is indeed better. Fixed!

As for the depth type, the only reason it is u32 here is to maintain consistency with the other trees of the library (SimpleSmt, MerkleTree, etc). Given they will be an enum (AdviceSet), it is desirable to have an uniform type definition for them as the interfaces for this enum will be shared across these types.

u8 sounds reasonable for all trees (simple/compact/merkle). Sounds good if I update to u8 there as well?

[bobbinth]

I'm not sure I fully understand how this would work. It seems like that we re-compute the root of the tree in self.insert_key() and this does not depend on the value being stored (since insert_key() does not take the value as the parameter). But the root of the tree should depend on the value as well (i.e., insert(k0, a) and insert(k0, b) should result in different tree roots.

Am I missing something?

[bobbinth]

Separately, we should probably add public accessor to return the root of the tree.

[vlopes11]

Indeed this is an open question. The point I need some clarification is that the key of a leaf is computed as hash(path + depth ⋅ 2^48, v), so it means it can't be an arbitrary argument of insert (i.e. the user cannot define his key, the state of the tree + the value will define the node in which the value will be stored, and the key will be constant regardless of the depth). Maybe I'm missing something? We could have a quick sync call to clarify this important point. cc @Al-Kindi-0

[bobbinth]

So, the rule for computing node hashes is as follows:

• Internal node: hash(left, right, domain = 0)
• Leaf at depth 16, 32, 48: hash(remaining_path, value, domain = depth)
• Leaf at depth 64: hash(remaining_path, values_hash, domain = 64), where values_hash is a sequential hash of all key-value pairs in the subtree defined by this node. The key-value pairs are (remaining_path, value) and are sorted by remaining_path.

[bobbinth]

it means it can't be an arbitrary argument of insert (i.e. the user cannot define his key, the state of the tree + the value will define the node in which the value will be stored, and the key will be constant regardless of the depth).

The user can define an arbitrary key, but where the value under that key is stored in the tree depends on what else is already stored in the tree.

[hackaugusto]

The user can define an arbitrary key

Question: At first I thought we would do hash(user_key) and use that value as the key for the Merkle tree. But from this comment, it seems you're saying we will use key. Is this correct? Wouldn't that make it trivial for collisions to happen? Or am I missing something? I misinterpreted the sentence above, it is hash(user_key).

Edit: Another question, why are we changing the domain, i.e. hash(remaining_path, value, domain = depth), instead of doing hash(remaining_path, value, depth) (as in, adding the depth as part of the data being hashed)

[bobbinth]

Question: At first I thought we would do hash(user_key) and use that value as the key for the Merkle tree. But from this comment, it seems you're saying we will use key. Is this correct? Wouldn't that make it trivial for collisions to happen? Or am I missing something?

The SMT does not by itself dictate the structure of keys and values - but how it is used in different places may impose this structure. For example, when the SMT is used for account storage, the key will be hashed first first, and then the result will be used as a key for the SMT. However, when we use SMT for account vault, we don't need to do this hashing as the structure of assets makes collisions unlikely.

Another question, why are we changing the domain, i.e. hash(remaining_path, value, domain = depth), instead of doing hash(remaining_path, value, depth) (as in, adding the depth as part of the data being hashed)

This is for efficiency reasons. hash(remaining_path, value, depth) would require 2 permutations of the hash function. By using domains we can do it in a single permutation but we do give up 2 bits of security. I don't think that it's a big deal though.

And in that case the sorting of the entries at depth 64 would be much more complicated, because it would need to sort the user's key, and they can be of arbitrary size. Am I missing something?

The key is always 4 elements and by the time we get to depth 64, one of these elements is already "accounted for". So, the remaining key is exactly 3 field elements. It will take some number of cycles to sort by these keys, but it shouldn't be too bad and also the likelihood that we'll and up with a lot of collisions at depth 64 is rather small.

[hackaugusto]

Thanks for the reply @bobbinth , I wish I could have done my updates a bit faster to save you a bit of time 😄

The SMT does not by itself dictate the structure of keys and values - but how it is used in different places may impose this structure [...] However, when we use SMT for account vault, we don't need to do this hashing as the structure of assets makes collisions unlikely.

Ah, I haven't thought about using an asset as key, makes sense to avoid re-hashing it, cool!

Just to clarify, AFAIU we do impose structure in the keys, namely that the key must be 4 Felts in length and it must be randomly distributed. Asset happens to fit that description, or am I missing something?

It would be interesting to represent that in the type system, with a trait, perhaps something like trait IntoDigest. Structs like asset which already fit the bill would just return self, other structs would hash themselves first.

This is for efficiency reasons. hash(remaining_path, value, depth) would require 2 permutations of the hash function.

Aha! clever, thanks for the explanation.

[bobbinth]

Just to clarify, AFAIU we do impose structure in the keys, namely that the key must be 4 Felts in length and it must be randomly distributed. Asset happens to fit that description, or am I missing something?

Yes, we do require the key to be exactly 4 field elements (a "word"). But we don't require the keys to be randomly distributed. It would be nice if they are (and in the rollup design we do ensure this) - but the tree should be able to handle elements which are not randomly distributed as well.

[hackaugusto]

After thinking about storage, my general impression is that this should be just an in-memory store, I left a comment with some thoughts on storage and a possible design.

[hackaugusto]

I went on a tangent, here is my thoughts on how to make an efficient version of this tree with storage.

• A batch has a size b, and a depth d.
• The code will do d look ups on the batch, and d - b look ups to the storage. These numbers seem a bit too high to me.

Here are some back of the envelope details:

• Field element are 64 bits (8 bytes)
• Word is 4 Felts (32 bytes). Since nodes are random (hash values), they can't be compressed and all 32 bytes have to be saved or recomputed
• A usual page size is 4 KiB (4096 bytes / 128 Words)
• 6 levels of the tree has 2 ** (6 + 1) -1 / 127 elements, so 6 levels can easily be represented by a page of memory.
• This SMT grows in depths of 2**4 / 16 levels
• The value of empty nodes is static, so only the non-empty nodes are required. Assuming the worst case, were nodes don't share any Node in a path, that is 16 words per entry.
• This means we can fit a minimum of 128/16 / 8 elements per page w/ their paths

Here are some notes about read/write patterns:

1. Given that the max tree depth is fixed at M=64, and tiers are set on T=16/2 ** 4
2. Because the nodes in the tree are hash values, and their position is defined by the hash, I'm going to assume they are evenly distributed (ignoring attack vectors), so there is an equal probability for all nodes on the same layer to be the parent of a leaf node.
3. The full tree has 2 ** M leaf nodes
4. A node at level l will have 2 ** (M-l) leaf nodes under it
5. Because on Merkle trees, a parent is updated if a child changes, for a node at layer l we have the probability of 2 ** (M-l) / 2 ** M or 2 ** -l this node is a parent of a leave, and will require an update too. (this relies on assumption 2)

node at layer l	probability of a node in this layer requiring an update too p(l)
0	1
1	0.5
2	0.25
3	0.125
4	0.0625
5	0.03125
6	0.015625
7	0.0078125

Using the table above I would say that writing down the first 3 layers is a bottle neck we can get rid off (IOW, these change so often, that it is better to not write them down to disk). One idea is to use one page of memory to represent the 6 first layers, instead of writing these to storage on every update, we can write them down on shutdowns as a cache or just recompute it. Then save the complete path as mentioned above, and then we have only 0.008% of write amplification on the case without a DoS attack.

Putting the above together, my suggestion for a version with storage would be:

• Use one page of memory for the first 6 levels of the tree
• Because of the above, the first tier is special, and only 10 words are stored in storage, allowing 12 tier elements to be stored per page
• For elements on tiers 2, 3, and 4, this goes down to 8 elements per page
• On writes, we find the node tier, then the corresponding pages to be update. With high probability we have to do only 1 to 3 reads / writes per tree insert, instead of d - b reads + d writes. (I haven't thought about how to find the right page, but should be easily doable with metadata)

[vlopes11]

That is very good to keep in mind when we implement a storage that is more robust. The current solution is aiming for a minimal complexity, in-memory storage.

We can proceed with the discussion here:
#30

[Al-Kindi-0]

Thank you for a great discussion! Here is my perspective based on what I gathered so far:
The way I look at the tiered-smt is as a bunch of nodes that can be localized by two quantities: a depth and an index.
For example, in the following diagram for a 2-tiers tiered-smt, the red (internal) node has coordinates (depth = 2, index = 2) while the blue (leaf) node has coordinates (depth = 4, index = 2).

This links to our setting as follows:
The blue node has key 0b0010 which decomposes into two parts, 0b00 (the parent of the blue node in the first tier) and 0b10 which is equal to 2 and is at level 4.
Thus it makes sense to store Nodes in storage as (depth, index) --> Node.

As for the operations on the tree/storage and how they fit into the above, I will give a small example of an update as it illustrates the point without getting into the complications of an insertion.
Consider the following 3 tiers tiered-smt of max depth 6:

Suppose we want to update the value associated with key 0b111110. Then we have to get the node associated to this key from the storage and we can start by looking up the following pair (depth = 2, index = 0b11) which should return an internal node. This implies that our leaf node is in a deeper tier. Next we look up (depth = 4, index = 0b1111) and we find once more an internal node. Finally, we look up (depth = 6, index = 0b111110) and we get a leaf node and we can update this key, in storage, with the new value. This will lead to a new node value in the tiered-smt and hence we need to update the tree by walking up to the root.
To do this, let parent_key = 0b111110 >> 1 and parent_depth = 6 - 1 then we look up in the storage the tuple (parent_key, parent_depth). In our example, we will get back an internal node which we can use to fetch the sibling and use that, together with the removed bit, to get a new value for the (internal) parent node.
Our current node now is the parent node we just computed and we can repeat what we just did. Note that, in our example, the (new) parent node has a left child this is the root of an empty sub-tree. Using the fact that we know its depth (i.e. 5) we can statically fetch its value and use it, together with the next removed bit, to compute the (new) hash of the (new) parent.

It might be that I have missed some crucial implementation details that make the above impractical or even totally unsound but I think that we can leverage the fact that leaf nodes can appear only at pre-defined levels (i.e. tiers) to make the implementation simpler/more performant.

[bobbinth]

An alternative approach to storage is to maintain a map of leaves such that we map a key to the value and depth. Specifically, something like key -> (value, depth) or in Rust:

struct Store {
    branches: BTreeMap<(u64, u8), BranchNode>,
    leaves: BTreeMap<Word, (Word, u8)>,
}

Then, if we need to look up value for a given key, we can do it directly (no need to try at several levels). And from that point on, depending on the operation we want to perform we can take different course of action.

[bobbinth]

Another thing to keep in mind: it may be beneficial to keep the top tier of the tree (i.e., up to depth 16) in memory as a fully materialized binary Merkle tree. This can be represented as a single vector which would require just 4MB of RAM to store.

[Al-Kindi-0]

An alternative approach to storage is to maintain a map of leaves such that we map a key to the value and depth. Specifically, something like key -> (value, depth) or in Rust:

struct Store {
    branches: BTreeMap<(u64, u8), BranchNode>,
    leaves: BTreeMap<Word, (Word, u8)>,
}

Then, if we need to look up value for a given key, we can do it directly (no need to try at several levels). And from that point on, depending on the operation we want to perform we can take different course of action.

Yes, this is a much better solution!

[Al-Kindi-0]

Another thing to keep in mind: it may be beneficial to keep the top tier of the tree (i.e., up to depth 16) in memory as a fully materialized binary Merkle tree. This can be represented as a single vector which would require just 4MB of RAM to store.

Makes sense, and together with a static array of empty hashes at different levels we can save also on the amount of hashing.

[vlopes11]

This last PR fixed how the keys are supposed to work. It should now reflect the correct implementation, but the implementation is somewhat naive.

I also added a couple of tests to assert the correctness of the implementation. Provided everything is ok with these tests, we can start to tweak the implementation to a more optimal state. We have a couple of points that definitely can be optimized.

[hackaugusto]

Another thing to keep in mind: it may be beneficial to keep the top tier of the tree (i.e., up to depth 16) in memory as a fully materialized binary Merkle tree. This can be represented as a single vector which would require just 4MB of RAM to store.

I think we should store less than 16 levels. Here are some occupancy numbers (biased to better occupancy, only first tier):

elements	non-empty nodes	occupancy
2	33.0	00.02%
4	63.0	00.04%
8	119.0	00.09%
16	223.0	00.17%
32	415.0	00.31%
64	767.0	00.58%
128	1407.0	01.07%
256	2559.0	01.95%
512	4607.0	03.51%
1024	8191.0	06.24%
2048	14335.0	10.93%
4096	24575.0	18.74%
8192	40959.0	31.24%
16384	65535.0	49.99%
32768	98303.0	74.99%
65536	131071.0	100%

I computed the rates above with:

>>> def n(e):
...   l = math.log2(e)
...   assert(l.is_integer())
...   return (16 - l) * e + (2 ** (l + 1) - 1)
...
>>> [(2 ** i, n(2 ** i), n(2 ** i) / (2 ** 17 - 1)) for i in range(1,17)]

4MB would quickly add up and make clients very memory heavy for very low usage.

[bobbinth]

4MB would quickly add up and make clients very memory heavy for very low usage.

I'm not sure I follow. For a single account we store only 2 SMTs: account storage and account vault. So, we would need at most 8 MB of RAM for these trees. An assumption I'm making is that most people probably won't maintain more than a dozen accounts, and thus, even if all accounts are loaded into memory, RAM requirements would be under 100 MB.

I'm not saying we should always materialize top 16 levels in memory - but I'm not sure memory pressure is a concern here.

[hackaugusto]

I'm not saying we should always materialize top 16 levels in memory - but I'm not sure memory pressure is a concern here.

My understanding is that we are also going to use this data structure to implement clients for the network. These clients will need to have instances of these trees for the DBs (account/nullifier/notes), to operate the transaction pool (validation/priotization/execution will need to parse the data), to execute network transactions (which may be done in parallel), and to operate the RPC endpoints (which probably will need separate copies and older versions of these trees to compute the results). I assume this structure will be used often enough that the 4MB will add up, probably not to become a deal breaker, but enough that we shouldn't use it if the occupancy is low, or prior to bench marking to check the benefits/costs.

[bobbinth]

My understanding is that we are also going to use this data structure to implement clients for the network. These clients will need to have instances of these trees for the DBs (account/nullifier/notes), to operate the transaction pool (validation/priotization/execution will need to parse the data), to execute network transactions (which may be done in parallel), and to operate the RPC endpoints (which probably will need separate copies and older versions of these trees to compute the results). I assume this structure will be used often enough that the 4MB will add up, probably not to become a deal breaker, but enough that we shouldn't use it if the occupancy is low, or prior to bench marking to check the benefits/costs.

I think there is probably a bigger discussion here about different participants in the network and their roles (and also aligning on the terminology). A "client" in my mind is software ran by the end-user to interact with the network (in some ways, it is analogous to "light client"). There are also "nodes" which maintain full blockchain state and can serve blockchain info to clients. There are also "operators" which are nodes which extend the blockchain by creating blocks. These operators are also the ones who execute network transactions.

This is just a preliminary list, and we might add some other roles (i.e., nodes which create transaction batches).

[bobbinth]

Thank you! This is not a full review, but I left a few comments inline.

Overall, a few things seem to be more complicated than what we currently need. Also, I don't think we take advantage of the fact that we know that leaves can be located only at 4 different depths (16, 32, 48, 64). I think taking this into account could simplify things a bit (i.e., we could probably get rid of TreeUpdateContext and BitsIterator).

[bobbinth]

I think this is more complicated than it needs to be: leaves can be only at depth 16, 32, 48, and 64. So, we don't really need to traverse the index. We can just check if there is a leaf at depth 16 for the corresponding prefix. If not, we can check depth 32 etc.

Depth 64 is a special case - if we got here and the leaf is not found, then we know we need to deal with a vector rather than a single value.

I'm wondering if the logic for figuring out how to retrieve a leaf node should be delegate to the storage. Maybe it makes sense to have something like storage.get_leaf_node(key) and from here, we'd just call this method.

[vlopes11]

The storage backend was heavily simplified so this function is also more straightforward now

[bobbinth]

A couple of comments here:

1. I would probably get rid of RefCell (at least for now).
2. Storing leaves in vectors might be too big of an overhead. In vast majority of cases only a single leaf will be stored at a given index. Since we know leaves can be stored only at 4 different depth, it might make sense to split leaves into 2 (or more) separate maps.
3. I'm not sure types is needed here. This information can be inferred. I think for a DB-backed implementation storing type info in memory may make sense, but for memory-backed implementation, I'm not sure.

[vlopes11]

1. Done! However, I think it's important we keep the function as fallible so we design the advice interfaces accordingly. wdyt?
2. That's true. We do have a minimal overhead if we use vectors as [foo].to_vec() for single elements as this will exist only in the stack, but then it's still an overhead. An alternative is to have an enum, but then adding an extra byte to all leaves based on an exception (that might not ever happen btw) doesn't seem right. I'm updating the code to have two maps: tier_leaves: BTreeMap<NodeIndex, Leaf>, ordered_lists: BTreeMap<NodeIndex, Vec<Leaf>>
3. For memory backed implementation there is indeed no strong reason to have a dedicated map for the types. I think we can benefit to keep all interfaces uniform, but we can treat that as implementation detail of the storage.

[vlopes11]

wdyt of the interfaces like this?

pub fn append_leaf_to_list(&mut self, leaf: Leaf, index: u64) -> Result<(), StorageError>

pub fn get_list_leaf_value(&self, key: &Word, index: u64) -> Result<Option<Word>, StorageError>

pub fn get_node(&self, index: &NodeIndex) -> Result<Option<Word>, StorageError>

pub fn get_tier_leaf_value(&self, key: &Word) -> Result<Option<(NodeIndex, Word)>, StorageError>

pub fn get_type(&self, index: &NodeIndex) -> Result<NodeType, StorageError>

pub fn get_leaf_value_from_list(&self, key: &Word, index: u64) -> Result<Option<Word>, StorageError>

pub fn get_leaves_list(&self, index: u64) -> Result<Option<Vec<Leaf>>, StorageError>

pub fn replace_node_index(&mut self, node: Word, index: NodeIndex) -> Result<(), StorageError>

pub fn replace_tier_leaf_index(&mut self, leaf: Leaf, index: NodeIndex) -> Result<(), StorageError>

[bobbinth]

I've thought of this for a while, and at this point I'm not sure what's the right interface. I could see quite a few approaches and they all require different types of interface (e.g., page-based approached described in #45 (comment) or materializing the first tier in memory as mentioned in #45 (comment)). There is also a question of how to ensure that updates are atomic in case of DB-backed storage, and probably a few other things I haven't thought of.

This makes me think that maybe we are trying to define a general interface prematurely. So, maybe for now we don't try to generalize. Let's assume that this is an in-memory variant of the Tiered SMT and implement accordingly (and keep it as simple as possible. Once we have this working (and integrated with Miden assembly implementation of SMT), we can thing about other storage options.

One potential way to describe the simplest in-memory storage is this:

pub struct Storage {
    nodes: BTreeMap<NodeIndex, Word>,
    upper_leaves: BTreeMap<NodeIndex, Leaf>,
    lower_leaves: BTreeMap<u64, Vec<Leaf>>,
}

I haven't spent too much time thinking about this specific approach though - so, there might be better ways.

[vlopes11]

A change in that direction was performed. I added a couple more auxiliary maps to keep all operations efficient, even if they are in-memory.

[bobbinth]

This could be a simple Rpo256::merge().

[vlopes11]

But wouldn't we couple the code to the internals of merge_in_domain if we assume they are the same (which they happen to be now)?

[bobbinth]

In my mind it is the opposite. Internal nodes are hashed without the domain - but here we are assuming that domain ZERO means without domain. This might change in the future (though probably won't). The basic specs are:

• Internal nodes are hashed without domain.
• Leaf nodes are hashed with domain such that the domain depends on depth.

[vlopes11]

Done! Seems we lack a function that will allow us to hash_elements in a domain (if that is possible). Left a TODO note

[bobbinth]

Where would we need to hash elements in a non-default domain?

[Al-Kindi-0]

Thank you @vlopes11 !
I am wondering if we should include tests which compare the abstract implementation of insertion as proposed in this PR against a more direct implementation of the algorithms we have but using only the following three components:

1. Index manipulations directly on key[3]. We can use a few helper functions in order not get too distracted by too many bit manipulations but we should try to limit the number of such functions.
2. 2-to-1 hashing with domain.
3. The storage.

For example, for the simplest case of insertion i.e. at depth 16, we can use something like:

let depth = 16_u8;
let mut current_node = Leaf::new(key.into(), value).hash(depth);
let mut current_key = key[3];

for n in (0..depth).rev() {
    let parent_key = current_key >> 1;
    let parent_node = storage
        .get_internal_node(parent_key, n)
        .unwrap_or_else(|_| storage.get_empty_node((n + 1) as usize));
    let (left, right) = if current_key & 1 == 1 {
        (parent_node.left, current_node)
    } else {
        (current_node, parent_node.right)
    };

    storage.insert_internal_node(parent_key, n, left, right);
    current_key = parent_key;
    current_node = rpo::merge(&[left, right]);
}
let expected_root = curr_node.into();

Then we can compare expected_root with root we get from the current PR method.

For other depths, we can use the same idea as above by thinking of internal nodes on the path to the inserted leaf as roots themselves i.e. we compute the internal node at depth 16 that is on the path to the inserted leaf using the code above and thereafter insert this computed internal node at depth 16 again using the same code above.

[bobbinth]

Thank you! This is not a full review yet - just a few things I noticed. Overall, I think it is moving in the right direction, but I wonder if we can simplify things even further.

I probably wouldn't make any structural changes before I do a full review (will try to do it by EOD Thursday my time) - but addressing the following items could be helpful:

• Updating tests to use build_path() or something like that.
• Changing get_path() to work like get_node_path() right now.

[bobbinth]

Question: is there a way to build this array dynamically (but still at compile time) rather than hard-coding values?

[vlopes11]

That would probably require a crate split as the build script would have to depend on a portion of miden-crypto, generating a cyclic dependency.

It might be a good thing to split more complex data structures from crypto (even if inside a worskpace) as we might face this situation for other cases as well.

[bobbinth]

I was thinking more along the route of making Rpo256::merge() be a const fn and then we can just compute all roots bottom up. But I'm not sure if having it as const fn is possible.

[vlopes11]

We could face some issues as some operands of Felt are implemented as trait functions (besides Hasher::merge that we can easily circumvent by implementing an additional const fn inside this crate).

This would probably mean we have to refactor Felt as const fn in traits are not expected to be supported by the language anytime soon:

https://github.com/rust-lang/rfcs/blob/5c8d632e2a473764c7123524e6df17f97d7b3a07/text/0911-const-fn.md?plain=1#L83-L85

[bobbinth]

Where would we need to hash elements in a non-default domain?

[bobbinth]

nit: I find the name of reverse() a bit unintuitive. We are not really reversing the index but moving up the tree, right? Maybe move_up() or something like that would be a better name?

[vlopes11]

I was trying to keep consistency with the opposite method, that is traverse. What about backtrack?

[bobbinth]

I would probably change this and make get_path() to be basically get_node_path() but exposed publicly. We may also be able to get away with just this method instead of having prove_membership() and prove_non_membership() - but I'll need to think about this a bit more.

[bobbinth]

This is related to @Al-Kindi-0's comment here. I wonder if we should replace this with a simple build_path() or something like that which would have the following signature:

pub fn build_path(&self, index: u64, depth: u8) -> MerklePath

And then this method would internally rely only on the storage of the Merkle tree to build the path (I think the only method that we'll need from there is get_node()).

[vlopes11]

I usually avoid using the structure I'm testing to craft the expected output of the tests as it might create false positives.

One alternative to that would be having an oversimplified storage inside the test engine, and clash both implementations to assert correctness. I would prefer that over using the original tree to test itself. Wdyt?

cc @Al-Kindi-0 ; to keep the discussion in one place, better we continue here.

[bobbinth]

I think we could have storage-specific tests in storage, and then here we can assume that the storage works correctly. Otherwise, we'd pretty much be duplicating the implementation of storage here, no?

[Al-Kindi-0]

@vlopes11 I am thinking more of something like going through an algorithm using pen and paper using a small example. It is true that we are in essence doing the same thing but the more primitive approach has other benefits e.g. testing the logic or documenting it.
The pen and paper in our case will be the potentially duplicated code but simplified through the specificity of the example we are going through.
As @bobbinth mentioned above, the duplication shouldn't be everywhere as we can assume some parts to be correct e.g. storage.

[vlopes11]

@bobbinth
#45 (comment)

One example is here. Right now it's diverging a bit from the specs for the sake of hashing everything in a single call, but we probably need a hash_elements_in_domain function

crypto/src/merkle/tiered_smt/mod.rs

Lines 290 to 303 in 2158ea3

	/// Hash a list of ordered leaves, producing a resulting digest.
	fn hash_ordered_leaves<I, L>(leaves: I) -> RpoDigest
	where
	L: Borrow<Leaf>,
	I: IntoIterator<Item = L>,
	{
	let inputs = leaves
	.into_iter()
	.map(|leaf| (Word::from(leaf.borrow().key), leaf.borrow().value))
	.flat_map(|(key, value)| key.into_iter().chain(value.into_iter()))
	.collect::<Vec<Felt>>();
	// TODO need a hash elements in domain so we can hash the batch of leaves.
	Rpo256::hash_elements(&inputs)
	}

[bobbinth]

Rather than scattering my comments throughout, I've decided to put them all into a single comment.

First, I think NodeIndex should be moved to the parent module (to be at the same level as MerklePath). We'll probably end up using it for other Merkle structs as well.

Second, I'd probably get rid of StorageError and just use MerkleError instead as I think StorageError could be one of the variants of MerkleError in the future.

Third, I've spent some time thinking about the public interface of TieredSmt struct, especially in the context of what we'll need for advice provider integration. I didn't get all the way there, but here is what I came up with:

impl TieredSmt {

    // PUBLIC ACCESSORS
    // --------------------------------------------------------------------------------------------

    pub fn root(&self) -> &Word {
        ...
    }

    pub fn get_node(&self, index: &NodeIndex) -> Result<Word, MerkleError> {
        ...
    }

    pub fn get_node_path(&self, index: &NodeIndex) -> Result<MerklePath, MerkleError> {
        ...
    }

    // Returns the index of the leaf with the specified key. If the leaf is not in the tree, returns the
    // index at which it could have been located. Specifically, this could result in indexes of the
    // following:
    // - An empty node.
    // - Another leaf which shares a prefix with the specified key.
    pub fn get_leaf_index(&self, key: &Word) -> Result<NodeIndex, MerkleError> {
        ...
    }

    // This basically calls `get_leaf_index()` and then uses the result to call `get_node_path()` and
    // build a `LeafProof` (described later on).
    pub fn get_leaf_proof(&self, key: &Word) -> Result<LeafProof, MerkleError> {
        ...
    }

    pub fn get_leaf_value(&self, key: &Word) -> Result<Option<Word>, MerkleError> {
        ...
    }

    // Returns a list of leaves at the 64th level specified by the index, or None if the corresponding
    // leaf is empty.
    pub fn get_bottom_leaves(&self, index: u64) -> Result<Option<&[Leaf]>, MerkleError> {
        ...
    }

    // STATE MUTATORS
    // --------------------------------------------------------------------------------------------

    pub fn insert(&mut self, key: Word, value: Word) -> Result<&Word, MerkleError> {
        ...
    }
}

Note that for get_leaf_index() and get_leaf_proof() we could have passed in a 64-bit index instead of Word (since we only need to take the 4th element of the word), but we pass the full key for consistency.

The definition of LeafProof could look something like this:

pub struct LeafProof {
    path: MerklePath,
    bottom_leaves: Vec<Leaf>,
}

And it would expose a verify() method with the following signature:

pub fn verify(&self, root: &Word, key: &Word, value: &Word) -> Result<bool, MerkleError>

This method would return true for membership proof, false for non-membership proof, and an error if the key is inconsistent with the proof.

The above interface does not cover:

• Deletion of leaves.
• Updates of leaves through the advice provider interface.

[vlopes11]

@bobbinth looks good! The changes are performed with a couple of differences:

• get_bottom_leaves will return an owned value as we would otherwise couple the storage API with the consumers of the tree via lifetime bounds

• LeafProof will have to contain an enum between empty, lower(Vec<Leaf>), and upper(Leaf`) options.

While it's trivial to check membership proofs, a non-membership have a couple of different cases

• The branch input is an empty constant. No special treatment required.
• The branch input is a non-empty value. In this case, we need to verify that the value opens to a given leaf; otherwise, it could be just a node that belongs to the path of the candidate. we need to provide the leaf (key-value pair) to prove the input commitment opens to that leaf (i.e. we know a tuple that is the pre-image of the input commitment for a given depth).
• The branch input is a non-empty, low tier value. In this case, we just assert the candidate doesn't belong to the bottom leaves set, and compute the hash of the bottom leaves.

For the deletion of leaves (probably not in this PR), I think we should go for something as simple as

pub fn remove(&mut self, key: &word) -> Result<&Word, StorageError>

[vlopes11]

@Al-Kindi-0 @bobbinth I updated the test engine to have a

pub fn expect_path_with_depth(&mut self, key: Word, depth: u8)

The implementation is as dumb as possible (i.e. will not attempt to cover any edge case), and it will fetch the node values directly from the storage. The resulting path should match.

[bobbinth]

• get_bottom_leaves will return an owned value as we would otherwise couple the storage API with the consumers of the tree via lifetime bounds

I think we can assume that bottom leaves will be always loaded into memory - there will be very few of them, and I don't imagine loading them into memory will every be a problem. So, I think returning &[Leaf] rather than Vec<Leaf> shouldn't be a problem - but returning Vec is also not a big deal (again, because this will probably almost never happen in practice).

While it's trivial to check membership proofs, a non-membership have a couple of different cases

Ah yes! Though I think the struct could remain pretty simple still. I'll write up a separate comment on this.

[bobbinth]

Looks good! Thank you! I think the interfaces are basically what we need for this PR. But I'd like to simplify the implementation quite a bit. I left some suggestions on what can be simplified inline.

[bobbinth]

Let's get rid of StorageError and use MerkleError instead (here and in other places). Also, I'd probably not use generic type for key. So, basically, the signature could be:

pub fn get_leaf_value(&self, key: &Word) -> Result<Option<Word>, MerkleError>

[vlopes11]

Done!

[bobbinth]

I would wrap EMPTY_SUBTREES into a struct so that we could just do something like get_word(depth) on it.

[vlopes11]

Done!

[bobbinth]

Should this not be unreachable?

[vlopes11]

More or less yes.

It should, if the provided index is consistent (i.e. its depth isn't greater than the maximum depth).

I'm adding a check that will return a MerkleError::InvalidDepth if that is the case

[bobbinth]

I would simplify this to look something like this:

pub fn get_leaf_index(&self, key: &Word) -> Result<NodeIndex, MerkleError> {
    self.storage.get_leaf_index(key[3].as_int())
}

I would delegate most logic here to the storage as I think the index can be determined with just a few lookups (we don't actually need to traverse anything to get the index).

[vlopes11]

One of the possible inputs of this function is a key that doesn't exist in the storage. In that case, we need to traverse the path until an empty node, and return it (where that key would expect to be inserted). That way, this can't be a simple fetch operation as tree logic (traverse between tiers) will necessarily be involved.

I would avoid as much as possible to couple any logic of the tiered tree to the storage as it would make our lives much harder in the future. Right now, we can just remove this storage and put anything else there, without the need to refactor anything, but if we couple the logic, it will not be so simple.

[bobbinth]

I would simplify this as:

pub fn get_bottom_leaves(&self, index: u64) -> Result<Option<&[Leaf]>, MerkleError> {
  Ok(self.storage.get_bottom_leaves(index))
}

As mentioned in my other comment, we can assume that bottom leaves are always in memory and remove all the complexity associated with making a copy of the vector.

[vlopes11]

Done with small diffs

• we are returning a Vec<Leaf> to not bind the lifetime of the storage to the output of the tree
• we fallback to an empty vec if the node is empty

[bobbinth]

I would simplify this to look something like:

pub struct Storage {
    nodes: BTreeMap<NodeIndex, Word>,
    upper_leaves: BTreeMap<NodeIndex, Leaf>,
    bottom_leaves: BTreeMap<u64, Vec<Leaf>>,
}

I know additional fields probably result in better performance, but I wouldn't worry about this for now as what we'll have now is definitely not the final implementation.

[vlopes11]

The proposed bottom_leaves is a solid improvement, but as for the others, it's not only a matter of performance but also simplicity.

If we, for example, want to know what is a node type, currently, we just self.storage.get_type. If we remove that mapping, then we need to if self.nodes.contains_key -> Node else if self.upper_leaves.contains_key || self.bottom_leaves.contains_key -> Leaf else Empty. This complexity will extend to the other operations as well, when it could be a simple get

I propose we merge the two ideas, picking the best of each. wdyt?

#[derive(Debug, Default)]
pub struct Storage {
    types: BTreeMap<NodeIndex, NodeType>,
    nodes: BTreeMap<NodeIndex, Word>,
    keys: BTreeMap<CanonicalWord, NodeIndex>,
    upper_leaves: BTreeMap<NodeIndex, Leaf>,
    bottom_leaves: BTreeMap<u64, Vec<Leaf>>,
}

[bobbinth]

My thinking was that get_node_type() method would be the only one impacted as we can put all the logic for determining the type there, and then everywhere else use this method. Or would this not work?

In my mind this simplifies things as there are fewer things we need to update during insertions/deletions, which were I expect the bulk of complexity is.

[vlopes11]

I'm not sure we are achieving simplification that way. Another example is a query to the storage on where a given key is. Currently, we storage.keys.get; if we remove that mapping, we will need to iterate all key/value pairs of the upper leaves, fallback to the bottom leaves and iterate their vectors, until we find a matching key. That or perform a full traversal, get a leaf, check if the key/value matches (could be a key/value pair with matching path but diverging key or value), and then return the index.

[bobbinth]

I would replace this with:

pub fn get_bottom_leaves(&self, index: u64) -> Option<&[Leaf]> {
    self.bottom_leaves.get(&index)
}

[bobbinth]

I would change the signature here to:

pub fn get_leaf_index(&self, key: u64) -> Result<NodeIndex, MerkleError>

I think we should be able to implement this method with several lookups.

[vlopes11]

Closed as stalled