fix: reset validator signing info missed blocks counter #8053
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,9 @@ import ( | |
| "github.com/cosmos/cosmos-sdk/types/module" | ||
| upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types" | ||
|
|
||
| slashing "github.com/cosmos/cosmos-sdk/x/slashing/keeper" | ||
| slashingtypes "github.com/cosmos/cosmos-sdk/x/slashing/types" | ||
|
|
||
| "github.com/osmosis-labs/osmosis/v24/app/keepers" | ||
| "github.com/osmosis-labs/osmosis/v24/app/upgrades" | ||
| ) | ||
|
|
@@ -23,8 +26,25 @@ func CreateUpgradeHandler( | |
| return nil, err | ||
| } | ||
|
|
||
| // Now that all deprecated historical TWAPs have been pruned via v24, we can delete is isPruning state entry as well | ||
| keepers.TwapKeeper.DeleteDeprecatedHistoricalTWAPsIsPruning(ctx) | ||
|
|
||
| // Reset missed blocks counter for all validators | ||
| resetMissedBlocksCounter(ctx, keepers.SlashingKeeper) | ||
|
|
||
| return migrations, nil | ||
| } | ||
| } | ||
|
|
||
| // resetMissedBlocksCounter resets the missed blocks counter for all validators back to zero. | ||
| // This corrects a mistake that was overlooked in v24, where we cleared all missedBlocks but did not reset the counter. | ||
| func resetMissedBlocksCounter(ctx sdk.Context, slashingKeeper *slashing.Keeper) { | ||
| // Iterate over all validators signing info | ||
| slashingKeeper.IterateValidatorSigningInfos(ctx, func(address sdk.ConsAddress, info slashingtypes.ValidatorSigningInfo) (stop bool) { | ||
| // Reset missed blocks counter | ||
| info.MissedBlocksCounter = 0 | ||
| slashingKeeper.SetValidatorSigningInfo(ctx, address, info) | ||
|
There was a problem hiding this comment. This isn't right -- this will have the opposite of problem of setting a Ceil / potentially leading to negative missed blocks. You have to re-compute the missed blocks by loading all the bit arrays. There was a problem hiding this comment. Oh I see, couldn't we just reset the bit array to zero again for simplicity @ValarDragon There was a problem hiding this comment. By bit array to zero I mean, just deleting them and start from where we did in the migration There was a problem hiding this comment. Yes that would work! (But they would have to be deleted) There was a problem hiding this comment. Good catch btw, my bad for overlooking There was a problem hiding this comment. I believe I did it correctly here, and added a test that replicates mainnet better: fbcd43c |
||
|
|
||
| return false | ||
| }) | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,97 @@ | ||
| package v25_test | ||
|
|
||
| import ( | ||
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/stretchr/testify/suite" | ||
|
|
||
| moduletestutil "github.com/cosmos/cosmos-sdk/types/module/testutil" | ||
| "github.com/cosmos/cosmos-sdk/x/slashing" | ||
| v4 "github.com/cosmos/cosmos-sdk/x/slashing/migrations/v4" | ||
| slashingtypes "github.com/cosmos/cosmos-sdk/x/slashing/types" | ||
| upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types" | ||
|
|
||
| abci "github.com/cometbft/cometbft/abci/types" | ||
|
|
||
| sdk "github.com/cosmos/cosmos-sdk/types" | ||
|
|
||
| "github.com/osmosis-labs/osmosis/v24/app/apptesting" | ||
| ) | ||
|
|
||
| const ( | ||
| v25UpgradeHeight = int64(10) | ||
| ) | ||
|
|
||
| var ( | ||
| consAddr = sdk.ConsAddress(sdk.AccAddress([]byte("addr1_______________"))) | ||
| ) | ||
|
|
||
| type UpgradeTestSuite struct { | ||
| apptesting.KeeperTestHelper | ||
| } | ||
|
|
||
| func TestUpgradeTestSuite(t *testing.T) { | ||
| suite.Run(t, new(UpgradeTestSuite)) | ||
| } | ||
|
|
||
| func (s *UpgradeTestSuite) TestUpgrade() { | ||
| s.Setup() | ||
|
|
||
| preMigrationSigningInfo := s.prepareMissedBlocksCounterTest() | ||
|
|
||
| // Run the upgrade | ||
| dummyUpgrade(s) | ||
| s.Require().NotPanics(func() { | ||
| s.App.BeginBlocker(s.Ctx, abci.RequestBeginBlock{}) | ||
| }) | ||
|
|
||
| s.executeMissedBlocksCounterTest(preMigrationSigningInfo) | ||
| } | ||
|
|
||
| func dummyUpgrade(s *UpgradeTestSuite) { | ||
| s.Ctx = s.Ctx.WithBlockHeight(v25UpgradeHeight - 1) | ||
| plan := upgradetypes.Plan{Name: "v25", Height: v25UpgradeHeight} | ||
| err := s.App.UpgradeKeeper.ScheduleUpgrade(s.Ctx, plan) | ||
| s.Require().NoError(err) | ||
| _, exists := s.App.UpgradeKeeper.GetUpgradePlan(s.Ctx) | ||
| s.Require().True(exists) | ||
|
|
||
| s.Ctx = s.Ctx.WithBlockHeight(v25UpgradeHeight) | ||
| } | ||
|
|
||
| func (s *UpgradeTestSuite) prepareMissedBlocksCounterTest() slashingtypes.ValidatorSigningInfo { | ||
| cdc := moduletestutil.MakeTestEncodingConfig(slashing.AppModuleBasic{}).Codec | ||
| slashingStoreKey := s.App.AppKeepers.GetKey(slashingtypes.StoreKey) | ||
| store := s.Ctx.KVStore(slashingStoreKey) | ||
|
|
||
| preMigrationSigningInfo := slashingtypes.ValidatorSigningInfo{ | ||
| Address: consAddr.String(), | ||
| StartHeight: 10, | ||
| IndexOffset: 100, | ||
| JailedUntil: time.Time{}, | ||
| Tombstoned: false, | ||
| MissedBlocksCounter: 1000, | ||
| } | ||
|
|
||
| // store old signing info and bitmap entries | ||
| bz := cdc.MustMarshal(&preMigrationSigningInfo) | ||
| store.Set(v4.ValidatorSigningInfoKey(consAddr), bz) | ||
|
|
||
| return preMigrationSigningInfo | ||
| } | ||
|
|
||
| func (s *UpgradeTestSuite) executeMissedBlocksCounterTest(preMigrationSigningInfo slashingtypes.ValidatorSigningInfo) { | ||
| postMigrationSigningInfo, found := s.App.SlashingKeeper.GetValidatorSigningInfo(s.Ctx, consAddr) | ||
| s.Require().True(found) | ||
|
|
||
| // Check that the missed blocks counter was reset to zero | ||
| s.Require().Equal(int64(0), postMigrationSigningInfo.MissedBlocksCounter) | ||
|
|
||
| // Check that all other fields are the same | ||
| s.Require().Equal(preMigrationSigningInfo.Address, postMigrationSigningInfo.Address) | ||
| s.Require().Equal(preMigrationSigningInfo.StartHeight, postMigrationSigningInfo.StartHeight) | ||
| s.Require().Equal(preMigrationSigningInfo.IndexOffset, postMigrationSigningInfo.IndexOffset) | ||
| s.Require().Equal(preMigrationSigningInfo.JailedUntil, postMigrationSigningInfo.JailedUntil) | ||
| s.Require().Equal(preMigrationSigningInfo.Tombstoned, postMigrationSigningInfo.Tombstoned) | ||
| } |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Sorry, drive by change, periods at the end of some but not all changelog entries was bothering me.