utils/mmr: safety & performance rework #274
Conversation
also changed some variable names and added some comments, ensured test worked with new compact model
Zk2u
added
enhancement
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## master #274 +/- ##
==========================================
- Coverage 58.23% 58.17% -0.06%
==========================================
Files 160 160
Lines 15304 15309 +5
==========================================
- Hits 8912 8906 -6
- Misses 6392 6403 +11
|
Zk2u
changed the title
CompactMmr
|
Edit: this may be a bigger PR than expected as the original MMR implementation was alloc free but this ported version doesn't have any bound checks on the number of elements (i.e if you allocate for 13 peaks, you're limited to 2^(13-1) = 4,096 elements and adding another leaf after this will (I believe) result in a panic in 103-111). We should either be returning errors or changing the peaks to be a vector so we can allocate further |
crates/util/mmr/src/lib.rs
Outdated
| let num_to_add = required - peaks.len(); | ||
| peaks.reserve_exact(num_to_add); | ||
| // note we add in a loop so we don't need to make 2 allocs | ||
| // (the ZEROs will be stack allocated... i think) |
There was a problem hiding this comment.
| // (the ZEROs will be stack allocated... i think) | |
| // (the ZEROs might be be stack allocated...) |
Unpersonal.
Also I don't understand the note.
On one sense, yes ZERO is not only const but also Sized, hence it can be trivially stack-allocated by the compiler.
However, the peaks is a Vec and MAYBE the compiler can totally infer the total size and any possible interactions at compile time, but there might be no such guarantees at run time.
Hence, I would not be surprised if peaks is heap-allocated.
There was a problem hiding this comment.
AFAIK Vecs' sizes cannot be inferred at build time, they're purely allocated at runtime.
What I mean is that instead of a vec![ZERO; num_to_add] then extending peaks (which correct, is heap allocated, all Vecs are) which would incur another allocation - once when initializing the vector then a second allocation to peaks when extending from the slice - we can avoid the initialization entirely by using a loop:
(0..num_to_add).for_each(|_| peaks.push(ZERO))Where each iteration of the loop puts a ZERO on the stack, then memcpy's it to the end of peaks. This avoids the second allocation required if we were to initialize another vector then extend from it.
|
Thanks for comments I'll reply in a sec. Well, I'm not sure how useful I'll be contributing to the devnet in the short time until deadline so if you want me to focus on that I probably can while I learn more about the high/mid level architecture. |
Co-authored-by: Jose Storopoli <[email protected]>
Zk2u
changed the title
CompactMmr
| .collect(), | ||
| element_count: self.element_count, | ||
| peaks: match min_peaks { | ||
| 0 => vec![ZERO], |
There was a problem hiding this comment.
Might want to tweak the representation on the decode side so an empty compact MMR has no peaks at all and this can just be an empty buffer.
| /// Returns the minimum peaks needed to store an MMR of `element_count` elements | ||
| #[inline] | ||
| fn min_peaks(element_count: u64) -> usize { | ||
| match element_count { | ||
| 0 => 0, | ||
| c => c.ilog2() as usize + 1, |
There was a problem hiding this comment.
Should split this out into a toplevel function so we don't have to call it with Self::.
| /// Compact representation of the MMR that should be borsh serializable easily. | ||
| #[derive(Debug, Clone, PartialEq, Eq, BorshSerialize, BorshDeserialize, Arbitrary)] | ||
| pub struct CompactMmr { |
There was a problem hiding this comment.
What would be really cool is to write manual borsh serialization so that we don't have to store the length of the peaks list and can infer it from the element count (since it's redundant, it's just popcnt).
|
This is nowhere near finished and I'm pausing work on the PR until it's needed/I have free time to do it. I think I'll rework the implementation entirely. I'll request another review if i pick this back up/anyone needs it actively. |
|
Not needed for devnet. |
Description
For my first day I thought I'd tackle something small that didn't need much contextual knowledge so I've been reworking the MMR port that @voidash worked on - mostly cleaning up but optimizing where possible.
Type of Change
Checklist
Related Issues
None