Skip to content
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

ICS 3: Connection Semantics #32

Merged
merged 84 commits into from May 21, 2019
Merged

Conversation

cwgoes
Copy link
Contributor

@cwgoes cwgoes commented Mar 7, 2019

Closes #3.

Specify datagrams & semantics for bidirectional connections.

Unidirectional connections (one-to-one, or one-to-many, or even many-to-one) are possible but will be specified separately.

@cwgoes cwgoes added tao Transport, authentication, & ordering layer. wip Issues or pull requests which are in progress. labels Mar 7, 2019
@cwgoes cwgoes changed the title WIP: ICS 3: Connection Semantics & Lifecycle ICS 3: Connection Semantics & Lifecycle Mar 7, 2019
@cwgoes cwgoes force-pushed the cwgoes/ics-3-connection-semantics branch from e3fb4a5 to 50322ef Compare March 19, 2019 13:54
spec/ics-3-connection-semantics/README.md Outdated Show resolved Hide resolved
spec/ics-3-connection-semantics/README.md Outdated Show resolved Hide resolved
spec/ics-3-connection-semantics/README.md Outdated Show resolved Hide resolved
spec/ics-3-connection-semantics/README.md Outdated Show resolved Hide resolved
spec/ics-3-connection-semantics/README.md Outdated Show resolved Hide resolved
spec/ics-3-connection-semantics/README.md Outdated Show resolved Hide resolved
spec/ics-3-connection-semantics/README.md Outdated Show resolved Hide resolved
spec/ics-3-connection-semantics/README.md Outdated Show resolved Hide resolved
spec/ics-3-connection-semantics/README.md Outdated Show resolved Hide resolved
spec/ics-3-connection-semantics/README.md Outdated Show resolved Hide resolved
@cwgoes
Copy link
Contributor Author

cwgoes commented Mar 28, 2019

Further whiteboarding on ICS 3:

overview

connopen

@cwgoes cwgoes changed the title ICS 3: Connection Semantics & Lifecycle ICS 3: Connection Semantics Mar 29, 2019
@cwgoes cwgoes added ready-for-review Pull requests which are ready for review. and removed wip Issues or pull requests which are in progress. labels May 13, 2019
@cwgoes
Copy link
Contributor Author

cwgoes commented May 13, 2019

Going to make the diagram fancier and refer to two chains, but the substance is here.

\node at (-0.5,1.5) [above=0.5mm, right=5mm] {\textsc{Connection State Machine}};

\end{tikzpicture}
\end{document}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cwgoes
Copy link
Contributor Author

cwgoes commented May 14, 2019

  • Also needs timeout recovery on the timing-out chain

@cwgoes
Copy link
Contributor Author

cwgoes commented May 14, 2019

Problem: what to do if we start initializing a channel, then final connection handshake times out?

Maybe we can just delete the channel, the channel handshake will not complete.

Concerns about name collisions?

We could enforce relations between the timeout heights.

@cwgoes cwgoes changed the title ICS 3: Bidirectional Connection Semantics ICS 3: Connection Semantics May 17, 2019
@cwgoes
Copy link
Contributor Author

cwgoes commented May 17, 2019

  • Reread for consistency
  • Simplify object notation

@cwgoes
Copy link
Contributor Author

cwgoes commented May 17, 2019

  • Ensure correct description of module interface

This ICS defines three subprotocols: opening handshake, header tracking, closing handshake. Datagrams defined herein are handled as external messages by the IBC relayer module defined in [ICS 26](../spec/ics-26-relayer-module).

![State Machine Diagram](state.png)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice diagram :) Could you make the text on transitions a bit bigger? It doesn't read easy currently.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, IMHO it would be clearer if you'd separate A and B FSMs. And separate the opening and closing subprotocols. In other words, have one diagram for connection establishment, showing FSM for A on the left, FSM for B on the right and the corresponding datagrams used in the communication between the two.
Same for closing connections.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback, makes sense - will do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Altered the font sizing. I'm not sure separating the FSMs for A and B is the right direction, though - the goal of the protocol construction is to allow reasoning about the combined state of the IBC handlers on both chains at once, so it seems like we want to combine them...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...I agree we want to look at chains' FSMs at once. But the current diagram seem to show some transitions upon events that are not valid one one side. Let me try to read it more carefully the way it is together with the code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should list the events in the FSM (CONNINIT, CONNOPENTRY, etc) and describe what triggers them. Maybe add also a column in the table below?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added two columns in each table, for prior state and posterior state - is that what you meant?

desiredCounterpartyIdentifier Identifier
clientIdentifier Identifier
counterpartyClientIdentifier Identifier
nextTimeoutHeight uint64
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a few words in the definition section about timeout heights.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if the explanation in #86 should actually become an ICS...

@cwgoes
Copy link
Contributor Author

cwgoes commented May 21, 2019

Merging as a draft, future comments are always welcome.

@cwgoes cwgoes merged commit 3304938 into master May 21, 2019
@cwgoes cwgoes deleted the cwgoes/ics-3-connection-semantics branch May 21, 2019 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-review Pull requests which are ready for review. tao Transport, authentication, & ordering layer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICS 3: IBC connection lifecycle, protocol negotation handshake
9 participants