fix: prevent duplicate piece IDs in schedulePieceDeletions #228
Conversation
- Add descriptive names to mapping keys for better readability - Optimize array processing by copying elements and deleting entire array - Replace individual pop() calls with bulk array deletion
Signed-off-by: Jakub Sztandera <[email protected]>
Kubuxu
force-pushed
the
fix/duplicate-piece-id-prevention
branch
from
c9b4cb5 to
bdb1ce7
Compare
Also add a redudant but explicit case that zero sized pieces won't be added to the data set Signed-off-by: Jakub Sztandera <[email protected]>
Kubuxu
force-pushed
the
fix/duplicate-piece-id-prevention
branch
from
bdb1ce7 to
fa640b3
Compare
|
Hey, @Chaitu-Tatipamula, I pushed two commits, one of which removes the whole slots when clearing out the list. |
Kubuxu
left a comment
Kubuxu
left a comment
There was a problem hiding this comment.
SGWM, but would like a second set of eyes from @wjmelements given that I pushed some code into this branch
src/PDPVerifier.sol
Outdated
| // This is a redudant check as Cids.isPaddingExcessive already checks for pieces which would result in leafCount == 0 | ||
| // but we keep it here for clarity | ||
| revert IndexedError(callIdx, "Leaf count of the pieece must be greater than 0"); | ||
| } |
There was a problem hiding this comment.
I'm for removing redundant checks
There was a problem hiding this comment.
They become untestable branches in the codecoverage
There was a problem hiding this comment.
I've added it because I spent a good 15 minutes verifying that it is actually true.
There was a problem hiding this comment.
can leave your comment about the assumption to preserve your hard work
There was a problem hiding this comment.
Removed the redundant check and just improved docstrings.
Signed-off-by: Jakub Sztandera <[email protected]>
4 participants
Fix: Prevent duplicate piece IDs in schedulePieceDeletions
Problem
The
schedulePieceDeletionsfunction didn't check for duplicate piece IDs, allowing the same piece to be scheduled for removal multiple times. This could happen within a single call or across multiple calls.Changes
isScheduledForRemovalmapping for O(1) lookups for duplicate checkingschedulePieceDeletionsto validate against existing scheduled removalsnextProvingPeriodto prevent stale stateTests
testSchedulePieceDeletionsDuplicatePrevention: Verifies duplicate detection works for both within-call and cross-call scenariostestMappingClearedAfterRemoval: Ensures mapping is properly cleared after removals are processedFixes #227