First version (ABCI only ATM) of documenting protobuf version translation#780
First version (ABCI only ATM) of documenting protobuf version translation#780sergio-mena wants to merge 2 commits intofeature/proto-upgradefrom
Conversation
| //---------------------------------------- | ||
| // Request types | ||
|
|
||
| // Handling v1 as v2: REJECT |
There was a problem hiding this comment.
Just to clarify, does this mean that, if we receive v1.Request, but we're expecting v2.Request, we reject it?
There was a problem hiding this comment.
Exactly.
And "reject" means:
- either "it doesn't make sense" (this is the case here)
- or it is an error condition
Let me know if you think it is worth distinguishing between REJECT and ERROR (I'm not doing it ATM)
There was a problem hiding this comment.
Great, so my intuition was correct. And I think "REJECT" is fine. Thanks!
thanethomson
left a comment
There was a problem hiding this comment.
I'd be interested to know what @mzabaluev thinks of this convention.
|
I think we need to build more understanding as to what these different versions exactly mean in terms of protocol messages on the wire and language bindings. In general, assuming backward-compatible definitions, there is no way to distinguish which version the sender used to encode the message. The only way it could be feasible is to detect when new fields are present (meaning that the sender's definition is newer than that used by the recipient), or when reserved fields used in older versions are present (meaning that the sender used an older definition). But by design of most protobuf stacks, it's not possible to deduce this information in the bindings that receive decoded messages; furthermore, it's discouraged by the general idea of evolving protobuf protocols. Unless you use some message metadata/reflection capabilities (not available in Rust/prost), you get no idea if the sender used a different version to encode since the unknown and reserved fields are ignored by the decoder, and the newly added enum variants are mapped to the zero value. |
There was a problem hiding this comment.
In light of the above comment, I don't think most of this information is applicable in practice. The compatibility comments should be limited to handling situations when a field added in a newer version of the message is missing or set to the zero value (these conditions are not expected to be practically distinguishable by the proto consumer), possibly meaning that the sender used an older version to encode.
|
@mzabaluev I mostly agree with your reasoning, however your reasoning is focusing on marshalling/unmarshalling of protobufs. This PR is not really about that, but rather domain knowledge a developer needs if they are not expert in those protobufs. For instance, it would help someone writing code that handles more than one version at once. I believe that's precisely what you're doing in cometbft/tendermint-rs#1312. Further, these lines contain the information you had to ask over Slack a few days ago (and rightly so, because the info was missing). My impression is that the word "handle" used in the text added in this PR is somewhat misleading. Maybe:
I guess it's best if we discuss this synchronously. |
|
Yes, the added comments should perhaps provide a migration guide between versions, in cases when just following the usual protobuf rules is not enough (if you can make it good by just filling the newly added fields with zero values, no additional information is needed). In the present form, it's not clear to me how the application developer should make use of the provided rules. |
|
In the specific case of ABCI, the application should be instructed to handle |
Exactly. That's what all the REJECTs I wrote for those mean... the REJECT means "don't even try to mix up versions of these messages". But there's more: the example I gave you is missing info without this PR. Also, some messages were renamed, and without this PR, it is not obvious for someone examining the various versions for ABCI protobufs. |
| //---------------------------------------- | ||
| // Blockchain Types | ||
|
|
||
| // Handling v1 as v3: ACCEPT |
There was a problem hiding this comment.
Differences from v1 to v3
|
|
||
|
|
||
| // Handling v1 as v2: ACCEPT | ||
| // SET abci_version:v2 TO "<NOT PROVIDED>" |
There was a problem hiding this comment.
| // SET abci_version:v2 TO "<NOT PROVIDED>" | |
| // follow normal protobuf rules |
There was a problem hiding this comment.
(from discussionwith @mzabaluev): Consider removing this, as this is considered as the default (so only document the "REJECT"s as counter-intuitive
| // Misc. | ||
|
|
||
| // v1 type: LastCommitInfo | ||
| // Handling v1 as v2: ACCEPT |
There was a problem hiding this comment.
A lonely "ACCEPT"
- not only means the two types are identical
- it should also means (for messages that are not identical) that normal protobuf rules should be followed
| string abci_version = 4; | ||
| } | ||
|
|
||
| // Handling v1 as v2: REJECT |
There was a problem hiding this comment.
| // Handling v1 as v2: REJECT | |
| // Handling v1 as v2: REJECT -- [explain why] |
everywhere
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
We decided not to pursue this approach |
Contributes to #95
This is my first attempt at documenting:
... and the same for interpreting a protobuf message from the current version while in the previous version.
This should be helpful for integrators that, e.g., need to work with two versions at the same time. They could take advantage of these annotations in order to construct domain types that would support the required versions of the protos.
I kind of invented some keywords along the way, with the intention of keeping the text concise. Those keywords should be self-explanatory. Let me know if that's not the case, and I'll add some explanations in the README.
Feedback is more than welcome. I have only done ABCI since I'd like to gather feedback to then proceed with the rest of the proto files.
PR checklist
.changelog(we use unclog to manage our changelog)docs/orspec/) and code comments