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: changes to sdk for interchain security #15917

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 0 additions & 5 deletions types/module/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -367,11 +367,6 @@ func (m *Manager) InitGenesis(ctx sdk.Context, cdc codec.JSONCodec, genesisData
}
}

// a chain must initialize with a non-empty validator set
Copy link
Member

Choose a reason for hiding this comment

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

This was added in v0.46 (so it is normal it isn't on v0.45.x and v0.45.x-ics). Could you please tell us which issues you are facing with it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I cannot, but @vuong177 can. He is the person who identified this stuff.

Copy link
Member

Choose a reason for hiding this comment

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

so the consumer app doesn't have a valset because it operate on the provider chain valset, so any time consumer app init genesis the valset check will always return an error

Copy link
Member

@julienrbrt julienrbrt Apr 24, 2023

Choose a reason for hiding this comment

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

I am not that familiar with ICS codebase, but this makes sense.
Have you investigated if the consumer app could recover the panic? I know this isn't ideal, but this is a UX improvement for all but consumer chains.

if len(validatorUpdates) == 0 {
panic(fmt.Sprintf("validator set is empty after InitGenesis, please ensure at least one validator is initialized with a delegation greater than or equal to the DefaultPowerReduction (%d)", sdk.DefaultPowerReduction))
}

return abci.ResponseInitChain{
Validators: validatorUpdates,
}
Expand Down
8 changes: 0 additions & 8 deletions x/slashing/keeper/infractions.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/x/slashing/types"
stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
)

// HandleValidatorSignature handles a validator signature, must be called once per validator per block.
Expand All @@ -16,9 +15,6 @@ func (k Keeper) HandleValidatorSignature(ctx sdk.Context, addr cryptotypes.Addre

// fetch the validator public key
consAddr := sdk.ConsAddress(addr)
if _, err := k.GetPubkey(ctx, addr); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

See #15908

panic(fmt.Sprintf("Validator consensus-address %s not found", consAddr))
}

