Repository navigation
codec: replace codec! with a Codec derive (SFT-8112) - #1
Merged
Merged
Conversation
A corrupt block count made the receiver reserve 16 bytes per block up front, which overflows the capacity on 32-bit targets and panics. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
badicsalex
force-pushed
the
SFT-8112-codec-derive
branch
3 times, most recently
from
October 10, 2026 20:42
19a5e26 to
04271a0
Compare
badicsalex
commented
Oct 10, 2026
| } | ||
| } | ||
|
|
||
| /// `usize` is a varint, because its width differs between targets |
Contributor
Author
There was a problem hiding this comment.
This is a design choice. There are no good solutions (having it fixed 64 is wasteful, fixed 32 is incompatible across architectures, not allowing usize would mean we have to modify some 60 fields in keyos)
| fn decode_bytes(bytes: B) -> Result<Self, Error> { | ||
| let mut reader = Reader::new(bytes); | ||
| Self::decode(&mut reader) | ||
| /// Decodes `Self` from the start of `bytes`, ignoring whatever follows it |
Contributor
Author
There was a problem hiding this comment.
The ignore part here is important, nothing checks the "junk after packet" case, which is different from rkyv and many serde implementations.
Could theoretically affect accidentally modifier frozen structs, but those should be flagged during review.
|
|
||
| /// Marks every encodable type except `u8` | ||
| /// | ||
| /// A `Vec<u8>` borrows as `&[u8]`, because its encoded bytes already are the values. Any other |
Contributor
Author
There was a problem hiding this comment.
I felt the need for this clunky documentation, this is an unfortunate workaround
| let ciphertext_address = record.payload.ciphertext.as_ptr() as usize; | ||
| let ciphertext_len = record.payload.ciphertext.len(); | ||
|
|
||
| // XXX: The reader only lends shared slices, so the ciphertext is located in |
Contributor
Author
There was a problem hiding this comment.
This is a hacky workaround so that we don't need a decode_mut with MutReader or whatever that would be. A lot of unsafe code.
| let bytes: Bytes = bytes.into(); | ||
| let plaintext = bytes.slice(bytes.len() - decrypt_len..); | ||
| let frames = wire::parse_session_frames(plaintext); | ||
| let bytes = Bytes::from_owner(bytes); |
Contributor
Author
There was a problem hiding this comment.
For Vec, this is practically equivalent to .into()
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
badicsalex
force-pushed
the
SFT-8112-codec-derive
branch
from
October 10, 2026 20:53
04271a0 to
487a9b6
Compare
Only the last byte can overflow, so it's the only one checked. Redundant encodings such as [0x80, 0x00] decode fine. Rejecting them is not really necessary, as nothing else makes encodings unique either: non-frozen structs skip unknown trailing fields. And the check costs measurably, 6-19 ns per multi-byte varint on the device. On the device, this cuts the time to decode small messages into their borrowed form by a third to a half. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
badicsalex
force-pushed
the
SFT-8112-codec-derive
branch
from
October 11, 2026 14:17
3e76017 to
02ec722
Compare
The derive is meant for keyos IPC, so by default it prefixes structs and enum variants with their length, which lets old and new versions of a type decode each other. QuantumLink's own wire types use it frozen, which keeps the codec! layout byte for byte. Decode no longer abstracts over the byte container. The borrowed form points into a &[u8] instead, and the frame parser slices stream payloads out of the record's Bytes itself. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
badicsalex
force-pushed
the
SFT-8112-codec-derive
branch
from
October 11, 2026 15:06
02ec722 to
8e29374
Compare
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
badicsalex
commented
Oct 11, 2026
| } | ||
|
|
||
| if byte & 0x80 == 0 { | ||
| if index > 0 && payload == 0 { |
Contributor
Author
There was a problem hiding this comment.
the payload == 0 check was removed, because it's harmless, but costs some 5% in performance.
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
1 participant
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.
Added a new derive that can create ql-codec Encode and Decode implementations for structs and enums, as well as a Ref version of the type, where applicable fields (Vec, String and friends) are typed as references instead.
Decode can decode to both owned and ref types, and convert from ref to owned. (I originally wrote decode_ref().to_owned() for every owned decode, but that's inefficient for Vec: it does two conversions)
The rest of the types were rewritten to use the derive if possible. Mostly mechanical. Wire format is exactly the same.
Some types that took buffer types and
Bytesstuff as generics were simplified, without losing zero-copy. This results in one hacky solution, but IMO that's better than having to implement a wholedecode_mutsuite. I'll flag this as a review comment.One interesting thing this allows: I marked all stucts as
frozen, to make the wire format same as onmain, but we can actually remove that for non-hot messages, allowing us to later easily extend the structs.