feat: propose chunk at hardfork boundary #1767
Conversation
WalkthroughVersion bumped to v4.7.7. Chunk proposer gains hardfork-boundary detection: when blocks in a proposed chunk have differing hardfork names, the chunk is truncated at the boundary, metrics are recorded and the truncated chunk is persisted immediately via an early-return path. Tests updated to exercise the boundary case. Changes
Sequence DiagramsequenceDiagram
participant Proposer as ProposeChunk
participant Checker as HardforkChecker
participant Logger as Logger
participant Metrics as Metrics
participant DB as Database
participant Constraints as Constraints
Proposer->>Checker: Inspect blocks' hardfork names
alt Hardfork boundary detected
Checker-->>Proposer: boundary = true
Proposer->>Proposer: Truncate blocks at boundary
Proposer->>Logger: Log boundary event
Proposer->>Metrics: Record boundary metrics
Proposer->>DB: Persist truncated chunk
Proposer-->>Proposer: Early return
else No boundary
Checker-->>Proposer: boundary = false
Proposer->>Constraints: Evaluate limits / timeout
Proposer->>Metrics: Record final metrics
Proposer->>DB: Persist chunk normally
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2024-10-20T16:13:20.397ZApplied to files:
🧬 Code graph analysis (1)rollup/internal/controller/watcher/chunk_proposer_test.go (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1767 +/- ##
===========================================
+ Coverage 36.54% 36.59% +0.04%
===========================================
Files 247 247
Lines 21226 21236 +10
===========================================
+ Hits 7757 7771 +14
+ Misses 12639 12634 -5
- Partials 830 831 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
rollup/internal/controller/watcher/chunk_proposer_test.go (1)
96-96: Consider extracting ChainConfig initialization for readability.The ChainConfig initialization is functionally correct, but the line exceeds 250 characters. For improved maintainability, consider extracting it to a separate variable or helper function.
Example refactor:
- cp := NewChunkProposer(context.Background(), &config.ChunkProposerConfig{ - MaxL2GasPerChunk: tt.maxL2Gas, - ChunkTimeoutSec: tt.chunkTimeoutSec, - MaxUncompressedBatchBytesSize: math.MaxUint64, - }, encoding.CodecV7, ¶ms.ChainConfig{LondonBlock: big.NewInt(0), BernoulliBlock: big.NewInt(0), CurieBlock: big.NewInt(0), DarwinTime: new(uint64), DarwinV2Time: new(uint64), EuclidTime: new(uint64), EuclidV2Time: new(uint64), FeynmanTime: new(uint64), GalileoTime: tt.GalileoTime}, db, nil) + chainConfig := ¶ms.ChainConfig{ + LondonBlock: big.NewInt(0), + BernoulliBlock: big.NewInt(0), + CurieBlock: big.NewInt(0), + DarwinTime: new(uint64), + DarwinV2Time: new(uint64), + EuclidTime: new(uint64), + EuclidV2Time: new(uint64), + FeynmanTime: new(uint64), + GalileoTime: tt.GalileoTime, + } + cp := NewChunkProposer(context.Background(), &config.ChunkProposerConfig{ + MaxL2GasPerChunk: tt.maxL2Gas, + ChunkTimeoutSec: tt.chunkTimeoutSec, + MaxUncompressedBatchBytesSize: math.MaxUint64, + }, encoding.CodecV7, chainConfig, db, nil)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
common/version/version.go(1 hunks)rollup/internal/controller/watcher/chunk_proposer_test.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
🔇 Additional comments (4)
common/version/version.go (1)
8-8: LGTM: Version bump aligns with the new feature.The version increment from v4.7.6 to v4.7.7 is appropriate for the hardfork boundary handling feature introduced in this PR.
rollup/internal/controller/watcher/chunk_proposer_test.go (3)
22-22: LGTM: Idiomatic helper for test setup.The
newUint64helper follows standard Go patterns for creating pointers to uint64 literals, making test case initialization cleaner.
31-31: LGTM: Test configuration extended appropriately.Adding the optional
GalileoTimefield enables fork boundary test scenarios while maintaining backward compatibility with existing test cases.
68-75: I'll help you verify the block2 timestamp claim. Let me search for the block2 fixture definition in the test file.
<function_calls>
#!/bin/bashSearch for block2 definition and its timestamp in the test file
rg -n -A20 'block2' rollup/internal/controller/watcher/chunk_proposer_test.go | head -60
</function_calls>Let me also search more broadly to understand the test structure:
<function_calls>
#!/bin/bashGet the overall structure of the test file to understand where fixtures are defined
wc -l rollup/internal/controller/watcher/chunk_proposer_test.go
</function_calls><function_calls>
#!/bin/bashSearch for block fixture definitions at the beginning or in setup functions
rg -n 'var.block|:= &types.Block|Timestamp.=' rollup/internal/controller/watcher/chunk_proposer_test.go | head -40
</function_calls>
Purpose or design rationale of this PR
Hard forks define chunk boundaries. After passing the next hard fork block height, we’d expect that rollup-relayer will immediately propose the last chunk of the previous version, but this was not the case during the Galileo testnet upgrade.
Root cause:
scroll/rollup/internal/controller/watcher/chunk_proposer.go
Line 253 in 2ecc42e
scroll/rollup/internal/controller/watcher/chunk_proposer.go
Line 292 in 2ecc42e
scroll/rollup/internal/controller/watcher/chunk_proposer.go
Line 325 in 2ecc42e
PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
Deployment tag versioning
Has
tagincommon/version.gobeen updated or have you addedbump-versionlabel to this PR?Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
Chores
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.