feat: add db migration/patch tool #1908
Conversation
JayT106
requested review from
randy-cro and
thomas-nguy
and removed request for
a team
This comment has been minimized.
This comment has been minimized.
WalkthroughAdd a top-level Changes
Sequence Diagram(s)sequenceDiagram
participant User as CLI User
participant DBcmd as database cmd
participant MigrateCLI as migrate cmd
participant MigrateEngine as dbmigrate.Migrate
participant Src as Source DB
participant Tgt as Target DB
User->>DBcmd: database migrate --source-home ... --target-home ...
DBcmd->>MigrateCLI: validate & forward flags
MigrateCLI->>MigrateEngine: Migrate(MigrateOptions)
MigrateEngine->>Src: open source DB
MigrateEngine->>Tgt: create/open target DB
rect rgb(230,245,255)
loop batched transfer
MigrateEngine->>Src: read batch
MigrateEngine->>Tgt: write batch
MigrateEngine->>MigrateEngine: update stats/log
end
end
opt verify enabled
MigrateEngine->>Tgt: reopen & verify keys
end
MigrateEngine-->>MigrateCLI: return stats
MigrateCLI-->>User: print summary
sequenceDiagram
participant User as CLI User
participant PatchCLI as patch cmd
participant Height as ParseHeightFlag
participant PatchEngine as dbmigrate.PatchDatabase
participant Src as Source DB
participant Tgt as Target DB
User->>PatchCLI: database patch --height 100-200 --database blockstore
PatchCLI->>Height: ParseHeightFlag("100-200")
Height-->>PatchCLI: HeightRange{Start:100,End:200}
PatchCLI->>PatchEngine: PatchDatabase(PatchOptions)
PatchEngine->>Src: open source DB
PatchEngine->>Tgt: open target DB
rect rgb(235,255,235)
loop for height-filtered keys
PatchEngine->>Src: iterate keys
alt key exists in target
PatchEngine->>PatchEngine: resolve conflict (prompt/strategy)
else
PatchEngine->>Tgt: write key (or dry-run)
end
end
end
PatchEngine-->>PatchCLI: return stats
PatchCLI-->>User: print summary
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Files/areas needing extra attention:
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (20)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
Makefile(1 hunks)cmd/cronosd/cmd/database.go(1 hunks)cmd/cronosd/cmd/migrate_db.go(1 hunks)cmd/cronosd/cmd/migrate_db_test.go(1 hunks)cmd/cronosd/cmd/patch_db.go(1 hunks)cmd/cronosd/cmd/root.go(1 hunks)cmd/cronosd/dbmigrate/QUICKSTART.md(1 hunks)cmd/cronosd/dbmigrate/README.md(1 hunks)cmd/cronosd/dbmigrate/height_filter.go(1 hunks)cmd/cronosd/dbmigrate/height_filter_test.go(1 hunks)cmd/cronosd/dbmigrate/height_parse_test.go(1 hunks)cmd/cronosd/dbmigrate/migrate.go(1 hunks)cmd/cronosd/dbmigrate/migrate_basic_test.go(1 hunks)cmd/cronosd/dbmigrate/migrate_dbname_test.go(1 hunks)cmd/cronosd/dbmigrate/migrate_no_rocksdb.go(1 hunks)cmd/cronosd/dbmigrate/migrate_rocksdb.go(1 hunks)cmd/cronosd/dbmigrate/migrate_rocksdb_test.go(1 hunks)cmd/cronosd/dbmigrate/migrate_test.go(1 hunks)cmd/cronosd/dbmigrate/patch.go(1 hunks)cmd/cronosd/dbmigrate/patch_test.go(1 hunks)cmd/cronosd/dbmigrate/swap-migrated-db.sh(1 hunks)x/cronos/keeper/keeper.go(3 hunks)x/cronos/rpc/api.go(2 hunks)x/e2ee/client/cli/encrypt.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-12T22:09:46.096Z
Learnt from: yihuang
Repo: crypto-org-chain/cronos PR: 1618
File: memiavl/db_test.go:199-199
Timestamp: 2024-10-12T22:09:46.096Z
Learning: In unit tests within `memiavl/db_test.go`, converting `int64` to `uint32` is acceptable.
Applied to files:
cmd/cronosd/dbmigrate/migrate_basic_test.gocmd/cronosd/dbmigrate/migrate_test.gocmd/cronosd/dbmigrate/migrate_dbname_test.gocmd/cronosd/dbmigrate/patch_test.gocmd/cronosd/dbmigrate/height_filter_test.gocmd/cronosd/dbmigrate/migrate_rocksdb_test.go
🧬 Code graph analysis (15)
cmd/cronosd/dbmigrate/migrate_rocksdb.go (2)
cmd/cronosd/dbmigrate/migrate_no_rocksdb.go (1)
PrepareRocksDBOptions(13-15)cmd/cronosd/opendb/opendb_rocksdb.go (1)
NewRocksdbOptions(88-138)
cmd/cronosd/cmd/database.go (2)
cmd/cronosd/cmd/migrate_db.go (1)
MigrateCmd(301-306)cmd/cronosd/cmd/patch_db.go (1)
PatchCmd(306-311)
cmd/cronosd/cmd/migrate_db_test.go (1)
cmd/cronosd/cmd/migrate_db.go (3)
DBTypeApp(28-28)DBTypeCometBFT(29-29)DBTypeAll(30-30)
cmd/cronosd/dbmigrate/height_parse_test.go (1)
cmd/cronosd/dbmigrate/height_filter.go (2)
HeightRange(20-24)ParseHeightFlag(124-151)
cmd/cronosd/dbmigrate/migrate_no_rocksdb.go (1)
cmd/cronosd/dbmigrate/migrate_rocksdb.go (1)
PrepareRocksDBOptions(14-17)
cmd/cronosd/dbmigrate/migrate_basic_test.go (2)
cmd/cronosd/dbmigrate/migrate_test.go (7)
TestCountKeys(60-88)TestMigrationStats(91-106)TestMigrateLargeDatabase(109-140)TestMigrateEmptyDatabase(143-167)TestMigrationWithoutVerification(170-196)TestMigrationBatchSizes(199-229)TestMigrateSpecialKeys(334-423)cmd/cronosd/dbmigrate/migrate.go (3)
MigrateOptions(23-45)Migrate(74-233)MigrationStats(48-54)
cmd/cronosd/dbmigrate/migrate_test.go (2)
cmd/cronosd/dbmigrate/migrate_basic_test.go (7)
TestCountKeys(41-69)TestMigrationStats(102-117)TestMigrateLargeDatabase(120-151)TestMigrateEmptyDatabase(154-178)TestMigrationWithoutVerification(181-207)TestMigrationBatchSizes(210-240)TestMigrateSpecialKeys(243-288)cmd/cronosd/dbmigrate/migrate.go (3)
MigrationStats(48-54)MigrateOptions(23-45)Migrate(74-233)
cmd/cronosd/cmd/patch_db.go (5)
memiavl/types.go (1)
Logger(6-10)cmd/cronosd/dbmigrate/height_filter.go (2)
ParseHeightFlag(124-151)HeightRange(20-24)cmd/cronosd/dbmigrate/migrate_no_rocksdb.go (1)
PrepareRocksDBOptions(13-15)cmd/cronosd/dbmigrate/migrate_rocksdb.go (1)
PrepareRocksDBOptions(14-17)cmd/cronosd/dbmigrate/patch.go (3)
PatchOptions(46-59)ConflictAsk(36-36)PatchDatabase(62-170)
cmd/cronosd/cmd/migrate_db.go (4)
cmd/cronosd/dbmigrate/height_filter.go (2)
ParseHeightFlag(124-151)HeightRange(20-24)cmd/cronosd/dbmigrate/migrate_no_rocksdb.go (1)
PrepareRocksDBOptions(13-15)cmd/cronosd/dbmigrate/migrate_rocksdb.go (1)
PrepareRocksDBOptions(14-17)cmd/cronosd/dbmigrate/migrate.go (4)
MigrationStats(48-54)MigrateOptions(23-45)Migrate(74-233)DefaultBatchSize(17-17)
cmd/cronosd/dbmigrate/migrate_dbname_test.go (1)
cmd/cronosd/dbmigrate/migrate.go (2)
MigrateOptions(23-45)Migrate(74-233)
cmd/cronosd/cmd/root.go (1)
cmd/cronosd/cmd/database.go (1)
DatabaseCmd(8-29)
cmd/cronosd/dbmigrate/migrate.go (2)
memiavl/types.go (2)
Logger(6-10)NewNopLogger(18-18)cmd/cronosd/dbmigrate/height_filter.go (3)
HeightRange(20-24)DBNameBlockstore(14-14)DBNameTxIndex(15-15)
cmd/cronosd/dbmigrate/height_filter_test.go (1)
cmd/cronosd/dbmigrate/height_filter.go (1)
HeightRange(20-24)
cmd/cronosd/dbmigrate/patch.go (3)
memiavl/types.go (1)
Logger(6-10)cmd/cronosd/dbmigrate/height_filter.go (3)
HeightRange(20-24)DBNameBlockstore(14-14)DBNameTxIndex(15-15)cmd/cronosd/dbmigrate/migrate.go (1)
MigrationStats(48-54)
cmd/cronosd/dbmigrate/migrate_rocksdb_test.go (2)
cmd/cronosd/dbmigrate/migrate.go (2)
MigrateOptions(23-45)Migrate(74-233)memiavl/types.go (2)
Logger(6-10)NewNopLogger(18-18)
🪛 Gitleaks (8.29.0)
cmd/cronosd/dbmigrate/README.md
[high] 941-941: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 LanguageTool
cmd/cronosd/dbmigrate/README.md
[style] ~493-~493: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...xample 6: Large Database Migration For very large databases, disable verification for fas...
(EN_WEAK_ADJECTIVE)
[grammar] ~1043-~1043: Ensure spelling is correct
Context: ... Safety and Best Practices #### Always Backup First ```bash # Timestamp your backups...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[uncategorized] ~1168-~1168: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ase content and paths. 5. "Failed to open source database" ``` Error: failed to open ...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
cmd/cronosd/dbmigrate/QUICKSTART.md
[grammar] ~51-~51: Ensure spelling is correct
Context: ...# or manually pkill cronosd ### 2. Backup Your Databases bash # Backup applic...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~494-~494: Ensure spelling is correct
Context: ...| HDD (100MB/s) | SSD (500MB/s) | NVMe (3GB/s) | |--------------|---------------|--...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
cmd/cronosd/dbmigrate/README.md
268-268: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
583-583: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
663-663: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
677-677: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
704-704: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
714-714: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
729-729: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
737-737: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
744-744: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
759-759: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
765-765: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
769-769: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
781-781: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
929-929: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
935-935: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1088-1088: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1114-1114: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1116-1116: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1130-1130: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1132-1132: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1142-1142: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1144-1144: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1155-1155: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1157-1157: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1168-1168: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1170-1170: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1218-1218: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1235-1235: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1277-1277: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1298-1298: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
cmd/cronosd/dbmigrate/QUICKSTART.md
126-126: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
138-138: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
355-355: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
360-360: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
750-750: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
756-756: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
762-762: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
768-768: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
794-794: Bare URL used
(MD034, no-bare-urls)
🪛 Shellcheck (0.11.0)
cmd/cronosd/dbmigrate/swap-migrated-db.sh
[warning] 212-212: FAILED_COUNT appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 345-345: Don't use ls | grep. Use a glob or a for loop with a condition to allow non-alphanumeric filenames.
(SC2010)
⏰ 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). (18)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (slow)
- GitHub Check: Run golangci-lint
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: Analyze (go)
- GitHub Check: unittest
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: semgrep-cloud-platform/scan
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
cmd/cronosd/dbmigrate/migrate_rocksdb.go (1)
60-68: Remove premature cleanup of read/write options before returning.This is the same critical issue from the previous review: the
defer Destroy()calls on lines 61, 63, and 65 will destroy the read/write options immediately when the function returns, while the returneddbm.RocksDBwrapper still references them. Any subsequent read/write operation will trigger use-after-free.Apply this diff to remove the premature cleanup:
ro := grocksdb.NewDefaultReadOptions() - defer ro.Destroy() wo := grocksdb.NewDefaultWriteOptions() - defer wo.Destroy() woSync := grocksdb.NewDefaultWriteOptions() - defer woSync.Destroy() woSync.SetSync(true) return dbm.NewRocksDBWithRawDB(db, ro, wo, woSync), nilThe caller or the
dbm.RocksDB.Close()method must be responsible for destroying these options when the database is closed.cmd/cronosd/dbmigrate/migrate_rocksdb_test.go (1)
31-31: Reduce RocksDB cache allocation to prevent OOM in tests.This is the same issue from the previous review: allocating a 3GB LRU cache for every test will exhaust memory in CI and on developer machines. Test fixtures only need tens of megabytes to validate functionality.
Apply this diff to use a test-appropriate cache size:
- bbto.SetBlockCache(grocksdb.NewLRUCache(3 << 30)) // 3GB + bbto.SetBlockCache(grocksdb.NewLRUCache(64 << 20)) // 64MB
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Makefile(1 hunks)cmd/cronosd/cmd/root.go(1 hunks)cmd/cronosd/dbmigrate/migrate_rocksdb.go(1 hunks)cmd/cronosd/dbmigrate/migrate_rocksdb_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/cronosd/cmd/root.go
- Makefile
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-04T16:09:08.694Z
Learnt from: yihuang
Repo: crypto-org-chain/cronos PR: 1618
File: memiavl/db_test.go:199-199
Timestamp: 2024-10-04T16:09:08.694Z
Learning: In unit tests within `memiavl/db_test.go`, converting `int64` to `uint32` is acceptable.
Applied to files:
cmd/cronosd/dbmigrate/migrate_rocksdb_test.go
🧬 Code graph analysis (2)
cmd/cronosd/dbmigrate/migrate_rocksdb_test.go (2)
cmd/cronosd/dbmigrate/migrate.go (2)
MigrateOptions(23-45)Migrate(74-233)memiavl/types.go (2)
Logger(6-10)NewNopLogger(18-18)
cmd/cronosd/dbmigrate/migrate_rocksdb.go (2)
cmd/cronosd/dbmigrate/migrate_no_rocksdb.go (1)
PrepareRocksDBOptions(13-15)cmd/cronosd/opendb/opendb_rocksdb.go (1)
NewRocksdbOptions(88-138)
⏰ 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). (17)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: unittest
- GitHub Check: Run golangci-lint
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (ibc)
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
cmd/cronosd/dbmigrate/migrate_rocksdb.go (1)
72-82: LGTM!The flush implementation correctly creates and destroys flush options locally, performs a synchronized flush when the DB is RocksDB, and safely returns nil for non-RocksDB instances.
cmd/cronosd/dbmigrate/migrate_rocksdb_test.go (2)
47-77: LGTM!The helper properly manages RocksDB resources by registering cleanup callbacks for all options objects. Test data population is straightforward and effective.
80-335: LGTM!The test suite provides comprehensive coverage of RocksDB migration scenarios:
- Cross-backend migrations (LevelDB ↔ RocksDB)
- In-place RocksDB migration
- Large dataset stress testing
- Custom options
- Full data integrity verification
All tests properly manage resources and use appropriate verification strategies.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1908 +/- ##
===========================================
+ Coverage 16.87% 37.13% +20.25%
===========================================
Files 72 127 +55
Lines 6163 9757 +3594
===========================================
+ Hits 1040 3623 +2583
- Misses 5000 5713 +713
- Partials 123 421 +298 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
cmd/cronosd/dbmigrate/migrate_rocksdb.go (2)
14-17: Fix the misleading comment about the second parameter.The comment incorrectly states "false: disable read-only mode," but the second parameter to
NewRocksdbOptionsissstFileWriter bool, not a read-only flag.Apply this diff to correct the comment:
func PrepareRocksDBOptions() interface{} { - // nil: use default options, false: disable read-only mode + // nil: use default options, false: sstFileWriter (not using SST file writer mode) return opendb.NewRocksdbOptions(nil, false) }
33-37: Resource leak: Default options are never destroyed.When
optsis nil, this code creates defaultgrocksdb.Optionsbut never destroys them, leaking native resources. RocksDB options must be explicitly freed.The cleanest fix is to require the caller to provide options:
// Handle nil opts by creating default options if opts == nil { - opts = grocksdb.NewDefaultOptions() - opts.SetCreateIfMissing(true) - opts.SetLevelCompactionDynamicLevelBytes(true) + return nil, fmt.Errorf("RocksDB options must be provided via PrepareRocksDBOptions()") }
🧹 Nitpick comments (1)
cmd/cronosd/dbmigrate/swap-migrated-db.sh (1)
345-350: Prefer a for loop overls | greppattern.The
ls | greppattern on line 347 can fail with non-alphanumeric filenames and is flagged by shellcheck (SC2010). Use a for loop with a glob pattern for robustness:-print_info "Current data directory contents:" -ls -lh "$DATA_DIR" | grep -E "^d" | awk '{print " " $9 " (" $5 ")"}' +print_info "Current data directory contents:" +for dir in "$DATA_DIR"/*/; do + if [[ -d "$dir" ]]; then + size=$(du -sh "$dir" 2>/dev/null | awk '{print $1}') + echo " $(basename "$dir") ($size)" + fi +done
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmd/cronosd/dbmigrate/migrate.go(1 hunks)cmd/cronosd/dbmigrate/migrate_basic_test.go(1 hunks)cmd/cronosd/dbmigrate/migrate_rocksdb.go(1 hunks)cmd/cronosd/dbmigrate/migrate_rocksdb_test.go(1 hunks)cmd/cronosd/dbmigrate/swap-migrated-db.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/cronosd/dbmigrate/migrate_basic_test.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-04T16:09:08.694Z
Learnt from: yihuang
Repo: crypto-org-chain/cronos PR: 1618
File: memiavl/db_test.go:199-199
Timestamp: 2024-10-04T16:09:08.694Z
Learning: In unit tests within `memiavl/db_test.go`, converting `int64` to `uint32` is acceptable.
Applied to files:
cmd/cronosd/dbmigrate/migrate_rocksdb_test.go
🧬 Code graph analysis (3)
cmd/cronosd/dbmigrate/migrate.go (2)
cmd/cronosd/dbmigrate/height_filter.go (3)
HeightRange(20-24)DBNameBlockstore(14-14)DBNameTxIndex(15-15)memiavl/db.go (2)
Load(146-266)DB(44-86)
cmd/cronosd/dbmigrate/migrate_rocksdb.go (2)
cmd/cronosd/dbmigrate/migrate_no_rocksdb.go (1)
PrepareRocksDBOptions(13-15)cmd/cronosd/opendb/opendb_rocksdb.go (1)
NewRocksdbOptions(88-138)
cmd/cronosd/dbmigrate/migrate_rocksdb_test.go (2)
cmd/cronosd/dbmigrate/migrate.go (2)
MigrateOptions(24-46)Migrate(75-241)memiavl/types.go (2)
Logger(6-10)NewNopLogger(18-18)
🪛 Shellcheck (0.11.0)
cmd/cronosd/dbmigrate/swap-migrated-db.sh
[warning] 214-214: FAILED_COUNT appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 347-347: Don't use ls | grep. Use a glob or a for loop with a condition to allow non-alphanumeric filenames.
(SC2010)
⏰ 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). (18)
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: unittest
- GitHub Check: build (macos-14)
- GitHub Check: Run golangci-lint
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc)
- GitHub Check: Analyze (go)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (12)
cmd/cronosd/dbmigrate/swap-migrated-db.sh (1)
203-210: Good fix: dry-run mode is now side-effect free.The backup directory creation is now properly gated behind the
DRY_RUNcheck, ensuring that--dry-runremains non-mutating as intended. This addresses the previous concern.cmd/cronosd/dbmigrate/migrate_rocksdb.go (3)
64-70: LGTM: Defer removal fixes use-after-free.The removal of
defercalls onro,wo, andwoSynccorrectly allowsdbm.NewRocksDBWithRawDBto take ownership of these resources. Theopts.Destroy()defer at line 58 is appropriate sinceOpenDbForReadOnlydoesn't take ownership of the options.
72-83: LGTM: Flush implementation is correct.The function properly:
- Type-asserts to access the underlying RocksDB instance
- Creates flush options with correct cleanup via defer
- Synchronously flushes memtables by setting
Wait(true)- Safely handles non-RocksDB instances
39-52: Verify resource ownership semantics with cosmos-db library.The code assumes
dbm.NewRocksDBWithRawDB(fromgithub.com/cosmos/cosmos-db) takes ownership of theReadOptionsandWriteOptionspassed to it, destroying them whenClose()is called. This pattern is consistent across the codebase. Please confirm this assumption by reviewing the cosmos-db library documentation or implementation to ensure the options are properly freed and there are no resource leaks.cmd/cronosd/dbmigrate/migrate.go (5)
58-64: Non-determinism from floating-point arithmetic is acceptable here.The security scanner flags floating-point division as a source of non-determinism. However, this is for progress reporting only and doesn't affect consensus or data integrity. The non-determinism is acceptable in this context.
86-88: time.Now() usage for metrics is acceptable.The security scanner flags
time.Now()calls as sources of non-determinism. However, these calls are used exclusively for progress tracking, logging, and performance metrics, not for consensus or data integrity. The non-determinism is acceptable in this operational context.
514-600: Verification correctly honors height range filtering.The verification logic properly implements height-range filtering:
- Checks if height filtering is needed (line 514)
- Uses bounded iterators for the specified database type (lines 525-537)
- Applies additional filtering for specific heights (lines 555-559)
- Compares only the keys that should have been migrated
This addresses the past concern about verification ignoring the requested height range.
360-425: Height filtering logic looks correct.The migration iterator properly handles height filtering in two stages:
- Bounded iterators narrow the range to min/max heights (via
getBlockstoreIterators/getTxIndexIterator)- Additional filtering at lines 374-378 ensures only specific heights are migrated when discrete heights are specified
This two-stage approach efficiently handles both range-based and discrete height selections.
427-453: LGTM: Retry logic handles OS file lock delays.The exponential backoff retry logic (50ms → 100ms → 200ms → 400ms → 800ms) appropriately handles OS-level file lock delays that can occur after database closure. This is particularly important for the verification phase where databases are reopened immediately after closing.
cmd/cronosd/dbmigrate/migrate_rocksdb_test.go (3)
20-44: LGTM: Test cache size is appropriate.The 64MB cache size is appropriate for tests, avoiding the OOM issues that would occur with a production-sized cache.
46-77: LGTM: Test setup properly manages resources.The test helper correctly:
- Uses
t.TempDir()for automatic cleanup- Registers cleanup callbacks for RocksDB options via
t.Cleanup()- Properly constructs RocksDB instances for testing
277-335: LGTM: Comprehensive data integrity test.This test provides thorough validation by:
- Capturing all source data before migration
- Performing byte-level comparison of every key-value pair
- Verifying the count matches expectations
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/cronosd/dbmigrate/migrate.go (1)
19-20: Remove unused exported constant.
DefaultWorkersis exported as part of the public API but is never referenced in the codebase. The migration runs single-threaded without worker pools.Apply this diff if concurrent workers are not planned:
- // DefaultWorkers is the number of concurrent workers for migration - DefaultWorkers = 4Otherwise, document its intended future use.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/cronosd/dbmigrate/migrate.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/cronosd/dbmigrate/migrate.go (3)
memiavl/types.go (2)
Logger(6-10)NewNopLogger(18-18)cmd/cronosd/dbmigrate/height_filter.go (3)
HeightRange(20-24)DBNameBlockstore(14-14)DBNameTxIndex(15-15)memiavl/db.go (2)
Load(146-266)DB(44-86)
⏰ 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). (19)
- GitHub Check: Run golangci-lint
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-14)
- GitHub Check: unittest
- GitHub Check: build (macos-latest)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: Analyze (go)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (6)
cmd/cronosd/dbmigrate/migrate.go (6)
58-64: Floating-point progress calculation is acceptable here.Static analysis flagged the floating-point division as non-deterministic. For an offline migration tool's progress reporting, this is not a consensus concern and the implementation is appropriate.
183-240: Migration and verification flow is well-structured.The implementation correctly:
- Applies height filtering when specified (via
shouldIncludeKeyin helper functions)- Flushes RocksDB memtable before verification
- Releases locks by closing DBs before verification
- Provides clear instructions for finalizing the migration
Past concerns about height-range filtering in verification appear to be properly addressed.
257-427: Height filtering and migration logic correctly implemented.The code properly addresses the past concern about discrete height ranges by applying
shouldIncludeKeyfiltering in both:
- Line 294:
countKeysWithHeightFilter- ensures accurate counts- Line 377:
migrateWithIterator- ensures only requested keys are migratedThis prevents copying keys between min/max bounds that weren't explicitly requested.
429-482: Retry logic with exponential backoff is well-designed.The retry mechanisms handle OS-level file lock delays that can occur after database closure. The exponential backoff (50ms → 800ms over 5 attempts) is appropriate for this use case.
484-730: Comprehensive two-phase verification properly handles height filtering.The verification correctly:
- Applies height filtering in both phases (lines 557, 669) to avoid false missing-key errors
- Compares source → target (phase 1) and target → source (phase 2) to detect both missing and extra keys
- Reports detailed progress and mismatches
Multiple
time.Now()calls flagged by static analysis are acceptable for this offline migration tool's progress reporting.
115-126: Add nil check in defer to prevent panic.If
NewDBfails and returns a nil database, the deferred cleanup attempts to callClose()on a nil receiver, which may panic depending on the implementation.Apply this diff:
sourceDBClosed := false defer func() { - if !sourceDBClosed { + if !sourceDBClosed && sourceDB != nil { sourceDB.Close() } }()Likely an incorrect or invalid review comment.
Docstrings generation was requested by @JayT106. * #1908 (comment) The following files were modified: * `cmd/cronosd/cmd/database.go` * `cmd/cronosd/cmd/migrate_db.go` * `cmd/cronosd/cmd/patch_db.go` * `cmd/cronosd/cmd/root.go` * `cmd/cronosd/dbmigrate/height_filter.go` * `cmd/cronosd/dbmigrate/migrate.go` * `cmd/cronosd/dbmigrate/migrate_no_rocksdb.go` * `cmd/cronosd/dbmigrate/migrate_rocksdb.go` * `cmd/cronosd/dbmigrate/patch.go` * `cmd/cronosd/dbmigrate/swap-migrated-db.sh` * `x/e2ee/client/cli/encrypt.go`
|
@coderabbitai re-review |
|
@JayT106: I'll perform a comprehensive re-review of all the changes in this PR. ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
x/cronos/keeper/keeper.go (2)
152-162: Explicit return is good, but iterator must be closed.The explicit return improves clarity. However, the iterator created on line 154 is never closed, causing a resource leak.
Apply this diff to close the iterator:
func (k Keeper) GetExternalContracts(ctx sdk.Context) (out []types.TokenMapping) { store := ctx.KVStore(k.storeKey) iter := prefix.NewStore(store, types.KeyPrefixDenomToExternalContract).Iterator(nil, nil) + defer iter.Close() for ; iter.Valid(); iter.Next() { out = append(out, types.TokenMapping{ Denom: string(iter.Key()), Contract: common.BytesToAddress(iter.Value()).Hex(), }) } return out }
165-175: Explicit return is good, but iterator must be closed.The explicit return improves clarity. However, the iterator created on line 167 is never closed, causing a resource leak.
Apply this diff to close the iterator:
func (k Keeper) GetAutoContracts(ctx sdk.Context) (out []types.TokenMapping) { store := ctx.KVStore(k.storeKey) iter := prefix.NewStore(store, types.KeyPrefixDenomToAutoContract).Iterator(nil, nil) + defer iter.Close() for ; iter.Valid(); iter.Next() { out = append(out, types.TokenMapping{ Denom: string(iter.Key()), Contract: common.BytesToAddress(iter.Value()).Hex(), }) } return out }
♻️ Duplicate comments (2)
cmd/cronosd/dbmigrate/migrate_rocksdb.go (1)
14-16: Comment needs correction per past review.The inline comment states "false: disable read-only mode," but according to past review feedback, the second parameter to
NewRocksdbOptionsissstFileWriter bool, not a read-only mode flag.cmd/cronosd/dbmigrate/patch.go (1)
194-199: Define aminhelper (the build still fails).
min(...)is not defined anywhere (Go has no builtin), so this file will not compile. Please add a small helper and use it here.- logger.Debug("Blockstore key found", - "key_prefix", string(it.Key()[:min(10, len(it.Key()))]), - "key_hex", fmt.Sprintf("%x", it.Key()[:min(20, len(it.Key()))]), + logger.Debug("Blockstore key found", + "key_prefix", string(it.Key()[:minInt(10, len(it.Key()))]), + "key_hex", fmt.Sprintf("%x", it.Key()[:minInt(20, len(it.Key()))]),+func minInt(a, b int) int { + if a < b { + return a + } + return b +}
🧹 Nitpick comments (4)
Makefile (1)
116-116: Remove unnecessary trailing semicolons in per-target test commands.The per-target test commands (test-memiavl, test-store, test-versiondb) use
&&correctly for error propagation. However, the trailing;characters at the end of each line are unnecessary in Makefile recipes—they create empty commands that add no value. This also conflicts with the AI summary's claim that "trailing semicolons" were removed.Apply this diff to clean up the trailing semicolons:
test-memiavl: - @cd memiavl && go test -tags=objstore -v -mod=readonly ./... -coverprofile=$(COVERAGE) -covermode=atomic; + @cd memiavl && go test -tags=objstore -v -mod=readonly ./... -coverprofile=$(COVERAGE) -covermode=atomic test-store: - @cd store && go test -tags=objstore -v -mod=readonly ./... -coverprofile=$(COVERAGE) -covermode=atomic; + @cd store && go test -tags=objstore -v -mod=readonly ./... -coverprofile=$(COVERAGE) -covermode=atomic test-versiondb: - @cd versiondb && go test -tags=objstore,rocksdb -v -mod=readonly ./... -coverprofile=$(COVERAGE) -covermode=atomic; + @cd versiondb && go test -tags=objstore,rocksdb -v -mod=readonly ./... -coverprofile=$(COVERAGE) -covermode=atomicAlso applies to: 119-119, 122-122
cmd/cronosd/dbmigrate/migrate_rocksdb.go (1)
33-38: Consider requiring caller to provide RocksDB options.The fallback path creates default options when
opts == nil, but this introduces hidden resource allocation that the caller may not be aware of. SincePrepareRocksDBOptions()is provided for obtaining options, it might be clearer to require non-nil options and return an error if nil, forcing the caller to explicitly prepare options via the exported function.If you choose to keep the fallback, the current implementation with the
createdOptsflag and deferred cleanup is correct.cmd/cronosd/dbmigrate/README.md (1)
268-268: Consider adding language specifiers to code blocks.Multiple fenced code blocks lack language specifiers, which would enable syntax highlighting. While not critical, adding appropriate language tags (e.g.,
bash,go, ```text) would improve readability.Also applies to: 583-583, 663-663, 677-677, 704-704, 714-714, 729-729, 737-737, 744-744, 781-781, 929-929, 935-935, 1088-1088, 1116-1116, 1132-1132, 1144-1144, 1157-1157, 1170-1170, 1277-1277, 1298-1298
cmd/cronosd/dbmigrate/swap-migrated-db.sh (1)
348-348: Consider alternative tols | grepfor robustness.Shellcheck warns that piping
lstogrepcan break with non-alphanumeric filenames. While this is unlikely to cause issues in this context (database directory names), a more robust approach would use a glob pattern or a for loop with test conditions.Alternative implementation:
echo "" print_info "Current data directory contents:" for dir in "$DATA_DIR"/*; do if [[ -d "$dir" ]]; then size=$(du -sh "$dir" 2>/dev/null | awk '{print $1}') basename=$(basename "$dir") echo " $basename ($size)" fi done
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
CHANGELOG.md(1 hunks)Makefile(1 hunks)cmd/cronosd/cmd/database.go(1 hunks)cmd/cronosd/cmd/migrate_db.go(1 hunks)cmd/cronosd/cmd/migrate_db_test.go(1 hunks)cmd/cronosd/cmd/patch_db.go(1 hunks)cmd/cronosd/cmd/root.go(1 hunks)cmd/cronosd/dbmigrate/QUICKSTART.md(1 hunks)cmd/cronosd/dbmigrate/README.md(1 hunks)cmd/cronosd/dbmigrate/height_filter.go(1 hunks)cmd/cronosd/dbmigrate/height_filter_test.go(1 hunks)cmd/cronosd/dbmigrate/height_parse_test.go(1 hunks)cmd/cronosd/dbmigrate/migrate.go(1 hunks)cmd/cronosd/dbmigrate/migrate_basic_test.go(1 hunks)cmd/cronosd/dbmigrate/migrate_dbname_test.go(1 hunks)cmd/cronosd/dbmigrate/migrate_no_rocksdb.go(1 hunks)cmd/cronosd/dbmigrate/migrate_rocksdb.go(1 hunks)cmd/cronosd/dbmigrate/migrate_rocksdb_test.go(1 hunks)cmd/cronosd/dbmigrate/migrate_test.go(1 hunks)cmd/cronosd/dbmigrate/patch.go(1 hunks)cmd/cronosd/dbmigrate/patch_test.go(1 hunks)cmd/cronosd/dbmigrate/swap-migrated-db.sh(1 hunks)x/cronos/keeper/keeper.go(3 hunks)x/cronos/rpc/api.go(2 hunks)x/e2ee/client/cli/encrypt.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-12T22:09:46.096Z
Learnt from: yihuang
Repo: crypto-org-chain/cronos PR: 1618
File: memiavl/db_test.go:199-199
Timestamp: 2024-10-12T22:09:46.096Z
Learning: In unit tests within `memiavl/db_test.go`, converting `int64` to `uint32` is acceptable.
Applied to files:
cmd/cronosd/dbmigrate/migrate_test.gocmd/cronosd/dbmigrate/height_filter_test.gocmd/cronosd/dbmigrate/migrate_rocksdb_test.gocmd/cronosd/dbmigrate/migrate_dbname_test.gocmd/cronosd/dbmigrate/patch_test.gocmd/cronosd/dbmigrate/migrate_basic_test.go
🪛 GitHub Check: CodeQL
cmd/cronosd/dbmigrate/migrate.go
[warning] 522-522: Calling the system time
Calling the system time may be a possible source of non-determinism
[warning] 585-585: Calling the system time
Calling the system time may be a possible source of non-determinism
[warning] 644-644: Calling the system time
Calling the system time may be a possible source of non-determinism
cmd/cronosd/dbmigrate/patch.go
[warning] 77-77: Calling the system time
Calling the system time may be a possible source of non-determinism
[warning] 168-168: Calling the system time
Calling the system time may be a possible source of non-determinism
[warning] 695-828: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 859-859: Calling the system time
Calling the system time may be a possible source of non-determinism
[warning] 1021-1021: Calling the system time
Calling the system time may be a possible source of non-determinism
[notice] 1013-1013: Floating point arithmetic
Floating point arithmetic operations are not associative and a possible source of non-determinism
[notice] 1234-1234: Floating point arithmetic
Floating point arithmetic operations are not associative and a possible source of non-determinism
🪛 Gitleaks (8.29.0)
cmd/cronosd/dbmigrate/README.md
[high] 941-941: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 LanguageTool
cmd/cronosd/dbmigrate/QUICKSTART.md
[grammar] ~51-~51: Ensure spelling is correct
Context: ...# or manually pkill cronosd ### 2. Backup Your Databases bash # Backup applic...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~494-~494: Ensure spelling is correct
Context: ...| HDD (100MB/s) | SSD (500MB/s) | NVMe (3GB/s) | |--------------|---------------|--...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
cmd/cronosd/dbmigrate/README.md
[style] ~493-~493: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...xample 6: Large Database Migration For very large databases, disable verification for fas...
(EN_WEAK_ADJECTIVE)
[grammar] ~1043-~1043: Ensure spelling is correct
Context: ... Safety and Best Practices #### Always Backup First ```bash # Timestamp your backups...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[uncategorized] ~1168-~1168: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ase content and paths. 5. "Failed to open source database" ``` Error: failed to open ...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🪛 markdownlint-cli2 (0.18.1)
cmd/cronosd/dbmigrate/QUICKSTART.md
126-126: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
138-138: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
355-355: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
360-360: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
750-750: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
756-756: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
762-762: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
768-768: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
794-794: Bare URL used
(MD034, no-bare-urls)
cmd/cronosd/dbmigrate/README.md
268-268: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
583-583: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
663-663: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
677-677: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
704-704: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
714-714: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
729-729: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
737-737: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
744-744: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
759-759: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
765-765: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
769-769: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
781-781: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
929-929: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
935-935: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1088-1088: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1114-1114: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1116-1116: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1130-1130: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1132-1132: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1142-1142: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1144-1144: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1155-1155: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1157-1157: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1168-1168: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1170-1170: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1218-1218: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1235-1235: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1277-1277: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1298-1298: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Shellcheck (0.11.0)
cmd/cronosd/dbmigrate/swap-migrated-db.sh
[warning] 348-348: Don't use ls | grep. Use a glob or a for loop with a condition to allow non-alphanumeric filenames.
(SC2010)
⏰ 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). (16)
- GitHub Check: unittest
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Run golangci-lint
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (ibc)
🔇 Additional comments (24)
CHANGELOG.md (1)
5-5: Changelog entry is well‑formed and properly positioned.The new entry for PR #1908 follows the established format, contains an accurate description, and is correctly placed in the UNRELEASED section at the top of the changelog.
Makefile (1)
112-113: Test target simplified to single invocation—verify coverage behavior.The test target now runs all packages in a single
go testinvocation rather than as separate target dependencies. This is cleaner and should execute faster. Ensure that the unified coverage profile ($(COVERAGE)) correctly captures coverage across all packages as expected by CI/reporting tools.x/e2ee/client/cli/encrypt.go (1)
92-105: LGTM - explicit return improves clarity.The change from a bare
returntoreturn erris semantically equivalent with named returns but makes the intent more explicit. The error-handling pattern witherrors.Joinin the deferred function correctly captures bothio.Copyandw.Close()errors.x/cronos/rpc/api.go (2)
101-114: Explicit returns improve code clarity.The change from bare returns to explicit return statements makes the control flow clearer and easier to understand. Returning accumulated values on error paths is an interesting pattern that preserves partial context, though current callers don't use them (they check
errfirst).
387-387: LGTM: Explicit return improves readability.Making the return statement explicit rather than relying on named return values improves code clarity.
x/cronos/keeper/keeper.go (1)
113-119: LGTM: Explicit return improves clarity.The change from implicit to explicit return makes the function's intent clearer while maintaining correct logic.
cmd/cronosd/dbmigrate/migrate_rocksdb.go (2)
65-80: LGTM!The resource lifecycle management is correct. The database options are destroyed after opening (since RocksDB copies them internally), while the read/write options are passed to the wrapper and will be cleaned up when the database is closed.
82-93: LGTM!The flush logic correctly handles the RocksDB-specific case with proper resource cleanup for the flush options. The no-op behavior for non-RocksDB instances is appropriate.
cmd/cronosd/dbmigrate/README.md (2)
941-941: False positive: Example transaction hash, not a secret.The Gitleaks warning flagging line 941 is a false positive. The hex string shown is example debug output illustrating the format of a transaction hash in the documentation, not an actual API key or secret.
1-1360: Excellent comprehensive documentation.This README provides thorough documentation covering:
- Clear usage examples for both migrate and patch commands
- Detailed architecture and implementation notes
- Height filtering with bounded iterator optimization explained
- CometBFT key format documentation
- Safety guidelines and troubleshooting
The documentation quality is excellent and will help users understand and use these tools effectively.
cmd/cronosd/dbmigrate/migrate_no_rocksdb.go (1)
1-32: LGTM!The stub implementations correctly handle the case when RocksDB support is not compiled in. Error messages are clear and instruct users to rebuild with
-tags rocksdb, which is helpful.cmd/cronosd/dbmigrate/swap-migrated-db.sh (3)
205-212: LGTM!The dry-run handling correctly prevents side effects. The backup directory is only created when not in dry-run mode, addressing the previous review concern.
15-40: Excellent input validation.The
validate_backup_suffixfunction properly sanitizes and validates the backup suffix using a character whitelist. This prevents potential path traversal or injection issues. The immediate validation of both default and user-provided values is a good security practice.
256-294: LGTM!The swap logic correctly handles both existing and missing original databases, provides clear logging for both dry-run and actual execution, and properly tracks success counts.
cmd/cronosd/cmd/root.go (1)
196-199: LGTM!The integration of
DatabaseCmd()follows the existing pattern used forChangeSetCmd()and cleanly adds the new database management commands to the root command. The nil check allows for conditional compilation scenarios.cmd/cronosd/cmd/database.go (1)
7-29: LGTM!The
DatabaseCmd()function cleanly defines the database command group with a helpful alias and properly wires the migrate and patch subcommands. The documentation in the Long field clearly describes the available subcommands.cmd/cronosd/dbmigrate/patch_test.go (3)
13-42: LGTM!The
TestIncrementBytestest covers important cases including simple increment, carry-over propagation, and the all-FF edge case that returns nil (representing no upper bound for iterators).
44-78: LGTM!The
TestFormatKeyPrefixtest appropriately covers ASCII text, binary data hex formatting, and truncation behavior. UsingContainsassertions is suitable given that the exact formatting may include additional decorations.
80-114: LGTM!The
TestFormatValuetest covers the key scenarios: ASCII text, binary data, and empty value representation. The test strategy is sound.cmd/cronosd/dbmigrate/migrate_test.go (5)
19-58: LGTM!The
setupTestDBhelper properly creates test databases with configurable backends and populates them with sample data. The resource cleanup with deferred calls for RocksDB-specific options is correct.
60-89: LGTM!
TestCountKeysproperly validates the key counting functionality with both populated and empty databases, which is essential for progress reporting.
109-141: LGTM!
TestMigrateLargeDatabaseappropriately usestesting.Short()to skip expensive tests when running in short mode. The test validates migration with a realistic dataset size and smaller batch sizes.
232-332: LGTM!
TestVerifyMigrationcomprehensively tests the verification logic with multiple scenarios: identical databases (passing), value mismatches, extra keys, and missing keys (all failing). This ensures the verification catches data integrity issues.
334-424: Excellent edge case coverage.
TestMigrateSpecialKeysthoroughly tests migration with problematic key patterns including null bytes, Unicode, large keys, and edge cases. The test correctly handles databases that may not support certain key types (like empty keys) by logging and continuing rather than failing, while still ensuring that successfully written keys are properly migrated.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cmd/cronosd/dbmigrate/migrate_basic_test.go (2)
263-267: Consider simplifying the empty-key guard.All keys in
specialKeyshave length > 0, so the conditionif len(key) > 0is always true. If empty-key testing is not needed, remove the condition; if it is needed, add an empty-key case to the test data.Apply this diff to remove the unnecessary condition:
for i, key := range specialKeys { - if len(key) > 0 { // Skip empty key if not supported - value := []byte(fmt.Sprintf("value-%d", i)) - err := db.Set(key, value) - require.NoError(t, err) - } + value := []byte(fmt.Sprintf("value-%d", i)) + err := db.Set(key, value) + require.NoError(t, err) }
242-286: Verify migrated content for special keys.The test only checks that
stats.ProcessedKeys > 0but doesn't verify that special keys (null bytes, unicode, large keys) were correctly migrated. Consider opening the target database and verifying a subset of the special keys, similar to the thorough verification done inTestHeightFilteredVerification.Add verification after migration:
// Verify the migrated data targetDataDir := filepath.Join(targetDir, "data") targetDB, err := dbm.NewDB("application.migrate-temp", dbm.GoLevelDBBackend, targetDataDir) require.NoError(t, err) defer targetDB.Close() // Verify a few special keys testKey := []byte("🔑emoji-key") value, err := targetDB.Get(testKey) require.NoError(t, err) require.NotNil(t, value) require.Equal(t, []byte("value-4"), value) // Verify null byte key nullKey := []byte("\x00") value, err = targetDB.Get(nullKey) require.NoError(t, err) require.NotNil(t, value)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/cronosd/cmd/migrate_db.go(1 hunks)cmd/cronosd/cmd/patch_db.go(1 hunks)cmd/cronosd/dbmigrate/migrate_basic_test.go(1 hunks)x/cronos/keeper/keeper.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/cronosd/cmd/migrate_db.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-12T22:09:46.096Z
Learnt from: yihuang
Repo: crypto-org-chain/cronos PR: 1618
File: memiavl/db_test.go:199-199
Timestamp: 2024-10-12T22:09:46.096Z
Learning: In unit tests within `memiavl/db_test.go`, converting `int64` to `uint32` is acceptable.
Applied to files:
cmd/cronosd/dbmigrate/migrate_basic_test.go
🧬 Code graph analysis (3)
x/cronos/keeper/keeper.go (2)
x/cronos/types/cronos.pb.go (3)
TokenMapping(147-150)TokenMapping(154-154)TokenMapping(155-157)x/cronos/types/keys.go (1)
KeyPrefixDenomToAutoContract(36-36)
cmd/cronosd/dbmigrate/migrate_basic_test.go (2)
cmd/cronosd/dbmigrate/migrate.go (3)
MigrateOptions(24-46)Migrate(75-240)MigrationStats(49-55)cmd/cronosd/dbmigrate/height_filter.go (3)
DBNameBlockstore(14-14)HeightRange(20-24)DBNameTxIndex(15-15)
cmd/cronosd/cmd/patch_db.go (6)
memiavl/types.go (1)
Logger(6-10)cmd/cronosd/dbmigrate/height_filter.go (2)
ParseHeightFlag(124-151)HeightRange(20-24)cmd/cronosd/dbmigrate/migrate_rocksdb.go (1)
PrepareRocksDBOptions(14-16)cmd/cronosd/dbmigrate/migrate_no_rocksdb.go (1)
PrepareRocksDBOptions(13-15)cmd/cronosd/dbmigrate/patch.go (2)
PatchOptions(46-59)PatchDatabase(62-170)cmd/cronosd/dbmigrate/migrate.go (1)
DefaultBatchSize(18-18)
⏰ 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). (18)
- GitHub Check: Run golangci-lint
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-latest)
- GitHub Check: unittest
- GitHub Check: build (ubuntu-latest)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: Analyze (go)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (4)
x/cronos/keeper/keeper.go (3)
113-119: LGTM! Explicit return improves clarity.The explicit return statement is a good practice that makes the code more readable and maintainable, even though the named return parameters would have returned implicitly.
152-163: Critical fix: iterator cleanup prevents resource leak.The addition of
defer iter.Close()at line 155 is essential—it ensures the iterator is properly released even if the function exits early or panics. This fixes a potential resource leak. The explicit return statement also improves code clarity.
166-177: Iterator cleanup properly prevents resource leak.The addition of
defer iter.Close()at line 169 correctly ensures the iterator is properly released, preventing a resource leak. This implementation is consistent with the fix applied toGetExternalContractsand follows Go best practices for resource management.cmd/cronosd/dbmigrate/migrate_basic_test.go (1)
288-380: Excellent verification coverage!This test provides comprehensive validation of height-filtered migration: it verifies the correct key count, checks that expected keys exist in the target, and confirms that out-of-range keys are excluded. The debug logging and thorough assertions make this a strong reference for other migration tests.
…migrate, and update docs
|
May need to sync with @songgaoye with this one |
Let #1895 get merged first. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cmd/cronosd/dbmigrate/migrate_rocksdb.go (1)
12-15: Clarify the misleading comment about the second parameter.The comment states "returns RocksDB options for migration" but doesn't clarify what the parameters mean. According to
cmd/cronosd/opendb/opendb_rocksdb.go, the second parameter toNewRocksdbOptionsissstFileWriter bool, not a read-only mode flag.Apply this diff to improve the documentation:
-// PrepareRocksDBOptions returns RocksDB options for migration +// PrepareRocksDBOptions returns RocksDB options for migration. +// Parameters: nil uses default options, false means not for SST file writer. func PrepareRocksDBOptions() interface{} { return opendb.NewRocksdbOptions(nil, false) }
🧹 Nitpick comments (14)
cmd/cronosd/dbmigrate/swap-migrated-db.sh (1)
345-349: Consider replacingls | grepwith a safer alternative.Per Shellcheck SC2010, using
ls | grepcan fail with non-alphanumeric filenames. Afindor glob-based approach is more robust.# List data directory echo "" print_info "Current data directory contents:" -ls -lh "$DATA_DIR" | grep -E "^d" | awk '{print " " $9 " (" $5 ")"}' +find "$DATA_DIR" -maxdepth 1 -type d -not -path "$DATA_DIR" -exec basename {} \; 2>/dev/null | while read -r dir; do + size=$(du -sh "$DATA_DIR/$dir" 2>/dev/null | awk '{print $1}') + echo " $dir ($size)" +donecmd/cronosd/dbmigrate/QUICKSTART.md (2)
126-134: Add language specifier to fenced code block.The code block showing migration output should specify a language (e.g.,
textorplaintext) for consistent markdown rendering.-``` +```text ================================================================================ MIGRATION COMPLETED SUCCESSFULLY ================================================================================
138-152: Add language specifier to fenced code block.Same issue - specify a language for the migration output code block.
-``` +```textcmd/cronosd/dbmigrate/migrate.go (3)
46-62: Atomic fields should not be copied by value.
MigrationStatscontainsatomic.Int64fields. If this struct is ever copied (e.g., passed by value or assigned), the atomic semantics break and behavior becomes undefined. TheProgress()andDuration()methods use a value receiver, which is fine for reads, but consider documenting that the struct must always be used via pointer, or switch to*atomic.Int64pointers.
291-344: Consider extracting common retry logic.Both
openDBWithRetryandopenRocksDBWithRetryimplement identical retry patterns differing only in the open function called. This could be consolidated using a higher-order function, but acceptable as-is given the limited scope.
479-487: Key count mismatch may double-count errors.If extra keys are found in phase 2 (lines 458-463), they're counted as mismatches. The key count comparison at line 480 then increments
mismatchCountagain. This could make error counts misleading. Consider either removing the count check (since phase 2 catches extras) or not incrementing mismatch count for the count difference.cmd/cronosd/dbmigrate/patch_integration_test.go (1)
444-455: Document the lexicographic height filtering limitation.The comment notes that height 16 might be included when filtering range 11-15 due to lexicographic string comparison. This is a known limitation that should ideally be addressed in the filtering implementation (using numeric comparison on extracted heights rather than lexicographic key bounds). Consider opening a follow-up issue to track this.
Would you like me to help draft a fix for the height filtering to use numeric comparison instead of lexicographic key bounds?
cmd/cronosd/dbmigrate/README.md (1)
1060-1124: Minor: Use proper heading syntax for error section titles.The error handling section uses emphasis (
####) followed by quoted strings. While this works, consider using consistent heading levels throughout.The current format is functional, but for consistency with the rest of the document, you could use:
-#### 1. "target database does not exist" +#### Error: "target database does not exist"This is a minor style suggestion; the current format is acceptable.
cmd/cronosd/dbmigrate/patch_advanced_test.go (1)
316-333: Consider simplifying the test assertion logic.The assertion logic handles cases where the function might return
(empty, nil)instead of(empty, error). This is valid but the comment at line 322 ("This is acceptable - empty/invalid data returns empty hash") makes the test expectation unclear.Consider splitting into separate test cases or clarifying what behavior is expected:
t.Run(tt.name, func(t *testing.T) { hash, err := extractEthereumTxHash(tt.data) if tt.expectError { - // Some cases may not return error but return empty hash instead - if err == nil && hash == "" { - // This is acceptable - empty/invalid data returns empty hash - return - } - require.Error(t, err) + require.True(t, err != nil || hash == "", + "Expected error or empty hash for invalid input") } else if tt.expectEmpty {This is a minor readability nit; the current implementation is functionally correct.
cmd/cronosd/dbmigrate/migrate_enhanced_test.go (1)
20-86: Test name is slightly misleading.The test handles special edge-case keys (null bytes, binary data, large keys/values) rather than "corrupted" keys. Consider renaming to
TestMigrateWithSpecialKeysorTestMigrateWithEdgeCaseKeysfor clarity. Otherwise, the test coverage is thorough and correctly verifies data integrity.cmd/cronosd/dbmigrate/height_filter.go (3)
26-46: Consider using a map for O(1) lookup with large specific height lists.The current implementation uses O(n) linear search for
SpecificHeights. For typical use cases with a few heights, this is fine. However, if users specify many heights (e.g.,--height 1,2,3,...,1000), consider converting to amap[int64]struct{}during parsing for O(1) lookups.This is a minor optimization concern and can be deferred if specific heights lists are expected to be small.
119-151: Potential edge case with negative height detection.Line 130 checks for
-character to detect range format, but this could incorrectly parse a single negative height like"-100"as a range. While negative heights are ultimately rejected during validation, the error message would be confusing ("invalid range format" instead of "height cannot be negative").Consider adding explicit negative number handling before the range detection:
+ // Check for negative single height (starts with '-' followed by digits only) + if heightStr[0] == '-' && !strings.Contains(heightStr[1:], "-") && !strings.Contains(heightStr, ",") { + height, err := parseInt64(heightStr) + if err != nil { + return HeightRange{}, fmt.Errorf("invalid height value: %w", err) + } + return HeightRange{}, fmt.Errorf("height cannot be negative: %d", height) + } + // Check if it's a range (contains '-') if bytes.IndexByte([]byte(heightStr), '-') >= 0 {
429-471: Fragile protobuf parsing - consider documenting the risk.The function uses heuristic byte-pattern matching to extract the block hash from the
BlockMetaprotobuf structure. While this works, it's fragile because:
- It relies on specific wire format details (field tags 0x0a)
- Changes to CometBFT's
BlockMetastructure could break it silentlyConsider adding a comment documenting this risk and the CometBFT version this was tested against. Alternatively, consider proper protobuf unmarshaling if the
BlockMetatype is available.cmd/cronosd/cmd/migrate_db.go (1)
299-305: Clarify the purpose of this wrapper function.The
MigrateCmdfunction returns the same command asMigrateDBCmdbut setscmd.Deprecated = "". This pattern suggests backward compatibility, but the intent is unclear. Consider adding a comment explaining why both functions exist.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
CHANGELOG.md(1 hunks)Makefile(1 hunks)cmd/cronosd/cmd/database.go(1 hunks)cmd/cronosd/cmd/migrate_db.go(1 hunks)cmd/cronosd/cmd/migrate_db_test.go(1 hunks)cmd/cronosd/cmd/patch_db.go(1 hunks)cmd/cronosd/cmd/patch_db_test.go(1 hunks)cmd/cronosd/cmd/root.go(1 hunks)cmd/cronosd/dbmigrate/QUICKSTART.md(1 hunks)cmd/cronosd/dbmigrate/README.md(1 hunks)cmd/cronosd/dbmigrate/height_filter.go(1 hunks)cmd/cronosd/dbmigrate/height_filter_test.go(1 hunks)cmd/cronosd/dbmigrate/height_parse_test.go(1 hunks)cmd/cronosd/dbmigrate/migrate.go(1 hunks)cmd/cronosd/dbmigrate/migrate_basic_test.go(1 hunks)cmd/cronosd/dbmigrate/migrate_dbname_test.go(1 hunks)cmd/cronosd/dbmigrate/migrate_enhanced_test.go(1 hunks)cmd/cronosd/dbmigrate/migrate_no_rocksdb.go(1 hunks)cmd/cronosd/dbmigrate/migrate_rocksdb.go(1 hunks)cmd/cronosd/dbmigrate/migrate_rocksdb_test.go(1 hunks)cmd/cronosd/dbmigrate/migrate_test.go(1 hunks)cmd/cronosd/dbmigrate/patch.go(1 hunks)cmd/cronosd/dbmigrate/patch_advanced_test.go(1 hunks)cmd/cronosd/dbmigrate/patch_integration_test.go(1 hunks)cmd/cronosd/dbmigrate/patch_test.go(1 hunks)cmd/cronosd/dbmigrate/swap-migrated-db.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- cmd/cronosd/cmd/database.go
- CHANGELOG.md
- cmd/cronosd/cmd/root.go
- Makefile
- cmd/cronosd/dbmigrate/migrate_basic_test.go
- cmd/cronosd/cmd/patch_db_test.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-12T22:09:46.096Z
Learnt from: yihuang
Repo: crypto-org-chain/cronos PR: 1618
File: memiavl/db_test.go:199-199
Timestamp: 2024-10-12T22:09:46.096Z
Learning: In unit tests within `memiavl/db_test.go`, converting `int64` to `uint32` is acceptable.
Applied to files:
cmd/cronosd/dbmigrate/migrate_dbname_test.gocmd/cronosd/dbmigrate/patch_test.gocmd/cronosd/dbmigrate/height_filter_test.gocmd/cronosd/dbmigrate/patch_advanced_test.gocmd/cronosd/dbmigrate/migrate_test.gocmd/cronosd/dbmigrate/migrate_rocksdb_test.go
🧬 Code graph analysis (11)
cmd/cronosd/dbmigrate/migrate_no_rocksdb.go (1)
cmd/cronosd/dbmigrate/migrate_rocksdb.go (1)
PrepareRocksDBOptions(13-15)
cmd/cronosd/dbmigrate/height_filter_test.go (1)
cmd/cronosd/dbmigrate/height_filter.go (1)
HeightRange(20-24)
cmd/cronosd/dbmigrate/height_parse_test.go (2)
cmd/cronosd/dbmigrate/height_filter.go (2)
HeightRange(20-24)ParseHeightFlag(124-151)versiondb/backend_test_utils.go (1)
Run(54-66)
cmd/cronosd/dbmigrate/migrate.go (3)
cmd/cronosd/cmd/migrate_db.go (1)
BackendType(34-34)memiavl/types.go (2)
Logger(6-10)NewNopLogger(18-18)cmd/cronosd/dbmigrate/patch.go (1)
DbExtension(21-21)
cmd/cronosd/dbmigrate/migrate_enhanced_test.go (2)
cmd/cronosd/dbmigrate/migrate.go (1)
MigrateOptions(24-44)memiavl/types.go (2)
Logger(6-10)NewNopLogger(18-18)
cmd/cronosd/dbmigrate/migrate_rocksdb.go (2)
cmd/cronosd/dbmigrate/migrate_no_rocksdb.go (1)
PrepareRocksDBOptions(13-15)cmd/cronosd/opendb/opendb_rocksdb.go (1)
NewRocksdbOptions(88-138)
cmd/cronosd/dbmigrate/patch_integration_test.go (2)
cmd/cronosd/dbmigrate/patch.go (2)
PatchOptions(45-58)PatchDatabase(122-200)cmd/cronosd/dbmigrate/height_filter.go (3)
DBNameBlockstore(14-14)HeightRange(20-24)DBNameTxIndex(15-15)
cmd/cronosd/dbmigrate/patch_advanced_test.go (3)
cmd/cronosd/dbmigrate/patch.go (2)
PatchOptions(45-58)PatchDatabase(122-200)memiavl/types.go (2)
Logger(6-10)NewNopLogger(18-18)cmd/cronosd/dbmigrate/height_filter.go (3)
DBNameTxIndex(15-15)HeightRange(20-24)DBNameBlockstore(14-14)
cmd/cronosd/dbmigrate/migrate_test.go (2)
cmd/cronosd/dbmigrate/migrate.go (3)
MigrationStats(47-53)MigrateOptions(24-44)Migrate(73-204)memiavl/types.go (2)
Logger(6-10)NewNopLogger(18-18)
cmd/cronosd/dbmigrate/migrate_rocksdb_test.go (1)
cmd/cronosd/dbmigrate/migrate.go (2)
MigrateOptions(24-44)Migrate(73-204)
cmd/cronosd/cmd/migrate_db.go (4)
memiavl/types.go (1)
Logger(6-10)cmd/cronosd/dbmigrate/migrate_rocksdb.go (1)
PrepareRocksDBOptions(13-15)cmd/cronosd/dbmigrate/migrate_no_rocksdb.go (1)
PrepareRocksDBOptions(13-15)cmd/cronosd/dbmigrate/migrate.go (4)
MigrationStats(47-53)MigrateOptions(24-44)Migrate(73-204)DefaultBatchSize(18-18)
🪛 Gitleaks (8.30.0)
cmd/cronosd/dbmigrate/README.md
[high] 887-887: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 LanguageTool
cmd/cronosd/dbmigrate/QUICKSTART.md
[grammar] ~51-~51: Ensure spelling is correct
Context: ...# or manually pkill cronosd ### 2. Backup Your Databases bash # Backup applic...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~458-~458: Ensure spelling is correct
Context: ...| HDD (100MB/s) | SSD (500MB/s) | NVMe (3GB/s) | |--------------|---------------|--...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
cmd/cronosd/dbmigrate/README.md
[style] ~399-~399: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...xample 5: Large Database Migration For very large databases, disable verification for fas...
(EN_WEAK_ADJECTIVE)
[grammar] ~989-~989: Ensure spelling is correct
Context: ... Safety and Best Practices #### Always Backup First ```bash # Timestamp your backups...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[uncategorized] ~1114-~1114: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... content and paths. #### 5. "Failed to open source database" ```text Error: failed to ope...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🪛 markdownlint-cli2 (0.18.1)
cmd/cronosd/dbmigrate/QUICKSTART.md
126-126: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
138-138: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
355-355: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
360-360: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🪛 Shellcheck (0.11.0)
cmd/cronosd/dbmigrate/swap-migrated-db.sh
[warning] 348-348: Don't use ls | grep. Use a glob or a for loop with a condition to allow non-alphanumeric filenames.
(SC2010)
⏰ 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). (17)
- GitHub Check: Run golangci-lint
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-latest)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: Analyze (go)
🔇 Additional comments (86)
cmd/cronosd/dbmigrate/migrate_no_rocksdb.go (1)
1-32: LGTM!Clean stub implementation with proper build tags for conditional compilation. The error messages are consistent and helpful, guiding users to rebuild with the correct tags. The comment on Line 29 explaining why the stub exists despite the check in
migrate.gois a good practice.cmd/cronosd/dbmigrate/swap-migrated-db.sh (1)
1-112: Well-structured script with good safety practices.The script has solid input validation (backup suffix sanitization), proper error handling with
set -euo pipefail, clear user prompts, and comprehensive dry-run support. The color-coded output improves usability.cmd/cronosd/dbmigrate/migrate_test.go (3)
19-58: Well-designed test helper with proper cleanup.The
setupTestDBhelper correctly handles both RocksDB and GoLevelDB backends with proper resource cleanup viadefer. Usingt.TempDir()for automatic directory cleanup is a good practice.
91-107: Good edge case handling in stats test.The test correctly verifies that
Progress()returns 0 whenTotalKeysis 0, which properly handles the division-by-zero edge case.
334-423: Excellent coverage of special key patterns.The test comprehensively covers edge cases including empty keys, null bytes, unicode characters, and large keys. The approach of tracking successfully written keys and verifying only those is robust, as it gracefully handles backends that don't support certain key patterns (like empty keys).
cmd/cronosd/dbmigrate/patch_test.go (1)
1-114: LGTM!Clean table-driven tests with good coverage of edge cases. The
incrementBytestest correctly verifies the all-0xFF case returns nil (for unbounded iterator upper bounds), and the format tests appropriately useContainsassertions for flexible output validation.cmd/cronosd/dbmigrate/QUICKSTART.md (2)
1-9: Comprehensive and well-structured documentation.The Quick Start Guide provides excellent coverage of both
migrateandpatchcommands with clear examples, troubleshooting tips, and practical workflows. The decision table at the end (lines 672-683) helps users choose the right command for their use case.
187-201: Verify migrated database naming convention consistency.The manual replacement example in the documentation shows
application.db.migrate-temp, but the review indicates the swap script usesapplication.migrate-temp.db. Confirm which naming convention the migration tool actually produces and ensure consistency across the script and documentation.cmd/cronosd/dbmigrate/migrate.go (3)
101-112: LGTM on source DB lifecycle management.The error check happens before the defer is registered, so there's no nil-panic risk. The
sourceDBClosedflag pattern correctly prevents double-close.
206-219: LGTM!Clean key counting implementation with proper iterator lifecycle management.
232-289: LGTM on batch migration logic.The batch lifecycle is correctly managed. The defer closure captures the variable reference, so it will close whichever batch is current at function exit. Key/value copying on lines 245-248 correctly handles iterator slice reuse.
cmd/cronosd/dbmigrate/height_filter_test.go (6)
10-85: LGTM! Comprehensive coverage for IsWithinRange.Good coverage of boundary conditions, empty ranges, and open-ended configurations. Table-driven tests are well-structured.
87-157: LGTM!Solid test coverage for
IsEmptyandStringmethods with clear test case naming.
159-217: LGTM!Good validation test coverage including edge cases for negative values and invalid ranges.
219-303: LGTM!Thorough test coverage for blockstore key extraction including all CometBFT key prefixes and edge cases.
305-453: LGTM!Excellent coverage of tx_index key extraction and the
shouldIncludeKeyfunction across different database types and height range scenarios.
455-534: LGTM!Helper functions are clear and the metadata extraction tests cover the key edge cases for protobuf-like parsing.
cmd/cronosd/dbmigrate/patch_integration_test.go (6)
18-85: LGTM! Well-structured test database setup.The helper creates realistic blockstore data with all key types (H:, P:, C:, SC:, EC:, BH:) and proper protobuf-like structure for H: values. Good use of
t.Helper()for clean test output.
117-181: LGTM!Good integration test for single-height patching with proper setup/teardown and verification of both patched and non-patched keys.
183-297: LGTM!Good coverage of height range and specific heights patching scenarios with appropriate verification.
605-661: LGTM! Good coverage of BH: key auto-patching.This test validates an important behavior where BH: (block hash lookup) keys are automatically patched alongside H: (block metadata) keys.
663-722: LGTM!Good parameterized test for key counting across different database types and height range scenarios.
505-529: VerifysetupBasicTestDBhelper is available.This test calls
setupBasicTestDB(lines 507, 510) which is not defined in this file. Ensure this helper is defined in another test file in the same package that is included in the!rocksdbbuild.cmd/cronosd/cmd/migrate_db_test.go (5)
9-54: LGTM!Good coverage of backend type parsing including valid types, aliases, and error cases.
56-87: LGTM!Comprehensive validation of database names including both valid and invalid inputs with descriptive test case names.
89-175: LGTM!Excellent test coverage for database name parsing including edge cases like empty entries, whitespace handling, and error messages.
177-250: LGTM!Good coverage of type constants and db-type to database name mapping.
252-307: LGTM!Important test confirming that the
--databasesflag correctly takes precedence over--db-typewhen both are specified.cmd/cronosd/dbmigrate/height_parse_test.go (4)
9-121: LGTM! Comprehensive ParseHeightFlag test coverage.Excellent coverage of parsing scenarios including empty input, single heights, ranges, multiple heights, negative values, invalid formats, and edge cases like empty elements in lists.
123-174: LGTM!Good coverage of
IsWithinRangewith specific heights including first, middle, last, and no-match scenarios.
176-246: LGTM!Good tests for String formatting with specific heights and HasSpecificHeights behavior.
248-312: LGTM!Good coverage of IsEmpty and Validate with specific heights, including the important test for negative height validation.
cmd/cronosd/dbmigrate/migrate_rocksdb.go (3)
81-92: LGTM!The
flushRocksDBfunction properly manages flush options lifecycle and handles the non-RocksDB case gracefully.
64-79: Add cleanup for read/write options on error path.If
OpenDbForReadOnlyfails, the read/write options (ro,wo,woSync) are leaked. This mirrors the error handling pattern inopenRocksDBForMigration.Apply this diff:
func openRocksDBForRead(dir string) (dbm.DB, error) { opts := grocksdb.NewDefaultOptions() defer opts.Destroy() + + ro := grocksdb.NewDefaultReadOptions() + wo := grocksdb.NewDefaultWriteOptions() + woSync := grocksdb.NewDefaultWriteOptions() + woSync.SetSync(true) + db, err := grocksdb.OpenDbForReadOnly(opts, dir, false) if err != nil { + ro.Destroy() + wo.Destroy() + woSync.Destroy() return nil, err } - ro := grocksdb.NewDefaultReadOptions() - wo := grocksdb.NewDefaultWriteOptions() - woSync := grocksdb.NewDefaultWriteOptions() - woSync.SetSync(true) - return dbm.NewRocksDBWithRawDB(db, ro, wo, woSync), nil }Likely an incorrect or invalid review comment.
39-43: No action required — RocksDB options handling is correct.The assumption in the comment is accurate. RocksDB's DB::Open takes Options by const reference and copies them internally into the DBImpl, so destroying locally-created options after the call is safe and proper. The implementation correctly preserves passed-in options while cleaning up only those created within this function.
cmd/cronosd/dbmigrate/README.md (3)
873-892: Gitleaks false positive: Example hex data, not a real API key.The static analysis tool flagged line 887 as a potential API key. This is example documentation showing what debug output would look like with sample hex-encoded transaction hashes. This is not a real secret.
517-543: LGTM!The API documentation for
MigrateOptionsandMigrationStatsprovides clear reference for programmatic usage.
1-11: Well-structured and comprehensive documentation.The README provides excellent coverage of both
migrateandpatchcommands with clear examples, troubleshooting guidance, and architecture overview. The documentation of CometBFT key formats and the three-pass patching approach for tx_index is particularly valuable.cmd/cronosd/dbmigrate/migrate_dbname_test.go (5)
18-38: LGTM!The
setupTestDBWithNamehelper is well-structured with proper use oft.Helper(), automatic cleanup viat.TempDir(), and clear data population logic.
40-78: LGTM!Comprehensive test covering all supported database names with proper subtest isolation and thorough statistics verification.
80-138: LGTM!Good integration test for sequential multi-database migration with proper aggregate statistics tracking.
140-169: LGTM!Good test verifying the default database name behavior when
DBNameis not explicitly set.
250-316: LGTM!Excellent isolation test ensuring that migrations of different databases remain independent and don't cross-contaminate data.
cmd/cronosd/cmd/patch_db.go (4)
228-251: LGTM!The validation logic properly handles both single and multi-database scenarios:
- Single DB: Requires
.dbextension and verifies existence- Multiple DBs: Rejects
.dbextension (expects data directory)The existence check at lines 246-251 ensures the documented "target MUST already exist" contract is enforced.
274-290: LGTM!The error handling correctly guards against nil
statsbefore accessing its fields, addressing the previous review concern about potential panics.
356-362: Clarify the purpose ofcmd.Deprecated = "".Setting
Deprecatedto an empty string seems unnecessary sincePatchDBCmd()doesn't set a deprecation message. Is this intended to explicitly clear a deprecation warning that might be set elsewhere?If this is defensive cleanup, consider adding a comment. If it's unnecessary, it can be removed:
func PatchCmd() *cobra.Command { cmd := PatchDBCmd() cmd.Use = "patch" - cmd.Deprecated = "" return cmd }
28-129: LGTM!Comprehensive command help text with clear examples covering single blocks, ranges, multiple heights, cross-backend patching, and dry-run mode. The IMPORTANT section clearly communicates prerequisites.
cmd/cronosd/dbmigrate/patch_advanced_test.go (6)
21-92: LGTM!The
setupTxIndexWithEthereumEventshelper creates realistic test data including:
- tx.height keys with proper format
- CometBFT transaction hashes
- Ethereum event-indexed keys (with and without
$es$suffix)- Properly marshaled TxResult with ethereum_tx events
This enables thorough testing of the three-pass tx_index patching logic.
94-146: LGTM!Excellent integration test verifying that Ethereum event keys are properly patched alongside tx.height and txhash keys.
381-426: LGTM!Comprehensive edge case coverage for the
incrementByteshelper, including overflow handling and carry propagation.
680-728: LGTM!Critical test verifying that dry-run mode truly doesn't modify the target database. The test confirms both the key count stays the same and specific heights remain absent from the target.
533-587: LGTM!Thorough test verifying all blockstore key types (H:, P:, C:, SC:, EC:) are properly patched across multiple heights.
730-736: Consider using the built-inminfunction if the project requires Go 1.21+.Go 1.21 introduced built-in
minandmaxfunctions. If this project's minimum Go version is 1.21 or higher, this helper can be removed and the built-in used directly.cmd/cronosd/dbmigrate/migrate_enhanced_test.go (6)
1-18: LGTM! Well-structured test file with appropriate build constraints.The build tags correctly exclude this file when RocksDB is enabled, and the imports are appropriate for the testing scenarios.
88-126: LGTM!The test correctly verifies that duplicate key writes result in only the final value being migrated, which is the expected key-value store behavior.
128-180: LGTM!The test correctly verifies that key ordering (lexicographic) is preserved during migration, which is essential for iterator-based operations.
182-396: LGTM!These tests provide excellent coverage for:
- Binary (non-UTF8) key handling
- Empty value preservation
- Migration statistics accuracy
- All supported database names
- Post-migration verification
The test structure is consistent and assertions are appropriate.
398-563: LGTM!Good coverage of:
- Progress tracking to 100%
- Large value handling with appropriate short-mode skip
- Various batch sizes for batching logic verification
- Automatic target directory creation
- Graceful handling of non-existent source directories
565-587: LGTM!The test verifies that default values are properly applied when optional parameters are omitted, ensuring a good developer experience with sensible defaults.
cmd/cronosd/dbmigrate/migrate_rocksdb_test.go (4)
1-44: LGTM! RocksDB test setup with appropriate cache sizing.The build tags are correct, and the RocksDB options configuration is appropriate for tests with a 64MB cache (addressed from previous review). The configuration mirrors production settings while remaining test-friendly.
46-78: LGTM!The helper function properly:
- Uses
t.Helper()for better test failure reporting- Registers cleanup functions for all RocksDB resources
- Creates a properly wrapped
dbm.DBinterface from raw RocksDB- Uses zero-padded key format for predictable ordering
80-135: LGTM!The test correctly verifies cross-backend migration from LevelDB to RocksDB, including:
- Migration statistics validation
- Actual data integrity verification by reading from target
- Correct unified path format (
application.migrate-temp.db)
137-338: LGTM!Excellent coverage of RocksDB migration scenarios:
- Bidirectional migrations (to/from LevelDB)
- Same-backend migrations (RocksDB→RocksDB for compaction)
- Large dataset stress testing with appropriate short-mode skip
- Custom RocksDB options handling
- Comprehensive data integrity verification by comparing all source/target keys
cmd/cronosd/dbmigrate/height_filter.go (6)
1-25: LGTM!The package declaration, imports, and
HeightRangestruct are well-designed. The struct cleanly supports both continuous ranges and specific height lists with clear field documentation.
48-117: LGTM!The helper methods are well-implemented:
IsEmpty()correctly identifies uninitialized rangesString()provides user-friendly output with smart truncationValidate()properly checks for negative values and invalid ranges
230-314: LGTM!The function correctly handles all documented CometBFT blockstore key formats:
H:,C:,SC:,EC:with direct height extractionP:with height extraction before the part indexBH:and metadata keys properly returnfalseforhasHeightThe
fmt.Sscanfusage is safe here for parsing single integers.
316-381: LGTM!The key filtering logic correctly:
- Extracts height from
tx.height/prefixed keys- Includes keys without height information (metadata, etc.)
- Falls through to include all keys for unsupported databases
473-476: LGTM!Simple and correct helper function for checking height filtering support.
383-414: Height filtering is correctly applied viashouldProcessKey()in iteration loops.Prefix-bounded iterators (e.g.,
H:toI:) do not constrain numeric height ranges since heights are string-encoded without zero-padding. However, this is handled correctly by Strategy B:shouldProcessKey()is called inpatchWithIterator()(patch.go line 1114) to filter keys by numeric height during iteration.cmd/cronosd/cmd/migrate_db.go (3)
1-62: LGTM! Well-structured type definitions and constants.The type definitions with string-based enums provide good type safety while maintaining readability. The
validDatabaseNamesmap enables efficient validation.
63-297: LGTM! Well-implemented CLI command.The command implementation is thorough:
- Comprehensive help text with clear examples
- Proper input validation with helpful error messages
- Nil-safe error handling for migration stats (addressed from past review)
- Per-database migration with aggregated statistics
- Clear post-migration guidance for users
The flag precedence (
--databasesover--db-type) is documented and correctly implemented.
307-355: LGTM!Helper functions are well-implemented:
parseBackendTypecorrectly handles theleveldbalias forgoleveldbparseDatabaseNamesproperly validates and trims inputgetDBNamesFromTypecleanly maps database types to their constituent databasescmd/cronosd/dbmigrate/patch.go (14)
1-58: LGTM! Clear type definitions and constants.The
EthTxInfo,ConflictResolutionenum, andPatchOptionsstruct are well-designed with clear documentation. The conflict resolution strategies provide good flexibility for different use cases.
60-84: LGTM!Validation is thorough and provides clear error messages. The requirement for a non-empty
HeightRangecorrectly enforces that patching is height-based (unlike full migration).
86-119: LGTM!Database opening functions correctly handle:
- Path construction and
.dbextension stripping- Backend-specific opening (RocksDB vs standard)
- Clear error messages with context
121-200: LGTM!The main
PatchDatabasefunction has a clean flow:
- Validation → 2. Dry-run announcement → 3. Open databases → 4. Count keys → 5. Patch data → 6. Flush → 7. Record stats
Resource cleanup with
deferis properly ordered (sourceDB first, then targetDB).
202-269: LGTM!The counting function correctly:
- Uses height filtering during counting
- Provides debug logging for the first few keys to aid troubleshooting
- Properly closes iterators after use
271-423: LGTM!The patching flow is well-decomposed:
patchDataWithHeightFilterroutes to appropriate handlerpatchBlockstoreDataprocesses all prefix iteratorsextractHeightAndTxIndexFromKeyhandles the complex key format with$es$suffix- Conflict checking with dynamic strategy updates is well-implemented
- Collection of txhashes and Ethereum tx info enables the multi-pass approach
456-549: LGTM!The function correctly implements the first pass of tx_index patching:
- Patches
tx.height/keys to target- Collects CometBFT txhashes for second pass
- Collects Ethereum tx info for third pass
- Proper batch management and dry-run support
551-588: LGTM!The three-pass tx_index patching strategy is well-orchestrated:
- Pass 1: Patch
tx.height/keys and collect transaction info- Pass 2: Patch CometBFT txhash lookup keys
- Pass 3: Patch Ethereum event-indexed keys
This ensures cross-reference integrity in the tx_index database.
590-693: LGTM!The txhash key patching correctly:
- Retrieves values from source database for each collected txhash
- Handles missing keys gracefully with debug logging
- Applies conflict resolution consistently
- Uses batch writes for efficiency
695-756: LGTM!
extractEthereumTxHashproperly unmarshals theTxResultprotobuf and validates the Ethereum txhash formatincrementBytescorrectly handles the byte increment with carry-over for iterator bounds
758-926: Map iteration order is non-deterministic but acceptable.The iteration over
ethTxInfosmap (line 774) has non-deterministic order. This is acceptable here because:
- Each key is processed independently
- The final database state is the same regardless of order
- This is a CLI tool, not consensus code
The function correctly uses specific prefixes to minimize iteration range and properly handles iterator cleanup.
1005-1075: Well-implemented BH: key patching for blockstore.The
patchSingleKeyfunction correctly patches the correspondingBH:<hash>key when processingH:(block metadata) keys. This ensures the block hash lookup index stays consistent with the block metadata.
1094-1195: LGTM! Main patching loop with progress tracking.The
patchWithIteratorfunction correctly:
- Applies height filtering via
shouldProcessKey- Handles conflicts with dynamic strategy updates
- Provides periodic progress logging (every 5 seconds)
- Properly manages batch writes and cleanup
1197-1341: LGTM! Well-designed interactive and display helpers.
promptKeyConflictprovides clear user prompts with single-key and bulk optionsformatKeyPrefixandformatValueintelligently handle both text and binary data- The 80% printable threshold heuristic is reasonable for distinguishing text from binary
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
cmd/cronosd/dbmigrate/README.md (1)
519-531: MigrateOptions API snippet may be outdated.The
MigrateOptionsstruct shown here should be verified against the actual implementation inmigrate.go. Based on the AI summary, the actual struct includes additional fields. Consider adding a note that this is a simplified representation or updating it to match the actual implementation.cmd/cronosd/dbmigrate/migrate_rocksdb.go (1)
12-15: Misleading comment about the second parameter.The comment states no explanation for the second parameter. According to
opendb/opendb_rocksdb.go, the second parameter toNewRocksdbOptionsissstFileWriter bool, not a read-only mode flag.// PrepareRocksDBOptions returns RocksDB options for migration func PrepareRocksDBOptions() interface{} { + // nil: use default options, false: not for SST file writer return opendb.NewRocksdbOptions(nil, false) }cmd/cronosd/dbmigrate/patch.go (1)
202-269: Fix height-range counting logic and undefinedminhelper.Two problems here:
min(...)is not defined anywhere, so this file does not compile.- Height filtering is only applied when
HasSpecificHeights()is true. For continuous ranges (e.g.10000-20000),needsFilteringis false, socountKeysForPatchcounts all keys under the blockstore/tx_index prefixes instead of respectingStart/End.That means a range like
--height 10000-20000is effectively treated as "all heights" in the key-counting step, which in turn misleads progress reporting and hides that the patch will touch more data than requested.You can fix both issues by:
- Introducing a small
minInthelper.- Treating any non-empty
HeightRangeas needing numeric filtering and usingIsWithinRangefor both ranges and specific heights.@@ func countKeysForPatch(db dbm.DB, dbName string, heightRange HeightRange, logger log.Logger) (int64, error) { var totalCount int64 - // If we have specific heights, we need to filter while counting - needsFiltering := heightRange.HasSpecificHeights() + // Any non-empty heightRange (range or specific heights) requires numeric filtering + needsFiltering := !heightRange.IsEmpty() @@ iterators, err := getBlockstoreIterators(db, heightRange) @@ - keysSeen := 0 + keysSeen := 0 for iterIdx, it := range iterators { @@ - height, hasHeight := extractHeightFromBlockstoreKey(it.Key()) - logger.Debug("Blockstore key found", - "key_prefix", string(it.Key()[:min(10, len(it.Key()))]), - "key_hex", fmt.Sprintf("%x", it.Key()[:min(20, len(it.Key()))]), + height, hasHeight := extractHeightFromBlockstoreKey(it.Key()) + logger.Debug("Blockstore key found", + "key_prefix", string(it.Key()[:minInt(10, len(it.Key()))]), + "key_hex", fmt.Sprintf("%x", it.Key()[:minInt(20, len(it.Key()))]), "has_height", hasHeight, "height", height, "in_range", !needsFiltering || (hasHeight && heightRange.IsWithinRange(height))) } } - if needsFiltering { - // Extract height and check if it's in our specific list - height, hasHeight := extractHeightFromBlockstoreKey(it.Key()) - if hasHeight && !heightRange.IsWithinRange(height) { - continue - } - } - totalCount++ + if needsFiltering { + // Extract height and check if it falls within HeightRange (range or specific heights) + height, hasHeight := extractHeightFromBlockstoreKey(it.Key()) + if !hasHeight || !heightRange.IsWithinRange(height) { + continue + } + } + totalCount++ @@ case DBNameTxIndex: @@ - for ; it.Valid(); it.Next() { - if needsFiltering { - // Extract height and check if it's in our specific list - height, hasHeight := extractHeightFromTxIndexKey(it.Key()) - if hasHeight && !heightRange.IsWithinRange(height) { - continue - } - } - totalCount++ - } + for ; it.Valid(); it.Next() { + if needsFiltering { + // Extract height and check if it falls within HeightRange (range or specific heights) + height, hasHeight := extractHeightFromTxIndexKey(it.Key()) + if !hasHeight || !heightRange.IsWithinRange(height) { + continue + } + } + totalCount++ + } @@ return totalCount, nil } + +// minInt returns the smaller of two ints. +func minInt(a, b int) int { + if a < b { + return a + } + return b +}
🧹 Nitpick comments (9)
cmd/cronosd/dbmigrate/QUICKSTART.md (2)
126-138: Add language specifiers to code blocks.These code blocks showing expected output should have a language specifier for consistency and to satisfy markdown linting:
-``` +```text ================================================================================ MIGRATION COMPLETED SUCCESSFULLYAlso applies to line 138.
355-362: Use proper headings instead of bold emphasis.For better document structure and accessibility, consider using proper heading syntax instead of bold emphasis:
-**Solution 1: Increase Batch Size** +#### Solution 1: Increase Batch Size ```bash cronosd database migrate --batch-size 50000 ...-Solution 2: Disable Verification
+#### Solution 2: Disable Verification</blockquote></details> <details> <summary>cmd/cronosd/cmd/patch_db_test.go (2)</summary><blockquote> `141-157`: **Consider importing error types from patch_db.go to avoid message drift.** The test defines local `targetPathError` and `targetExistenceError` types that duplicate error messages from `patch_db.go`. If the actual error messages change, these tests would pass while no longer matching production behavior. Consider one of these approaches: 1. Export the error types from `patch_db.go` and use them here 2. Use `errors.Is()` or `errors.As()` with sentinel errors 3. Add a comment noting these must stay in sync with `patch_db.go` --- `55-84`: **Tests simulate validation logic rather than calling actual implementation.** The test manually reimplements the validation logic from `patch_db.go` rather than calling the actual validation function. This approach: - Risks diverging from the real implementation - Doesn't test the actual code path If `patch_db.go` exposes validation as a testable function, consider calling it directly. Otherwise, consider adding integration tests that exercise the full command. </blockquote></details> <details> <summary>cmd/cronosd/dbmigrate/patch_advanced_test.go (1)</summary><blockquote> `250-334`: **Error handling test has potential logic issue in assertion.** Lines 319-331: The test logic allows both error return and empty hash as acceptable outcomes for some error cases, which is fine for flexibility. However, the comment at line 320-323 suggests this behavior is intentional - consider making the expected behavior more explicit in the test case struct (e.g., add `expectEmptyHash` field). </blockquote></details> <details> <summary>cmd/cronosd/cmd/migrate_db_test.go (1)</summary><blockquote> `252-307`: **Good precedence test, but test assertions could be cleaner.** The test correctly verifies that `--databases` takes precedence over `--db-type`. However, the assertion at line 304 (`require.Equal(t, tt.useDatabases, tt.databasesFlag != "")`) is redundant since `useDatabases` is defined exactly as `databasesFlag != ""` in the test cases. Consider removing the redundant assertion or replacing `useDatabases` with a more meaningful field like `expectDatabasesFlagUsed`: ```diff - require.Equal(t, tt.useDatabases, tt.databasesFlag != "") + // The useDatabases field documents intent; assertion confirms logiccmd/cronosd/dbmigrate/swap-migrated-db.sh (2)
345-349: Avoidls | grepfor directory listing.As flagged by Shellcheck (SC2010), using
ls | grepcan fail with non-alphanumeric filenames and is generally fragile.Replace with a
forloop orfind:-# List data directory -echo "" -print_info "Current data directory contents:" -ls -lh "$DATA_DIR" | grep -E "^d" | awk '{print " " $9 " (" $5 ")"}' +# List data directory +echo "" +print_info "Current data directory contents:" +for dir in "$DATA_DIR"/*/; do + if [[ -d "$dir" ]]; then + dir_name=$(basename "$dir") + dir_size=$(du -sh "$dir" 2>/dev/null | awk '{print $1}') + echo " $dir_name ($dir_size)" + fi +done
289-293: Dry-run still increments SUCCESS_COUNT.In dry-run mode,
SUCCESS_COUNTis incremented (line 292) even though no actual swap occurred. This may be intentional (to show what would succeed), but consider renaming toWOULD_SUCCEED_COUNTor adding a comment to clarify the intent.cmd/cronosd/cmd/patch_db.go (1)
269-272: Consider whetherConflictAskis appropriate for non-interactive CLI usage.
ConflictStrategy: dbmigrate.ConflictAskprompts the user for each conflict, which may not work well in automated/scripted environments. Consider adding a flag (e.g.,--conflict-strategy) to allow users to specifyoverwrite,skip, oraskbehavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
CHANGELOG.md(1 hunks)Makefile(1 hunks)cmd/cronosd/cmd/database.go(1 hunks)cmd/cronosd/cmd/migrate_db.go(1 hunks)cmd/cronosd/cmd/migrate_db_test.go(1 hunks)cmd/cronosd/cmd/patch_db.go(1 hunks)cmd/cronosd/cmd/patch_db_test.go(1 hunks)cmd/cronosd/cmd/root.go(1 hunks)cmd/cronosd/dbmigrate/QUICKSTART.md(1 hunks)cmd/cronosd/dbmigrate/README.md(1 hunks)cmd/cronosd/dbmigrate/height_filter.go(1 hunks)cmd/cronosd/dbmigrate/height_filter_test.go(1 hunks)cmd/cronosd/dbmigrate/height_parse_test.go(1 hunks)cmd/cronosd/dbmigrate/migrate.go(1 hunks)cmd/cronosd/dbmigrate/migrate_basic_test.go(1 hunks)cmd/cronosd/dbmigrate/migrate_dbname_test.go(1 hunks)cmd/cronosd/dbmigrate/migrate_enhanced_test.go(1 hunks)cmd/cronosd/dbmigrate/migrate_no_rocksdb.go(1 hunks)cmd/cronosd/dbmigrate/migrate_rocksdb.go(1 hunks)cmd/cronosd/dbmigrate/migrate_rocksdb_test.go(1 hunks)cmd/cronosd/dbmigrate/migrate_test.go(1 hunks)cmd/cronosd/dbmigrate/patch.go(1 hunks)cmd/cronosd/dbmigrate/patch_advanced_test.go(1 hunks)cmd/cronosd/dbmigrate/patch_integration_test.go(1 hunks)cmd/cronosd/dbmigrate/patch_test.go(1 hunks)cmd/cronosd/dbmigrate/swap-migrated-db.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- cmd/cronosd/cmd/database.go
- cmd/cronosd/dbmigrate/height_filter_test.go
- cmd/cronosd/dbmigrate/height_parse_test.go
- CHANGELOG.md
- Makefile
- cmd/cronosd/dbmigrate/patch_test.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-04T16:09:08.694Z
Learnt from: yihuang
Repo: crypto-org-chain/cronos PR: 1618
File: memiavl/db_test.go:199-199
Timestamp: 2024-10-04T16:09:08.694Z
Learning: In unit tests within `memiavl/db_test.go`, converting `int64` to `uint32` is acceptable.
Applied to files:
cmd/cronosd/dbmigrate/patch_advanced_test.gocmd/cronosd/dbmigrate/migrate_basic_test.gocmd/cronosd/dbmigrate/migrate_rocksdb_test.gocmd/cronosd/dbmigrate/migrate_dbname_test.gocmd/cronosd/dbmigrate/migrate_test.go
🧬 Code graph analysis (12)
cmd/cronosd/dbmigrate/migrate.go (3)
cmd/cronosd/cmd/migrate_db.go (1)
BackendType(34-34)memiavl/types.go (2)
Logger(6-10)NewNopLogger(18-18)cmd/cronosd/dbmigrate/patch.go (1)
DbExtension(21-21)
cmd/cronosd/dbmigrate/migrate_rocksdb.go (3)
cmd/cronosd/dbmigrate/migrate_no_rocksdb.go (1)
PrepareRocksDBOptions(13-15)cmd/cronosd/opendb/opendb_rocksdb.go (1)
NewRocksdbOptions(88-138)cmd/cronosd/cmd/migrate_db.go (1)
RocksDB(40-40)
cmd/cronosd/cmd/patch_db_test.go (1)
cmd/cronosd/dbmigrate/patch.go (1)
DbExtension(21-21)
cmd/cronosd/cmd/migrate_db_test.go (1)
cmd/cronosd/cmd/migrate_db.go (14)
BackendType(34-34)GoLevelDB(38-38)LevelDB(39-39)RocksDB(40-40)DatabaseName(43-43)Application(47-47)Blockstore(48-48)State(49-49)TxIndex(50-50)Evidence(51-51)DbType(25-25)App(29-29)CometBFT(30-30)All(31-31)
cmd/cronosd/dbmigrate/migrate_no_rocksdb.go (1)
cmd/cronosd/dbmigrate/migrate_rocksdb.go (1)
PrepareRocksDBOptions(13-15)
cmd/cronosd/dbmigrate/patch_advanced_test.go (4)
cmd/cronosd/cmd/migrate_db.go (1)
BackendType(34-34)cmd/cronosd/dbmigrate/patch.go (2)
PatchOptions(45-58)PatchDatabase(122-200)memiavl/types.go (2)
Logger(6-10)NewNopLogger(18-18)cmd/cronosd/dbmigrate/height_filter.go (3)
DBNameTxIndex(15-15)HeightRange(20-24)DBNameBlockstore(14-14)
cmd/cronosd/dbmigrate/patch_integration_test.go (2)
cmd/cronosd/dbmigrate/patch.go (1)
PatchOptions(45-58)cmd/cronosd/dbmigrate/height_filter.go (2)
DBNameBlockstore(14-14)HeightRange(20-24)
cmd/cronosd/dbmigrate/migrate_enhanced_test.go (3)
cmd/cronosd/dbmigrate/migrate.go (1)
MigrateOptions(24-44)memiavl/types.go (2)
Logger(6-10)NewNopLogger(18-18)memiavl/db.go (1)
Load(146-266)
cmd/cronosd/cmd/root.go (1)
cmd/cronosd/cmd/database.go (1)
DatabaseCmd(8-29)
cmd/cronosd/dbmigrate/migrate_rocksdb_test.go (3)
memiavl/db.go (2)
DB(44-86)Load(146-266)cmd/cronosd/dbmigrate/migrate.go (2)
MigrateOptions(24-44)Migrate(73-204)memiavl/types.go (2)
Logger(6-10)NewNopLogger(18-18)
cmd/cronosd/dbmigrate/migrate_dbname_test.go (2)
cmd/cronosd/cmd/migrate_db.go (1)
BackendType(34-34)cmd/cronosd/dbmigrate/migrate.go (2)
MigrateOptions(24-44)Migrate(73-204)
cmd/cronosd/dbmigrate/migrate_test.go (3)
cmd/cronosd/cmd/migrate_db.go (1)
BackendType(34-34)cmd/cronosd/dbmigrate/migrate.go (3)
MigrationStats(47-53)MigrateOptions(24-44)Migrate(73-204)memiavl/types.go (2)
Logger(6-10)NewNopLogger(18-18)
🪛 Gitleaks (8.30.0)
cmd/cronosd/dbmigrate/README.md
[high] 887-887: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 LanguageTool
cmd/cronosd/dbmigrate/README.md
[style] ~399-~399: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...xample 5: Large Database Migration For very large databases, disable verification for fas...
(EN_WEAK_ADJECTIVE)
[grammar] ~989-~989: Ensure spelling is correct
Context: ... Safety and Best Practices #### Always Backup First ```bash # Timestamp your backups...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[uncategorized] ~1114-~1114: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... content and paths. #### 5. "Failed to open source database" ```text Error: failed to ope...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
cmd/cronosd/dbmigrate/QUICKSTART.md
[grammar] ~51-~51: Ensure spelling is correct
Context: ...# or manually pkill cronosd ### 2. Backup Your Databases bash # Backup applic...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~458-~458: Ensure spelling is correct
Context: ...| HDD (100MB/s) | SSD (500MB/s) | NVMe (3GB/s) | |--------------|---------------|--...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
cmd/cronosd/dbmigrate/QUICKSTART.md
126-126: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
138-138: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
355-355: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
360-360: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🪛 Shellcheck (0.11.0)
cmd/cronosd/dbmigrate/swap-migrated-db.sh
[warning] 348-348: Don't use ls | grep. Use a glob or a for loop with a condition to allow non-alphanumeric filenames.
(SC2010)
⏰ 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). (20)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (ibc)
- GitHub Check: Run golangci-lint
- GitHub Check: Analyze (go)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-latest)
- GitHub Check: unittest
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (59)
cmd/cronosd/dbmigrate/README.md (1)
870-892: False positive: Gitleaks warning on example debug output.The static analysis flagged line 887 as a potential API key, but this is clearly example debug output showing how binary txhash values are displayed as hex strings (e.g.,
0x1a2b3c...). This is safe documentation content.cmd/cronosd/dbmigrate/migrate_no_rocksdb.go (1)
1-32: LGTM!The stub implementations correctly provide compile-time compatibility for non-RocksDB builds with clear error messages guiding users to rebuild with the appropriate build tag.
cmd/cronosd/dbmigrate/migrate_rocksdb.go (2)
31-43: Resource management for default options is now correct.The pattern of tracking
createdOptsand only deferringDestroy()when the function created the options internally properly addresses the previous resource leak concern. Options passed fromPrepareRocksDBOptions()remain managed by the caller.
64-79: LGTM - Read-only opener correctly manages resources.The implementation correctly:
- Defers
opts.Destroy()since RocksDB copies options internally duringOpenDbForReadOnly- Passes
ro,wo,woSynctoNewRocksDBWithRawDBwithout destroying them, as the wrapper takes ownershipcmd/cronosd/cmd/root.go (1)
197-200: LGTM!The database command integration follows the established pattern used for
ChangeSetCmdand is correctly placed before global flags are applied.cmd/cronosd/dbmigrate/migrate_test.go (3)
60-89: LGTM - Key counting test coverage.Good test coverage for the
countKeysfunction with both populated and empty database scenarios.
232-332: Excellent verification test coverage.The
TestVerifyMigrationfunction provides thorough coverage of verification scenarios including:
- Identical databases (pass)
- Value mismatches (fail)
- Extra keys in target (fail)
- Missing keys in target (fail)
This ensures the verification logic correctly detects all types of data integrity issues.
334-423: Good edge case coverage for special keys.The test covers important edge cases:
- Empty keys (handled gracefully if unsupported)
- Null bytes
- Unicode characters
- Large keys
- Keys with whitespace
The approach of tracking successfully written keys and verifying only those is robust.
cmd/cronosd/dbmigrate/patch_advanced_test.go (5)
1-19: LGTM! Build constraints and imports are well-structured.The file correctly uses both the new-style (
//go:build !rocksdb) and old-style (// +build !rocksdb) build constraints for compatibility. Imports are appropriate for the test scenarios.
22-92: Well-designed test helper for Ethereum transaction indexing.The
setupTxIndexWithEthereumEventshelper creates realistic test data including:
tx.heightkeys with proper format- TxResult protobuf structures with
ethereum_txevents- Event-indexed keys with
$es$suffix handlingThis provides good coverage for the Ethereum event key patching workflow.
148-248: Comprehensive table-driven tests for Ethereum tx hash extraction.The test cases cover:
- Valid hashes with/without
0xprefix- Missing
ethereum_txevents- Missing
ethereumTxHashattributeGood edge case coverage.
680-728: Thorough dry-run verification test.The test correctly verifies that:
- Original key count is preserved
- Specific height keys are NOT present after dry-run
This ensures dry-run mode is truly side-effect free at the data level.
730-736: Consider using Go's built-inminfunction instead of this helper.Go 1.21+ provides a built-in
minfunction that can replace this helper. Verify the project's Go version requirement ingo.modto determine if this refactor is applicable; if the project targets Go 1.21+, this helper can be removed.cmd/cronosd/dbmigrate/patch_integration_test.go (5)
1-16: LGTM! Integration test file setup is correct.Build constraints and imports are properly configured for non-RocksDB testing.
18-85: Well-structured blockstore test helper with realistic key formats.The helper creates all expected CometBFT blockstore key types:
H:(block metadata with embedded hash)P:(block parts)C:(commit)SC:(seen commit)EC:(extended commit for ABCI 2.0)BH:(block header by hash)BS:H(metadata)The protobuf-like structure for the hash embedding (lines 40-43) is a good approximation for testing.
443-455: Known lexicographic ordering limitation is properly documented.The comment acknowledges that string-based height comparison in LevelDB iteration can include unintended heights (e.g., "16" falling between "11" and "2"). This is documented as a known limitation rather than a bug, which is appropriate for test transparency.
505-529: Good negative test for unsupported database patching.The test verifies that attempting to patch
application.db(which doesn't support height-based patching) returns an appropriate error. This aligns with the PR comments stating that application.db patching is not supported.
663-721: VerifycountKeysForPatchfunction exists and matches expected signature.The test calls
countKeysForPatch(db, tt.dbName, tt.heightRange, log.NewTestLogger(t))with parameters of type(dbm.DB, string, HeightRange, logger.Logger)and expects a return of(int64, error)based on the assertion patterns. This function definition should be present in the same package or imported; confirm it exists with the correct signature.cmd/cronosd/cmd/migrate_db_test.go (3)
1-7: LGTM! Test file setup is minimal and correct.Standard test file with testify/require import.
9-54: Comprehensive backend type parsing tests.Tests cover:
- Valid backends:
goleveldb,leveldb(alias),rocksdb- Invalid backend handling
- Empty string handling
Good input validation coverage.
89-175: Thorough database name parsing tests with edge cases.Tests cover:
- Single/multiple databases
- Whitespace handling (including extra spaces)
- Invalid database names
- Empty entries in comma-separated list
- Completely empty input
The different error substrings (
"invalid database name","no valid databases specified","no databases specified") provide good error message specificity.cmd/cronosd/dbmigrate/swap-migrated-db.sh (2)
1-50: Well-structured script initialization with proper validation.Good practices observed:
set -euo pipefailfor strict error handling- BACKUP_SUFFIX validation prevents injection via unsafe characters
- Default values with immediate validation
205-212: Dry-run side-effect issue has been addressed.The backup directory creation is now correctly gated behind
[[ "$DRY_RUN" == false ]], ensuring dry-run mode makes no filesystem changes.cmd/cronosd/dbmigrate/migrate.go (6)
72-112: LGTM! Migration initialization with sensible defaults.The function properly:
- Sets default batch size and target home
- Uses NopLogger as fallback
- Defaults DBName to "application"
- Logs clear startup information
137-142: Nil check in defer has been addressed.The defer now correctly checks
targetDB != nilbefore callingClose(), preventing potential panics if database creation fails.
232-289: Migration loop makes defensive copies correctly.Lines 244-248 correctly copy key/value slices before adding to batch, as iterators may reuse underlying slices. The batching and progress reporting logic is solid.
291-344: Robust retry logic with exponential backoff.Both
openDBWithRetryandopenRocksDBWithRetryimplement proper exponential backoff (50ms → 100ms → 200ms → 400ms → 800ms) with clear logging. This handles OS-level file lock delays gracefully.
346-500: Verification logic does not filter by HeightRange, creating failures for partial migrations.If
HeightRangeis supported for partial database migrations, theverifyMigrationfunction must filter keys accordingly. Currently, it iterates all keys in both databases, causing false mismatches when the target contains only a height-filtered subset.
16-21:DefaultWorkersconstant requires usage verification.The constant is defined at line 20 but its usage cannot be verified due to system limitations. Before removing this constant, confirm whether it is:
- Referenced elsewhere in the codebase
- Part of an exported API that external packages may depend on
- Intended for future implementation of concurrent worker migration
Search the codebase for all references to
DefaultWorkersto make an informed decision about removal.cmd/cronosd/dbmigrate/migrate_basic_test.go (5)
1-38: LGTM! Well-structured test setup.The build tags correctly exclude RocksDB builds, and
setupBasicTestDBprovides a clean helper for creating temporary test databases with configurable backends and key counts.
40-117: Good test coverage for key counting and statistics.The tests correctly validate key counting for edge cases (0 keys, 100 keys) and verify that
MigrationStatsproperly tracks progress via atomic operations.
119-240: Comprehensive batch and edge-case testing.The tests cover important scenarios: large datasets (with
testing.Short()skip), empty databases, verification toggle, and various batch sizes. This ensures the migration logic handles different operational conditions correctly.
242-286: Special keys test correctly handles edge cases.The test now properly asserts errors unconditionally for supported keys. The
if len(key) > 0guard appropriately skips empty keys (which may not be supported by all backends) rather than suppressing write errors.
288-349: Path correctness validation is thorough.The test verifies both that the expected directory structure is created and that the migrated database can be opened and contains the correct data. This ensures logging and actual paths are consistent.
cmd/cronosd/cmd/patch_db.go (5)
28-129: Comprehensive help text and examples.The command documentation clearly explains supported databases, height specification formats, important caveats about target database requirements, and provides practical examples for various use cases including dry-run mode.
143-200: Validation logic is thorough and well-structured.Required flags are checked early, database names are validated against an allowlist, and height specifications are properly parsed and validated before any operations begin.
222-251: Target path handling correctly implements all previously requested safeguards.The implementation now properly:
- Rejects
.dbpaths when patching multiple databases (lines 232-235)- Requires
.dbextension for single-database targets (lines 240-243)- Verifies target database existence before patching (lines 246-251)
274-290: Nil-stats guard correctly prevents panics on early failures.The error handling now properly checks if
statsis non-nil before accessing its fields, addressing the previous review concern.
338-351: Flag definitions are clear with appropriate defaults and shortcuts.The flags provide sensible defaults (goleveldb source, rocksdb target) and short flags for common operations. Required flags are properly marked.
cmd/cronosd/dbmigrate/migrate_dbname_test.go (5)
18-38: Clean helper function for named database setup.The helper properly includes the database name in key/value patterns, making it easy to verify data isolation between different databases during testing.
40-78: Comprehensive DB name coverage.Testing all five standard database names (application, blockstore, state, tx_index, evidence) ensures the migration logic handles each correctly, including positive duration verification.
80-138: Multi-database migration test validates isolation and aggregation.The test correctly creates multiple databases in a shared source directory, migrates each sequentially, and verifies both per-database stats and the aggregate total.
140-169: Default DBName behavior is properly tested.The test verifies that omitting
DBNameinMigrateOptionscorrectly defaults to "application", matching the documented behavior inmigrate.go.
171-343: Additional edge cases provide good coverage.Tests for CometBFT databases, empty databases with names, different DB name isolation, and special characters (underscores) in names ensure robust handling across various scenarios.
cmd/cronosd/dbmigrate/migrate_enhanced_test.go (8)
20-86: Thorough edge-case testing for special key patterns.The test covers null bytes, max bytes, embedded nulls, large keys (1KB), and large values (1MB), validating that all are correctly migrated and retrievable from the target database.
88-126: Duplicate key handling test validates last-write-wins semantics.The test correctly verifies that when keys are written multiple times, the migration transfers exactly the final unique key count (5 keys, not 15 writes).
128-180: Key ordering preservation test is well-designed.The test verifies that LevelDB's lexicographic ordering is maintained after migration by inserting keys in random order and checking sorted retrieval.
182-280: Binary keys and empty values tests ensure data integrity.These tests validate that non-UTF8 binary keys and zero-length values are handled correctly without corruption during migration.
282-421: Statistics accuracy and progress tracking tests are thorough.The tests verify that
TotalKeys,ProcessedKeys,ErrorCount,Progress(), andDuration()all report accurate values upon migration completion.
424-474: Large value test appropriately guarded withtesting.Short().The test handles 50MB of data (5 × 10MB values), which is properly skipped in short mode to avoid slow CI runs.
540-563: Non-existent source test has permissive assertion.The test acknowledges that some backends auto-create directories rather than failing. This is acceptable behavior documentation, though consider logging a warning in the actual
Migratefunction if the source appears empty/auto-created.
565-587: Default values test validates fallback behavior.The test confirms that omitting
BatchSize,Logger, andDBNamedoesn't cause errors and that defaults are applied correctly.cmd/cronosd/cmd/migrate_db.go (6)
25-61: Type-safe constants improve code quality.The
DbType,BackendType, andDatabaseNametype definitions with associated constants provide compile-time safety and clear documentation of valid values.
63-121: Comprehensive help text with clear examples.The command documentation explains the migration process, database types, important caveats, and provides practical examples covering various migration scenarios.
122-177: Validation and database selection logic is well-structured.The code properly validates backends, prevents same-backend migrations, and implements the precedence rule where
--databasestakes priority over--db-type.
202-251: Per-database migration loop with proper error handling.The nil-stats guard (lines 222-235) correctly prevents panics when
Migratereturns(nil, err), addressing the previous review concern.
253-283: Summary logging is informative and actionable.The completion summary includes total statistics, per-database paths, and clear next steps for the operator to follow.
307-355: Helper functions are clean and well-documented.
parseBackendTypecorrectly handles theleveldbalias forgoleveldb.parseDatabaseNamesvalidates against the allowlist and handles edge cases.getDBNamesFromTypereturns the correct database sets for each type.cmd/cronosd/dbmigrate/migrate_rocksdb_test.go (1)
20-78: RocksDB test options and setup look appropriate.Options, cache size, and temporary RocksDB setup are sensible for tests, and cleanup via
t.Cleanupplusdb.Close()provides good hygiene. No changes needed here.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
cmd/cronosd/dbmigrate/patch.go (3)
456-587: Height filtering fortx.height/keys now honors both ranges and specific heights
patchTxHeightKeysnow unconditionally extracts the height and usesopts.HeightRange.IsWithinRange(height)to decide whether to process each key, which fixes the earlier issue where onlySpecificHeightswere respected and continuous ranges were ignored. Combined with the dedicated conflict handler and batching, this loop now matches the documented--heightsemantics for tx_index patching.
926-953:shouldProcessKeycorrectly centralizes height-range filteringThe updated
shouldProcessKeynow:
- Treats an empty
HeightRangeas “no filter”.- Extracts height for both
blockstoreandtx_indexprefixes.- Skips keys without a parseable height whenever a range/specific heights are requested.
- Uses
heightRange.IsWithinRange(height)so both continuous ranges and specific height sets are supported.This matches the intended semantics and fixes the earlier behavior where range-based
Start/Endfilters were ignored.
217-244: Undefinedminhelper still breaks build and can panic on short keysThe debug logging in
countKeysForPatchstill callsmin(...):"key_prefix", string(it.Key()[:min(10, len(it.Key()))]), "key_hex", fmt.Sprintf("%x", it.Key()[:min(20, len(it.Key()))]),Go has no built-in
minforint, and this file does not define one, so this will not compile. Fixing this also ensures safe slicing when keys are shorter than 10/20 bytes.Suggested diff:
@@ - logger.Debug("Blockstore key found", - "key_prefix", string(it.Key()[:min(10, len(it.Key()))]), - "key_hex", fmt.Sprintf("%x", it.Key()[:min(20, len(it.Key()))]), + logger.Debug("Blockstore key found", + "key_prefix", string(it.Key()[:minInt(10, len(it.Key()))]), + "key_hex", fmt.Sprintf("%x", it.Key()[:minInt(20, len(it.Key()))]), "has_height", hasHeight, "height", height, "in_range", !needsFiltering || (hasHeight && heightRange.IsWithinRange(height))) @@ func countKeysForPatch(db dbm.DB, dbName string, heightRange HeightRange, logger log.Logger) (int64, error) { @@ return totalCount, nil } + +// minInt returns the smaller of a and b. +func minInt(a, b int) int { + if a < b { + return a + } + return b +}cmd/cronosd/dbmigrate/migrate_test.go (1)
19-64: RocksDB option lifetime issue resolved witht.CleanupFor the RocksDB backend in
setupTestDB, you now:
- Allocate
ro,wo,woSyncwithout deferringDestroy()immediately.- Register a
t.Cleanupthat destroys the options after the test completes.- Let ownership of these options transfer to
dbm.NewRocksDBWithRawDB, which is closed by the tests.This fixes the prior use-after-free risk while keeping resource cleanup explicit and tied to the test lifecycle.
🧹 Nitpick comments (4)
cmd/cronosd/dbmigrate/QUICKSTART.md (2)
355-355: Convert emphasis to proper markdown headings.Lines 355 and 360 use bold emphasis (
**Solution 1**,**Solution 2**) as section headers instead of markdown headings. Use###for consistency with the document structure.Apply this diff:
### Migration is Slow -**Solution 1: Increase Batch Size** +### Solution 1: Increase Batch Size ```bash cronosd database migrate --batch-size 50000 ...-Solution 2: Disable Verification
+### Solution 2: Disable Verificationcronosd database migrate --verify=false ...Also applies to: 360-360
126-134: Add language specifiers to code blocks.Fenced code blocks at lines 126-134 and 138-152 should declare their language for syntax highlighting and clarity. These appear to be command output or text content.
Apply these diffs:
Look for: -``` +```text ================================================================================ MIGRATION COMPLETED SUCCESSFULLYLook for: -``` +```text 4:30PM INF Starting migration database=application 4:30PM INF Migration completed database=application processed_keys=21 total_keys=21Also applies to: 138-152
cmd/cronosd/dbmigrate/patch.go (1)
588-924: MigrationStats semantics for tx_index patches may exceed 100%In
patchTxHashKeysandpatchEthereumEventKeysFromSource, you incrementstats.ProcessedKeysfor txhash and Ethereum event keys as well:stats.ProcessedKeys.Add(int64(batchCount))However,
stats.TotalKeysfor tx_index patches is derived fromcountKeysForPatch, which counts onlytx.height/...keys. This means for tx_index patchesProcessedKeyscan legitimately exceedTotalKeysif both txhash and Ethereum event keys are written, which could be confusing ifMigrationStatsis surfaced to users (e.g., viaProgress()).Not urgent, but consider either:
- Documenting that, for tx_index,
ProcessedKeyscounts all written keys whereasTotalKeysis onlytx.heightkeys, or- Tracking separate counters (e.g., for base vs. derived keys) if you plan to display percentage progress for tx_index patches.
cmd/cronosd/dbmigrate/migrate_test.go (1)
239-338: Avoid double-closingsourceDBinTestVerifyMigrationIn
TestVerifyMigrationeach subtest does:sourceDir, sourceDB := setupTestDB(...) defer sourceDB.Close() ... // Close databases before verification sourceDB.Close() targetDB.Close()This closes
sourceDBtwice. Cosmos DB implementations are likely tolerant, but it’s non-idiomatic and could obscure real resource-lifetime issues.You can drop either the
defer sourceDB.Close()or the explicitsourceDB.Close()in the “Close databases before verification” block (keeping the explicittargetDB.Close()).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.md(1 hunks)cmd/cronosd/dbmigrate/QUICKSTART.md(1 hunks)cmd/cronosd/dbmigrate/migrate_test.go(1 hunks)cmd/cronosd/dbmigrate/patch.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-12T22:09:46.096Z
Learnt from: yihuang
Repo: crypto-org-chain/cronos PR: 1618
File: memiavl/db_test.go:199-199
Timestamp: 2024-10-12T22:09:46.096Z
Learning: In unit tests within `memiavl/db_test.go`, converting `int64` to `uint32` is acceptable.
Applied to files:
cmd/cronosd/dbmigrate/migrate_test.go
🧬 Code graph analysis (1)
cmd/cronosd/dbmigrate/patch.go (4)
cmd/cronosd/cmd/migrate_db.go (1)
TxIndex(50-50)memiavl/types.go (1)
Logger(6-10)cmd/cronosd/dbmigrate/height_filter.go (3)
HeightRange(20-24)DBNameBlockstore(14-14)DBNameTxIndex(15-15)cmd/cronosd/dbmigrate/migrate.go (1)
MigrationStats(47-53)
🪛 LanguageTool
cmd/cronosd/dbmigrate/QUICKSTART.md
[grammar] ~51-~51: Ensure spelling is correct
Context: ...# or manually pkill cronosd ### 2. Backup Your Databases bash # Backup applic...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~458-~458: Ensure spelling is correct
Context: ...| HDD (100MB/s) | SSD (500MB/s) | NVMe (3GB/s) | |--------------|---------------|--...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
cmd/cronosd/dbmigrate/QUICKSTART.md
126-126: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
138-138: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
355-355: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
360-360: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ 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). (20)
- GitHub Check: build (macos-14)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: build (macos-latest)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: Analyze (go)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (ibc)
- GitHub Check: unittest
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: Run golangci-lint
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (7)
cmd/cronosd/dbmigrate/patch.go (5)
44-200: Patch orchestration and option validation look solid
PatchOptions,validatePatchOptions, andPatchDatabasewire together logging, height-range validation, DB opening, key counting, and RocksDB flushing cleanly. Requiring a non-emptyHeightRangeand restrictingDBNametoblockstore/tx_indexkeeps this tool scoped to height-based patching, which matches the CLI intent.
307-454: Tx.height parsing and Ethereum txhash collection behavior
extractHeightAndTxIndexFromKey,patchTxHeightKeyAndCollect, andcollectEthereumTxInfocorrectly:
- Parse
tx.height/<h>/<h>/<txindex>[$es$<seq>]keys defensively.- Guard on parse failures and missing values.
- Copy iterator-backed values before storing txhashes or writing to batches.
Errors when extracting Ethereum txhashes are logged at
Debugand treated as “no eth hash”, which is a reasonable trade-off for not failing the entire patch on malformed events.
955-1075: Conflict handling and BH: companion key patching look correct
handleKeyConflictmirrors theConflictResolutionsemantics cleanly (ask/skip/replace/replace-all) and logs previews atDebugonly.patchSingleKeycorrectly:
- Honors
DryRunwith detailed logging.- For
DBNameBlockstore, patches companionBH:<hash>keys when the primaryH:metadata key is written, both in dry-run (preview only) and actual modes.- Copies values out of the source DB before putting them into a batch to avoid iterator buffer reuse issues.
This keeps block header-by-hash lookups consistent with height-index patches.
1077-1195: Batching and progress logging inpatchWithIterator
patchWithIterator:
- Uses
shouldProcessKeyfor height filtering, so blockstore patches respect the requested range.- Applies
handleKeyConflictfor each key and tracks askippedCount.- Batches writes via
writeAndResetBatchand logs periodic progress every 5 seconds usingMigrationStats, with an explicit dry-run branch.Given
PatchDatabaseexits early whenTotalKeys == 0, the percentage computation avoids division-by-zero.The non-determinism warnings about
time.Nowand progress logging are acceptable here since this is an offline admin tool, not consensus-critical code.
1197-1341: Interactive conflict prompt and formatting helpers are robust
promptKeyConflictclearly presents conflict details and supports one-off and “all” actions (r/s/R/S), aligning with theConflictResolutionenum used elsewhere.formatKeyPrefixandformatValueuse simple heuristics to distinguish text vs. binary data and handle truncation safely, avoiding panics on very shortmaxLenvalues.formatStrategyprovides stable, human-readable labels that match the strategy constants.These utilities should make conflict resolution and debugging much more usable without affecting core behavior.
cmd/cronosd/dbmigrate/migrate_test.go (2)
97-236: Migration stats and basic migration tests cover key behaviors
TestMigrationStats,TestMigrateLargeDatabase,TestMigrateEmptyDatabase,TestMigrationWithoutVerification, andTestMigrationBatchSizescollectively validate:
MigrationStats.Progress()for 0%, 50%, and 100%.- End-to-end migrations across varying dataset sizes and batch sizes.
- Behavior with and without verification enabled.
The patterns (temp dirs, explicit Close, short-mode gating) are sound and give good coverage over the core migrate path.
340-429: Nice coverage for special key patterns in migration
TestMigrateSpecialKeysexercises migration with:
- Empty and NUL-containing keys,
- Whitespace and newline-containing keys,
- Unicode/emoji keys, and
- Large 1 KiB keys,
while gracefully skipping an unsupported empty key. It then verifies each successfully written key on the target DB. This is a strong, practical edge-case test for the generic migration path.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cmd/cronosd/dbmigrate/QUICKSTART.md (1)
197-197: ✅ Filename pattern inconsistencies resolved.All instances now correctly use the consistent
${db}.migrate-temp.dbpattern (e.g.,application.migrate-temp.db). The issues flagged in previous reviews have been addressed.Also applies to: 217-218, 392-392, 441-441
🧹 Nitpick comments (2)
cmd/cronosd/dbmigrate/QUICKSTART.md (2)
353-363: Use markdown headings instead of bold text for subsections.Lines 355 and 360 use bold text as section headers (Solution 1: ...), but these should be proper markdown headings (####) for consistency with the document hierarchy and to support accessibility tools.
Apply this diff to fix the Troubleshooting section:
### Troubleshooting -**Solution 1: Increase Batch Size** +#### Solution 1: Increase Batch SizeSimilarly, update line 360:
-**Solution 2: Disable Verification** +#### Solution 2: Disable Verification
126-134: [Optional] Add language tags to output code blocks for markdown consistency.Markdownlint prefers fenced code blocks to have a language identifier. The output blocks at lines 126–134 and 138–152 could be marked as
textorbash(representing shell output) for consistency with other code blocks in the document.-``` +```text ================================================================================and
-``` +```bashThis is a minor style improvement for markdown consistency.
Also applies to: 138-152
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/cronosd/dbmigrate/QUICKSTART.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
cmd/cronosd/dbmigrate/QUICKSTART.md
[grammar] ~51-~51: Ensure spelling is correct
Context: ...# or manually pkill cronosd ### 2. Backup Your Databases bash # Backup applic...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~458-~458: Ensure spelling is correct
Context: ...| HDD (100MB/s) | SSD (500MB/s) | NVMe (3GB/s) | |--------------|---------------|--...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
cmd/cronosd/dbmigrate/QUICKSTART.md
126-126: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
138-138: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
355-355: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
360-360: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ 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). (20)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (ibc)
- GitHub Check: Run golangci-lint
- GitHub Check: build (ubuntu-latest)
- GitHub Check: unittest
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: Analyze (go)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (2)
cmd/cronosd/dbmigrate/QUICKSTART.md (2)
482-495: ✅ Documentation accurately reflects tool limitations and constraints.The documentation correctly states that:
- The tool assumes source database has correct data (no corruption fixing)
- Patching only supports
blockstoreandtx_indexdatabases- Target database must exist for patch operations
- Application database cannot be patched (migrate only)
This aligns with the PR discussion and tool behavior.
1-683: ✅ Comprehensive and well-structured database operations guide.The QUICKSTART.md provides excellent coverage of both migration and patching workflows:
- Clear prerequisites and step-by-step procedures
- Multiple command variations with practical examples
- Extensive troubleshooting section with recovery procedures
- Performance optimization tips and estimated timings for different storage media
- Proper emphasis on backup and verification steps
- Detailed explanation of the three-pass tx_index patching strategy
The documentation effectively complements the implementation and README, serving as a practical operational guide for operators.
4 participants
Implementation new CLI to support db migration or patch between goleveldb and rocksdb backend.
For details, can read the README in
cmd/cronosd/dbmigrateSummary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.