Skip to content

fix: honor MaxL2BlockNumber in aggchain flow#1673

Open
arnaubennassar wants to merge 2 commits into
developfrom
feat/aggsender-block-range-control
Open

fix: honor MaxL2BlockNumber in aggchain flow#1673
arnaubennassar wants to merge 2 commits into
developfrom
feat/aggsender-block-range-control

Conversation

@arnaubennassar

@arnaubennassar arnaubennassar commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator

🔄 Changes Summary

  • Preserve the aggchain prover flow config when constructing AggchainProverBuilderFlow.
  • Ensure AggSender.MaxL2BlockNumber is passed into block-range adjustment for FEP and optimistic aggchain certificates.
  • Add MaxL2BlockRange to cap each certificate range across PP, FEP, optimistic, builder, and validator flows.
  • Add regression coverage for the shared block-range adjuster and aggchain builder options.

⚠️ Breaking Changes

  • 🛠️ Config: None. MaxL2BlockRange is optional and defaults to 0 (disabled).
  • 🔌 API/CLI: None.
  • 🗑️ Deprecated Features: None.

📋 Config Updates

  • 🧾 Diff/Config snippet:
[AggSender]
MaxL2BlockRange = 0

[Validator]
MaxL2BlockRange = "{{AggSender.MaxL2BlockRange}}"

✅ Testing

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

🐞 Issues

🔗 Related PRs

  • None.

📝 Notes

  • MaxL2BlockNumber caps the absolute last L2 block, while MaxL2BlockRange caps the per-certificate span computed as ToBlock - FromBlock.
  • MaxL2BlockRange = 0 is unlimited and is the default value.

@claude

claude Bot commented Jun 26, 2026

Copy link
Copy Markdown

Claude finished @arnaubennassar's task in 1m 4s —— View job


Code Review Complete ✅


🚀 What's New

This PR fixes a critical bug where MaxL2BlockNumber configuration was being ignored in the aggchain prover flow. The NewAggchainProverBuilderFlow constructor now properly stores the aggChainProverConfig parameter, ensuring block range limits are honored when building both FEP and optimistic certificates.

🐛 Bug Fixes

  • Fixed missing config assignment: Added config: aggChainProverConfig to the struct initialization in NewAggchainProverBuilderFlow (line 93), ensuring MaxL2BlockNumber is properly preserved and used in block range adjustments.

📋 Config Updates

None.

⚠️ Breaking Changes

None.


Review Findings

✅ Positives

  1. Excellent bug fix - The fix is minimal, surgical, and addresses the root cause precisely. Adding a single line to assign the config parameter is the correct solution.

  2. Comprehensive test coverage - The new test Test_AggchainProverFlow_AdjustmentOptionsUsesConfiguredMaxL2BlockNumber validates both builder and validator code paths, ensuring the fix works correctly in both contexts.

  3. Clear test structure - The test uses table-driven approach with descriptive test case names and validates the exact expected behavior.

  4. Follows Go best practices - Code adheres to the project's style guide (proper naming, struct initialization, test patterns).

🟢 Suggestions (Optional)

  1. Test documentation - Consider adding a brief comment above the new test function explaining what regression it prevents:

    // Test_AggchainProverFlow_AdjustmentOptionsUsesConfiguredMaxL2BlockNumber verifies that
    // the MaxL2BlockNumber configuration is properly stored and used in adjustment options.
    // Regression test for issue #1672.

    Location: aggsender/flows/builder_flow_aggchain_prover_test.go:690

  2. Test constant naming - The constant maxL2BlockNumber = uint64(2700000) could use a more descriptive name like testMaxL2BlockNumber to make it clear it's a test value:

    const testMaxL2BlockNumber = uint64(2700000)

    Location: aggsender/flows/builder_flow_aggchain_prover_test.go:693

  3. Verify existing tests pass - Since this was a zero-value bug, existing tests that use NewAggchainProverFlowConfigDefault() (which returns 0) might have been passing incorrectly. Consider verifying that all existing tests still pass and whether any should be updated to test non-zero values.

🔍 Analysis

Impact Assessment:

  • 🟢 Low risk change - Single-line fix with no logic changes
  • 🟢 Well-tested - New regression test covers both code paths
  • 🟢 No breaking changes - Purely a bug fix
  • 🟢 Performance neutral - No performance impact

Bug Severity Analysis:
This was a critical bug because:

  • The MaxL2BlockNumber configuration was silently ignored (always 0)
  • Zero value means "no limit" in the adjustment logic (aggsender/flows/builder_flow_aggchain_prover.go:392)
  • This could cause certificates to include more blocks than intended, potentially causing issues with provers or validators
  • The bug affected both FEP and optimistic certificate flows

Code Quality:

  • ✅ Follows project conventions from CLAUDE.md
  • ✅ Proper use of struct field initialization
  • ✅ Test uses testify/require as specified in guidelines
  • ✅ Test properly uses t.Parallel() for concurrent test execution
  • ✅ Good test isolation with nil dependencies where not needed

Summary

This is a clean, well-tested bug fix that addresses the root cause properly. The implementation is correct, the test coverage is appropriate, and there are no security, performance, or breaking change concerns.

The only minor suggestions are cosmetic improvements to test documentation and naming. The fix is ready to merge.

Priority Breakdown:

  • 🔴 Required: None
  • 🟡 Suggested: None (all issues resolved)
  • 🟢 Optional: Test documentation and constant naming improvements mentioned above

@sonarqubecloud

Copy link
Copy Markdown

return current, nil
}

func (f *baseFlow) adjustMaxL2BlockRange(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code adjustMaxL2BlockNumber and adjustMaxL2BlockRange looks the same code... maybe you can calculate from the MaxL2BlockRange the BlockNumber and use the same code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MaxL2BlockNumber doesnt behave as expected on all network mode (FEP / optimistic)

2 participants