[commonware-storage/mmr] less restrictive in-mem-mmr forgetting #484

roberto-bayardo · 2025-02-12T01:04:32Z

Addresses #459

patrick-ogrady · 2025-02-12T16:46:42Z

storage/src/mmr/iterator.rs

+    oldest_remembered_node_pos: u64,
+) -> u64 {
+    for (peak_pos, height) in peak_iterator {
+        if peak_pos >= oldest_remembered_node_pos {


nit: I like the idea of continuing on peak_pos < oldest_remembered_node_pos to reduce nesting

me too, changed.

patrick-ogrady · 2025-02-12T16:48:51Z

storage/src/mmr/iterator.rs

+            return oldest_remembered_node_pos;
+        }
+    }
+    0 // mmr is empty


nit: I think we should be more careful about this case? Couldn't you also hit here if each peak_pos is < oldest_remembered_node_pos? I think we should handle those differently.

In a valid MMR, the very last node is always a peak. So this wouldn't happen, unless the MMR is empty, or oldest_remembered_node_pos is specified incorrectly.

I think I'm referring to the latter case. It "shouldn't" happen but I think we should consider returning an error if it does.

I see. I have made it panic(), is that OK vs returning error? I think of this like an array out of bounds condition.

if it should never ever happen, I think assert/panic makes sense.

patrick-ogrady · 2025-02-12T16:49:05Z

storage/src/mmr/iterator.rs

+    0 // mmr is empty
+}
+
+/// Returns the position of the oldest node whose hash will be required to prove inclusion of


nit whose "digest'

patrick-ogrady · 2025-02-12T16:49:30Z

storage/src/mmr/iterator.rs

