feat(tools): add legacy data migration subcommand#173
Conversation
Port legacy data migration tool from PR #137 into existing bin/tools as a new 'migrate' subcommand. Migrates data from MDBX to RocksDB + static files for TransactionSenders, AccountChangeSets, Receipts, TransactionHashNumbers, AccountsHistory, and StoragesHistory tables. Key changes: - Add migrate.rs: core migration logic for static files and RocksDB - Add migrate_command.rs: CLI command with options (batch-size, skip-static-files, skip-rocksdb, keep-mdbx) - Add progress.rs: progress logging utilities - Update main.rs: add Migrate variant to Commands enum - Update Cargo.toml: add reth-provider/rocksdb, reth-stages/rocksdb, reth-static-file-types, reth-storage-api dependencies - Update .reth-dev.toml: add reth-stages and reth-static-file-types entries for local dev builds Co-Authored-By: vuicheesiew@gmail.com <vuicheesiew@gmail.com>
|
@claude please review it |
|
@claude please review |
|
@claude review |
|
Claude finished @JimmyShi22's task in 2m 43s —— View job Code Review — feat(tools): add legacy data migration subcommand
SummaryThe PR ports the legacy data migration tool from PR #137 into the existing Critical / High Issues1.
|
|
@review pls comment inline |
|
Claude finished @JimmyShi22's task in 2m 12s —— View job Code Review — feat(tools): add legacy data migration subcommand
13 inline comments posted on the PR covering: Critical / Correctness
Performance
Code Quality
Design / UX
Documentation
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overall structure is sound — parallelizing static files vs RocksDB migrations and using rayon within each is a good design. However there are several correctness concerns, a performance issue, and the destructive-by-default MDBX clearing behavior needs careful review before merging.
| current_block, | ||
| )?; | ||
| } | ||
| if writer.current_block_number().is_some() { |
There was a problem hiding this comment.
Potential correctness bug: ensure_at_block skipped for brand-new writers
When the writer is brand-new (no prior data), current_block_number() returns None, so the ensure_at_block(block.saturating_sub(1)) call is skipped entirely. This means blocks between start and the first entry in the cursor (blocks with no account changesets) will not be padded in the static file.
If the static file writer contract requires all blocks to be explicitly represented from the start, this will produce a corrupted file for chains where early blocks have no state changes. Either call ensure_at_block unconditionally, or explicitly handle the None case to pad from start to block - 1.
| entries_processed += 1; | ||
| } | ||
|
|
||
| if !block_changesets.is_empty() { |
There was a problem hiding this comment.
blocks_processed is not updated for the final block after the loop
After the walk_range loop ends, blocks_processed is never incremented for current_block (the last group of blocks). The final progress logged in the summary will undercount. Consider adding blocks_processed += current_block - start + 1; (or similar) after the loop before the commit step.
| for block in start..=to_block { | ||
| if let Some(body) = provider.block_body_indices(block)? { | ||
| for tx_num in body.first_tx_num..body.first_tx_num + body.tx_count { | ||
| if let Some(receipt) = cursor.seek_exact(tx_num)?.map(|(_, r)| r) { |
There was a problem hiding this comment.
Performance: seek_exact per-tx is O(log N) on monotonically increasing keys
seek_exact performs a key lookup for each individual tx_num. Since the outer block loop iterates sequentially and tx_num is monotonically increasing, this should instead use a sequential cursor walk — seek to body.first_tx_num once, then call cursor.next() for subsequent entries in the range. Using seek_exact per entry here is significantly slower than necessary on large chains.
| let table_start = Instant::now(); | ||
| let mut last_log = Instant::now(); | ||
|
|
||
| let count = match table { |
There was a problem hiding this comment.
Significant code duplication across the three RocksDB table migration arms
The TransactionHashNumbers, AccountsHistory, and StoragesHistory arms are nearly identical (~60 lines repeated 3×): create cursor, create batch, iterate walk(None), put into batch, commit every batch_size, final commit. Any bug fix or enhancement needs to be applied in all three places.
Consider extracting the shared logic into a generic helper — e.g., a macro or a function generic over the table type via trait bounds — so the loop body is written once.
| } | ||
| } | ||
|
|
||
| batch.commit()?; |
There was a problem hiding this comment.
Potentially wasteful empty batch commit
When count % batch_size == 0, the batch was already committed inside the loop and a fresh batch = rocksdb.batch() was created. The unconditional batch.commit() here then commits an empty batch. This is likely a no-op but is wasteful. Guard it: if count % batch_size != 0 { batch.commit()?; }
(Same pattern applies to lines 277 and 300 in the AccountsHistory and StoragesHistory arms.)
|
|
||
| // Check if TransactionSenders actually has data in static files | ||
| let static_file_provider = provider_factory.static_file_provider(); | ||
| let senders_have_data = static_file_provider |
There was a problem hiding this comment.
senders_have_data check may give false negative for genesis-only chains
get_highest_static_file_tx returns None for a chain where the genesis block has zero transactions (i.e., no transactions were ever appended). In that case, TransactionSenders static file storage would not be enabled even though the migration ran correctly. Consider also checking get_highest_static_file_block(StaticFileSegment::TransactionSenders).is_some() as a secondary condition.
| provider.write_storage_settings(new_settings)?; | ||
|
|
||
| // Drop migrated MDBX tables unless --keep-mdbx is set | ||
| if !self.keep_mdbx { |
There was a problem hiding this comment.
Destructive-by-default: MDBX tables are cleared unless --keep-mdbx is passed
For a data migration tool operating on potentially TBs of irreplaceable data, having the default behavior be destructive and irreversible is risky. Users running this in scripts or automation may not realize data will be deleted. Consider inverting the default: keep MDBX tables by default and require an explicit --clear-mdbx flag to delete them. This is a safer UX pattern for one-way destructive operations.
| warn!(target: "reth::cli", "Receipts will NOT be migrated due to contract log pruning"); | ||
| } | ||
|
|
||
| // Start tracking time |
There was a problem hiding this comment.
Redundant comment
// Start tracking time is self-evident from let start_time = Instant::now(). The comment adds no information. Remove it to reduce noise.
| reth-chainspec.workspace = true | ||
| reth-provider.workspace = true | ||
| reth-provider = { workspace = true, features = ["rocksdb"] } | ||
| reth-stages = { workspace = true, features = ["rocksdb"] } |
There was a problem hiding this comment.
Workaround dependency needs an explanatory comment and tracking issue
reth-stages with rocksdb feature is not imported or used anywhere in this crate — it's only here to fix a feature unification compilation error in upstream reth-stages. Without a comment, future maintainers will not know why this is here and may remove it or be confused.
Please add a comment explaining this:
# Workaround: reth-stages must be listed with `rocksdb` feature to fix upstream feature
# unification bug. Not actually imported by this crate. Remove once upstream fixes the issue.
reth-stages = { workspace = true, features = ["rocksdb"] }Ideally reference an upstream tracking issue if one exists.
|
|
||
| if can_migrate_receipts { | ||
| info!(target: "reth::cli", "Clearing Receipts table..."); | ||
| tx.clear::<tables::Receipts<<<N as reth_node_builder::NodeTypes>::Primitives as reth_primitives_traits::NodePrimitives>::Receipt>>()?; |
There was a problem hiding this comment.
Fragile turbofish for Receipts table type — consider a type alias
tx.clear::<tables::Receipts<<<N as reth_node_builder::NodeTypes>::Primitives as reth_primitives_traits::NodePrimitives>::Receipt>>()?;This deeply nested type expression is brittle and hard to read. A type alias at the top of finalize() would improve readability and maintainability:
type ReceiptTable<N> = tables::Receipts<
<<N as reth_node_builder::NodeTypes>::Primitives
as reth_primitives_traits::NodePrimitives>::Receipt
>;
tx.clear::<ReceiptTable<N>>()?;
Vui-Chee
left a comment
Vui-Chee
left a comment
There was a problem hiding this comment.
Devin Review found 1 potential issue.
View 5 additional findings in Devin Review.
| let new_settings = StorageSettings::legacy() | ||
| .with_receipts_in_static_files(can_migrate_receipts) | ||
| .with_account_changesets_in_static_files(true) | ||
| .with_transaction_senders_in_static_files(senders_have_data) | ||
| .with_transaction_hash_numbers_in_rocksdb(true) | ||
| .with_account_history_in_rocksdb(true) | ||
| .with_storages_history_in_rocksdb(true); |
There was a problem hiding this comment.
🔴 Storage settings ignore --skip-static-files and --skip-rocksdb flags, causing data to be read from empty backends
In finalize(), the StorageSettings are built unconditionally enabling all migrated backends (lines 160-166), regardless of whether --skip-static-files or --skip-rocksdb was passed. For example, .with_account_changesets_in_static_files(true) is hardcoded even if static file migration was skipped, and .with_transaction_hash_numbers_in_rocksdb(true) is hardcoded even if RocksDB migration was skipped. While the MDBX clearing code correctly checks these flags (bin/tools/migrate_command.rs:180 and bin/tools/migrate_command.rs:198), the settings themselves don't. After migration with e.g. --skip-rocksdb, the persisted settings would tell the node to look up transaction hash numbers, account history, and storage history from RocksDB—which is empty—instead of MDBX where the data still resides. This would cause lookup failures at runtime.
| let new_settings = StorageSettings::legacy() | |
| .with_receipts_in_static_files(can_migrate_receipts) | |
| .with_account_changesets_in_static_files(true) | |
| .with_transaction_senders_in_static_files(senders_have_data) | |
| .with_transaction_hash_numbers_in_rocksdb(true) | |
| .with_account_history_in_rocksdb(true) | |
| .with_storages_history_in_rocksdb(true); | |
| let new_settings = StorageSettings::legacy() | |
| .with_receipts_in_static_files(!self.skip_static_files && can_migrate_receipts) | |
| .with_account_changesets_in_static_files(!self.skip_static_files) | |
| .with_transaction_senders_in_static_files(!self.skip_static_files && senders_have_data) | |
| .with_transaction_hash_numbers_in_rocksdb(!self.skip_rocksdb) | |
| .with_account_history_in_rocksdb(!self.skip_rocksdb) | |
| .with_storages_history_in_rocksdb(!self.skip_rocksdb); |
Was this helpful? React with 👍 or 👎 to provide feedback.
3 participants
Description
Ports the legacy data migration tool from PR #137 into the existing
bin/toolsbinary as a newmigratesubcommand, rather than adding a separate binary.The
migratecommand moves data from legacy MDBX-only storage into the new RocksDB + static files layout:Static file and RocksDB migrations run in parallel via
std::thread::scope, and segments within each are parallelized withrayon.New files
bin/tools/migrate.rs— core migration logic for static file segments and RocksDB tablesbin/tools/migrate_command.rs— CLI command struct (--batch-size,--skip-static-files,--skip-rocksdb,--keep-mdbx)bin/tools/progress.rs— progress logging with ETACLI flags
--batch-size--skip-static-files--skip-rocksdb--keep-mdbxNotable API adaptations from PR #137
The original PR targeted an older reth revision. Key changes made during porting:
batch_with_auto_commit()→batch()(API no longer exists)set_expected_block_start()— block start is set at construction viaget_writer(genesis_block, segment)StorageSettings::base()→StorageSettings::legacy()StorageChangeSetssegment (no longer exists in this reth revision)Dependency note
Enabled
reth-stageswithrocksdbfeature as a workaround for upstream feature unification issue. Enabling onlyreth-provider/rocksdbcauses compilation error inreth-stages(upstream bug at this reth revision).reth-stagesis not actually imported/used by this code — it's only added to resolve feature flags.Type of Change
Code Guidelines
Relevant guidelines:
Checklist
Testing
cargo c -p xlayer-reth-tools— passes successfully.just ttimed out during workspace-wide compilation. No runtime testing performed. The migration logic is adapted from PR #137 but has not been validated on the current codebase.Additional Notes
Dependency workaround: The
reth-stageswithrocksdbfeature is only added to fix feature unification, not because the tools binary uses it. Is this acceptable?Batch API semantics: Changed from
batch_with_auto_commit()tobatch(). Verify this is correct — the code manually callsbatch.commit()after everybatch_sizeoperations. Doesbatch()require explicit commit, or auto-commit on drop?Default MDBX clearing behavior: By default (
--keep-mdbxNOT set), the tool clears the migrated MDBX tables after migration. This is destructive and irreversible. Users must opt-in to keep MDBX data. Is this the desired UX?No checkpoint/resume for RocksDB migration: RocksDB table migrations start from scratch each time (walk from
None). Static file migration is resumable viaget_highest_static_file_block, but RocksDB migration is all-or-nothing per table. Is this acceptable?Complex type for Receipts table: Line 193 uses a turbofish with nested associated types:
This is fragile. Verify it compiles and is correct.
No tests added: The migration logic has no unit tests. Runtime behavior is untested.
Related