Add versions to packet and create Eureka packet commitment #6986

AdityaSripal · 2024-07-29T16:41:39Z

Description

TODO:

Use IBC Version to switch between the two hashing functions
Get feedback from port router team on new hashing function
Merge into single entry point for packet msg handler
Parameterize the commitment function. This should be done in a separate PR after some discussion and is not necessary for initial release

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.

DimitrisJim · 2024-07-30T09:52:53Z

proto/ibc/core/channel/v1/channel.proto

@@ -122,6 +129,10 @@ message Packet {
  ibc.core.client.v1.Height timeout_height = 7 [(gogoproto.nullable) = false];
  // block timestamp (in nanoseconds) after which the packet times out
  uint64 timeout_timestamp = 8;
+  // which IBC version to use to commit this packet
+  IBCVersion ibc_version = 9;


Can't we store ibcversion under client id prefixed key in state and look it up when necessary instead of putting it in packet? We could just write a single byte value for it and migrating away from it when classic is dropped would be easier? This would need us to forbid new connections/channels on a eureka client tho I believe.

Just a thought that came to me initially looking at this packet field.

Yes we could do it this way, and I am open to that. There are a couple reasons I like having the IBC Version in the packet.

One minor reason is that we could use the same client for Eureka and Classic. Of course, we could disable this if we choose to in other ways.

The major reason is that I think having these versions explicitly defined in the Packet will make it easier for us to upgrade in the future. If we have ProtocolVersion stored on a per-client basis, the entire chain and all applications on it have to make a synchronous decision to move to the next protocol. If it's a per-packet basis, one application can decide to upgrade if the sender supports the new version. If the receiver doesn't then it just doesn't get received and we time out. i think in general having version in the packet and being permissive to version-related failures to relay will make the system more easily upgradable over time than a prenegotiated version.

cc: @colin-axner

Also this is an enum that is not actually being hashed into the commitment. So it is really taking a small amount of space in the packet. The packet is also only in the transaction bytes, it is not stored on chain except for a couple edge-cases.

AdityaSripal · 2024-07-30T10:44:10Z

modules/core/04-channel/types/packet.go

 // NOTE: sdk.Uint64ToBigEndian sets the uint64 to a slice of length 8.
-func CommitPacket(cdc codec.BinaryCodec, packet Packet) []byte {


The codec was never used in a version of IBC. Think it was used in early pre-launch implementations but got changed before launch. the function signature never changed tho

colin-axner · 2024-07-30T13:00:55Z

proto/ibc/core/channel/v1/channel.proto

+  // which IBC version to use to commit this packet
+  IBCVersion protocol_version = 9;
+  // version which application should use to process the packet data
+  string app_version = 10;


I'm fine with this to move forward, but I feel the app_version should be inside the packet data

I think we can do that in a multi packet data version

colin-axner · 2024-07-30T13:04:04Z

proto/ibc/core/channel/v1/channel.proto

+  // This will be treated as a classic packet
+  IBC_VERSION_UNSPECIFIED = 0;


Hmm, should we assign classic to 0? Would be encoding compliant with the existing structure, I'm fairly sure (empty fields not encoded), but it doesn't matter so much since this is just go api

Proto makes me suffix this enum with UNSPECIFIED. idk if it can be overridden.

Agreed, this is just inside the chain doesn't go over the protocol so it doesn't matter

ibc-go/proto/ibc/core/channel/v1/channel.proto

Line 85 in 4a0da8f

ORDER_NONE_UNSPECIFIED = 0 [(gogoproto.enumvalue_customname) = "NONE"];

#6986 (comment)

colin-axner · 2024-07-30T13:07:36Z

proto/ibc/core/channel/v1/channel.proto

+  // which IBC version to use to commit this packet
+  IBCVersion protocol_version = 9;


The idea here being rather than introspecting the channel-id, we have the sender specify a version?

For other reviewers, you should think of the packet type as our go api and the commitment structure as the spec requirement. 2 ibc implementations could use different internal packet types and still be spec compliant

…a/add-version-to-packet

sangier

I can't spot anything wired, LGTM!

DimitrisJim

love the encompassing commit packet, left some initial comments/qs

DimitrisJim · 2024-07-31T11:17:56Z

modules/core/04-channel/types/packet.go

+	timeoutBuf := sdk.Uint64ToBigEndian(packet.GetTimeoutTimestamp())
+
+	// only hash the timeout height if it is non-zero. This will allow us to remove it entirely in the future
+	if !reflect.DeepEqual(timeoutHeight, clienttypes.ZeroHeight()) {


does height have an Eq in its interface we could use?

DimitrisJim · 2024-07-31T11:18:13Z

modules/core/04-channel/types/packet.go

@@ -35,6 +55,51 @@ func CommitPacket(cdc codec.BinaryCodec, packet Packet) []byte {
 	return hash[:]
 }

+// CommitEurekaPacket returns the Eureka packet commitment bytes. The commitment consists of:
+// sha256_hash(timeout_timestamp + timeout_height.RevisionNumber + timeout_height.RevisionHeight + sha256_hash(data))


mention hasing of version?

DimitrisJim · 2024-07-31T11:18:26Z

modules/core/04-channel/types/packet.go

 // NOTE: sdk.Uint64ToBigEndian sets the uint64 to a slice of length 8.
-func CommitPacket(cdc codec.BinaryCodec, packet Packet) []byte {
+func CommitClassicPacket(packet Packet) []byte {


these two called by CommitPacket should now be able to be made private no?

DimitrisJim · 2024-07-31T11:19:59Z

modules/core/04-channel/types/msgs_test.go

@@ -64,6 +64,7 @@ var (
 	disabledTimeout   = clienttypes.ZeroHeight()
 	validPacketData   = []byte("testdata")
 	unknownPacketData = []byte("unknown")
+	validVersion      = "ics-20"


guessing this is passed someplace as packet app version?

in packet_test.go, just put the vars in one place rather than splitting them across files. We already use these vars in that test file

DimitrisJim · 2024-07-31T11:24:13Z

modules/core/04-channel/types/packet.go

+	buf = append(buf, destinationHash[:]...)
+
+	// hash the version only if it is nonempty
+	if packet.AppVersion != "" {


where would have empty app version? do we need validation for this field?

I don't see any version validation in the handshakes.

I'm not sure if we should be allowing it to be empty. Maybe we shouldn't, but i was thinking a blank version would imply use your default version. So if transfer receives a blank version, it tries to unmarshal data into ics20-v1 packet for example

colin-axner · 2024-07-31T12:34:41Z

proto/ibc/core/channel/v1/channel.proto

+  IBC_VERSION_CLASSIC = 1;
+  // IBC version 2 implements the Eureka protocol
+  IBC_VERSION_EUREKA = 2;


looked over this with @chatton since it overlaps with our work. He made a good point that Eureka name is leaking into the codebase. I think long term, referring to this as v1 and v2 packet commitment structures would be more intuitive. Otherwise you keep adding codenames for every version (which I find to be very confusing)

Agreed, switched to v2 for Eureka and v1 for classic. With this context, I think it still makes sense to leave the 0 enum as UNSPECIFIED but treat it as a v1 packet. What do you think?

I prefer that to zero indexing the IBC_Version_1 enum

colin-axner · 2024-07-31T12:40:20Z

modules/core/04-channel/types/packet.go

+// NOTE: The commitment MUSTs be a fixed length preimage 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 {
+	switch packet.ProtocolVersion {


if we aren't proving the protocol version a relayer could provide the opposite version that a packet was sent on. Unsure how likely the impact would be, but certainly creates some opportunity

if it was a eureka packet would fail on chan lookup and if a classic on counterparty lookup? this is assuming we don't allow chans/conns on eureka clients though, unsure for allowing them.

It would only matter if there was a hash collision between classic packets and eureka packets. Since we changed the commitment algorithm, I don't think that's possible here.

The same exact packet fields with the only thing changed being the ProtocolVersion would lead to different commitments, since the commitment algorithm is different even for the existing fields (and we newly hash dest identifiers)

And additionally two different packets hashing to the same hash by swapping out the ProtocolVersion would break the sha256 preimage collision so we don't need to worry about that.

I have thought about the issue of future maintainer adding a third version. And this third version commits packets in the exact same way as v2. In this case, if the future maintainer is not careful, they can allow relayers to modify the version on receive.

I could signal the possibility of that footgun here by explicitly hashing protocol version. But that would only be a hint for right behavior, since future maintainers may not do it (if they don't change commitment hashes in between the next versions, they will need to hash in the protocol version)

modules/core/04-channel/types/packet.go

srdtrk · 2024-08-01T02:11:35Z

modules/core/04-channel/types/packet.go

+	destinationHash := sha256.Sum256([]byte(packet.GetDestChannel()))
+	buf = append(buf, destinationHash[:]...)
+
+	// hash the version only if it is nonempty


why? What's wrong with hashing the version even if it is empty

We could do this, thought to save on the compute if hash is empty. More relevant for VM implementations

I think it is better not to have branching in the code when not necessary. A single hash isn't gonna make or break it. So can we hash even if it is empty?

DimitrisJim

lgtm, sweet!

sonarqubecloud · 2024-08-01T13:03:20Z

Quality Gate passed for 'ibc-go'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

AdityaSripal · 2024-08-01T13:38:52Z

Merging to not block future PRs. I believe I've answered people's questions. If there are followups, please open new issues or discuss on the eureka channel

AdityaSripal added 2 commits July 29, 2024 18:38

add versions to packet and separate commitment function

5c2c7e7

use IBC Version to switch hashing

6de4765

DimitrisJim reviewed Jul 30, 2024

View reviewed changes

AdityaSripal added 2 commits July 30, 2024 12:04

fix build and tests, found bug in switch logic

da66323

add documentation

e84cdf2

AdityaSripal requested a review from DimitrisJim July 30, 2024 10:34

AdityaSripal commented Jul 30, 2024

View reviewed changes

AdityaSripal changed the title ~~add versions to packet and separate commitment function~~ Add versions to packet and create Eureka packet commitment Jul 30, 2024

AdityaSripal marked this pull request as ready for review July 30, 2024 10:47

AdityaSripal requested review from colin-axner, damiannolan, chatton, crodriguezvega, gjermundgaraba and bznein as code owners July 30, 2024 10:47

improve code docstrings

69bdce8

AdityaSripal added this to the Eureka alpha milestone Jul 30, 2024

This was linked to issues Jul 30, 2024

Introduce Version field into the Packet struct #6971

Closed

Implement new packet hashing for IBC Eureka #6972

Closed

colin-axner reviewed Jul 30, 2024

View reviewed changes

AdityaSripal requested a review from srdtrk July 30, 2024 15:36

Merge branch 'feat/ibc-eureka' of github.com:cosmos/ibc-go into adity…

7e7be96

…a/add-version-to-packet

sangier approved these changes Jul 31, 2024

View reviewed changes

DimitrisJim reviewed Jul 31, 2024

View reviewed changes

colin-axner reviewed Jul 31, 2024

View reviewed changes

address jim review

a594af8

srdtrk reviewed Aug 1, 2024

View reviewed changes

AdityaSripal added the feat: ibc-eureka label Aug 1, 2024

rename eureka to v2

6d72a4b

DimitrisJim approved these changes Aug 1, 2024

View reviewed changes

AdityaSripal merged commit 44215f7 into feat/ibc-eureka Aug 1, 2024
63 of 65 checks passed

AdityaSripal deleted the aditya/add-version-to-packet branch August 1, 2024 13:38

This was referenced Aug 1, 2024

Implement new packet hashing for IBC Eureka #6972

Closed

Introduce Version field into the Packet struct #6971

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add versions to packet and create Eureka packet commitment #6986

Add versions to packet and create Eureka packet commitment #6986

AdityaSripal commented Jul 29, 2024 •

edited

Loading

DimitrisJim Jul 30, 2024

AdityaSripal Jul 30, 2024 •

edited

Loading

AdityaSripal Jul 30, 2024

AdityaSripal Jul 30, 2024

colin-axner Jul 30, 2024

AdityaSripal Jul 30, 2024

colin-axner Jul 30, 2024

AdityaSripal Jul 30, 2024

colin-axner Jul 31, 2024

AdityaSripal Jul 31, 2024

colin-axner Jul 30, 2024

sangier left a comment

DimitrisJim left a comment

DimitrisJim Jul 31, 2024

DimitrisJim Jul 31, 2024

DimitrisJim Jul 31, 2024

DimitrisJim Jul 31, 2024

AdityaSripal Jul 31, 2024

DimitrisJim Jul 31, 2024 •

edited

Loading

AdityaSripal Jul 31, 2024

colin-axner Jul 31, 2024

AdityaSripal Jul 31, 2024

colin-axner Jul 31, 2024 •

edited

Loading

DimitrisJim Jul 31, 2024 •

edited

Loading

AdityaSripal Jul 31, 2024

srdtrk Aug 1, 2024

AdityaSripal Aug 1, 2024

srdtrk Aug 2, 2024

DimitrisJim left a comment

sonarqubecloud bot commented Aug 1, 2024

AdityaSripal commented Aug 1, 2024

		// NOTE: sdk.Uint64ToBigEndian sets the uint64 to a slice of length 8.
		func CommitPacket(cdc codec.BinaryCodec, packet Packet) []byte {

		// This will be treated as a classic packet
		IBC_VERSION_UNSPECIFIED = 0;

		// which IBC version to use to commit this packet
		IBCVersion protocol_version = 9;

Add versions to packet and create Eureka packet commitment #6986

Add versions to packet and create Eureka packet commitment #6986

Conversation

AdityaSripal commented Jul 29, 2024 • edited Loading

Description

Choose a reason for hiding this comment

AdityaSripal Jul 30, 2024 • edited Loading

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

sangier left a comment

Choose a reason for hiding this comment

DimitrisJim 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

DimitrisJim Jul 31, 2024 • edited Loading

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 Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

DimitrisJim Jul 31, 2024 • edited Loading

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

DimitrisJim left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Aug 1, 2024

Quality Gate passed for 'ibc-go'

AdityaSripal commented Aug 1, 2024

AdityaSripal commented Jul 29, 2024 •

edited

Loading

AdityaSripal Jul 30, 2024 •

edited

Loading

DimitrisJim Jul 31, 2024 •

edited

Loading

colin-axner Jul 31, 2024 •

edited

Loading

DimitrisJim Jul 31, 2024 •

edited

Loading