-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CMT: Subtree Append #171
base: main
Are you sure you want to change the base?
CMT: Subtree Append #171
Conversation
} | ||
|
||
if self.rightmost_proof.index == 0 { | ||
// TODO(sorend): Consider if we want to be able to initialize a tree with a subtree append (I think it would be cool, but is a little complex). write an equivallent function for subtree_append: initialize_tree_from_subtree_append |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think 0 should always be a safe index to start from because the subtree topology doesn't interfere with the main tree topology (i.e. each node can be mapped cleanly to another node)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree, in general appending to an empty tree should be ok. I think the reason why I skipped it here for now was because there are multiple sizes of trees that are valid to append to an empty tree, and I was unsure if it would gel well with the logic for appending to an initialized tree.
But it might be as simple as making a different size check on the depth of tree being appended if the tree is empty. will review the logic here
change_list, | ||
self.rightmost_proof.index + subtree_rightmost_index - 1, | ||
); | ||
self.rightmost_proof.index = self.rightmost_proof.index + subtree_rightmost_index; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can potentially mess up the tree
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed due to the intersection check
hash_to_parent( | ||
&mut node, | ||
&subtree_rightmost_proof[i], | ||
((subtree_rightmost_index - 1) >> i) & 1 == 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic seems like the right idea!
let intersection = self.rightmost_proof.index.trailing_zeros() as usize; | ||
|
||
// @dev: At any given time (other than initialization), there is only one valid size of subtree that can be appended | ||
if subtree_rightmost_proof.len() != intersection { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jarry-xiao here we enforce that we only append a tree of the correct size given the nodes inserted into the tree so far
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah this is quite clever! Makes sense, because the intersection nodes maps to the root of the subtree where the newly appended node is the left most child
I think the logic here is looking sound after your explainer comment. The main thing that still seems somewhat uncertain to me is how to actually go about performing a tree migration. Operationally, it seems difficult to "wait" for the desired indices to become available for append. Maybe this can be handled purely from the controlling contract level where aggregated trees completely ban regular appends and only allow for subtrees of a certain size to be appended. Still, this is definitely a step in the right direction |
self.rightmost_proof.proof[i] = subtree_rightmost_proof[i]; | ||
} else if i == intersection { | ||
// Compute where the new node intersects the main tree | ||
hash_to_parent(&mut node, &intersection_node, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To check my understanding, at this point node == root(subtree)
. For extra precaution, can we assert that here?
This PR adds functionality to append a subtree to a CMT!
The high-level process roughly goes as follows (similar to what @jarry-xiao initially outlined):
Why we only need one changelog in order to accurately fast-forward proofs for subsequent
replace_leaf
calls.Why we don't allow initializing a CMT via subtree append (for now, worth for the future?):