Separate validation and handling for recv slash packets

tests/e2e/slashing.go

@@ -238,14 +238,13 @@ func (suite *CCVTestSuite) TestHandleSlashPacketDoubleSigning() {
 		slashingtypes.ValidatorSigningInfo{Address: consAddr.String()},
 	)
 
-	_, err := providerKeeper.HandleSlashPacket(suite.providerCtx(), suite.consumerChain.ChainID,
+	providerKeeper.HandleSlashPacket(suite.providerCtx(), suite.consumerChain.ChainID,
 		ccv.NewSlashPacketData(
 			abci.Validator{Address: tmVal.Address, Power: 0},
 			uint64(0),
 			stakingtypes.DoubleSign,
 		),
 	)
-	suite.NoError(err)
 
 	// verify that validator is jailed in the staking and slashing modules' states
 	suite.Require().True(providerStakingKeeper.IsValidatorJailed(suite.providerCtx(), consAddr))
@@ -255,29 +254,69 @@ func (suite *CCVTestSuite) TestHandleSlashPacketDoubleSigning() {
 	suite.Require().True(signingInfo.Tombstoned)
 }
 
-// TestHandleSlashPacketErrors tests errors for the HandleSlashPacket method in an e2e testing setting
-func (suite *CCVTestSuite) TestHandleSlashPacketErrors() {
+// TestOnRecvSlashPacketErrors tests errors for the OnRecvSlashPacket method in an e2e testing setting
+func (suite *CCVTestSuite) TestOnRecvSlashPacketErrors() {
+
 	providerStakingKeeper := suite.providerApp.GetE2eStakingKeeper()
-	ProviderKeeper := suite.providerApp.GetProviderKeeper()
+	providerKeeper := suite.providerApp.GetProviderKeeper()
 	providerSlashingKeeper := suite.providerApp.GetE2eSlashingKeeper()
-	consumerChainID := suite.consumerChain.ChainID
+	firstBundle := suite.getFirstBundle()
+	consumerChainID := firstBundle.Chain.ChainID
+
+	suite.SetupAllCCVChannels()
 
 	// sync contexts block height
 	ctx := suite.providerCtx()
 
-	// expect an error if initial block height isn't set for consumer chain
-	_, err := ProviderKeeper.HandleSlashPacket(ctx, consumerChainID, ccv.SlashPacketData{})
-	suite.Require().Error(err, "slash validator with invalid infraction height")
-
-	// save VSC ID
-	vID := ProviderKeeper.GetValidatorSetUpdateId(ctx)
+	// Expect panic if ccv channel is not established via dest channel of packet
+	suite.Panics(func() {
+		providerKeeper.OnRecvSlashPacket(ctx, channeltypes.Packet{}, ccv.SlashPacketData{})
+	})
 
-	// remove block height for current VSC ID
-	ProviderKeeper.DeleteValsetUpdateBlockHeight(ctx, vID)
+	// Add correct channelID to packet. Now we will not panic anymore.
+	packet := channeltypes.Packet{DestinationChannel: firstBundle.Path.EndpointB.ChannelID}
 
-	// expect an error if block height mapping VSC ID is zero
-	_, err = ProviderKeeper.HandleSlashPacket(ctx, consumerChainID, ccv.SlashPacketData{ValsetUpdateId: vID})
-	suite.Require().Error(err, "slash with height mapping to zero")
+	// Init chain height is set by established CCV channel
+	// Delete init chain height and confirm expected error
+	initChainHeight, found := providerKeeper.GetInitChainHeight(ctx, consumerChainID)
+	suite.Require().True(found)
+	providerKeeper.DeleteInitChainHeight(ctx, consumerChainID)
+
+	packetData := ccv.SlashPacketData{ValsetUpdateId: 0}
+	errAck := providerKeeper.OnRecvSlashPacket(ctx, packet, packetData)
+	suite.Require().False(errAck.Success())
+	errAckCast := errAck.(channeltypes.Acknowledgement)
+	suite.Require().Equal(
+		fmt.Sprintf("cannot find infraction height matching the validator update id 0 for chain %s",
+			firstBundle.Chain.ChainID), errAckCast.GetError())
+
+	// Restore init chain height
+	providerKeeper.SetInitChainHeight(ctx, consumerChainID, initChainHeight)
+
+	// now the method will fail at infraction height check.
+	packetData.Infraction = stakingtypes.InfractionEmpty
+	errAck = providerKeeper.OnRecvSlashPacket(ctx, packet, packetData)
+	suite.Require().False(errAck.Success())
+	errAckCast = errAck.(channeltypes.Acknowledgement)
+	suite.Require().Equal(
+		fmt.Sprintf("invalid infraction type: %s", packetData.Infraction.String()), errAckCast.GetError())
+
+	// save current VSC ID
+	vscID := providerKeeper.GetValidatorSetUpdateId(ctx)
+
+	// remove block height value mapped to current VSC ID
+	providerKeeper.DeleteValsetUpdateBlockHeight(ctx, vscID)
+
+	// Instantiate packet data with current VSC ID
+	packetData = ccv.SlashPacketData{ValsetUpdateId: vscID}
+
+	// expect an error if mapped block height is not found
+	errAck = providerKeeper.OnRecvSlashPacket(ctx, packet, packetData)
+	suite.Require().False(errAck.Success())
+	errAckCast = errAck.(channeltypes.Acknowledgement)
+	suite.Require().Equal(
+		fmt.Sprintf("cannot find infraction height matching the validator update id %d for chain %s",
+			vscID, firstBundle.Chain.ChainID), errAckCast.GetError())
 
 	// construct slashing packet with non existing validator
 	slashingPkt := ccv.NewSlashPacketData(
@@ -286,12 +325,13 @@ func (suite *CCVTestSuite) TestHandleSlashPacketErrors() {
 	)
 
 	// Set initial block height for consumer chain
-	ProviderKeeper.SetInitChainHeight(ctx, consumerChainID, uint64(ctx.BlockHeight()))
+	providerKeeper.SetInitChainHeight(ctx, consumerChainID, uint64(ctx.BlockHeight()))
 
-	// expect the slash to not succeed if validator doesn't exist
-	success, err := ProviderKeeper.HandleSlashPacket(ctx, consumerChainID, slashingPkt)
-	suite.Require().NoError(err, "slashing an unknown validator should not result in error")
-	suite.Require().False(success, "did slash unknown validator")
+	// Expect no error ack if validator does not exist
+	// TODO: this behavior should be changed to return an error ack,
+	// see: https://github.com/cosmos/interchain-security/issues/546
+	ack := providerKeeper.OnRecvSlashPacket(ctx, packet, slashingPkt)
+	suite.Require().True(ack.Success())
 
 	// jail an existing validator
 	val := suite.providerChain.Vals.Validators[0]
@@ -301,7 +341,7 @@ func (suite *CCVTestSuite) TestHandleSlashPacketErrors() {
 	suite.coordinator.CommitBlock(suite.providerChain)
 	// Update suite.ctx bc CommitBlock updates only providerChain's current header block height
 	ctx = suite.providerChain.GetContext()
-	suite.Require().NotZero(ProviderKeeper.GetValsetUpdateBlockHeight(ctx, vID))
+	suite.Require().NotZero(providerKeeper.GetValsetUpdateBlockHeight(ctx, vscID))
 
 	// create validator signing info
 	valInfo := slashingtypes.NewValidatorSigningInfo(sdk.ConsAddress(val.Address), ctx.BlockHeight(),
@@ -310,31 +350,29 @@ func (suite *CCVTestSuite) TestHandleSlashPacketErrors() {
 
 	// update validator address and VSC ID
 	slashingPkt.Validator.Address = val.Address
-	slashingPkt.ValsetUpdateId = vID
+	slashingPkt.ValsetUpdateId = vscID
 
 	// expect to slash and jail validator
-	_, err = ProviderKeeper.HandleSlashPacket(ctx, consumerChainID, slashingPkt)
-	suite.Require().NoError(err, "did slash jail validator")
+	ack = providerKeeper.OnRecvSlashPacket(ctx, packet, slashingPkt)
+	suite.Require().True(ack.Success())
+	suite.Require().True(providerStakingKeeper.IsValidatorJailed(ctx, consAddr))
 
-	// expect error when infraction type in unspecified
+	// expect error ack when infraction type in unspecified
 	tmAddr := suite.providerChain.Vals.Validators[1].Address
 	slashingPkt.Validator.Address = tmAddr
 	slashingPkt.Infraction = stakingtypes.InfractionEmpty
 
 	valInfo.Address = sdk.ConsAddress(tmAddr).String()
 	providerSlashingKeeper.SetValidatorSigningInfo(ctx, sdk.ConsAddress(tmAddr), valInfo)
 
-	_, err = ProviderKeeper.HandleSlashPacket(ctx, consumerChainID, slashingPkt)
-	suite.Require().EqualError(err, fmt.Sprintf("invalid infraction type: %v", stakingtypes.InfractionEmpty))
+	errAck = providerKeeper.OnRecvSlashPacket(ctx, packet, slashingPkt)
+	suite.Require().False(errAck.Success())
 
-	// expect to slash jail validator
+	// expect to slash and jail validator
 	slashingPkt.Infraction = stakingtypes.DoubleSign
-	_, err = ProviderKeeper.HandleSlashPacket(ctx, consumerChainID, slashingPkt)
-	suite.Require().NoError(err)
-
-	// expect the slash to not succeed when validator is tombstoned
-	success, _ = ProviderKeeper.HandleSlashPacket(ctx, consumerChainID, slashingPkt)
-	suite.Require().False(success)
+	ack = providerKeeper.OnRecvSlashPacket(ctx, packet, slashingPkt)
+	suite.Require().True(ack.Success())
+	suite.Require().True(providerStakingKeeper.IsValidatorJailed(ctx, sdk.ConsAddress(tmAddr)))
 }
 
 // TestHandleSlashPacketDistribution tests the slashing of an undelegation balance
@@ -426,8 +464,7 @@ func (suite *CCVTestSuite) TestHandleSlashPacketDistribution() {
 		)
 
 		// slash
-		_, err := providerKeeper.HandleSlashPacket(suite.providerChain.GetContext(), suite.consumerChain.ChainID, slashPacket)
-		suite.Require().NoError(err)
+		providerKeeper.HandleSlashPacket(suite.providerChain.GetContext(), suite.consumerChain.ChainID, slashPacket)
 
 		ubd, found := providerStakingKeeper.GetUnbondingDelegation(suite.providerChain.GetContext(), delAddr, valAddr)
 		suite.Require().True(found)

testutil/e2e/debug_test.go

@@ -116,8 +116,8 @@ func TestHandleSlashPacketDoubleSigning(t *testing.T) {
 	runCCVTestByName(t, "TestHandleSlashPacketDoubleSigning")
 }
 
-func TestHandleSlashPacketErrors(t *testing.T) {
-	runCCVTestByName(t, "TestHandleSlashPacketErrors")
+func TestOnRecvSlashPacketErrors(t *testing.T) {
+	runCCVTestByName(t, "TestOnRecvSlashPacketErrors")
 }
 
 func TestHandleSlashPacketDistribution(t *testing.T) {

testutil/keeper/expectations.go

@@ -5,6 +5,7 @@ import (
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types"
+	stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
 	clienttypes "github.com/cosmos/ibc-go/v3/modules/core/02-client/types"
 	conntypes "github.com/cosmos/ibc-go/v3/modules/core/03-connection/types"
 	channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types"
@@ -90,6 +91,63 @@ func GetMocksForStopConsumerChain(ctx sdk.Context, mocks *MockedKeepers) []*gomo
 	}
 }
 
+func GetMocksForHandleSlashPacket(ctx sdk.Context, mocks MockedKeepers,
+	expectedPacketData ccv.SlashPacketData, valToReturn stakingtypes.Validator) []*gomock.Call {
+
+	// These first two calls are always made.
+	calls := []*gomock.Call{
+
+		mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr(
+			ctx, sdk.ConsAddress(expectedPacketData.Validator.Address)).Return(
+			valToReturn, true,
+		).Times(1),
+
+		mocks.MockSlashingKeeper.EXPECT().IsTombstoned(ctx,
+			sdk.ConsAddress(expectedPacketData.Validator.Address)).Return(false).Times(1),
+	}
+
+	// Different calls are made depending on the type infraction.
+	if expectedPacketData.Infraction == stakingtypes.Downtime {
+		calls = append(calls,
+			mocks.MockSlashingKeeper.EXPECT().SlashFractionDowntime(ctx).Return(sdk.NewDec(1)).Times(1),
+			mocks.MockSlashingKeeper.EXPECT().DowntimeJailDuration(ctx).Return(time.Hour).Times(1),
+		)
+
+	} else if expectedPacketData.Infraction == stakingtypes.DoubleSign {
+		calls = append(calls,
+			mocks.MockSlashingKeeper.EXPECT().SlashFractionDoubleSign(ctx).Return(sdk.NewDec(1)).Times(1),
+			mocks.MockSlashingKeeper.EXPECT().Tombstone(ctx,
+				sdk.ConsAddress(expectedPacketData.Validator.Address)).Times(1),
+		)
+	}
+
+	// Slash is always called.
+	calls = append(calls, mocks.MockStakingKeeper.EXPECT().Slash(
+		ctx,
+		sdk.ConsAddress(expectedPacketData.Validator.Address),
+		gomock.Any(),  // infraction height
+		int64(0),      // power
+		sdk.NewDec(1), // Slash fraction
+		expectedPacketData.Infraction).Return().Times(1),
+	)
+
+	// Jail() is only called if the validator is not already jailed.
+	if !valToReturn.IsJailed() {
+		calls = append(calls, mocks.MockStakingKeeper.EXPECT().Jail(
+			gomock.Eq(ctx),
+			gomock.Eq(sdk.ConsAddress(expectedPacketData.Validator.Address)),
+		).Return())
+	}
+
+	// JailUntil() time is always updated.
+	calls = append(calls,
+		mocks.MockSlashingKeeper.EXPECT().JailUntil(ctx, sdk.ConsAddress(expectedPacketData.Validator.Address),
+			gomock.Any()).Times(1),
+	)
+
+	return calls
+}
+
 func ExpectLatestConsensusStateMock(ctx sdk.Context, mocks MockedKeepers, clientID string, consState *ibctmtypes.ConsensusState) *gomock.Call {
 	return mocks.MockClientKeeper.EXPECT().
 		GetLatestClientConsensusState(ctx, clientID).Return(consState, true).Times(1)

x/ccv/provider/keeper/genesis.go

@@ -40,17 +40,13 @@ func (k Keeper) InitGenesis(ctx sdk.Context, genState *types.GenesisState) {
 		k.SetPendingConsumerRemovalProp(ctx, prop.ChainId, prop.StopTime)
 	}
 	for _, ubdOp := range genState.UnbondingOps {
-		if err := k.SetUnbondingOp(ctx, ubdOp); err != nil {
-			panic(fmt.Errorf("unbonding op could not be persisted: %w", err))
-		}
+		k.SetUnbondingOp(ctx, ubdOp)
 	}
 
 	// Note that MatureUnbondingOps aren't stored across blocks, but it
 	// might be used after implementing sovereign to consumer transition
 	if genState.MatureUnbondingOps != nil {
-		if err := k.AppendMaturedUnbondingOps(ctx, genState.MatureUnbondingOps.Ids); err != nil {
-			panic(err)
-		}
+		k.AppendMaturedUnbondingOps(ctx, genState.MatureUnbondingOps.Ids)
 	}
 
 	// Set initial state for each consumer chain

x/ccv/provider/keeper/hooks.go

@@ -46,12 +46,11 @@ func (h Hooks) AfterUnbondingInitiated(ctx sdk.Context, ID uint64) error {
 		h.k.SetUnbondingOpIndex(ctx, consumerChainID, valsetUpdateID, index)
 	}
 
-	// Set unbondingOp
-	if err := h.k.SetUnbondingOp(ctx, unbondingOp); err != nil {
-		// If there was an error persisting the unbonding op, panic to end execution for
-		// the current tx and prevent committal of this invalid state.
-		panic(fmt.Errorf("unbonding op could not be persisted: %w", err))
-	}
+	// Set unbonding op.
+	// If there is an error persisting the unbonding op, the method will
+	// panic to end execution for the current tx and prevent committal
+	// of this invalid state.
+	h.k.SetUnbondingOp(ctx, unbondingOp)
 
 	// Call back into staking to tell it to stop this op from unbonding when the unbonding period is complete
 	if err := h.k.stakingKeeper.PutUnbondingOnHold(ctx, ID); err != nil {

x/ccv/provider/keeper/keeper.go

@@ -303,14 +303,13 @@ func (k Keeper) SetConsumerChain(ctx sdk.Context, channelID string) error {
 }
 
 // Save UnbondingOp by unique ID
-func (k Keeper) SetUnbondingOp(ctx sdk.Context, unbondingOp ccv.UnbondingOp) error {
+func (k Keeper) SetUnbondingOp(ctx sdk.Context, unbondingOp ccv.UnbondingOp) {
 	store := ctx.KVStore(k.storeKey)
 	bz, err := unbondingOp.Marshal()
 	if err != nil {
-		return err
+		panic(fmt.Errorf("unbonding op could not be marshaled: %w", err))
 	}
 	store.Set(types.UnbondingOpKey(unbondingOp.Id), bz)
-	return nil
 }
 
 // Get UnbondingOp by unique ID
@@ -450,13 +449,13 @@ func (k Keeper) GetMaturedUnbondingOps(ctx sdk.Context) (ids []uint64, err error
 }
 
 // AppendMaturedUnbondingOps adds a list of ids to the list of matured unbonding operation ids
-func (k Keeper) AppendMaturedUnbondingOps(ctx sdk.Context, ids []uint64) error {
+func (k Keeper) AppendMaturedUnbondingOps(ctx sdk.Context, ids []uint64) {
 	if len(ids) == 0 {
-		return nil
+		return
 	}
 	existingIds, err := k.GetMaturedUnbondingOps(ctx)
 	if err != nil {
-		return err
+		panic(fmt.Sprintf("failed to get matured unbonding operations: %s", err))
 	}
 
 	maturedOps := ccv.MaturedUnbondingOps{
@@ -466,10 +465,9 @@ func (k Keeper) AppendMaturedUnbondingOps(ctx sdk.Context, ids []uint64) error {
 	store := ctx.KVStore(k.storeKey)
 	bz, err := maturedOps.Marshal()
 	if err != nil {
-		return err
+		panic(fmt.Sprintf("failed to marshal matured unbonding operations: %s", err))
 	}
 	store.Set(types.MaturedUnbondingOpsKey(), bz)
-	return nil
 }
 
 // ConsumeMaturedUnbondingOps empties and returns list of matured unbonding operation ids (if it exists)