perf: minimize logic on rechecktx for recvpacket #6280
Conversation
WalkthroughWalkthroughThe changes made across various modules aim to optimize the RedundantRelayDecorator for CheckTx and RecheckTx operations in the IBC module. By streamlining the logic and reducing unnecessary processing, these modifications target performance improvements, particularly in consensus and mempool handling. Changes
Assessment against linked issues
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
modules/core/keeper/msg_server.go
Outdated
| @@ -526,6 +526,21 @@ func (k *Keeper) RecvPacket(goCtx context.Context, msg *channeltypes.MsgRecvPack | |||
| return &channeltypes.MsgRecvPacketResponse{Result: channeltypes.SUCCESS}, nil | |||
| } | |||
|
|
|||
| func (k *Keeper) RecvPacketReCheckTx(goCtx context.Context, msg *channeltypes.MsgRecvPacket) (*channeltypes.MsgRecvPacketResponse, error) { | |||
There was a problem hiding this comment.
if 04-channel msg server logic was moved, we could do with only a single public api for all this logic
colin-axner
requested review from
AdityaSripal,
damiannolan and
chatton
as code owners
colin-axner
added
backport-to-v8.3.x
backport-to-v7.5.x
priority
![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
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (5)
- modules/core/04-channel/keeper/ante.go (1 hunks)
- modules/core/04-channel/keeper/ante_test.go (1 hunks)
- modules/core/04-channel/keeper/packet.go (2 hunks)
- modules/core/ante/ante.go (3 hunks)
- modules/core/ante/ante_test.go (4 hunks)
Additional Context Used
GitHub Check Runs (1)
lint failure (5)
modules/core/04-channel/keeper/ante_test.go: [failure] 28-28:
stringinvalid-porthas 3 occurrences, make it a constant (goconst)
modules/core/04-channel/keeper/ante_test.go: [failure] 5-5:
duplicated-imports: Package "github.com/cosmos/ibc-go/v8/modules/core/04-channel/types" already imported (revive)
modules/core/04-channel/keeper/ante_test.go: [failure] 4-4:
ST1019: package "github.com/cosmos/ibc-go/v8/modules/core/04-channel/types" is being imported more than once (stylecheck)
Path-based Instructions (5)
modules/core/04-channel/keeper/ante.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/core/04-channel/keeper/ante_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/core/ante/ante.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/core/ante/ante_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/core/04-channel/keeper/packet.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (6)
modules/core/04-channel/keeper/ante.go (1)
9-22: The implementation ofRecvPacketReCheckTxinKeeperstruct effectively handles replay protection by checking the existence of the channel and applying replay protection logic. This is a crucial enhancement for ensuring that redundant relay transactions are filtered out during theReCheckTxphase.modules/core/04-channel/keeper/ante_test.go (1)
9-66: The test functionTestRecvPacketReCheckTxis well-structured and covers various scenarios including successful packet reception, channel not found, and redundant relay. This ensures that the new replay protection logic is robustly tested.modules/core/ante/ante.go (1)
105-118: The functionrecvPacketReCheckTxwithinRedundantRelayDecoratoris designed to handle the core IBC receiving logic during theReCheckTxphase, skipping application logic for performance. This separation is crucial for optimizing the handling of packet receptions in different transaction contexts.modules/core/ante/ante_test.go (2)
Line range hint
178-492: The test functionTestAnteDecoratorCheckTxthoroughly tests the new logic in theAnteHandlefunction ofRedundantRelayDecorator, covering various scenarios including success on new messages and failure on redundant messages. This comprehensive testing ensures that the new logic behaves as expected under different conditions.
493-583: The test functionTestAnteDecoratorReCheckTxeffectively tests theReCheckTxlogic in theAnteHandlefunction, ensuring that new and redundant messages are handled correctly. This is crucial for verifying the performance optimizations made in therecvPacketReCheckTxfunction.modules/core/04-channel/keeper/packet.go (1)
191-209: The separation of replay protection logic into theapplyReplayProtectionfunction fromRecvPacketis a significant improvement. This not only cleans up theRecvPacketfunction but also enhances modularity and maintainability of the code.
…mos/ibc-go into colin/6232-perf-rechecktx-recvpacket
…rechecktx-recvpacket
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- modules/core/04-channel/keeper/ante_test.go (1 hunks)
- modules/core/ante/ante.go (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- modules/core/04-channel/keeper/ante_test.go
- modules/core/ante/ante.go
|
|
||
| // RecvPacketReCheckTx applies replay protection ensuring that when relay messages are | ||
| // re-executed in ReCheckTx, we can appropriately filter out redundant relay transactions. | ||
| func (k *Keeper) RecvPacketReCheckTx(ctx sdk.Context, packet types.Packet) error { |
There was a problem hiding this comment.
Wondering if there would be an alternative way to implement this where we don't need to increase the public API of the channel keeper...
Also, for my understanding: why what we do in ReCheckTx is different from what we do in CheckTx?
There was a problem hiding this comment.
Outside of copy pasting the code, you would need to increase the public API of the channel keeper. I do not recommend copy pasting the code due to the codes sensitivity. It'd be nice to limit the channel keeper API, but we need to do some restructuring and make use of the internal package more. I'd say we should start by addressing #19 and moving the respective msg servers to their respective keepers
There was a problem hiding this comment.
ReCheckTx is rerun on the mempool after every block commit. It is redundant to recheck proofs and other logic which will not change between commits
Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
…mos/ibc-go into colin/6232-perf-rechecktx-recvpacket
| func (rrd RedundantRelayDecorator) recvPacketReCheckTx(ctx sdk.Context, msg *channeltypes.MsgRecvPacket) (*channeltypes.MsgRecvPacketResponse, error) { | ||
| // If the packet was already received, perform a no-op | ||
| // Use a cached context to prevent accidental state changes | ||
| cacheCtx, writeFn := ctx.CacheContext() |
There was a problem hiding this comment.
added for consistency with checkTx
|
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (3)
CHANGELOG.md (3)
Line range hint
195-195: Remove trailing spaces.- * (core/ante) [\#6302](https://github.com/cosmos/ibc-go/pull/6302) Performance: Skip app callbacks during RecvPacket execution in checkTx within the redundant relay ante handler. + * (core/ante) [\#6302](https://github.com/cosmos/ibc-go/pull/6302) Performance: Skip app callbacks during RecvPacket execution in checkTx within the redundant relay ante handler.
Line range hint
245-245: Remove trailing spaces.- * (core/ante) [\#6280](https://github.com/cosmos/ibc-go/pull/6280) Performance: Skip redundant proof checking in RecvPacket execution in reCheckTx within the redundant relay ante handler. + * (core/ante) [\#6280](https://github.com/cosmos/ibc-go/pull/6280) Performance: Skip redundant proof checking in RecvPacket execution in reCheckTx within the redundant relay ante handler.
Line range hint
180-180: Use a descriptive text for the URL to improve accessibility and SEO.- * [\#6193](https://github.com/cosmos/ibc-go/pull/6193) Bump Cosmos SDK to v0.50.6. + * [Bump Cosmos SDK to v0.50.6](https://github.com/cosmos/ibc-go/pull/6193).
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional Context Used
Path-based Instructions (1)
CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
* perf: minimize logic on rechecktx for recvpacket * refactor: rework layout for recvpacket rechecktx * test: add tests for 04-channel rechecktx func * test: add tests for core ante handler * chore: add comment explaining is rechecktx usage * linter appeasement * chore: add changelog entry * Update modules/core/ante/ante.go Co-authored-by: Carlos Rodriguez <carlos@interchain.io> * imp: use cached ctx for consistency * refactor: change added test to use expected errors * lint --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> (cherry picked from commit 56ae97d) # Conflicts: # modules/core/04-channel/keeper/packet.go # modules/core/ante/ante.go
* perf: minimize logic on rechecktx for recvpacket * refactor: rework layout for recvpacket rechecktx * test: add tests for 04-channel rechecktx func * test: add tests for core ante handler * chore: add comment explaining is rechecktx usage * linter appeasement * chore: add changelog entry * Update modules/core/ante/ante.go Co-authored-by: Carlos Rodriguez <carlos@interchain.io> * imp: use cached ctx for consistency * refactor: change added test to use expected errors * lint --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> (cherry picked from commit 56ae97d) # Conflicts: # modules/core/04-channel/keeper/packet.go # modules/core/ante/ante.go
* perf: minimize logic on rechecktx for recvpacket * refactor: rework layout for recvpacket rechecktx * test: add tests for 04-channel rechecktx func * test: add tests for core ante handler * chore: add comment explaining is rechecktx usage * linter appeasement * chore: add changelog entry * Update modules/core/ante/ante.go Co-authored-by: Carlos Rodriguez <carlos@interchain.io> * imp: use cached ctx for consistency * refactor: change added test to use expected errors * lint --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> (cherry picked from commit 56ae97d) # Conflicts: # modules/core/ante/ante.go
) * perf: minimize logic on rechecktx for recvpacket (#6280) * perf: minimize logic on rechecktx for recvpacket * refactor: rework layout for recvpacket rechecktx * test: add tests for 04-channel rechecktx func * test: add tests for core ante handler * chore: add comment explaining is rechecktx usage * linter appeasement * chore: add changelog entry * Update modules/core/ante/ante.go Co-authored-by: Carlos Rodriguez <carlos@interchain.io> * imp: use cached ctx for consistency * refactor: change added test to use expected errors * lint --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> (cherry picked from commit 56ae97d) # Conflicts: # modules/core/04-channel/keeper/packet.go # modules/core/ante/ante.go * fix: merge conflicts * lint * lint --------- Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
) * perf: minimize logic on rechecktx for recvpacket (#6280) * perf: minimize logic on rechecktx for recvpacket * refactor: rework layout for recvpacket rechecktx * test: add tests for 04-channel rechecktx func * test: add tests for core ante handler * chore: add comment explaining is rechecktx usage * linter appeasement * chore: add changelog entry * Update modules/core/ante/ante.go Co-authored-by: Carlos Rodriguez <carlos@interchain.io> * imp: use cached ctx for consistency * refactor: change added test to use expected errors * lint --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> (cherry picked from commit 56ae97d) # Conflicts: # modules/core/04-channel/keeper/packet.go # modules/core/ante/ante.go * fix: merge conflicts --------- Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
) * perf: minimize logic on rechecktx for recvpacket (#6280) * perf: minimize logic on rechecktx for recvpacket * refactor: rework layout for recvpacket rechecktx * test: add tests for 04-channel rechecktx func * test: add tests for core ante handler * chore: add comment explaining is rechecktx usage * linter appeasement * chore: add changelog entry * Update modules/core/ante/ante.go Co-authored-by: Carlos Rodriguez <carlos@interchain.io> * imp: use cached ctx for consistency * refactor: change added test to use expected errors * lint --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> (cherry picked from commit 56ae97d) # Conflicts: # modules/core/ante/ante.go * fix: merge conflicts --------- Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
Description
a riff on top of #6248
closes: #6232
can be backported to any release line as patch release
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor
Tests