refactor(aggsender): unify block range adjustment

[arnaubennassar]

🔄 Changes Summary

• Unify aggsender certificate block-range adjustment under a single AdjustBlockRange flow path.
• Move max-L2 limiting, finalized-root claim provability checks, certificate-size trimming, and invalid-claim posterior-unclaim validation into the same ordered pipeline.
• Remove the old standalone max-L2 limiter and the CertificateBuildParams range mutator helpers, then update PP and aggchain prover flows and mocks to use the unified adjuster.
• Rewrite and extend aggsender tests to cover the new adjustment flow and root-based behavior.

⚠️ Breaking Changes

• 🛠️ Config: None.
• 🔌 API/CLI: Internal aggsender flow interfaces now use AdjustBlockRange; LimitCertSize, Range, AdjustToBlock, and the standalone max-L2 limiter path were removed.
• 🗑️ Deprecated Features: Removed MaxL2BlockNumberLimiter and its dedicated tests.

📋 Config Updates

• 🧾 Diff/Config snippet: None.

✅ Testing

• 🤖 Automatic: go test ./aggsender/...
• 🖱️ Manual: Not run.

🐞 Issues

• None.

🔗 Related PRs

• None.

📝 Notes

• Claim filtering for finalized data is now driven by L1InfoTreeRootFromWhichToProve: proposer paths select the finalized root up front, while validator paths assert that the provided root is finalized before accepting the range.
• Invalid-claim/unclaim validation remains the last adjustment step so earlier truncations cannot silently invalidate the final certificate range.

[claude]

Claude finished @arnaubennassar's task in 2m 40s —— View job

---

Code Review Complete

🚀 What's New

This PR successfully unifies the aggsender certificate block-range adjustment logic under a single AdjustBlockRange method. The refactoring consolidates four previously separate adjustment flows (max-L2 limiting, finalized-root claim provability checks, certificate-size trimming, and invalid-claim posterior-unclaim validation) into one ordered pipeline in aggsender/flows/adjust_block_range.go.

🐛 Bug Fixes

None.

📋 Config Updates

None.

⚠️ Breaking Changes

• Internal API changes: The AggsenderFlowBaser interface now exposes AdjustBlockRange with a unified BlockRangeAdjustmentOptions struct. The following methods/types were removed:
  • LimitCertSize method
  • CertificateBuildParams.Range() method
  • CertificateBuildParams.AdjustToBlock() method
  • MaxL2BlockNumberLimiter class and its dedicated tests
• Flow behavior: Claim filtering based on finalized data is now driven by L1InfoTreeRootFromWhichToProve, with proposer paths selecting the finalized root upfront and validator paths asserting that the provided root is finalized.

---

📊 Review Summary

✅ Strengths

1. 🟢 Excellent test coverage: The new adjust_block_range_test.go file contains 690 lines of comprehensive tests covering edge cases, error conditions, and normal flows. Tests are well-structured and use table-driven patterns.

2. 🟢 Clear separation of concerns: The unified AdjustBlockRange method has a clean pipeline structure with well-defined steps:

   • adjustMaxL2BlockRange
   • validateRootToProveIsFinalized
   • adjustClaimsNotProvableAgainstRoot
   • limitCertSize
   • adjustInvalidClaimsAreNotUnclaimed

3. 🟢 Better code organization: Removing 258 lines from flow_base.go and consolidating logic reduces cognitive load and makes the codebase easier to maintain.

4. 🟢 Consistent error handling: Error messages are descriptive and provide context about what went wrong.

5. 🟢 Performance optimization: The GER validation cache (gerValidationCache) prevents redundant queries for the same Global Exit Root across multiple claims.

🟡 Suggested Improvements

1. 🟡 Documentation (aggsender/flows/adjust_block_range.go:33-72):

   • The AdjustBlockRange function lacks a doc comment explaining the adjustment pipeline flow and what each option does
   • Consider adding inline comments explaining the ordering rationale (e.g., why invalid-claim validation happens last)

2. 🟡 Magic numbers (aggsender/flows/adjust_block_range.go:362-408):

   • The filter functions use capacity estimation via aggkitcommon.EstimateSliceCapacity but the logic could be more readable with a comment explaining the span/fullSpan calculation

3. 🟡 Potential nil pointer (aggsender/flows/adjust_block_range.go:269-296):

   • claimHasPosteriorUnclaim modifies the usedUnclaims slice but doesn't validate that the slice length matches the unclaims length. While the current usage is safe, adding a defensive check would improve robustness.

4. 🟡 Test clarity (aggsender/flows/adjust_block_range_test.go:382-435):

   • Some test names like "handles nil claim global index" could be more specific about the expected behavior (e.g., "returns false when claim has nil global index")

🔴 Issues Found

