-
Notifications
You must be signed in to change notification settings - Fork 596
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
perf: minimize necessary execution on recvpacket checktx #6302
Conversation
WalkthroughWalkthroughThe recent changes in the Changes
Possibly related 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Out of diff range and nitpick comments (1)
modules/core/ante/ante.go (1)
4-5
: Consider grouping standard library imports separately from third-party imports for better readability according to the Go style guide.
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- modules/core/ante/ante.go (3 hunks)
- modules/core/ante/ante_test.go (3 hunks)
Additional Context Used
Path-based Instructions (2)
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"
Additional comments not posted (5)
modules/core/ante/ante.go (2)
35-43
: The conditional logic inside theAnteHandle
function for handlingMsgRecvPacket
types introduces a new methodrecvPacketCheckTx
for the check transaction context. Ensure that this method is thoroughly tested to handle all edge cases, especially since it bypasses some of the standard processing inRecvPacket
.
105-129
: The implementation ofrecvPacketCheckTx
is crucial for the performance improvement. It's good to see error wrapping used for better error context. However, ensure that the error messages are clear and provide enough information for debugging. Also, consider adding more detailed comments explaining the rationale behind each step, especially the use ofCacheContext
which is critical for avoiding state changes during simulations.modules/core/ante/ante_test.go (3)
4-4
: The import of thefmt
package is appropriate given its usage in the new test case for simulating an application callback error. This is a good use of the package for generating error messages in test scenarios.
347-359
: The new test case "success on app callback error, app callbacks are skipped for performance" is crucial for validating the performance improvements by skipping application callbacks. Ensure that this test case adequately simulates the conditions under which therecvPacketCheckTx
would be used in production to verify its effectiveness and robustness.
470-477
: The test case "no success on recvPacket checkTx, no capability found" is important for ensuring robust error handling inrecvPacketCheckTx
. It tests the scenario where no capability is found, which should be a critical error case. This is a good addition to ensure the function behaves correctly under failure scenarios.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very clean! :)
I like the godoc comments and tests, very nice! ❤️
modules/core/ante/ante.go
Outdated
response *channeltypes.MsgRecvPacketResponse | ||
err error | ||
) | ||
if ctx.IsCheckTx() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So confusing to have isCheckTx/isReCheckTx and ExecMode()
😅
Just noting for reviewers that here we are in CheckTxMode
explicitly because we've checked also !simulate
above on L28.
On the v0.50+ line there is union between IsCheckTx=true
and ExecMode == execModeSimulate
(after cosmos/cosmos-sdk#20346). i.e. when IsCheckTx
is true when in execModeSimulate
(this is because simulateTx runs on the checkState multistore)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is where it should be the other way around like you mentioned @colin-axner. When recheckTx=true then checkTx=true... because ¯_(ツ)_/¯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- modules/core/ante/ante.go (3 hunks)
- modules/core/ante/ante_test.go (4 hunks)
Files skipped from review as they are similar to previous changes (2)
- modules/core/ante/ante.go
- modules/core/ante/ante_test.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Out of diff range and nitpick comments (2)
CHANGELOG.md (2)
Line range hint
179-179
: Consider using descriptive text for URLs to improve readability and accessibility.- * [\#6193](https://github.com/cosmos/ibc-go/pull/6193) Bump Cosmos SDK to v0.50.6. + * [\#6193](Bump Cosmos SDK to v0.50.6)[https://github.com/cosmos/ibc-go/pull/6193]
Line range hint
194-194
: Remove trailing spaces to maintain clean and consistent formatting in the document.- * (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.Also applies to: 244-244
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"
Quality Gate passed for 'ibc-go'Issues Measures |
* perf: only perform core ibc logic on recvpacket checktx * try me linter * fix: reorder if and add comment * chore: add changelog entry (cherry picked from commit 0993246) # Conflicts: # CHANGELOG.md # modules/core/ante/ante.go # modules/core/ante/ante_test.go
* perf: only perform core ibc logic on recvpacket checktx * try me linter * fix: reorder if and add comment * chore: add changelog entry (cherry picked from commit 0993246) # Conflicts: # CHANGELOG.md # modules/core/ante/ante.go # modules/core/ante/ante_test.go
* perf: only perform core ibc logic on recvpacket checktx * try me linter * fix: reorder if and add comment * chore: add changelog entry (cherry picked from commit 0993246) # Conflicts: # CHANGELOG.md # modules/core/ante/ante.go # modules/core/ante/ante_test.go
) (#6333) * perf: minimize necessary execution on recvpacket checktx (#6302) * perf: only perform core ibc logic on recvpacket checktx * try me linter * fix: reorder if and add comment * chore: add changelog entry (cherry picked from commit 0993246) # Conflicts: # CHANGELOG.md # modules/core/ante/ante.go # modules/core/ante/ante_test.go * fix: merge conflicts --------- Co-authored-by: colin axnér <[email protected]>
) (#6334) * perf: minimize necessary execution on recvpacket checktx (#6302) * perf: only perform core ibc logic on recvpacket checktx * try me linter * fix: reorder if and add comment * chore: add changelog entry (cherry picked from commit 0993246) # Conflicts: # CHANGELOG.md # modules/core/ante/ante.go # modules/core/ante/ante_test.go * fix: merge conflicts --------- Co-authored-by: colin axnér <[email protected]>
) (#6335) * perf: minimize necessary execution on recvpacket checktx (#6302) * perf: only perform core ibc logic on recvpacket checktx * try me linter * fix: reorder if and add comment * chore: add changelog entry (cherry picked from commit 0993246) # Conflicts: # CHANGELOG.md # modules/core/ante/ante.go # modules/core/ante/ante_test.go * fix: merge conflicts --------- Co-authored-by: colin axnér <[email protected]>
Description
implements suggestion started in #6248
ref: #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/
).godoc
comments.Files changed
in the GitHub PR explorer.SonarCloud Report
in the comment section below once CI passes.Summary by CodeRabbit
New Features
RecvPacket
execution incheckTx
within the redundant relay ante handler.Bug Fixes
OnRecvPacket
callbacks andRecvPacket
messages to ensure robust handling and stability.