Merge branch 'main' into 720-fix-event-emit

.github/workflows/proto.yml

@@ -10,16 +10,6 @@ permissions:
   contents: read
 
 jobs:
-  lint:
-    runs-on: ubuntu-latest
-    timeout-minutes: 5
-    steps:
-      - uses: actions/checkout@v3
-      - uses: bufbuild/[email protected]
-      - uses: bufbuild/buf-lint-action@v1
-        with:
-          input: "proto"
-
   break-check:
     runs-on: ubuntu-latest
     steps:

CHANGELOG.md

@@ -10,8 +10,9 @@ Add an entry to the unreleased section whenever merging a PR to main that is not
 
 Date: June 21st, 2023
 
-Interchain Security v3 uses SDK 0.47 and IBC 7. 
+Interchain Security v3 uses SDK 0.47 and IBC 7.
 
+* (fix) [#1093](https://github.com/cosmos/interchain-security/pull/1093) Make SlashPacketData backward compatible when sending data over the wire 
 * (deps) [#1019](https://github.com/cosmos/interchain-security/pull/1019) Bump multiple dependencies. 
   * Bump [cosmos-sdk](https://github.com/cosmos/cosmos-sdk) to [v0.47.3](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.47.3).
   * Bump [ibc-go](https://github.com/cosmos/ibc-go) to [v7.1.0](https://github.com/cosmos/ibc-go/releases/tag/v7.1.0).

go.mod

@@ -28,7 +28,7 @@ require (
 	golang.org/x/sys v0.7.0 // indirect
 	google.golang.org/genproto v0.0.0-20230410155749-daa745c078e1
 	google.golang.org/grpc v1.56.1
-	google.golang.org/protobuf v1.30.0
+	google.golang.org/protobuf v1.31.0
 	gopkg.in/yaml.v2 v2.4.0
 )
 

go.sum

@@ -1808,8 +1808,8 @@ google.golang.org/protobuf v1.26.0/go.mod h1:9q0QmTI4eRPtz6boOQmLYwt+qCgq0jsYwAQ
 google.golang.org/protobuf v1.27.1/go.mod h1:9q0QmTI4eRPtz6boOQmLYwt+qCgq0jsYwAQnmE0givc=
 google.golang.org/protobuf v1.28.0/go.mod h1:HV8QOd/L58Z+nl8r43ehVNZIU/HEI6OcFqwMG9pJV4I=
 google.golang.org/protobuf v1.28.1/go.mod h1:HV8QOd/L58Z+nl8r43ehVNZIU/HEI6OcFqwMG9pJV4I=
-google.golang.org/protobuf v1.30.0 h1:kPPoIgf3TsEvrm0PFe15JQ+570QVxYzEvvHqChK+cng=
-google.golang.org/protobuf v1.30.0/go.mod h1:HV8QOd/L58Z+nl8r43ehVNZIU/HEI6OcFqwMG9pJV4I=
+google.golang.org/protobuf v1.31.0 h1:g0LDEJHgrBl9N9r17Ru3sqWhkIx2NB67okBHPwC7hs8=
+google.golang.org/protobuf v1.31.0/go.mod h1:HV8QOd/L58Z+nl8r43ehVNZIU/HEI6OcFqwMG9pJV4I=
 gopkg.in/alecthomas/kingpin.v2 v2.2.6/go.mod h1:FMv+mEhP44yOT+4EoQTLFTRgOQ1FBLkstjWtayDeSgw=
 gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
 gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=

proto/interchain_security/ccv/v1/ccv.proto

@@ -84,4 +84,42 @@ enum ConsumerPacketDataType {
   // VSCMatured packet
   CONSUMER_PACKET_TYPE_VSCM = 2
       [ (gogoproto.enumvalue_customname) = "VscMaturedPacket" ];
-}
+}
+
+// ConsumerPacketData contains a consumer packet data and a type tag
+// that is compatible with ICS v1 and v2 over the wire. It is not used for internal storage.
+message ConsumerPacketDataV1 {
+  ConsumerPacketDataType type = 1;
+
+  oneof data {
+    SlashPacketDataV1 slashPacketData = 2;
+    VSCMaturedPacketData vscMaturedPacketData = 3;
+  }
+}
+
+// This packet is sent from the consumer chain to the provider chain
+// It is backward compatible with the ICS v1 and v2 version of the packet.
+message SlashPacketDataV1 {
+  tendermint.abci.Validator validator = 1 [
+    (gogoproto.nullable) = false,
+    (gogoproto.moretags) = "yaml:\"validator\""
+  ];
+  // map to the infraction block height on the provider
+  uint64 valset_update_id = 2;
+  // tell if the slashing is for a downtime or a double-signing infraction
+  InfractionType infraction = 3;
+}
+
+// InfractionType indicates the infraction type a validator commited.
+// NOTE: ccv.InfractionType to maintain compatibility between ICS versions
+// using different versions of the cosmos-sdk and ibc-go modules.
+enum InfractionType {
+  option (gogoproto.goproto_enum_prefix) = false;
+
+  // UNSPECIFIED defines an empty infraction type.
+  INFRACTION_TYPE_UNSPECIFIED = 0 [(gogoproto.enumvalue_customname) = "InfractionEmpty"];
+  // DOUBLE_SIGN defines a validator that double-signs a block.
+  INFRACTION_TYPE_DOUBLE_SIGN = 1 [(gogoproto.enumvalue_customname) = "DoubleSign"];
+  // DOWNTIME defines a validator that missed signing too many blocks.
+  INFRACTION_TYPE_DOWNTIME = 2 [(gogoproto.enumvalue_customname) = "Downtime"];
+}

tests/integration/slashing.go

@@ -255,7 +255,7 @@ func (s *CCVTestSuite) TestSlashPacketAcknowledgement() {
 	err := consumerKeeper.OnAcknowledgementPacket(s.consumerCtx(), packet, channeltypes.NewResultAcknowledgement(ack.Acknowledgement()))
 	s.Require().NoError(err)
 
-	err = consumerKeeper.OnAcknowledgementPacket(s.consumerCtx(), packet, channeltypes.NewErrorAcknowledgement(fmt.Errorf("another error")))
+	err = consumerKeeper.OnAcknowledgementPacket(s.consumerCtx(), packet, ccv.NewErrorAcknowledgementWithLog(s.consumerCtx(), fmt.Errorf("another error")))
 	s.Require().Error(err)
 }
 
@@ -330,7 +330,8 @@ func (suite *CCVTestSuite) TestOnRecvSlashPacketErrors() {
 	errAck := providerKeeper.OnRecvSlashPacket(ctx, packet, packetData)
 	suite.Require().False(errAck.Success())
 	errAckCast := errAck.(channeltypes.Acknowledgement)
-	// TODO: see if there's a way to get error reason like before
+	// Error strings in err acks are now thrown out by IBC core to prevent app hash.
+	// Hence a generic error string is expected.
 	suite.Require().Equal("ABCI code: 1: error handling packet: see events for details", errAckCast.GetError())
 
 	// Restore init chain height
@@ -341,7 +342,6 @@ func (suite *CCVTestSuite) TestOnRecvSlashPacketErrors() {
 	errAck = providerKeeper.OnRecvSlashPacket(ctx, packet, packetData)
 	suite.Require().False(errAck.Success())
 	errAckCast = errAck.(channeltypes.Acknowledgement)
-	// TODO: see if there's a way to get error reason like before
 	suite.Require().Equal("ABCI code: 1: error handling packet: see events for details", errAckCast.GetError())
 
 	// save current VSC ID
@@ -357,7 +357,6 @@ func (suite *CCVTestSuite) TestOnRecvSlashPacketErrors() {
 	errAck = providerKeeper.OnRecvSlashPacket(ctx, packet, packetData)
 	suite.Require().False(errAck.Success())
 	errAckCast = errAck.(channeltypes.Acknowledgement)
-	// TODO: see if there's a way to get error reason like before
 	suite.Require().Equal("ABCI code: 1: error handling packet: see events for details", errAckCast.GetError())
 
 	// construct slashing packet with non existing validator

tests/integration/throttle.go

@@ -9,6 +9,7 @@ import (
 	channeltypes "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types"
 
 	icstestingutils "github.com/cosmos/interchain-security/v3/testutil/ibc_testing"
+	"github.com/cosmos/interchain-security/v3/x/ccv/provider"
 	providertypes "github.com/cosmos/interchain-security/v3/x/ccv/provider/types"
 	ccvtypes "github.com/cosmos/interchain-security/v3/x/ccv/types"
 )
@@ -314,8 +315,8 @@ func (s *CCVTestSuite) TestPacketSpam() {
 
 	// Recv 500 packets from consumer to provider in same block
 	for _, packet := range packets {
-		consumerPacketData := ccvtypes.ConsumerPacketData{}
-		ccvtypes.ModuleCdc.MustUnmarshalJSON(packet.GetData(), &consumerPacketData)
+		consumerPacketData, err := provider.UnmarshalConsumerPacket(packet) // Same func used by provider's OnRecvPacket
+		s.Require().NoError(err)
 		providerKeeper.OnRecvSlashPacket(s.providerCtx(), packet, *consumerPacketData.GetSlashPacketData())
 	}
 
@@ -368,8 +369,8 @@ func (s *CCVTestSuite) TestDoubleSignDoesNotAffectThrottling() {
 
 	// Recv 500 packets from consumer to provider in same block
 	for _, packet := range packets {
-		consumerPacketData := ccvtypes.ConsumerPacketData{}
-		ccvtypes.ModuleCdc.MustUnmarshalJSON(packet.GetData(), &consumerPacketData)
+		consumerPacketData, err := provider.UnmarshalConsumerPacket(packet) // Same func used by provider's OnRecvPacket
+		s.Require().NoError(err)
 		providerKeeper.OnRecvSlashPacket(s.providerCtx(), packet, *consumerPacketData.GetSlashPacketData())
 	}
 
@@ -464,8 +465,10 @@ func (s *CCVTestSuite) TestQueueOrdering() {
 
 	// Recv 500 packets from consumer to provider in same block
 	for i, packet := range packets {
-		consumerPacketData := ccvtypes.ConsumerPacketData{}
-		ccvtypes.ModuleCdc.MustUnmarshalJSON(packet.GetData(), &consumerPacketData)
+
+		consumerPacketData, err := provider.UnmarshalConsumerPacket(packet) // Same func used by provider's OnRecvPacket
+		s.Require().NoError(err)
+
 		// Type depends on index packets were appended from above
 		if (i+5)%10 == 0 {
 			vscMaturedPacketData := consumerPacketData.GetVscMaturedPacketData()
@@ -678,8 +681,8 @@ func (s *CCVTestSuite) TestSlashSameValidator() {
 
 	// Recv and queue all slash packets.
 	for _, packet := range packets {
-		consumerPacketData := ccvtypes.ConsumerPacketData{}
-		ccvtypes.ModuleCdc.MustUnmarshalJSON(packet.GetData(), &consumerPacketData)
+		consumerPacketData, err := provider.UnmarshalConsumerPacket(packet) // Same func used by provider's OnRecvPacket
+		s.Require().NoError(err)
 		providerKeeper.OnRecvSlashPacket(s.providerCtx(), packet, *consumerPacketData.GetSlashPacketData())
 	}
 
@@ -739,8 +742,8 @@ func (s CCVTestSuite) TestSlashAllValidators() { //nolint:govet // this is a tes
 
 	// Recv and queue all slash packets.
 	for _, packet := range packets {
-		consumerPacketData := ccvtypes.ConsumerPacketData{}
-		ccvtypes.ModuleCdc.MustUnmarshalJSON(packet.GetData(), &consumerPacketData)
+		consumerPacketData, err := provider.UnmarshalConsumerPacket(packet) // Same func used by provider's OnRecvPacket
+		s.Require().NoError(err)
 		providerKeeper.OnRecvSlashPacket(s.providerCtx(), packet, *consumerPacketData.GetSlashPacketData())
 	}
 
@@ -786,10 +789,9 @@ func (s *CCVTestSuite) TestLeadingVSCMaturedAreDequeued() {
 			ibcSeqNum := uint64(i)
 			packet := s.constructSlashPacketFromConsumer(*bundle,
 				*s.providerChain.Vals.Validators[0], stakingtypes.Infraction_INFRACTION_DOWNTIME, ibcSeqNum)
-			packetData := ccvtypes.ConsumerPacketData{}
-			ccvtypes.ModuleCdc.MustUnmarshalJSON(packet.GetData(), &packetData)
-			providerKeeper.OnRecvSlashPacket(s.providerCtx(),
-				packet, *packetData.GetSlashPacketData())
+			consumerPacketData, err := provider.UnmarshalConsumerPacket(packet) // Same func used by provider's OnRecvPacket
+			s.Require().NoError(err)
+			providerKeeper.OnRecvSlashPacket(s.providerCtx(), packet, *consumerPacketData.GetSlashPacketData())
 		}
 	}
 
@@ -877,10 +879,9 @@ func (s *CCVTestSuite) TestVscMaturedHandledPerBlockLimit() {
 			ibcSeqNum := uint64(i)
 			packet := s.constructSlashPacketFromConsumer(*bundle,
 				*s.providerChain.Vals.Validators[0], stakingtypes.Infraction_INFRACTION_DOWNTIME, ibcSeqNum)
-			packetData := ccvtypes.ConsumerPacketData{}
-			ccvtypes.ModuleCdc.MustUnmarshalJSON(packet.GetData(), &packetData)
-			providerKeeper.OnRecvSlashPacket(s.providerCtx(),
-				packet, *packetData.GetSlashPacketData())
+			consumerPacketData, err := provider.UnmarshalConsumerPacket(packet) // Same func used by provider's OnRecvPacket
+			s.Require().NoError(err)
+			providerKeeper.OnRecvSlashPacket(s.providerCtx(), packet, *consumerPacketData.GetSlashPacketData())
 		}
 	}
 

x/ccv/consumer/ibc_module.go

@@ -231,7 +231,7 @@ func (am AppModule) OnRecvPacket(
 		data types.ValidatorSetChangePacketData
 	)
 	if err := types.ModuleCdc.UnmarshalJSON(packet.GetData(), &data); err != nil {
-		errAck := channeltypes.NewErrorAcknowledgement(fmt.Errorf("cannot unmarshal CCV packet data"))
+		errAck := types.NewErrorAcknowledgementWithLog(ctx, fmt.Errorf("cannot unmarshal CCV packet data"))
 		ack = &errAck
 	} else {
 		ack = am.keeper.OnRecvVSCPacket(ctx, packet, data)

x/ccv/consumer/keeper/relay_test.go

@@ -279,7 +279,7 @@ func TestOnAcknowledgementPacket(t *testing.T) {
 		).Return(nil).Times(1),
 	)
 
-	ack = channeltypes.NewErrorAcknowledgement(fmt.Errorf("error"))
+	ack = types.NewErrorAcknowledgementWithLog(ctx, fmt.Errorf("error"))
 	err = consumerKeeper.OnAcknowledgementPacket(ctx, packet, ack)
 	require.Nil(t, err)
 }

x/ccv/provider/ibc_module.go

@@ -174,29 +174,26 @@ func (am AppModule) OnRecvPacket(
 	packet channeltypes.Packet,
 	_ sdk.AccAddress,
 ) ibcexported.Acknowledgement {
-	var (
-		ack            ibcexported.Acknowledgement
-		consumerPacket ccv.ConsumerPacketData
-	)
-	// unmarshall consumer packet
-	if err := ccv.ModuleCdc.UnmarshalJSON(packet.GetData(), &consumerPacket); err != nil {
-		errAck := channeltypes.NewErrorAcknowledgement(fmt.Errorf("cannot unmarshal CCV packet data"))
+	consumerPacket, err := UnmarshalConsumerPacket(packet)
+	if err != nil {
+		errAck := ccv.NewErrorAcknowledgementWithLog(ctx, err)
+		return &errAck
+	}
+
+	// TODO: call ValidateBasic method on consumer packet data
+	// See: https://github.com/cosmos/interchain-security/issues/634
+
+	var ack ibcexported.Acknowledgement
+	switch consumerPacket.Type {
+	case ccv.VscMaturedPacket:
+		// handle VSCMaturedPacket
+		ack = am.keeper.OnRecvVSCMaturedPacket(ctx, packet, *consumerPacket.GetVscMaturedPacketData())
+	case ccv.SlashPacket:
+		// handle SlashPacket
+		ack = am.keeper.OnRecvSlashPacket(ctx, packet, *consumerPacket.GetSlashPacketData())
+	default:
+		errAck := ccv.NewErrorAcknowledgementWithLog(ctx, fmt.Errorf("invalid consumer packet type: %q", consumerPacket.Type))
 		ack = &errAck
-	} else {
-		// TODO: call ValidateBasic method on consumer packet data
-		// See: https://github.com/cosmos/interchain-security/issues/634
-
-		switch consumerPacket.Type {
-		case ccv.VscMaturedPacket:
-			// handle VSCMaturedPacket
-			ack = am.keeper.OnRecvVSCMaturedPacket(ctx, packet, *consumerPacket.GetVscMaturedPacketData())
-		case ccv.SlashPacket:
-			// handle SlashPacket
-			ack = am.keeper.OnRecvSlashPacket(ctx, packet, *consumerPacket.GetSlashPacketData())
-		default:
-			errAck := channeltypes.NewErrorAcknowledgement(fmt.Errorf("invalid consumer packet type: %q", consumerPacket.Type))
-			ack = &errAck
-		}
 	}
 
 	ctx.EventManager().EmitEvent(
@@ -210,6 +207,33 @@ func (am AppModule) OnRecvPacket(
 	return ack
 }
 
+func UnmarshalConsumerPacket(packet channeltypes.Packet) (consumerPacket ccv.ConsumerPacketData, err error) {
+	// First try unmarshaling into ccv.ConsumerPacketData type
+	if err := ccv.ModuleCdc.UnmarshalJSON(packet.GetData(), &consumerPacket); err != nil {
+		// If failed, packet should be a v1 slash packet, retry for ConsumerPacketDataV1 packet type
+		var v1Packet ccv.ConsumerPacketDataV1
+		errV1 := ccv.ModuleCdc.UnmarshalJSON(packet.GetData(), &v1Packet)
+		if errV1 != nil {
+			// If neither worked, return error
+			return ccv.ConsumerPacketData{}, errV1
+		}
+
+		// VSC matured packets should not be unmarshaled as v1 packets
+		if v1Packet.Type == ccv.VscMaturedPacket {
+			return ccv.ConsumerPacketData{}, fmt.Errorf("VSC matured packets should be correctly unmarshaled")
+		}
+
+		// Convert from v1 packet type
+		consumerPacket = ccv.ConsumerPacketData{
+			Type: v1Packet.Type,
+			Data: &ccv.ConsumerPacketData_SlashPacketData{
+				SlashPacketData: v1Packet.GetSlashPacketData().FromV1(),
+			},
+		}
+	}
+	return consumerPacket, nil
+}
+
 // OnAcknowledgementPacket implements the IBCModule interface
 func (am AppModule) OnAcknowledgementPacket(
 	ctx sdk.Context,

x/ccv/provider/ibc_module_test.go

@@ -7,6 +7,7 @@ import (
 	authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
 	capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types"
 
+	"github.com/cosmos/ibc-go/v7/modules/core/02-client/types"
 	conntypes "github.com/cosmos/ibc-go/v7/modules/core/03-connection/types"
 	channeltypes "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types"
 	host "github.com/cosmos/ibc-go/v7/modules/core/24-host"
@@ -338,3 +339,62 @@ func TestOnChanOpenConfirm(t *testing.T) {
 		ctrl.Finish()
 	}
 }
+
+func TestUnmarshalConsumerPacket(t *testing.T) {
+	testCases := []struct {
+		name               string
+		packet             channeltypes.Packet
+		expectedPacketData ccv.ConsumerPacketData
+	}{
+		{
+			name: "vsc matured",
+			packet: channeltypes.NewPacket(
+				ccv.ConsumerPacketData{
+					Type: ccv.VscMaturedPacket,
+					Data: &ccv.ConsumerPacketData_VscMaturedPacketData{
+						VscMaturedPacketData: &ccv.VSCMaturedPacketData{
+							ValsetUpdateId: 420,
+						},
+					},
+				}.GetBytes(),
+				342, "sourcePort", "sourceChannel", "destinationPort", "destinationChannel", types.Height{}, 0,
+			),
+			expectedPacketData: ccv.ConsumerPacketData{
+				Type: ccv.VscMaturedPacket,
+				Data: &ccv.ConsumerPacketData_VscMaturedPacketData{
+					VscMaturedPacketData: &ccv.VSCMaturedPacketData{
+						ValsetUpdateId: 420,
+					},
+				},
+			},
+		},
+		{
+			name: "slash packet",
+			packet: channeltypes.NewPacket(
+				ccv.ConsumerPacketData{
+					Type: ccv.SlashPacket,
+					Data: &ccv.ConsumerPacketData_SlashPacketData{
+						SlashPacketData: &ccv.SlashPacketData{
+							ValsetUpdateId: 789,
+						},
+					},
+				}.GetBytes(), // Note packet data is converted to v1 bytes here
+				342, "sourcePort", "sourceChannel", "destinationPort", "destinationChannel", types.Height{}, 0,
+			),
+			expectedPacketData: ccv.ConsumerPacketData{
+				Type: ccv.SlashPacket,
+				Data: &ccv.ConsumerPacketData_SlashPacketData{
+					SlashPacketData: &ccv.SlashPacketData{
+						ValsetUpdateId: 789,
+					},
+				},
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		actualConsumerPacketData, err := provider.UnmarshalConsumerPacket(tc.packet)
+		require.NoError(t, err)
+		require.Equal(t, tc.expectedPacketData, actualConsumerPacketData)
+	}
+}