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

feat: include rotate keys logic in abci #18236

Merged
merged 115 commits into from
Dec 22, 2023
Merged
Show file tree
Hide file tree
Changes from 82 commits
Commits
Show all changes
115 commits
Select commit Hold shift + click to select a range
b6b512a
feat: implement rotate cons key method in msg server
atheeshp Oct 20, 2023
7c7322a
fix tests
atheeshp Oct 24, 2023
9aef4f4
Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/add-msg-s…
atheeshp Oct 24, 2023
37675e6
fix tests
atheeshp Oct 24, 2023
a9be784
fix tests
atheeshp Oct 24, 2023
2a713b5
Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/add-msg-s…
atheeshp Oct 24, 2023
43923ce
go mod tidy
atheeshp Oct 24, 2023
d674890
Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/add-msg-s…
atheeshp Oct 24, 2023
c9f32a7
feat: include rotate keys logic in abci
atheeshp Oct 24, 2023
20f27b7
Merge branch 'main' into ap/add-msg-srvr-code
atheeshp Oct 25, 2023
a51cca3
Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/add-msg-s…
atheeshp Oct 25, 2023
cacc0d8
go mod
atheeshp Oct 25, 2023
e9231e4
Merge branch 'ap/add-msg-srvr-code' of github.com:cosmos/cosmos-sdk i…
atheeshp Oct 25, 2023
5086c5f
fix lint
atheeshp Oct 26, 2023
202e2ef
Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/add-msg-s…
atheeshp Oct 26, 2023
3b52332
review changes
atheeshp Oct 26, 2023
4843ae1
fix tests
atheeshp Oct 26, 2023
67e15e0
Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/add-msg-s…
atheeshp Oct 27, 2023
8ffaa71
code rabbit review changes
atheeshp Oct 27, 2023
c093905
Merge branch 'ap/add-msg-srvr-code' of github.com:cosmos/cosmos-sdk i…
atheeshp Oct 27, 2023
cce3c19
Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/add-msg-s…
atheeshp Oct 30, 2023
4a63bde
conflicts
atheeshp Oct 30, 2023
4350e5b
review changes
atheeshp Oct 30, 2023
bbcb932
Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/add-msg-s…
atheeshp Oct 30, 2023
7b2b9bd
fix tests
atheeshp Oct 31, 2023
6e8eb7f
review changes
atheeshp Oct 31, 2023
04f1cbe
Merge branch 'main' into ap/add-msg-srvr-code
atheeshp Oct 31, 2023
55496f9
Merge branch 'ap/add-msg-srvr-code' of github.com:cosmos/cosmos-sdk i…
atheeshp Oct 31, 2023
535d00f
review changes
atheeshp Oct 31, 2023
46ae281
review changes
atheeshp Oct 31, 2023
12274e9
Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/add-msg-s…
atheeshp Oct 31, 2023
b19d163
Merge branch 'ap/add-msg-srvr-code' of github.com:cosmos/cosmos-sdk i…
atheeshp Oct 31, 2023
e463855
go mod
atheeshp Oct 31, 2023
552e683
go mod
atheeshp Oct 31, 2023
2ea32bc
fix go mod
atheeshp Oct 31, 2023
fb3363f
Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/add-msg-s…
atheeshp Oct 31, 2023
9976530
go mod
atheeshp Oct 31, 2023
f0113eb
Merge branch 'ap/add-msg-srvr-code' of github.com:cosmos/cosmos-sdk i…
atheeshp Nov 1, 2023
d4439ec
Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/add-msg-s…
atheeshp Nov 2, 2023
87aebe8
Merge branch 'ap/add-msg-srvr-code' of github.com:cosmos/cosmos-sdk i…
atheeshp Nov 2, 2023
3a0f4da
review changes
atheeshp Nov 2, 2023
89ca04f
proto
atheeshp Nov 2, 2023
6d138b8
proto-gen
atheeshp Nov 2, 2023
49f1745
review changes
atheeshp Nov 2, 2023
4183c78
review changes
atheeshp Nov 2, 2023
327929f
proto-gen
atheeshp Nov 2, 2023
efb8022
review changes
atheeshp Nov 2, 2023
dfbb63a
Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/add-msg-s…
atheeshp Nov 2, 2023
dd8c72c
review changes
atheeshp Nov 2, 2023
0359d96
review changes
atheeshp Nov 2, 2023
441f6ee
Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/add-msg-s…
atheeshp Nov 3, 2023
f41fdf0
go mod tidy
atheeshp Nov 3, 2023
c7563f5
Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/add-msg-s…
atheeshp Nov 6, 2023
44731ce
try fixing go mod
atheeshp Nov 6, 2023
8a00a07
fix go mod changes
atheeshp Nov 6, 2023
7ed213f
review change
atheeshp Nov 6, 2023
78c4eda
review changes
atheeshp Nov 6, 2023
b236334
review changes
atheeshp Nov 7, 2023
a4b06da
Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/add-msg-s…
atheeshp Nov 7, 2023
cae4de4
clean comments
atheeshp Nov 7, 2023
1539aaa
review changes
atheeshp Nov 7, 2023
e30bece
Merge branch 'main' into ap/add-msg-srvr-code
atheeshp Nov 7, 2023
d4bf237
Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/add-msg-s…
atheeshp Nov 7, 2023
20bf052
conflicts
atheeshp Nov 7, 2023
10f6e38
Merge branch 'ap/add-msg-srvr-code' of github.com:cosmos/cosmos-sdk i…
atheeshp Nov 7, 2023
d32c833
go mod tidy
atheeshp Nov 7, 2023
05e560a
review changes
atheeshp Nov 8, 2023
e27c72a
Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/add-msg-s…
atheeshp Nov 8, 2023
aee5587
Merge branch 'ap/add-msg-srvr-code' of github.com:cosmos/cosmos-sdk i…
atheeshp Nov 8, 2023
06d7170
Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/abci-and-…
atheeshp Nov 15, 2023
d560884
conflicts
atheeshp Nov 15, 2023
17852c2
go mod
atheeshp Nov 15, 2023
9a7a82b
go mod
atheeshp Nov 15, 2023
f98c94c
Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/abci-and-…
atheeshp Nov 16, 2023
1a2a897
Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/abci-and-…
atheeshp Nov 17, 2023
1f04845
Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/abci-and-…
atheeshp Nov 20, 2023
82bc8f5
review changes
atheeshp Nov 21, 2023
03469a5
review changes
atheeshp Nov 21, 2023
a8544f0
nit
atheeshp Nov 21, 2023
ea22a3d
fix tests
atheeshp Nov 22, 2023
3756d84
fix tests
atheeshp Nov 22, 2023
91fe4f1
review changes
atheeshp Nov 22, 2023
5ed6003
add tests for rotation
atheeshp Nov 29, 2023
1191858
Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/abci-and-…
atheeshp Nov 29, 2023
75eb409
add extra test
atheeshp Nov 29, 2023
088255b
review changes
atheeshp Dec 1, 2023
c221c55
Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/abci-and-…
atheeshp Dec 1, 2023
6b55e4b
review changes
atheeshp Dec 1, 2023
0f1011b
review changes
atheeshp Dec 1, 2023
cc2c0a1
review changes
atheeshp Dec 12, 2023
3d5c8b4
Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/abci-and-…
atheeshp Dec 12, 2023
1ca3562
Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/abci-and-…
atheeshp Dec 13, 2023
cddbc58
Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/abci-and-…
atheeshp Dec 14, 2023
f362a93
review changes
atheeshp Dec 17, 2023
42979cb
Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/abci-and-…
atheeshp Dec 17, 2023
fbd4e41
review changes
atheeshp Dec 17, 2023
9961cc5
fix test
atheeshp Dec 18, 2023
a8bab2f
lint
atheeshp Dec 19, 2023
159375e
test
atheeshp Dec 19, 2023
663c8ef
lint
atheeshp Dec 19, 2023
19bf12b
Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/abci-and-…
atheeshp Dec 19, 2023
320af37
review changes
atheeshp Dec 20, 2023
fcabe10
lint
atheeshp Dec 20, 2023
5e4736a
review changes
atheeshp Dec 20, 2023
2eedad0
Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/abci-and-…
atheeshp Dec 20, 2023
97c871d
remove unnecessary code
atheeshp Dec 20, 2023
8f219bd
add test to check validator identifier
atheeshp Dec 21, 2023
1b1f4b7
Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/abci-and-…
atheeshp Dec 21, 2023
c742783
fix lint
atheeshp Dec 21, 2023
7a4419a
review changes
atheeshp Dec 21, 2023
075865b
Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/abci-and-…
atheeshp Dec 21, 2023
98862ef
fix `GetValidatorByConsAddr`
atheeshp Dec 21, 2023
bc53191
Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/abci-and-…
atheeshp Dec 21, 2023
693a7eb
review changes
atheeshp Dec 21, 2023
bd92cec
Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/abci-and-…
atheeshp Dec 21, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions tests/integration/staking/keeper/unbonding_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ func SetupUnbondingTests(t *testing.T, f *fixture, hookCalled *bool, ubdeID *uin
mockStackingHooks.EXPECT().BeforeDelegationSharesModified(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
mockStackingHooks.EXPECT().BeforeValidatorModified(gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
mockStackingHooks.EXPECT().BeforeValidatorSlashed(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
mockStackingHooks.EXPECT().AfterConsensusPubKeyUpdate(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
f.stakingKeeper.SetHooks(types.NewMultiStakingHooks(mockStackingHooks))

addrDels = simtestutil.AddTestAddrsIncremental(f.bankKeeper, f.stakingKeeper, f.sdkCtx, 2, math.NewInt(10000))
Expand Down
5 changes: 5 additions & 0 deletions x/distribution/keeper/hooks.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"cosmossdk.io/x/distribution/types"
stakingtypes "cosmossdk.io/x/staking/types"

cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
sdk "github.com/cosmos/cosmos-sdk/types"
)

Expand Down Expand Up @@ -187,3 +188,7 @@ func (h Hooks) BeforeDelegationRemoved(_ context.Context, _ sdk.AccAddress, _ sd
func (h Hooks) AfterUnbondingInitiated(_ context.Context, _ uint64) error {
return nil
}

func (h Hooks) AfterConsensusPubKeyUpdate(_ context.Context, _, _ cryptotypes.PubKey, _ sdk.Coin) error {
return nil
}
7 changes: 7 additions & 0 deletions x/evidence/keeper/infraction.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,13 @@ func (k Keeper) handleEquivocationEvidence(ctx context.Context, evidence *types.
}

if len(validator.GetOperator()) != 0 {
// get the consAddr again, this is because validator might've rotated it's key.
valConsAddr, err := validator.GetConsAddr()
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain this a little bit more? Getting the consAddr this way doesn't get it from the store again if that's what you meant

Copy link
Member

Choose a reason for hiding this comment

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

@atheeshp ping on this one, I think you added more comments but I'm still confused 😅

Copy link
Contributor Author

@atheeshp atheeshp Dec 21, 2023

Choose a reason for hiding this comment

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

an evidence for a validator's behaviour can be submitted till the unbonding period window.
When a validator rotates it's key that will update the validator.ConsAddr to the new address in the validator details immediately. But if the evidence submitted (within the unbonding period of the rotation) with old cons address we cannot find the required details of the validator in the slashing state because of the key rotation. To find them we need to get the validator's rotated consAddr which will be present at validator.consAddr

if err != nil {
return err
}
consAddr = valConsAddr

if _, err := k.slashingKeeper.GetPubkey(ctx, consAddr.Bytes()); err != nil {
// Ignore evidence that cannot be handled.
//
Copy link
Contributor

Choose a reason for hiding this comment

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

The code correctly checks if the validator has an operator and attempts to get the updated consensus address. However, there is a potential issue with the error handling in line 50. If GetPubkey returns an error other than a not found error, it should not be ignored because it might indicate a more serious problem with the state or the database. The current implementation assumes any error from GetPubkey means the public key is not found, which might not always be the case. It would be safer to check the specific error type before deciding to ignore it.

if _, err := k.slashingKeeper.GetPubkey(ctx, consAddr.Bytes()); err != nil {
    if errors.Is(err, someErrorTypeIndicatingNotFound) {
        // Handle not found error specifically
        logger.Error(fmt.Sprintf("ignore evidence; expected public key for validator %s not found", consAddr))
        return nil
    }
    // Handle other potential errors
    return err
}

Expand Down
13 changes: 13 additions & 0 deletions x/slashing/keeper/hooks.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
sdkmath "cosmossdk.io/math"
"cosmossdk.io/x/slashing/types"

cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
sdk "github.com/cosmos/cosmos-sdk/types"
)

Expand Down Expand Up @@ -99,3 +100,15 @@ func (h Hooks) BeforeValidatorSlashed(_ context.Context, _ sdk.ValAddress, _ sdk
func (h Hooks) AfterUnbondingInitiated(_ context.Context, _ uint64) error {
return nil
}

func (h Hooks) AfterConsensusPubKeyUpdate(ctx context.Context, oldPubKey, newPubKey cryptotypes.PubKey, _ sdk.Coin) error {
atheeshp marked this conversation as resolved.
Show resolved Hide resolved
if err := h.k.PerformConsensusPubKeyUpdate(ctx, oldPubKey, newPubKey); err != nil {
return err
}

if err := h.k.AddrPubkeyRelation.Remove(ctx, oldPubKey.Address()); err != nil {
return err
}

return nil
}
42 changes: 42 additions & 0 deletions x/slashing/keeper/signing_info.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
errorsmod "cosmossdk.io/errors"
"cosmossdk.io/x/slashing/types"

cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
sdk "github.com/cosmos/cosmos-sdk/types"
)

Expand Down Expand Up @@ -209,3 +210,44 @@ func (k Keeper) GetValidatorMissedBlocks(ctx context.Context, addr sdk.ConsAddre

return missedBlocks, err
}

atheeshp marked this conversation as resolved.
Show resolved Hide resolved
// PerformConsensusPubKeyUpdate updates cons address to its pub key relation
// Updates signing info, missed blocks (removes old one, and sets new one)
func (k Keeper) PerformConsensusPubKeyUpdate(ctx context.Context, oldPubKey, newPubKey cryptotypes.PubKey) error {
Copy link
Member

Choose a reason for hiding this comment

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

private?

Copy link
Member

Choose a reason for hiding this comment

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

this would get removed if we went with the map look up right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are using map look-up only for missed blocks, we need to keep this method for migrating the signing-info (which has one entry for a validator).

// Connect new consensus address with PubKey
if err := k.AddrPubkeyRelation.Set(ctx, newPubKey.Address(), newPubKey); err != nil {
return err
}

// Migrate ValidatorSigningInfo from oldPubKey to newPubKey
signingInfo, err := k.ValidatorSigningInfo.Get(ctx, sdk.ConsAddress(oldPubKey.Address()))
if err != nil {
return types.ErrInvalidConsPubKey.Wrap("failed to get signing info for old public key")
}

if err := k.ValidatorSigningInfo.Set(ctx, sdk.ConsAddress(newPubKey.Address()), signingInfo); err != nil {
return err
}

if err := k.ValidatorSigningInfo.Remove(ctx, sdk.ConsAddress(oldPubKey.Address())); err != nil {
atheeshp marked this conversation as resolved.
Show resolved Hide resolved
return err
}

// Migrate ValidatorMissedBlockBitArray from oldPubKey to newPubKey
missedBlocks, err := k.GetValidatorMissedBlocks(ctx, sdk.ConsAddress(oldPubKey.Address()))
if err != nil {
return err
}

if err := k.DeleteMissedBlockBitmap(ctx, sdk.ConsAddress(oldPubKey.Address())); err != nil {
return err
}

for _, missed := range missedBlocks {
if err := k.SetMissedBlockBitmapValue(ctx, sdk.ConsAddress(newPubKey.Address()), missed.Index, missed.Missed); err != nil {
return err
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if there's a way we can just update the existing record address? Or is it indexed by address?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or is it indexed by address?

Yes, in slashing module most of the places consAddress is being used in state for keys.

Copy link
Member

Choose a reason for hiding this comment

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

this seems expensive, is there anyway to make it lazy? im worried for chains like neutron a dos vector could be get in the validator set, miss 70k+ blocks migrate and watch the chain halt for the migration.

Maybe we dont need to migrate this at all? Staking forever keeps a map of old key to new key for slashing?

atheeshp marked this conversation as resolved.
Show resolved Hide resolved

return nil
}
atheeshp marked this conversation as resolved.
Show resolved Hide resolved
1 change: 1 addition & 0 deletions x/slashing/types/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,5 @@ var (
ErrNoSigningInfoFound = errors.Register(ModuleName, 8, "no validator signing info found")
ErrValidatorTombstoned = errors.Register(ModuleName, 9, "validator already tombstoned")
ErrInvalidSigner = errors.Register(ModuleName, 10, "expected authority account as only signer for proposal message")
ErrInvalidConsPubKey = errors.Register(ModuleName, 11, "invalid consensus pubkey")
)
110 changes: 110 additions & 0 deletions x/staking/keeper/cons_pubkey.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,14 @@ import (
"time"

"cosmossdk.io/collections"
"cosmossdk.io/collections/indexes"
errorsmod "cosmossdk.io/errors"
"cosmossdk.io/x/staking/types"

codectypes "github.com/cosmos/cosmos-sdk/codec/types"
cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
)
alexanderbez marked this conversation as resolved.
Show resolved Hide resolved

// maxRotations is the value of max rotations can be made in unbonding period for a validator.
atheeshp marked this conversation as resolved.
Show resolved Hide resolved
Expand Down Expand Up @@ -47,6 +51,54 @@ func (k Keeper) setConsPubKeyRotationHistory(
return k.setConsKeyQueue(ctx, queueTime, valAddr)
}

// This method gets called from the `ApplyAndReturnValidatorSetUpdates`(from endblocker) method.
atheeshp marked this conversation as resolved.
Show resolved Hide resolved
//
// This method makes the relative state changes to update the keys,
// also maintains a map with old to new conskey rotation which is needed to retrieve the old conskey.
// And also triggers the hook to make changes required in slashing and distribution modules.
func (k Keeper) updateToNewPubkey(ctx context.Context, val types.Validator, oldPubKey, newPubKey *codectypes.Any, fee sdk.Coin) error {
consAddr, err := val.GetConsAddr()
if err != nil {
return err
}

if err := k.ValidatorByConsensusAddress.Remove(ctx, consAddr); err != nil {
return err
}

if err := k.DeleteValidatorByPowerIndex(ctx, val); err != nil {
return err
}

val.ConsensusPubkey = newPubKey
if err := k.SetValidator(ctx, val); err != nil {
return err
}
if err := k.SetValidatorByConsAddr(ctx, val); err != nil {
return err
}
if err := k.SetValidatorByPowerIndex(ctx, val); err != nil {
return err
}

oldPk, ok := oldPubKey.GetCachedValue().(cryptotypes.PubKey)
if !ok {
return errorsmod.Wrapf(sdkerrors.ErrInvalidType, "Expecting cryptotypes.PubKey, got %T", oldPk)
}

newPk, ok := newPubKey.GetCachedValue().(cryptotypes.PubKey)
if !ok {
return errorsmod.Wrapf(sdkerrors.ErrInvalidType, "Expecting cryptotypes.PubKey, got %T", newPk)
atheeshp marked this conversation as resolved.
Show resolved Hide resolved
}

// Sets a map to newly rotated consensus key with old consensus key
if err := k.RotatedConsKeyMapIndex.Set(ctx, oldPk.Address(), newPk.Address()); err != nil {
return err
}

return k.Hooks().AfterConsensusPubKeyUpdate(ctx, oldPk, newPk, fee)
}
atheeshp marked this conversation as resolved.
Show resolved Hide resolved

// exceedsMaxRotations returns true if the key rotations exceed the limit, currently we are limiting one rotation for unbonding period.
func (k Keeper) exceedsMaxRotations(ctx context.Context, valAddr sdk.ValAddress) error {
count := 0
Expand Down Expand Up @@ -90,3 +142,61 @@ func bytesSliceExists(sliceList [][]byte, targetBytes []byte) bool {
}
return false
}

// UpdateAllMaturedConsKeyRotatedKeys udpates all the matured key rotations.
atheeshp marked this conversation as resolved.
Show resolved Hide resolved
func (k Keeper) UpdateAllMaturedConsKeyRotatedKeys(ctx sdk.Context, maturedTime time.Time) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like this should be PurgeAllMaturedConsKeyRotatedKeys

Copy link
Member

Choose a reason for hiding this comment

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

can we make this private as well. Want to avoid another module accidentally calling this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are using this method in the tests

maturedRotatedValAddrs, err := k.GetAllMaturedRotatedKeys(ctx, maturedTime)
if err != nil {
return err
}

for _, valAddr := range maturedRotatedValAddrs {
err := k.deleteConsKeyIndexKey(ctx, valAddr, maturedTime)
if err != nil {
return err
}
}

return nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The UpdateAllMaturedConsKeyRotatedKeys function is correctly iterating over matured validator addresses and deleting the consensus key index key. However, the function name has a typo; it should be UpdateAllMaturedConsKeyRotatedKeys instead of udpates.


// deleteConsKeyIndexKey deletes the key which is formed with the given valAddr, time.
Copy link
Member

Choose a reason for hiding this comment

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

this comment is wrong as it's not correctly describing what happens inside the function. The function deletes all ValidatorConsensusKeyRotationRecordIndexKey for a valAddr that time is <= to the passed in ts. Otherwise this would be just a Delete(Join(valAddr, ts))

func (k Keeper) deleteConsKeyIndexKey(ctx sdk.Context, valAddr sdk.ValAddress, ts time.Time) error {
rng := new(collections.Range[collections.Pair[[]byte, time.Time]]).
StartInclusive(collections.Join(valAddr.Bytes(), time.Time{})).
EndInclusive(collections.Join(valAddr.Bytes(), ts))
atheeshp marked this conversation as resolved.
Show resolved Hide resolved

return k.ValidatorConsensusKeyRotationRecordIndexKey.Walk(ctx, rng, func(key collections.Pair[[]byte, time.Time]) (stop bool, err error) {
return false, k.ValidatorConsensusKeyRotationRecordIndexKey.Remove(ctx, key)
})
Comment on lines +203 to +207
Copy link
Contributor

Choose a reason for hiding this comment

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

The deleteConsKeyIndexKey function may remove more records than intended if the range is not correctly specified. Ensure that the range only includes the specific records that need to be deleted and add tests to verify the correct behavior.

Comment on lines +201 to +207
Copy link
Contributor

Choose a reason for hiding this comment

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

The deleteConsKeyIndexKey function may remove more records than intended if the range is not correctly specified. Ensure that the range only includes the specific records that need to be deleted and add tests to verify the correct behavior. This aligns with the previous comment on the potential issue with the range used in deletion.

}

// GetAllMaturedRotatedKeys returns all matured valaddresses .
func (k Keeper) GetAllMaturedRotatedKeys(ctx sdk.Context, matureTime time.Time) ([][]byte, error) {
atheeshp marked this conversation as resolved.
Show resolved Hide resolved
valAddrs := [][]byte{}

// get an iterator for all timeslices from time 0 until the current HeaderInfo time
rng := new(collections.Range[time.Time]).EndInclusive(matureTime)
err := k.ValidatorConsensusKeyRotationRecordQueue.Walk(ctx, rng, func(key time.Time, value types.ValAddrsOfRotatedConsKeys) (stop bool, err error) {
valAddrs = append(valAddrs, value.Addresses...)
return false, k.ValidatorConsensusKeyRotationRecordQueue.Remove(ctx, key)
})
if err != nil {
return nil, err
}

return valAddrs, nil
}

// GetBlockConsPubKeyRotationHistory iterator over the rotation history for the given height.
func (k Keeper) GetBlockConsPubKeyRotationHistory(ctx context.Context) ([]types.ConsPubKeyRotationHistory, error) {
sdkCtx := sdk.UnwrapSDKContext(ctx)

iterator, err := k.RotationHistory.Indexes.Block.MatchExact(ctx, uint64(sdkCtx.BlockHeight()))
atheeshp marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return nil, err
}
defer iterator.Close()

return indexes.CollectValues(ctx, k.RotationHistory, iterator)
}
15 changes: 15 additions & 0 deletions x/staking/keeper/hooks_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package keeper_test

import (
sdk "github.com/cosmos/cosmos-sdk/types"
)

func (s *KeeperTestSuite) TestHookAfterConsensusPubKeyUpdate() {
stKeeper := s.stakingKeeper
ctx := s.ctx
require := s.Require()

rotationFee := sdk.NewInt64Coin("stake", 1000000)
err := stKeeper.Hooks().AfterConsensusPubKeyUpdate(ctx, PKs[0], PKs[1], rotationFee)
require.NoError(err)
}
Comment on lines +1 to +15
Copy link
Contributor

Choose a reason for hiding this comment

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

The test function TestHookAfterConsensusPubKeyUpdate is well-structured and follows the standard Go testing conventions. It uses the require package to assert that no error is returned from the AfterConsensusPubKeyUpdate method, which is a good practice for failing fast on errors in tests. However, the test seems to only check for the absence of errors and does not verify the state changes or side effects that should result from the AfterConsensusPubKeyUpdate call. It would be beneficial to add assertions to ensure that the intended effects on the state (like updates to the community pool or slashing logic) actually occur.

// Additional assertions should be added here to check for the expected state changes.
// For example, if the community pool is supposed to be updated, assert that it has the expected value.
// If slashing conditions are to be checked, assert that they are handled correctly.

2 changes: 1 addition & 1 deletion x/staking/keeper/msg_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -661,7 +661,7 @@ func (k msgServer) RotateConsPubKey(ctx context.Context, msg *types.MsgRotateCon
return nil, err
}

err = k.Keeper.bankKeeper.SendCoinsFromAccountToModule(ctx, sdk.AccAddress(valAddr), types.DistributionModuleName, sdk.NewCoins(params.KeyRotationFee))
err = k.Keeper.bankKeeper.SendCoinsFromAccountToModule(ctx, sdk.AccAddress(valAddr), types.PoolModuleName, sdk.NewCoins(params.KeyRotationFee))
if err != nil {
return nil, err
}
Expand Down
55 changes: 55 additions & 0 deletions x/staking/keeper/val_state_change.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import (
"cosmossdk.io/math"
"cosmossdk.io/x/staking/types"

cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec"
cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
sdk "github.com/cosmos/cosmos-sdk/types"
)

Expand Down Expand Up @@ -112,6 +114,11 @@ func (k Keeper) BlockValidatorUpdates(ctx context.Context) ([]abci.ValidatorUpda
)
}

err = k.UpdateAllMaturedConsKeyRotatedKeys(sdkCtx, sdkCtx.HeaderInfo().Time)
if err != nil {
return nil, err
}

return validatorUpdates, nil
}

Expand Down Expand Up @@ -235,6 +242,54 @@ func (k Keeper) ApplyAndReturnValidatorSetUpdates(ctx context.Context) (updates
updates = append(updates, validator.ABCIValidatorUpdateZero())
}

// ApplyAndReturnValidatorSetUpdates checks if there is ConsPubKeyRotationHistory
// with ConsPubKeyRotationHistory.RotatedHeight == ctx.BlockHeight() and if so, generates 2 ValidatorUpdate,
// one for a remove validator and one for create new validator
historyObjects, err := k.GetBlockConsPubKeyRotationHistory(ctx)
if err != nil {
return nil, err
}

for _, history := range historyObjects {
valAddr := history.OperatorAddress
if err != nil {
return nil, err
}

validator := k.mustGetValidator(ctx, valAddr)

oldPk := history.OldConsPubkey.GetCachedValue().(cryptotypes.PubKey)
oldTmPk, err := cryptocodec.ToCmtProtoPublicKey(oldPk)
atheeshp marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return nil, err
}

newPk := history.NewConsPubkey.GetCachedValue().(cryptotypes.PubKey)
newTmPk, err := cryptocodec.ToCmtProtoPublicKey(newPk)
if err != nil {
return nil, err
}

if !(validator.Jailed || validator.Status != types.Bonded) {
Copy link
Member

Choose a reason for hiding this comment

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

Add a comment here as this kind of checks can be confusing (and I think it might be wrong). This means that a validator update will be added/modified if the validator is not jailed (regardless of the bonding status); but if the validator is jailed only if the validator is bonded.
I don't think that being jailed AND bonded is possible tho.

Copy link
Contributor

Choose a reason for hiding this comment

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

You cannot be jailed and bonded. As soon as you're jailed, you're put into the unbonding statue/queue. Agree with @facundomedica here. Let's (A) avoid !(...) and (B) add a very good comment above explaining what we're doing 👍

Copy link
Contributor Author

@atheeshp atheeshp Dec 17, 2023

Choose a reason for hiding this comment

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

This means that a validator update will be added/modified if the validator is not jailed (regardless of the bonding status)

  • No, if the validator is not jailed it'll definitely check the status of the validator (it can be unbonded, unbonding or unspecified status)
  • if the validator is jailed it'll abort the updation (doesn't enter the if block)
  • if the validator jailed false and not in bonded status then also if condition doesn't satisfy.
Assuming the condition should as like below:
// validator cannot rotate it's keys if not bonded or jailed.
// - a validator can be unbonding state but jailed status false
// - a validator can be jailed and status can be unbonding

updates = append(updates, abci.ValidatorUpdate{
PubKey: oldTmPk,
Power: 0,
})

updates = append(updates, abci.ValidatorUpdate{
PubKey: newTmPk,
Power: validator.ConsensusPower(powerReduction),
})

if err := k.updateToNewPubkey(ctx, validator, history.OldConsPubkey, history.NewConsPubkey, history.Fee); err != nil {
return nil, err
}
}
}

// TODO: at previousVotes Iteration logic of AllocateTokens, previousVote using OldConsPubKey
// match up with ConsPubKeyRotationHistory, and replace validator for token allocation
atheeshp marked this conversation as resolved.
Show resolved Hide resolved

// Update the pools based on the recent updates in the validator set:
// - The tokens from the non-bonded candidates that enter the new validator set need to be transferred
// to the Bonded pool.
Expand Down
Loading
Loading