Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: use versioned signer message data #5489

Merged

Conversation

hstove
Copy link
Contributor

@hstove hstove commented Nov 21, 2024

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 hstove requested review from a team as code owners November 21, 2024 01:10
@hstove hstove requested review from jcnelson and jferrant November 21, 2024 01:10
Copy link
Contributor

@obycode obycode left a comment

Choose a reason for hiding this comment

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

Nice!

Copy link
Member

@jcnelson jcnelson left a comment

Choose a reason for hiding this comment

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

In addition to using Vec<T: StacksMessageCodec>, is there a plan to make this new serialization code backwards-compatible? If so, then please also add it. If not, then let's figure out a way to ship this so that all signers upgrade at the same time, since new signers won't be able to decode old signers' messages and vice versa.

@hstove
Copy link
Contributor Author

hstove commented Nov 21, 2024

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 👍🏼

@hstove hstove requested a review from jcnelson November 22, 2024 01:10
@hstove hstove linked an issue Nov 22, 2024 that may be closed by this pull request
Copy link
Collaborator

@jferrant jferrant left a comment

Choose a reason for hiding this comment

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

Noice. LGTM!

@hstove hstove merged commit 78bcf0a into feat/time-based-tenure-extend Nov 22, 2024
1 check passed
@hstove hstove deleted the feat/block-response-data-versioned branch November 22, 2024 20:35
@blockstack-devops
Copy link
Contributor

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@stacks-network stacks-network locked as resolved and limited conversation to collaborators Nov 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Signer] Message versioning?
5 participants