ICS4: Introduce Socket type to capture the (Port, Channel) tuple

[adizere]

The tuple (Port, Channel) appears all over the place in the core ICS spec, notably in ICS 004. We can refactor this into a single type, named Socket, to reduce the number of arguments in some methods, and simplify some calls and interfaces.

interface Packet {
  sequence: uint64
  timeoutHeight: Height
  timeoutTimestamp: uint64
-  sourcePort: Identifier
-  sourceChannel: Identifier
+ sourceSocket: (Identifier, Identifier)
-  destPort: Identifier
-  destChannel: Identifier
+ destSocket: (Identifier, Identifier)
  data: bytes
}

I will do a first pass over the specs and open a PR, but first I'd like to make sure there is no obvious drawback (e.g., awkward or misleading) about introducing a Socket type.

Ref: https://github.com/informalsystems/ibc-rs/issues/745#issuecomment-828739480

[colin-axner]

I'm in favor, but this is breaks channel handshakes (I assume the protobuf definition would change?)

We would need to utilize the connection version to indicate that the v2 channel protobuf definition should be used, in order to maintain backwards compatibility. I believe you would always need to have the connection as well, in order to decode the channel from the store. For example, if I want to query transfer/channel-0, how would I know whether it uses v1 or v2 protobuf definition for decoding? The only way I can think of is via the connection struct

Alternatively, we could push this change to a time when we are ready to disable creation of new channels to chains running older versions. That is, force all new channel handshakes to utilize connection versions >= 2 (or whatever the next version increment is)

In general I'm in favor of not letting backwards compatibility hinder improvements (even if minor). I'd rather work on building a robust setup for supporting breaking changes in a backwards compatible fashion. Otherwise IBC will become frozen in time

[ethanfrey]

I think this would make the spec a lot clearer for the pseudo-code and descriptions. I would not change the protobuf files though.

I understood the idea to just make the spec easier to follow.

[cwgoes]

As I understand your proposal, the Socket object is not first-class within IBC - none of the logic will change - it's just the combination of a channel identifier and a port identifier. That's fine although we don't do it too much for other cases where data could be combined into a single composite datatype (e.g. maybe timeoutHeight and timeoutTimestamp should be Timeouts or something). For that reason, I'm just a wee bit hesitant, because I'm not sure how to draw a consistent line as to what merits a composite data structure and what does not. This is a very minor concern though, and simplifying the handshake arguments would be nice.

Regarding upgrades I concur with @colin-axner and I do not think changing the protobuf here is high-priority enough to merit its own upgrade, I would just bundle it with another one.

[adizere]

Thank you all for the quick feedback, these are all points that deserve a thorough discussion!

First, it would be good for this change to eventually propagate into the protobufs.

We can treat as a separate problem the question of whether we do it in a backwards compatible way ASAP, or whether we delay & bundle this with other changes (and deprecate v1.channels). Let's discuss this question now..

Alternatively, we could push this change to a time when we are ready to disable creation of new channels to chains running older versions. That is, force all new channel handshakes to utilize connection versions >= 2 (or whatever the next version increment is)

I would vote that the change to protobufs should wait. Reading through these comments, I see two reasons to prefer that:

1. there isn't enough clarity (to me, at least) as to how to implement this in a backwards compatible way. It would be nice if we could just add a new type with url "/ibc.core.channel.v2.MsgChannelOpenInit" and use that, but to @colin-axner's point this change will need to rely on support from ICS03/connection versions as well, so it doesn't look like a straightforward change.
2. even if we limit this change to just the specs (temporarily), there are immediate benefits already.

I'd go even further and propose that other types (e.g., Timeouts as mentioned by @cwgoes) should benefit from the same kind of improvements and we can treat them with a similar approach.

I can see going forward as follows:

1. we do the change in a PR and convince ourselves that it merits at least a spec-only change,
2. assuming the PR gets merged, I agree that this is not pressing (not a bug or critical feature), so implementing this in protobufs can wait to be bundled with other changes, and eventually we'll completely break away from v1.channels.
3. I will start investigating other types and interfaces that could benefit from similar simplifications.

In general I'm in favor of not letting backwards compatibility hinder improvements (even if minor). I'd rather work on building a robust setup for supporting breaking changes in a backwards compatible fashion. Otherwise IBC will become frozen in time

This is important, I'm very curious to learn more about maintaining backwards compatibility. We're already experiencing different SDK versions in practice (e.g., yesterday with Hermes trying to relay between a regen 1.1.0 building on SDK 0.43.0.beta1 and a gaiad 4.2.1 atop SDK 0.42.4). One way to think about this is as a balancing between two concerns: avoiding the issue of IBC becoming "frozen in time", versus the mounting complexity of maintaining compatibility across multiple versions.

[colin-axner]

Nice summary @adizere . Sounds like a good course of action.

I realized there are more options for backwards compatibility than I initially commented on. If we only changed the packet protobuf definition, on ibc-go, the only affected items are the Msgs. This still requires relayers to be smart enough to know which Msg to submit to which chain based on code versioning (so still a fairly significant concern). The packet protobuf definition isn't completely verified by IBC as we construct a hash of parts of it using a key which contains the other information (src channel id/port id)

I'd suggest we make a discussion thread on this repository to discuss breaking changes which cause concern for backwards compatibility. I think it'd be good if we start documenting which changes are really hard/complex vs which are relatively easy and straightforward. This will help us identify problem areas that we can hopefully mitigate in the future, or batch a bunch of improvements for some mega upgrade down the line. I think it is good we discuss these issues on cosmos/ibc since it is hard for me as a developer of ibc-go to understand how much complexity a change might cause on the relayer

[adizere]

This idea proved not very useful nor getting traction in particular due to breaking nature. Might be useful for a v2 specification of IBC in the future.