ICS4: Add special Localhost ConnectionID for localhost handling #879
Conversation
AdityaSripal
requested review from
mpoke,
cwgoes and
colin-axner
as code owners
|
|
||
| Implementations will reserve a special connectionID: `connection-localhost` which applications can build their channels on in order to communicate with a counterparty on the same state machine. | ||
|
|
||
| In all channel verification methods, the channel logic must first check that the connectionID is the sentinel localhost connectionID: `connection-localhost`. If it is, then we simply introspect our own channel store to check if the expected state is stored at the expected key path. If the connection is not a localhost, then we delegate verification to the connection layer and ultimately the counterparty client so it can verify that the expected state is stored at the expected key path on the remote chain. |
There was a problem hiding this comment.
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 and transfer on connection-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.
I like this idea. It assumes we have different (port, channel, connection) ids on both sides, right?
The connection ids would be connection-localhost on both sides so really just assuming different channel ids.
What if someone makes a channel between
transferandtransferonconnection-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).
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 to GenerateChannelIdentifier in ChanOpenInit & ChanOpenTry should ensure that we end up with different channel ids.
There was a problem hiding this comment.
What if someone makes a channel between transfer and transfer on connection-localhost?
They would have different channelIDs, but the same portID and connectionID
There was a problem hiding this comment.
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.
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.
btw, I really like this idea. Much simpler and finally adding localhost will be nice
|
|
||
| This would require first checking the underlying connectionID on any channel-level messages. If the underlying connectionID is `connection-localhost`, then the relayer must construct the message with an empty proof and proofHeight and submit the message back to the originating chain. | ||
|
|
||
| Implementations **may** choose to implement localhost such that the next message in the handshake or packet flow is automatically called without relayer-driven transactions. However, implementors must take care to ensure that automatic message execution does not cause gas consumption issues. |
There was a problem hiding this comment.
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.
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.
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.
|
|
||
| In all channel verification methods, the channel logic must first check that the connectionID is the sentinel localhost connectionID: `connection-localhost`. If it is, then we simply introspect our own channel store to check if the expected state is stored at the expected key path. If the connection is not a localhost, then we delegate verification to the connection layer and ultimately the counterparty client so it can verify that the expected state is stored at the expected key path on the remote chain. | ||
|
|
||
| Here is an example of how this would be implemented for the `recvPacket` handler: |
There was a problem hiding this comment.
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.
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
|
|
||
| ## Technical Specification | ||
|
|
||
| Implementations will reserve a special connectionID: `connection-localhost` which applications can build their channels on in order to communicate with a counterparty on the same state machine. |
There was a problem hiding this comment.
Why not just localhost?
There was a problem hiding this comment.
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.
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.
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.
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 so localhost fails due to being only 9 chars.
There was a problem hiding this comment.
My preference would be localhost (adjust the connection validation to allow a lower limit), but I won't push very hard for it
There was a problem hiding this comment.
I kinda like it as-is
Co-authored-by: Carlos Rodriguez <[email protected]>
|
@ethanfrey Would like your input on the choice to bypass the lower layers and do verification for apps right here in channel where the state already exists as opposed to creating a client and going down all the layers and doing additional unnecessary checks (tho it is perhaps more in line with the original design and thinking) |
…nto aditya/localhost-connection
I like the idea for using an optimisation for the localhost loopback. The "magic name" of connection-localhost is well-chosen to be clear and avoid collisions, but some might object to a magic name here, rather than some "magic client type". I am not one of them. My only point is the channel handshake and packet events should be the same, so it should be as easy as possible for a relayer to relayer localhost (maybe just remove the assertion that both sides are not the same and configure to relay only on |
| // NOTE: It currently takes in a packet so we can call the connection verify function appropriately | ||
| // however, a future refactor that abstracts verification at the connection layer | ||
| // may remove the need for this additional argument. | ||
| function verifyPacketCommitment(connectionID: string, packet: Packet, keyPath: bytes, expectedState: bytes, proof: CommitmentProof, proofHeight: Height) { |
There was a problem hiding this comment.
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
|
Closing in favour of #936. |
Allows applications to use IBC to talk to each other on the same statemachine
Dependent on #878 merging to reduce the extra file diff