-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
fix: Do not call Remove during Walk #19833
Conversation
WalkthroughThe recent updates across various modules aim to enhance data removal processes. By incorporating a method to gather keys for removal before deletion, the system ensures more effective and accurate removal operations. Furthermore, improvements like simplifying deletion logic for missed block bitmaps and utilizing 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 (
|
This comment has been minimized.
This comment has been minimized.
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.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (4)
- x/feegrant/keeper/keeper.go (2 hunks)
- x/gov/keeper/tally.go (2 hunks)
- x/slashing/keeper/signing_info.go (1 hunks)
- x/staking/keeper/cons_pubkey.go (2 hunks)
Additional comments: 7
x/staking/keeper/cons_pubkey.go (2)
- 205-205: The change to use
Clear
for deletion instead of iterating withWalk
is a good improvement in terms of efficiency and simplicity. Ensure that the range defined for deletion accurately targets the intended keys.- 214-230: Collecting keys before deletion is a good practice to avoid issues during iteration. Ensure that all keys are correctly identified and added to the
keysToRemove
list before proceeding with deletion.x/slashing/keeper/signing_info.go (1)
- 185-185: The change to directly clear the
ValidatorMissedBlockBitmap
instead of using a walk and remove operation simplifies the deletion process and improves efficiency. Ensure that the range defined for deletion accurately targets the intended keys.x/feegrant/keeper/keeper.go (1)
- 302-316: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [305-332]
Collecting keys before deletion in the
RemoveExpiredAllowances
function is a good practice to avoid issues during iteration. Ensure that all keys are correctly identified and added to thekeysToRemove
list before proceeding with deletion.x/gov/keeper/tally.go (3)
- 250-250: The introduction of
votesToRemove
to store votes for later removal is a good practice to avoid modifying a collection while iterating over it. This change aligns with the PR's objective to address unsafe deletions during iteration and enhances the safety and stability of the SDK.- 296-297: Appending votes to
votesToRemove
for later removal is correctly implemented. This approach ensures that the iteration over votes is not compromised by immediate deletions, which could lead to unexpected behavior or errors.- 303-307: Iterating over
votesToRemove
to remove votes from the store is correctly implemented. This deferred deletion strategy is a safer alternative to direct removal during iteration, effectively addressing the critical issue the PR aims to solve.
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.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional comments: 1
CHANGELOG.md (1)
- 101-101: The changelog entry for PR fix: Do not call Remove during Walk #19833 is clear and follows the established format. However, to enhance clarity, consider specifying the modules or areas of the SDK affected by this change, as this would provide readers with a better understanding of the impact of the fix.
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.
lgtm! great that none of those got released and most of collections migration happened in v0.51.
Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
Description
As it is not safe to perform deletions during iteration, we should remove these from the SDK.
We either:
Clear
method to which we pass a range.We are still doing it in a couple of places:
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...
!
in the type prefix if API or client breaking changeCHANGELOG.md
Reviewers 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...
Summary by CodeRabbit