Introduce `VariableLengthVector` by tgeoghegan · Pull Request #1467 · divviup/libprio-rs

tgeoghegan · 2026-07-09T01:14:11Z

TLS presentation language provides fixed and variable length vectors (1). Variable length vectors have a minimum and maximum length and are prefixed with a length that can be 1 to 4 bytes wide.

Thus far, prio::codec has handled these using the encode_u*_items and decode_u*_items functions. This works, but has some drawbacks:

Because these are free functions instead of implementations of traits Encode, Decode, etc. on some type, it is hard to provide convenience functions like Decode::get_decoded generically.
For similar reasons, we always take an encoding parameter, which is () in the majority of cases.
Callers have to deal with the encoded length of variable length vectors themselves instead of using Encode::encoded_len.
The minimum length is not enforced.

This commit introduces a new struct VariableLengthVector. It is generic over:

minimum length,
the type of the length prefix,
and the type of the items.

See the doccomments on VariableLengthVector for examples of how to use it to realize different TLS PR declarations. We provide Encode and Decode implementations, but not ParameterizedEncode/Decode, based on the observation that we would never use it here in prio or in Janus.

tgeoghegan · 2026-07-09T01:17:36Z

The changes in idpf.rs and the ping pong topology illustrate usage of this, but see also this commit on Janus which shows how we'd use it in a couple types over there. I think it makes the encoding and decoding of messages much more uniform and less surprising, and so it's worth it. We still have encode/decode_fixlen_items hanging around. I think a very similar type to VariableLengthVector could be used for those, but in another PR.

tgeoghegan · 2026-07-09T01:24:19Z

I didn't wire anything up for U24 here, because I want to remove that never used type altogether in #1468.

tgeoghegan · 2026-07-09T15:05:27Z

I suggest reviewing this one commit at a time: the first commit introduces the new struct and proves its equivalence to the existing functions by extending tests, then the next commit uses it throughout the crate and removes the now obsolete functions.

I'm eager to hear suggestions about how we could improve ergonomics. I implemented From<Vec<E: Encode>>, AsRef<[E]> and AsMut<[E]> so that callsites that previously used a plain Vec<E> can adapt to using VariableLengthVector with minimal extra boilerplate. But we still end up with a lot of .into(), .as_ref(), .as_mut() calls. We might consider adding Deref[Mut]<Target = [E]> to make this more graceful, but I'm wary of surprising behaviors caused by implementing those traits. I'm also not sure if it's wise to implement Deref[Mut] and also AsRef/AsMut.

jcjones

I'm eager to hear suggestions about how we could improve ergonomics. I implemented From<Vec<E: Encode>>, AsRef<[E]> and AsMut<[E]> so that callsites that previously used a plain Vec can adapt to using VariableLengthVector with minimal extra boilerplate. But we still end up with a lot of .into(), .as_ref(), .as_mut() calls. We might consider adding Deref[Mut]<Target = [E]> to make this more graceful, but I'm wary of surprising behaviors caused by implementing those traits. I'm also not sure if it's wise to implement Deref[Mut] and also AsRef/AsMut.

idpf.rs is probably as bad as it gets, it seems, and that's not that bad. I don't think Deref[Mut] is a good idea -- we don't want to reach through to slice methods on accident.

Maybe we can do some kind of iter or iter_mut helpers to reduce boilerplate, though I don't know what. But I think the explicitness is fine. It's... honesty.

jcjones · 2026-07-09T19:20:06Z

