feat: multidownloader reorg unsafe sync v2 #1462
Conversation
joanestebanr
force-pushed
the
feature/multidownloader_reorg_unsafe_sync-v2
branch
from
507d2fe to
a4578b3
Compare
joanestebanr
changed the title
There was a problem hiding this comment.
Pull request overview
This pull request implements comprehensive reorganization handling and unsafe sync capabilities for the multidownloader component. The PR introduces new data structures and logic for detecting and processing blockchain reorganizations, separates synced data into finalized (safe) and non-finalized (unsafe) categories, and adds state management to track sync progress.
Changes:
- Introduces reorg detection and processing infrastructure with new types (ReorgError, ReorgData, ReorgProcessor)
- Implements unsafe sync mode that downloads non-finalized blocks with reorg checking
- Adds comprehensive state management to separate synced and pending segments
- Refactors storage layer with new tables for tracking reorged blocks and logs
Reviewed changes
Copilot reviewed 48 out of 48 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
| types/map_block_header.go | New type for mapping block numbers to headers |
| types/list_block_header.go | New type for list of block headers with helper methods |
| types/block_header.go | Added Empty() method to check if block header is empty |
| multidownloader/types/sync_segment.go | Added Empty() and IsEmpty() methods for segment management |
| multidownloader/types/storager.go | Extended interface with reorg and finalization support |
| multidownloader/types/reorg_*.go | New types for reorg error handling and data structures |
| multidownloader/storage/storage_*.go | Refactored storage layer with separate files for blocks, reorgs, and sync |
| multidownloader/storage/migrations/0002.sql | New migration adding reorg tracking tables |
| multidownloader/state.go | New state management for tracking synced vs pending segments |
| multidownloader/reorg_processor*.go | New reorg detection and processing logic |
| multidownloader/evm_multidownloader.go | Major refactoring with unsafe sync support and reorg handling |
| multidownloader/config.go | Added PeriodToCheckReorgs configuration |
| Test files | Updated and added tests for new functionality |
| // If FromBlock is 0 means that is empty | ||
| BlockRange aggkitcommon.BlockRange |
There was a problem hiding this comment.
The comment at line 15 says "If FromBlock is 0 means that is empty" but the IsEmpty method checks if FromBlock > ToBlock to determine emptiness. These two approaches are inconsistent. The comment should be updated to match the actual implementation in IsEmpty, or the implementation should be changed to match the comment.
| CREATE TABLE logs_reorged ( | ||
| chain_id BIGINT NOT NULL , | ||
| block_number BIGINT NOT NULL , | ||
| address TEXT NOT NULL, -- | ||
| topics TEXT NOT NULL, -- list of hashes in JSON | ||
| data BLOB, -- | ||
| tx_hash TEXT NOT NULL, | ||
| tx_index INTEGER NOT NULL, | ||
| log_index INTEGER NOT NULL, -- “index” is a reserved keyword | ||
| PRIMARY KEY (address, chain_id,block_number, log_index), | ||
| FOREIGN KEY (chain_id, block_number) REFERENCES blocks_reorged(chain_id, block_number) | ||
| ); | ||
|
|
||
| CREATE INDEX idx_logs_reorged_block_number ON logs_reorged(block_number); | ||
|
|
||
| CREATE TABLE blocks_reorged ( | ||
| chain_id BIGINT NOT NULL REFERENCES reorgs(chain_id), | ||
| block_number BIGINT NOT NULL, | ||
| block_hash TEXT NOT NULL, | ||
| block_timestamp INTEGER NOT NULL, | ||
| block_parent_hash TEXT NOT NULL, | ||
| PRIMARY KEY (chain_id, block_number) | ||
| ); | ||
|
|
||
| CREATE TABLE reorgs ( | ||
| chain_id BIGINT PRIMARY KEY, | ||
| detected_at_block BIGINT NOT NULL, | ||
| reorged_from_block BIGINT NOT NULL, | ||
| reorged_to_block BIGINT NOT NULL, | ||
| detected_timestamp INTEGER NOT NULL, | ||
| network_latest_block INTEGER NOT NULL, -- which was the latest block in the detection moment | ||
| network_finalized_block INTEGER NOT NULL, -- which was the finalized block in the detection moment | ||
| network_finalized_block_name TEXT NOT NULL, -- name of the finalized block (e.g., "finalized", "safe", etc.) | ||
| description TEXT -- extran information, can be null | ||
| ); No newline at end of file |
There was a problem hiding this comment.
The SQL migration has table creation order issues. The logs_reorged table references blocks_reorged via FOREIGN KEY at line 15, but blocks_reorged is defined later at line 20. Additionally, blocks_reorged references reorgs table at line 21, but reorgs is defined at line 29. In SQLite, tables must be created before they can be referenced in foreign key constraints. The order should be: reorgs, blocks_reorged, logs_reorged.
| func (s *State) OnNewSyncedLogQuery(logQuery *mdrtypes.LogQuery) error { | ||
| err := s.Synced.AddLogQuery(logQuery) | ||
| if err != nil { | ||
| return fmt.Errorf("OnNewSyncedLogQuery: addding syned segment: %w", err) |
There was a problem hiding this comment.
The comment at line 85 contains a typo: "addding syned segment" should be "adding synced segment".
| return fmt.Errorf("OnNewSyncedLogQuery: addding syned segment: %w", err) | |
| return fmt.Errorf("OnNewSyncedLogQuery: adding synced segment: %w", err) |
| // TODO: optimize this to don't check all blocks | ||
| // TODO: Find the first block to reorg |
There was a problem hiding this comment.
The TODO comment at line 165 mentions optimizing to not check all blocks and finding the first block to reorg. However, this is a critical performance concern for reorg detection. Consider creating a tracking issue for this optimization or removing the TODO if the current implementation is acceptable.
| // TODO: optimize this to don't check all blocks | |
| // TODO: Find the first block to reorg | |
| // Compare all candidate blocks against RPC headers; return immediately on the first mismatch (reorg). | |
| // Further performance optimizations (e.g., narrowing the checked range) can be evaluated separately if needed. |
| func (lbs ListBlockHeaders) BlockNumbers() []uint64 { | ||
| result := make([]uint64, 0, len(lbs)) | ||
| for _, header := range lbs { | ||
| result = append(result, header.Number) | ||
| } | ||
| sort.Slice(result, func(i, j int) bool { | ||
| return result[i] < result[j] | ||
| }) | ||
| return result | ||
| } |
There was a problem hiding this comment.
The function BlockNumbers creates a slice with initial capacity of len(lbs), then appends to it, and finally sorts it. However, the sorting is unnecessary if you're just collecting block numbers. Block numbers should already be in the order they appear in the list. If sorting is needed for a specific reason, add a comment explaining why. Additionally, consider if sorted block numbers are always required or if this could be an optional parameter.
- Rename SyncerConfig.ContractsAddr to ContractAddresses for better naming consistency
- Updated all references across multidownloader, l1infotreesync, and types packages
- Updated all unit tests to use new field name
Improvements:
- Add auto-initialization in EVMMultidownloader.Start() when not already initialized
- Adds safety check to call Initialize() if IsInitialized() returns false
- Logs initialization attempt for better observability
- Fix typo in log_query.go panic message: "unsuppoerted" → "unsupported"
- Add nil check for query.ToBlock in NewLogQueryFromEthereumFilter to prevent nil pointer panics
- Fix typo in evm_multidownloader.go log message: "relauncing" → "relaunching"
Database:
- Fix SQL migration 0002.sql:
- Add missing DROP statements for blocks_reorged and reorgs tables in Down migration
- Fix typo in comment: "extran" → "extra"
Types:
- Update BlockHeader.Empty() to only check for nil (removed Number == 0 check)
- Update NewListBlockHeadersEmpty to pre-allocate slice with nil elements
- Add nil checks in ListBlockHeaders.ToMap() and BlockNumbers() methods
- Add clarifying comment to ReorgError about OffendingBlockNumber behavior
Testing:
- Add unit test for Start() auto-initialization error handling
- Re-enable TestEVMMultidownloader_Start with simplified test case
- Update test contracts compilation scripts (logemitter)
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
joanestebanr
force-pushed
the
feature/multidownloader_reorg_unsafe_sync-v2
branch
from
fd74186 to
4b119d1
Compare
|
Claude finished @joanestebanr's task in 2m 10s —— View job Code Review Complete ✅I've completed a thorough review of this PR implementing reorganization handling and unsafe sync capabilities for the multidownloader component. Here's my analysis: 🚀 What's New
🐛 Bug FixesNone - this is a new feature implementation. 📋 Config Updates# New configuration field in multidownloader config:
PeriodToCheckReorgs = "5s" # Default: 5 seconds
# Description: Duration to wait before checking for reorgs
# If set to 0, reorgs are checked only when a new block appears
|
| Category | Count |
|---|---|
| 🔴 Critical Issues | 5 |
| 🟡 Suggested Improvements | 6 |
| 🟢 Minor Issues | 3 |
| ✅ Positive Observations | 6 |
Priority Actions:
- Fix SQL migration table order (blocks PR merge - will fail on SQLite)
- Fix lowest block query (causes incorrect behavior)
- Complete migration rollback (prevents clean downgrades)
- Document or fix genesis block handling (semantic clarity)
- Address panic in production code (production stability)
This is a solid implementation with good architecture. The critical issues are mostly fixable SQL problems that would prevent the migration from working correctly. Once addressed, this PR will provide robust reorg handling for the multidownloader.
🔄 Changes Summary
📋 Config Updates
✅ Testing
🐞 Issues
🔗 Related PRs
📝 Notes