From c74e2887b0b73e81d48c2f33e6b1020090089ee0 Mon Sep 17 00:00:00 2001 From: atheeshp <59333759+atheeshp@users.noreply.github.com> Date: Tue, 25 Jul 2023 14:35:26 +0530 Subject: [PATCH] refactor(x/staking): migrate `historicalInfo` to use collections (#17063) --- CHANGELOG.md | 2 + ...rminstic_test.go => deterministic_test.go} | 12 +-- .../staking/keeper/grpc_query_test.go | 4 +- x/staking/keeper/grpc_query.go | 2 +- x/staking/keeper/historical_info.go | 81 ++----------------- x/staking/keeper/historical_info_test.go | 40 +++++---- x/staking/keeper/keeper.go | 2 + x/staking/types/errors.go | 1 - x/staking/types/historical_info.go | 17 ---- x/staking/types/historical_info_test.go | 22 ----- x/staking/types/keys.go | 9 +-- x/staking/types/keys_test.go | 20 ----- 12 files changed, 42 insertions(+), 170 deletions(-) rename tests/integration/staking/keeper/{determinstic_test.go => deterministic_test.go} (99%) diff --git a/CHANGELOG.md b/CHANGELOG.md index 282343f91e..cfba0b2b0f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -49,6 +49,8 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### API Breaking Changes +* (x/slashing) [17063](https://github.com/cosmos/cosmos-sdk/pull/17063) Use collections for `HistoricalInfo`: + * remove `Keeper`: `GetHistoricalInfo`, `SetHistoricalInfo`, * (x/staking) [17062](https://github.com/cosmos/cosmos-sdk/pull/17062) Use collections for `ValidatorUpdates`: * remove `Keeper`: `SetValidatorUpdates`, `GetValidatorUpdates` * (x/slashing) [17023](https://github.com/cosmos/cosmos-sdk/pull/17023) Use collections for `ValidatorSigningInfo`: diff --git a/tests/integration/staking/keeper/determinstic_test.go b/tests/integration/staking/keeper/deterministic_test.go similarity index 99% rename from tests/integration/staking/keeper/determinstic_test.go rename to tests/integration/staking/keeper/deterministic_test.go index 85c6ff94e5..15b607ad95 100644 --- a/tests/integration/staking/keeper/determinstic_test.go +++ b/tests/integration/staking/keeper/deterministic_test.go @@ -669,10 +669,10 @@ func TestGRPCHistoricalInfo(t *testing.T) { height := rapid.Int64Min(0).Draw(rt, "height") - assert.NilError(t, f.stakingKeeper.SetHistoricalInfo( + assert.NilError(t, f.stakingKeeper.HistoricalInfo.Set( f.ctx, - height, - &historicalInfo, + uint64(height), + historicalInfo, )) req := &stakingtypes.QueryHistoricalInfoRequest{ @@ -693,10 +693,10 @@ func TestGRPCHistoricalInfo(t *testing.T) { height := int64(127) - assert.NilError(t, f.stakingKeeper.SetHistoricalInfo( + assert.NilError(t, f.stakingKeeper.HistoricalInfo.Set( f.ctx, - height, - &historicalInfo, + uint64(height), + historicalInfo, )) req := &stakingtypes.QueryHistoricalInfoRequest{ diff --git a/tests/integration/staking/keeper/grpc_query_test.go b/tests/integration/staking/keeper/grpc_query_test.go index a5edf2c04f..4b152d62f1 100644 --- a/tests/integration/staking/keeper/grpc_query_test.go +++ b/tests/integration/staking/keeper/grpc_query_test.go @@ -29,7 +29,7 @@ func createValidatorAccs(t *testing.T, f *fixture) ([]sdk.AccAddress, []types.Va sortedVals := make([]types.Validator, len(validators)) copy(sortedVals, validators) hi := types.NewHistoricalInfo(header, sortedVals, f.stakingKeeper.PowerReduction(f.sdkCtx)) - assert.NilError(t, f.stakingKeeper.SetHistoricalInfo(f.sdkCtx, 5, &hi)) + assert.NilError(t, f.stakingKeeper.HistoricalInfo.Set(f.sdkCtx, uint64(5), hi)) return addrs, validators } @@ -731,7 +731,7 @@ func TestGRPCQueryHistoricalInfo(t *testing.T) { qr := f.app.QueryHelper() queryClient := types.NewQueryClient(qr) - hi, found := f.stakingKeeper.GetHistoricalInfo(ctx, 5) + hi, found := f.stakingKeeper.HistoricalInfo.Get(ctx, uint64(5)) assert.Assert(t, found) var req *types.QueryHistoricalInfoRequest diff --git a/x/staking/keeper/grpc_query.go b/x/staking/keeper/grpc_query.go index 7b72ec54a7..b74c332d0a 100644 --- a/x/staking/keeper/grpc_query.go +++ b/x/staking/keeper/grpc_query.go @@ -390,7 +390,7 @@ func (k Querier) HistoricalInfo(ctx context.Context, req *types.QueryHistoricalI return nil, status.Error(codes.InvalidArgument, "height cannot be negative") } - hi, err := k.GetHistoricalInfo(ctx, req.Height) + hi, err := k.Keeper.HistoricalInfo.Get(ctx, uint64(req.Height)) if err != nil { return nil, status.Errorf(codes.NotFound, "historical info for height %d not found", req.Height) } diff --git a/x/staking/keeper/historical_info.go b/x/staking/keeper/historical_info.go index 4cc9cded97..3f33d13eba 100644 --- a/x/staking/keeper/historical_info.go +++ b/x/staking/keeper/historical_info.go @@ -4,83 +4,12 @@ import ( "context" "errors" - storetypes "cosmossdk.io/store/types" + "cosmossdk.io/collections" sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/x/staking/types" ) -// GetHistoricalInfo gets the historical info at a given height -func (k Keeper) GetHistoricalInfo(ctx context.Context, height int64) (types.HistoricalInfo, error) { - store := k.storeService.OpenKVStore(ctx) - key := types.GetHistoricalInfoKey(height) - - value, err := store.Get(key) - if err != nil { - return types.HistoricalInfo{}, err - } - - if value == nil { - return types.HistoricalInfo{}, types.ErrNoHistoricalInfo - } - - return types.UnmarshalHistoricalInfo(k.cdc, value) -} - -// SetHistoricalInfo sets the historical info at a given height -func (k Keeper) SetHistoricalInfo(ctx context.Context, height int64, hi *types.HistoricalInfo) error { - store := k.storeService.OpenKVStore(ctx) - key := types.GetHistoricalInfoKey(height) - value, err := k.cdc.Marshal(hi) - if err != nil { - return err - } - return store.Set(key, value) -} - -// DeleteHistoricalInfo deletes the historical info at a given height -func (k Keeper) DeleteHistoricalInfo(ctx context.Context, height int64) error { - store := k.storeService.OpenKVStore(ctx) - key := types.GetHistoricalInfoKey(height) - - return store.Delete(key) -} - -// IterateHistoricalInfo provides an iterator over all stored HistoricalInfo -// objects. For each HistoricalInfo object, cb will be called. If the cb returns -// true, the iterator will break and close. -func (k Keeper) IterateHistoricalInfo(ctx context.Context, cb func(types.HistoricalInfo) bool) error { - store := k.storeService.OpenKVStore(ctx) - iterator, err := store.Iterator(types.HistoricalInfoKey, storetypes.PrefixEndBytes(types.HistoricalInfoKey)) - if err != nil { - return err - } - defer iterator.Close() - - for ; iterator.Valid(); iterator.Next() { - histInfo, err := types.UnmarshalHistoricalInfo(k.cdc, iterator.Value()) - if err != nil { - return err - } - if cb(histInfo) { - break - } - } - - return nil -} - -// GetAllHistoricalInfo returns all stored HistoricalInfo objects. -func (k Keeper) GetAllHistoricalInfo(ctx context.Context) ([]types.HistoricalInfo, error) { - var infos []types.HistoricalInfo - err := k.IterateHistoricalInfo(ctx, func(histInfo types.HistoricalInfo) bool { - infos = append(infos, histInfo) - return false - }) - - return infos, err -} - // TrackHistoricalInfo saves the latest historical-info and deletes the oldest // heights that are below pruning height func (k Keeper) TrackHistoricalInfo(ctx context.Context) error { @@ -99,14 +28,14 @@ func (k Keeper) TrackHistoricalInfo(ctx context.Context) error { // over the historical entries starting from the most recent version to be pruned // and then return at the first empty entry. for i := sdkCtx.BlockHeight() - int64(entryNum); i >= 0; i-- { - _, err := k.GetHistoricalInfo(ctx, i) + _, err := k.HistoricalInfo.Get(ctx, uint64(i)) if err != nil { - if errors.Is(err, types.ErrNoHistoricalInfo) { + if errors.Is(err, collections.ErrNotFound) { break } return err } - if err = k.DeleteHistoricalInfo(ctx, i); err != nil { + if err = k.HistoricalInfo.Remove(ctx, uint64(i)); err != nil { return err } } @@ -125,5 +54,5 @@ func (k Keeper) TrackHistoricalInfo(ctx context.Context) error { historicalEntry := types.NewHistoricalInfo(sdkCtx.BlockHeader(), lastVals, k.PowerReduction(ctx)) // Set latest HistoricalInfo at current height - return k.SetHistoricalInfo(ctx, sdkCtx.BlockHeight(), &historicalEntry) + return k.HistoricalInfo.Set(ctx, uint64(sdkCtx.BlockHeight()), historicalEntry) } diff --git a/x/staking/keeper/historical_info_test.go b/x/staking/keeper/historical_info_test.go index d057009366..4f7d92285c 100644 --- a/x/staking/keeper/historical_info_test.go +++ b/x/staking/keeper/historical_info_test.go @@ -3,6 +3,7 @@ package keeper_test import ( cmtproto "github.com/cometbft/cometbft/proto/tendermint/types" + "cosmossdk.io/collections" "cosmossdk.io/math" "github.com/cosmos/cosmos-sdk/x/staking/testutil" @@ -33,17 +34,17 @@ func (s *KeeperTestSuite) TestHistoricalInfo() { } hi := stakingtypes.NewHistoricalInfo(ctx.BlockHeader(), validators, keeper.PowerReduction(ctx)) - require.NoError(keeper.SetHistoricalInfo(ctx, 2, &hi)) + require.NoError(keeper.HistoricalInfo.Set(ctx, uint64(2), hi)) - recv, err := keeper.GetHistoricalInfo(ctx, 2) - require.NoError(err, "HistoricalInfo not found after set") + recv, err := keeper.HistoricalInfo.Get(ctx, uint64(2)) + require.NoError(err, "HistoricalInfo found after set") require.Equal(hi, recv, "HistoricalInfo not equal") require.True(IsValSetSorted(recv.Valset, keeper.PowerReduction(ctx)), "HistoricalInfo validators is not sorted") - require.NoError(keeper.DeleteHistoricalInfo(ctx, 2)) + require.NoError(keeper.HistoricalInfo.Remove(ctx, uint64(2))) - recv, err = keeper.GetHistoricalInfo(ctx, 2) - require.ErrorIs(err, stakingtypes.ErrNoHistoricalInfo, "HistoricalInfo found after delete") + recv, err = keeper.HistoricalInfo.Get(ctx, uint64(2)) + require.ErrorIs(err, collections.ErrNotFound, "HistoricalInfo not found after delete") require.Equal(stakingtypes.HistoricalInfo{}, recv, "HistoricalInfo is not empty") } @@ -74,12 +75,12 @@ func (s *KeeperTestSuite) TestTrackHistoricalInfo() { } hi4 := stakingtypes.NewHistoricalInfo(h4, valSet, keeper.PowerReduction(ctx)) hi5 := stakingtypes.NewHistoricalInfo(h5, valSet, keeper.PowerReduction(ctx)) - require.NoError(keeper.SetHistoricalInfo(ctx, 4, &hi4)) - require.NoError(keeper.SetHistoricalInfo(ctx, 5, &hi5)) - recv, err := keeper.GetHistoricalInfo(ctx, 4) + require.NoError(keeper.HistoricalInfo.Set(ctx, uint64(4), hi4)) + require.NoError(keeper.HistoricalInfo.Set(ctx, uint64(5), hi5)) + recv, err := keeper.HistoricalInfo.Get(ctx, uint64(4)) require.NoError(err) require.Equal(hi4, recv) - recv, err = keeper.GetHistoricalInfo(ctx, 5) + recv, err = keeper.HistoricalInfo.Get(ctx, uint64(5)) require.NoError(err) require.Equal(hi5, recv) @@ -112,16 +113,16 @@ func (s *KeeperTestSuite) TestTrackHistoricalInfo() { Header: header, Valset: vals, } - recv, err = keeper.GetHistoricalInfo(ctx, 10) + recv, err = keeper.HistoricalInfo.Get(ctx, uint64(10)) require.NoError(err, "GetHistoricalInfo failed after BeginBlock") require.Equal(expected, recv, "GetHistoricalInfo returned unexpected result") // Check HistoricalInfo at height 5, 4 is pruned - recv, err = keeper.GetHistoricalInfo(ctx, 4) - require.ErrorIs(err, stakingtypes.ErrNoHistoricalInfo, "GetHistoricalInfo did not prune earlier height") + recv, err = keeper.HistoricalInfo.Get(ctx, uint64(4)) + require.ErrorIs(err, collections.ErrNotFound, "GetHistoricalInfo did not prune earlier height") require.Equal(stakingtypes.HistoricalInfo{}, recv, "GetHistoricalInfo at height 4 is not empty after prune") - recv, err = keeper.GetHistoricalInfo(ctx, 5) - require.ErrorIs(err, stakingtypes.ErrNoHistoricalInfo, "GetHistoricalInfo did not prune first prune height") + recv, err = keeper.HistoricalInfo.Get(ctx, uint64(5)) + require.ErrorIs(err, collections.ErrNotFound, "GetHistoricalInfo did not prune first prune height") require.Equal(stakingtypes.HistoricalInfo{}, recv, "GetHistoricalInfo at height 5 is not empty after prune") } @@ -147,10 +148,15 @@ func (s *KeeperTestSuite) TestGetAllHistoricalInfo() { expHistInfos := []stakingtypes.HistoricalInfo{hist1, hist2, hist3} for i, hi := range expHistInfos { - require.NoError(keeper.SetHistoricalInfo(ctx, int64(9+i), &hi)) //nolint:gosec // G601: Implicit memory aliasing in for loop. + require.NoError(keeper.HistoricalInfo.Set(ctx, uint64(int64(9+i)), hi)) } - infos, err := keeper.GetAllHistoricalInfo(ctx) + var infos []stakingtypes.HistoricalInfo + err := keeper.HistoricalInfo.Walk(ctx, nil, func(key uint64, info stakingtypes.HistoricalInfo) (stop bool, err error) { + infos = append(infos, info) + return false, nil + }) + require.NoError(err) require.Equal(expHistInfos, infos) } diff --git a/x/staking/keeper/keeper.go b/x/staking/keeper/keeper.go index 35b04a8deb..8154b1b532 100644 --- a/x/staking/keeper/keeper.go +++ b/x/staking/keeper/keeper.go @@ -33,6 +33,7 @@ type Keeper struct { consensusAddressCodec addresscodec.Codec Schema collections.Schema + HistoricalInfo collections.Map[uint64, types.HistoricalInfo] LastTotalPower collections.Item[math.Int] ValidatorUpdates collections.Item[types.ValidatorUpdates] } @@ -76,6 +77,7 @@ func NewKeeper( validatorAddressCodec: validatorAddressCodec, consensusAddressCodec: consensusAddressCodec, LastTotalPower: collections.NewItem(sb, types.LastTotalPowerKey, "last_total_power", sdk.IntValue), + HistoricalInfo: collections.NewMap(sb, types.HistoricalInfoKey, "historical_info", collections.Uint64Key, codec.CollValue[types.HistoricalInfo](cdc)), ValidatorUpdates: collections.NewItem(sb, types.ValidatorUpdatesKey, "validator_updates", codec.CollValue[types.ValidatorUpdates](cdc)), } diff --git a/x/staking/types/errors.go b/x/staking/types/errors.go index 0044156435..403d779c09 100644 --- a/x/staking/types/errors.go +++ b/x/staking/types/errors.go @@ -40,7 +40,6 @@ var ( ErrBothShareMsgsGiven = errors.Register(ModuleName, 35, "both shares amount and shares percent provided") ErrNeitherShareMsgsGiven = errors.Register(ModuleName, 36, "neither shares amount nor shares percent provided") ErrInvalidHistoricalInfo = errors.Register(ModuleName, 37, "invalid historical info") - ErrNoHistoricalInfo = errors.Register(ModuleName, 38, "no historical info found") ErrEmptyValidatorPubKey = errors.Register(ModuleName, 39, "empty validator public key") ErrCommissionLTMinRate = errors.Register(ModuleName, 40, "commission cannot be less than min rate") ErrUnbondingNotFound = errors.Register(ModuleName, 41, "unbonding operation not found") diff --git a/x/staking/types/historical_info.go b/x/staking/types/historical_info.go index 0c9b6a14c1..62eafddbcd 100644 --- a/x/staking/types/historical_info.go +++ b/x/staking/types/historical_info.go @@ -9,7 +9,6 @@ import ( "cosmossdk.io/errors" "cosmossdk.io/math" - "github.com/cosmos/cosmos-sdk/codec" codectypes "github.com/cosmos/cosmos-sdk/codec/types" ) @@ -27,22 +26,6 @@ func NewHistoricalInfo(header cmtproto.Header, valSet Validators, powerReduction } } -// MustUnmarshalHistoricalInfo wll unmarshal historical info and panic on error -func MustUnmarshalHistoricalInfo(cdc codec.BinaryCodec, value []byte) HistoricalInfo { - hi, err := UnmarshalHistoricalInfo(cdc, value) - if err != nil { - panic(err) - } - - return hi -} - -// UnmarshalHistoricalInfo will unmarshal historical info and return any error -func UnmarshalHistoricalInfo(cdc codec.BinaryCodec, value []byte) (hi HistoricalInfo, err error) { - err = cdc.Unmarshal(value, &hi) - return hi, err -} - // ValidateBasic will ensure HistoricalInfo is not nil and sorted func ValidateBasic(hi HistoricalInfo) error { if len(hi.Valset) == 0 { diff --git a/x/staking/types/historical_info_test.go b/x/staking/types/historical_info_test.go index 142780146f..46a9468bc4 100644 --- a/x/staking/types/historical_info_test.go +++ b/x/staking/types/historical_info_test.go @@ -8,8 +8,6 @@ import ( cmtproto "github.com/cometbft/cometbft/proto/tendermint/types" "github.com/stretchr/testify/require" - "github.com/cosmos/cosmos-sdk/codec" - "github.com/cosmos/cosmos-sdk/codec/legacy" sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/x/staking/types" ) @@ -28,26 +26,6 @@ func createValidators(t *testing.T) []types.Validator { } } -func TestHistoricalInfo(t *testing.T) { - validators := createValidators(t) - hi := types.NewHistoricalInfo(header, validators, sdk.DefaultPowerReduction) - require.True(t, sort.IsSorted(types.Validators(hi.Valset)), "Validators are not sorted") - - var value []byte - require.NotPanics(t, func() { - value = legacy.Cdc.MustMarshal(&hi) - }) - require.NotNil(t, value, "Marshaled HistoricalInfo is nil") - - recv, err := types.UnmarshalHistoricalInfo(codec.NewAminoCodec(legacy.Cdc), value) - require.Nil(t, err, "Unmarshalling HistoricalInfo failed") - require.Equal(t, hi.Header, recv.Header) - for i := range hi.Valset { - require.True(t, hi.Valset[i].Equal(&recv.Valset[i])) - } - require.True(t, sort.IsSorted(types.Validators(hi.Valset)), "Validators are not sorted") -} - func TestValidateBasic(t *testing.T) { validators := createValidators(t) hi := types.HistoricalInfo{ diff --git a/x/staking/types/keys.go b/x/staking/types/keys.go index 57689058c9..8de7195ee6 100644 --- a/x/staking/types/keys.go +++ b/x/staking/types/keys.go @@ -53,7 +53,7 @@ var ( RedelegationQueueKey = []byte{0x42} // prefix for the timestamps in redelegations queue ValidatorQueueKey = []byte{0x43} // prefix for the timestamps in validator queue - HistoricalInfoKey = []byte{0x50} // prefix for the historical info + HistoricalInfoKey = collections.NewPrefix(80) // prefix for the historical info ValidatorUpdatesKey = collections.NewPrefix(97) // prefix for the end block validator updates key ParamsKey = []byte{0x51} // prefix for parameters for module x/staking @@ -418,10 +418,3 @@ func GetREDsToValDstIndexKey(valDstAddr sdk.ValAddress) []byte { func GetREDsByDelToValDstIndexKey(delAddr sdk.AccAddress, valDstAddr sdk.ValAddress) []byte { return append(GetREDsToValDstIndexKey(valDstAddr), address.MustLengthPrefix(delAddr)...) } - -// GetHistoricalInfoKey returns a key prefix for indexing HistoricalInfo objects. -func GetHistoricalInfoKey(height int64) []byte { - heightBytes := make([]byte, 8) - binary.BigEndian.PutUint64(heightBytes, uint64(height)) - return append(HistoricalInfoKey, heightBytes...) -} diff --git a/x/staking/types/keys_test.go b/x/staking/types/keys_test.go index 60d248da5e..c4bd5400ea 100644 --- a/x/staking/types/keys_test.go +++ b/x/staking/types/keys_test.go @@ -3,9 +3,7 @@ package types_test import ( "bytes" "encoding/hex" - math2 "math" "math/big" - "strconv" "testing" "time" @@ -132,21 +130,3 @@ func TestTestGetValidatorQueueKeyOrder(t *testing.T) { require.Equal(t, -1, bytes.Compare(keyB, endKey)) // keyB <= endKey require.Equal(t, 1, bytes.Compare(keyC, endKey)) // keyB >= endKey } - -func TestGetHistoricalInfoKey(t *testing.T) { - tests := []struct { - height int64 - want []byte - }{ - {0, append(types.HistoricalInfoKey, []byte{0, 0, 0, 0, 0, 0, 0, 0}...)}, - {1, append(types.HistoricalInfoKey, []byte{0, 0, 0, 0, 0, 0, 0, 1}...)}, - {2, append(types.HistoricalInfoKey, []byte{0, 0, 0, 0, 0, 0, 0, 2}...)}, - {514, append(types.HistoricalInfoKey, []byte{0, 0, 0, 0, 0, 0, 2, 2}...)}, - {math2.MaxInt64, append(types.HistoricalInfoKey, []byte{127, 255, 255, 255, 255, 255, 255, 255}...)}, - } - for _, tt := range tests { - t.Run(strconv.FormatInt(tt.height, 10), func(t *testing.T) { - require.Equal(t, tt.want, types.GetHistoricalInfoKey(tt.height)) - }) - } -}