refactor(aggsender): unify block range adjustment#1573
refactor(aggsender): unify block range adjustment#1573arnaubennassar merged 10 commits intodevelopfrom
Conversation
|
Claude finished @arnaubennassar's task in 2m 40s —— View job Code Review Complete🚀 What's NewThis PR successfully unifies the aggsender certificate block-range adjustment logic under a single 🐛 Bug FixesNone. 📋 Config UpdatesNone.
|
There was a problem hiding this comment.
Pull request overview
This PR refactors aggsender’s certificate block-range adjustment logic into a unified AdjustBlockRange pipeline, removing the previous standalone max-L2 limiter and range-mutator helpers, and updating builder flows and tests accordingly.
Changes:
- Introduces a unified
AdjustBlockRangeflow that applies max-L2 limiting, root-provability checks, certificate-size trimming, and invalid-claim/unclaim validation in one ordered pipeline. - Updates PP and aggchain prover flows (and mocks) to use
AdjustBlockRange, deleting the old max-L2 limiter implementation and tests. - Expands/rewrites tests around the new adjustment behavior, and tweaks E2E Docker compose UID/GID handling for rootless Docker.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/envs/loader.go | Detects rootless Docker for compose UID/GID injection; makes aggkit data dir permissions /tmp-like. |
| aggsender/types/interfaces.go | Adds AdjustBlockRange to flow base interface; removes LimitCertSize and max-L2 limiter interface. |
| aggsender/types/certificate_build_params.go | Removes Range/AdjustToBlock; adds GetClaimsFilteringUnclaims. |
| aggsender/types/certificate_build_params_test.go | Replaces range/adjust tests with GetClaimsFilteringUnclaims tests. |
| aggsender/types/certificate_build_params_additional_test.go | Adds additional helper tests (nil handling, retry semantics, filtering edge cases). |
| aggsender/types/block_range_adjustment.go | Adds BlockRangeAdjustmentOptions struct for flow-specific constraints. |
| aggsender/optimistic/optimistichash/calculate_hash_commit_imported_bridges.go | Preallocates hash input buffer for imported-bridge commit hashing. |
| aggsender/mocks/mock_aggsender_flow_baser.go | Updates mock to include AdjustBlockRange; removes LimitCertSize. |
| aggsender/flows/max_l2blocknumber_limiter.go | Deleted (superseded by unified adjuster). |
| aggsender/flows/max_l2blocknumber_limiter_test.go | Deleted (superseded by unified adjuster tests). |
| aggsender/flows/flow_base.go | Removes old non-finalized-claim adjustment and exported size limiter usage from build-param generation. |
| aggsender/flows/flow_base_test.go | Updates tests to target new adjustment entrypoint and unexported size limiter. |
| aggsender/flows/builder_flow_pp.go | Routes PP flow through AdjustBlockRange (generate + get build params). |
| aggsender/flows/builder_flow_pp_test.go | Updates PP flow tests for root-proof-based behavior and unified adjuster calls. |
| aggsender/flows/builder_flow_pp_additional_test.go | Adds PP flow tests covering GenerateBuildParams / BuildCertificate / Signer wiring with adjuster. |
| aggsender/flows/builder_flow_aggchain_prover.go | Routes aggchain prover flow through AdjustBlockRange, adjusts range cloning behavior, and adds post-prover adjust call. |
| aggsender/flows/builder_flow_aggchain_prover_test.go | Updates aggchain prover flow tests for proof-based provability checks and adjuster usage. |
| aggsender/flows/builder_flow_aggchain_prover_optimistic_test.go | Updates optimistic aggchain prover test scaffolding to mock AdjustBlockRange. |
| aggsender/flows/adjust_block_range.go | New unified adjuster implementation (max-L2, root finalization validation, provability trimming, size limiting, invalid-claim/unclaim validation, cloning helpers). |
| aggsender/flows/adjust_block_range_test.go | New test suite covering unified adjuster components and edge cases. |
| // GetClaimsFilteringUnclaims returns a list of claims that contains all the claims of the CertificateBuildParams | ||
| // except the ones that have been unclaimed | ||
| func (c *CertificateBuildParams) GetClaimsFilteringUnclaims() []claimsynctypes.Claim { | ||
| filteredClaims := make([]claimsynctypes.Claim, 0, len(c.Claims)) | ||
| if len(c.Unclaims) == 0 { | ||
| // 99.9% of the times c.Unclaims is going to be empty | ||
| filteredClaims = append(filteredClaims, c.Claims...) | ||
| return filteredClaims | ||
| } |
There was a problem hiding this comment.
GetClaimsFilteringUnclaims does not handle a nil receiver (it immediately reads c.Claims/c.Unclaims), while the other CertificateBuildParams helpers in this file explicitly return safe defaults when c is nil. Consider adding an early if c == nil { return nil } (or an empty slice) to avoid potential panics and keep the API consistent.
| ErrMaxL2BlockNumberExceededInARetryCert = errors.New("maxL2BlockNumberLimiter. " + | ||
| "Max L2 block number exceeded in a retry certificate") | ||
| ErrComplete = errors.New("maxL2BlockNumberLimiter. " + | ||
| "All certs send, no more certificates can be sent") | ||
| ErrBuildParamsIsNil = errors.New("maxL2BlockNumberLimiter. BuildParams is nil") |
There was a problem hiding this comment.
The exported sentinel errors still use the old "maxL2BlockNumberLimiter" prefix even though this logic now lives in AdjustBlockRange. Updating the error messages to match the new component name would make logs and wrapped errors less confusing to operators and tests that assert on strings.
| ErrMaxL2BlockNumberExceededInARetryCert = errors.New("maxL2BlockNumberLimiter. " + | |
| "Max L2 block number exceeded in a retry certificate") | |
| ErrComplete = errors.New("maxL2BlockNumberLimiter. " + | |
| "All certs send, no more certificates can be sent") | |
| ErrBuildParamsIsNil = errors.New("maxL2BlockNumberLimiter. BuildParams is nil") | |
| ErrMaxL2BlockNumberExceededInARetryCert = errors.New("AdjustBlockRange. " + | |
| "Max L2 block number exceeded in a retry certificate") | |
| ErrComplete = errors.New("AdjustBlockRange. " + | |
| "All certs send, no more certificates can be sent") | |
| ErrBuildParamsIsNil = errors.New("AdjustBlockRange. BuildParams is nil") |
|
|
||
| if rootForLeafCount.Index > latestFinalizedRoot.Index { | ||
| return fmt.Errorf("L1 info tree root %s at leaf count %d is not finalized yet. latest finalized index: %d", | ||
| buildParams.L1InfoTreeRootFromWhichToProve.Hex(), buildParams.L1InfoTreeLeafCount, latestFinalizedRoot.Index+1) |
There was a problem hiding this comment.
In validateRootToProveIsFinalized the error says "latest finalized index" but formats latestFinalizedRoot.Index+1. Either report the actual latest finalized index (latestFinalizedRoot.Index) or rename the label to "leaf count" to avoid off-by-one confusion when debugging finalization mismatches.
| buildParams.L1InfoTreeRootFromWhichToProve.Hex(), buildParams.L1InfoTreeLeafCount, latestFinalizedRoot.Index+1) | |
| buildParams.L1InfoTreeRootFromWhichToProve.Hex(), buildParams.L1InfoTreeLeafCount, latestFinalizedRoot.Index) |
| if err != nil { | ||
| return nil, fmt.Errorf("ppFlow - error adjusting block range: %w", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
AdjustBlockRange now performs range truncation (size limiting / root provability trimming). If forceOneBridgeExit is true, the current early-return check happens before AdjustBlockRange, so the adjusted range could lose its only bridge exits and still proceed to build a claims-only certificate. Consider re-checking forceOneBridgeExit (and/or IsEmpty) after AdjustBlockRange, or have AdjustBlockRange enforce RequireOneBridgeInCertificate as a final post-condition after all truncation steps.
| if p.forceOneBridgeExit && buildParams.NumberOfBridges() == 0 { | |
| // AdjustBlockRange may truncate the range and remove bridge exits, so re-check the final range. | |
| p.log.Infof("PPFlow - forceOneBridgeExit is true, but adjusted range has no bridges, "+ | |
| "so no certificate will be built for range: %d - %d", | |
| buildParams.FromBlock, buildParams.ToBlock) | |
| return nil, nil | |
| } | |
| if buildParams.IsEmpty() { | |
| // AdjustBlockRange may truncate the range and remove all bridges/claims, so re-check the final range. | |
| p.log.Infof("PPFlow - no bridges or claims found for adjusted range: %d - %d, so no certificate will be built", | |
| buildParams.FromBlock, buildParams.ToBlock) | |
| return nil, nil | |
| } |
|
|
||
| return current, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
RequireOneBridgeInCertificate is only enforced inside adjustMaxL2BlockRange. Later steps (adjustClaimsNotProvableAgainstRoot, limitCertSize, adjustInvalidClaimsAreNotUnclaimed) can also shrink the range and potentially remove all bridges while leaving claims. If flows rely on this option (e.g. PP with forceOneBridgeExit), consider enforcing it as a final check at the end of AdjustBlockRange (after all truncations).
| return current, nil | |
| } | |
| if err := validateRequireOneBridgeInCertificate(current, options); err != nil { | |
| return nil, err | |
| } | |
| return current, nil | |
| } | |
| func validateRequireOneBridgeInCertificate( | |
| buildParams *types.CertificateBuildParams, | |
| options types.BlockRangeAdjustmentOptions, | |
| ) error { | |
| if !options.RequireOneBridgeInCertificate || buildParams == nil { | |
| return nil | |
| } | |
| if len(buildParams.Claims) > 0 && len(buildParams.Bridges) == 0 { | |
| return fmt.Errorf( | |
| "require one bridge in certificate: adjusted certificate for block range [%d, %d] contains claims but no bridges", | |
| buildParams.FromBlock, | |
| buildParams.ToBlock, | |
| ) | |
| } | |
| return nil | |
| } |
| buildParams, err = a.baseFlow.AdjustBlockRange(ctx, buildParams, a.adjustmentOptions(false)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("aggchainProverFlow - error adjusting block range after prover result: %w", err) | ||
| } |
There was a problem hiding this comment.
AdjustBlockRange is called after the proof is generated and the params are sliced to aggchainProof.EndBlock. If any adjustment step further trims the range (e.g. invalid-claim/unclaim validation after an unclaim got truncated out), buildParams.ToBlock can become < AggchainProof.EndBlock, leaving the proof inconsistent with the final certificate range. Consider making this post-prover AdjustBlockRange validate-only (no range changes), or detect range changes and regenerate the proof for the final range.
joanestebanr
left a comment
joanestebanr
left a comment
There was a problem hiding this comment.
After discussing the solution, from a functional point of view it seems ok to me
|
@copilot resolve the merge conflicts in this pull request |
|
🔄 Changes Summary
AdjustBlockRangeflow path.CertificateBuildParamsrange mutator helpers, then update PP and aggchain prover flows and mocks to use the unified adjuster.AdjustBlockRange;LimitCertSize,Range,AdjustToBlock, and the standalone max-L2 limiter path were removed.MaxL2BlockNumberLimiterand its dedicated tests.📋 Config Updates
✅ Testing
go test ./aggsender/...🐞 Issues
🔗 Related PRs
📝 Notes
L1InfoTreeRootFromWhichToProve: proposer paths select the finalized root up front, while validator paths assert that the provided root is finalized before accepting the range.