From 65a0b5f210e1dd00f483007479984c9d1b5b4d15 Mon Sep 17 00:00:00 2001 From: Shawn <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Thu, 29 Jun 2023 10:47:52 -0700 Subject: [PATCH 1/3] UnmarshalConsumerPacket func, use it in tests --- tests/integration/throttle.go | 89 +++++++++-------------------------- x/ccv/provider/ibc_module.go | 57 +++++++++++----------- 2 files changed, 53 insertions(+), 93 deletions(-) diff --git a/tests/integration/throttle.go b/tests/integration/throttle.go index 997765a2c2..0620bdd6b6 100644 --- 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,15 +315,9 @@ func (s *CCVTestSuite) TestPacketSpam() { // Recv 500 packets from consumer to provider in same block for _, packet := range packets { - consumerPacketData := ccvtypes.ConsumerPacketData{} - consumerPacketDataV1 := ccvtypes.ConsumerPacketDataV1{} - err := ccvtypes.ModuleCdc.UnmarshalJSON(packet.GetData(), &consumerPacketData) - if err == nil { - providerKeeper.OnRecvSlashPacket(s.providerCtx(), packet, *consumerPacketData.GetSlashPacketData()) - } else { - ccvtypes.ModuleCdc.MustUnmarshalJSON(packet.GetData(), &consumerPacketDataV1) - providerKeeper.OnRecvSlashPacket(s.providerCtx(), packet, *consumerPacketDataV1.GetSlashPacketData().FromV1()) - } + consumerPacketData, err := provider.UnmarshalConsumerPacket(packet) // Same func used by provider's OnRecvPacket + s.Require().NoError(err) + providerKeeper.OnRecvSlashPacket(s.providerCtx(), packet, *consumerPacketData.GetSlashPacketData()) } // Execute block to handle packets in endblock @@ -374,15 +369,9 @@ func (s *CCVTestSuite) TestDoubleSignDoesNotAffectThrottling() { // Recv 500 packets from consumer to provider in same block for _, packet := range packets { - consumerPacketData := ccvtypes.ConsumerPacketData{} - consumerPacketDataV1 := ccvtypes.ConsumerPacketDataV1{} - err := ccvtypes.ModuleCdc.UnmarshalJSON(packet.GetData(), &consumerPacketData) - if err == nil { - providerKeeper.OnRecvSlashPacket(s.providerCtx(), packet, *consumerPacketData.GetSlashPacketData()) - } else { - ccvtypes.ModuleCdc.MustUnmarshalJSON(packet.GetData(), &consumerPacketDataV1) - providerKeeper.OnRecvSlashPacket(s.providerCtx(), packet, *consumerPacketDataV1.GetSlashPacketData().FromV1()) - } + consumerPacketData, err := provider.UnmarshalConsumerPacket(packet) // Same func used by provider's OnRecvPacket + s.Require().NoError(err) + providerKeeper.OnRecvSlashPacket(s.providerCtx(), packet, *consumerPacketData.GetSlashPacketData()) } // Execute block to handle packets in endblock @@ -476,18 +465,10 @@ func (s *CCVTestSuite) TestQueueOrdering() { // Recv 500 packets from consumer to provider in same block for i, packet := range packets { - consumerPacketData := ccvtypes.ConsumerPacketData{} - consumerPacketDataV1 := ccvtypes.ConsumerPacketDataV1{} - err := ccvtypes.ModuleCdc.UnmarshalJSON(packet.GetData(), &consumerPacketData) - if err != nil { - ccvtypes.ModuleCdc.MustUnmarshalJSON(packet.GetData(), &consumerPacketDataV1) - consumerPacketData = ccvtypes.ConsumerPacketData{ - Type: consumerPacketDataV1.Type, - Data: &ccvtypes.ConsumerPacketData_SlashPacketData{ - SlashPacketData: consumerPacketDataV1.GetSlashPacketData().FromV1(), - }, - } - } + + 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() @@ -700,15 +681,9 @@ func (s *CCVTestSuite) TestSlashSameValidator() { // Recv and queue all slash packets. for _, packet := range packets { - consumerPacketData := ccvtypes.ConsumerPacketData{} - consumerPacketDataV1 := ccvtypes.ConsumerPacketDataV1{} - err := ccvtypes.ModuleCdc.UnmarshalJSON(packet.GetData(), &consumerPacketData) - if err == nil { - providerKeeper.OnRecvSlashPacket(s.providerCtx(), packet, *consumerPacketData.GetSlashPacketData()) - } else { - ccvtypes.ModuleCdc.MustUnmarshalJSON(packet.GetData(), &consumerPacketDataV1) - providerKeeper.OnRecvSlashPacket(s.providerCtx(), packet, *consumerPacketDataV1.GetSlashPacketData().FromV1()) - } + consumerPacketData, err := provider.UnmarshalConsumerPacket(packet) // Same func used by provider's OnRecvPacket + s.Require().NoError(err) + providerKeeper.OnRecvSlashPacket(s.providerCtx(), packet, *consumerPacketData.GetSlashPacketData()) } // We should have 6 pending slash packet entries queued. @@ -767,15 +742,9 @@ func (s CCVTestSuite) TestSlashAllValidators() { //nolint:govet // this is a tes // Recv and queue all slash packets. for _, packet := range packets { - consumerPacketData := ccvtypes.ConsumerPacketData{} - consumerPacketDataV1 := ccvtypes.ConsumerPacketDataV1{} - err := ccvtypes.ModuleCdc.UnmarshalJSON(packet.GetData(), &consumerPacketData) - if err == nil { - providerKeeper.OnRecvSlashPacket(s.providerCtx(), packet, *consumerPacketData.GetSlashPacketData()) - } else { - ccvtypes.ModuleCdc.MustUnmarshalJSON(packet.GetData(), &consumerPacketDataV1) - providerKeeper.OnRecvSlashPacket(s.providerCtx(), packet, *consumerPacketDataV1.GetSlashPacketData().FromV1()) - } + consumerPacketData, err := provider.UnmarshalConsumerPacket(packet) // Same func used by provider's OnRecvPacket + s.Require().NoError(err) + providerKeeper.OnRecvSlashPacket(s.providerCtx(), packet, *consumerPacketData.GetSlashPacketData()) } // We should have 24 pending slash packet entries queued. @@ -820,15 +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{} - packetDataV1 := ccvtypes.ConsumerPacketDataV1{} - err := ccvtypes.ModuleCdc.UnmarshalJSON(packet.GetData(), &packetData) - if err == nil { - providerKeeper.OnRecvSlashPacket(s.providerCtx(), packet, *packetData.GetSlashPacketData()) - } else { - ccvtypes.ModuleCdc.MustUnmarshalJSON(packet.GetData(), &packetDataV1) - providerKeeper.OnRecvSlashPacket(s.providerCtx(), packet, *packetDataV1.GetSlashPacketData().FromV1()) - } + consumerPacketData, err := provider.UnmarshalConsumerPacket(packet) // Same func used by provider's OnRecvPacket + s.Require().NoError(err) + providerKeeper.OnRecvSlashPacket(s.providerCtx(), packet, *consumerPacketData.GetSlashPacketData()) } } @@ -916,15 +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{} - packetDataV1 := ccvtypes.ConsumerPacketDataV1{} - err := ccvtypes.ModuleCdc.UnmarshalJSON(packet.GetData(), &packetData) - if err == nil { - providerKeeper.OnRecvSlashPacket(s.providerCtx(), packet, *packetData.GetSlashPacketData()) - } else { - ccvtypes.ModuleCdc.MustUnmarshalJSON(packet.GetData(), &packetDataV1) - providerKeeper.OnRecvSlashPacket(s.providerCtx(), packet, *packetDataV1.GetSlashPacketData().FromV1()) - } + 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 index 56e1bb9c16..683fcf9329 100644 --- a/x/ccv/provider/ibc_module.go +++ b/x/ccv/provider/ibc_module.go @@ -174,39 +174,17 @@ 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 { - // retry for v1 packet type - var v1Packet ccv.ConsumerPacketDataV1 - errV1 := ccv.ModuleCdc.UnmarshalJSON(packet.GetData(), &v1Packet) - if errV1 != nil { - errAck := ccv.NewErrorAcknowledgementWithLog(ctx, fmt.Errorf("cannot unmarshal CCV packet data")) - ack = &errAck - return ack - } - - if v1Packet.Type == ccv.VscMaturedPacket { - errAck := ccv.NewErrorAcknowledgementWithLog(ctx, fmt.Errorf("unexpected VSCMaturedPacket packet type")) - ack = &errAck - return ack - } - consumerPacket = ccv.ConsumerPacketData{ - Type: v1Packet.Type, - Data: &ccv.ConsumerPacketData_SlashPacketData{ - SlashPacketData: v1Packet.GetSlashPacketData().FromV1(), - }, - } + 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 @@ -230,6 +208,31 @@ func (am AppModule) OnRecvPacket( return ack } +func UnmarshalConsumerPacket(packet channeltypes.Packet) (consumerPacket ccv.ConsumerPacketData, err error) { + // First try unmarshaling into ccv.ConsumerPacketData var, consumerPacket + if err := ccv.ModuleCdc.UnmarshalJSON(packet.GetData(), &consumerPacket); err != nil { + // If failed, retry for v1 packet type + var v1Packet ccv.ConsumerPacketDataV1 + errV1 := ccv.ModuleCdc.UnmarshalJSON(packet.GetData(), &v1Packet) + if errV1 != nil { + return ccv.ConsumerPacketData{}, errV1 + } + + 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, From 1fa49927e283e1f244c9783eb570f435ba4c275f Mon Sep 17 00:00:00 2001 From: Shawn <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Thu, 29 Jun 2023 11:06:50 -0700 Subject: [PATCH 2/3] added test --- x/ccv/provider/ibc_module.go | 6 ++-- x/ccv/provider/ibc_module_test.go | 60 +++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 2 deletions(-) diff --git a/x/ccv/provider/ibc_module.go b/x/ccv/provider/ibc_module.go index 683fcf9329..d6d1188ba4 100644 --- a/x/ccv/provider/ibc_module.go +++ b/x/ccv/provider/ibc_module.go @@ -209,15 +209,17 @@ func (am AppModule) OnRecvPacket( } func UnmarshalConsumerPacket(packet channeltypes.Packet) (consumerPacket ccv.ConsumerPacketData, err error) { - // First try unmarshaling into ccv.ConsumerPacketData var, consumerPacket + // First try unmarshaling into ccv.ConsumerPacketData type if err := ccv.ModuleCdc.UnmarshalJSON(packet.GetData(), &consumerPacket); err != nil { - // If failed, retry for v1 packet type + // 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") } diff --git a/x/ccv/provider/ibc_module_test.go b/x/ccv/provider/ibc_module_test.go index 42279fbb73..96e0eb97d1 100644 --- 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) + } +} From aa8f73a2a6f92e41b9742519c1ed35a5d70d016e Mon Sep 17 00:00:00 2001 From: Shawn <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Thu, 29 Jun 2023 14:06:45 -0700 Subject: [PATCH 3/3] format --- x/ccv/provider/ibc_module.go | 1 - 1 file changed, 1 deletion(-) diff --git a/x/ccv/provider/ibc_module.go b/x/ccv/provider/ibc_module.go index d6d1188ba4..698d37385b 100644 --- a/x/ccv/provider/ibc_module.go +++ b/x/ccv/provider/ibc_module.go @@ -174,7 +174,6 @@ func (am AppModule) OnRecvPacket( packet channeltypes.Packet, _ sdk.AccAddress, ) ibcexported.Acknowledgement { - consumerPacket, err := UnmarshalConsumerPacket(packet) if err != nil { errAck := ccv.NewErrorAcknowledgementWithLog(ctx, err)