Skip to content

fix(aztec-nr): account for AES PKCS#7 padding in message plaintext length#20840

Merged
nchamo merged 4 commits intomerge-train/fairiesfrom
fix/f-336-review-fixes
Feb 26, 2026
Merged

fix(aztec-nr): account for AES PKCS#7 padding in message plaintext length#20840
nchamo merged 4 commits intomerge-train/fairiesfrom
fix/f-336-review-fixes

Conversation

@nchamo
Copy link
Contributor

@nchamo nchamo commented Feb 25, 2026

Summary

  • Fixes index-out-of-bounds when using notes with arrays by accounting for AES PKCS#7 padding expansion (16 bytes) when computing MESSAGE_PLAINTEXT_LEN
  • Updates MAX_NOTE_PACKED_LEN (9 -> 8) and MAX_EVENT_SERIALIZED_LEN (11 -> 10) in both Noir and TypeScript to match the corrected capacity
  • Adds boundary tests for max-size and oversized messages, notes, and encryption plaintexts

Fixes F-336
Fixes #20737

@nchamo nchamo self-assigned this Feb 25, 2026
@nchamo nchamo added ci-no-fail-fast Sets NO_FAIL_FAST in the CI so the run is not aborted on the first failure ci-draft Run CI on draft PRs. labels Feb 25, 2026
'0x0000000000000000000000000000000000000000000000000000000000000000',
'0x0000000000000000000000000000000000000000000000000000000000000000',
'0x0000000000000000000000000000000000000000000000000000000000000000',
'0x0000000000000000000000000000000000000000000000000000000000000000', // content end (MAX_NOTE_PACKED_LEN = 10)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment was wrong, the value used to be 9

} from './util/encoding.js';

const MAX_EVENT_LEN = 12; // This is MAX_MESSAGE_CONTENT_LEN - PRIVATE_EVENT_RESERVED_FIELDS
const MAX_EVENT_LEN = 10; // This is MAX_MESSAGE_CONTENT_LEN - PRIVATE_EVENT_MSG_PLAINTEXT_RESERVED_FIELDS_LEN
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why MAX_EVENT_LEN in rpc_translator.ts went from 12 to 10

Two corrections are stacked here:

  1. Pre-existing off-by-one: The old value of 12 was MAX_MESSAGE_CONTENT_LEN itself, not MAX_MESSAGE_CONTENT_LEN - PRIVATE_EVENT_MSG_PLAINTEXT_RESERVED_FIELDS_LEN as the comment claimed. The reserved fields subtraction (- 1) was never applied, so the old value should have been 11.
  2. PKCS#7 padding fix (the actual bug fix in this PR): MESSAGE_PLAINTEXT_LEN was computed without accounting for the 16-byte AES PKCS#7 padding block that is always appended when encrypting field-aligned data. Subtracting this reduces MESSAGE_PLAINTEXT_LEN by 1, which propagates down to MAX_MESSAGE_CONTENT_LEN (12 -> 11) and consequently MAX_EVENT_LEN (11 -> 10).

@nchamo nchamo marked this pull request as ready for review February 26, 2026 15:50
@nchamo nchamo requested a review from nventuro as a code owner February 26, 2026 15:50
Copy link
Contributor

@nventuro nventuro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tyvm! the writeup is quite nice

Comment on lines +154 to +157
/// AES128-CBC encryption for Aztec protocol messages.
///
/// ## Overview
///
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

y u no 120 char

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, reflowed to 120 chars.

// the output `header_ciphertext_bytes` is 16 bytes larger than the input in this case.
let header_ciphertext_bytes = aes128_encrypt(header_plaintext, header_iv, header_sym_key);
// I recall that converting a slice to an array incurs constraints, so I'll check the length this way instead:
// Static check avoids the constraint cost of slice-to-array conversion.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there are slices anywhere here?

assert_eq(decoded_content, BoundedVec::from_array(msg_content));
}

#[test(should_fail_with = "Invalid message content: it must have a length of at most MAX_MESSAGE_CONTENT_LEN")]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am very surprised this is caught by a test. Static asserts trigger compilation errors, not runtime errors, and I thought #[test] would not catch these - I expected nargo test to fail instead. Can you check with the Noir team that this is correct?

