From 70ee71e32c17ab2e5c8c3e248fda7f44f3bec906 Mon Sep 17 00:00:00 2001 From: Shawn <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Tue, 27 Jun 2023 15:32:32 -0700 Subject: [PATCH 1/2] log with err ack --- tests/integration/slashing.go | 7 +++---- x/ccv/consumer/ibc_module.go | 2 +- x/ccv/consumer/keeper/relay_test.go | 3 ++- x/ccv/provider/ibc_module.go | 4 ++-- x/ccv/provider/keeper/relay.go | 6 +++--- x/ccv/types/utils.go | 5 +++++ 6 files changed, 16 insertions(+), 11 deletions(-) diff --git a/tests/integration/slashing.go b/tests/integration/slashing.go index d4e960a9cd..68b632efe1 100644 --- a/tests/integration/slashing.go +++ b/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 diff --git a/x/ccv/consumer/ibc_module.go b/x/ccv/consumer/ibc_module.go index d55a1998af..0ca0b29370 100644 --- a/x/ccv/consumer/ibc_module.go +++ b/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) diff --git a/x/ccv/consumer/keeper/relay_test.go b/x/ccv/consumer/keeper/relay_test.go index 299d316d21..c31fa23ec4 100644 --- a/x/ccv/consumer/keeper/relay_test.go +++ b/x/ccv/consumer/keeper/relay_test.go @@ -12,6 +12,7 @@ import ( "github.com/cosmos/cosmos-sdk/crypto/keys/ed25519" capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types" + ccvtypes "github.com/cosmos/interchain-security/v3/x/ccv/types" clienttypes "github.com/cosmos/ibc-go/v7/modules/core/02-client/types" channeltypes "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types" @@ -279,7 +280,7 @@ func TestOnAcknowledgementPacket(t *testing.T) { ).Return(nil).Times(1), ) - ack = channeltypes.NewErrorAcknowledgement(fmt.Errorf("error")) + ack = ccvtypes.NewErrorAcknowledgementWithLog(ctx, fmt.Errorf("error")) err = consumerKeeper.OnAcknowledgementPacket(ctx, packet, ack) require.Nil(t, err) } diff --git a/x/ccv/provider/ibc_module.go b/x/ccv/provider/ibc_module.go index b543c8927e..df0f6066a3 100644 --- a/x/ccv/provider/ibc_module.go +++ b/x/ccv/provider/ibc_module.go @@ -180,7 +180,7 @@ func (am AppModule) OnRecvPacket( ) // unmarshall consumer packet if err := ccv.ModuleCdc.UnmarshalJSON(packet.GetData(), &consumerPacket); err != nil { - errAck := channeltypes.NewErrorAcknowledgement(fmt.Errorf("cannot unmarshal CCV packet data")) + errAck := ccv.NewErrorAcknowledgementWithLog(ctx, fmt.Errorf("cannot unmarshal CCV packet data")) ack = &errAck } else { // TODO: call ValidateBasic method on consumer packet data @@ -194,7 +194,7 @@ func (am AppModule) OnRecvPacket( // handle SlashPacket ack = am.keeper.OnRecvSlashPacket(ctx, packet, *consumerPacket.GetSlashPacketData()) default: - errAck := channeltypes.NewErrorAcknowledgement(fmt.Errorf("invalid consumer packet type: %q", consumerPacket.Type)) + errAck := ccv.NewErrorAcknowledgementWithLog(ctx, fmt.Errorf("invalid consumer packet type: %q", consumerPacket.Type)) ack = &errAck } } diff --git a/x/ccv/provider/keeper/relay.go b/x/ccv/provider/keeper/relay.go index 1aa0858da6..350bc968d4 100644 --- a/x/ccv/provider/keeper/relay.go +++ b/x/ccv/provider/keeper/relay.go @@ -33,7 +33,7 @@ func (k Keeper) OnRecvVSCMaturedPacket( } if err := k.QueueThrottledVSCMaturedPacketData(ctx, chainID, packet.Sequence, data); err != nil { - return channeltypes.NewErrorAcknowledgement(fmt.Errorf( + return ccv.NewErrorAcknowledgementWithLog(ctx, fmt.Errorf( "failed to queue VSCMatured packet data: %s", err.Error())) } @@ -330,7 +330,7 @@ func (k Keeper) OnRecvSlashPacket(ctx sdk.Context, packet channeltypes.Packet, d "vscID", data.ValsetUpdateId, "infractionType", data.Infraction, ) - return channeltypes.NewErrorAcknowledgement(err) + return ccv.NewErrorAcknowledgementWithLog(ctx, err) } // The slash packet validator address may be known only on the consumer chain, @@ -366,7 +366,7 @@ func (k Keeper) OnRecvSlashPacket(ctx sdk.Context, packet channeltypes.Packet, d // Queue slash packet data in the same (consumer chain specific) queue as vsc matured packet data, // to enforce order of handling between the two packet data types. if err := k.QueueThrottledSlashPacketData(ctx, chainID, packet.Sequence, data); err != nil { - return channeltypes.NewErrorAcknowledgement(fmt.Errorf("failed to queue slash packet data: %s", err.Error())) + return ccv.NewErrorAcknowledgementWithLog(ctx, fmt.Errorf("failed to queue slash packet data: %s", err.Error())) } k.Logger(ctx).Info("slash packet received and enqueued", diff --git a/x/ccv/types/utils.go b/x/ccv/types/utils.go index f27dab2b04..524725a532 100644 --- a/x/ccv/types/utils.go +++ b/x/ccv/types/utils.go @@ -85,6 +85,11 @@ func SendIBCPacket( return err } +func NewErrorAcknowledgementWithLog(ctx sdk.Context, err error) channeltypes.Acknowledgement { + ctx.Logger().Error("IBC ErrorAcknowledgement constructed", "error", err) + return channeltypes.NewErrorAcknowledgement(err) +} + // AppendMany appends a variable number of byte slices together func AppendMany(byteses ...[]byte) (out []byte) { for _, bytes := range byteses { From 6fc24a04d8b83762d4f7f815e1ae8ff448ad1952 Mon Sep 17 00:00:00 2001 From: Shawn <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Tue, 27 Jun 2023 15:38:05 -0700 Subject: [PATCH 2/2] linter --- x/ccv/consumer/keeper/relay_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/x/ccv/consumer/keeper/relay_test.go b/x/ccv/consumer/keeper/relay_test.go index c31fa23ec4..1c2ba5152f 100644 --- a/x/ccv/consumer/keeper/relay_test.go +++ b/x/ccv/consumer/keeper/relay_test.go @@ -12,7 +12,6 @@ import ( "github.com/cosmos/cosmos-sdk/crypto/keys/ed25519" capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types" - ccvtypes "github.com/cosmos/interchain-security/v3/x/ccv/types" clienttypes "github.com/cosmos/ibc-go/v7/modules/core/02-client/types" channeltypes "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types" @@ -280,7 +279,7 @@ func TestOnAcknowledgementPacket(t *testing.T) { ).Return(nil).Times(1), ) - ack = ccvtypes.NewErrorAcknowledgementWithLog(ctx, fmt.Errorf("error")) + ack = types.NewErrorAcknowledgementWithLog(ctx, fmt.Errorf("error")) err = consumerKeeper.OnAcknowledgementPacket(ctx, packet, ack) require.Nil(t, err) }