Skip to content
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: less time intensive slashing migration #580

Merged
merged 15 commits into from
Mar 21, 2024
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (slashing) [#543](https://github.com/osmosis-labs/cosmos-sdk/pull/543) Make slashing not write sign info every block
* (authz) [#513](https://github.com/osmosis-labs/cosmos-sdk/pull/513) Limit expired authz grant pruning to 200 per block
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The phrase "Make slashing not write sign info every block" could be rephrased for clarity. Suggestion: "Optimize slashing by reducing sign info writes".

- * (slashing) [#543](https://github.com/osmosis-labs/cosmos-sdk/pull/543) Make slashing not write sign info every block
+ * (slashing) [#543](https://github.com/osmosis-labs/cosmos-sdk/pull/543) Optimize slashing by reducing sign info writes

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
* (authz) [#513](https://github.com/osmosis-labs/cosmos-sdk/pull/513) Limit expired authz grant pruning to 200 per block
* (authz) [#513](https://github.com/osmosis-labs/cosmos-sdk/pull/513) Limit expired authz grant pruning to 200 per block
* (slashing) [#543](https://github.com/osmosis-labs/cosmos-sdk/pull/543) Optimize slashing by reducing sign info writes

* (gov) [#514](https://github.com/osmosis-labs/cosmos-sdk/pull/514) Let gov hooks return an error
* (slashing) [#580](https://github.com/osmosis-labs/cosmos-sdk/pull/580) Less time intensive slashing migration

## [State Compatible]

Comment on lines 44 to 50
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [42-42]

The term "CacheKV speedups" might be more accurately described as "CacheKV performance improvements" to clearly convey the nature of the changes.

- * (store) [#525](https://github.com/osmosis-labs/cosmos-sdk/pull/525) CacheKV speedups
+ * (store) [#525](https://github.com/osmosis-labs/cosmos-sdk/pull/525) CacheKV performance improvements

📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [51-51]

The term "Removal of regex usage on denom validation" could be more descriptive. Suggesting: "Improve denom validation by removing regex usage".

- * (coin) [#570](https://github.com/osmosis-labs/cosmos-sdk/pull/570) Removal of regex usage on denom validation
+ * (coin) [#570](https://github.com/osmosis-labs/cosmos-sdk/pull/570) Improve denom validation by removing regex usage

📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [59-59]

The entry "Bumps IAVL v1 version for bug fixes" could be reworded for clarity and consistency with other entries. Suggestion: "Update IAVL v1 version to include bug fixes".

- * (store) [#575](https://github.com/osmosis-labs/cosmos-sdk/pull/575) Bumps IAVL v1 version for bug fixes
+ * (store) [#575](https://github.com/osmosis-labs/cosmos-sdk/pull/575) Update IAVL v1 version to include bug fixes

📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [72-72]

The phrase "Speedup to UnmarshalBalanceCompat" could be improved for clarity. Suggestion: "Enhance performance of UnmarshalBalanceCompat".

- * (x/bank) [#547](https://github.com/osmosis-labs/cosmos-sdk/pull/547) Speedup to UnmarshalBalanceCompat
+ * (x/bank) [#547](https://github.com/osmosis-labs/cosmos-sdk/pull/547) Enhance performance of UnmarshalBalanceCompat

📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [78-78]

The entry "Fix IAVL db grow issue" could be more descriptive. Suggestion: "Address IAVL database growth issue".

- * (store) [#537](https://github.com/osmosis-labs/cosmos-sdk/pull/537) Fix IAVL db grow issue
+ * (store) [#537](https://github.com/osmosis-labs/cosmos-sdk/pull/537) Address IAVL database growth issue

📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [100-100]

The term "Speedup to UnmarshalBalanceCompat" could be rephrased for better clarity. Suggestion: "Enhance UnmarshalBalanceCompat performance".

- * (x/bank) [#546](https://github.com/osmosis-labs/cosmos-sdk/pull/546) Speedup to UnmarshalBalanceCompat
+ * (x/bank) [#546](https://github.com/osmosis-labs/cosmos-sdk/pull/546) Enhance UnmarshalBalanceCompat performance

📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [109-109]

The entry "Add QueryEventForTxCmd cmd to subscribe and wait event for transaction by hash." could be rephrased for clarity. Suggestion: "Introduce QueryEventForTxCmd command for event subscription and waiting based on transaction hash."

- * (client/rpc) [#17274](https://github.com/cosmos/cosmos-sdk/pull/17274) Add `QueryEventForTxCmd` cmd to subscribe and wait event for transaction by hash.
+ * (client/rpc) [#17274](https://github.com/cosmos/cosmos-sdk/pull/17274) Introduce `QueryEventForTxCmd` command for event subscription and waiting based on transaction hash.

📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [114-114]

The phrase "Add MsgSubmitProposal SetMsgs method." could be more descriptive. Suggestion: "Introduce SetMsgs method in MsgSubmitProposal."

- * (x/gov) [#17387](https://github.com/cosmos/cosmos-sdk/pull/17387) Add `MsgSubmitProposal` `SetMsgs` method.
+ * (x/gov) [#17387](https://github.com/cosmos/cosmos-sdk/pull/17387) Introduce `SetMsgs` method in `MsgSubmitProposal`.

📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [115-115]

The entry "Emit VoterAddr in proposal_vote event." could be rephrased for clarity. Suggestion: "Include VoterAddr in the proposal_vote event."

- * (x/gov) [#17354](https://github.com/cosmos/cosmos-sdk/issues/17354) Emit `VoterAddr` in `proposal_vote` event.
+ * (x/gov) [#17354](https://github.com/cosmos/cosmos-sdk/issues/17354) Include `VoterAddr` in the `proposal_vote` event.

📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [117-117]

The phrase "Add MigrateHandler to allow reuse migrate genesis related function." could be improved for grammatical accuracy. Suggestion: "Introduce MigrateHandler for reusable migration of genesis-related functions."

- * (x/genutil) [#17296](https://github.com/cosmos/cosmos-sdk/pull/17296) Add `MigrateHandler` to allow reuse migrate genesis related function.
+ * (x/genutil) [#17296](https://github.com/cosmos/cosmos-sdk/pull/17296) Introduce `MigrateHandler` for reusable migration of genesis-related functions.

📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [123-123]

The phrase "Properly allow to combine depinject-enabled modules and non-depinject-enabled modules in app v2." could be rephrased for clarity. Suggestion: "Enable seamless integration of depinject-enabled and non-depinject-enabled modules in app v2."

- * (runtime) [#17284](https://github.com/cosmos/cosmos-sdk/pull/17284) Properly allow to combine depinject-enabled modules and non-depinject-enabled modules in app v2.
+ * (runtime) [#17284](https://github.com/cosmos/cosmos-sdk/pull/17284) Enable seamless integration of depinject-enabled and non-depinject-enabled modules in app v2.

📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [126-126]

The phrase "Do not try validate msgURL as web URL in draft-proposal command." could be improved for grammatical accuracy. Suggestion: "Avoid validating msgURL as a web URL in the draft-proposal command."

- * (x/gov,x/group) [#17220](https://github.com/cosmos/cosmos-sdk/pull/17220) Do not try validate `msgURL` as web URL in `draft-proposal` command.
+ * (x/gov,x/group) [#17220](https://github.com/cosmos/cosmos-sdk/pull/17220) Avoid validating `msgURL` as a web URL in the `draft-proposal` command.

📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [138-138]

The phrase "Improve simd prune UX by using the app default home directory and set pruning method as first variable argument (defaults to default)." could be rephrased for clarity. Suggestion: "Enhance simd prune user experience by utilizing the app's default home directory and setting the pruning method as the primary variable argument."

- * (cli) [#16856](https://github.com/cosmos/cosmos-sdk/pull/16856) Improve `simd prune` UX by using the app default home directory and set pruning method as first variable argument (defaults to default).
+ * (cli) [#16856](https://github.com/cosmos/cosmos-sdk/pull/16856) Enhance `simd prune` user experience by utilizing the app's default home directory and setting the pruning method as the primary variable argument.

📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [154-154]

The phrase "Add circuit breaker setter in baseapp." could be rephrased for clarity. Suggestion: "Introduce circuit breaker setter functionality in BaseApp."

- * (baseapp) [#16290](https://github.com/cosmos/cosmos-sdk/pull/16290) Add circuit breaker setter in baseapp.
+ * (baseapp) [#16290](https://github.com/cosmos/cosmos-sdk/pull/16290) Introduce circuit breaker setter functionality in BaseApp.

📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [156-156]

The phrase "Add WithExtensionOptions in tx Factory to allow SetExtensionOptions with given extension options." could be rephrased for clarity. Suggestion: "Introduce WithExtensionOptions in the transaction Factory to enable setting extension options."

- * (tx) [#15992](https://github.com/cosmos/cosmos-sdk/pull/15992) Add `WithExtensionOptions` in tx Factory to allow `SetExtensionOptions` with given extension options.
+ * (tx) [#15992](https://github.com/cosmos/cosmos-sdk/pull/15992) Introduce `WithExtensionOptions` in the transaction Factory to enable setting extension options.

📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [160-160]

The phrase "Make DefaultProposalHandler.ProcessProposalHandler return a ProcessProposal NoOp when using none or a NoOp mempool." could be rephrased for clarity. Suggestion: "Ensure DefaultProposalHandler.ProcessProposalHandler returns a NoOp for ProcessProposal when using a NoOp or empty mempool."

- * (baseapp) [#16407](https://github.com/cosmos/cosmos-sdk/pull/16407) Make `DefaultProposalHandler.ProcessProposalHandler` return a ProcessProposal NoOp when using none or a NoOp mempool.
+ * (baseapp) [#16407](https://github.com/cosmos/cosmos-sdk/pull/16407) Ensure `DefaultProposalHandler.ProcessProposalHandler` returns a NoOp for ProcessProposal when using a NoOp or empty mempool.

📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [163-163]

The phrase "Use cosmossdk.io/log package for logging instead of CometBFT logger." could be rephrased for clarity. Suggestion: "Adopt cosmossdk.io/log for logging, replacing the CometBFT logger."

- * (server) [#15984](https://github.com/cosmos/cosmos-sdk/pull/15984) Use `cosmossdk.io/log` package for logging instead of CometBFT logger.
+ * (server) [#15984](https://github.com/cosmos/cosmos-sdk/pull/15984) Adopt `cosmossdk.io/log` for logging, replacing the CometBFT logger.

📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [168-168]

The phrase "Update simulation to allow non-EOA accounts to stake." could be rephrased for clarity. Suggestion: "Modify simulation to enable staking for non-EOA accounts."

- * (x/staking) [#16068](https://github.com/osmosis-labs/cosmos-sdk/pull/16068) Update simulation to allow non-EOA accounts to stake.
+ * (x/staking) [#16068](https://github.com/osmosis-labs/cosmos-sdk/pull/16068) Modify simulation to enable staking for non-EOA accounts.

📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [170-170]

The phrase "Rename interface ExtensionOptionI back to TxExtensionOptionI to avoid breaking change." could be rephrased for clarity. Suggestion: "Revert interface name from ExtensionOptionI to TxExtensionOptionI to prevent breaking changes."

- * (types) [#16145](https://github.com/cosmos/cosmos-sdk/pull/16145) Rename interface `ExtensionOptionI` back to `TxExtensionOptionI` to avoid breaking change.
+ * (types) [#16145](https://github.com/cosmos/cosmos-sdk/pull/16145) Revert interface name from `ExtensionOptionI` to `TxExtensionOptionI` to prevent breaking changes.

📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [171-171]

The phrase "Add Close method to BaseApp for custom app to cleanup resource in graceful shutdown." could be rephrased for clarity. Suggestion: "Introduce Close method in BaseApp for resource cleanup during graceful shutdown of custom apps."

- * (baseapp) [#16193](https://github.com/cosmos/cosmos-sdk/pull/16193) Add `Close` method to `BaseApp` for custom app to cleanup resource in graceful shutdown.
+ * (baseapp) [#16193](https://github.com/cosmos/cosmos-sdk/pull/16193) Introduce `Close` method in `BaseApp` for resource cleanup during graceful shutdown of custom apps.

📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [176-176]

The phrase "Do not override some Comet config is purposely set differently in InterceptConfigsPreRunHandler." could be rephrased for clarity. Suggestion: "Avoid overriding specific Comet configurations that are intentionally set differently in InterceptConfigsPreRunHandler."

- * (server) [#16395](https://github.com/cosmos/cosmos-sdk/pull/16395) Do not override some Comet config is purposely set differently in `InterceptConfigsPreRunHandler`.
+ * (server) [#16395](https://github.com/cosmos/cosmos-sdk/pull/16395) Avoid overriding specific Comet configurations that are intentionally set differently in `InterceptConfigsPreRunHandler`.

📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [177-177]

The phrase "Fix StateSync Restore by excluding memory store." could be rephrased for clarity. Suggestion: "Correct StateSync restoration process by excluding the memory store."

- * (store) [#16449](https://github.com/cosmos/cosmos-sdk/pull/16449) Fix StateSync Restore by excluding memory store.
+ * (store) [#16449](https://github.com/cosmos/cosmos-sdk/pull/16449) Correct StateSync restoration process by excluding the memory store.

📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [178-178]

The phrase "Allow any addresses in client.ValidatePromptAddress." could be rephrased for clarity. Suggestion: "Enable validation for all addresses in client.ValidatePromptAddress."

- * (cli) [#16312](https://github.com/cosmos/cosmos-sdk/pull/16312) Allow any addresses in `client.ValidatePromptAddress`.
+ * (cli) [#16312](https://github.com/cosmos/cosmos-sdk/pull/16312) Enable validation for all addresses in `client.ValidatePromptAddress`.

Expand Down
5 changes: 5 additions & 0 deletions x/slashing/abci.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import (
"github.com/cosmos/cosmos-sdk/x/slashing/types"
)

var deprecatedBitArrayPruneLimitPerBlock = 2000
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could be convinced to lower this, but I would rather have the one time payment of deleting every block frontloaded at time of upgrade rather than slight degradation for a very long time.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would agree with this


// BeginBlocker check for infraction evidence or downtime of validators
// on every begin block
func BeginBlocker(ctx sdk.Context, req abci.RequestBeginBlock, k keeper.Keeper) {
Expand All @@ -23,4 +25,7 @@ func BeginBlocker(ctx sdk.Context, req abci.RequestBeginBlock, k keeper.Keeper)
for _, voteInfo := range req.LastCommitInfo.GetVotes() {
k.HandleValidatorSignatureWithParams(ctx, params, voteInfo.Validator.Address, voteInfo.Validator.Power, voteInfo.SignedLastBlock)
}

// If there are still entries for the deprecated MissedBlockBitArray, delete them up until we hit the per block limit
k.DeleteDeprecatedValidatorMissedBlockBitArray(ctx, deprecatedBitArrayPruneLimitPerBlock)
Dismissed Show dismissed Hide dismissed
}
42 changes: 42 additions & 0 deletions x/slashing/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"github.com/cosmos/cosmos-sdk/codec"
cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
sdk "github.com/cosmos/cosmos-sdk/types"
v4 "github.com/cosmos/cosmos-sdk/x/slashing/migrations/v4"
"github.com/cosmos/cosmos-sdk/x/slashing/types"
stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
)
Expand Down Expand Up @@ -115,3 +116,44 @@ func (k Keeper) deleteAddrPubkeyRelation(ctx sdk.Context, addr cryptotypes.Addre
store := ctx.KVStore(k.storeKey)
store.Delete(types.AddrPubkeyRelationKey(addr))
}

func (k Keeper) DeleteDeprecatedValidatorMissedBlockBitArray(ctx sdk.Context, iterationLimit int) {
store := ctx.KVStore(k.storeKey)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going between using store vs using params a few times. I thought params was the best at first since we already call getParams in the BeginBlocker, but it felt kind of awkward.

if store.Get(types.IsPruningKey) == nil {
return
}

// Iterate over all the validator signing infos and delete the deprecated missed block bit arrays
valSignInfoIter := sdk.KVStorePrefixIterator(store, types.ValidatorSigningInfoKeyPrefix)
defer valSignInfoIter.Close()

iterationCounter := 0
for ; valSignInfoIter.Valid(); valSignInfoIter.Next() {
address := types.ValidatorSigningInfoAddress(valSignInfoIter.Key())

// Creat anonymous function to scope defer statement
func() {
iter := storetypes.KVStorePrefixIterator(store, v4.DeprecatedValidatorMissedBlockBitArrayPrefixKey(address))
defer iter.Close()

for ; iter.Valid(); iter.Next() {
store.Delete(iter.Key())
iterationCounter++
if iterationCounter >= iterationLimit {
break
}
}
}()

if iterationCounter >= iterationLimit {
break
}
}

ctx.Logger().Info("Deleted deprecated missed block bit arrays", "count", iterationCounter)

// If we have deleted all the deprecated missed block bit arrays, we can delete the pruning key (set to nil)
if iterationCounter == 0 {
store.Delete(types.IsPruningKey)
}
}
19 changes: 12 additions & 7 deletions x/slashing/migrations/v4/keys.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,13 @@ import (
const MissedBlockBitmapChunkSize = 1024 // 2^10 bits

var (
ValidatorSigningInfoKeyPrefix = []byte{0x01}
validatorMissedBlockBitArrayKeyPrefix = []byte{0x02}
ValidatorSigningInfoKeyPrefix = []byte{0x01}
deprecatedValidatorMissedBlockBitArrayKeyPrefix = []byte{0x02}

// NOTE: sdk v0.50 uses the same key prefix for both deprecated and new missed block bitmaps.
// We needed to use a new key, because we are skipping deletion of all old keys at upgrade time
// due to how long this would bring the chain down. We use 0x10 here to prevent overlap with any future keys.
validatorMissedBlockBitMapKeyPrefix = []byte{0x10}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set to 0x10 to prevent overlap if more keys are introduced in the next sdk release we upgrade to

)

func ValidatorSigningInfoKey(v sdk.ConsAddress) []byte {
Expand All @@ -27,18 +32,18 @@ func ValidatorSigningInfoAddress(key []byte) (v sdk.ConsAddress) {
return sdk.ConsAddress(addr)
}

func validatorMissedBlockBitArrayPrefixKey(v sdk.ConsAddress) []byte {
return append(validatorMissedBlockBitArrayKeyPrefix, address.MustLengthPrefix(v.Bytes())...)
func DeprecatedValidatorMissedBlockBitArrayPrefixKey(v sdk.ConsAddress) []byte {
return append(deprecatedValidatorMissedBlockBitArrayKeyPrefix, address.MustLengthPrefix(v.Bytes())...)
}

func ValidatorMissedBlockBitArrayKey(v sdk.ConsAddress, i int64) []byte {
func DeprecatedValidatorMissedBlockBitArrayKey(v sdk.ConsAddress, i int64) []byte {
b := make([]byte, 8)
binary.LittleEndian.PutUint64(b, uint64(i))
return append(validatorMissedBlockBitArrayPrefixKey(v), b...)
return append(DeprecatedValidatorMissedBlockBitArrayPrefixKey(v), b...)
}

func validatorMissedBlockBitmapPrefixKey(v sdk.ConsAddress) []byte {
return append(validatorMissedBlockBitArrayKeyPrefix, address.MustLengthPrefix(v.Bytes())...)
return append(validatorMissedBlockBitMapKeyPrefix, address.MustLengthPrefix(v.Bytes())...)
}

func ValidatorMissedBlockBitmapKey(v sdk.ConsAddress, chunkIndex int64) []byte {
Expand Down
28 changes: 17 additions & 11 deletions x/slashing/migrations/v4/migrate.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,13 @@ func Migrate(ctx sdk.Context, cdc codec.BinaryCodec, store storetypes.KVStore, p
var missedBlocks []types.ValidatorMissedBlocks
iterateValidatorSigningInfos(ctx, cdc, store, func(addr sdk.ConsAddress, info types.ValidatorSigningInfo) (stop bool) {
bechAddr := addr.String()
localMissedBlocks := GetValidatorMissedBlocks(ctx, cdc, store, addr, params)

// We opt to reset all validators missed blocks to improve upgrade performance
// localMissedBlocks := GetValidatorMissedBlocks(ctx, cdc, store, addr, params)

missedBlocks = append(missedBlocks, types.ValidatorMissedBlocks{
Address: bechAddr,
MissedBlocks: localMissedBlocks,
MissedBlocks: []types.MissedBlock{},
})

return false
Expand All @@ -39,7 +41,10 @@ func Migrate(ctx sdk.Context, cdc codec.BinaryCodec, store storetypes.KVStore, p
return err
}

deleteValidatorMissedBlockBitArray(ctx, store, addr)
// We skip the deletion here in favor of spreading out across multiple block for performance reasons
// We set the isPruning key to true to indicate that we are in the process of pruning
store.Set(types.IsPruningKey, []byte{1})
// deleteDeprecatedValidatorMissedBlockBitArray(ctx, store, addr)

for _, b := range mb.MissedBlocks {
// Note: It is not necessary to store entries with missed=false, i.e. where
Expand Down Expand Up @@ -86,7 +91,7 @@ func iterateValidatorMissedBlockBitArray(
) {
for i := int64(0); i < params.SignedBlocksWindow; i++ {
var missed gogotypes.BoolValue
bz := store.Get(ValidatorMissedBlockBitArrayKey(addr, i))
bz := store.Get(DeprecatedValidatorMissedBlockBitArrayKey(addr, i))
if bz == nil {
continue
}
Expand Down Expand Up @@ -114,14 +119,15 @@ func GetValidatorMissedBlocks(
return missedBlocks
}

func deleteValidatorMissedBlockBitArray(ctx sdk.Context, store storetypes.KVStore, addr sdk.ConsAddress) {
iter := storetypes.KVStorePrefixIterator(store, validatorMissedBlockBitArrayPrefixKey(addr))
defer iter.Close()
// No longer use this
// func deleteDeprecatedValidatorMissedBlockBitArray(ctx sdk.Context, store storetypes.KVStore, addr sdk.ConsAddress) {
// iter := storetypes.KVStorePrefixIterator(store, DeprecatedValidatorMissedBlockBitArrayPrefixKey(addr))
// defer iter.Close()

for ; iter.Valid(); iter.Next() {
store.Delete(iter.Key())
}
}
// for ; iter.Valid(); iter.Next() {
// store.Delete(iter.Key())
// }
// }

func setMissedBlockBitmapValue(ctx sdk.Context, store storetypes.KVStore, addr sdk.ConsAddress, index int64, missed bool) error {
// get the chunk or "word" in the logical bitmap
Expand Down
20 changes: 11 additions & 9 deletions x/slashing/migrations/v4/migrate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package v4_test
import (
"testing"

"github.com/bits-and-blooms/bitset"
gogotypes "github.com/cosmos/gogoproto/types"
"github.com/stretchr/testify/require"

Expand Down Expand Up @@ -35,7 +34,7 @@ func TestMigrate(t *testing.T) {
// all even blocks are missed
missed := &gogotypes.BoolValue{Value: i%2 == 0}
bz := cdc.MustMarshal(missed)
store.Set(v4.ValidatorMissedBlockBitArrayKey(consAddr, i), bz)
store.Set(v4.DeprecatedValidatorMissedBlockBitArrayKey(consAddr, i), bz)
}

err := v4.Migrate(ctx, cdc, store, params)
Expand All @@ -44,15 +43,18 @@ func TestMigrate(t *testing.T) {
for i := int64(0); i < params.SignedBlocksWindow; i++ {
chunkIndex := i / v4.MissedBlockBitmapChunkSize
chunk := store.Get(v4.ValidatorMissedBlockBitmapKey(consAddr, chunkIndex))
require.NotNil(t, chunk)

bs := bitset.New(uint(v4.MissedBlockBitmapChunkSize))
require.NoError(t, bs.UnmarshalBinary(chunk))
// We reset all validators missed blocks to improve upgrade performance,
// so we expect all chunks to be empty
require.Nil(t, chunk)

// ensure all even blocks are missed
bitIndex := uint(i % v4.MissedBlockBitmapChunkSize)
require.Equal(t, i%2 == 0, bs.Test(bitIndex))
require.Equal(t, i%2 == 1, !bs.Test(bitIndex))
// bs := bitset.New(uint(v4.MissedBlockBitmapChunkSize))
// require.NoError(t, bs.UnmarshalBinary(chunk))

// // ensure all even blocks are missed
// bitIndex := uint(i % v4.MissedBlockBitmapChunkSize)
// require.Equal(t, i%2 == 0, bs.Test(bitIndex))
// require.Equal(t, i%2 == 1, !bs.Test(bitIndex))
}

// ensure there's only one chunk for a window of size 100
Expand Down
12 changes: 8 additions & 4 deletions x/slashing/types/keys.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,14 @@ const (
// - 0x03<accAddrLen (1 Byte)><accAddr_Bytes>: cryptotypes.PubKey

var (
ParamsKey = []byte{0x00} // Prefix for params key
ValidatorSigningInfoKeyPrefix = []byte{0x01} // Prefix for signing info
ValidatorMissedBlockBitmapKeyPrefix = []byte{0x02} // Prefix for missed block bitmap
AddrPubkeyRelationKeyPrefix = []byte{0x03} // Prefix for address-pubkey relation
ParamsKey = []byte{0x00} // Prefix for params key
ValidatorSigningInfoKeyPrefix = []byte{0x01} // Prefix for signing info
AddrPubkeyRelationKeyPrefix = []byte{0x03} // Prefix for address-pubkey relation

ValidatorMissedBlockBitmapKeyPrefix = []byte{0x10} // Prefix for missed block bitmap

IsPruningKey = []byte{0x09}
TrueByteValue = []byte{0x01}
)

// ValidatorSigningInfoKey - stored by *Consensus* address (not operator address)
Expand Down
Loading