Skip to content

protocol: negotiate the message version from a supported set - #2047

Open
leftwo wants to merge 1 commit into
alan/been-active-v14-statefrom
alan/been-active-v14-negotiate
Open

leftwo wants to merge 1 commit into
alan/been-active-v14-statefrom
alan/been-active-v14-negotiate

Conversation

@leftwo

@leftwo leftwo commented Oct 15, 2026

Copy link
Copy Markdown
Contributor

More building up to the protocol change, getting the framework in place to support multiple version.

Add MessageVersion::supported(). This is the versions this build can speak, newest first. We derive
current() from the first entry. Add negotiate_version(), which picks the newest version that both
sides offer.

The downstairs now replies to HereIAm with a negotiated version. The upstairs offers every
supported version when sending HereIAm and accepts any of the supported versions that
comes back in the YesItsMe response.

The supported set is still just V13, so nothing changes on the wire. V14 will be added to it once both
sides handle the V14 messages.

Update the comments around Message and CRUCIBLE_MESSAGE_VERSION to indicate that a new
version is added to MessageVersion::supported().

@leftwo
leftwo requested a review from jmpesp October 15, 2026 17:57
@leftwo
leftwo added this pull request to stack #2045 October 15, 2026 17:57
More building up to the protocol change, getting the framework
in place to support multiple version.

Add MessageVersion::supported(). This is the versions this build
can speak, newest first.  We derive current() from the first entry.
Add negotiate_version(), which picks the newest version that both sides
offer.

The downstairs now replies to HereIAm with a negotiated version.
The upstairs offers every supported version in HereIAm and accepts
 any of them in YesItsMe response.

The supported set is still just V13, so nothing changes on the wire.
V14 will be added to it once both sides handle the V14 messages.

Update the comments around Message and CRUCIBLE_MESSAGE_VERSION to
say that a new version is added to MessageVersion::supported().
@leftwo
leftwo force-pushed the alan/been-active-v14-negotiate branch from 88a0429 to 7e7f224 Compare October 15, 2026 21:33
@leftwo

leftwo commented Oct 16, 2026

Copy link
Copy Markdown
Contributor Author

@claude review

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I reviewed this PR and didn't find any bugs. It changes how the upstairs and downstairs agree on a protocol version during the handshake, and it sets up how V14 will be added later, so a human look would still be worthwhile.

What was reviewed:

  • negotiate_version and the downstairs HereIAm path. With supported() still only [V13], the bytes on the wire are the same as before. Pre-change peers in both directions still settle on V13: an old upstairs sends 13 with no alternates, and an old downstairs accepts 13 from the alternates.
  • The upstairs YesItsMe check: a version is accepted only if it converts to a MessageVersion and is in supported(). The new test covers V12, which is a real version but older than anything we support.
  • The upstairs only logs the negotiated version and doesn't store it. That's fine while only one version exists, but it will need to be kept once V14 messages depend on it.
Extended reasoning...

The change touches the protocol crate's version handling, the downstairs HereIAm/YesItsMe handshake, and the upstairs negotiation state machine. It has no auth, crypto or injection surface. The change is behavior-preserving today because only V13 is supported, and new unit tests cover ordering, the TryFrom round trip and negotiation. I deferred rather than approved because this is the core connection handshake of a storage system and it lays down the framework for supporting more than one version, which is a design choice the maintainers should weigh in on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant