fix testnet issues #279
fix testnet issues #279
Conversation
📝 WalkthroughWalkthroughThis pull request updates cryptographic library imports and testnet command invocations while introducing new server management functionalities. The changes modify the Ed25519 import in the main application file and update the testnet command integration in the command module. Additionally, two new files in the server package provide a comprehensive framework for blockchain node operations including testnet creation, configuration handling, telemetry, gRPC and API server startup, database management, and graceful shutdown processes. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant CLI as CLI Command
participant TC as Testnet Creator
participant APP as Application Server
participant GRPC as gRPC Server
participant API as API Server
participant TEL as Telemetry
U->>CLI: Run start/testnet command
CLI->>TC: Invoke InPlaceTestnetCreator (and AddTestnetCreatorCommand)
TC->>APP: Initialize blockchain node & testnet
APP->>TEL: Initialize telemetry metrics
APP->>GRPC: Start gRPC service
APP->>API: Start API service
Note over APP: Manage asynchronous tasks & graceful shutdown
Suggested reviewers
Poem
✨ 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:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
server/util.go (2)
13-17: Check data directory creation or error handling.When calling
dbm.NewDB("application", backendType, dataDir), ensure that the data directory always exists or that the function gracefully handles directory creation errors. Otherwise, there is a potential risk of failing silently if the directory is missing.
18-27: Consider adding error logging for empty traceWriterFile scenario.If
traceWriterFileis empty, the logic simply returns without creating a writer. It might be worth logging or explicitly acknowledging that no trace file is being generated to avoid confusion during debugging.server/start.go (4)
111-207: Provide additional user guidance on the irreversible nature of testnet modification.The
InPlaceTestnetCreatorcommand correctly warns about the permanent changes to on-disk state. Consider adding verbose logs or a recovery note for potential backups, ensuring operators are fully aware of potential data implications.
209-261: Validate flags for boundary conditions.The
addStartNodeFlagsfunction sets a variety of node configuration flags. Ensure that invalid or risky values (e.g., negative intervals, extremely large memory constraints) are checked or documented. For example, consider bounding the maximum memory or file sizes to prevent excessive resource usage.
331-359: Close application resources after errors.The
startAppfunction defers acleanupFnthat closes the app and trace writer. This is a good pattern. Ensure any additional opened resources here (e.g., external APIs or file handles) are also accounted for in the cleanup for a robust shutdown.
817-870: Consider explicit logging around gRPC-only mode.When
gRPCOnlyis true, the code setssvrCfg.GRPC.Enable = true; it might be helpful to log or mention that API server remains configurable, but CometBFT is disabled. This avoids user confusion regarding full node vs. gRPC-only usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/app.go(1 hunks)cmd/bitsongd/cmd/root.go(2 hunks)server/start.go(1 hunks)server/util.go(1 hunks)
🔇 Additional comments (9)
app/app.go (1)
50-50: Ensure Ed25519 usage is consistent throughout the codebase.Changing the import path for the Ed25519 package is typically fine, but confirm that any references to the old package have been updated and that the new import aligns with the rest of the application’s cryptographic utilities.
server/util.go (2)
1-12: No major issues; package and imports look good.The newly introduced package structure and imports appear consistent. No obvious concurrency or security concerns are evident at this stage.
29-34: Testnet creator command structure looks solid.Attaching the testnet creation command to the root command is a clean approach. Verify that interactive help text is sufficient for users who may be unfamiliar with the in-place testnet creation process.
cmd/bitsongd/cmd/root.go (2)
37-37: Use of an alias import for improved clarity.Renaming the import to
testnetserverhelps differentiate it from the standardserverpackage. This should improve readability and reduce confusion.
270-270: Confirmed reorganization of testnet command logic.Switching from
server.AddTestnetCreatorCommandtotestnetserver.AddTestnetCreatorCommandis consistent with the new file structure and import. Good job maintaining clarity for future collaboration.server/start.go (4)
275-298: Graceful fallback path in standalone mode.When skipping CometBFT (
withCmt == false), the logic defaults tostartStandAlone. This separation is clean. Just confirm that all resources needed (e.g., gRPC, API) are still functional or gracefully degrade if CometBFT is not started.
300-329: CPU profiling logic is well-contained.Wrapping the main callback in
wrapCPUProfileneatly isolates CPU profiling concerns. No issues with concurrency or resource leaks are apparent since the function defersStopCPUProfile()on completion.
545-793: Potential risk in rewriting chain data and forging commits.The
testnetifylogic forcibly rewrites the chain ID, block store, and validator set, which is expected for a testnet migration. However, be aware that leftover references (e.g., uncleaned references to prior blocks or extended commits) could cause corruption if not thoroughly tested. Confirm that all relevant indexes are updated consistently.
872-911: Restart safety for in-process CometBFT node.When stopping and restarting the node from within the same process, confirm that ephemeral states (e.g., ephemeral peer sessions, handshake data) are properly cleared. The
cleanupFncallstmNode.Stop(), which is standard, but watch for any reuse pitfalls if the user restarts in the same session.
What is the issue
Running testnet using the snapshot of bitsong mainnet fails with this issue.
Cometbft internally creates a new app state using the last commit from the snapshot when the testnet node runs first time using an existing snapshot.
Here is the relevant code for the operation.
https://github.com/cometbft/cometbft/blob/v0.38.16/consensus/state.go#L197
Reconstructing the last commit requires the last validator set to be matched to the last validator signatures but we have a problem here.
Currently, the last validator set is reset to the testnet node validator properly but last validator signatures still remain as the old ones from the snapshot.
What is the solution
Cosmos SDK already supports testnetify function to reset the last commit for testnet node validator but it is designed for only the chains that use legacy commit type of CometBFT.
Bitsong is using extended commit type, which is an enhanced version of legacy commit type introduced by recent versions of CometBFT and we need to make testnetify function compatible with extended commit type.
What are the changes
Key changes for fixing the issue:
https://github.com/bitsongofficial/go-bitsong/pull/279/files#diff-97289af2ab66016f168df6fccac70626d125f170e5cf0841fcdc8fa624999608R701-R727
Summary by CodeRabbit
New Features
Refactor