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)
✨ Finishing Touches
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 -->
| App: cometBFTState.Version.Consensus.App, | ||
| }, | ||
| ChainID: cometBFTState.ChainID, | ||
| InitialHeight: uint64(cometBFTState.LastBlockHeight), // The initial height is the migration height |
There was a problem hiding this comment.
InitialHeight should reflect the chain's actual initial height.
Setting InitialHeight to LastBlockHeight seems incorrect. It should be the height at which the chain was initialized, not the current migration height.
Consider using the actual initial height from the CometBFT state or configuration.
🤖 Prompt for AI Agents
In server/migration_cmd.go at line 222, the InitialHeight is incorrectly set to
LastBlockHeight, which represents the current migration height. Instead, update
InitialHeight to use the chain's actual initial height from the CometBFT state
or configuration, ensuring it reflects the chain's true starting height rather
than the current block height.
There was a problem hiding this comment.
this is correct, as the start of the rollkit chain (technically a new chain), will the be the end of the last chain.
There was a problem hiding this comment.
@julienrbrt Thank you for the clarification! That makes perfect sense - for chain migration continuity, the new Rollkit chain should indeed start where the CometBFT chain left off. I understand now that this is an intentional design decision.
✏️ Learnings added
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.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| _ = extendedCommitInfo | ||
| // rollkitStore.SaveExtendedCommit(context.Background(), header.Height(), &extendedCommitInfo) | ||
| panic("Saving extended commit info is not implemented yet") | ||
| } |
There was a problem hiding this comment.
Replace panic with proper error handling.
Using panic for unimplemented functionality is poor user experience. Return an error instead.
- _ = 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")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _ = 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") |
🤖 Prompt for AI Agents
In server/migration_cmd.go around lines 106 to 109, replace the panic call used
for unimplemented saving of extended commit info with proper error handling by
returning an error instead. Modify the function to return an error indicating
that saving extended commit info is not implemented yet, avoiding abrupt
termination and improving user experience.
|
|
||
| // Only save extended commit info if vote extensions are enabled | ||
| if enabled := cometBFTstate.ConsensusParams.ABCI.VoteExtensionsEnabled(block.Height); enabled { | ||
| extendedCommit := cometBlockStore.LoadBlockExtendedCommit(lastBlockHeight) |
There was a problem hiding this comment.
Fix incorrect height variable usage.
The code uses lastBlockHeight instead of the current iteration variable height.
- extendedCommit := cometBlockStore.LoadBlockExtendedCommit(lastBlockHeight)
+ extendedCommit := cometBlockStore.LoadBlockExtendedCommit(height)🤖 Prompt for AI Agents
In server/migration_cmd.go at line 80, replace the variable lastBlockHeight with
the current loop iteration variable height when calling LoadBlockExtendedCommit
to ensure the correct block height is used in each iteration.
| for height := lastBlockHeight; height > 0; height-- { | ||
| cmd.Printf("Migrating block %d...\n", height) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider iterating blocks in chronological order.
The current implementation iterates blocks backward from lastBlockHeight to 1. For consistency and potential dependencies between blocks, consider iterating forward instead.
- for height := lastBlockHeight; height > 0; height-- {
+ for height := int64(1); height <= lastBlockHeight; height++ {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for height := lastBlockHeight; height > 0; height-- { | |
| cmd.Printf("Migrating block %d...\n", height) | |
| for height := int64(1); height <= lastBlockHeight; height++ { | |
| cmd.Printf("Migrating block %d...\n", height) |
🤖 Prompt for AI Agents
In server/migration_cmd.go around lines 68 to 69, the loop iterates blocks
backward from lastBlockHeight down to 1, which may cause issues with
dependencies between blocks. Change the loop to iterate forward from 1 up to
lastBlockHeight to process blocks in chronological order, ensuring consistency
and correct handling of dependencies.
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