-
Notifications
You must be signed in to change notification settings - Fork 74
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
Perform the protobuf encoding and decoding manually #2326
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Automatically approving tomaka's pull requests. This auto-approval will be removed once more maintainers are active.
The |
twiggy diff reportDifference in .wasm size before and after this pull request.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to add some tests for the encoder in src/util/protobuf.rs
for slice in protobuf::bytes_tag_encode(2, key) { | ||
out.extend_from_slice(slice.as_ref()); | ||
} | ||
debug_assert_eq!(out.len(), CAPACITY); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do u want to add debug_assert_eq
in other similar places as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it's a bit complicated. The expected length of the final buffer is not so easily calculable because numbers are encoded in LEB128, in other words in a variable-length way.
For example, if you send a buffer of 30 bytes, the message will be 32 bytes (30 + 2). But if you send a buffer of 150 bytes, the message will be 153 bytes (150 + 3).
I've done this debug_assert_eq!
thing in peer_id.rs
because the encoding of a PeerId is fully deterministic and always has the same size, but in other places I feel like hardcoding a capacity would be a bit too hacky.
src/network/kademlia.rs
Outdated
let mut buf = Vec::with_capacity(protobuf.encoded_len()); | ||
protobuf.encode(&mut buf).unwrap(); | ||
buf | ||
let mut out = Vec::new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about capacity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add some capacities everywhere 👍
Pushed some tests. Through these tests I've found a way to make the LEB128 decoding code panic. Fixed it and added that to the CHANGELOG. |
Fix #2285
Fix #2312
Unfortunately, the situation regarding protobuf in the Rust ecosystem is rather dire.
prost
is the only proper library available, and it needs to perform a very annoying build step that also requirescmake
to be installed. The bindings generated byprost
are also veryVec
-heavy and cannot be made zero cost. It is also not possible to detect missing fields, asprost
will automatically use a default value if a field is missing.For these reasons, this PR replaces the
prost
library with manual encoding and decoding of the protobuf messages.In terms in code, it makes the code less easy to read because we replace field names with their indices. However, I believe that it's worth it. All the protocols we use are stable, so there is no risk of mistake in backporting a change to the protocol, which is the main reason for having easy-to-read field names.