cosmos / ibc-go

Inter-Blockchain Communication Protocol (IBC) implementation in Golang.
https://ibc.cosmos.network/
MIT License
550 stars 584 forks source link

Alias new channel IDs to the client IDs #6975

Closed AdityaSripal closed 1 month ago

AdityaSripal commented 3 months ago

Allow multiple aliases to map to the same client-id. This should be implemented as a many-to-one layer in the channel keeper. When doing packet processing, retrieve the client id by resolving the channel id to the underlying client using the alias map.

The alias map can be added to using genesis, migration, or through governance.

This will allow a backwards compatible transition from ibc-classic to ibc-eureka since we do not need to migrate over channel ids and transfer escrows. It will also allow governance to use this alias map in the future for more readable channel-ids.

DimitrisJim commented 3 months ago

dropping comment made to keep track of additional validation func that can be used for counterparty id https://github.com/cosmos/ibc-go/pull/6982#discussion_r1696842279

colin-axner commented 2 months ago

Delay periods are the only reason I see not to do an automatic migration. Perhaps we could add logic:

In addition, we should consider spreading the migration across several blocks since it doesn't need to all be done in one go.

colin-axner commented 2 months ago

Ah, connection counterparties may vary even when using the same local client id. I believe you will need to alias this as well. Essentially, we would have a map from channelID -> {local_client_id, counterparty_connection_id} and in the counterparty map, connection_id -> {counterparty_client_id, prefix}. I believe things would break if you tried changing the counterparty value in existing connections as the counterparty would make an assertion on the dest_channel_id which would be incorrect

colin-axner commented 2 months ago

Thinking about this some more, we would need to store the counterparty alias as well, this would be utilized in RecvPacket when checking the source information. Given that this would then turn into an alias map which is just the channel/connection layers with condensed information, rather than duplicating this information, lets rely on the channel/connection layers (even if it is two store reads rather than one) in the meantime?

Once we plan to remove the connection/channel structs, then an alias map seems useful, but I believe the best motivation to support existing channels to use eureka would be to access the app version?

I can open a pr with resolving channel ids to client ids to add support for eureka flow with existing channels. Then maybe as a separate, nice-to-have issue, we could attempt to migrate connection structs to this alias map. The map would be:

alias map channelID -> {local_client_id, counterparty_channel_id} with the counterparty map adding counterparty_channel_id -> {counterparty_client_id, prefix}

It's unclear to me if the connection IDs need to be referenced at all?

colin-axner commented 2 months ago

Actually, I'm lost on this safety of this issue.

It's unsafe to send packets for classic channels using eureka, as we need to continue doing classic processing (channel upgrade logic for example)

If we send packets using client ids, this should be seen as a distinct channel (it will generate a different denom)

Until we are ready to migrate from channel/connection layers completely, I'm not sure I understand the correctness and motivation for this issue?

AdityaSripal commented 2 months ago

I want to keep v1 channel processing as it is. But i would like to be able to send v2 packets on with v1 channel identifiers on day 1.

I don't see why we can't have v2 packets that just use those channel identifiers as identifiers down to the underlying client that can exist side-by-side with the regular v1 channel packet flow (for UNORDERED channels at least).

That way we don't break existing functionality

But we immediately add in the ability to create new versioned packets on the fly And the ability to create multiatomicity packets as well that preserves fungibility with v1 packets that haven't migrated over

AdityaSripal commented 1 month ago

Fallback method must still be pulled into feature branch from #7174 before it can be closed

DimitrisJim commented 1 month ago

fallback method was added in #7358, closing!