perf: exclude pruning from tendermint update client in ante handler execution #6278
Conversation
WalkthroughWalkthroughThe recent update enhances the performance of the Changes
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 (
|
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
Out of diff range and nitpick comments (1)
modules/core/keeper/msg_server.go (1)
491-494: Consider adding a comment explaining the rationale behind the early return condition.This helps maintain clarity for future code maintenance and understanding the specific conditions under which application callbacks are skipped.
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (4)
- modules/core/ante/ante_test.go (2 hunks)
- modules/core/keeper/msg_server.go (1 hunks)
- modules/light-clients/07-tendermint/update.go (1 hunks)
- modules/light-clients/07-tendermint/update_test.go (1 hunks)
Additional Context Used
Path-based Instructions (4)
modules/light-clients/07-tendermint/update.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/light-clients/07-tendermint/update_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/keeper/msg_server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (3)
modules/light-clients/07-tendermint/update.go (1)
140-143: Skip pruning logic during CheckTx and ReCheckTx to enhance performance.This change aligns with the PR's objective to optimize performance by avoiding unnecessary pruning during transaction checks. Ensure that this modification does not inadvertently affect other functionalities by thorough testing, especially in scenarios where state updates are critical.
modules/core/ante/ante_test.go (1)
346-359: Ensure new test case adequately covers scenarios where app callbacks are skipped.This test case is crucial for verifying that the new logic to skip application callbacks under certain conditions works as intended. It's important to ensure that this test covers all relevant scenarios and edge cases to prevent regressions or unexpected behavior in production.
modules/light-clients/07-tendermint/update_test.go (1)
560-613: Ensure the new pruning logic is covered by tests.
|
looks like a lot of e2e test failures, are they related? 👀 |
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- modules/core/ante/ante_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- modules/core/ante/ante_test.go
|
Looks like hermes might be using events in checktx for write ack which is why e2e's fail to relay. By skipping app callbacks, we cannot emit those events |
|
Looks to be that tx simulation uses a context that has IsCheckTx as true. The if statement can be changed to use the exec mode instead |
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- modules/core/keeper/msg_server.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- modules/core/keeper/msg_server.go
|
Tested transfer e2e locally with cosmos/cosmos-sdk#20342 and seems to pass when context |
modules/core/keeper/msg_server.go
Outdated
| @@ -488,6 +488,11 @@ func (k *Keeper) RecvPacket(goCtx context.Context, msg *channeltypes.MsgRecvPack | |||
| return nil, errorsmod.Wrap(err, "receive packet verification failed") | |||
| } | |||
|
|
|||
| // performance: return early for the redundant relayer ante handler | |||
| if (ctx.IsCheckTx() || ctx.IsReCheckTx()) && ctx.ExecMode() != sdk.ExecModeSimulate { | |||
There was a problem hiding this comment.
For backport to v7.5 we should just add a new func which plumbs through the simulate bool from ante. We can do similar to the checkTxUpdateClient method I added on the ante handler itself.
There is no ability to check runTxModeSimulate on the v0.47 line (before it was changed to "execMode")
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/light-clients/07-tendermint/update.go (1 hunks)
- modules/light-clients/07-tendermint/update_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- modules/light-clients/07-tendermint/update.go
- modules/light-clients/07-tendermint/update_test.go
colin-axner
changed the title
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/light-clients/07-tendermint/update.go (1 hunks)
- modules/light-clients/07-tendermint/update_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- modules/light-clients/07-tendermint/update.go
- modules/light-clients/07-tendermint/update_test.go
| // performance: do not prune in checkTx | ||
| // simulation must prune for accurate gas estimation | ||
| if (!ctx.IsCheckTx() && !ctx.IsReCheckTx()) || ctx.ExecMode() == sdk.ExecModeSimulate { | ||
| cs.pruneOldestConsensusState(ctx, cdc, clientStore) | ||
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- modules/light-clients/07-tendermint/update_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- modules/light-clients/07-tendermint/update_test.go
There was a problem hiding this comment.
Thank you, @colin-axner. Do you think we should a changelog entry for this one?
We were thinking of adding a single changelog for all the improvements, but I guess since they will be split up in backports I can add one |
…andler-perf-exclusions
…osmos/ibc-go into colin/6232-antehandler-perf-exclusions
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (3)
CHANGELOG.md (3)
Line range hint
193-193: Remove trailing spaces to maintain clean and professional documentation.- * (core/ante) [\#6278](https://github.com/cosmos/ibc-go/pull/6278) Performance: Exclude pruning from tendermint client updates in ante handler executions. + * (core/ante) [\#6278](https://github.com/cosmos/ibc-go/pull/6278) Performance: Exclude pruning from tendermint client updates in ante handler executions.
Line range hint
243-243: Remove trailing spaces to maintain clean and professional documentation.- * (core/ante) [\#6278](https://github.com/cosmos/ibc-go/pull/6278) Performance: Exclude pruning from tendermint client updates in ante handler executions. + * (core/ante) [\#6278](https://github.com/cosmos/ibc-go/pull/6278) Performance: Exclude pruning from tendermint client updates in ante handler executions.
Line range hint
178-178: Replace the bare URL with a markdown link for better readability and consistency.- * [\#6232](https://github.com/cosmos/ibc-go/issues/6232) Bump CometBFT to v0.38.7. + * ([\#6232](https://github.com/cosmos/ibc-go/issues/6232)) Bump CometBFT to v0.38.7.
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"
Additional comments not posted (1)
CHANGELOG.md (1)
68-68: Ensure the changelog entry for PR #6278 is linked to the correct PR and issue.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the PR and issue links for the changelog entry. # Test: Search for the PR and issue links in the changelog. Expect: Correct links. rg --type md '\[\#6278\]\(https://github.com/cosmos/ibc-go/pull/6278\)' CHANGELOG.md rg --type md '\[\#6232\]\(https://github.com/cosmos/ibc-go/issues/6232\)' CHANGELOG.mdLength of output: 175
Script:
#!/bin/bash # Description: Verify the PR link for the changelog entry. # Test: Search for the PR link in the changelog. Expect: Correct link. rg --type md '\[\#6278\]\(https://github.com/cosmos/ibc-go/pull/6278\)' CHANGELOG.mdLength of output: 86
Script:
#!/bin/bash # Description: Verify the PR link for the changelog entry with a broader search pattern. # Test: Search for the PR link in the changelog. Expect: Correct link. rg --type md '#6278' CHANGELOG.mdLength of output: 190
|
…xecution (#6278) * performance: exit early on recvpacket to exclude app callbacks * perf: skip pruning on check tx and recheck tx * change fmt.Errorf to errors.New * fix: check execMode simulate in conditional * revert: recv packet changes * fix: account for simulation in pruning check * fix: reuse checkTx ctx * chore: add changelog --------- Co-authored-by: Damian Nolan <damiannolan@gmail.com> (cherry picked from commit 67b23cd) # Conflicts: # modules/light-clients/07-tendermint/update_test.go
…xecution (backport #6278) (#6331) * perf: exclude pruning from tendermint update client in ante handler execution (#6278) * performance: exit early on recvpacket to exclude app callbacks * perf: skip pruning on check tx and recheck tx * change fmt.Errorf to errors.New * fix: check execMode simulate in conditional * revert: recv packet changes * fix: account for simulation in pruning check * fix: reuse checkTx ctx * chore: add changelog --------- Co-authored-by: Damian Nolan <damiannolan@gmail.com> (cherry picked from commit 67b23cd) # Conflicts: # modules/light-clients/07-tendermint/update_test.go * fix: merge conflicts --------- Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
Description
ref: #6232
can only be backported to v8.3.x as SDK does not have exec mode api in context on v0.47.x
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