fn encrypt<let PlaintextLen: u32>(
plaintext: [Field; PlaintextLen],
recipient: AztecAddress,
) -> [Field; MESSAGE_CIPHERTEXT_LEN] {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we not statically assert PlaintextLen < MAX_LEN?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added std::static_assert(PlaintextLen <= MESSAGE_PLAINTEXT_LEN, ...) at the top of encrypt. Also updated the encrypt_oversized_plaintext test to use should_fail_with to match the new message.

encoding::{
EPH_PK_SIGN_BYTE_SIZE_IN_BYTES, EPH_PK_X_SIZE_IN_FIELDS, HEADER_CIPHERTEXT_SIZE_IN_BYTES,
MESSAGE_CIPHERTEXT_LEN, MESSAGE_PLAINTEXT_LEN,
MESSAGE_CIPHERTEXT_LEN, MESSAGE_PLAINTEXT_LEN, MESSAGE_PLAINTEXT_SIZE_IN_BYTES,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit weird that we take the constant from messages, since the constant itself is derived from AES. But this will make sense later on. It does look a bit odd now though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, we should refactor it when we start making more changes to this

@nventuro nventuro linked an issue Feb 26, 2026 that may be closed by this pull request
@nchamo nchamo merged commit 10197f4 into merge-train/fairies Feb 26, 2026
20 checks passed
@nchamo nchamo deleted the fix/f-336-review-fixes branch February 26, 2026 20:33
AztecBot pushed a commit that referenced this pull request Feb 26, 2026
…ngth (#20840)

## Summary

- Fixes index-out-of-bounds when using notes with arrays by accounting for AES PKCS#7 padding expansion (16 bytes) when computing `MESSAGE_PLAINTEXT_LEN`
- Updates `MAX_NOTE_PACKED_LEN` (9 -> 8) and `MAX_EVENT_SERIALIZED_LEN` (11 -> 10) in both Noir and TypeScript to match the corrected capacity
- Adds boundary tests for max-size and oversized messages, notes, and encryption plaintexts

Fixes F-336
Fixes #20737
@AztecBot
Copy link
Collaborator

✅ Successfully backported to backport-to-v4-staging #20923.

github-merge-queue bot pushed a commit that referenced this pull request Feb 27, 2026
BEGIN_COMMIT_OVERRIDE
fix(aztec-up): install noir-profiler alongside nargo (#20896)
fix(aztec-nr): account for AES PKCS#7 padding in message plaintext
length (#20840)
END_COMMIT_OVERRIDE
AztecBot added a commit that referenced this pull request Feb 27, 2026
The backport of #20840 (AES PKCS#7 padding fix) changed MAX_NOTE_PACKED_LEN
and MAX_EVENT_SERIALIZED_LEN, which alters compiled protocol contracts bytecode
and thus the Protocol Contracts tree root hash.
AztecBot pushed a commit that referenced this pull request Feb 27, 2026
…ngth (#20840)

## Summary

- Fixes index-out-of-bounds when using notes with arrays by accounting for AES PKCS#7 padding expansion (16 bytes) when computing `MESSAGE_PLAINTEXT_LEN`
- Updates `MAX_NOTE_PACKED_LEN` (9 -> 8) and `MAX_EVENT_SERIALIZED_LEN` (11 -> 10) in both Noir and TypeScript to match the corrected capacity
- Adds boundary tests for max-size and oversized messages, notes, and encryption plaintexts

Fixes F-336
Fixes #20737
@AztecBot
Copy link
Collaborator

✅ Successfully backported to backport-to-v4-staging #20980.

AztecBot added a commit that referenced this pull request Mar 2, 2026
PR #20840 changed MAX_EVENT_LEN from 12 to 10 in yarn-project/txe/src/rpc_translator.ts
to account for AES PKCS#7 padding, but forgot to update the corresponding Noir constant
MAX_EVENT_SERIALIZATION_LENGTH in txe_oracles.nr. This caused the TXE to return 50 fields
(10 * 5) while Noir expected 60 fields (12 * 5), crashing emit_and_discover_event test.
AztecBot added a commit that referenced this pull request Mar 3, 2026
## Summary

Two fixes:

### 1. Fix `MAX_EVENT_SERIALIZATION_LENGTH` mismatch (fixes backport CI failure)

#20840 changed `MAX_EVENT_LEN` from 12 to 10 in `yarn-project/txe/src/rpc_translator.ts` but forgot to update the corresponding Noir constant `MAX_EVENT_SERIALIZATION_LENGTH` in `txe_oracles.nr`. This caused the TXE oracle to return `10 * 5 = 50` fields while the Noir oracle declaration expected `12 * 5 = 60` fields, crashing the `emit_and_discover_event` test.

### 2. Re-enable TXE-dependent noir tests in CI

Since Feb 24, all TXE-dependent noir tests (`noir-projects-txe-tests`) have been silently disabled in CI. The `build_and_test` function in `bootstrap.sh` has a guard `if [ -z "${1:-}" ]` that only runs TXE tests when no argument is passed, but CI always calls `build_and_test fast` or `build_and_test full`. This means the TXE tests only ran in local dev (no args) but never in CI.

This fix removes the conditional so TXE tests run in all CI modes (ci-fast, ci-full, ci-full-no-test-cache).

[ClaudeBox log](http://ci.aztec-labs.com/7a1d8c5993720a62-5)
AztecBot added a commit that referenced this pull request Mar 3, 2026
## Summary

Two fixes:

### 1. Fix `MAX_EVENT_SERIALIZATION_LENGTH` mismatch (fixes backport CI failure)

#20840 changed `MAX_EVENT_LEN` from 12 to 10 in `yarn-project/txe/src/rpc_translator.ts` but forgot to update the corresponding Noir constant `MAX_EVENT_SERIALIZATION_LENGTH` in `txe_oracles.nr`. This caused the TXE oracle to return `10 * 5 = 50` fields while the Noir oracle declaration expected `12 * 5 = 60` fields, crashing the `emit_and_discover_event` test.

### 2. Re-enable TXE-dependent noir tests in CI

Since Feb 24, all TXE-dependent noir tests (`noir-projects-txe-tests`) have been silently disabled in CI. The `build_and_test` function in `bootstrap.sh` has a guard `if [ -z "${1:-}" ]` that only runs TXE tests when no argument is passed, but CI always calls `build_and_test fast` or `build_and_test full`. This means the TXE tests only ran in local dev (no args) but never in CI.

This fix removes the conditional so TXE tests run in all CI modes (ci-fast, ci-full, ci-full-no-test-cache).

[ClaudeBox log](http://ci.aztec-labs.com/7a1d8c5993720a62-5)
github-merge-queue bot pushed a commit that referenced this pull request Mar 3, 2026
… CI (#21020)

## Summary

Two fixes:

### 1. Fix `MAX_EVENT_SERIALIZATION_LENGTH` mismatch (fixes backport CI
failure)

#20840 changed
`MAX_EVENT_LEN` from 12 to 10 in
`yarn-project/txe/src/rpc_translator.ts` but forgot to update the
corresponding Noir constant `MAX_EVENT_SERIALIZATION_LENGTH` in
`txe_oracles.nr`. This caused the TXE oracle to return `10 * 5 = 50`
fields while the Noir oracle declaration expected `12 * 5 = 60` fields,
crashing the `emit_and_discover_event` test.

### 2. Re-enable TXE-dependent noir tests in CI

Since Feb 24, all TXE-dependent noir tests (`noir-projects-txe-tests`)
have been silently disabled in CI. The `build_and_test` function in
`bootstrap.sh` has a guard `if [ -z "${1:-}" ]` that only runs TXE tests
when no argument is passed, but CI always calls `build_and_test fast` or
`build_and_test full`. This means the TXE tests only ran in local dev
(no args) but never in CI.

This fix removes the conditional so TXE tests run in all CI modes
(ci-fast, ci-full, ci-full-no-test-cache).

[ClaudeBox log](http://ci.aztec-labs.com/7a1d8c5993720a62-5)
AztecBot pushed a commit that referenced this pull request Mar 3, 2026
… CI (#21020)

## Summary

Two fixes:

### 1. Fix `MAX_EVENT_SERIALIZATION_LENGTH` mismatch (fixes backport CI
failure)

#20840 changed
`MAX_EVENT_LEN` from 12 to 10 in
`yarn-project/txe/src/rpc_translator.ts` but forgot to update the
corresponding Noir constant `MAX_EVENT_SERIALIZATION_LENGTH` in
`txe_oracles.nr`. This caused the TXE oracle to return `10 * 5 = 50`
fields while the Noir oracle declaration expected `12 * 5 = 60` fields,
crashing the `emit_and_discover_event` test.

### 2. Re-enable TXE-dependent noir tests in CI

Since Feb 24, all TXE-dependent noir tests (`noir-projects-txe-tests`)
have been silently disabled in CI. The `build_and_test` function in
`bootstrap.sh` has a guard `if [ -z "${1:-}" ]` that only runs TXE tests
when no argument is passed, but CI always calls `build_and_test fast` or
`build_and_test full`. This means the TXE tests only ran in local dev
(no args) but never in CI.

This fix removes the conditional so TXE tests run in all CI modes
(ci-fast, ci-full, ci-full-no-test-cache).

[ClaudeBox log](http://ci.aztec-labs.com/7a1d8c5993720a62-5)
ludamad pushed a commit that referenced this pull request Mar 3, 2026
… CI (backport #21020) (#21027)

Backport of #21020
to v4.

Cherry-pick applied cleanly — no conflicts.

## Summary

Two fixes:

### 1. Fix `MAX_EVENT_SERIALIZATION_LENGTH` mismatch
PR #20840 changed `MAX_EVENT_LEN` from 12 to 10 in
`yarn-project/txe/src/rpc_translator.ts` but forgot to update the
corresponding Noir constant `MAX_EVENT_SERIALIZATION_LENGTH` in
`txe_oracles.nr`. This caused the TXE oracle to return `10 * 5 = 50`
fields while the Noir oracle declaration expected `12 * 5 = 60` fields,
crashing the `emit_and_discover_event` test.

### 2. Re-enable TXE-dependent noir tests in CI
The `build_and_test` function in `bootstrap.sh` had a guard that only
ran TXE tests when no argument was passed, but CI always calls
`build_and_test fast` or `build_and_test full`. This fix removes the
conditional so TXE tests run in all CI modes.

ClaudeBox log: http://ci.aztec-labs.com/ed670f8e16fb711b-1
ludamad added a commit that referenced this pull request Mar 3, 2026
BEGIN_COMMIT_OVERRIDE
fix(aztec-nr): account for AES PKCS#7 padding in message plaintext
length (#20840)
feat: remove epk sign from message payload (#20926)
chore: increase mainnet local ejection threshold to 190k (#20884)
feat: add pinned-build support for protocol contracts in noir-contracts
(v4) (#20982)
fix!: undo bad fix (#20987)
chore: backport #20967 to v4 (metric on how many epochs validator has
been on committee) (#20989)
chore(sequencer): e2e tests for invalid signature recovery in checkpoint
attestations (#20971)
feat: allow custom addresses to be prefunded with fee juice in local
network (backport #21000) (#21004)
chore: increase max fee bots use in tests (#20867)
END_COMMIT_OVERRIDE
johnathan79717 pushed a commit that referenced this pull request Mar 4, 2026
## Summary

Two fixes:

### 1. Fix `MAX_EVENT_SERIALIZATION_LENGTH` mismatch (fixes backport CI failure)

#20840 changed `MAX_EVENT_LEN` from 12 to 10 in `yarn-project/txe/src/rpc_translator.ts` but forgot to update the corresponding Noir constant `MAX_EVENT_SERIALIZATION_LENGTH` in `txe_oracles.nr`. This caused the TXE oracle to return `10 * 5 = 50` fields while the Noir oracle declaration expected `12 * 5 = 60` fields, crashing the `emit_and_discover_event` test.

### 2. Re-enable TXE-dependent noir tests in CI

Since Feb 24, all TXE-dependent noir tests (`noir-projects-txe-tests`) have been silently disabled in CI. The `build_and_test` function in `bootstrap.sh` has a guard `if [ -z "${1:-}" ]` that only runs TXE tests when no argument is passed, but CI always calls `build_and_test fast` or `build_and_test full`. This means the TXE tests only ran in local dev (no args) but never in CI.

This fix removes the conditional so TXE tests run in all CI modes (ci-fast, ci-full, ci-full-no-test-cache).

[ClaudeBox log](http://ci.aztec-labs.com/7a1d8c5993720a62-5)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-to-v4 ci-draft Run CI on draft PRs. ci-no-fail-fast Sets NO_FAIL_FAST in the CI so the run is not aborted on the first failure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] incorrect detection of too-large note

3 participants