1. 🔴 Security: Missing validation for nil GlobalIndex (aggsender/types/certificate_build_params.go:153-179):

   func (c *CertificateBuildParams) GetClaimsFilteringUnclaims() []claimsynctypes.Claim {
       // ...
       for i, unclaim := range c.Unclaims {
           if claim.GlobalIndex.Cmp(unclaim.GlobalIndex) == 0 && !usedUnclaims[i] {  // Line 167

   Issue: If unclaim.GlobalIndex is nil, calling .Cmp() will cause a nil pointer dereference panic.

   Fix: Add nil checks before comparison:

   if claim.GlobalIndex != nil && unclaim.GlobalIndex != nil && 
      claim.GlobalIndex.Cmp(unclaim.GlobalIndex) == 0 && !usedUnclaims[i] {

   Fix this →

2. 🔴 Logic: Inconsistent nil handling (aggsender/flows/adjust_block_range.go:269-296):

   func claimHasPosteriorUnclaim(...) bool {
       if claim.GlobalIndex == nil {
           return false  // Line 275
       }
       for idx, unclaim := range unclaims {
           if usedUnclaims[idx] || unclaim.GlobalIndex == nil {
               continue  // Line 280
           }

   Issue: The function returns false when claim.GlobalIndex is nil (line 275) but silently skips unclaims with nil GlobalIndex (line 280). This inconsistency could lead to unexpected behavior. The function should document why nil claims return false but nil unclaims are skipped.

   Recommendation: Add a comment explaining the rationale or ensure consistent handling.

3. 🟡 Error message clarity (aggsender/flows/adjust_block_range.go:86-88):

   return nil, fmt.Errorf("adjustBlockRange can't adapt the retry certificate, "+
       "the ToBlock %d is greater than the maxL2BlockNumber %d. Err: %w",
       buildParams.ToBlock, options.MaxL2BlockNumber, ErrMaxL2BlockNumberExceededInARetryCert)

   The error prefix "adjustBlockRange" is lowercase and inconsistent with Go conventions. Consider "AdjustBlockRange" or removing the prefix since the function name is already in the stack trace.

4. 🟡 Missing edge case test: The cloneCertificateBuildParamsWithRange function correctly preserves the ExtraData field (line 358), but there's no explicit test verifying that an empty string ExtraData is preserved vs. a nil/missing field.

5. 🟡 Code duplication: The filter functions filterBridgesInRange, filterClaimsInRange, and filterUnclaimsInRange (lines 362-408) have nearly identical logic. Consider using Go generics or a helper function to reduce duplication.

🔍 Additional Observations

1. CI Failure: The "Go Vulnerability Check" workflow is failing due to two vulnerabilities in dependencies:

   • google.golang.org/grpc@v1.79.1 (Authorization bypass) - Fixed in v1.79.3
   • github.com/pion/dtls/v2@v2.2.12 (AES GCM nonce issue) - No fix available yet

   Note: These are dependency issues unrelated to this PR's changes. They should be addressed separately.

2. Migration quality: The removal of MaxL2BlockNumberLimiter (127 lines) and the old adjustment logic (258 lines from flow_base.go) is clean. All error constants were properly migrated to the new file.

3. Interface changes: The AggsenderFlowBaser interface is cleaner with the unified AdjustBlockRange method replacing multiple adjustment methods.

4. Backward compatibility: While this is a breaking change for internal APIs, external interfaces remain unchanged, which is good for API consumers.

---

🎯 Recommendations

Priority: 🔴 Required

1. Fix the nil pointer dereference risk in GetClaimsFilteringUnclaims (certificate_build_params.go:167)

Priority: 🟡 Suggested

1. Add doc comments to AdjustBlockRange explaining the pipeline flow and ordering
2. Add test case for ExtraData preservation in cloneCertificateBuildParamsWithRange
3. Consider refactoring the three filter functions to reduce code duplication
4. Improve error message consistency (capitalization)
5. Document the nil handling rationale in claimHasPosteriorUnclaim

Priority: 🟢 Optional

1. Update dependency versions to fix the security vulnerabilities (separate PR)
2. Add inline comments explaining the adjustment step ordering rationale
3. Consider adding benchmark tests for the new unified flow to ensure no performance regression

---

✅ Verdict

This is a well-executed refactoring that significantly improves code organization and maintainability. The unified adjustment flow is clearer and more testable than the previous scattered approach. However, there is one critical nil pointer dereference bug that must be fixed before merging.

Once the required fix is applied, this PR will be ready to merge. The suggested improvements are non-blocking but would further enhance code quality.

Approval Status: ⚠️ Changes Requested (fix nil pointer dereference bug)

---

[sonarqubecloud]

Quality Gate passed

Issues
7 New issues
0 Accepted issues

Measures
0 Security Hotspots
91.5% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud