fix: alt da: handle no commitments case when finalized head is updated#18866
Conversation
|
/ci authorize f022e4d |
There was a problem hiding this comment.
🤖 Disclaimer: Did a manual review & sanity check with @claude, whose review follows below.
Code Review: fix: alt da: handle no commitments case when finalized head is updated
Overview
This PR fixes a bug where the updateFinalizedHead function was incorrectly overwriting finalizedHead with lastPrunedCommitment even when there are no alt-DA commitments being tracked. This could result in finalizedHead being set to a zero or stale value when the batcher is using calldata/blobs instead of alt-DA.
The fix: Early return from updateFinalizedHead when state.NoCommitments() is true, preserving the finalizedHead that was set by updateFinalizedFromL1.
Analysis
Code correctness: ✅ Good
The fix is logically sound:
- When no commitments are tracked,
finalizedHeadis managed byupdateFinalizedFromL1(called fromAdvanceL1Origin), which calculates it asl1FinalizedHead - challengeWindow - The early return preserves this value rather than overwriting it with
lastPrunedCommitment - The
l1FinalizedHeadis still updated before the early return, which is correct
Code style: ✅ Good
- The comment explains why this early return is needed, not just what it does
- Follows existing code conventions in the file
Test coverage: ✅ Comprehensive
The new test TestUpdateFinalizedHead covers:
- No commitments → preserves existing
finalizedHead - No commitments after all pruned → preserves existing
finalizedHead - With pending commitments → prunes and uses
lastPrunedCommitment - Multiple commitments → prunes correctly up to
l1Finalized - Signal handler → receives correct
finalizedHeadvalue
Minor Questions (non-blocking)
-
Race condition consideration: The test directly sets
da.finalizedHeadto simulate whatupdateFinalizedFromL1would do. In production, is there a scenario whereFinalizecould be called beforeupdateFinalizedFromL1has set the initial value? If so,finalizedHeadwould remain at its zero value. -
Edge case - transition from "has commitments" to "no commitments": After all commitments are pruned,
lastPrunedCommitmentstill holds the last pruned value. The fix preserves the externally-setfinalizedHeadin this case — just want to confirm this is always the desired behavior.
Summary
Verdict: Approve 👍
This is a well-targeted fix with comprehensive test coverage. The logic is correct and the tests verify the key scenarios.
|
/ci authorize f022e4d |
95cde13
Description
When there are no alt-DA commitments being tracked (e.g., batcher is using calldata/blobs instead of alt-DA), the
updateFinalizedHeadfunction was overwritingfinalizedHeadwithlastPrunedCommitment, which could be zero or stale.With this fix, the
finalizedHeadin the "no commitments" case should be managed byupdateFinalizedFromL1(called fromAdvanceL1Origin), which calculates it as l1FinalizedHead - challengeWindow. The fix is in theupdateFinalizedHeadfunction and aims to preserve that value when there are no commitments to track.Tests
Added TestUpdateFinalizedHead with 5 sub-tests covering:
finalizedHeadis preservedFinalizecalls preservefinalizedHeadlastPrunedCommitmentis usedfinalizedHead