Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
ICS4: Add special Localhost ConnectionID for localhost handling #879
ICS4: Add special Localhost ConnectionID for localhost handling #879
Changes from 2 commits
9097d38
ebd5e18
e9ac72e
e98d7fd
cf07e29
2ef32fc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just
localhost
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just kept it to @jtieri initial choice. Seems reasonable to me so you know its a sentinel connectionID as opposed to a sentinel clientID for example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In ibc-go there is connection id validation that requires the id to be in the format
connection-{N}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting but there's no check on if the suffix is a number? Also would this ever get called on the localhost connection anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh actually i was wrong, there is connection id validation that enforces the previous format i mentioned but that does not get called in the channel layer. The validation that gets called in
channel.ValidateBasic
seems to just enforce a character limit between 10-64 solocalhost
fails due to being only 9 chars.https://github.com/cosmos/ibc-go/blob/3e8935e3334cac640a3f03b7a501a72aa6013624/modules/core/04-channel/types/channel.go#L68
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My preference would be
localhost
(adjust the connection validation to allow a lower limit), but I won't push very hard for itThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kinda like it as-is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this idea. It assumes we have different (port, channel, connection) ids on both sides, right?
What if someone makes a channel between
transfer
andtransfer
onconnection-localhost
?Both ends would have the same port and channel ID and it would be impossible to distinguish the two sides of the channel (when inspecting own store).
I think we need something to handle this case (even if that is making such channels illegal and refusing to create them)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The connection ids would be
connection-localhost
on both sides so really just assuming different channel ids.The connections don't actually exist in state, you just check if the connection id is set to
connection-localhost
and since both ends are on the same chain the calls toGenerateChannelIdentifier
inChanOpenInit
&ChanOpenTry
should ensure that we end up with different channel ids.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jtieri is correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They would have different channelIDs, but the same portID and connectionID
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add this as a testcase @jtieri if it doesn't already exist just to prove the above assertion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If they get unique channel-ids, perfect.
I was afraid this would be one channel, but yeah, if they are constructed separately as if they are different chains, they should be eg. channel-7 and channel-8
A test would be good though, as this is a lot of reasoning about a key correctness issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw, I really like this idea. Much simpler and finally adding localhost will be nice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be worth for completeness to at least list all the handlers were these modifications should be done?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea the one problem i see with that approach is that it will keep changing as we add/remove verification methods and this will be hard to keep up-to date.
That's why i think there should be separate verify functions at the channel layer so it doesn't get forgotten by implementors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only nit about the addition to the spec is that it breezes over additional complexity that might need to be handled by implementations with regards to validation for connection related parameters. For example, checking if a packet has timed out. To check if a packet has timed out, we need to check the the timestamp or height associated with a specific proof or consensus state height. This requires referencing the client associated with a connection. Even if the spec doesn't spell out how to do this. It should mention that using a localhost connection might mean performing connection related validation in a different manner depending on the type of connection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious what possible solutions exist for dealing with the gas consumption issue mentioned here. I know in the last ibc call the idea of using
EndBlock
for localhost packets was mentioned, would the same gas consumption solution for the relayer then be able to be applied to eliminate the need for relayers on localhost ibc connections?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use EndBlock, but there are still block gas limits. Its possible it just would require some forethought (perhaps maintain a queue and limit the number of transactions that get processed per block). CCV already has some prior art on this i believe. leaning towards leaving this as-is in the spec, since its very implementation-specific
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think some auto-call will be interesting, but a possible attack vector, and needs to be done very carefully.
But it definitely should be called in a different atomic transaction. My desire for such local host calls is to allow async passing and insulate from rollback/gas. For example, you could send "update messages" when something changed (ideally via pub sub) and listeners wouldn't add extra cost or DoS to the sender, as they need to pay own gas later (and making the issue of relayer incentivization even more pressing)
Anyway, just my two cents. I see you are going in this direction, and it is quite cool.