add codec prop test#2087
Conversation
27005c5 to
504bb68
Compare
e821d03 to
9ccf284
Compare
GitGab19
left a comment
There was a problem hiding this comment.
Can you add some docs on top of each test, explaining what it does?
Also, here's a review point by Claude, I don't know if this is something you already evaluated or not:
5. No noise/encryption path tested
What exists in the codebase
The codec has two entirely separate encoder/decoder implementations:
WithoutNoise<B, T>(aliased asEncoder<T>/StandardDecoder<T>) — the plaintext path the PR tests.WithNoise<B, T>(aliased asNoiseEncoder<T>/StandardNoiseDecoder<T>) — the Noise-encrypted path, gated behind#[cfg(feature = "noise_sv2")].
The WithNoise encoder and decoder are architecturally quite different from their plaintext counterparts. Looking at the actual code:
Encoder: WithNoise::encode takes a State enum parameter (either HandShake, NotInitialized, or Transport) and manages two separate internal buffers — sv2_buffer for the serialized frame and noise_buffer for the encrypted output. The payload is chunked and encrypted in SV2_FRAME_CHUNK_SIZE pieces using AEAD, with each chunk getting a separate MAC tag appended (AEAD_MAC_LEN). The header is encrypted separately from the payload. This is meaningfully more complex than WithoutNoise::encode, which just serializes into a single buffer.
Decoder: WithNoise::next_frame takes a mutable State reference and branches on it, with different logic for NotInitialized, HandShake, and Transport phases. In Transport state, it decrypts the header first (reading exactly ENCRYPTED_SV2_FRAME_HEADER_SIZE bytes), then decrypts the payload in chunks, reassembling into sv2_buffer. None of this logic is exercised by the PR's tests.
Why this matters for property testing specifically
Property tests are particularly valuable here because the noise codec has several subtle invariants that are easy to get wrong under random inputs:
1. Chunk boundary arithmetic. The encoder slices the payload into chunks of SV2_FRAME_CHUNK_SIZE - AEAD_MAC_LEN bytes and the decoder reassembles them. A property test with randomly-sized payloads would exercise whether the start/end pointer arithmetic in both the encoder and decoder stays in sync across all payload sizes — including edge cases like payloads that land exactly on a chunk boundary, or payloads of zero length.
2. Encrypted length inflation. Every chunk grows by AEAD_MAC_LEN bytes after encryption. The decoder uses header.encrypted_len() to know how many noise bytes to expect. A roundtrip property test would verify that the encoder and decoder agree on this length calculation for all payload sizes, which the plaintext tests cannot exercise at all since there's no MAC overhead.
3. The State machine transitions. WithNoise::encode and WithNoise::next_frame both dispatch on State. A property test could verify that the same State object used to encode can be used to decode — i.e., that the codec's state machine doesn't get corrupted mid-frame. This is particularly important because State::Transport holds a NoiseCodec that maintains nonce counters internally; mismatched nonce counts between encoder and decoder would cause authentication failures.
4. danger_set_start usage. Both encode and decode_noise_frame use self.noise_buffer.danger_set_start(encrypted_len) and self.sv2_buffer.danger_set_start(decrypted_len) to manage buffer offsets during chunk processing. This is explicitly marked as "danger" for a reason — it unsafely repositions the buffer's read cursor. Random payload sizes would probe whether these offsets are computed correctly in all cases.
Why it's genuinely hard to test
This isn't just an oversight — testing the noise path is legitimately harder. The WithNoise types require a fully established State::Transport(NoiseCodec), which means you'd need to either:
- Run a complete Noise handshake between two
WithNoiseinstances to produce a validStatefor both sides, or - Construct a
NoiseCodecdirectly using known test keys (which requires eithernoise_sv2to expose test helpers, or using the existing key utilities fromkey-utilsindev-dependencies).
Looking at dev-dependencies in Cargo.toml, key-utils is already available, which suggests the primitives to set up test keys exist. A property test could plausibly construct a pair of NoiseCodec instances from fixed test keys, then fuzz the message payload while verifying the roundtrip. The handshake itself doesn't need to be property-tested — it can be fixed — and then quickcheck can drive the payload content.
What a follow-up test could look like (sketch)
#[cfg(all(test, feature = "noise_sv2"))]
mod noise_prop_tests {
#[quickcheck]
fn prop_noise_encode_decode_roundtrip(msg: TestMessage, msg_type: u8) -> TestResult {
// Set up a Noise session from fixed test keys
let (mut encode_state, mut decode_state) = setup_noise_transport_pair();
let frame = match Sv2Frame::from_message(msg.clone(), msg_type, 0, false) {
Some(f) => f,
None => return TestResult::discard(),
};
let mut encoder = NoiseEncoder::<TestMessage>::new();
let encrypted = match encoder.encode(Frame::Sv2(frame), &mut encode_state) {
Ok(e) => e,
Err(_) => return TestResult::failed(),
};
let mut decoder = StandardNoiseDecoder::<TestMessage>::new();
// feed encrypted bytes and call next_frame with decode_state...
// verify msg survives the roundtrip
}
}The main cost is writing setup_noise_transport_pair(), which is non-trivial but one-time work. Given that the noise path is the one actually used in production SV2 deployments, it's worth tracking as a concrete follow-up issue.
|
The noise encoder/decoder tests existed earlier, but since those APIs are likely to change soon or at least gain stronger assertions and clearer type-state transitions, I have removed them for now to avoid them becoming outdated. I will add documentation for the current tests. |
9ccf284 to
7280bd3
Compare
Updated the PR with comments on test, I have also included the noise scenario for which API's might not change but not covering everything. |
Part of: #2069