[Signer] Message versioning?

[jcnelson]

I didn't get a chance to ask on the review of #5466, but since the wire format of BlockAccept is changing, should the wire format itself be updated to be versioned? Not all signers will upgrade at the same time; signers with the new format in #5466 will produce BlockAccepts that (1) other signers can't decode and (2) older nodes can't decode.

[hstove]

I'm totally on board with a version byte, but I'd note that we're a bit lucky in this case in that the only time we're reading a list of signer messages is from reading StackerDBChunksEvent, where each chunk just has data, and then each chunk's data is deserialized into a SignerMessage.

So, when it comes to the question of "should nodes/signers completely ignore signer messages with a future version", I think there may be some debate, vs trying to be as backwards-compatible as possible.

Another option is to add an explicit bytes_len field to consensus_serialize to prevent the issues around deserializing a list, but that, too, wouldn't be strictly backwards compatible.

[jferrant]

I do think we need signer versioning, but I imagine the version number should be the very first byte of the message. This will not be backwards compatible and would require us to update all our signers/miners if we add it now.

Like Hank mentioned, we are lucky in the sense that we always just attempt to consensus deserialize whatever SignerMessage our signer/node version expects and we have enforced backwards compatibility by setting the missing timestamp value to u64::MAX. This def is not a good long term solution, but should we plan this addition of a version number to be part of a hardfork improvement?

[hstove]

I think there are a few things here:

• We're adding new data to the end of the bytes. This is fine when deserializing a single message, but this is an issue if you tried to deserialize a Vec<SignerMessage>.
• We don't ever (I think?) actually deserialize a Vec<SignerMessage>, we just have Vec<StackerDBChunksData>, where changing the serialization code in SignerMessage doesn't break things.
• In this case I think we want to be able to change the struct without breaking un-upgraded miners & signers. Otherwise, this can easily lead to a chain stall. Similarly, if we have a scheme like "If version > MAX_VERSION, ignore" would cause a similar stall scenario.

With all that said, I think we should do a few things now:

• Add a version byte at the end of the existing serialization (to not break existing code)
• Add a bytes_len field to the end of consensus_serialize as well
• When deserializing, if we "know" the version specified, deserialize it using the "normal" path. Otherwise, if we don't "know" this version, just read until bytes_len. This ensures backwards compatibility (at least from the point on where nodes know how to handle bytes_len.

If we seriously wanted to have a scheme like "nodes ignore signer messages with a higher version", we need to implement an "activation block" mechanism, so that nodes/signers all upgrade at the same time. I'd like to avoid this if possible.