// don't update missed blocks when validator's jailed
if k.sk.IsValidatorJailed(ctx, consAddr) {
Expand Down Expand Up @@ -88,17 +84,13 @@ func (k Keeper) HandleValidatorSignature(ctx sdk.Context, addr cryptotypes.Addre
// Note that this *can* result in a negative "distributionHeight" up to -ValidatorUpdateDelay-1,
// i.e. at the end of the pre-genesis block (none) = at the beginning of the genesis block.
// That's fine since this is just used to filter unbonding delegations & redelegations.
distributionHeight := height - sdk.ValidatorUpdateDelay - 1

coinsBurned := k.sk.SlashWithInfractionReason(ctx, consAddr, distributionHeight, power, k.SlashFractionDowntime(ctx), stakingtypes.Infraction_INFRACTION_DOWNTIME)
ctx.EventManager().EmitEvent(
sdk.NewEvent(
types.EventTypeSlash,
sdk.NewAttribute(types.AttributeKeyAddress, consAddr.String()),
sdk.NewAttribute(types.AttributeKeyPower, fmt.Sprintf("%d", power)),
sdk.NewAttribute(types.AttributeKeyReason, types.AttributeValueMissingSignature),
sdk.NewAttribute(types.AttributeKeyJailed, consAddr.String()),
sdk.NewAttribute(types.AttributeKeyBurnedCoins, coinsBurned.String()),
),
)
k.sk.Jail(ctx, consAddr)
Expand Down
2 changes: 1 addition & 1 deletion x/slashing/testutil/expected_keepers_mocks.go

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

2 changes: 1 addition & 1 deletion x/slashing/types/expected_keepers.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ type StakingKeeper interface {
ValidatorByConsAddr(sdk.Context, sdk.ConsAddress) stakingtypes.ValidatorI // get a particular validator by consensus address

// slash the validator and delegators of the validator, specifying offence height, offence power, and slash fraction
Slash(sdk.Context, sdk.ConsAddress, int64, int64, sdk.Dec) math.Int
Slash(sdk.Context, sdk.ConsAddress, int64, int64, sdk.Dec, stakingtypes.Infraction)
SlashWithInfractionReason(sdk.Context, sdk.ConsAddress, int64, int64, sdk.Dec, stakingtypes.Infraction) math.Int
Jail(sdk.Context, sdk.ConsAddress) // jail a validator
Unjail(sdk.Context, sdk.ConsAddress) // unjail a validator
Expand Down
132 changes: 126 additions & 6 deletions x/staking/keeper/slash.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,132 @@ import (
//
// Infraction was committed at the current height or at a past height,
// not at a height in the future
func (k Keeper) Slash(ctx sdk.Context, consAddr sdk.ConsAddress, infractionHeight int64, power int64, slashFactor sdk.Dec) math.Int {
func (k Keeper) Slash(ctx sdk.Context, consAddr sdk.ConsAddress, infractionHeight int64, power int64, slashFactor sdk.Dec, _ types.Infraction) {
logger := k.Logger(ctx)

if slashFactor.IsNegative() {
panic(fmt.Errorf("attempted to slash with a negative slash factor: %v", slashFactor))
}

// Amount of slashing = slash slashFactor * power at time of infraction
amount := k.TokensFromConsensusPower(ctx, power)
slashAmountDec := sdk.NewDecFromInt(amount).Mul(slashFactor)
slashAmount := slashAmountDec.TruncateInt()

// ref https://github.com/cosmos/cosmos-sdk/issues/1348

validator, found := k.GetValidatorByConsAddr(ctx, consAddr)
if !found {
// If not found, the validator must have been overslashed and removed - so we don't need to do anything
// NOTE: Correctness dependent on invariant that unbonding delegations / redelegations must also have been completely
// slashed in this case - which we don't explicitly check, but should be true.
// Log the slash attempt for future reference (maybe we should tag it too)
logger.Error(
"WARNING: ignored attempt to slash a nonexistent validator; we recommend you investigate immediately",
"validator", consAddr.String(),
)
return
}

// should not be slashing an unbonded validator
if validator.IsUnbonded() {
panic(fmt.Sprintf("should not be slashing unbonded validator: %s", validator.GetOperator()))
}

operatorAddress := validator.GetOperator()

// call the before-modification hook
k.hooks.BeforeValidatorModified(ctx, operatorAddress)

Check warning

Code scanning / gosec

Errors unhandled.

Errors unhandled.

// Track remaining slash amount for the validator
// This will decrease when we slash unbondings and
// redelegations, as that stake has since unbonded
remainingSlashAmount := slashAmount

switch {
case infractionHeight > ctx.BlockHeight():
// Can't slash infractions in the future
panic(fmt.Sprintf(
"impossible attempt to slash future infraction at height %d but we are at height %d",
infractionHeight, ctx.BlockHeight()))

case infractionHeight == ctx.BlockHeight():
// Special-case slash at current height for efficiency - we don't need to
// look through unbonding delegations or redelegations.
logger.Info(
"slashing at current height; not scanning unbonding delegations & redelegations",
"height", infractionHeight,
)

case infractionHeight < ctx.BlockHeight():
// Iterate through unbonding delegations from slashed validator
unbondingDelegations := k.GetUnbondingDelegationsFromValidator(ctx, operatorAddress)
for _, unbondingDelegation := range unbondingDelegations {
amountSlashed := k.SlashUnbondingDelegation(ctx, unbondingDelegation, infractionHeight, slashFactor)
if amountSlashed.IsZero() {
continue
}
remainingSlashAmount = remainingSlashAmount.Sub(amountSlashed)
}

// Iterate through redelegations from slashed source validator
redelegations := k.GetRedelegationsFromSrcValidator(ctx, operatorAddress)
for _, redelegation := range redelegations {
amountSlashed := k.SlashRedelegation(ctx, validator, redelegation, infractionHeight, slashFactor)
if amountSlashed.IsZero() {
continue
}

remainingSlashAmount = remainingSlashAmount.Sub(amountSlashed)
}
}

// cannot decrease balance below zero
tokensToBurn := sdk.MinInt(remainingSlashAmount, validator.Tokens)
tokensToBurn = sdk.MaxInt(tokensToBurn, sdk.ZeroInt()) // defensive.

// we need to calculate the *effective* slash fraction for distribution
if validator.Tokens.IsPositive() {
effectiveFraction := sdk.NewDecFromInt(tokensToBurn).QuoRoundUp(sdk.NewDecFromInt(validator.Tokens))
// possible if power has changed
if effectiveFraction.GT(math.LegacyOneDec()) {
effectiveFraction = math.LegacyOneDec()
}
// call the before-slashed hook
k.hooks.BeforeValidatorSlashed(ctx, operatorAddress, effectiveFraction)

Check warning

Code scanning / gosec

Errors unhandled.

Errors unhandled.
}

// Deduct from validator's bonded tokens and update the validator.
// Burn the slashed tokens from the pool account and decrease the total supply.
validator = k.RemoveValidatorTokens(ctx, validator, tokensToBurn)

switch validator.GetStatus() {
case types.Bonded:
if err := k.burnBondedTokens(ctx, tokensToBurn); err != nil {
panic(err)
}
case types.Unbonding, types.Unbonded:
if err := k.burnNotBondedTokens(ctx, tokensToBurn); err != nil {
panic(err)
}
default:
panic("invalid validator status")
}

logger.Info(
"validator slashed by slash factor",
"validator", validator.GetOperator().String(),
"slash_factor", slashFactor.String(),
"burned", tokensToBurn,
)
}

// SlashWithInfractionReason implementation doesn't require the infraction (types.Infraction) to work but is required by Interchain Security.
func (k Keeper) SlashWithInfractionReason(ctx sdk.Context, consAddr sdk.ConsAddress, infractionHeight int64, power int64, slashFactor sdk.Dec, _ types.Infraction) math.Int {
return k.LegacySlash(ctx, consAddr, infractionHeight, power, slashFactor)
}

func (k Keeper) LegacySlash(ctx sdk.Context, consAddr sdk.ConsAddress, infractionHeight int64, power int64, slashFactor sdk.Dec) math.Int {
logger := k.Logger(ctx)

if slashFactor.IsNegative() {
Expand Down Expand Up @@ -156,11 +281,6 @@ func (k Keeper) Slash(ctx sdk.Context, consAddr sdk.ConsAddress, infractionHeigh
return tokensToBurn
}

// SlashWithInfractionReason implementation doesn't require the infraction (types.Infraction) to work but is required by Interchain Security.
func (k Keeper) SlashWithInfractionReason(ctx sdk.Context, consAddr sdk.ConsAddress, infractionHeight int64, power int64, slashFactor sdk.Dec, _ types.Infraction) math.Int {
return k.Slash(ctx, consAddr, infractionHeight, power, slashFactor)
}

// jail a validator
func (k Keeper) Jail(ctx sdk.Context, consAddr sdk.ConsAddress) {
validator := k.mustGetValidatorByConsAddr(ctx, consAddr)
Expand Down
3 changes: 2 additions & 1 deletion x/staking/keeper/slash_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package keeper_test
import (
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/x/staking/testutil"
"github.com/cosmos/cosmos-sdk/x/staking/types"
)

// tests Jail, Unjail
Expand Down Expand Up @@ -46,5 +47,5 @@ func (s *KeeperTestSuite) TestSlashAtFutureHeight() {
require.NoError(err)

fraction := sdk.NewDecWithPrec(5, 1)
require.Panics(func() { keeper.Slash(ctx, consAddr, 1, 10, fraction) })
require.Panics(func() { keeper.Slash(ctx, consAddr, 1, 10, fraction, types.Infraction_INFRACTION_UNSPECIFIED) })
}
2 changes: 1 addition & 1 deletion x/staking/types/expected_keepers.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ type ValidatorSet interface {
StakingTokenSupply(sdk.Context) math.Int // total staking token supply

// slash the validator and delegators of the validator, specifying offence height, offence power, and slash fraction
Slash(sdk.Context, sdk.ConsAddress, int64, int64, sdk.Dec) math.Int
Slash(sdk.Context, sdk.ConsAddress, int64, int64, sdk.Dec, Infraction)
SlashWithInfractionReason(sdk.Context, sdk.ConsAddress, int64, int64, sdk.Dec, Infraction) math.Int
Copy link
Member

Choose a reason for hiding this comment

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

So, v0.45.x-ics did have a breaking change in Slash, but in v0.47 we decided not to break the API for users, hence the SlashWithInfractionReason.

Copy link
Member

Choose a reason for hiding this comment

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

@vuong177 hey can you check this out, can we move the slash func for consumer keeper to SlashWithInfractionReason

Jail(sdk.Context, sdk.ConsAddress) // jail a validator
Unjail(sdk.Context, sdk.ConsAddress) // unjail a validator
Expand Down