+/// Forgetting this position will render the node with position `provable_pos` unprovable.
+pub(crate) fn oldest_required_proof_pos(peak_iterator: PeakIterator, provable_pos: u64) -> u64 {
+    for (peak_pos, height) in peak_iterator {
+        if peak_pos >= provable_pos {


nit: again prefer continue on <

patrick-ogrady · 2025-02-12T16:50:07Z

storage/src/mmr/iterator.rs

+            return provable_pos;
+        }
+    }
+    0 // mmr is empty


Like above, want to be careful with this condition when peak_iterator is non-zero

responded above

patrick-ogrady · 2025-02-12T16:51:07Z

storage/src/mmr/mem.rs

+
+    // The hashes of the MMR's peaks that are older than oldest_remembered_pos, keyed by their
+    // position.
+    old_peaks: HashMap<u64, H::Digest>,


Ah, ok so I was on-track with my questions about separating peak tracking 👍

patrick-ogrady · 2025-02-12T16:56:40Z

storage/src/mmr/mem.rs

+    /// Forgotten nodes will no longer be provable, nor will some nodes that follow them in some
+    /// cases.  Use forget_max() to guarantee a desired node (and all that follow it) will remain
+    /// provable after forgetting.
+    pub fn forget_to_pos(&mut self, pos: u64) {


I wonder if we want this one to be public (could be called forget_unchecked). Seems better to corral users to either forget_all (which I think should be called forget_max) or forget_max (which I think should be called forget_to_pos).

I find forget_all to be more descriptive/literal than forget_max for the case where we forget everything. I think forget_max may just be too ambiguous. Still trying to think of a better name though...

Alright I've changed "forget_max" to just plain old "forget", left forget_to_pos named as is but reduced its visiblity to internal to the crate only. WDYT?

I think its definitely an improvement. 👍

Not sure how I feel about forget_all vs forget_max but that's your call. Definitely "good enough" now.

patrick-ogrady · 2025-02-12T16:58:35Z

storage/src/mmr/journaled.rs

@@ -85,7 +85,7 @@ impl<B: Blob, E: RStorage<B>, H: Hasher> Mmr<B, E, H> {
            let item = journal.read(i).await?;
            vec.push(item);
        }
-        let mem_mmr = MemMmr::init(vec, oldest_peak);
+        let mem_mmr = MemMmr::init(vec, oldest_peak, vec![]);


Oh hmm, not in this case, since we're bootstrapping with all nodes starting from the very oldest peak. But we probably don't want to do this, and instead bootstrap in a "forget_all" state.

oh how weird, seems I managed to edit your comment with my reply.

changed to bootstrap in the "forget_all" state (in which case there are old peaks).

patrick-ogrady · 2025-02-12T16:58:47Z

storage/src/mmr/journaled.rs

@@ -85,7 +85,7 @@ impl<B: Blob, E: RStorage<B>, H: Hasher> Mmr<B, E, H> {
            let item = journal.read(i).await?;
            vec.push(item);


Recommend calling vec something more descriptive.

have reworked this code to bootstrap in the "forget_all" state, with more descriptive names.

patrick-ogrady · 2025-02-12T18:48:10Z

storage/src/mmr/iterator.rs

+            if parent_pos == provable_pos {
+                // If we hit the node we are trying to prove while walking the path, then no
+                // older nodes are required to prove it.
+                println!("HELLO!!!!");


We should remove this

hehe, you think? Leftover from my coverage debugging...

patrick-ogrady · 2025-02-12T18:52:00Z

storage/src/mmr/iterator.rs

+            return oldest_remembered_node_pos;
+        }
+    }
+    0 // mmr is empty


I think I'm referring to the latter case. It "shouldn't" happen but I think we should consider returning an error if it does.

codecov · 2025-02-12T23:15:32Z

Codecov Report

Attention: Patch coverage is 98.24561% with 4 lines in your changes missing coverage. Please review.

Project coverage is 94.30%. Comparing base (ccf2f88) to head (7ea6d0b).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
storage/src/mmr/iterator.rs	96.55%	2 Missing ⚠️
storage/src/mmr/mem.rs	97.40%	2 Missing ⚠️

@@            Coverage Diff             @@
##             main     #484      +/-   ##
==========================================
+ Coverage   94.25%   94.30%   +0.05%     
==========================================
  Files          97       97              
  Lines       28213    28372     +159     
==========================================
+ Hits        26591    26756     +165     
+ Misses       1622     1616       -6

Files with missing lines	Coverage Δ
storage/src/mmr/journaled.rs	`99.06% <100.00%> (+0.02%)`	⬆️
storage/src/mmr/verification.rs	`95.40% <100.00%> (+0.64%)`	⬆️
storage/src/mmr/iterator.rs	`98.69% <96.55%> (-1.31%)`	⬇️
storage/src/mmr/mem.rs	`95.25% <97.40%> (+1.72%)`	⬆️

... and 2 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 ccf2f88...7ea6d0b. Read the comment docs.

roberto-bayardo force-pushed the improve-mmr-forgetting branch 10 times, most recently from a2d6750 to bf64d3e Compare February 12, 2025 02:32

roberto-bayardo changed the title ~~[WIP] [commonware-storage/mmr] less restrictive in-mem-mmr forgetting~~ [commonware-storage/mmr] less restrictive in-mem-mmr forgetting Feb 12, 2025

roberto-bayardo marked this pull request as ready for review February 12, 2025 02:46

patrick-ogrady requested changes Feb 12, 2025

View reviewed changes

roberto-bayardo force-pushed the improve-mmr-forgetting branch 2 times, most recently from 0818788 to c31e780 Compare February 12, 2025 17:56

roberto-bayardo requested a review from patrick-ogrady February 12, 2025 18:02

roberto-bayardo added 3 commits February 12, 2025 10:50

less restrictive in-mem-mmr forgetting

943e9bf

bootstrap the in-mem cache in the 'forget_all' state

fbb6044

change names/visibility of forget related functions

94c718d

roberto-bayardo force-pushed the improve-mmr-forgetting branch from 300a62a to 94c718d Compare February 12, 2025 18:51

fix rebase

db8ba90

patrick-ogrady reviewed Feb 12, 2025

View reviewed changes

make proof boundary computation panic on invalid mmr params

7ea6d0b

roberto-bayardo requested a review from patrick-ogrady February 12, 2025 19:48

patrick-ogrady approved these changes Feb 12, 2025

View reviewed changes

roberto-bayardo merged commit 8d2d837 into main Feb 12, 2025
7 checks passed

roberto-bayardo deleted the improve-mmr-forgetting branch February 12, 2025 23:15

roberto-bayardo restored the improve-mmr-forgetting branch February 12, 2025 23:15

roberto-bayardo deleted the improve-mmr-forgetting branch February 12, 2025 23:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[commonware-storage/mmr] less restrictive in-mem-mmr forgetting #484

[commonware-storage/mmr] less restrictive in-mem-mmr forgetting #484

roberto-bayardo commented Feb 12, 2025 •

edited

Loading

patrick-ogrady Feb 12, 2025

roberto-bayardo Feb 12, 2025

patrick-ogrady Feb 12, 2025

roberto-bayardo Feb 12, 2025

patrick-ogrady Feb 12, 2025

roberto-bayardo Feb 12, 2025

patrick-ogrady Feb 12, 2025

patrick-ogrady Feb 12, 2025

roberto-bayardo Feb 12, 2025

patrick-ogrady Feb 12, 2025

roberto-bayardo Feb 12, 2025

patrick-ogrady Feb 12, 2025

roberto-bayardo Feb 12, 2025

patrick-ogrady Feb 12, 2025

patrick-ogrady Feb 12, 2025

roberto-bayardo Feb 12, 2025

roberto-bayardo Feb 12, 2025

patrick-ogrady Feb 12, 2025

patrick-ogrady Feb 12, 2025 •

edited by roberto-bayardo

Loading

roberto-bayardo Feb 12, 2025

roberto-bayardo Feb 12, 2025

patrick-ogrady Feb 12, 2025

roberto-bayardo Feb 12, 2025

patrick-ogrady Feb 12, 2025

roberto-bayardo Feb 12, 2025

roberto-bayardo Feb 12, 2025

patrick-ogrady Feb 12, 2025

codecov bot commented Feb 12, 2025

		@@ -85,7 +85,7 @@ impl<B: Blob, E: RStorage<B>, H: Hasher> Mmr<B, E, H> {
		let item = journal.read(i).await?;
		vec.push(item);

[commonware-storage/mmr] less restrictive in-mem-mmr forgetting #484

[commonware-storage/mmr] less restrictive in-mem-mmr forgetting #484

Conversation

roberto-bayardo commented Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrick-ogrady Feb 12, 2025 • edited by roberto-bayardo Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 12, 2025

Codecov Report

roberto-bayardo commented Feb 12, 2025 •

edited

Loading

patrick-ogrady Feb 12, 2025 •

edited by roberto-bayardo

Loading