Multiproof with tree trait #44
Conversation
Also, set the number of children reported by Node::Branch to 16 instead of the number of non-empty nodes.
This commit effectively makes the parameter in LEAF() obsolete, because it is assumed that the leaf only contains the label that it needs.
|
I was going through quilt/pm#2 recently and it seems you and Matt are thinking along the same lines (e.g. see "Authenticated multi-proof backend" in his first post, and the 3rd post). The difference being you're trying to generalize from the MPT and multiproof, he from a sparse merkle tree and SSZ like proofs.Maybe eventually it'll be possible to converge on the same API, that'll be great! you guys should talk :) |
There was a problem hiding this comment.
So I still think the proof format and the tree structure are tighly coupled, and it'll be difficult to decouple them, but it might well be possible and IMO this PR is a good start in that direction. Maybe there are still some unclear points, but I expect it'll improve iteratively.
The code's level is above my Rust experience and I can't tell how idiomatic it is. Specially around the Tree, KeyValueStore, Node and Multiproof traits and structs and their relationship, but to me it seems kinda logical. I just had a few minor comments, but I'm approving anyway. If you think they make sense also feel free to address them either here or in later PRs.
| type Value = Vec<u8>; | ||
| } | ||
|
|
||
| impl Tree<Node> for Node { |
There was a problem hiding this comment.
I imagine the naming around Tree and Node might be confusing for someone reading the code. I don't really have better suggestions, maybe Node and MPTNode, or Tree and MPT?
There was a problem hiding this comment.
I agree, at least for the Node part. I want to try binary trees and I will rename it MPTree at this stage. Don't want to make this PR longer than strictly necessary.
|
|
||
| fn from_hash(h: Vec<u8>) -> Self; | ||
| fn new_empty() -> Self; | ||
| fn new_extension(ext: Vec<u8>, child: Self) -> Self; |
There was a problem hiding this comment.
These new_* methods seem to be specific to the MPT tree. Is there maybe a way in Rust to only define them on Node and then cast a Tree to a Node when these methods are needed?
There was a problem hiding this comment.
I looked for a way to do that, and didn't find any. I guess this could be split into a simpler Tree trait that doesn't have any extension and a more complex TreeWithExtension : Tree that supports them. I need to give it more thoughts after trying to implement a binary tree with this. So far, I assumed that a binary tree would simply implement new_extension as a panic!
| NibbleKey::from(key[key.len() - *keylength..].to_vec()), | ||
| value.to_vec(), | ||
| )); | ||
| LEAF(keylength) => { |
There was a problem hiding this comment.
Ah LEAF's operand has been deprecated as in it's not used, but it still has an operand, I misread your comment initially. This makes it easy to remove the operand later thanks 👍
| stack.push(b) | ||
| } else { | ||
| return Err(format!( | ||
| "Could not pop a value from the stack, that is required for a BRANCH({})", |
There was a problem hiding this comment.
Hm, these lines don't seem to have proper indentation in github's diff explorer. But since CI passed cargo fmt... I assume it's alright.
There was a problem hiding this comment.
I agree it's weird, but that's the output of my cargo fmt.
|
This is really cool! I'm excited to see how you guys evolve this. I think it is good that we are both thinking in similar terms of building these abstractions, and hopefully we get to a point down the line where we converge 😄.
This is a tough thing to fully decouple all in one go, and I agree this appears to be a great step forward. IMO as these things mature, we'll continue to find ways to naturally decouple them! |
|
@lightclient @s1na yes, convergence is near! 😉 |
This PR prepares the ground for supporting several tree formats. This addresses #10 and supersedes #42 and #43
Treetrait that supports operations like insertion and iteration over the set of child nodes, then re-implements Node in terms of this trait.KeyandValuetypes, making is easier to switch between a nibble-/byte-/binary-based key type.Multiproofstructure is parameterized by the tree type, to be able to reconstruct different types of trees from a single proof type.rebuilda method ofMultiproof, which is the first step in supporting different proof types (the end goal is to support proofs that use instructions and proofs that do not)LEAF(i)operand i as expressed in Drop LEAF's operand #19