[storage/mmr] handle case of proving an MMR is empty #1168
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
da747d2 to
7457abe
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR refines the MMR proof verification API to handle the degenerate case of an empty MMR by removing the need to explicitly provide the end element position and deriving it automatically.
- Adjusts API functions in Proof to compute end positions from the elements length
- Introduces a default instance for Proof and updates tests, benches, and related modules accordingly
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| storage/src/mmr/verification.rs | API changes in Proof including default implementation and end position derivation |
| storage/src/mmr/journaled.rs | Removed redundant last element position parameter |
| storage/src/mmr/hasher.rs | Updated testing calls to match the new API signature |
| storage/src/mmr/benches/prove_many_elements.rs | Adjusted benchmark calls removing outdated parameters |
| storage/src/adb/current.rs | Updated proof calls to use the new API |
| storage/src/adb/any.rs | Modified proof verification call to align with API changes |
Comments suppressed due to low confidence (2)
storage/src/mmr/verification.rs:131
- [nitpick] Consider adding an inline comment to explain how and why 'end_element_pos' is derived from the number of elements, which may help future maintainers understand this computation.
};
storage/src/mmr/verification.rs:80
- [nitpick] Consider adding documentation to the default implementation of Proof to clarify its purpose in handling empty proofs.
impl<D: Digest> Default for Proof<D> {
d2d06c2 to
a845b8b
Compare
| &self, | ||
| hasher: &mut H, | ||
| elements: E, | ||
| elements: &[E], |
There was a problem hiding this comment.
I think removing end_element_pos makes sense but prefer to keep the IntoIterator for E (makes it much more flexible for elements?)
There was a problem hiding this comment.
I had to remove IntoIterator since in this change we need to know the lenght of the range of elements. And it turned out we didn't rely on that increased flexibillty anywhere.
There was a problem hiding this comment.
(And actually the caller would otherwise need to know the length of the range if they were required to provide the end_offset, so IntoIterator is actually probably not providing any value here.)
storage/src/mmr/verification.rs
Outdated
| return false; | ||
| } | ||
| }; | ||
| let end_element_pos = if elements.len() == 1 { |
There was a problem hiding this comment.
I think we can just do leaf_num_to_pos(start_leaf + elements.len() as u64 - 1)? We already handle the case of empty above?
There was a problem hiding this comment.
Yeah we could, I was just optimizing away the overhead of leaf_num_to_pos for this "common" case. It's pretty fast operation but it is still log(n). WDYT?
|
Removing |
8ff037f to
66c7413
Compare
2a6b63c to
a85644a
Compare
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## main #1168 +/- ##
=======================================
Coverage 91.21% 91.22%
=======================================
Files 216 216
Lines 57964 57937 -27
=======================================
- Hits 52874 52854 -20
+ Misses 5090 5083 -7
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Uh oh!
There was an error while loading. Please reload this page.