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.
🔄 Changes Summary
📋 Config Updates
✅ Testing
🐞 Issues
🔗 Related PRs
📝 Notes