fix(server): allow align block header with skip check header in grpc server #23244
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request introduces a new feature in the Cosmos SDK that allows aligning block headers with skip check headers in the gRPC server. The changes span multiple files across the server configuration and implementation, adding a new Changes
Sequence DiagramsequenceDiagram
participant Client
participant GRPCServer
participant Application
participant Context
Client->>GRPCServer: Request with configuration
GRPCServer->>Application: RegisterGRPCServerWithSkipCheckHeader(server, skipCheckHeader)
Application->>Context: CreateQueryContextWithCheckHeader(!skipCheckHeader)
Context-->>GRPCServer: Return context
GRPCServer-->>Client: Process request
Possibly Related PRs
Suggested Labels
Suggested Reviewers
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
|
github-actions
bot
added
C:Confix
mmsqe
requested review from
julienrbrt,
akhilkumarpilli,
a team,
hieuvubk,
kocubinski,
facundomedica,
aaronc and
testinginprod
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
baseapp/grpcserver.go (1)
29-31: Consider adding error handling for edge casesThe implementation should consider handling edge cases where the context creation fails specifically due to header validation. This would help in distinguishing between different types of failures and provide better error messages.
func (app *BaseApp) RegisterGRPCServerWithSkipCheckHeader(server gogogrpc.Server, skipCheckHeader bool) { + if skipCheckHeader { + app.logger.Info("header validation is disabled for gRPC server") + } // ... rest of the implementation ... sdkCtx, err := app.CreateQueryContextWithCheckHeader(height, false, !skipCheckHeader) if err != nil { + if !skipCheckHeader && errors.Is(err, sdkerrors.ErrInvalidHeader) { + return nil, errorsmod.Wrapf(err, "header validation failed") + } return nil, err }Also applies to: 57-57
server/config/config.toml.tpl (1)
174-176: Enhance the configuration documentation.The comment for
skip-check-headercould be more descriptive. Consider explaining:
- The purpose and impact of bypassing header checks
- When this option should be enabled/disabled
- Any security implications
-# SkipCheckHeader defines if the gRPC server should bypass check header. +# SkipCheckHeader defines if the gRPC server should bypass header checks. +# Enable this option only for pruned nodes or when running under gRPC. +# Note: Disabling header checks may affect chain security guarantees. skip-check-header = {{ .GRPC.SkipCheckHeader }}CHANGELOG.md (4)
Line range hint
1-1: Add version links to release headersThe version headers in the changelog should include links to the corresponding GitHub releases/tags for easier navigation.
- ## [v0.47.0](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.47.0) - 2023-03-14 + ## [v0.47.0]
48-48: Fix typo in changelog entryThere's a typo in "heaader" which should be "header".
- * (server) [#23244](https://github.com/cosmos/cosmos-sdk/pull/23244) Allow align block header with skip check heaader in grpc server. + * (server) [#23244](https://github.com/cosmos/cosmos-sdk/pull/23244) Allow align block header with skip check header in grpc server.
Line range hint
1-2000: Improve changelog organizationThe changelog is well-maintained but could benefit from:
- A table of contents for easier navigation
- Consistent formatting for PR references
- Clearer migration instructions with code examples
Line range hint
1-2000: Add deprecation notices more prominentlyConsider adding a dedicated "Deprecation Notices" section at the top of each release to highlight important deprecations that require user attention.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
CHANGELOG.md(1 hunks)baseapp/grpcserver.go(2 hunks)server/config/config.go(1 hunks)server/config/config.toml.tpl(1 hunks)server/grpc/server.go(2 hunks)server/types/app.go(1 hunks)server/v2/api/grpc/config.go(1 hunks)server/v2/testdata/app.toml(1 hunks)tools/confix/data/v2-app.toml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
server/config/config.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/grpc/server.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/api/grpc/config.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/types/app.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
baseapp/grpcserver.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: repo-analysis
- GitHub Check: test-system-v2
- GitHub Check: Analyze
- GitHub Check: Summary
🔇 Additional comments (5)
server/types/app.go (1)
38-40: Breaking Change: Interface Method ModificationThe removal of
RegisterGRPCServerand addition ofRegisterGRPCServerWithSkipCheckHeaderis a breaking change that will require updates in all implementations of theApplicationinterface. Consider providing a migration guide for users.server/grpc/server.go (1)
23-26: LGTM: Clean implementation of the new interfaceThe changes correctly implement the new interface and properly pass the configuration option to the registration method.
Also applies to: 43-43
baseapp/grpcserver.go (1)
26-27: LGTM: Good backward compatibilityThe default implementation maintains backward compatibility by calling the new method with
skipCheckHeaderset tofalse.server/v2/testdata/app.toml (1)
12-13: LGTM!The test configuration is consistent with the main configuration template and uses appropriate default values.
tools/confix/data/v2-app.toml (1)
47-48: LGTM!The configuration is consistent across all files and follows a secure-by-default approach with
skip-check-header = false.
|
First and foremost, what is the use case? Additionally, server is being killed from main (#22904 / #23238). I think this is the moment where the main branch diverges from release/v0.52.x, which will hold the last baseapp implementation. Could you target directly release/v0.52.x instead for the baseapp changes? The v2 changes should still target main, so this is fine, however this PR doesn't implement the proposed functionality. |
… server ensure overwrite with latest header under grpc only or pruned node
mmsqe
requested review from
tac0turtle,
auricom,
cool-develope,
lucaslopezf,
JulianToledano,
ziscky,
raynaudoe,
alpe and
sontrinh16
as code owners
mmsqe
changed the title
github-actions
bot
removed
C:server/v2
mmsqe
changed the title
ensure overwrite with latest header under grpc only or pruned node
Description
Closes: #XXXX
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
New Features
SkipCheckHeaderconfiguration to bypass header checks in gRPC serverConfiguration
skip-check-headeroption added to gRPC configuration filesfalseImprovements