stacks-network / stacks-core

The Stacks blockchain implementation
https://docs.stacks.co
GNU General Public License v3.0
3.01k stars 672 forks source link

feat: use versioned signer message data #5489

Open hstove opened 1 day ago

hstove commented 1 day ago

This essentially implements the design I commented in the related issue.

To ensure that, when deserializing, we always consume the full "intended" bytes, this struct appends an internal bytes_len in the serialization format. Any remaining bytes are consumed and included as unknown_bytes.

We really want to maintain backwards compatibility here, because it's bad if nodes/signers can't deserialize newer messages. To handle this, any future properties to BlockResponseData can be simply written/read from inside inner_consensus_serialize and consensus_deserialize. As long as new properties are only appended, and that these new properties can have a default value, this design should last through future versions.

Apologies for not implementing it this way when I added the signer version to block responses. I hadn't considered the need to ensure consistency in the amount of bytes consumed when dealing with different versions.

hstove commented 19 hours ago

since new signers won't be able to decode old signers' messages and vice versa

There is a unit test to ensure that new signers can decode old signer messages, which uses a fixture.

Old signers can decode new messages, with the problem that they don't fully consume the serialized bytes. We actually already inadvertently introduced this problem when adding signer versions to block responses, and no problems came from it, because of what I've mentioned before about how we only ever decode a Vec<StackerDBChunksData> and not any Vec<SignerMessage> or Vec<BlockResponse>. I mainly just want to avoid the extra complexity, both in code and release coordination, of adding an activation block.

I'll resolve those other comments though, good points 👍🏼