Fix: Patch Consumer initiated slashing

- Use reverse iterator to get last VSCId: [comment-ref](https://github.com)

- Implement Producer chain slashing packet acknowledgement: [comment-ref](cosmos#28 (comment))

- Make unit-tests pass using custom [IBC-GO branch](https://github.com/sainoe/ibc-go/tree/sainoe/valset-update-fixes) with [cosmos#28](cosmos/ibc-go#918 (comment)) fixes

Author: sainoe

go.mod

@@ -37,6 +37,8 @@ require (
 replace (
 	github.com/99designs/keyring => github.com/cosmos/keyring v1.1.7-0.20210622111912-ef00f8ac3d76
 	github.com/cosmos/cosmos-sdk => github.com/cosmos/cosmos-sdk v0.44.1-0.20220112185710-fa19ad5f85c5
+	// github.com/cosmos/ibc-go => github.com/sainoe/ibc-go v1.2.1-0.20220217160816-34b9d8254659
+	github.com/cosmos/ibc-go => /Users/simon/Dev/go-workspace/src/github.com/sainoe/ibc-go
 	github.com/gogo/protobuf => github.com/regen-network/protobuf v1.3.3-alpha.regen.1
 	google.golang.org/grpc => google.golang.org/grpc v1.33.2
 )

go.sum

@@ -259,8 +259,6 @@ github.com/cosmos/iavl v0.15.3/go.mod h1:OLjQiAQ4fGD2KDZooyJG9yz+p2ao2IAYSbke8mV
 github.com/cosmos/iavl v0.16.0/go.mod h1:2A8O/Jz9YwtjqXMO0CjnnbTYEEaovE8jWcwrakH3PoE=
 github.com/cosmos/iavl v0.17.1 h1:b/Cl8h1PRMvsu24+TYNlKchIu7W6tmxIBGe6E9u2Ybw=
 github.com/cosmos/iavl v0.17.1/go.mod h1:7aisPZK8yCpQdy3PMvKeO+bhq1NwDjUwjzxwwROUxFk=
-github.com/cosmos/ibc-go v1.2.1-0.20211111105346-12a60b13a024 h1:Xe7dltlisXJljiFiTDXgm5HDWDE50zU8P08hFLVLmNk=
-github.com/cosmos/ibc-go v1.2.1-0.20211111105346-12a60b13a024/go.mod h1:AINUrX29q811dcarbf5wzKQZKOg46FCLX4apqblzKac=
 github.com/cosmos/keyring v1.1.7-0.20210622111912-ef00f8ac3d76 h1:DdzS1m6o/pCqeZ8VOAit/gyATedRgjvkVI+UCrLpyuU=
 github.com/cosmos/keyring v1.1.7-0.20210622111912-ef00f8ac3d76/go.mod h1:0mkLWIoZuQ7uBoospo5Q9zIpqq6rYCPJDSUdeCJvPM8=
 github.com/cosmos/ledger-cosmos-go v0.11.1 h1:9JIYsGnXP613pb2vPjFeMMjBI5lEDsEaF6oYorTy6J4=

x/ccv/child/keeper/hooks.go

@@ -20,10 +20,16 @@ func (k Keeper) AfterValidatorDowntime(ctx sdk.Context, consAddr sdk.ConsAddress
 		return
 	}
 
+	// get last the valset update id
+	valsetUpdate, err := k.GetLastUnbondingPacketData(ctx)
+	if err != nil {
+		return
+	}
+
 	// increase jail time
 	signInfo.JailedUntil = ctx.BlockHeader().Time.Add(k.slashingKeeper.DowntimeJailDuration(ctx))
 
-	// reset the missed block counters
+	// reset missed block counters
 	signInfo.MissedBlocksCounter = 0
 	signInfo.IndexOffset = 0
 	k.slashingKeeper.ClearValidatorMissedBlockBitArray(ctx, consAddr)
@@ -38,6 +44,7 @@ func (k Keeper) AfterValidatorDowntime(ctx sdk.Context, consAddr sdk.ConsAddress
 			Address: consAddr.Bytes(),
 			Power:   power,
 		},
+		valsetUpdate.ValsetUpdateId,
 		k.slashingKeeper.SlashFractionDowntime(ctx).TruncateInt64(),
 		signInfo.JailedUntil.UnixNano(),
 	)

x/ccv/child/keeper/keeper.go

@@ -317,15 +317,26 @@ func (k Keeper) VerifyParentChain(ctx sdk.Context, channelID string) error {
 	return nil
 }
 
-// GetLastUnbondingPacket returns the last unbounding packet stored in lexical order
-func (k Keeper) GetLastUnbondingPacket(ctx sdk.Context) ccv.ValidatorSetChangePacketData {
-	ubdPacket := &channeltypes.Packet{}
-	k.IterateUnbondingPacket(ctx, func(seq uint64, packet channeltypes.Packet) bool {
-		*ubdPacket = packet
-		return false
-	})
+// GetLastUnbondingPacket returns the last unbounding packet data stored in lexical order
+func (k Keeper) GetLastUnbondingPacketData(ctx sdk.Context) (ccv.ValidatorSetChangePacketData, error) {
+	store := ctx.KVStore(k.storeKey)
+	// use a reverse iterator to get the last entry
+	iterator := sdk.KVStoreReversePrefixIterator(store, []byte(types.UnbondingPacketPrefix))
+	if !iterator.Valid() {
+		return ccv.ValidatorSetChangePacketData{}, sdkerrors.Wrapf(ccv.ErrInvalidChildState, "Invalid unbonding packet iterator")
+	}
+
+	var packet channeltypes.Packet
+	err := packet.Unmarshal(iterator.Value())
+	if err != nil {
+		return ccv.ValidatorSetChangePacketData{}, err
+	}
+
 	var data ccv.ValidatorSetChangePacketData
+	err = ccv.ModuleCdc.UnmarshalJSON(packet.GetData(), &data)
+	if err != nil {
+		return ccv.ValidatorSetChangePacketData{}, err
+	}
 
-	ccv.ModuleCdc.UnmarshalJSON(ubdPacket.GetData(), &data)
-	return data
+	return data, nil
 }

x/ccv/child/keeper/keeper_test.go

@@ -382,6 +382,10 @@ func (suite *KeeperTestSuite) TestValidatorDowntime() {
 	// add the validator pubkey and signing info to the store
 	app.SlashingKeeper.AddPubkey(ctx, pubkey)
 
+	// set unbounding packet with valset update id
+	vscPacket := ccv.ValidatorSetChangePacketData{ValsetUpdateId: uint64(3)}
+	app.ChildKeeper.SetUnbondingPacket(ctx, uint64(0), channeltypes.Packet{Data: vscPacket.GetBytes()})
+
 	valInfo := slashingtypes.NewValidatorSigningInfo(consAddr, ctx.BlockHeight(), ctx.BlockHeight()-1,
 		time.Time{}.UTC(), false, int64(0))
 	app.SlashingKeeper.SetValidatorSigningInfo(ctx, consAddr, valInfo)
@@ -419,8 +423,7 @@ func (suite *KeeperTestSuite) TestAfterValidatorDowntimeHook() {
 
 	now := time.Now()
 
-	// Cover the cases when the validatir jailing duration
-	// is either elapsed, null or still going
+	// test cases with different jail durations
 	testcases := []struct {
 		jailedUntil time.Time
 		expUpdate   bool
@@ -436,11 +439,23 @@ func (suite *KeeperTestSuite) TestAfterValidatorDowntimeHook() {
 	},
 	}
 
-	// synchronize the block time with the test cases
+	// set validator signing info to not panic
 	ctx = ctx.WithBlockTime(now)
 	valInfo := slashingtypes.NewValidatorSigningInfo(consAddr, int64(1), int64(1),
 		time.Time{}.UTC(), false, int64(0))
+	app.SlashingKeeper.SetValidatorSigningInfo(ctx, consAddr, valInfo)
+
+	// expect no updates when there is no unbonding packets
+	app.ChildKeeper.AfterValidatorDowntime(ctx, consAddr, int64(1))
+	signInfo, _ := app.SlashingKeeper.GetValidatorSigningInfo(ctx, consAddr)
+
+	suite.Require().True(false == !(signInfo.JailedUntil.Equal(signInfo.JailedUntil)))
 
+	// set unbounding packet with valset update id
+	vscPacket := ccv.ValidatorSetChangePacketData{ValsetUpdateId: uint64(3)}
+	app.ChildKeeper.SetUnbondingPacket(ctx, uint64(0), channeltypes.Packet{Data: vscPacket.GetBytes()})
+
+	// test cases
 	for _, tc := range testcases {
 		// set the current unjailing time
 		valInfo.JailedUntil = tc.jailedUntil
@@ -461,8 +476,17 @@ func (suite *KeeperTestSuite) TestGetLastUnboundingPacket() {
 	app := suite.childChain.App.(*app.App)
 	ctx := suite.childChain.GetContext()
 
-	ubdPacket := app.ChildKeeper.GetLastUnbondingPacket(ctx)
-	suite.Require().Zero(ubdPacket.ValsetUpdateId)
+	// check if IBC packet is valid
+	_, err := app.ChildKeeper.GetLastUnbondingPacketData(ctx)
+	suite.NotNil(err)
+
+	app.ChildKeeper.SetUnbondingPacket(ctx, uint64(0), channeltypes.Packet{Sequence: 1})
+
+	// check if unbouding packet data is valid
+	_, err = app.ChildKeeper.GetLastUnbondingPacketData(ctx)
+	suite.NotNil(err)
+
+	// check if the last packet stored is returned
 	for i := 0; i < 5; i++ {
 		pd := ccv.NewValidatorSetChangePacketData(
 			[]abci.ValidatorUpdate{},
@@ -473,6 +497,7 @@ func (suite *KeeperTestSuite) TestGetLastUnboundingPacket() {
 		app.ChildKeeper.SetUnbondingPacket(ctx, uint64(i), packet)
 	}
 
-	ubdPacket = app.ChildKeeper.GetLastUnbondingPacket(ctx)
+	ubdPacket, err := app.ChildKeeper.GetLastUnbondingPacketData(ctx)
+	suite.Nil(err)
 	suite.Require().Equal(uint64(4), ubdPacket.ValsetUpdateId)
 }

x/ccv/child/keeper/relay.go

@@ -42,7 +42,7 @@ func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet, newCha
 	} else {
 		pendingChanges = utils.AccumulateChanges(currentChanges.ValidatorUpdates, newChanges.ValidatorUpdates)
 	}
-	k.SetPendingChanges(ctx, ccv.ValidatorSetChangePacketData{ValidatorUpdates: pendingChanges})
+	k.SetPendingChanges(ctx, ccv.ValidatorSetChangePacketData{ValidatorUpdates: pendingChanges, ValsetUpdateId: newChanges.ValsetUpdateId})
 
 	// Save unbonding time and packet
 	unbondingTime := ctx.BlockTime().Add(types.UnbondingTime)
@@ -92,7 +92,7 @@ func (k Keeper) UnbondMaturePackets(ctx sdk.Context) error {
 
 // SendPacket sends a packet that initiates the given validator
 // slashing and jailing on the provider chain.
-func (k Keeper) SendPacket(ctx sdk.Context, val abci.Validator, slashFraction, jailedUntil int64) error {
+func (k Keeper) SendPacket(ctx sdk.Context, val abci.Validator, valsetUpdateID uint64, slashFraction, jailedUntil int64) error {
 
 	// check the setup
 	channelID, ok := k.GetParentChannel(ctx)
@@ -117,13 +117,7 @@ func (k Keeper) SendPacket(ctx sdk.Context, val abci.Validator, slashFraction, j
 		)
 	}
 
-	// add the last ValsetUpdateId to the packet data so that the provider
-	// can find the block height when the downtime happened
-	valsetUpdateId := k.GetLastUnbondingPacket(ctx).ValsetUpdateId
-	if valsetUpdateId == 0 {
-		return sdkerrors.Wrapf(ccv.ErrInvalidChildState, "last valset update id not set")
-	}
-	packetData := ccv.NewValidatorDowtimePacketData(val, valsetUpdateId, slashFraction, jailedUntil)
+	packetData := ccv.NewValidatorDowntimePacketData(val, valsetUpdateID, slashFraction, jailedUntil)
 	packetDataBytes := packetData.GetBytes()
 
 	// send ValidatorDowntime infractions in IBC packet
@@ -140,6 +134,9 @@ func (k Keeper) SendPacket(ctx sdk.Context, val abci.Validator, slashFraction, j
 	return nil
 }
 
-func (k Keeper) OnAcknowledgementPacket(ctx sdk.Context, packet channeltypes.Packet, ack channeltypes.Acknowledgement) error {
+func (k Keeper) OnAcknowledgementPacket(ctx sdk.Context, packet channeltypes.Packet, data ccv.ValidatorDowntimePacketData, ack channeltypes.Acknowledgement) error {
+	if err := ack.GetError(); err != "" {
+		return fmt.Errorf(err)
+	}
 	return nil
 }

x/ccv/child/keeper/relay_test.go

@@ -14,6 +14,7 @@ import (
 	"github.com/cosmos/interchain-security/x/ccv/types"
 	ccv "github.com/cosmos/interchain-security/x/ccv/types"
 	abci "github.com/tendermint/tendermint/abci/types"
+	"github.com/tendermint/tendermint/libs/bytes"
 )
 
 func (suite *KeeperTestSuite) TestOnRecvPacket() {
@@ -239,3 +240,23 @@ func (suite *KeeperTestSuite) TestUnbondMaturePackets() {
 	suite.Require().Nil(ackBytes3, "acknowledgement written for unbonding packet 3")
 
 }
+
+func (suite *KeeperTestSuite) TestOnAcknowledgement() {
+	packetData := types.NewValidatorDowntimePacketData(
+		abci.Validator{Address: bytes.HexBytes{}, Power: int64(1)}, uint64(1), int64(4), int64(1),
+	)
+
+	packet := channeltypes.NewPacket(packetData.GetBytes(), 1, parenttypes.PortID, suite.path.EndpointB.ChannelID,
+		childtypes.PortID, suite.path.EndpointA.ChannelID, clienttypes.Height{}, uint64(time.Now().Add(60*time.Second).UnixNano()))
+	ack := channeltypes.NewResultAcknowledgement([]byte{1})
+
+	// expect no error
+	err := suite.childChain.App.(*app.App).ChildKeeper.OnAcknowledgementPacket(suite.ctx, packet, packetData, ack)
+	suite.Nil(err)
+
+	// expect an error
+	ack = channeltypes.NewErrorAcknowledgement("error")
+
+	err = suite.childChain.App.(*app.App).ChildKeeper.OnAcknowledgementPacket(suite.ctx, packet, packetData, ack)
+	suite.NotNil(err)
+}

x/ccv/child/module.go

@@ -350,16 +350,49 @@ func (am AppModule) OnRecvPacket(
 // OnAcknowledgementPacket implements the IBCModule interface
 func (am AppModule) OnAcknowledgementPacket(
 	ctx sdk.Context,
-	_ channeltypes.Packet,
+	packet channeltypes.Packet,
 	acknowledgement []byte,
 	_ sdk.AccAddress,
 ) (*sdk.Result, error) {
 	var ack channeltypes.Acknowledgement
 	if err := ccv.ModuleCdc.UnmarshalJSON(acknowledgement, &ack); err != nil {
-		fmt.Println(err)
-		return nil, sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "cannot unmarshal parent packet acknowledgement: %v", err)
+		return nil, sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "cannot unmarshal child packet acknowledgement: %v", err)
+	}
+	var data ccv.ValidatorDowntimePacketData
+	if err := ccv.ModuleCdc.UnmarshalJSON(packet.GetData(), &data); err != nil {
+		return nil, sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "cannot unmarshal child packet data: %s", err.Error())
+	}
+
+	if err := am.keeper.OnAcknowledgementPacket(ctx, packet, data, ack); err != nil {
+		return nil, err
+	}
+
+	ctx.EventManager().EmitEvent(
+		sdk.NewEvent(
+			ccv.EventTypePacket,
+			sdk.NewAttribute(sdk.AttributeKeyModule, types.ModuleName),
+			sdk.NewAttribute(ccv.AttributeKeyAck, ack.String()),
+		),
+	)
+	switch resp := ack.Response.(type) {
+	case *channeltypes.Acknowledgement_Result:
+		ctx.EventManager().EmitEvent(
+			sdk.NewEvent(
+				ccv.EventTypePacket,
+				sdk.NewAttribute(ccv.AttributeKeyAckSuccess, string(resp.Result)),
+			),
+		)
+	case *channeltypes.Acknowledgement_Error:
+		ctx.EventManager().EmitEvent(
+			sdk.NewEvent(
+				ccv.EventTypePacket,
+				sdk.NewAttribute(ccv.AttributeKeyAckError, resp.Error),
+			),
+		)
 	}
-	return nil, sdkerrors.Wrap(ccv.ErrInvalidChannelFlow, "cannot receive acknowledgement on child port, child chain does not send packet over channel.")
+	return &sdk.Result{
+		Events: ctx.EventManager().Events().ToABCIEvents(),
+	}, nil
 }
 
 // OnTimeoutPacket implements the IBCModule interface

x/ccv/parent/keeper/relay.go

@@ -126,9 +126,9 @@ func (k Keeper) EndBlockCallback(ctx sdk.Context) {
 
 // OnRecvPacket slahes and jails the given validator in the packet data
 func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet, data ccv.ValidatorDowntimePacketData) exported.Acknowledgement {
-	if childChannel, ok := k.GetChannelToChain(ctx, packet.DestinationChannel); !ok && childChannel != packet.DestinationChannel {
+	if chainID, ok := k.GetChannelToChain(ctx, packet.DestinationChannel); !ok {
 		ack := channeltypes.NewErrorAcknowledgement(
-			fmt.Sprintf("packet sent on a channel %s other than the established child channel %s", packet.DestinationChannel, childChannel),
+			fmt.Sprintf("chain ID doesn't exist for channel ID: %s", chainID),
 		)
 		chanCap, _ := k.scopedKeeper.GetCapability(ctx, host.ChannelCapabilityPath(packet.DestinationPort, packet.DestinationChannel))
 		k.channelKeeper.ChanCloseInit(ctx, packet.DestinationPort, packet.DestinationChannel, chanCap)
@@ -141,7 +141,8 @@ func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet, data c
 		return &ack
 	}
 
-	return nil
+	ack := channeltypes.NewResultAcknowledgement([]byte{byte(1)})
+	return ack
 }
 
 // HandleConsumerDowntime gets the validator and the downtime infraction height from the packet data
@@ -151,11 +152,17 @@ func (k Keeper) HandleConsumerDowntime(ctx sdk.Context, downtimeData ccv.Validat
 	consAddr := sdk.ConsAddress(downtimeData.Validator.Address)
 
 	// get the validator data
-	val, found := k.stakingKeeper.GetValidatorByConsAddr(ctx, consAddr)
+	validator, found := k.stakingKeeper.GetValidatorByConsAddr(ctx, consAddr)
 	if !found {
 		return fmt.Errorf("cannot find validator with address %s", consAddr.String())
 	}
 
+	// make sure the validator is not yet unbonded;
+	// stakingKeeper.Slash() panics otherwise
+	if validator.IsUnbonded() {
+		return fmt.Errorf("should not be slashing unbonded validator: %s", validator.GetOperator())
+	}
+
 	// get the downtime block height from the valsetUpdateID
 	blockHeight := k.GetValsetUpdateBlockHeight(ctx, downtimeData.ValsetUpdateId)
 	if blockHeight == 0 {
@@ -164,10 +171,10 @@ func (k Keeper) HandleConsumerDowntime(ctx sdk.Context, downtimeData ccv.Validat
 
 	// slash and jail the validator
 	k.stakingKeeper.Slash(ctx, consAddr, int64(blockHeight), downtimeData.Validator.Power, sdk.NewDec(1).QuoInt64(downtimeData.SlashFraction))
-	if !val.Jailed {
+	if !validator.IsJailed() {
 		k.stakingKeeper.Jail(ctx, consAddr)
 	}
-	// TODO: check if the missed block bits and sign info need to be reseted
+
 	k.slashingKeeper.JailUntil(ctx, consAddr, ctx.BlockHeader().Time.Add(time.Duration(downtimeData.JailTime)))
 
 	return nil

x/ccv/parent/module.go

@@ -340,8 +340,6 @@ func (am AppModule) OnRecvPacket(
 		),
 	)
 
-	// NOTE: In successful case, acknowledgement will be written asynchronously
-	// after unbonding period has elapsed.
 	return ack
 }