Repository navigation
Conversation
There was a problem hiding this comment.
I'm Approving because my concerns aren't about the logic, just the comments.
But please do address the flagged comments, the PR description, and the git commit message too before merging.
ordered so the numbering says something
In my experience, this ordering can't really have meaning, over the long-term. When we add messages to mini protocols, we simply allocate the next tag number. That's the simplest way to avoid unnecessarily spoiling backwards-compatibility/excessive version-based conditionals/messy codec impls/etc.
So I hesitate to emphasize the order. Even this PR on seems like unnecessary tidying to me: ultimately, no one should actually care about the tag numbers other than them being unique and we can't promise those numbers will be "nice and meaningful" forever (without obligating the devs/codecs to unnecessary versioning dances).
But: I don't object to this PR, it's harmless on the testnet and it is pleasant/beneficial for the first iteration to avoid surprises (eg nonsense tags). My only objection is the PR description and a few small comments claiming that some order is particularly useful/appropriate.
ea318bf to
9f4b563
Compare
9f4b563 to
3cea3a5
Compare
9d9d05a to
27e0cb3
Compare
This is purely cosmetic and just fits as we are changing the mini-protocols for a good reason anyways.
27e0cb3 to
c54515e
Compare
Both protocols had holes. LeiosNotify ran 0-7 only because MsgQuit and MsgCanceled were appended after MsgDone; LeiosFetch used 0-3 and then 9, with 6-8 held for batch messages that were never built.Each now numbers from zero contiguously, ordered so the numbering says something. Termination comes first in both. Then, in LeiosNotify, the request and the responses it can draw, with votes last so that moving them to a votes mini-protocol later costs no renumbering. In LeiosFetch, each request sits beside the reply it draws.
The commented-out batch and vote placeholders go with them: never implemented, and no longer planned.
This is a wire-format change on both mini-protocols, so every node has to adopt it together.
This is purely cosmetic and just fits as we are changing the
mini-protocols for a good reason anyways.