fix: default comet port #23343
fix: default comet port #23343
Conversation
📝 WalkthroughWalkthroughThe pull request modifies the RPC client instantiation in the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
Documentation and Community
|
|
@tac0turtle your pull request is missing a changelog! |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/v2/cometbft/config.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
server/v2/cometbft/config.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: tests (00)
- GitHub Check: test-system-v2
- GitHub Check: Analyze
- GitHub Check: Summary
🔇 Additional comments (2)
server/v2/cometbft/config.go (2)
21-21: LGTM on the implementation.The port change implementation itself is clean and the configuration struct is well-documented with appropriate comments.
21-21: Verify the default port against CometBFT documentation.The change from port 26658 to 26656 appears to align with CometBFT's standard port configuration. Let's verify this against the official documentation.
✅ Verification successful
Port change to 26656 is correct and matches CometBFT documentation
The change aligns with CometBFT's documented standard P2P port (26656) as confirmed in multiple documentation files, including the production setup guide.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for port configuration in CometBFT documentation and code # Expected: Find evidence that 26656 is the correct default port # Search for port references in markdown files rg -A 5 "26656|26658" --type md # Search for default port configurations in Go files ast-grep --pattern 'Address.*=.*"tcp://.*:26656"'Length of output: 1013
server/v2/cometbft/config.go
Outdated
| @@ -18,7 +18,7 @@ func DefaultAppTomlConfig() *AppTomlConfig { | |||
| MinRetainBlocks: 0, | |||
| HaltHeight: 0, | |||
| HaltTime: 0, | |||
| Address: "tcp://127.0.0.1:26658", | |||
| Address: "tcp://127.0.0.1:26656", | |||
There was a problem hiding this comment.
Breaking Change: Default RPC server port has changed.
Changing the default port from 26658 to 26656 is a breaking change that might affect systems relying on the previous default configuration. This should be documented in the changelog.
Please ensure to:
- Add a breaking change entry to the CHANGELOG.md
- Consider adding a migration guide for users
|
Probably should be changed here too: cosmos-sdk/server/v2/cometbft/server.go Line 328 in c79e19d |
wrong flag, pushed the actual fix, this endpoint is for out of process |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/v2/cometbft/grpc.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
server/v2/cometbft/grpc.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: tests (00)
- GitHub Check: test-simapp-v2
- GitHub Check: test-system-v2
- GitHub Check: test-sim-nondeterminism
- GitHub Check: test-integration
- GitHub Check: Analyze
- GitHub Check: build (amd64)
- GitHub Check: Summary
🔇 Additional comments (2)
server/v2/cometbft/grpc.go (2)
527-527: LGTM! Configuration source change looks correct.The change from
AppTomlConfig.AddresstoConfigTomlConfig.RPC.ListenAddressaligns with the PR objective to fix the default Comet port.
527-527: Verify related changes in server.go.As suggested in the PR comments, there might be related changes needed in
server.go.Let's verify if there are any inconsistencies in the server configuration:
✅ Verification successful
No changes needed in server.go
The configurations in
server.goandgrpc.goserve different purposes:
server.gousesAppTomlConfig.Addressfor ABCI server creationgrpc.gousesConfigTomlConfig.RPC.ListenAddressfor RPC client creation🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any other occurrences of AppTomlConfig.Address in the codebase rg -A 5 "AppTomlConfig.Address" # Search for any other RPC client creation patterns ast-grep --pattern 'client.NewClientFromNode($_)'Length of output: 1220
| @@ -524,7 +524,7 @@ func (c *consensus[T]) maybeHandleExternalServices(ctx context.Context, req *abc | |||
|
|
|||
| // Handle tx service | |||
| if strings.HasPrefix(req.Path, "/cosmos.tx.v1beta1.Service") { | |||
| rpcClient, _ := client.NewClientFromNode(c.cfg.AppTomlConfig.Address) | |||
| rpcClient, _ := client.NewClientFromNode(c.cfg.ConfigTomlConfig.RPC.ListenAddress) | |||
There was a problem hiding this comment.
Handle potential errors from NewClientFromNode.
The error from NewClientFromNode is being ignored which could lead to nil pointer dereferences if the client creation fails.
Apply this diff to handle the error:
- rpcClient, _ := client.NewClientFromNode(c.cfg.ConfigTomlConfig.RPC.ListenAddress)
+ rpcClient, err := client.NewClientFromNode(c.cfg.ConfigTomlConfig.RPC.ListenAddress)
+ if err != nil {
+ return nil, fmt.Errorf("failed to create RPC client: %w", err)
+ }📝 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.
| rpcClient, _ := client.NewClientFromNode(c.cfg.ConfigTomlConfig.RPC.ListenAddress) | |
| rpcClient, err := client.NewClientFromNode(c.cfg.ConfigTomlConfig.RPC.ListenAddress) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to create RPC client: %w", err) | |
| } |
Co-authored-by: Alex | Interchain Labs <alex@skip.money> Co-authored-by: Julien Robert <julien@rbrt.fr> (cherry picked from commit 9f048eb)
Description
Closes: #23329
fix comet defult port
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit