You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This PR introduces a pub/sub notification system for GER (Global Exit Root) reorgs in the L1InfoTreeSync component. When a blockchain reorg occurs and affects L1InfoTree leaves, subscribers can now receive notifications containing details about which GERs were removed. This enables other components (like aggsender) to react to reorgs and update their state accordingly.
🐛 Bug Fixes
None
📋 Config Updates
None
⚠️ Breaking Changes
None - This is a purely additive change. The new SubscribeToGERReorg method adds functionality without modifying existing APIs.
Code Review Feedback
🟢 Strengths
Excellent test coverage - The PR includes comprehensive tests covering:
Basic subscription and event delivery (processor_test.go:392-483)
Edge case: no events when no leaves affected (processor_test.go:485-515)
Edge case: no events when reorg block doesn't exist (processor_test.go:517-554)
Integration test via public API (l1infotreesync_test.go:336-375)
Well-documented API - The SubscribeToGERReorg method includes clear godoc with usage example (l1infotreesync.go:491-513)
Proper encapsulation - The pub/sub mechanism is correctly hidden in the processor, exposed only through the public API
Defensive programming - Events are only published when both rowsAffected > 0 AND len(reorgedLeaves) > 0 (processor.go:384-396)
🟡 Suggestions
Performance consideration - Line processor.go:345-352: The query fetches ALL affected leaves into memory before the DELETE. For very large reorgs (hundreds or thousands of blocks with multiple events each), this could be memory-intensive. Consider if there's a reasonable upper bound on reorg size, or if pagination/batching would be beneficial.
Minor: Log redundancy - Line processor.go:379: The log message includes "block rows affected" and "l1info_leaf entries removed", but the first might be confusing since one block can have multiple l1info_leaf entries. Consider rewording to "blocks removed" for clarity.
Consistency - The GERReorgEvent struct at processor.go:126-136 follows good patterns. Consider if Timestamp should be time.Time instead of uint64 for better type safety, though keeping it as uint64 is acceptable for JSON serialization compatibility.
Appropriate use of mutex for thread-safety (halted/unhalt)
Uses testify with require (not assert) as specified in CLAUDE.md
Thread Safety: ✅
The gerReorgNotifier uses aggkitcommon.PubSub which should handle concurrent subscribers
The reorg operation properly uses transactions
Error Handling: ✅
Properly checks sql.ErrNoRows when querying affected leaves
Transaction rollback is correctly deferred
Test Coverage: ✅
Tests cover happy path, edge cases, and concurrent scenarios
Concurrent test (processor_test.go:292-390) is particularly thorough
Security & Performance
Security: ✅ No security concerns identified
No SQL injection (uses parameterized queries)
No unauthorized access patterns
Performance: 🟡 Minor consideration
The query at processor.go:347 could be expensive for large reorgs, but this is likely acceptable given that reorgs are rare events
Summary
This is a well-implemented feature that adds important reorg notification capabilities to the l1infotreesync component. The code quality is high, with excellent test coverage and clear documentation. The pub/sub pattern is appropriate for this use case.
Recommendation: ✅ Approve - No blocking issues found. The suggestions above are optional improvements that don't block merging.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔄 Changes Summary
📋 Config Updates
✅ Testing
🐞 Issues
🔗 Related PRs
📝 Notes