(Backport) fix(x/authz)!: limit expired authz grant pruning to 200 per block #513
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -8,7 +8,7 @@ | |||
| // BeginBlocker is called at the beginning of every block | |||
| func BeginBlocker(ctx sdk.Context, keeper keeper.Keeper) { | |||
| // delete all the mature grants | |||
| if err := keeper.DequeueAndDeleteExpiredGrants(ctx); err != nil { | |||
| if err := keeper.DequeueAndDeleteExpiredGrants(ctx, 200); err != nil { | |||
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
|
Because this isn't state compatible, lets target the osmo/v0.47.5 branch. That will behave like our "main" branch |
|
@czarcas7ic I think 54cc86a this PR also might have been state breaking should we revert it and also have it targeted toward osmo/v0.47.5 branch? |
czarcas7ic
force-pushed
the
osmo/v0.47.5
branch
from
8992442
to
e95dddd
Compare
|
Warning Rate Limit Exceeded@czarcas7ic has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 11 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe recent updates involve modifying 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (3)
- x/authz/keeper/keeper.go (2 hunks)
- x/authz/keeper/keeper_test.go (2 hunks)
- x/authz/module/abci.go (1 hunks)
Additional comments: 2
x/authz/keeper/keeper.go (1)
- 374-394: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [377-408]
The implementation of the
limitparameter in theDequeueAndDeleteExpiredGrantsfunction is a significant improvement aimed at enhancing performance by controlling the number of expired grants pruned per block. This change effectively addresses potential performance bottlenecks by limiting the loop iterations based on the provided limit. However, it's crucial to ensure that the function correctly handles edge cases, such as when the limit is set to 0 (indicating no pruning should occur) or when it's set to a very high number that could potentially prune more grants than available. It would be beneficial to add tests covering these edge cases to ensure the system behaves as expected under various conditions.x/authz/keeper/keeper_test.go (1)
- 392-465: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [373-461]
The modifications to the tests in
keeper_test.go, including the addition of thelimitparameter to theDequeueAndDeleteExpiredGrantsfunction calls and the introduction of new tests to verify the pruning behavior based on differentlimitvalues, are commendable. These changes help ensure that the new functionality works as expected under various conditions. It's crucial to maintain comprehensive test coverage, especially for edge cases and scenarios that might involve concurrent modifications to the grants being pruned. Consider adding more tests to cover such scenarios, ensuring the system's resilience and reliability.
| @@ -8,7 +8,7 @@ import ( | |||
| // BeginBlocker is called at the beginning of every block | |||
| func BeginBlocker(ctx sdk.Context, keeper keeper.Keeper) { | |||
| // delete all the mature grants | |||
| if err := keeper.DequeueAndDeleteExpiredGrants(ctx); err != nil { | |||
| if err := keeper.DequeueAndDeleteExpiredGrants(ctx, 200); err != nil { | |||
There was a problem hiding this comment.
The addition of a limit parameter to the DequeueAndDeleteExpiredGrants function call within the BeginBlocker function is a critical change aimed at improving performance by controlling the number of expired grants pruned per block. This approach helps in preventing potential performance bottlenecks when dealing with a large number of expired grants. However, the use of panic for error handling in this context raises concerns. In blockchain systems, stability is crucial, and panicking could lead to unintended consequences, including halting the blockchain. It's advisable to consider a more resilient error handling strategy that allows the system to gracefully handle errors without compromising stability.
Description
Backport of PR 18737 and PR 19315 from upstream
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!in the type prefix if API or client breaking changeSummary by CodeRabbit