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

feat: add acknowledgePacket handler to channel/v2 #7412

Open
wants to merge 4 commits into
base: feat/ibc-eureka
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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
13 changes: 9 additions & 4 deletions modules/core/04-channel/v2/keeper/events.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,17 @@ func EmitSendPacketEvents(ctx context.Context, packet channeltypesv2.Packet) {
// TODO: https://github.com/cosmos/ibc-go/issues/7386
}

// EmitTimeoutPacketEvents emits events for the TimeoutPacket handler.
func EmitTimeoutPacketEvents(ctx context.Context, packet channeltypesv2.Packet) {
// EmitRecvPacketEvents emits events for the RecvPacket handler.
func EmitRecvPacketEvents(ctx context.Context, packet channeltypesv2.Packet) {
// TODO: https://github.com/cosmos/ibc-go/issues/7386
}

// EmitRecvPacketEvents emits events for the RecvPacket handler.
func EmitRecvPacketEvents(ctx context.Context, packet channeltypesv2.Packet) {
// EmitAcknowledgePacketEvents emits events for the AcknowledgePacket handler.
func EmitAcknowledgePacketEvents(ctx context.Context, packet channeltypesv2.Packet, acknowledgement channeltypesv2.Acknowledgement) {
// TODO: https://github.com/cosmos/ibc-go/issues/7386
}

// EmitTimeoutPacketEvents emits events for the TimeoutPacket handler.
func EmitTimeoutPacketEvents(ctx context.Context, packet channeltypesv2.Packet) {
// TODO: https://github.com/cosmos/ibc-go/issues/7386
}
24 changes: 18 additions & 6 deletions modules/core/04-channel/v2/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,17 +75,17 @@ func (k *Keeper) GetCounterparty(ctx context.Context, clientID string) (types.Co
}

// GetPacketReceipt returns the packet receipt from the packet receipt path based on the sourceID and sequence.
func (k *Keeper) GetPacketReceipt(ctx context.Context, sourceID string, sequence uint64) (string, bool) {
func (k *Keeper) GetPacketReceipt(ctx context.Context, sourceID string, sequence uint64) []byte {
Copy link
Contributor

Choose a reason for hiding this comment

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

why the removal of the bool return value?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the usage later, I guess technically i provides no added value as we can do a len check in line, and also we would never be storing an empty bz array so that case does not need to be differentiated. I also see we do it this way in the channel v1 stuff, will leave the comment here in case anyone else was wondering!

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, might change this back to be honest. As v1 has the bool return value for receipts (but not commitments - which I also changed).

Actually had a todo which I forgot to address here where the usage should be HasReceipt I think. I can push an update before merging!

store := k.storeService.OpenKVStore(ctx)
bigEndianBz := sdk.Uint64ToBigEndian(sequence)
bz, err := store.Get(hostv2.PacketReceiptKey(sourceID, bigEndianBz))
if err != nil {
panic(err)
}
if len(bz) == 0 {
return "", false
return nil
}
return string(bz), true
return bz
}

// SetPacketReceipt writes the packet receipt under the receipt path
Expand Down Expand Up @@ -121,17 +121,17 @@ func (k *Keeper) HasPacketAcknowledgement(ctx context.Context, sourceID string,
}

// GetPacketCommitment returns the packet commitment hash under the commitment path.
func (k *Keeper) GetPacketCommitment(ctx context.Context, sourceID string, sequence uint64) (string, bool) {
func (k *Keeper) GetPacketCommitment(ctx context.Context, sourceID string, sequence uint64) []byte {
store := k.storeService.OpenKVStore(ctx)
bigEndianBz := sdk.Uint64ToBigEndian(sequence)
bz, err := store.Get(hostv2.PacketCommitmentKey(sourceID, bigEndianBz))
if err != nil {
panic(err)
}
if len(bz) == 0 {
return "", false
return nil
}
return string(bz), true
return bz
}

// SetPacketCommitment writes the commitment hash under the commitment path.
Expand Down Expand Up @@ -199,3 +199,15 @@ func (k *Keeper) AliasV1Channel(ctx context.Context, portID, channelID string) (
}
return counterparty, true
}

// getV1Counterparty attempts to retrieve a v1 channel from the channel keeper if it exists, then converts it
Copy link
Member Author

Choose a reason for hiding this comment

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

moved from relay.go to keeper.go

// to a v2 counterparty and stores it in the v2 channel keeper for future use
func (k *Keeper) getV1Counterparty(ctx context.Context, port, id string) (types.Counterparty, bool) {
if counterparty, ok := k.AliasV1Channel(ctx, port, id); ok {
// we can key on just the channel here since channel ids are globally unique
k.SetCounterparty(ctx, id, counterparty)
return counterparty, true
}

return types.Counterparty{}, false
}
38 changes: 36 additions & 2 deletions modules/core/04-channel/v2/keeper/msg_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,42 @@ func (k *Keeper) SendPacket(ctx context.Context, msg *channeltypesv2.MsgSendPack
return &channeltypesv2.MsgSendPacketResponse{Sequence: sequence}, nil
}

func (*Keeper) Acknowledgement(ctx context.Context, acknowledgement *channeltypesv2.MsgAcknowledgement) (*channeltypesv2.MsgAcknowledgementResponse, error) {
panic("implement me")
func (k *Keeper) Acknowledgement(ctx context.Context, msg *channeltypesv2.MsgAcknowledgement) (*channeltypesv2.MsgAcknowledgementResponse, error) {
sdkCtx := sdk.UnwrapSDKContext(ctx)
relayer, err := sdk.AccAddressFromBech32(msg.Signer)
if err != nil {
sdkCtx.Logger().Error("acknowledgement failed", "error", errorsmod.Wrap(err, "Invalid address for msg Signer"))
return nil, errorsmod.Wrap(err, "Invalid address for msg Signer")
}

cacheCtx, writeFn := sdkCtx.CacheContext()
err = k.acknowledgePacket(cacheCtx, msg.Packet, msg.Acknowledgement, msg.ProofAcked, msg.ProofHeight)

switch err {
case nil:
writeFn()
case channeltypesv1.ErrNoOpMsg:
// no-ops do not need event emission as they will be ignored
sdkCtx.Logger().Debug("no-op on redundant relay", "source-id", msg.Packet.SourceId)
return &channeltypesv2.MsgAcknowledgementResponse{Result: channeltypesv1.NOOP}, nil
default:
sdkCtx.Logger().Error("acknowledgement failed", "source-id", msg.Packet.SourceId, "error", errorsmod.Wrap(err, "acknowledge packet verification failed"))
return nil, errorsmod.Wrap(err, "acknowledge packet verification failed")
}

_ = relayer

// TODO: implement once app router is wired up.
// https://github.com/cosmos/ibc-go/issues/7384
// for _, pd := range msg.PacketData {
// cbs := k.PortKeeper.AppRouter.Route(pd.SourcePort)
// err := cbs.OnSendPacket(ctx, msg.SourceId, sequence, msg.TimeoutTimestamp, pd, signer)
// if err != nil {
// return nil, err
// }
// }

return nil, nil
}

// RecvPacket implements the PacketMsgServer RecvPacket method.
Expand Down
Copy link
Member Author

Choose a reason for hiding this comment

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

renamed relay.go to packet.go

Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,6 @@ import (
"github.com/cosmos/ibc-go/v9/modules/core/packet-server/types"
)

// getV1Counterparty attempts to retrieve a v1 channel from the channel keeper if it exists, then converts it
// to a v2 counterparty and stores it in the v2 channel keeper for future use
func (k *Keeper) getV1Counterparty(ctx context.Context, port, id string) (channeltypesv2.Counterparty, bool) {
if counterparty, ok := k.AliasV1Channel(ctx, port, id); ok {
// we can key on just the channel here since channel ids are globally unique
k.SetCounterparty(ctx, id, counterparty)
return counterparty, true
}

return channeltypesv2.Counterparty{}, false
}

// sendPacket constructs a packet from the input arguments, writes a packet commitment to state
// in order for the packet to be sent to the counterparty.
func (k *Keeper) sendPacket(
Expand Down Expand Up @@ -137,8 +125,9 @@ func (k Keeper) recvPacket(
// REPLAY PROTECTION: Packet receipts will indicate that a packet has already been received
// on unordered channels. Packet receipts must not be pruned, unless it has been marked stale
// by the increase of the recvStartSequence.
_, found := k.GetPacketReceipt(ctx, packet.DestinationId, packet.Sequence)
if found {
// TODO: change to HasReceipt
receipt := k.GetPacketReceipt(ctx, packet.DestinationId, packet.Sequence)
if receipt != nil {
EmitRecvPacketEvents(ctx, packet)
// This error indicates that the packet has already been relayed. Core IBC will
// treat this error as a no-op in order to prevent an entire relay transaction
Expand Down Expand Up @@ -173,6 +162,62 @@ func (k Keeper) recvPacket(
return nil
}

func (k Keeper) acknowledgePacket(ctx context.Context, packet channeltypesv2.Packet, acknowledgement channeltypesv2.Acknowledgement, proof []byte, proofHeight exported.Height) error {
// Lookup counterparty associated with our channel and ensure
// that the packet was indeed sent by our counterparty.
counterparty, ok := k.GetCounterparty(ctx, packet.SourceId)
Copy link
Member Author

Choose a reason for hiding this comment

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

this will become a v2 channelEnd, and below clientID := counterparty.ClientID will read better... right now its slightly confusing.

The channelEnd.ClientID would suggest to me that it's the clientID of the client representing the counterparty state machine. Rather than a counterparty.ClientID which would suggest to me its the clientID on the counterparty which repesents this chain (self host where this code is being executed)

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah the naming is a bit all over the place at the moment with regards to counterparty/channel/ids etc, hopefully we'll be able to sort this out quite soon!

if !ok {
return errorsmod.Wrap(types.ErrCounterpartyNotFound, packet.SourceId)
}

if counterparty.ClientId != packet.DestinationId {
return channeltypes.ErrInvalidChannelIdentifier
}
clientID := counterparty.ClientId

commitment := k.GetPacketCommitment(ctx, packet.SourceId, packet.Sequence)
if len(commitment) == 0 {
// TODO: emit events
// k.EmitAcknowledgePacketEvent(ctx, packet, nil)

// This error indicates that the acknowledgement has already been relayed
// or there is a misconfigured relayer attempting to prove an acknowledgement
// for a packet never sent. Core IBC will treat this error as a no-op in order to
// prevent an entire relay transaction from failing and consuming unnecessary fees.
return channeltypes.ErrNoOpMsg
}

packetCommitment := channeltypesv2.CommitPacket(packet)

// verify we sent the packet and haven't cleared it out yet
if !bytes.Equal(commitment, packetCommitment) {
return errorsmod.Wrapf(channeltypes.ErrInvalidPacket, "commitment bytes are not equal: got (%v), expected (%v)", packetCommitment, commitment)
}

path := hostv2.PacketAcknowledgementKey(packet.DestinationId, sdk.Uint64ToBigEndian(packet.Sequence))
merklePath := types.BuildMerklePath(counterparty.MerklePathPrefix, path)
Copy link
Member Author

Choose a reason for hiding this comment

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

BuildMerklePath, used here and other funcs in this file is imported from the packet-server package.

We should move this to the channel/v2 types pkg afaik. We should not depend on anything in packet-server (which will ultimately be removed).

I'd also like some insight into the implementation of BuildMerklePath.
Some questions I have are:
Why are we not using the MerklePrefix type for the "ibc" prefix used by counterparties - instead we're using MerklePath.
Secondly, implementation looks complex and kinda funky, haven't stepped through to see exactly why that is, was hoping someone could explain it for me.

Copy link
Contributor

Choose a reason for hiding this comment

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

re dependencies: I think we can do a pass at the end and make sure there is no dependency on v1 anything within the channel/v2 package.


if err := k.ClientKeeper.VerifyMembership(
ctx,
clientID,
proofHeight,
0, 0,
proof,
merklePath,
channeltypesv2.CommitAcknowledgement(acknowledgement),
); err != nil {
return errorsmod.Wrapf(err, "failed packet acknowledgement verification for client (%s)", clientID)
}

k.DeletePacketCommitment(ctx, packet.SourceId, packet.Sequence)

k.Logger(ctx).Info("packet acknowledged", "sequence", strconv.FormatUint(packet.GetSequence(), 10), "source_id", packet.GetSourceId(), "destination_id", packet.GetDestinationId())

EmitAcknowledgePacketEvents(ctx, packet, acknowledgement)
Comment on lines +214 to +216
Copy link
Member Author

Choose a reason for hiding this comment

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

I guess these logs and events should be at the msg server handler layer. wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that would make the most sense yeah, can do in a follow up though I think!


return nil
}

// timeoutPacket implements the timeout logic required by a packet handler.
// The packet is checked for correctness including asserting that the packet was
// sent and received on clients which are counterparties for one another.
Expand Down Expand Up @@ -209,9 +254,8 @@ func (k Keeper) timeoutPacket(
}

// check that the commitment has not been cleared and that it matches the packet sent by relayer
commitment, ok := k.GetPacketCommitment(ctx, packet.SourceId, packet.Sequence)

if !ok {
commitment := k.GetPacketCommitment(ctx, packet.SourceId, packet.Sequence)
if len(commitment) == 0 {
EmitTimeoutPacketEvents(ctx, packet)
// This error indicates that the timeout has already been relayed
// or there is a misconfigured relayer attempting to prove a timeout
Expand All @@ -222,7 +266,7 @@ func (k Keeper) timeoutPacket(

packetCommitment := channeltypesv2.CommitPacket(packet)
// verify we sent the packet and haven't cleared it out yet
if !bytes.Equal([]byte(commitment), packetCommitment) {
if !bytes.Equal(commitment, packetCommitment) {
return errorsmod.Wrapf(channeltypes.ErrInvalidPacket, "packet commitment bytes are not equal: got (%v), expected (%v)", commitment, packetCommitment)
}

Expand Down
55 changes: 55 additions & 0 deletions modules/core/04-channel/v2/types/commitment.go
Copy link
Member Author

Choose a reason for hiding this comment

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

created commitment.go to house commitment funcs

Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
package types

import (
"crypto/sha256"

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

// CommitPacket returns the V2 packet commitment bytes. The commitment consists of:
// sha256_hash(timeout) + sha256_hash(destinationID) + sha256_hash(packetData) from a given packet.
// This results in a fixed length preimage.
// NOTE: A fixed length preimage is ESSENTIAL to prevent relayers from being able
// to malleate the packet fields and create a commitment hash that matches the original packet.
func CommitPacket(packet Packet) []byte {
buf := sdk.Uint64ToBigEndian(packet.GetTimeoutTimestamp())

destIDHash := sha256.Sum256([]byte(packet.DestinationId))
buf = append(buf, destIDHash[:]...)

for _, data := range packet.Data {
buf = append(buf, hashPacketData(data)...)
}

hash := sha256.Sum256(buf)
return hash[:]
}

// hashPacketData returns the hash of the packet data.
func hashPacketData(data PacketData) []byte {
var buf []byte
sourceHash := sha256.Sum256([]byte(data.SourcePort))
buf = append(buf, sourceHash[:]...)
destHash := sha256.Sum256([]byte(data.DestinationPort))
buf = append(buf, destHash[:]...)
payloadValueHash := sha256.Sum256(data.Payload.Value)
buf = append(buf, payloadValueHash[:]...)
payloadEncodingHash := sha256.Sum256([]byte(data.Payload.Encoding))
buf = append(buf, payloadEncodingHash[:]...)
payloadVersionHash := sha256.Sum256([]byte(data.Payload.Version))
buf = append(buf, payloadVersionHash[:]...)
hash := sha256.Sum256(buf)
return hash[:]
}

// CommitAcknowledgement returns the hash of the acknowledgement data.
func CommitAcknowledgement(acknowledgement Acknowledgement) []byte {
var buf []byte
for _, ack := range acknowledgement.GetAcknowledgementResults() {
hash := sha256.Sum256(ack.RecvPacketResult.GetAcknowledgement())
buf = append(buf, hash[:]...)
}

hash := sha256.Sum256(buf)
return hash[:]
}
39 changes: 0 additions & 39 deletions modules/core/04-channel/v2/types/packet.go
Original file line number Diff line number Diff line change
@@ -1,13 +1,10 @@
package types

import (
"crypto/sha256"
"strings"

errorsmod "cosmossdk.io/errors"

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

host "github.com/cosmos/ibc-go/v9/modules/core/24-host"
)

Expand Down Expand Up @@ -87,39 +84,3 @@ func (p Payload) Validate() error {
}
return nil
}

// CommitPacket returns the V2 packet commitment bytes. The commitment consists of:
// sha256_hash(timeout) + sha256_hash(destinationID) + sha256_hash(packetData) from a given packet.
// This results in a fixed length preimage.
// NOTE: A fixed length preimage is ESSENTIAL to prevent relayers from being able
// to malleate the packet fields and create a commitment hash that matches the original packet.
func CommitPacket(packet Packet) []byte {
buf := sdk.Uint64ToBigEndian(packet.GetTimeoutTimestamp())

destIDHash := sha256.Sum256([]byte(packet.DestinationId))
buf = append(buf, destIDHash[:]...)

for _, data := range packet.Data {
buf = append(buf, hashPacketData(data)...)
}

hash := sha256.Sum256(buf)
return hash[:]
}

// hashPacketData returns the hash of the packet data.
func hashPacketData(data PacketData) []byte {
var buf []byte
sourceHash := sha256.Sum256([]byte(data.SourcePort))
buf = append(buf, sourceHash[:]...)
destHash := sha256.Sum256([]byte(data.DestinationPort))
buf = append(buf, destHash[:]...)
payloadValueHash := sha256.Sum256(data.Payload.Value)
buf = append(buf, payloadValueHash[:]...)
payloadEncodingHash := sha256.Sum256([]byte(data.Payload.Encoding))
buf = append(buf, payloadEncodingHash[:]...)
payloadVersionHash := sha256.Sum256([]byte(data.Payload.Version))
buf = append(buf, payloadVersionHash[:]...)
hash := sha256.Sum256(buf)
return hash[:]
}
Loading