fix: create delegation reverse index over multiple blocks at 1000 items per block #622
Conversation
|
@PaddyMc your pull request is missing a changelog! |
PaddyMc
force-pushed
the
fix/staking-migration
branch
2 times, most recently
from
d83c4c6 to
d99377a
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
x/staking/keeper/abci.go
Outdated
| func() { | ||
| defer func() { | ||
| if r := recover(); r != nil { | ||
| k.Logger(sdk.UnwrapSDKContext(ctx)).Error("Panic in MigrateDelegationsByValidatorIndex", "recover", r) | ||
| } | ||
| }() | ||
|
|
||
| // Only migrate 1000 items per block to make sure block times don't grow too much | ||
| k.MigrateDelegationsByValidatorIndex(sdk.UnwrapSDKContext(ctx), 1000) | ||
| }() |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
PaddyMc
force-pushed
the
fix/staking-migration
branch
from
d99377a to
8e4a89e
Compare
PaddyMc
force-pushed
the
fix/staking-migration
branch
from
8e4a89e to
a8ff26f
Compare
| }() | ||
|
|
||
| // Only migrate 10000 items per block to make the migration as fast as possible | ||
| k.MigrateDelegationsByValidatorIndex(sdk.UnwrapSDKContext(ctx), 10000) |
Check warning
Code scanning / gosec
Errors unhandled. Warning
| // Check the store to see if there is a value stored under the key | ||
| key := store.Get(types.NextMigrateDelegationsByValidatorIndexKey) | ||
| if key != nil { | ||
| // Users will never see this error as if there is an error the function defaults to the legacy implementation below | ||
| return fmt.Errorf("store migration is not finished, try again later") | ||
| } |
There was a problem hiding this comment.
Can we add a TODO to remove after v26, so we don't keep code that people a year from now will not remember why it was added.
| delAddrLen := bz[0] | ||
| bz = bz[1:] // remove the length byte of delegator address. | ||
| if len(bz) == 0 { | ||
| return nil, nil, fmt.Errorf("no bytes left to parse delegator address: %X", bz) | ||
| } | ||
|
|
||
| del := bz[:int(delAddrLen)] | ||
| bz = bz[int(delAddrLen):] // remove the length byte of a delegator address | ||
| if len(bz) == 0 { | ||
| return nil, nil, fmt.Errorf("no bytes left to parse delegator address: %X", bz) | ||
| } |
There was a problem hiding this comment.
Im confused by the comments here. It looks like we remove the length bye of the delegator address, and then the comments are saying we do it again on the same bz that we already removed the length byte on.
There was a problem hiding this comment.
Ah this is an incorrect comment what happens is:
- remove the length prefix
- remove the delegator address
Description
Currently our migration takes 1 min to completed with network effects at worst case scenario we could see 15 mins downtime.
This PR migrates the reverse index creation at 1000 items per block, this number was chosen to have the migration run for 1hr.
(190_000_000 * 2) / 60 => 1hr-ish
How to test
run tests
x/staking/keepergo test ./... --run TestKeeperTestSuite/TestDelegationsByValidatorMigrationsrun
in-place-testnetand query the nodeosmosisd q staking delegations-to osmovaloper14lzvt4gdwh2q4ymyjqma0p4j4aykpn92l4warrin-place-testneterrorGetValidator, debug through the query working as expected