[commonware-storage/mmr/mem] API extensions to support generic proof generation + forgetting tweaks

[roberto-bayardo]

• add ability to initialize an mmr from a vector of nodes
• add ability to request individual nodes
• made proof generation dependent only on a simple Mmr trait instead of a specific implementation
• clarified some of the pruning documentation
• fixed edge case bug in ensuring position validity for proof generation after pruning
• removed ability to forget to arbitrary point in history since it doesn't properly detect how proving is affected by it. will re-work forgetting in a subsequent PR (tracking issue: [commonware-storage/mmr] Improve forgetting behavior in mem::Mmr #459)

[patrick-ogrady]

Thanks for adding tests for this 🙏

[patrick-ogrady]

is there a non-verifiable MMR in verification?

[patrick-ogrady]

nit: I would call this verification::Storage

[roberto-bayardo]

Renamed "Storage"

[patrick-ogrady]

nit: I wonder if there is a clean way to put the in-memory MMR at mmr::Memory (mmr::mem::Mmr seems suboptimal)

[patrick-ogrady]

Was this a bug?

[roberto-bayardo]

not a bug technically in that nothing was broken, but it was just being more conservative than necessary.

[patrick-ogrady]

nit: you can do crate::mmr::{} to avoid these duplicate prefix imports

[roberto-bayardo]

done

[patrick-ogrady]

Cool to see this broken out!

[patrick-ogrady]

To avoid creating a new Hasher on each iteration, you can do:

diff --git a/storage/src/mmr/mem.rs b/storage/src/mmr/mem.rs
index 78e7c494..245b389d 100644
--- a/storage/src/mmr/mem.rs
+++ b/storage/src/mmr/mem.rs
@@ -38,7 +38,8 @@ impl<H: CHasher> Storage<H> for Mmr<H> {
     }
 
     fn get_node(&self, position: u64) -> Option<&H::Digest> {
-        self.nodes.get(self.pos_to_index(position))
+        self.nodes
+            .get(Self::pos_to_index(self.oldest_remembered_pos, position))
     }
 }
 
@@ -77,29 +78,31 @@ impl<H: CHasher> Mmr<H> {
     }
 
     /// Return the position of the element given its index in the current nodes vector.
-    fn index_to_pos(&self, index: usize) -> u64 {
-        index as u64 + self.oldest_remembered_pos
+    fn index_to_pos(oldest_remembered_pos: u64, index: usize) -> u64 {
+        oldest_remembered_pos + index as u64
     }
 
     /// Return the index of the element in the current nodes vector given its position in the MMR.
-    fn pos_to_index(&self, pos: u64) -> usize {
-        (pos - self.oldest_remembered_pos) as usize
+    fn pos_to_index(oldest_remembered_pos: u64, pos: u64) -> usize {
+        (pos - oldest_remembered_pos) as usize
     }
 
     /// Add an element to the MMR and return its position in the MMR.
     pub fn add(&mut self, element: &H::Digest) -> u64 {
         let peaks = nodes_needing_parents(self.peak_iterator());
-        let element_pos = self.index_to_pos(self.nodes.len());
+        let element_pos = Self::index_to_pos(self.oldest_remembered_pos, self.nodes.len());
 
         // Insert the element into the MMR as a leaf.
-        let mut hash = Hasher::new(&mut self.hasher).leaf_hash(element_pos, element);
+        let mut hasher = Hasher::new(&mut self.hasher);
+        let mut hash = hasher.leaf_hash(element_pos, element);
         self.nodes.push(hash.clone());
 
         // Compute the new parent nodes if any, and insert them into the MMR.
         for sibling_pos in peaks.into_iter().rev() {
-            let parent_pos = self.index_to_pos(self.nodes.len());
-            let sibling_hash = &self.nodes[self.pos_to_index(sibling_pos)];
-            hash = Hasher::new(&mut self.hasher).node_hash(parent_pos, sibling_hash, &hash);
+            let parent_pos = Self::index_to_pos(self.oldest_remembered_pos, self.nodes.len());
+            let sibling_hash =
+                &self.nodes[Self::pos_to_index(self.oldest_remembered_pos, sibling_pos)];
+            hash = hasher.node_hash(parent_pos, sibling_hash, &hash);
             self.nodes.push(hash.clone());
         }
         element_pos
@@ -158,7 +161,7 @@ impl<H: CHasher> Mmr<H> {
     }
 
     fn forget_to_pos(&mut self, pos: u64) {
-        let nodes_to_remove = self.pos_to_index(pos);
+        let nodes_to_remove = Self::pos_to_index(self.oldest_remembered_pos, pos);
         self.oldest_remembered_pos = pos;
         self.nodes = self.nodes[nodes_to_remove..self.nodes.len()].to_vec();
     }

[patrick-ogrady]

I'm not sure I "love" this per se (and think creating a new hasher at the start of any call is reasonable) but if you don't want to do that, this should be more efficient.

[roberto-bayardo]

Hmm, I think this dilutes the value of the "pos_to_index" abstraction. I suspect the compiler will optimize away the Hasher::new overhead so I think I prefer leaving it as is.

[patrick-ogrady]

I think you need to remove this?

[codecov]

Codecov Report

Attention: Patch coverage is 99.30556% with 1 line in your changes missing coverage. Please review.

Project coverage is 94.28%. Comparing base (3a65f7e) to head (e572183).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
storage/src/mmr/mem.rs	98.59%	1 Missing ⚠️

@@            Coverage Diff             @@
##             main     #456      +/-   ##
==========================================
+ Coverage   94.17%   94.28%   +0.10%     
==========================================
  Files          94       95       +1     
  Lines       27403    27750     +347     
==========================================
+ Hits        25808    26163     +355     
+ Misses       1595     1587       -8     

Files with missing lines	Coverage Δ
storage/src/mmr/verification.rs	94.78% <100.00%> (+0.74%)	⬆️
storage/src/mmr/mem.rs	94.58% <98.59%> (+0.13%)	⬆️

... and 29 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a65f7e...e572183. Read the comment docs.