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

Use versioned handshake when connecting hydra-nodes #1010

Closed
ghost opened this issue Aug 4, 2023 · 1 comment · Fixed by #1381
Closed

Use versioned handshake when connecting hydra-nodes #1010

ghost opened this issue Aug 4, 2023 · 1 comment · Fixed by #1381
Labels
amber ⚠️ Medium complexity or partly unclear feature L2 Affect off-chain part of the Head protocol/network 💬 feature A feature on our roadmap
Milestone

Comments

@ghost
Copy link

ghost commented Aug 4, 2023

Why

Users should be able to tell whether nodes are able to communicate before opening a Head (i.e. spending ADAs to post transactions on-chain)

What

Make the networking stack detect when incompatible versions are used between nodes.

Incompatible version means

  • messages are not understood by next/previous version (does not round trip)
  • a new message had been added, which is ignored by older versions (= additions are also breaking)

Scope:

  • When a hydra-node realizes it is on a different version than a peer, it logs an error and an API server output PeerProtocolVersionMismatch is produced analogous to PeerDisconnected.
    • The hydra-tui displays this as an error and has the mismatching peer listed with a red color.
  • A hydra-node only supports a single network protocol version.

How

Also related to #1080

  • Improve our ouroboros-network stack to using a versioned handshake protocol with a description of which versions of the Hydra off-chain protocol are supported by each party will allow the hydra-node to fail earlier and provide better information to operators on the reasons why the hydra head cannot operate.

Alternative (unlikely feasible):

  • Switch to another network stack that provides a similar version-ed messaging mechanism
@ghost ghost added 💭 idea An idea or feature request network ux Related to user experience L2 Affect off-chain part of the Head protocol/network and removed 💭 idea An idea or feature request labels Aug 4, 2023
@ch1bo ch1bo added the 💭 idea An idea or feature request label Aug 4, 2023
@ch1bo ch1bo added task Subtask of a bigger feature. and removed 💭 idea An idea or feature request labels Aug 8, 2023
@ch1bo ch1bo added 💬 feature A feature on our roadmap and removed task Subtask of a bigger feature. labels Apr 2, 2024
@ch1bo ch1bo moved this to Next in Hydra Head Roadmap Apr 2, 2024
@ch1bo ch1bo added this to the 1.0.0 milestone Apr 2, 2024
@ch1bo ch1bo added amber ⚠️ Medium complexity or partly unclear feature and removed network ux Related to user experience labels Apr 2, 2024
@locallycompact locallycompact self-assigned this Apr 3, 2024
@locallycompact locallycompact linked a pull request Apr 3, 2024 that will close this issue
@ch1bo
Copy link
Member

ch1bo commented Apr 9, 2024

Discussed this in grooming today:

  • To make this testable, we should not follow the wrong path of how the Connectivity messages are wired in here:
    image
  • As the version mismatch is depending on a non-configurable version, this would become impossible to test from an end-to-end integration point and no other test suite includes Hydra.Node.Run (where the Connectivity messages are wired right now)
  • Instead, we should bite the bullet of changing the NetworkInput to the HeadLogic layer to also these "control messages" and handle their mapping to ServerOutput in the HeadLogic (or a wrapping NodeLogic - they have nothing to do with the head state)

@ch1bo ch1bo moved this from Next to Now in Hydra Head Roadmap Apr 10, 2024
@ch1bo ch1bo modified the milestones: 1.0.0, 0.17.0 Apr 15, 2024
@github-project-automation github-project-automation bot moved this from Now to Done in Hydra Head Roadmap May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
amber ⚠️ Medium complexity or partly unclear feature L2 Affect off-chain part of the Head protocol/network 💬 feature A feature on our roadmap
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants