Update mtree_merge doc comments
#1260
Conversation
| @@ -158,8 +158,7 @@ pub(super) fn mtree_set(span: &mut SpanBuilder) -> Result<Option<CodeBlock>, Ass | |||
| /// follows: | |||
| /// - merged root, 4 elements | |||
| /// | |||
| /// This operation will fail if either of the input roots doesn't exist as Merkle tree in the | |||
| /// advice provider. | |||
| /// It is not expected for provided roots to be in the advice provider. | |||
There was a problem hiding this comment.
I'm not sure that a brief comment is sufficient here, although it is the way I can make it correct without additional explanations.
I also considering this comment, but IMO it is a bit bulky:
/// In case of working with Merkle trees, it is expected that the provided roots exist as Merkle
/// trees in the advice provider, but this is not checked. In case of working with Merkle Mountain
/// range it is not expected for provided roots to be in the advice provider.
There was a problem hiding this comment.
IMO it is better to have the bulky comment. Otherwise it takes more time to onboard people.
Question: Why is there a difference in behavior? Can't we align it? It would be best if both structures behaved the same.
There was a problem hiding this comment.
I agree, that was an initial idea of the issue, although it turns out to be quite complicated to make this function work the same for the both structures: #943 (comment)
There was a problem hiding this comment.
I think a more detailed comment is better - but also I don't think we should mention Merkle trees or Merkle mountain ranges here. The instruction behaves the same way in either case, and how downstream code choses to use it is not relevant here, IMO.
| /// Returns an error if a Merkle tree for either of the specified roots cannot be found in this | ||
| /// advice provider. | ||
| /// It is not checked whether a Merkle tree for either of the specified roots can be found in | ||
| /// this advice provider. | ||
| fn merge_roots(&mut self, lhs: Word, rhs: Word) -> Result<Word, ExecutionError>; |
There was a problem hiding this comment.
Working on this issue I noticed that for now we are using this function (and mtree_merge instruction respectively) only in the MMR collection. In that case may be it makes sense to rename internal functions to merge_nodes to avoid inconsistency in merging roots not being roots?
There was a problem hiding this comment.
I was thinking about it as well - but note sure if merge_nodes is better. Technically, what we are doing here is inserting a new tree into the Merkle store - so, insert_tree would be more appropriate, but that can also be confusing.
So, I'd probably keep it as merge_roots.
| @@ -158,8 +158,7 @@ pub(super) fn mtree_set(span: &mut SpanBuilder) -> Result<Option<CodeBlock>, Ass | |||
| /// follows: | |||
| /// - merged root, 4 elements | |||
| /// | |||
| /// This operation will fail if either of the input roots doesn't exist as Merkle tree in the | |||
| /// advice provider. | |||
| /// It is not expected for provided roots to be in the advice provider. | |||
There was a problem hiding this comment.
IMO it is better to have the bulky comment. Otherwise it takes more time to onboard people.
Question: Why is there a difference in behavior? Can't we align it? It would be best if both structures behaved the same.
Fumuran
force-pushed
the
andrey-update-merge-roots-docs
branch
from
8cce96e
to
ee81b80
Compare
| /// Returns an error if a Merkle tree for either of the specified roots cannot be found in this | ||
| /// advice provider. | ||
| /// It is not checked whether a Merkle tree for either of the specified roots can be found in | ||
| /// this advice provider. | ||
| fn merge_roots(&mut self, lhs: Word, rhs: Word) -> Result<Word, ExecutionError>; |
There was a problem hiding this comment.
I was thinking about it as well - but note sure if merge_nodes is better. Technically, what we are doing here is inserting a new tree into the Merkle store - so, insert_tree would be more appropriate, but that can also be confusing.
So, I'd probably keep it as merge_roots.
This small PR changes the doc comments related to
mtree_mergeinstruction.It removes any comments about returning an error if provided roots are not in the advice provider (since we don't return it).