Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update WriteAcknowledgementV2 to not depend on channels #7238

Merged
merged 16 commits into from
Sep 10, 2024
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion modules/apps/callbacks/testing/simapp/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -470,7 +470,7 @@ func NewSimApp(
appCodec, keys[ibctransfertypes.StoreKey], app.GetSubspace(ibctransfertypes.ModuleName),
app.IBCFeeKeeper, // ISC4 Wrapper: fee IBC middleware
app.IBCKeeper.ChannelKeeper, app.IBCKeeper.PortKeeper,
app.AccountKeeper, app.BankKeeper, scopedTransferKeeper,
app.AccountKeeper, app.BankKeeper, app.PacketServer, scopedTransferKeeper,
authtypes.NewModuleAddress(govtypes.ModuleName).String(),
)

Expand Down
5 changes: 2 additions & 3 deletions modules/apps/transfer/ibc_module_v2.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"fmt"

errorsmod "cosmossdk.io/errors"

sdk "github.com/cosmos/cosmos-sdk/types"

"github.com/cosmos/ibc-go/v9/modules/apps/transfer/internal/events"
Expand All @@ -15,9 +16,7 @@ import (
ibcerrors "github.com/cosmos/ibc-go/v9/modules/core/errors"
)

var (
_ porttypes.IBCModuleV2 = (*IBCModuleV2)(nil)
)
var _ porttypes.IBCModuleV2 = (*IBCModuleV2)(nil)

