Run account plus migration in v8 upgrade #2628
Conversation
WalkthroughThe changes in this pull request involve modifications to the upgrade handling logic within the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (3)
protocol/app/upgrades/v8.0/upgrade.go (3)
103-104: Consider implementing safeguards for the migration process.The migration is called without any safety mechanisms. Consider adding:
- Progress tracking for large datasets
- Batched processing to prevent memory issues
- Validation of migrated data
- Rollback mechanism in case of failures
Here's a suggested approach:
- MigrateAccountplusAccountState(sdkCtx, accountplusKeeper) + // Add progress tracking + sdkCtx.Logger().Info("Starting accountplus migration with batch processing") + const batchSize = 1000 + var processed, total int + + // First count total records + iterator := storetypes.KVStorePrefixIterator(store, nil) + for ; iterator.Valid(); iterator.Next() { + total++ + } + iterator.Close() + + // Process in batches + for { + migrated, done, err := MigrateAccountplusAccountStateBatch(sdkCtx, accountplusKeeper, batchSize) + if err != nil { + panic(fmt.Sprintf("Migration failed at record %d: %s", processed, err)) + } + processed += migrated + sdkCtx.Logger().Info(fmt.Sprintf("Migrated %d/%d records", processed, total)) + if done { + break + } + } + + // Validate migration + if err := ValidateMigration(sdkCtx, accountplusKeeper); err != nil { + panic(fmt.Sprintf("Migration validation failed: %s", err)) + }
Line range hint
22-66: Review memory management in MigrateAccountplusAccountState.The current implementation stores all keys and states in memory before applying changes. This could be problematic for large datasets.
Consider implementing a batched approach:
- var keysToDelete [][]byte - var accountStatesToSet []struct { - address sdk.AccAddress - accountState accountplustypes.AccountState - } + const batchSize = 1000 + keysToDelete := make([][]byte, 0, batchSize) + accountStatesToSet := make([]struct { + address sdk.AccAddress + accountState accountplustypes.AccountState + }, 0, batchSize)Also, consider adding transaction boundaries for atomic updates:
// Process batch if len(accountStatesToSet) >= batchSize { // Begin transaction store.BeginTransaction() // Set new states for _, item := range accountStatesToSet { k.SetAccountState(ctx, item.address, item.accountState) } // Delete old keys for _, key := range keysToDelete { store.Delete(key) } // Commit transaction store.CommitTransaction() // Clear batch keysToDelete = keysToDelete[:0] accountStatesToSet = accountStatesToSet[:0] }
Line range hint
32-35: Add data validation before panic.The panic on unexpected prefixed keys could be enhanced with more context.
Consider adding data validation:
- if bytes.HasPrefix(key, []byte(accountplustypes.AccountStateKeyPrefix)) { - panic(fmt.Sprintf("unexpected key with prefix %X found during migration", accountplustypes.AccountStateKeyPrefix)) - } + if bytes.HasPrefix(key, []byte(accountplustypes.AccountStateKeyPrefix)) { + // Log the problematic key and its value for debugging + value := iterator.Value() + panic(fmt.Sprintf( + "unexpected key with prefix %X found during migration. Key: %X, Value: %X", + accountplustypes.AccountStateKeyPrefix, + key, + value, + )) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
protocol/app/upgrades.go(1 hunks)protocol/app/upgrades/v8.0/upgrade.go(1 hunks)
🔇 Additional comments (2)
protocol/app/upgrades.go (1)
36-36: LGTM: AccountPlusKeeper integration looks correct.
The addition of app.AccountPlusKeeper to the upgrade handler is properly integrated into the existing upgrade infrastructure. The panic check for duplicate handlers above this change provides appropriate safety guarantees.
protocol/app/upgrades/v8.0/upgrade.go (1)
97-97: LGTM: Parameter addition is consistent.
The addition of accountplusKeeper parameter to CreateUpgradeHandler is properly typed and consistent with the migration requirements.
|
@Mergifyio backport release/protocol/v8.x |
✅ Backports have been created
|
(cherry picked from commit a957b13)
Co-authored-by: jayy04 <[email protected]>
Changelist
[Describe or list the changes made in this PR]
Test Plan
[Describe how this PR was tested (if applicable)]
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
New Features
Bug Fixes