perf: Speedup ante-handler's update client check, by only checking in first CheckTx #6241
Conversation
ValarDragon
requested review from
colin-axner,
AdityaSripal,
damiannolan and
chatton
as code owners
WalkthroughWalkthroughThe changes in this update focus on improving client type and module retrieval, enhancing validation for client updates, and optimizing call structures for better code organization and efficiency within the IBC module. 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.
Thanks for opening these improvements @ValarDragon. Will try to get others looped in when back from vacation this week and come to conclusions on how to merge/proceed with the suggestions!
Just left one q for now
| // don't do prune logic in CheckTx | ||
| if !ctx.IsCheckTx() { | ||
| cs.pruneOldestConsensusState(ctx, cdc, clientStore) | ||
| } |
There was a problem hiding this comment.
if we are avoiding this in CheckTx then should it also be avoided in ReCheckTx?
Is there something that is particularly expensive about this other than just wanting to avoid unnecessary store operations in CheckTx? I know we construct an iterator but it only used to access the oldest header info in store and breaks immediately after the first cb.
There was a problem hiding this comment.
Oh no it should have been both, great catch!
There was a problem hiding this comment.
Is this just because you've suggested in the issue that we should avoid client updates on ReCheckTx completely?
edit: ah, caught in cross commenting action 👍🏻
|
if we're going to add these checkTx variants for different msgs, might be worth sweeping them all into an |
![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 (2)
- modules/core/02-client/keeper/client.go (2 hunks)
- modules/light-clients/07-tendermint/update.go (1 hunks)
Additional Context Used
Path-based Instructions (2)
modules/core/02-client/keeper/client.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.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.
Additional comments not posted (4)
modules/core/02-client/keeper/client.go (3)
62-73: The implementation ofgetClientTypeAndModuleis efficient and aligns with the PR's objectives to streamline client type and module retrieval.
74-83: Integration ofgetClientTypeAndModuleinUpdateClientfunction is correctly implemented and improves the maintainability of the code.
131-150: The newCheckTxUpdateClientfunction is well-implemented, correctly using the new method for client retrieval and appropriately handling the CheckTx phase.modules/light-clients/07-tendermint/update.go (1)
140-143: Skipping prune logic inCheckTxandReCheckTxphases inUpdateStatefunction is a good optimization, reducing unnecessary operations during these phases.
There was a problem hiding this comment.
Hope you don't mind @ValarDragon but I will rug your code to another PR so you don't have to deal with updating this and keeping in sync with main. (since this is from an osmo fork we can't directly push and GH doesn't support "allow edits from maintainers" on org forks)
| } | ||
| } | ||
|
|
||
| _ = clientModule.UpdateState(ctx, clientID, clientMsg) |
There was a problem hiding this comment.
needs: #6276 to avoid an edge case where misbehaviour would panic on antes and never get in
|
closing, both additions added in #6279 and #6278 Thanks @ValarDragon for bringing this to our attention! |
Description
Component of #6232
This PR makes us not do misbehavior checks in CheckTx, and only do Commit verification checks once.
This also avoids expensive Commit Marshalling and pruning from the CheckTx logic
I'm unsure if this is the desired API though. Please feel free to just close this if there is another way you'd like this to be handled.
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