abci: Make ABCI and P2P wire-level length delimiters consistent #9182
Merged
thanethomson merged 5 commits intomainfrom Aug 11, 2022
Merged
abci: Make ABCI and P2P wire-level length delimiters consistent #9182thanethomson merged 5 commits intomainfrom
thanethomson merged 5 commits intomainfrom
Conversation
f605f12 to
23e08c9
Compare
cmwaters
approved these changes
Aug 8, 2022
c3154bf to
fbd89f7
Compare
tychoish
reviewed
Aug 9, 2022
UPGRADING.md
Outdated
|
|
||
| * In v0.34, messages on the wire used to be length-delimited with `int64` | ||
| values, which was inconsistent with the `uint64` length delimiters used in the | ||
| P2P layer. Both now consistently use `uint64` length delimiters. |
Contributor
There was a problem hiding this comment.
I think this makes the change more confusing: because what we're doing is just using varint encoding rather than encoding (effecitively) two fixed width integers. In this statement we say "here's the thing that we changed" and not "here's what you need to do (update your socket client implementation.)"
Reading this, my gut says "but aren't uint64 and int64 integers encoded the same on all twos-compliment machines?" and while this is true, the change is actually about varint encoding.
Contributor
Author
There was a problem hiding this comment.
We were using varint encoding before, but we were varint-encoding an int64 value instead of a uint64 value. The two types of varint encodings produce different values on the wire, making them incompatible.
Client libraries (such as the one in Rust) usually support the uint64 varint encoding, and not int64, meaning that if anyone wants to interface with ABCI via TSP they need to roll their own length-prefixed decoding mechanism.
I'll clarify here.
31680d3 to
fd7e843
Compare
Migrate ABCI to use protoio (uint64 length delimiters) instead of int64 length delimiters to be consistent with the approach used in the P2P layer. Closes: #5783
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
…ing change Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
fd7e843 to
cff8f6b
Compare
sergio-mena
reviewed
Aug 11, 2022
|
|
||
| The benefit of using this `varint` encoding over the old version (where integers were encoded as `<len of len><big endian len>` is that | ||
| it is the standard way to encode integers in Protobuf. It is also generally shorter. | ||
| Tendermint Socket Protocol is an asynchronous, raw socket server which provides |
Contributor
There was a problem hiding this comment.
Could we mark somewhere that a similar change will be needed in the "abci++" folder that @jmalicevic is working on (see #9201)?
@jmalicevic , maybe you could extend the issue's description with an extra checkbox?
samricotta
pushed a commit
that referenced
this pull request
Aug 12, 2022
* abci: use protoio for length delimitation (#5818) Migrate ABCI to use protoio (uint64 length delimiters) instead of int64 length delimiters to be consistent with the approach used in the P2P layer. Closes: #5783 * Import ReadMsg interface change from #5868 Signed-off-by: Thane Thomson <connect@thanethomson.com> * Convert PR number to link in UPGRADING Signed-off-by: Thane Thomson <connect@thanethomson.com> * Update Tendermint Socket Protocol docs to reflect length prefix encoding change Signed-off-by: Thane Thomson <connect@thanethomson.com> * Clarify that length delimiters are varints Signed-off-by: Thane Thomson <connect@thanethomson.com> Signed-off-by: Thane Thomson <connect@thanethomson.com> Co-authored-by: Marko <marbar3778@yahoo.com>
samricotta
pushed a commit
that referenced
this pull request
Aug 12, 2022
* abci: use protoio for length delimitation (#5818) Migrate ABCI to use protoio (uint64 length delimiters) instead of int64 length delimiters to be consistent with the approach used in the P2P layer. Closes: #5783 * Import ReadMsg interface change from #5868 Signed-off-by: Thane Thomson <connect@thanethomson.com> * Convert PR number to link in UPGRADING Signed-off-by: Thane Thomson <connect@thanethomson.com> * Update Tendermint Socket Protocol docs to reflect length prefix encoding change Signed-off-by: Thane Thomson <connect@thanethomson.com> * Clarify that length delimiters are varints Signed-off-by: Thane Thomson <connect@thanethomson.com> Signed-off-by: Thane Thomson <connect@thanethomson.com> Co-authored-by: Marko <marbar3778@yahoo.com>
samricotta
pushed a commit
that referenced
this pull request
Aug 16, 2022
* abci: use protoio for length delimitation (#5818) Migrate ABCI to use protoio (uint64 length delimiters) instead of int64 length delimiters to be consistent with the approach used in the P2P layer. Closes: #5783 * Import ReadMsg interface change from #5868 Signed-off-by: Thane Thomson <connect@thanethomson.com> * Convert PR number to link in UPGRADING Signed-off-by: Thane Thomson <connect@thanethomson.com> * Update Tendermint Socket Protocol docs to reflect length prefix encoding change Signed-off-by: Thane Thomson <connect@thanethomson.com> * Clarify that length delimiters are varints Signed-off-by: Thane Thomson <connect@thanethomson.com> Signed-off-by: Thane Thomson <connect@thanethomson.com> Co-authored-by: Marko <marbar3778@yahoo.com>
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #9176.
PR checklist
CHANGELOG_PENDING.mdupdated, or no changelog entry neededdocs/) and code comments, or nodocumentation updates needed