quinn-proto: Non-determinism in endpoint/connection/congestion

[michael-yxchen]

While debugging an issue I have, I found that quinn-proto protocol isn't fully deterministic. The source of randomness seems to be the call to StdRng::from_entropy in three places:

quinn/quinn-proto/src/endpoint.rs

Line 62 in e1e1e6e

rng: StdRng::from_entropy(),

quinn/quinn-proto/src/connection/mod.rs

Line 270 in e1e1e6e

let mut rng = StdRng::from_entropy();

quinn/quinn-proto/src/congestion/bbr/mod.rs

Line 99 in e1e1e6e

random_number_generator: rand::rngs::StdRng::from_entropy(),

Are these intended?

Thank you for the help!

[djc]

It's more interesting to look at how those random number generators are used. Did we promise somewhere that it would be fully deterministic?

[michael-yxchen]

Sure, I'll circle back on how those RNGs are are used.

I got the idea that quinn-proto would be fully deterministic from here

quinn/quinn-proto/src/lib.rs

Lines 3 to 5 in e1e1e6e

	//! quinn-proto contains a fully deterministic implementation of QUIC protocol logic. It contains
	//! no networking code and does not get any relevant timestamps from the operating system. Most
	//! users may want to use the futures-based quinn API instead.

I was thinking that it'd imply that there'd be no source of randomness from within, though it wasn't explicitly mentioned.

[djc]

Yeah, I suppose the "fully deterministic" is a bit of a lie!

[Ralith]

Yep, that comment is out of date. The randomized stuff is usually of little interest, though -- tokens and a few skipped packet numbers, and whatever BBR does if you opt into that.

We could allow a RNG seed to be passed in to recover perfect determinism if someone actually wants that.

[michael-yxchen]

For context, one of our nightly tests depends on quinn-proto. It failed one time but we were never able to reproduce the error. It would be really helpful if the protocol can accept some RNG seeds so the test is perfectly deterministic.

I can write up a patch if people don't have the capacity to do it.

[Ralith]

I'd be happy to review such a patch. I think adding an argument to quinn_proto::{Endpoint, Connection}::new should do the trick.