BaseFold: Extract Merkle tree inner into a new struct. #560
Conversation
yczhangsjtu
changed the title
There was a problem hiding this comment.
Please see comments.
Btw, you don't need to give the types everywhere, Rust can figure much of that out on its own. Here's an example of two places, but there are more:
diff --git a/mpcs/src/basefold/commit_phase.rs b/mpcs/src/basefold/commit_phase.rs
index 992745bb..5df3aa19 100644
--- a/mpcs/src/basefold/commit_phase.rs
+++ b/mpcs/src/basefold/commit_phase.rs
@@ -98,8 +98,7 @@ where
);
if i > 0 {
- let running_tree =
- MerkleTree::<E>::new(running_tree_inner, FieldType::Ext(running_oracle));
+ let running_tree = MerkleTree::new(running_tree_inner, FieldType::Ext(running_oracle));
trees.push(running_tree);
}
@@ -114,7 +113,7 @@ where
// Then the oracle will be used to fold to the next oracle in the next
// round. After that, this oracle is free to be moved to build the
// complete Merkle tree.
- running_tree_inner = MerkleTreeDigests::<E>::from_leaves_ext(&new_running_oracle);
+ running_tree_inner = MerkleTreeDigests::from_leaves_ext(&new_running_oracle);
let running_root = running_tree_inner.root();
write_digest_to_transcript(&running_root, transcript);
roots.push(running_root.clone());
diff --git a/mpcs/src/util/merkle_tree.rs b/mpcs/src/util/merkle_tree.rs
index 53568af8..81af42bd 100644
--- a/mpcs/src/util/merkle_tree.rs
+++ b/mpcs/src/util/merkle_tree.rs
@@ -83,9 +83,7 @@ where
.iter()
.take(self.height() - 1)
.enumerate()
- .map(|(index, layer)| {
- Digest::<E::BaseField>(layer[(leaf_group_index >> index) ^ 1].clone().0)
- })
+ .map(|(index, layer)| Digest(layer[(leaf_group_index >> index) ^ 1].clone().0))
.collect(),
)
}(Sometimes adding extra redundant type information helps readability, but here it just seems to add verbosity.)
| pub fn root_from_inner(inner: &[Vec<Digest<E::BaseField>>]) -> Digest<E::BaseField> { | ||
| inner.last().unwrap()[0].clone() | ||
| pub fn root_ref(&self) -> &Digest<E::BaseField> { | ||
| &self.inner.last().unwrap()[0] |
There was a problem hiding this comment.
It looks like we require that inner not be empty. I suggest we encode that in the type, instead of going with Vec which is happy being empty.
There was a problem hiding this comment.
Yes, it would be better to let the compiler help us enforce such constraints. But there are more to constrain than this, e.g., the size of the vectors should be 1, 2, 4, 8, and so on. I don't know how to encode all these in the type. Currently, it's just relying on inner being private, and is never changed once the Merkle tree is built, and all the unwrap() are concealed.
| where | ||
| E::BaseField: Serialize + DeserializeOwned, | ||
| { | ||
| pub fn compute_inner(leaves: &FieldType<E>) -> Vec<Vec<Digest<E::BaseField>>> { | ||
| merkelize::<E>(&[leaves]) | ||
| pub fn from_leaves(leaves: &FieldType<E>) -> Self { |
There was a problem hiding this comment.
Consider implementing From instances instead? (Not completely sure.)
There was a problem hiding this comment.
I'm not sure, either. Computing the Merkle tree is an expensive procedure, so we probably want it to be always invoked explicitly.
There was a problem hiding this comment.
Rust never invokes from implicitly.
There was a problem hiding this comment.
No, not in the language. I just have the impression (which may be wrong) that .into() gives people the hint that this is a cheap type conversion, invoking some wrapper functions, without any expensive stuff.
|
Could you please add in the PR description why we are doing this? What do we want to accomplish that gets easier or simpler with this refactoring? Oh, and is this supposed to be a pure refactoring, or does it change any behaviour? |
Extract small PR from #294
Currently the inner (without the leafs) of the Merkle tree is represented by a
Vec<Vec<Digest>>. Sometimes (for optimization reasons, e.g., when the leaf values should stay in the original variable and we don't want to clone it into the Merkle tree) this inner part needs to be accessed and stored separately from the leaves. So we encapsulate it in astruct MerkleTreeDigestsfor more readability, implementing common methods on them, and allow enforcing particular constraints over theVec<Vec<Digest>>.This refactor provides new APIs for
MerkleTreeDigestsand make the correspondingMerkleTreeAPIs a thin wrapper around the APIs forMerkleTreeDigests. There are some new APIs and the behavior of existing APIs stay the same.