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

abci: Make ABCI and P2P wire-level length delimiters consistent #9182

Merged
merged 5 commits into from
Aug 11, 2022

Conversation

thanethomson
Copy link
Contributor

@thanethomson thanethomson commented Aug 6, 2022

Closes #9176.


PR checklist

  • Tests written/updated, or no tests needed
  • CHANGELOG_PENDING.md updated, or no changelog entry needed
  • Updated relevant documentation (docs/) and code comments, or no
    documentation updates needed

@thanethomson thanethomson force-pushed the thane/9176-fix-abci-proto-delim branch from f605f12 to 23e08c9 Compare August 6, 2022 17:26
@thanethomson thanethomson marked this pull request as ready for review August 6, 2022 17:41
@thanethomson thanethomson requested a review from a team August 6, 2022 17:41
@thanethomson thanethomson force-pushed the thane/9176-fix-abci-proto-delim branch 2 times, most recently from c3154bf to fbd89f7 Compare August 9, 2022 12:06
@thanethomson thanethomson requested a review from a team August 9, 2022 12:20
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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@thanethomson thanethomson force-pushed the thane/9176-fix-abci-proto-delim branch from 31680d3 to fd7e843 Compare August 9, 2022 20:07
tac0turtle and others added 5 commits August 11, 2022 07:57
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>
@thanethomson thanethomson force-pushed the thane/9176-fix-abci-proto-delim branch from fd7e843 to cff8f6b Compare August 11, 2022 11:57

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
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

@thanethomson thanethomson merged commit ae1fc74 into main Aug 11, 2022
@thanethomson thanethomson deleted the thane/9176-fix-abci-proto-delim branch August 11, 2022 17:30
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>
@jmalicevic jmalicevic mentioned this pull request Aug 26, 2022
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done/Merged
Development

Successfully merging this pull request may close these issues.

abci: Port message length delimiter changes to main
5 participants