feat(server): add migration command #113
Conversation
|
""" WalkthroughA new migration command, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MigrationCmd
participant CometBFTStore
participant RollkitStore
User->>MigrationCmd: Execute rollkit-migration
MigrationCmd->>CometBFTStore: Load block and state stores
MigrationCmd->>CometBFTStore: Get latest block and state
MigrationCmd->>RollkitStore: Initialize Rollkit state store
MigrationCmd->>RollkitStore: Convert and save initial state
loop For each block (latest to 1)
MigrationCmd->>CometBFTStore: Load block
MigrationCmd->>MigrationCmd: Convert block to Rollkit format
MigrationCmd->>RollkitStore: Save block components
end
MigrationCmd->>RollkitStore: Close store
MigrationCmd-->>User: Print progress/completion messages
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 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 (3)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Going back to this on Monday |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
server/migration_cmd.go (2)
157-157: Address TODO comment about signature handling.The TODO indicates uncertainty about signature assignment.
Would you like assistance in determining the correct signature handling for the Rollkit header?
226-226: Address hardcoded DAHeight value.The
DAHeightis hardcoded to 1 with a TODO comment. This needs to be properly implemented.How should the DAHeight be determined during migration? Would you like help implementing the logic to set this value correctly?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
go.mod(1 hunks)server/migration_cmd.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: lint / golangci-lint
- GitHub Check: Test with Rollkit Chain
- GitHub Check: test / Run Unit Tests
🔇 Additional comments (3)
go.mod (1)
19-19: LGTM! Necessary dependency change for migration functionality.Moving
cometbft-dbto direct requirements is appropriate since the new migration command directly imports and uses this package.server/migration_cmd.go (2)
165-165: Verify LastDataHash should reference previous block.The
LastDataHashin metadata uses the current block'sDataHash, but "Last" typically refers to the previous block.Please verify if this should reference the previous block's data hash instead of the current block's.
148-149: ```shell
#!/bin/bash
set -euo pipefailLocate the cometBlockToRollkit function and surrounding context
rg "func cometBlockToRollkit" -n -C5 --color never
Inspect the usage of LastHeaderHash in this file
rg "LastHeaderHash" -n -C5 --color never
</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
server/migration_cmd.go (6)
39-56: Add proper resource cleanup for database connections.The CometBFT block store and state store databases are opened but never explicitly closed, causing resource leaks.
Add defer statements to ensure proper cleanup:
cometBlockStore, cometStateStore, err := loadStateAndBlockStore(config) if err != nil { return err } + defer func() { + if cometBlockStore != nil { + cometBlockStore.Close() + } + }() + defer func() { + if cometStateStore != nil { + cometStateStore.Close() + } + }()
74-75: Consider iterating blocks in chronological order.Processing blocks backward may cause issues with dependencies between blocks and is counterintuitive for migration workflows.
- for height := lastBlockHeight; height > 0; height-- { + for height := int64(1); height <= lastBlockHeight; height++ {
86-86: Fix incorrect height variable usage.The code uses
lastBlockHeightinstead of the current iteration variableheight.- extendedCommit := cometBlockStore.LoadBlockExtendedCommit(lastBlockHeight) + extendedCommit := cometBlockStore.LoadBlockExtendedCommit(height)
112-115: Replace panic with proper error handling.Using panic for unimplemented functionality provides poor user experience and abruptly terminates the migration process.
- _ = extendedCommitInfo - // rollkitStore.SaveExtendedCommit(context.Background(), header.Height(), &extendedCommitInfo) - panic("Saving extended commit info is not implemented yet") + // TODO: Implement SaveExtendedCommit + // rollkitStore.SaveExtendedCommit(context.Background(), header.Height(), &extendedCommitInfo) + return fmt.Errorf("saving extended commit info is not implemented yet for block height %d", height)
231-231: Fix incorrect InitialHeight assignment.Setting
InitialHeighttoLastBlockHeightis incorrect - it should represent the chain's actual starting height, not the migration point.- InitialHeight: uint64(cometBFTState.LastBlockHeight), // The initial height is the migration height + InitialHeight: uint64(cometBFTState.InitialHeight), // Use the actual chain initial height
185-212: Add error handling for database closure.The function opens databases but doesn't provide a way for callers to close them properly, leading to resource leaks.
Consider returning a cleanup function or modifying the signature to include cleanup responsibilities:
-func loadStateAndBlockStore(config *cfg.Config) (*store.BlockStore, state.Store, error) { +func loadStateAndBlockStore(config *cfg.Config) (*store.BlockStore, state.Store, func(), error) { dbType := dbm.BackendType(config.DBBackend) if !os.FileExists(filepath.Join(config.DBDir(), "blockstore.db")) { - return nil, nil, fmt.Errorf("no blockstore found in %v", config.DBDir()) + return nil, nil, nil, fmt.Errorf("no blockstore found in %v", config.DBDir()) } // Get BlockStore blockStoreDB, err := dbm.NewDB("blockstore", dbType, config.DBDir()) if err != nil { - return nil, nil, err + return nil, nil, nil, err } blockStore := store.NewBlockStore(blockStoreDB) if !os.FileExists(filepath.Join(config.DBDir(), "state.db")) { + blockStoreDB.Close() - return nil, nil, fmt.Errorf("no statestore found in %v", config.DBDir()) + return nil, nil, nil, fmt.Errorf("no statestore found in %v", config.DBDir()) } // Get StateStore stateDB, err := dbm.NewDB("state", dbType, config.DBDir()) if err != nil { + blockStoreDB.Close() - return nil, nil, err + return nil, nil, nil, err } stateStore := state.NewStore(stateDB, state.StoreOptions{ DiscardABCIResponses: config.Storage.DiscardABCIResponses, }) + cleanup := func() { + blockStoreDB.Close() + stateDB.Close() + } - return blockStore, stateStore, nil + return blockStore, stateStore, cleanup, nil }
🧹 Nitpick comments (1)
server/migration_cmd.go (1)
166-166: Resolve signature handling uncertainty.The TODO comment indicates uncertainty about signature handling, which is critical for block validation.
The signature assignment needs clarification. Would you like me to research the correct signature mapping between CometBFT and Rollkit block structures?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/migration_cmd.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test with Rollkit Chain
- GitHub Check: test / Run Unit Tests
- GitHub Check: lint / golangci-lint
🔇 Additional comments (1)
server/migration_cmd.go (1)
157-158: Verify header hash mapping correctness.Both
LastHeaderHashandLastCommitHashare set to the same value (block.LastCommitHash.Bytes()), which may be incorrect.Verify that this mapping is correct by checking the Rollkit and CometBFT documentation for the expected semantics of these fields.
#!/bin/bash # Search for header hash usage patterns in the codebase echo "Searching for LastHeaderHash usage patterns..." rg -A 3 -B 3 "LastHeaderHash" echo -e "\nSearching for LastCommitHash usage patterns..." rg -A 3 -B 3 "LastCommitHash"
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
server/migration_cmd.go (2)
93-93: Fix incorrect height variable usage.The code uses
lastBlockHeightinstead of the current iteration variableheight, which will load the same extended commit for every block iteration.- extendedCommit := cometBlockStore.LoadBlockExtendedCommit(lastBlockHeight) + extendedCommit := cometBlockStore.LoadBlockExtendedCommit(height)
119-122: Replace panic with proper error handling.Using panic for unimplemented functionality provides poor user experience and will crash the application unexpectedly.
- _ = extendedCommitInfo - // rollkitStore.SaveExtendedCommit(context.Background(), header.Height(), &extendedCommitInfo) - panic("Saving extended commit info is not implemented yet") + // TODO: Implement SaveExtendedCommit + // rollkitStore.SaveExtendedCommit(context.Background(), header.Height(), &extendedCommitInfo) + return fmt.Errorf("saving extended commit info is not implemented yet")
🧹 Nitpick comments (1)
server/migration_cmd.go (1)
185-187: Add transaction validation during conversion.The transaction conversion is performed without validation, which could propagate invalid data to the Rollkit store.
for _, tx := range block.Txs { + if len(tx) == 0 { + continue // Skip empty transactions + } data.Txs = append(data.Txs, rollkittypes.Tx(tx)) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/migration_cmd.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: julienrbrt
PR: rollkit/go-execution-abci#113
File: server/migration_cmd.go:231-231
Timestamp: 2025-06-20T13:20:04.011Z
Learning: In the rollkit migration command (server/migration_cmd.go), InitialHeight is correctly set to LastBlockHeight because the Rollkit chain is technically a new chain that starts where the CometBFT chain ended, ensuring continuity during migration.
server/migration_cmd.go (1)
Learnt from: julienrbrt
PR: rollkit/go-execution-abci#113
File: server/migration_cmd.go:231-231
Timestamp: 2025-06-20T13:20:04.011Z
Learning: In the rollkit migration command (server/migration_cmd.go), InitialHeight is correctly set to LastBlockHeight because the Rollkit chain is technically a new chain that starts where the CometBFT chain ended, ensuring continuity during migration.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: lint / golangci-lint
- GitHub Check: test / Run Unit Tests
- GitHub Check: Test with Rollkit Chain
🔇 Additional comments (3)
server/migration_cmd.go (3)
173-174: Complete the signature implementation.The TODO comment indicates incomplete signature handling. The current implementation assigns the proposer's signature but doesn't handle the SignedHeader signature properly.
Please clarify the intended behavior for the SignedHeader signature field. Should it be:
- The proposer's signature from the block (current implementation)
- A new signature over the Rollkit header
- Some other signature scheme?
This affects the integrity verification of migrated blocks.
128-128: LGTM: Proper resource cleanup implemented.Good implementation of resource cleanup using
errors.Jointo ensure all stores are closed even if some close operations fail.
238-238: LGTM: Correct InitialHeight assignment.Based on previous feedback, this correctly sets InitialHeight to LastBlockHeight since the Rollkit chain is a new chain starting where the CometBFT chain ended, ensuring migration continuity.
Overview
Based on rollkit/cosmos-sdk-starter#63
Closes: #77
ref: #102
Summary by CodeRabbit
New Features
Chores