fix: make SlashPacketData backward compatible when sending data over …

…the wire (#1093) * fix: use v1 slash types on consumer side * fix: update provider ibc_module to also handle v1 slash packets * chore: update linter * fix problematic packet handling on provider * rm unused function * refactor/test: 1093 continued (#1104) * UnmarshalConsumerPacket func, use it in tests * added test * format --------- Co-authored-by: Marius Poke <[email protected]> Co-authored-by: Shawn <[email protected]> (cherry picked from commit 7a33cb0)
cosmos · Jun 30, 2023 · f394736 · f394736
1 parent 81f5cc9
commit f394736
Show file tree

Hide file tree

Showing 7 changed files with 919 additions and 93 deletions.
diff --git a/proto/interchain_security/ccv/v1/ccv.proto b/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"];
+}
diff --git a/tests/integration/throttle.go b/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())
 		}
 	}
 

diff --git a/x/ccv/provider/ibc_module.go b/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 := ccv.NewErrorAcknowledgementWithLog(ctx, 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 := ccv.NewErrorAcknowledgementWithLog(ctx, 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,

diff --git a/x/ccv/provider/ibc_module_test.go b/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)
+	}
+}
diff --git a/x/ccv/provider/proposal_handler_test.go b/x/ccv/provider/proposal_handler_test.go
@@ -81,7 +81,8 @@ func TestProviderProposalHandler(t *testing.T) {
 		{
 			name: "unsupported proposal type",
 			// lint rule disabled because this is a test case for an unsupported proposal type
-			content: &distributiontypes.CommunityPoolSpendProposal{ // nolint:staticcheck
+			// nolint:staticcheck
+			content: &distributiontypes.CommunityPoolSpendProposal{
 				Title:       "title",
 				Description: "desc",
 				Recipient:   "",

diff --git a/x/ccv/types/ccv.go b/x/ccv/types/ccv.go
@@ -59,6 +59,23 @@ func NewSlashPacketData(validator abci.Validator, valUpdateId uint64, infraction
 	}
 }
 
+// NewSlashPacketDataV1 creates a new SlashPacketDataV1 that uses ccv.InfractionTypes to maintain backward compatibility.
+func NewSlashPacketDataV1(validator abci.Validator, valUpdateId uint64, infractionType stakingtypes.Infraction) *SlashPacketDataV1 {
+	v1Type := InfractionEmpty
+	switch infractionType {
+	case stakingtypes.Infraction_INFRACTION_DOWNTIME:
+		v1Type = Downtime
+	case stakingtypes.Infraction_INFRACTION_DOUBLE_SIGN:
+		v1Type = DoubleSign
+	}
+
+	return &SlashPacketDataV1{
+		Validator:      validator,
+		ValsetUpdateId: valUpdateId,
+		Infraction:     v1Type,
+	}
+}
+
 func (vdt SlashPacketData) ValidateBasic() error {
 	if len(vdt.Validator.Address) == 0 || vdt.Validator.Power == 0 {
 		return errorsmod.Wrap(ErrInvalidPacketData, "validator fields cannot be empty")
@@ -76,6 +93,10 @@ func (vdt SlashPacketData) GetBytes() []byte {
 	return valDowntimeBytes
 }
 
+func (vdt SlashPacketData) ToV1() *SlashPacketDataV1 {
+	return NewSlashPacketDataV1(vdt.Validator, vdt.ValsetUpdateId, vdt.Infraction)
+}
+
 func (cp ConsumerPacketData) ValidateBasic() (err error) {
 	switch cp.Type {
 	case VscMaturedPacket:
@@ -99,7 +120,45 @@ func (cp ConsumerPacketData) ValidateBasic() (err error) {
 	return
 }
 
+// Convert to bytes while maintaining over the wire compatibility with previous versions.
 func (cp ConsumerPacketData) GetBytes() []byte {
-	bytes := ModuleCdc.MustMarshalJSON(&cp)
+	return cp.ToV1Bytes()
+}
+
+// ToV1Bytes converts the ConsumerPacketData to JSON byte array compatible
+// with the format used by ICS versions using cosmos-sdk v45 (ICS v1 and ICS v2).
+func (cp ConsumerPacketData) ToV1Bytes() []byte {
+	if cp.Type != SlashPacket {
+		bytes := ModuleCdc.MustMarshalJSON(&cp)
+		return bytes
+	}
+
+	sp := cp.GetSlashPacketData()
+	spdv1 := NewSlashPacketDataV1(sp.Validator, sp.ValsetUpdateId, sp.Infraction)
+	cpv1 := ConsumerPacketDataV1{
+		Type: cp.Type,
+		Data: &ConsumerPacketDataV1_SlashPacketData{
+			SlashPacketData: spdv1,
+		},
+	}
+	bytes := ModuleCdc.MustMarshalJSON(&cpv1)
 	return bytes
 }
+
+// FromV1 converts SlashPacketDataV1 to SlashPacketData.
+// Provider must handle both V1 and later versions of the SlashPacketData.
+func (vdt1 SlashPacketDataV1) FromV1() *SlashPacketData {
+	newType := stakingtypes.Infraction_INFRACTION_UNSPECIFIED
+	switch vdt1.Infraction {
+	case Downtime:
+		newType = stakingtypes.Infraction_INFRACTION_DOWNTIME
+	case DoubleSign:
+		newType = stakingtypes.Infraction_INFRACTION_DOUBLE_SIGN
+	}
+
+	return &SlashPacketData{
+		Validator:      vdt1.Validator,
+		ValsetUpdateId: vdt1.ValsetUpdateId,
+		Infraction:     newType,
+	}
+}