fix: handle empty or malformed logs during message processing

[nchamo]

Problem

AES128::decrypt in aes128.nr currently panics at 5 points when given malformed ciphertext or wrong-key data. During message discovery (do_sync_state), a panic in decrypt crashes the entire sync process rather than gracefully skipping the unprocessable log.

Additionally, do_sync_state itself panics when it encounters empty logs, since it unconditionally indexes into the log without checking its length first.

Panic points in decrypt

1. Empty ciphertext -- ciphertext.get(0) panics when the BoundedVec is empty.
2. Short header plaintext -- header_plaintext.get(0) / .get(1) panic when the AES decrypt oracle returns fewer than 2 bytes (e.g. wrong-key PKCS#7 stripping produces an empty result).
3. Invalid ciphertext length -- BoundedVec::from_parts(ciphertext_with_padding, ciphertext_length) panics when the 2-byte header decodes to a length exceeding MESSAGE_PLAINTEXT_SIZE_IN_BYTES (e.g. 65535 from corrupted data).
4. Invalid plaintext length -- fields_from_bytes asserts bytes.len() % 32 == 0, panicking when the decrypted body has a non-aligned length (e.g. 33 bytes).
5. Field overflow -- fields_from_bytes asserts each 32-byte chunk fits within the BN254 field modulus, panicking when decrypted bytes exceed it (e.g. 0xFF repeated 32 times).

Panic in do_sync_state

do_sync_state processes every pending tagged log without validating its size. It would panic when pending_tagged_log.log.get(0) was called on empty logs

Important for reviewers

I recommend using "hide whitespace" config when reviewing file changes, since it would make the changes easier to understand

Fixes F-356
Fixes F-191

[nchamo]

Needed for the do_sync_state test

[nchamo]

Needed for the do_sync_state test

[nchamo]

Since Noir lacks early return, I though the best way to make this clear as possible was to fully embrace Option. Open to other suggestions though

[nventuro]

embrace monads

[nventuro]

It's a bit annoying that we end up in nested-hell, but it is the cleanest way of handling this I think.

[nchamo]

All these scenarios used to cause a panic

[nchamo]

I'm now using try_fields_from_bytes to avoid the repeated logic, but let me know if you think the two should coexist (maybe as a form of optimization?)

[nventuro]

But expect can fail, no? So if the bytes cannot be put into fields we'll panic. Is this not an issue?

[nchamo]

Sorry I wasn't clear. Both functions can co-exists. I didn't delete fields_from_bytes because it was public and maybe someone users are using it

My comment tried (and failed) to imply that having fields_from_bytes use try_fields_from_bytes might not be the most optimized solution, since we do things a little differently to handle the Option instead of panicking. It should be negligible, but I wanted to double check

[nventuro]

Instead of doing things this way, just create the capsule array and push into it.

[nventuro]

Suggested change

	unconstrained fn do_sync_state_can_handle_empty_logs() {
	unconstrained fn do_sync_state_does_not_panic_on_empty_logs() {

[nventuro]

It's a bit annoying that we end up in nested-hell, but it is the cleanest way of handling this I think.

[nventuro]

What if this were a lambda?

|ciphertext| {
    if ciphertext.len() > 0 {
        Option::some(
            (ciphertext.get(0), array::subbvec(ciphertext, EPH_PK_X_SIZE_IN_FIELDS)),
        )
    } else {
        Option::none()
    }
}.and_then(|(eph_pk_x, masked_fields)| {

[nventuro]

Better to have the len be MESSAGE_PLAINTEXT_SIZE_IN_BYTES+1, so that we make sure we're handling the edge case correctly.

[nventuro]

But expect can fail, no? So if the bytes cannot be put into fields we'll panic. Is this not an issue?

[nventuro]

Suggested change

	pub fn try_fields_from_bytes<let N: u32>(bytes: BoundedVec<u8, N>) -> Option<BoundedVec<Field, N / 32>> {
	fn try_fields_from_bytes<let N: u32>(bytes: BoundedVec<u8, N>) -> Option<BoundedVec<Field, N / 32>> {

Let's keep API surface small for now.

[nventuro]

Might read easier if extracting this body into a fn. Typically helps with nested options etc.

[nventuro]

Better to test with p directly to check the edge case.

[benesjan]

Since Nico went through this and the feedback was not yet addressed here I don't think my review is necessary at this point.

@nchamo please request review again from me if you would like me to chime in.

[nventuro]

Extremely minor (and probably late at this stage), but I personally prefer inverting the blocks here, sort of like an early return. OTherwise the dangling block is a bit hard to read.

[nventuro]

If we already have nesting anyway I don't think this gains us much?

[nventuro]

Suggested change

	Option::some(ciphertext)
	.and_then(|ciphertext| {
	if ciphertext.len() > 0 {
	let masked_fields: BoundedVec<Field, MESSAGE_CIPHERTEXT_LEN - EPH_PK_X_SIZE_IN_FIELDS> =
	array::subbvec(ciphertext, EPH_PK_X_SIZE_IN_FIELDS);
	Option::some((ciphertext.get(0), masked_fields))
	} else {
	Option::none()
	}
	})
	.and_then(|(eph_pk_x, masked_fields)| {
	if ciphertext.len() > 0 {
	let masked_fields: BoundedVec<Field, MESSAGE_CIPHERTEXT_LEN - EPH_PK_X_SIZE_IN_FIELDS> =
	array::subbvec(ciphertext, EPH_PK_X_SIZE_IN_FIELDS);
	Option::some((ciphertext.get(0), masked_fields))
	} else {
	Option::none()
	}
	.and_then(|(eph_pk_x, masked_fields)| {

[nventuro]

This is insufficient, we need to test that the oracle does not fail. There may be failure later on (e.g. the len of 0 looks scary).

[nchamo]

Since we need to do some oracle work beforehand, I created F-452 to tackle this separately. The idea would be to catch all potential errors during decryption, and have the oracle return an Option

Will leave the tests here though, since we didn't have many 😅

[nventuro]

Good work overall

[nventuro]

This would be a good place to have a Result, so that we can know why the process failed. Oh well.

[AztecBot]

❌ Failed to cherry-pick to v4-next due to conflicts. (🤖) View backport run.