[WIP] Skip endblocker flag #25091
[WIP] Skip endblocker flag #25091
Conversation
📝 WalkthroughWalkthroughA new mechanism to optionally skip the EndBlocker logic in the BaseApp was introduced. This includes a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant App
participant BaseApp
User->>CLI: Start node with --skip-endblocker flag
CLI->>App: Create application instance
CLI->>App: getBaseAppFromApp()
App->>BaseApp: (via method or reflection) retrieve BaseApp pointer
CLI->>BaseApp: SetSkipEndBlocker(true)
CLI->>App: Start application
App->>BaseApp: endBlock(ctx)
BaseApp->>BaseApp: Check skipEndBlocker flag
alt skipEndBlocker is true
BaseApp-->>App: Return empty EndBlock, no error
else skipEndBlocker is false
BaseApp->>BaseApp: Run normal EndBlocker logic
BaseApp-->>App: Return EndBlock result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 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: 1
🧹 Nitpick comments (1)
server/start.go (1)
637-671: Reasonable approach with appropriate fallback handling.The function uses a sensible two-tier approach: trying the interface method first, then falling back to reflection for embedded fields. The reflection logic is implemented correctly with proper type checking and safety guards.
Consider adding a comment about the reflection approach being fragile and dependent on specific embedding patterns, as this could help future maintainers understand the constraints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (3)
baseapp/baseapp.go(2 hunks)baseapp/options.go(2 hunks)server/start.go(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (7)
baseapp/options.go (1)
147-150: LGTM! Well-structured option function.The function follows the established pattern for BaseApp options and provides a clean way to configure EndBlocker skipping. The comment clearly explains the use case.
baseapp/baseapp.go (2)
166-168: LGTM! Well-documented field addition.The field placement and documentation are appropriate. The field is logically grouped with similar configuration flags and has a clear comment explaining its purpose.
742-745: LGTM! Clean early return implementation.The early return logic is well-implemented and maintains the method contract while effectively bypassing EndBlocker processing when the flag is set. The comment clearly explains the behavior.
server/start.go (4)
13-13: LGTM! Necessary imports for the new functionality.The
reflectandbaseappimports are appropriately added to support the BaseApp extraction and configuration functionality.Also applies to: 43-43
84-84: LGTM! Flag constant follows established conventions.The constant name and value follow the established patterns for CLI flags in this codebase.
694-703: LGTM! Well-implemented flag handling with appropriate logging.The implementation correctly checks the flag, attempts to configure the BaseApp, and provides appropriate logging for both success and failure cases. The defensive approach of continuing execution even if BaseApp access fails is appropriate.
1088-1088: LGTM! Proper flag registration with clear documentation.The flag is correctly registered as a boolean with an appropriate default value and clear help text explaining its purpose and use case.
| // SetSkipEndBlocker sets the skipEndBlocker flag for the BaseApp. | ||
| func (app *BaseApp) SetSkipEndBlocker(skipEndBlocker bool) { | ||
| app.skipEndBlocker = skipEndBlocker | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add sealed check for consistency and safety.
The method should include a sealed check like other setter methods in this file to prevent modification after BaseApp initialization.
Apply this diff to add the sealed check:
// SetSkipEndBlocker sets the skipEndBlocker flag for the BaseApp.
func (app *BaseApp) SetSkipEndBlocker(skipEndBlocker bool) {
+ if app.sealed {
+ panic("SetSkipEndBlocker() on sealed BaseApp")
+ }
app.skipEndBlocker = skipEndBlocker
}📝 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.
| // SetSkipEndBlocker sets the skipEndBlocker flag for the BaseApp. | |
| func (app *BaseApp) SetSkipEndBlocker(skipEndBlocker bool) { | |
| app.skipEndBlocker = skipEndBlocker | |
| } | |
| // SetSkipEndBlocker sets the skipEndBlocker flag for the BaseApp. | |
| func (app *BaseApp) SetSkipEndBlocker(skipEndBlocker bool) { | |
| if app.sealed { | |
| panic("SetSkipEndBlocker() on sealed BaseApp") | |
| } | |
| app.skipEndBlocker = skipEndBlocker | |
| } |
🤖 Prompt for AI Agents
In baseapp/options.go around lines 417 to 420, the SetSkipEndBlocker method
lacks a sealed check to prevent modifications after BaseApp initialization. Add
a check at the start of the method to verify if the BaseApp is sealed, and if
so, panic or return an error to block further changes. This ensures consistency
and safety like other setter methods in the file.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
baseapp/abci.go
Outdated
| var response *abci.ExecTxResult | ||
|
|
||
| if _, err := app.txDecoder(rawTx); err == nil { | ||
| response = app.deliverTx(rawTx) | ||
| if app.queryOnlyMode { | ||
| response = app.deliverTxQueryOnly(rawTx) | ||
| } else { | ||
| response = app.deliverTx(rawTx) | ||
| } |
There was a problem hiding this comment.
Change potentially affects state.
Call sequence:
(*github.com/cosmos/cosmos-sdk/baseapp.BaseApp).internalFinalizeBlock (baseapp/abci.go:718)
(*github.com/cosmos/cosmos-sdk/baseapp.BaseApp).FinalizeBlock (baseapp/abci.go:885)
This is an untested vibe-coded flag to skip the EndBlocker.
Summary by CodeRabbit
New Features
Chores