Repository navigation
Conversation
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().
88a0429 to
7e7f224
Compare
|
@claude review |
There was a problem hiding this comment.
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_versionand the downstairsHereIAmpath. Withsupported()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
YesItsMecheck: a version is accepted only if it converts to aMessageVersionand is insupported(). 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.
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 derivecurrent()from the first entry. Addnegotiate_version(), which picks the newest version that bothsides offer.
The downstairs now replies to
HereIAmwith a negotiated version. The upstairs offers everysupported version when sending
HereIAmand accepts any of the supported versions thatcomes back in the
YesItsMeresponse.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
MessageandCRUCIBLE_MESSAGE_VERSIONto indicate that a newversion is added to
MessageVersion::supported().