Draft: Aliasing the v1 channels to v2 counterparty so that v1 channels can support v2 packets #7174

AdityaSripal · 2024-08-14T14:32:04Z

Description

closes: #6975

The following is the simplest possible aliasing that preserves two different code paths for v1 and v2 packets.

The channel is converted into a v2 counterparty for the purpose of handling v2 packets. In this way, you can consider the connection and channel handshake as being very complicated ways to map the identifier (portID, channelID) to the client, and (cpPortId, cpChannelId) to the client counterparty.

This will allow v1 channels to immediately start using v2 packets while retaining the identifiers that they already use. This is critical for immediately enabling atomtic multi-packet data on existing transfer channels. As well as allowing existing channels to very easily upgrade their versions by simply sending a v2 packet with a new version rather than going through channel upgradability

Wire the ability to alias channels by converting Channel v1 into a channel v2 in a one-time fashion (write into state)
Test the aliasing ability specifically against transfer because this is the only important app to alias. Ensure denoms don't change when moving from v1 packet to v2 packet with same channel id
Decide on what to do with in-flight channel upgrades (ensure both sides have eureka enabled before disabling channel upgrades)
Rename Counterparty struct to Channel

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

AdityaSripal · 2024-08-14T15:05:02Z

modules/core/04-channel/keeper/keeper.go

@@ -95,6 +96,30 @@ func (k *Keeper) SetChannel(ctx sdk.Context, portID, channelID string, channel t
 	store.Set(host.ChannelKey(portID, channelID), bz)
 }

+// GetV2Counterparty returns a version 2 counterparty for the given port and channel ID
+// by converting the channel into a version 2 counterparty
+func (k *Keeper) GetV2Counterparty(ctx sdk.Context, portID, channelID string) (clienttypes.Counterparty, bool) {


This effectively resolves the portID and channel ID into a counterparty. So the portID and channelID are in effect the clientID here. It's just a different arbitrary string identifier.

The one annoying thing is that the portID here is both acting as part of the v2 "client-id" (though we could move away from this with a different storage format since channel ids are unique) and also selecting the application to use.

This is fine for packets that are only sending one app data.

For MULTI_PACKET_DATA, we can deal with this by leaving the top portID as is, with the plan to eventually deprecate it. And then still allowing different ports in the internal packet data list

crodriguezvega

Thank you, @AdityaSripal.

Taking transfer packets as an example, with the channel aliasing it will be possible to send a packet from an ibc-go v10 chain to another ibc-go v10 chain either on a existing transfer channel that was already set up or over eureka, right? Are we going to leave the decision of over which channel to send the packet to the user? If that's the case, would we need to add a protocol version field to MsgSendPacket? And the transfer application, which also sends a MsgSendPacket, should specify decide if the packet should be sent over a classic or an eureka channel, right?

It would be nice if there was a way by which we could internally determine if a counterparty chain supports eureka and then we prioritise sending the packet over a eureka channel instead of the existing classic channel. But I don't think that's going to be easy, right?

crodriguezvega · 2024-08-16T15:37:39Z

modules/core/packet-server/keeper/keeper.go

@@ -125,7 +125,7 @@ func (k Keeper) RecvPacket(
 	// Lookup counterparty associated with our channel and ensure that it was packet was indeed
 	// sent by our counterparty.
 	// Note: This can be implemented by the current keeper
-	counterparty, ok := k.ClientKeeper.GetCounterparty(ctx, packet.DestinationChannel)
+	counterparty, ok := k.GetCounterparty(ctx, packet.DestinationPort, packet.DestinationChannel)


If we have a transfer V2 packet sent from an ibc-go v10 chain to another ibc-go v10 chain over an existing channel, the packet's DestinationChannel will be filled in with the client ID of the destination chain, right? But then in the OnRecvPacket callback of the transfer the hash of the denomination will not match that of the tokens received previously (since those used the destination channel ID and not a client ID). Then those vouchers would not be fungible, right?

No, the DestinationChannel will continue to be the existing channel identifier (that's the purpose of this adjustment, to keep fungibility but allow existing channels to send v2 packets)

# Conflicts: # modules/core/02-client/keeper/keeper_test.go # modules/core/02-client/types/client.go # modules/core/02-client/types/client.pb.go # modules/core/02-client/types/client_test.go # modules/core/02-client/types/errors.go # modules/core/02-client/types/msgs.go # modules/core/keeper/msg_server_test.go # modules/core/packet-server/keeper/keeper.go # modules/core/packet-server/keeper/keeper_test.go # modules/core/packet-server/types/keys.go # proto/ibc/core/client/v1/client.proto # testing/endpoint.go

crodriguezvega

A lot of changes had been done on the PR branch before it was synced with the feature branch, so I took me a bit of time to fix the conflicts and sync. I had to comment out some things (for which we can open an issue to keep track). I hope I didn't miss anything while fixing the conflicts. 🤞

crodriguezvega · 2024-09-09T07:05:50Z

modules/core/02-client/client/cli/tx.go

-	flags.AddTxFlagsToCmd(cmd)
-	return cmd
-}
+// func newProvideCounterpartyCmd() *cobra.Command {


I commented out this for now because in this PR MsgProvideCounterparty has changed and needs more information from the CLI to fill it all in.

crodriguezvega · 2024-09-09T07:10:54Z

modules/core/02-client/keeper/grpc_query.go

-	counterparty, _ := q.GetCounterparty(sdkCtx, req.ClientId)
-	res.Counterparty = counterparty
+	// counterparty, _ := q.GetCounterparty(sdkCtx, req.ClientId)
+	// res.Counterparty = counterparty


Commented this also out because there is a circular dependency in the pb.go file between 02-client and packet-server types.

crodriguezvega · 2024-09-09T07:12:27Z

proto/ibc/core/client/v1/query.proto

-  string       creator      = 1;
-  Counterparty counterparty = 2 [(gogoproto.nullable) = false];
+  string creator = 1;
+  // ibc.core.packetserver.v1.Counterparty counterparty = 2 [(gogoproto.nullable) = false];


Commented this out to prevent the circular dependency I mentioned above. We might need to move Counterparty to a different place.

I'd assume we'd simply move the grpc query? We can leave this commented out for now until the core logic is complete, though.

colin-axner · 2024-09-10T14:28:49Z

proto/ibc/core/packetserver/v1/counterparty.proto

+  // client id of the counterparty stored on our chain
+  string client_id = 1;
+  // the counterparty channel identifier that must be used by the packet
+  string counterparty_channel = 2;


Suggested change

string counterparty_channel = 2;

string counterparty_id = 2;

naming suggestion, fine as is

colin-axner · 2024-09-10T14:30:13Z

proto/ibc/core/packetserver/v1/counterparty.proto

+// Counterparty defines the counterparty for a light client to implement IBC eureka protocol
+// this value is stored under our side's channel ID. which we will use to write all packet messages
+// sent to counterparty


Suggested change

// Counterparty defines the counterparty for a light client to implement IBC eureka protocol

// this value is stored under our side's channel ID. which we will use to write all packet messages

// sent to counterparty

// Counterparty defines the counterparty for a light client to implement IBC eureka protocol

// this value is stored under our side's channel ID, which we will use to write all packet messages

// sent to counterparty.

colin-axner · 2024-09-10T14:31:05Z

proto/ibc/core/packetserver/v1/counterparty.proto

+  // the client identifier of the counterparty chain
+  // client id of the counterparty stored on our chain


Suggested change

// the client identifier of the counterparty chain

// client id of the counterparty stored on our chain

// the client identifier of the light client representing the counterparty chain

colin-axner · 2024-09-10T14:31:55Z

proto/ibc/core/packetserver/v1/counterparty.proto

+  // the counterparty channel identifier that must be used by the packet
+  string counterparty_channel = 2;
+  // the key path used to store packet flow messages that the counterparty
+  // will use to send to us. We will append the channelID and sequence in order to create the final path.


Suggested change

// will use to send to us. We will append the channelID and sequence in order to create the final path.

// will use to send to us. In backwards compatible cases, we will append the channelID and sequence in order to create the final path.

colin-axner · 2024-09-10T14:32:37Z

proto/ibc/core/packetserver/v1/counterparty.proto

+// Counterparty defines the counterparty for a light client to implement IBC eureka protocol
+// this value is stored under our side's channel ID. which we will use to write all packet messages
+// sent to counterparty
+message Counterparty {


Think it'd be nice to see this Counterparty type as something used by historical situations (channel/port), but not necessarily only for that

colin-axner · 2024-09-10T14:33:40Z

proto/ibc/core/packetserver/v1/tx.proto

+  option (gogoproto.goproto_getters) = false;
+
+  // unique identifier we will use to write all packet messages sent to counterparty
+  string channel_id = 1;


Suggested change

string channel_id = 1;

string id = 1;

similar naming suggestion

colin-axner · 2024-09-10T14:36:30Z

modules/core/packet-server/types/keys.go

+	// CounterpartyKey is the key used to store counterparty in the client store.
+	// the counterparty key is imported from types instead of host because
+	// the counterparty key is not a part of the ics-24 host specification
+	CounterpartyKey = "counterparty"


wondering if we should be using collections when creating a new store?

On second thought, if we are reusing the channel store, fine as is

colin-axner · 2024-09-10T14:37:16Z

modules/core/04-channel/keeper/keeper.go

@@ -95,6 +97,30 @@ func (k *Keeper) SetChannel(ctx sdk.Context, portID, channelID string, channel t
 	store.Set(host.ChannelKey(portID, channelID), bz)
 }

+// GetV2Counterparty returns a version 2 counterparty for the given port and channel ID
+// by converting the channel into a version 2 counterparty
+func (k *Keeper) GetV2Counterparty(ctx sdk.Context, portID, channelID string) (packetserver.Counterparty, bool) {


Interesting this isn't a function of the packet server keeper? I'm assuming usage still needs to be implemented?

colin-axner · 2024-09-10T14:39:29Z

modules/core/04-channel/keeper/keeper.go

+	if !ok {
+		return packetserver.Counterparty{}, false
+	}
+	merklePathPrefix := commitmentv2types.NewMerklePath(connection.Counterparty.Prefix.KeyPrefix, []byte(""))


byte slice needs channel id info?

colin-axner · 2024-09-10T15:03:02Z

modules/core/keeper/msg_server.go

@@ -143,10 +145,10 @@ func (k *Keeper) IBCSoftwareUpgrade(goCtx context.Context, msg *clienttypes.MsgI
 }

 // ProvideCounterparty defines a rpc handler method for MsgProvideCounterparty.
-func (k *Keeper) ProvideCounterparty(goCtx context.Context, msg *clienttypes.MsgProvideCounterparty) (*clienttypes.MsgProvideCounterpartyResponse, error) {
+func (k *Keeper) ProvideCounterparty(goCtx context.Context, msg *packetservertypes.MsgProvideCounterparty) (*packetservertypes.MsgProvideCounterpartyResponse, error) {


Lets initialize the packet sequence when setting the counterparty? This would allow us to remove the if's in send packet. I feel like calling ProvideCounterparty should be viewed as finalizing the communication details

colin-axner · 2024-09-10T15:09:40Z

modules/core/packet-server/keeper/relay.go

+	counterparty, ok := k.GetCounterparty(ctx, sourceChannel)
+	if !ok {
+		return 0, errorsmod.Wrap(types.ErrCounterpartyNotFound, sourceChannel)
+	}


my understanding is that we basically want logic:

if channeltypes.IsChannelIDFormat(sourceChannel) { counterparty, ok = k.channelKeeper.GetV2Counterparty(ctx, sourcePort, sourceChannel) } else { counterparty, ok = k.GetCounterparty(ctx, sourceChannel) } if !ok { return 0, errorsmod.Wrap(types.ErrCounterpartyNotFound, sourceChannel) }

colin-axner · 2024-09-10T15:19:36Z

I think we should consider breaking up this pr for digestibility, I think there are some changes that should have been left for a followup.
~~1. Moving Counterparty type to packet server~~
~~2. Moving packet handlers out of keeper.go~~
3. Changing Counterparty structure
4. Implementing GetV2Counterparty usage
5. Ensuring no v1 packet code path disruption (channel upgrades)

colin-axner · 2024-09-17T11:01:39Z

proto/ibc/core/packetserver/v1/counterparty.proto

+message Counterparty {
+  // the client identifier of the counterparty chain
+  // client id of the counterparty stored on our chain
+  string client_id = 1;


I think this can be removed now. I see this mapping as:

key: my identifier (v1: channel/port, v2: clientid)
value: counterparty id, merkle path

The client id used to verify the counterparty is in the key, or can be resolved in the v1 case. It seems redundant atm, I would guess in the actual implementation it would only be referenced by the grpc, but we should probably create a new struct for grpc, like we did with the other types IdentifiedClientState for example

really we have a mapping from: our reserved key path -> counterparty reserved key path

resolving the client id responsible for the key path, should happen outside of this mapping?

Before I forget, I went down the thought rabbit hole on this. The proposed construction works perfectly fine, but there's just some minor considerations one could have:

The current construction embeds the clientID into the Counterparty. When we handle v1 cases, the clientID is resolved and set into the counterparty type. In the v2 case, the value is redundant as it is in the key setting the value. The main benefit is code structure as it bundles all the necessary components into a single type. One could also return the clientID in the get counterparty function to achieve the same functionality.

The main reason to keep this approach, would be for future scenarios where we delete the channel/connection mappings and you want to migrate such that all v1 mappings are directly embedded into the counterparty type. One could have two mappings, as I hint at in the above comment. One to obtain the counterparties keyspace which is set for all v1/v2 and one to obtain the light client either directly in v2 case, or via a lookup in v1 case.

The reason why I went this rabbit hole was primarily due to naming. counterparty.clientID referring to your local client identifier, and not a client identifier on the counterparty, could be confusing. Alternatively, if you renamed the Counterparty to something like Connection or Channel, then connection.clientId and connection.counterpartyID could make sense. Either way, just a minor consideration.

aliasing start

2a2d739

AdityaSripal changed the base branch from main to feat/ibc-eureka August 14, 2024 14:32

AdityaSripal commented Aug 14, 2024

View reviewed changes

crodriguezvega reviewed Aug 18, 2024

View reviewed changes

crodriguezvega added the feat: ibc-eureka label Aug 26, 2024

AdityaSripal and others added 5 commits August 30, 2024 17:34

move counterparty logic and msgs into packetserver

016f1f7

get all tests passing with new counterparty msg/handler in new package

ae910a8

add in channel id and leave merkle path as a prefix

faa9add

change relay logic to separate client id from channel id

5b71520

crodriguezvega reviewed Sep 9, 2024

View reviewed changes

crodriguezvega added 2 commits September 9, 2024 09:16

lint

f4dc2b2

lint redux

a752e41

DimitrisJim mentioned this pull request Sep 10, 2024

chore: move handler implementations into a relay.go file. #7271

Merged

10 tasks

colin-axner reviewed Sep 10, 2024

View reviewed changes

colin-axner reviewed Sep 17, 2024

View reviewed changes

AdityaSripal added 2 commits September 18, 2024 17:35

wire GetV2Counterparty to packet server

a8b8569

fix merge

1c01783

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Draft: Aliasing the v1 channels to v2 counterparty so that v1 channels can support v2 packets #7174

Draft: Aliasing the v1 channels to v2 counterparty so that v1 channels can support v2 packets #7174

AdityaSripal commented Aug 14, 2024 •

edited

Loading

AdityaSripal Aug 14, 2024

crodriguezvega left a comment

crodriguezvega Aug 16, 2024

colin-axner Aug 28, 2024

crodriguezvega left a comment

crodriguezvega Sep 9, 2024

crodriguezvega Sep 9, 2024

crodriguezvega Sep 9, 2024

DimitrisJim Sep 9, 2024

colin-axner Sep 10, 2024

colin-axner Sep 10, 2024

colin-axner Sep 10, 2024

colin-axner Sep 10, 2024

colin-axner Sep 10, 2024

colin-axner Sep 10, 2024

colin-axner Sep 10, 2024

colin-axner Sep 10, 2024

colin-axner Sep 10, 2024

colin-axner Sep 10, 2024

colin-axner Sep 10, 2024

colin-axner Sep 10, 2024

colin-axner commented Sep 10, 2024 •

edited

Loading

colin-axner Sep 17, 2024

colin-axner Sep 17, 2024

colin-axner Sep 17, 2024

		// the client identifier of the counterparty chain
		// client id of the counterparty stored on our chain

	// the client identifier of the counterparty chain
	// client id of the counterparty stored on our chain
	// the client identifier of the light client representing the counterparty chain

	// will use to send to us. We will append the channelID and sequence in order to create the final path.
	// will use to send to us. In backwards compatible cases, we will append the channelID and sequence in order to create the final path.

Draft: Aliasing the v1 channels to v2 counterparty so that v1 channels can support v2 packets #7174

Are you sure you want to change the base?

Draft: Aliasing the v1 channels to v2 counterparty so that v1 channels can support v2 packets #7174

Conversation

AdityaSripal commented Aug 14, 2024 • edited Loading

Description

Choose a reason for hiding this comment

crodriguezvega left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crodriguezvega left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-axner commented Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdityaSripal commented Aug 14, 2024 •

edited

Loading

colin-axner commented Sep 10, 2024 •

edited

Loading