// IBCModuleV2 implements the ICS26 interface for transfer given the transfer keeper.
type IBCModuleV2 struct {
Expand Down
415 changes: 387 additions & 28 deletions modules/apps/transfer/ibc_module_v2_test.go

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion modules/apps/transfer/keeper/forwarding.go
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ func (k Keeper) acknowledgeForwardedPacketV2(ctx sdk.Context, forwardedPacket, p
recvResult.Status = channeltypes.PacketStatus_Failure
}

if err := k.channelKeeper.WriteAcknowledgementAsyncV2(ctx, forwardedPacket, types.ModuleName, recvResult); err != nil {
if err := k.packetServerKeeper.WriteAcknowledgementAsyncV2(ctx, forwardedPacket, types.ModuleName, recvResult); err != nil {
return err
}

Expand Down
36 changes: 20 additions & 16 deletions modules/apps/transfer/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package keeper
import (
"errors"
"fmt"
packetserver "github.com/cosmos/ibc-go/v9/modules/core/packet-server/keeper"
"strings"

"cosmossdk.io/log"
Expand Down Expand Up @@ -30,12 +31,13 @@ type Keeper struct {
cdc codec.BinaryCodec
legacySubspace types.ParamSubspace

ics4Wrapper porttypes.ICS4Wrapper
channelKeeper types.ChannelKeeper
portKeeper types.PortKeeper
authKeeper types.AccountKeeper
bankKeeper types.BankKeeper
scopedKeeper exported.ScopedKeeper
ics4Wrapper porttypes.ICS4Wrapper
channelKeeper types.ChannelKeeper
packetServerKeeper *packetserver.Keeper
portKeeper types.PortKeeper
authKeeper types.AccountKeeper
bankKeeper types.BankKeeper
scopedKeeper exported.ScopedKeeper

// the address capable of executing a MsgUpdateParams message. Typically, this
// should be the x/gov module account.
Expand All @@ -52,6 +54,7 @@ func NewKeeper(
portKeeper types.PortKeeper,
authKeeper types.AccountKeeper,
bankKeeper types.BankKeeper,
packetServerKeeper *packetserver.Keeper,
scopedKeeper exported.ScopedKeeper,
authority string,
) Keeper {
Expand All @@ -65,16 +68,17 @@ func NewKeeper(
}

return Keeper{
cdc: cdc,
storeKey: key,
legacySubspace: legacySubspace,
ics4Wrapper: ics4Wrapper,
channelKeeper: channelKeeper,
portKeeper: portKeeper,
authKeeper: authKeeper,
bankKeeper: bankKeeper,
scopedKeeper: scopedKeeper,
authority: authority,
cdc: cdc,
storeKey: key,
legacySubspace: legacySubspace,
ics4Wrapper: ics4Wrapper,
channelKeeper: channelKeeper,
portKeeper: portKeeper,
authKeeper: authKeeper,
bankKeeper: bankKeeper,
scopedKeeper: scopedKeeper,
packetServerKeeper: packetServerKeeper,
authority: authority,
}
}

Expand Down
3 changes: 3 additions & 0 deletions modules/apps/transfer/keeper/keeper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ func (suite *KeeperTestSuite) TestNewKeeper() {
suite.chainA.GetSimApp().IBCKeeper.PortKeeper,
suite.chainA.GetSimApp().AccountKeeper,
suite.chainA.GetSimApp().BankKeeper,
suite.chainA.GetSimApp().PacketServer,
suite.chainA.GetSimApp().ScopedTransferKeeper,
suite.chainA.GetSimApp().ICAControllerKeeper.GetAuthority(),
)
Expand All @@ -78,6 +79,7 @@ func (suite *KeeperTestSuite) TestNewKeeper() {
suite.chainA.GetSimApp().IBCKeeper.PortKeeper,
authkeeper.AccountKeeper{}, // empty account keeper
suite.chainA.GetSimApp().BankKeeper,
suite.chainA.GetSimApp().PacketServer,
suite.chainA.GetSimApp().ScopedTransferKeeper,
suite.chainA.GetSimApp().ICAControllerKeeper.GetAuthority(),
)
Expand All @@ -92,6 +94,7 @@ func (suite *KeeperTestSuite) TestNewKeeper() {
suite.chainA.GetSimApp().IBCKeeper.PortKeeper,
suite.chainA.GetSimApp().AccountKeeper,
suite.chainA.GetSimApp().BankKeeper,
suite.chainA.GetSimApp().PacketServer,
suite.chainA.GetSimApp().ScopedTransferKeeper,
"", // authority
)
Expand Down
6 changes: 0 additions & 6 deletions modules/apps/transfer/types/expected_keepers.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,6 @@ type ChannelKeeper interface {
GetNextSequenceSend(ctx sdk.Context, portID, channelID string) (uint64, bool)
GetAllChannelsWithPortPrefix(ctx sdk.Context, portPrefix string) []channeltypes.IdentifiedChannel
HasChannel(ctx sdk.Context, portID, channelID string) bool
WriteAcknowledgementAsyncV2(
ctx sdk.Context,
packet channeltypes.PacketV2,
appName string,
recvResult channeltypes.RecvPacketResult,
) error
}

// ClientKeeper defines the expected IBC client keeper
Expand Down
6 changes: 3 additions & 3 deletions modules/core/04-channel/keeper/events.go
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ func EmitSendPacketEventV2(ctx sdk.Context, packet types.PacketV2, channel types
ctx.EventManager().EmitEvents(sdk.Events{
sdk.NewEvent(
types.EventTypeSendPacket,
//sdk.NewAttribute(types.AttributeKeyDataHex, hex.EncodeToString(packet.GetData())), TODO
// sdk.NewAttribute(types.AttributeKeyDataHex, hex.EncodeToString(packet.GetData())), TODO
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ref #7248

sdk.NewAttribute(types.AttributeKeyTimeoutHeight, timeoutHeight.String()),
sdk.NewAttribute(types.AttributeKeyTimeoutTimestamp, fmt.Sprintf("%d", packet.GetTimeoutTimestamp())),
sdk.NewAttribute(types.AttributeKeySequence, fmt.Sprintf("%d", packet.GetSequence())),
Expand Down Expand Up @@ -202,7 +202,7 @@ func EmitRecvPacketEventV2(ctx sdk.Context, packet types.PacketV2, channel types
ctx.EventManager().EmitEvents(sdk.Events{
sdk.NewEvent(
types.EventTypeRecvPacket,
//sdk.NewAttribute(types.AttributeKeyDataHex, hex.EncodeToString(packet.GetData())), TODO
// sdk.NewAttribute(types.AttributeKeyDataHex, hex.EncodeToString(packet.GetData())), TODO
sdk.NewAttribute(types.AttributeKeyTimeoutHeight, packet.GetTimeoutHeight().String()),
sdk.NewAttribute(types.AttributeKeyTimeoutTimestamp, fmt.Sprintf("%d", packet.GetTimeoutTimestamp())),
sdk.NewAttribute(types.AttributeKeySequence, fmt.Sprintf("%d", packet.GetSequence())),
Expand Down Expand Up @@ -326,7 +326,7 @@ func EmitAcknowledgePacketEventV2(ctx sdk.Context, packet types.PacketV2, channe
})
}

// emitTimeoutPacketEvent emits a timeout packet event. It will be emitted both the first time a packet
// EmitTimeoutPacketEvent emits a timeout packet event. It will be emitted both the first time a packet
// is timed out for a certain sequence and for all duplicate timeouts.
func EmitTimeoutPacketEvent(ctx sdk.Context, packet types.Packet, channel types.Channel) {
ctx.EventManager().EmitEvents(sdk.Events{
Expand Down
110 changes: 2 additions & 108 deletions modules/core/04-channel/keeper/packet.go
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ func (k *Keeper) RecvPacketV2(
return errorsmod.Wrap(err, "couldn't verify counterparty packet commitment")
}

//if err := k.applyReplayProtection(ctx, packet, channel); err != nil {
// if err := k.applyReplayProtection(ctx, packet, channel); err != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you know why this is commented out?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it may have been commented out to keep diff low, it'll require a duplicate v2 version of all the functions down that code path.

I think we can probably come up with a cleaner solution in the end where the v2 code path is the only path anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also just realised this entire fn doesn't need to exist, channelKeeper.RecvPacketV2 was the impl that existed on the port router branch, we can remove this since the v2 flow will always go through the PacketServerKeeper

// return err
//}

Expand All @@ -298,7 +298,7 @@ func (k *Keeper) RecvPacketV2(
)

// emit an event that the relayer can query for
//emitRecvPacketEvent(ctx, packet, channel)
// emitRecvPacketEvent(ctx, packet, channel)

return nil
}
Expand Down Expand Up @@ -456,112 +456,6 @@ func (k *Keeper) WriteAcknowledgement(
return nil
}

// WriteAcknowledgementAsyncV2 TODO: this has not been tested at all.
func (k *Keeper) WriteAcknowledgementAsyncV2(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved to packetServerKeeper

ctx sdk.Context,
packet types.PacketV2,
appName string,
recvResult types.RecvPacketResult,
) error {

// we should have stored the multi ack structure in OnRecvPacket
ackResults, found := k.GetMultiAcknowledgement(ctx, packet.GetDestinationPort(), packet.GetDestinationChannel(), packet.GetSequence())
if !found {
return errorsmod.Wrapf(types.ErrInvalidAcknowledgement, "multi-acknowledgement not found for %s", appName)
}

// find the index that corresponds to the app.
index := slices.IndexFunc(ackResults.AcknowledgementResults, func(result types.AcknowledgementResult) bool {
return result.AppName == appName
})

if index == -1 {
return errorsmod.Wrapf(types.ErrInvalidAcknowledgement, "acknowledgement not found for %s", appName)
}

existingResult := ackResults.AcknowledgementResults[index]

// ensure that the existing status is async.
if existingResult.RecvPacketResult.Status != types.PacketStatus_Async {
return errorsmod.Wrapf(types.ErrInvalidAcknowledgement, "acknowledgement for %s is not async", appName)
}

// modify the result and set it back.
ackResults.AcknowledgementResults[index].RecvPacketResult = recvResult
k.SetMultiAcknowledgement(ctx, packet.GetDestinationPort(), packet.GetDestinationChannel(), packet.GetSequence(), ackResults)

// check if all acknowledgements are now sync.
isAsync := slices.ContainsFunc(ackResults.AcknowledgementResults, func(ackResult types.AcknowledgementResult) bool {
return ackResult.RecvPacketResult.Status == types.PacketStatus_Async
})

if !isAsync {
// if there are no more async acks, we can write the final multi ack.
return k.WriteAcknowledgementV2(ctx, packet, ackResults)
}

// we have updated one app's result, but there are still async results pending acknowledgement.
return nil

}

func (k *Keeper) WriteAcknowledgementV2(
ctx sdk.Context,
packet types.PacketV2,
multiAck types.MultiAcknowledgement,
) error {
channel, found := k.GetChannel(ctx, packet.GetDestinationPort(), packet.GetDestinationChannel())
if !found {
return errorsmod.Wrap(types.ErrChannelNotFound, packet.GetDestinationChannel())
}

if !slices.Contains([]types.State{types.OPEN, types.FLUSHING, types.FLUSHCOMPLETE}, channel.State) {
return errorsmod.Wrapf(types.ErrInvalidChannelState, "expected one of [%s, %s, %s], got %s", types.OPEN, types.FLUSHING, types.FLUSHCOMPLETE, channel.State)
}

// REPLAY PROTECTION: The recvStartSequence will prevent historical proofs from allowing replay
// attacks on packets processed in previous lifecycles of a channel. After a successful channel
// upgrade all packets under the recvStartSequence will have been processed and thus should be
// rejected. Any asynchronous acknowledgement writes from packets processed in a previous lifecycle of a channel
// will also be rejected.
recvStartSequence, _ := k.GetRecvStartSequence(ctx, packet.GetDestinationPort(), packet.GetDestinationChannel())
if packet.GetSequence() < recvStartSequence {
return errorsmod.Wrap(types.ErrPacketReceived, "packet already processed in previous channel upgrade")
}

// NOTE: IBC app modules might have written the acknowledgement synchronously on
// the OnRecvPacket callback so we need to check if the acknowledgement is already
// set on the store and return an error if so.
if k.HasPacketAcknowledgementV2(ctx, packet.GetDestinationPort(), packet.GetDestinationChannel(), packet.GetSequence()) {
return types.ErrAcknowledgementExists
}

if len(multiAck.AcknowledgementResults) == 0 {
return errorsmod.Wrap(types.ErrInvalidAcknowledgement, "acknowledgement cannot be empty")
}

multiAckBz := k.cdc.MustMarshal(&multiAck)
// set the acknowledgement so that it can be verified on the other side
k.SetPacketAcknowledgementV2(
ctx, packet.GetDestinationPort(), packet.GetDestinationChannel(), packet.GetSequence(),
types.CommitAcknowledgement(multiAckBz),
)

// log that a packet acknowledgement has been written
k.Logger(ctx).Info(
"acknowledgement written",
"sequence", strconv.FormatUint(packet.GetSequence(), 10),
"src_port", packet.GetSourcePort(),
"src_channel", packet.GetSourceChannel(),
"dst_port", packet.GetDestinationPort(),
"dst_channel", packet.GetDestinationChannel(),
)

EmitWriteAcknowledgementEventV2(ctx, packet, channel, multiAck)

return nil
}

// AcknowledgePacket is called by a module to process the acknowledgement of a
// packet previously sent by the calling module on a channel to a counterparty
// module on the counterparty chain. Its intended usage is within the ante
Expand Down
1 change: 1 addition & 0 deletions modules/core/04-channel/types/expected_keepers.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ type ClientKeeper interface {
GetClientConsensusState(ctx sdk.Context, clientID string, height exported.Height) (exported.ConsensusState, bool)
GetClientLatestHeight(ctx sdk.Context, clientID string) clienttypes.Height
GetClientTimestampAtHeight(ctx sdk.Context, clientID string, height exported.Height) (uint64, error)
GetCounterparty(ctx sdk.Context, clientID string) (clienttypes.Counterparty, bool)
}

// ConnectionKeeper expected account IBC connection keeper
Expand Down
16 changes: 8 additions & 8 deletions modules/core/keeper/msg_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,13 @@ package keeper

import (
"context"
errorsmod "cosmossdk.io/errors"
"errors"
"fmt"

"golang.org/x/exp/slices"

errorsmod "cosmossdk.io/errors"

sdk "github.com/cosmos/cosmos-sdk/types"

capabilitytypes "github.com/cosmos/ibc-go/modules/capability/types"
Expand Down Expand Up @@ -614,14 +616,13 @@ func (k *Keeper) recvPacketV2(goCtx context.Context, msg *channeltypes.MsgRecvPa
if err := k.PacketServerKeeper.WriteAcknowledgementV2(ctx, msg.PacketV2, multiAck); err != nil {
return nil, err
}
} else {
// TODO; move to PacketServerKeeper this will blow up with no channels
k.ChannelKeeper.SetMultiAcknowledgement(ctx, msg.PacketV2.GetDestinationPort(), msg.PacketV2.GetDestinationChannel(), msg.PacketV2.GetSequence(), multiAck)
return &channeltypes.MsgRecvPacketResponse{Result: channeltypes.SUCCESS}, nil
}

//defer telemetry.ReportRecvPacket(msg.Packet)
// TODO; move to PacketServerKeeper this will blow up with no channels
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

k.ChannelKeeper.SetMultiAcknowledgement(ctx, msg.PacketV2.GetDestinationPort(), msg.PacketV2.GetDestinationChannel(), msg.PacketV2.GetSequence(), multiAck)

//ctx.Logger().Info("receive packet callback succeeded", "port-id", msg.Packet.SourcePort, "channel-id", msg.Packet.SourceChannel, "result", channeltypes.SUCCESS.String())
// ctx.Logger().Info("receive packet callback succeeded", "port-id", msg.Packet.SourcePort, "channel-id", msg.Packet.SourceChannel, "result", channeltypes.SUCCESS.String())

return &channeltypes.MsgRecvPacketResponse{Result: channeltypes.SUCCESS}, nil
}
Expand Down Expand Up @@ -854,14 +855,13 @@ func (k *Keeper) acknowledgementV2(goCtx context.Context, msg *channeltypes.MsgA
for _, pd := range msg.PacketV2.Data {
cb := k.PortKeeper.AppRouter.Route(pd.AppName)
err = cb.OnAcknowledgementPacketV2(ctx, msg.PacketV2, pd.Payload, recvResults[pd.AppName], relayer)

if err != nil {
ctx.Logger().Error("acknowledgement failed", "port-id", msg.PacketV2.SourcePort, "channel-id", msg.PacketV2.SourceChannel, "error", errorsmod.Wrap(err, "acknowledge packet callback failed"))
return nil, errorsmod.Wrap(err, "acknowledge packet callback failed")
}
}

//defer telemetry.ReportAcknowledgePacket(msg.PacketV2)
// defer telemetry.ReportAcknowledgePacket(msg.PacketV2)

ctx.Logger().Info("acknowledgement succeeded", "port-id", msg.PacketV2.SourcePort, "channel-id", msg.PacketV2.SourceChannel, "result", channeltypes.SUCCESS.String())

Expand Down
Loading
Loading