fix: preserve empty items when merging checkpoints #19288
Conversation
| let num_checkpoints = num_left_checkpoints + num_right_checkpoints; | ||
| assert( | ||
| num_checkpoints <= AZTEC_MAX_EPOCH_DURATION as u16, | ||
| num_checkpoints <= AZTEC_MAX_EPOCH_DURATION, |
There was a problem hiding this comment.
Is it a problem that we're comparing against this max value (48) instead of the actual number of checkpoints that L1 currently expects to be in an epoch (32)?
Does L1 reject the epoch if someone tries to create an epoch with >32 checkpoints?
There was a problem hiding this comment.
I thought L1 also used AZTEC_MAX_EPOCH_DURATION. Where did you see the actual number being 32?
There was a problem hiding this comment.
I just picked up from recent slack discussions that epochs are 32 checkpoints at the moment
| for i in 0..N { | ||
| is_gte_start |= i == start_index; | ||
| is_lt_end &= i != end_index; |
There was a problem hiding this comment.
Should we consistently use those fns in for_loop.nr, or have we decided against doing that?
There was a problem hiding this comment.
I would suggest against doing that. I don't think it's common in software engineering to create utils for for loops. I find it easier to reason about the code with the for loop written in it. If we use the utils, I usually have to restore the code back to for loop mentally or literally 😅
There was a problem hiding this comment.
Javascript map, for_each are utils for for loops :P
That's interesting, because my brain does the opposite: I go "Oh no, I have to reason about these |= and &= approaches and think about this in detail", instead of "It is correct because I can trust the utils are performing this logic correctly".
There was a problem hiding this comment.
I worry that we'll have a bug in one of our many for loops, which all slightly differ in how they do the |= logic. The for_loop.nr utils were intended to make it consistent and free of bug worries.
There was a problem hiding this comment.
Javascript map, for_each are utils for for loops
These are methods for arrays, I also find them useful and I agree they are pretty common (same for custom types/structs and our ClaimedLengthArray)! The number of items to loop and the range the callback will be applied are the same in most languages.
I was referring to utils that takes values whose meanings are defined in the functions calling the utils.
which all slightly differ in how they do the |= logic
That's probably a reason why they shouldn't use a general purposed util 😉
There was a problem hiding this comment.
which all slightly differ in how they do the |= logic
Hahahaha, no I mean they all differ, but they actually try to do the same thing.
These are methods for arrays,
They suuure are! copy_items_into_array is for arrays too :P
The L1 contract processes fees in the array one by one, assuming each entry corresponds to the checkpoint at the same position, and uses the index to get the fee header. But the circuits allow zero fee and recipient. So when merging fees from two checkpoints' public inputs, their indexes need to be preserved, we can't skip the empty items.