+    for VariableLengthVector<MIN_LEN, LEN, D>
+{
+    fn decode(bytes: &mut Cursor<&[u8]>) -> Result<Self, CodecError> {
+        // Read two bytes to get length of opaque byte vector


It's not two anymore, right, it's LEN?

Suggested change

// Read two bytes to get length of opaque byte vector

// Read LEN bytes to get length of opaque byte vector

jcjones · 2026-07-09T19:22:44Z

+                + self
+                    .contents
+                    .iter()
+                    .map(|item| item.encoded_len().unwrap_or(0))


If the unwrapped item is None, then the resulting total is wrong. Which seems harmless as-coded, but could be a footgun in the future. Maybe doing a sum() to a final Option that can propagate the None would bypass the hazard?

jcjones · 2026-07-09T19:27:30Z

+    for VariableLengthVector<MIN_LEN, LEN, E>
+{
+    fn encode(&self, bytes: &mut Vec<u8>) -> Result<(), CodecError> {
+        if self.contents.len() < MIN_LEN


I'm confused about MIN_LEN here. In the decode path it's checked against the encoded byte length, so it must mean it's in bytes. But self.contents.len() is documented 10 lines up as being the length of the vector, which is not the byte length... unless we happen to choose u8 as E, which we're not asserting.

You're correct, I got it wrong. Fixed, and I added tests to check that we handle bounds correctly for both byte vectors and vectors of non-trivial types.

divergentdave

I think implementing both Deref and AsRef would be fine, but if we have Deref and DerefMut implemented, that obviates the need for AsRef, since automatic dereferencing will do the right thing wherever we need it, without an explicit method call. Maybe we should try setting <VariableLengthVector as Deref>::Target to the vector, so that vector mutating methods just work, and slice methods go through automatic dereferencing twice. VariableLengthVector is a wrapper type that is primarily useful for encoding and decoding, so it would be nice for it to have less impact in other places where the code only cares about the vector on the inside.

I'm not sure at present whether I like the before or after state better. On the one hand, this provides a way to check minimum encoded lengths easily, and reduces repetitions between the 8-bit, 16-bit, and 32-bit length prefix versions of the implementation. On the other hand, we're primarily trading a function call for a method call in Encode and Decode implementations, the wire format of a mesage is now split across those implementations and type definitions, rather than being fully described by encode or decode methods, and the minimum encoded length check is a low-value check, since it will rarely be able to do much more than reject an empty vector.

divergentdave · 2026-07-09T17:56:08Z

    #[error("vector length exceeded range of length prefix")]
    LengthPrefixOverflow,

+    /// The number of items in a variable-length vector is outside of the type's range.


We should rephrase this to avoid bytes vs. elements confusion.

Suggested change

/// The number of items in a variable-length vector is outside of the type's range.

/// The number of bytes in a variable-length vector is outside of the type's range.

divergentdave · 2026-07-09T17:56:36Z

+        if self.contents.len() < MIN_LEN
+            || self.contents.len()
+                > LEN::max_value()
+                    .try_into()
+                    .map_err(|_| CodecError::Other("prefix max length too big for usize".into()))?


These checks are incorrect, as they compare the length in elements to the minimum and maximum lengths in bytes. The related tests only work because they are encoding vectors of bytes.

divergentdave · 2026-07-09T18:30:26Z

+                + self
+                    .contents
+                    .iter()
+                    .map(|item| item.encoded_len().unwrap_or(0))


This is incorrect, if encoded_len() returns None, it means the type doesn't provide any hint, and we fell back to the default implementation. We should return None from this method as well, rather than assuming a length of zero. I see Sum is implemented for Option<T> such that receiving a None short-circuits the sum and returns a None for the sum. Thus, we could do self.contents.iter().map(Encode::encoded_len).sum::<Option<usize>>()?.

Fixed, and I added a test to check that VariableLengthVector<E>::encoded_len returns None if E::encoded_len == None.

TLS presentation language provides fixed and variable length vectors ([1]). Variable length vectors have a minimum and maximum length and are prefixed with a length that can be 1 to 4 bytes wide. Thus far, `prio::codec` has handled these using the `encode_u*_items` and `decode_u*_items` functions. This works, but has some drawbacks: - Because these are free functions instead of implementations of traits `Encode, Decode`, etc. on some type, it is hard to provide convenience functions like `Decode::get_decoded` generically. - For similar reasons, we always take an encoding parameter, which is `()` in the majority of cases. - Callers have to deal with the encoded length of variable length vectors themselves instead of using `Encode::encoded_len`. - The minimum length is not enforced. This commit introduces a new struct `VariableLengthVector`. It is generic over: - minimum length, - the type of the length prefix, - and the type of the items. See the doccomments on `VariableLengthVector` for examples of how to use it to realize different TLS PR declarations. We provide `Encode` and `Decode` implementations, but not `ParameterizedEncode/Decode`, based on the observation that we would never use it here in `prio` or in Janus. This commit extends tests to prove that the encodings emitted by this new type are equivalent to what the existing functions did. [1]: https://datatracker.ietf.org/doc/html/rfc8446#section-3.4

Remove the `encode_u*_items` and `decode_u*_items` functions and replace their usage with `VariableLengthVector`.

tgeoghegan · 2026-07-09T21:05:47Z

While I was in here, I also aligned doccomments with Rust RFC 1574. Mostly in that I rewrote so that each doccomment starts with a single sentence on a single line.

tgeoghegan · 2026-07-09T21:14:59Z

Besides reviewing in terms of addressing the bugs, we should still discuss whether we want this at all. As David points out, the only tangible benefit here is that we now guard against too-short vectors, which was pretty unlikely to begin with. So the benefit here is subjective, in that I think it looks nicer, and it's better to have methods than functions. So we might still decide that this isn't worth the trouble.

jcjones

I like the way this looks, but I'm still only a junior rustacean. I wasn't there when the deep magics were written.

tgeoghegan requested a review from a team as a code owner July 9, 2026 01:14

tgeoghegan force-pushed the timg/codec-vlv branch from 8757a4f to 62e329a Compare July 9, 2026 01:19

jcjones requested changes Jul 9, 2026

View reviewed changes

divergentdave reviewed Jul 9, 2026

View reviewed changes

tgeoghegan added 3 commits July 9, 2026 15:55

adopt VariableLengthVector throughout the crate

6bb4954

Remove the `encode_u*_items` and `decode_u*_items` functions and replace their usage with `VariableLengthVector`.

review comments

20ae249

tgeoghegan force-pushed the timg/codec-vlv branch from 62e329a to 20ae249 Compare July 9, 2026 21:04

tgeoghegan requested review from divergentdave and jcjones July 9, 2026 21:06

jcjones approved these changes Jul 9, 2026

View reviewed changes

	// Read two bytes to get length of opaque byte vector
	// Read LEN bytes to get length of opaque byte vector

	/// The number of items in a variable-length vector is outside of the type's range.
	/// The number of bytes in a variable-length vector is outside of the type's range.

Uh oh!

Conversation

tgeoghegan commented Jul 9, 2026

Uh oh!

tgeoghegan commented Jul 9, 2026

Uh oh!

tgeoghegan commented Jul 9, 2026

Uh oh!

tgeoghegan commented Jul 9, 2026

Uh oh!

jcjones left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

divergentdave left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tgeoghegan commented Jul 9, 2026

Uh oh!

tgeoghegan commented Jul 9, 2026

Uh oh!

jcjones left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants