-
Notifications
You must be signed in to change notification settings - Fork 396
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
Changes from all commits
9097d38
ebd5e18
e9ac72e
e98d7fd
cf07e29
2ef32fc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,76 @@ | ||
# Localhost Support in 04-channel | ||
|
||
## Rationale | ||
|
||
IBC provides the ICS-26 interface for two mutually-untrusted applications to negotiate a communication channel and start sending messages to each other. This interface is now widely adopted by applications (both smart contracts and static modules) so that they can speak to applications on remote chains. However, there is no way to reuse this interface to communicate with applications on the same chain. So applications that wish to communicate with counterparties on remote and local state machines must for the moment have two separate implementations to handle both cases. This is a dev-UX issue that we can solve by providing localhost functionality (analogous to localhost for the internet protocol). | ||
|
||
One approach is to implement this feature at the client layer. However this leads to a number of inefficiencies. Firstly, it requires the creation of a connection (handshake messages mediated by relayers) to be sent from the state machine back to itself in order to connect. It then requires that state that exists at the channel layer be routed all the way into the client so that it can then be inspected for verification purposes. Lastly, it does not immediately lend itself to future improvements that might make localhost packet flows atomic. | ||
|
||
The state that applications wish to verify about their counterparties are all stored in the channel store (i.e. `channelEnd`, `packet_commitment`, `receipt`, `acknowledgement`). Thus, the simplest approach is to add the special localhost handling here at the channel layer, rather than propogating it all the way down the stack. This provides the uniform interface that applications are looking for with minimal changes to the core handlers. | ||
|
||
## 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. | ||
|
||
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. Choose a reason for hiding this commentThe 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 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 commentThe reason will be displayed to describe this comment to others. Learn more.
The connection ids would be
The connections don't actually exist in state, you just check if the connection id is set to There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe 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 |
||
|
||
Here is an example of how this would be implemented for the `recvPacket` handler: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 |
||
|
||
```typescript | ||
function recvPacket(packet: OpaquePacket, | ||
proof: CommitmentProof, | ||
proofHeight: Height, | ||
relayer: string): Packet { | ||
// pre-verification logic | ||
|
||
// get the underlying connectionID for this channel | ||
channel = provableStore.get(channelPath(packet.destPort, packet.destChannel)) | ||
connectionID = channel.connectionHops[0] | ||
|
||
//construct the expectedState and keypath | ||
expectedState = commitPacket(packet) | ||
keyPath = packetCommitmentPath(packet.sourcePort, packet.sourceChannel, packet.Sequence) | ||
|
||
|
||
verifyPacketCommitment(connectionID, keyPath, expectedState, proof, proofHeight) | ||
AdityaSripal marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// post-verification logic | ||
|
||
return packet | ||
} | ||
|
||
// private function created to abstract the verification logic of localhost and remote chains from the rest of channel logic | ||
// | ||
// 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. Choose a reason for hiding this commentThe 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 |
||
if connectionID == LocalhostConnection { | ||
// verify packet by checking if the expected state | ||
// exists in our own state at the given keyPath | ||
// NOTE: proof and proofHeight are ignored in this case | ||
actualState = provableStore.get(keyPath) | ||
abortTransactionUnless(expectedState === actualState) | ||
} else { | ||
abortTransactionUnless(connection !== null) | ||
abortTransactionUnless(connection.state === OPEN) | ||
|
||
abortTransactionUnless(connection.verifyPacketData( | ||
proofHeight, | ||
proof, | ||
packet.sourcePort, | ||
packet.sourceChannel, | ||
packet.sequence, | ||
expectedState, | ||
)) | ||
} | ||
} | ||
``` | ||
|
||
## Relayer Messages | ||
|
||
Relayers supporting localhost packet flow must be adapted to submit messages from sending applications back to the originating chain. | ||
|
||
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. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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. |
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