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)
 }