Fix: Patch Consumer initiated slashing cosmos#28

- Increment `ValsetUpdateID` once per provider chain block; fix the impacted unit-tests

- Use a reverse iterator to get last  the `ValsetUpdateID` on consumer chains

- Check that slashing packets are sent in the slashing hook unit-tests

- Add acknowledgement boilerplates to the provider chain

- Allow slashing with 0 jail time

- Change the consumer `SendPacket` API to `SendPenalties`

TODO: slashing round-trip tests require a [custom IBC-GO](https://github.com/sainoe/ibc-go/tree/sainoe/valset-update-fixes). This will be sorted out after the cosmos#45 merge.

.gitignore

@@ -1,3 +1,4 @@
+.DS_STORE
 vue/node_modules
 vue/dist
 release/

go.mod

@@ -11,15 +11,13 @@ require (
 	github.com/google/go-cmp v0.5.7 // indirect
 	github.com/gorilla/mux v1.8.0
 	github.com/grpc-ecosystem/grpc-gateway v1.16.0
-	github.com/grpc-ecosystem/grpc-gateway/v2 v2.7.3 // indirect
 	github.com/improbable-eng/grpc-web v0.14.1 // indirect
 	github.com/jhump/protoreflect v1.9.0 // indirect
 	github.com/lib/pq v1.10.2 // indirect
 	github.com/mattn/go-isatty v0.0.14 // indirect
 	github.com/onsi/ginkgo v1.16.4 // indirect
 	github.com/onsi/gomega v1.13.0 // indirect
 	github.com/prometheus/common v0.30.0 // indirect
-	github.com/regen-network/cosmos-proto v0.3.1 // indirect
 	github.com/rs/zerolog v1.25.0 // indirect
 	github.com/spf13/cast v1.4.1
 	github.com/spf13/cobra v1.2.1
@@ -30,13 +28,13 @@ require (
 	golang.org/x/crypto v0.0.0-20210513164829-c07d793c2f9a // indirect
 	google.golang.org/genproto v0.0.0-20220118154757-00ab72f36ad5
 	google.golang.org/grpc v1.43.0
-	google.golang.org/grpc/cmd/protoc-gen-go-grpc v1.0.0 // indirect
 	google.golang.org/protobuf v1.27.1
 )
 
 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.20220218173609-8bbfac63fdd7
 	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
 )

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)
@@ -32,15 +38,18 @@ func (k Keeper) AfterValidatorDowntime(ctx sdk.Context, consAddr sdk.ConsAddress
 	k.slashingKeeper.SetValidatorSigningInfo(ctx, consAddr, signInfo)
 
 	// send packet to initiate slashing on the provider chain
-	k.SendPacket(
+	err = k.SendPenalties(
 		ctx,
 		abci.Validator{
 			Address: consAddr.Bytes(),
 			Power:   power,
 		},
+		valsetUpdate.ValsetUpdateId,
 		k.slashingKeeper.SlashFractionDowntime(ctx).TruncateInt64(),
 		signInfo.JailedUntil.UnixNano(),
 	)
+
+	panic(err)
 }
 
 // Hooks wrapper struct for ChildKeeper

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

@@ -372,8 +372,13 @@ func TestKeeperTestSuite(t *testing.T) {
 // are registred and triggered when a validator has downtime
 func (suite *KeeperTestSuite) TestValidatorDowntime() {
 	// initial setup
-	app := suite.childChain.App.(*app.App)
-	ctx := suite.childChain.GetContext()
+	suite.SetupCCVChannel()
+	app, ctx := suite.childChain.App.(*app.App), suite.ctx
+	channelID := suite.path.EndpointA.ChannelID
+
+	// add parent channel to childkeeper store
+	app.ChildKeeper.SetParentChannel(ctx, channelID)
+	app.IBCKeeper.ChannelKeeper.SetChannel(ctx, types.PortID, channelID, suite.path.EndpointB.GetChannel())
 
 	// create a validator pubkey and address
 	pubkey := ed25519.GenPrivKey().PubKey()
@@ -382,10 +387,18 @@ 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)
 
+	// save next sequence before sending slashing packet
+	seq, ok := app.GetIBCKeeper().ChannelKeeper.GetNextSequenceSend(ctx, types.PortID, suite.path.EndpointA.ChannelID)
+	suite.Require().True(ok)
+
 	// Sign 1000 blocks
 	valPower := int64(1)
 	height := int64(0)
@@ -399,70 +412,109 @@ func (suite *KeeperTestSuite) TestValidatorDowntime() {
 		app.SlashingKeeper.HandleValidatorSignature(ctx, pubkey.Address().Bytes(), valPower, false)
 	}
 
+	// check that the validator signing info are correctly updated
 	signInfo, found := app.SlashingKeeper.GetValidatorSigningInfo(ctx, consAddr)
 	suite.Require().True(found)
-	suite.Require().NotZero(signInfo.JailedUntil)
-	suite.Require().Zero(signInfo.MissedBlocksCounter)
+	suite.Require().NotZero(signInfo.JailedUntil, "did not update validator unjail until")
+	suite.Require().Zero(signInfo.MissedBlocksCounter, "did not reset validator missed block counter")
 	suite.Require().Zero(signInfo.IndexOffset)
 	app.SlashingKeeper.IterateValidatorMissedBlockBitArray(ctx, consAddr, func(_ int64, missed bool) bool {
 		suite.Require().False(missed)
 		return false
 	})
+
+	// verify that the slashing packet was sent
+	commit := app.IBCKeeper.ChannelKeeper.GetPacketCommitment(ctx, types.PortID, channelID, seq)
+	suite.Require().NotNil(commit, "did not found slashing packet commitment")
 }
 
 // TestAfterValidatorDowntimeHook tests the slashing hook implementation logic
 func (suite *KeeperTestSuite) TestAfterValidatorDowntimeHook() {
-	app := suite.childChain.App.(*app.App)
-	ctx := suite.ctx
+	// initial setup
+	suite.SetupCCVChannel()
+
+	app, ctx := suite.childChain.App.(*app.App), suite.ctx
+	channelID := suite.path.EndpointA.ChannelID
+	app.ChildKeeper.SetParentChannel(ctx, channelID)
+	app.IBCKeeper.ChannelKeeper.SetChannel(ctx, types.PortID, channelID, suite.path.EndpointB.GetChannel())
 
 	consAddr := sdk.ConsAddress(ed25519.GenPrivKey().PubKey().Bytes()).Bytes()
 
-	now := time.Now()
+	// set initial validator signing info
+	signInfo := slashingtypes.NewValidatorSigningInfo(consAddr, int64(1), int64(1),
+		time.Time{}.UTC(), false, int64(0))
+	app.SlashingKeeper.SetValidatorSigningInfo(ctx, consAddr, signInfo)
+
+	// store sequence
+	seq, ok := app.GetIBCKeeper().ChannelKeeper.GetNextSequenceSend(ctx, types.PortID, suite.path.EndpointA.ChannelID)
+	suite.Require().True(ok)
+
+	// expect no updates when no unbonding packets exist
+	app.ChildKeeper.AfterValidatorDowntime(ctx, consAddr, int64(1))
+	newSignInfo, _ := app.SlashingKeeper.GetValidatorSigningInfo(ctx, consAddr)
 
-	// Cover the cases when the validatir jailing duration
-	// is either elapsed, null or still going
+	suite.Require().True(signInfo.JailedUntil.Equal(newSignInfo.JailedUntil), "updated signing info when no unbonding packets exist")
+
+	// check that no slashing packet was sent
+	commit := app.IBCKeeper.ChannelKeeper.GetPacketCommitment(ctx, types.PortID, channelID, seq)
+	suite.Require().Nil(commit, "sent slashing packet when no unbonding packets exist")
+
+	// set unbounding packet with valset update id
+	vscPacket := ccv.ValidatorSetChangePacketData{ValsetUpdateId: uint64(3)}
+	app.ChildKeeper.SetUnbondingPacket(ctx, uint64(0), channeltypes.Packet{Data: vscPacket.GetBytes()})
+
+	blockTime := suite.childChain.GetContext().BlockTime()
+
+	// test cases with jailing times being zero, elapsed and pending
 	testcases := []struct {
 		jailedUntil time.Time
 		expUpdate   bool
-	}{{
-		jailedUntil: now.Add(-1 * time.Hour),
-		expUpdate:   true,
-	}, {
-		jailedUntil: time.Time{}, // null
-		expUpdate:   true,
-	}, {
-		jailedUntil: now.Add(1 * time.Hour),
-		expUpdate:   false,
-	},
+	}{
+		{
+			jailedUntil: time.Time{},
+			expUpdate:   true,
+		}, {
+			jailedUntil: blockTime.Add(-1 * time.Hour),
+			expUpdate:   true,
+		}, {
+			jailedUntil: blockTime.Add(1 * time.Hour),
+			expUpdate:   false,
+		},
 	}
 
-	// synchronize the block time with the test cases
-	ctx = ctx.WithBlockTime(now)
-	valInfo := slashingtypes.NewValidatorSigningInfo(consAddr, int64(1), int64(1),
-		time.Time{}.UTC(), false, int64(0))
-
 	for _, tc := range testcases {
-		// set the current unjailing time
-		valInfo.JailedUntil = tc.jailedUntil
-		app.SlashingKeeper.SetValidatorSigningInfo(ctx, consAddr, valInfo)
-		// execute the hook logic
-		app.ChildKeeper.AfterValidatorDowntime(
-			ctx, consAddr, int64(1))
-		// verify if we have the expected output
-		signInfo, found := app.SlashingKeeper.GetValidatorSigningInfo(ctx, consAddr)
-		suite.Require().True(found)
-		suite.Require().True(tc.expUpdate == !(signInfo.JailedUntil.Equal(tc.jailedUntil)))
+		// set test case signing info
+		signInfo = slashingtypes.NewValidatorSigningInfo(consAddr, int64(1), int64(1), tc.jailedUntil, false, int64(0))
+		app.SlashingKeeper.SetValidatorSigningInfo(ctx, consAddr, signInfo)
+		// save current sequence
+		seq, _ = app.GetIBCKeeper().ChannelKeeper.GetNextSequenceSend(ctx, types.PortID, suite.path.EndpointA.ChannelID)
+		// execute hook logic
+		app.ChildKeeper.AfterValidatorDowntime(ctx, consAddr, int64(1))
+		// check signing info state
+		newSignInfo, _ = app.SlashingKeeper.GetValidatorSigningInfo(ctx, consAddr)
+		suite.Require().True(tc.expUpdate == !(signInfo.JailedUntil.Equal(newSignInfo.JailedUntil)))
+
+		// check that slashing packet was sent only if expected
+		commit = app.IBCKeeper.ChannelKeeper.GetPacketCommitment(ctx, types.PortID, channelID, seq)
+		suite.Require().Equal(tc.expUpdate, commit != nil)
 	}
-
-	suite.Require()
 }
 
 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 +525,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)
@@ -90,9 +90,8 @@ func (k Keeper) UnbondMaturePackets(ctx sdk.Context) error {
 	return nil
 }
 
-// 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 {
+// SendPenalty sends a given validator slashing and jailing penalties
+func (k Keeper) SendPenalties(ctx sdk.Context, validator abci.Validator, valsetUpdateID uint64, slashFraction, jailedUntil int64) error {
 
 	// check the setup
 	channelID, ok := k.GetParentChannel(ctx)
@@ -117,13 +116,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(validator, valsetUpdateID, slashFraction, jailedUntil)
 	packetDataBytes := packetData.GetBytes()
 
 	// send ValidatorDowntime infractions in IBC packet
@@ -140,6 +133,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)
+}