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.

Sign up for GitHub

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!: update unbonding pausing for PSS #1728

Merged
merged 4 commits into from
Apr 11, 2024
Merged

fix!: update unbonding pausing for PSS #1728

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
33c0b2a
draft PSS unbonding fix
sainoe Mar 26, 2024
3e4eeb2
fix hook logic to retrieve validator address from ubd op
sainoe Apr 5, 2024
df60ced
add unbonding pausing unit-test
sainoe Apr 5, 2024
26ae70e
remove panic in hook
sainoe Apr 8, 2024
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
59 changes: 59 additions & 0 deletions testutil/keeper/mocks.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

59 changes: 58 additions & 1 deletion x/ccv/provider/keeper/hooks.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
v1 "github.com/cosmos/cosmos-sdk/x/gov/types/v1"
stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"

"github.com/cosmos/interchain-security/v4/x/ccv/provider/types"
providertypes "github.com/cosmos/interchain-security/v4/x/ccv/provider/types"
ccvtypes "github.com/cosmos/interchain-security/v4/x/ccv/types"
)
Expand Down Expand Up @@ -35,8 +36,64 @@ func (k *Keeper) Hooks() Hooks {
func (h Hooks) AfterUnbondingInitiated(ctx sdk.Context, id uint64) error {
var consumerChainIDS []string

// get validator address from unbonding operation
unbondingType, found := h.k.stakingKeeper.GetUnbondingType(ctx, id)
vadAddrBech32 := ""
if !found {
ctx.Logger().Error("undefined type for unbonding operation id: %d", id)
Copy link
Contributor

@insumity insumity Apr 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Should this and the ones below return an actual error?
It will then be logged here. Right?
Or do we return nil so the transaction does not fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Returning an error here can potentially halt the provider when the hook is called after a validator starts unbonding in val_state_change.go. So my suggestion is to stick to just logging those errors from the hook's perspective without returning any.

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense. Thanks!

return nil
}

switch unbondingType {
case stakingtypes.UnbondingType_UnbondingDelegation:
ubd, found := h.k.stakingKeeper.GetUnbondingDelegationByUnbondingID(ctx, id)
if !found {
ctx.Logger().Error("unfound ubonding delegation for unbonding id: %d", id)
return nil
}
vadAddrBech32 = ubd.ValidatorAddress
case stakingtypes.UnbondingType_Redelegation:
red, found := h.k.stakingKeeper.GetRedelegationByUnbondingID(ctx, id)
if !found {
ctx.Logger().Error("unfound relegation for unbonding operation id: %d", id)
return nil
}
vadAddrBech32 = red.ValidatorSrcAddress
case stakingtypes.UnbondingType_ValidatorUnbonding:
val, found := h.k.stakingKeeper.GetValidatorByUnbondingID(ctx, id)
if !found {
ctx.Logger().Error("unfound validator for unbonding operation id: %d", id)
return nil
}
vadAddrBech32 = val.OperatorAddress
default:
ctx.Logger().Error("invalid unbonding operation type: %s", unbondingType)
return nil
}

valAddr, err := sdk.ValAddressFromBech32(vadAddrBech32)
if err != nil {
ctx.Logger().Error(err.Error())
return nil
}

validator, found := h.k.stakingKeeper.GetValidator(ctx, valAddr)
if !found {
ctx.Logger().Error("unfound validator for validator address %s", vadAddrBech32)
return nil
}

consAddr, err := validator.GetConsAddr()
if err != nil {
ctx.Logger().Error(err.Error())
return nil
}

// get all consumers where the validator is in the validator set
for _, chain := range h.k.GetAllConsumerChains(ctx) {
consumerChainIDS = append(consumerChainIDS, chain.ChainId)
if h.k.IsConsumerValidator(ctx, chain.ChainId, types.NewProviderConsAddress(consAddr)) {
consumerChainIDS = append(consumerChainIDS, chain.ChainId)
}
}

if len(consumerChainIDS) == 0 {
Expand Down
68 changes: 67 additions & 1 deletion x/ccv/provider/keeper/relay_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@ import (

"cosmossdk.io/math"

codectypes "github.com/cosmos/cosmos-sdk/codec/types"
cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec"
cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
sdk "github.com/cosmos/cosmos-sdk/types"
stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"

Expand Down Expand Up @@ -468,6 +470,10 @@ func TestHandleVSCMaturedPacket(t *testing.T) {

// Start first unbonding without any consumers registered
var unbondingOpId uint64 = 1
gomock.InOrder(
mocks.MockStakingKeeper.EXPECT().GetUnbondingType(ctx, unbondingOpId).Return(stakingtypes.UnbondingType_Undefined, false),
)

err := pk.Hooks().AfterUnbondingInitiated(ctx, unbondingOpId)
require.NoError(t, err)
// Check that no unbonding op was stored
Expand All @@ -478,12 +484,34 @@ func TestHandleVSCMaturedPacket(t *testing.T) {
pk.IncrementValidatorSetUpdateId(ctx)
require.Equal(t, uint64(2), pk.GetValidatorSetUpdateId(ctx))

// Registered first consumer
// Register first consumer
pk.SetConsumerClientId(ctx, "chain-1", "client-1")

// Create 2 validators
vals := []stakingtypes.Validator{}
valsPk := []cryptotypes.PubKey{}
for i := 0; i < 2; i++ {
pubkey, err := cryptocodec.FromTmPubKeyInterface(cryptotestutil.NewCryptoIdentityFromIntSeed(54321 + i).TMCryptoPubKey())
require.NoError(t, err)
valsPk = append(valsPk, pubkey)
pkAny, err := codectypes.NewAnyWithValue(pubkey)
require.NoError(t, err)
vals = append(vals, stakingtypes.Validator{ConsensusPubkey: pkAny})
}

// Opt-in one validator to consumer
pk.SetConsumerValidator(ctx, "chain-1", types.ConsumerValidator{ProviderConsAddr: valsPk[0].Address()})

// Start second unbonding
unbondingOpId = 2
gomock.InOrder(
mocks.MockStakingKeeper.EXPECT().GetUnbondingType(ctx, unbondingOpId).Return(stakingtypes.UnbondingType_UnbondingDelegation, true),
mocks.MockStakingKeeper.EXPECT().GetUnbondingDelegationByUnbondingID(ctx, unbondingOpId).Return(
stakingtypes.UnbondingDelegation{
ValidatorAddress: sdk.ValAddress([]byte{1}).String(),
}, true),
mocks.MockStakingKeeper.EXPECT().GetValidator(ctx, sdk.ValAddress([]byte{1})).
Return(vals[0], true),
mocks.MockStakingKeeper.EXPECT().PutUnbondingOnHold(ctx, unbondingOpId).Return(nil),
)
err = pk.Hooks().AfterUnbondingInitiated(ctx, unbondingOpId)
Expand All @@ -507,10 +535,21 @@ func TestHandleVSCMaturedPacket(t *testing.T) {
// Registered second consumer
pk.SetConsumerClientId(ctx, "chain-2", "client-2")

// Opt-in both validators to second consumer
pk.SetConsumerValidator(ctx, "chain-2", types.ConsumerValidator{ProviderConsAddr: valsPk[0].Address()})
pk.SetConsumerValidator(ctx, "chain-2", types.ConsumerValidator{ProviderConsAddr: valsPk[1].Address()})

// Start third and fourth unbonding
unbondingOpIds := []uint64{3, 4}
for _, id := range unbondingOpIds {
gomock.InOrder(
mocks.MockStakingKeeper.EXPECT().GetUnbondingType(ctx, id).Return(stakingtypes.UnbondingType_Redelegation, true),
mocks.MockStakingKeeper.EXPECT().GetRedelegationByUnbondingID(ctx, id).Return(
stakingtypes.Redelegation{
ValidatorSrcAddress: sdk.ValAddress([]byte{1}).String(),
}, true),
mocks.MockStakingKeeper.EXPECT().GetValidator(ctx, sdk.ValAddress([]byte{1})).
Return(vals[0], true),
mocks.MockStakingKeeper.EXPECT().PutUnbondingOnHold(ctx, id).Return(nil),
)
err = pk.Hooks().AfterUnbondingInitiated(ctx, id)
Expand All @@ -531,6 +570,33 @@ func TestHandleVSCMaturedPacket(t *testing.T) {
require.Equal(t, unbondingOpIds, ids)
}

// Increment vscID
pk.IncrementValidatorSetUpdateId(ctx)
require.Equal(t, uint64(4), pk.GetValidatorSetUpdateId(ctx))

// Start fith unbonding
unbondingOpId = 5
gomock.InOrder(
mocks.MockStakingKeeper.EXPECT().GetUnbondingType(ctx, unbondingOpId).Return(stakingtypes.UnbondingType_ValidatorUnbonding, true),
mocks.MockStakingKeeper.EXPECT().GetValidatorByUnbondingID(ctx, unbondingOpId).Return(
stakingtypes.Validator{
OperatorAddress: sdk.ValAddress([]byte{1}).String(),
}, true),
mocks.MockStakingKeeper.EXPECT().GetValidator(ctx, sdk.ValAddress([]byte{1})).
Return(vals[1], true),
mocks.MockStakingKeeper.EXPECT().PutUnbondingOnHold(ctx, unbondingOpId).Return(nil),
)
err = pk.Hooks().AfterUnbondingInitiated(ctx, unbondingOpId)
require.NoError(t, err)

// Check that an unbonding op was stored for chain-2 only
// since it's the only consumer the unbonding validator has opted-in to
expectedChains = []string{"chain-2"}
unbondingOp, found = pk.GetUnbondingOp(ctx, unbondingOpId)
require.True(t, found)
require.Equal(t, unbondingOpId, unbondingOp.Id)
require.Equal(t, expectedChains, unbondingOp.UnbondingConsumerChains)

// Handle VSCMatured packet from chain-1 for vscID 1.
// Note that no VSCPacket was sent as the chain was not yet registered,
// but the code should still work
Expand Down
4 changes: 4 additions & 0 deletions x/ccv/types/expected_keepers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@ type StakingKeeper interface {
IterateLastValidatorPowers(ctx sdk.Context, cb func(addr sdk.ValAddress, power int64) (stop bool))
PowerReduction(ctx sdk.Context) math.Int
PutUnbondingOnHold(ctx sdk.Context, id uint64) error
GetUnbondingDelegationByUnbondingID(ctx sdk.Context, id uint64) (ubd stakingtypes.UnbondingDelegation, found bool)
GetRedelegationByUnbondingID(ctx sdk.Context, id uint64) (red stakingtypes.Redelegation, found bool)
GetValidatorByUnbondingID(ctx sdk.Context, id uint64) (val stakingtypes.Validator, found bool)
IterateValidators(ctx sdk.Context, f func(index int64, validator stakingtypes.ValidatorI) (stop bool))
Validator(ctx sdk.Context, addr sdk.ValAddress) stakingtypes.ValidatorI
IsValidatorJailed(ctx sdk.Context, addr sdk.ConsAddress) bool
Expand All @@ -54,6 +57,7 @@ type StakingKeeper interface {
GetUnbondingDelegationsFromValidator(ctx sdk.Context, valAddr sdk.ValAddress) (ubds []stakingtypes.UnbondingDelegation)
GetRedelegationsFromSrcValidator(ctx sdk.Context, valAddr sdk.ValAddress) (reds []stakingtypes.Redelegation)
GetUnbondingType(ctx sdk.Context, id uint64) (unbondingType stakingtypes.UnbondingType, found bool)
BlockValidatorUpdates(ctx sdk.Context) []abci.ValidatorUpdate
}

// SlashingKeeper defines the contract expected to perform ccv slashing
Expand Down
Loading