Skip to content

feat(pxe)!: pass BoundedVec sizes as oracle params#21176

Merged
nchamo merged 3 commits intomerge-train/fairiesfrom
feat/oracle-param-bounded-vec-sizes
Mar 10, 2026
Merged

feat(pxe)!: pass BoundedVec sizes as oracle params#21176
nchamo merged 3 commits intomerge-train/fairiesfrom
feat/oracle-param-bounded-vec-sizes

Conversation

@nchamo
Copy link
Contributor

@nchamo nchamo commented Mar 5, 2026

Summary

In #20840, we introduced a change to reflect the actual max length used for messages and notes, we were not calculating it correctly beforehand. The main issue is that aztec-nr used those notes/events max lengths as the max size for BoundedVec when communicating with PXE. And, on PXE's side, we also used that value to deserialize the BoundedVec.

But when we updated those values, old contracts were no longer compatible with the new version of PXE and new contracts were no longer compatible with the old version of PXE.

We want to prevent this from happening again, so we are now passing these values from aztec-nr to PXE. In this PR, we are simply passing the max possible length through the oracle, so this would be a breaking change

We are taking a different approach in #20957 by using capsules, which would fix the previous breaking change in #20840, while fixing the issue for future size changes

Fixes #14617
Fixes F-375

@nchamo nchamo requested a review from nventuro as a code owner March 5, 2026 16:36
@nchamo nchamo self-assigned this Mar 5, 2026
@nchamo nchamo marked this pull request as draft March 5, 2026 16:47
@nchamo nchamo added the ci-draft Run CI on draft PRs. label Mar 5, 2026

const MAX_PUBLIC_LOG_LEN_FOR_NOTE_COMPLETION = MAX_NOTE_PACKED_LEN;
const MAX_LOG_CONTENT_LEN = Math.max(MAX_PUBLIC_LOG_LEN_FOR_NOTE_COMPLETION, PRIVATE_LOG_CIPHERTEXT_LEN);
const MAX_LOG_CONTENT_LEN = PRIVATE_LOG_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.

This implicitly assumes that notes will not be longer than logs no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I wrote this on the other PR

I believe it didn't make sense to have this. MAX_NOTE_PACKED_LEN is calculated in aztec-nr from PRIVATE_LOG_CIPHERTEXT_LEN, and it should always be lower for it. But please correct me if I'm wrong or I'm missing something

Copy link
Contributor

Choose a reason for hiding this comment

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

But please correct me if I'm wrong or I'm missing something

Nial mentioned that this limit might not make sense as there is no reason for notes to not be longer in case they are delivered via offchain messages.

Could you expand on why this change was done here? I didn't review your other PRs that I assumed led to this so I don't have context here. Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made this change here because:

  • MAX_NOTE_PACKED_LEN and PRIVATE_LOG_CIPHERTEXT_LEN are actually calculated on the aztec-nr side of things, and the values are just copied to typescript
  • PRIVATE_LOG_CIPHERTEXT_LEN = 15, while MAX_NOTE_PACKED_LEN = 8. This is because: `
MAX_NOTE_PACKED_LEN = 15 (PRIVATE_LOG_CIPHERTEXT_LEN) 
                      - 3 (aes stuff) 
                      - 1 (message metadata)
                      - 3 (private note metadata: owner, storage slot, randomness)
                    = 8 

So if MAX_NOTE_PACKED_LEN is PRIVATE_LOG_CIPHERTEXT_LEN minus some fields, max(PRIVATE_LOG_CIPHERTEXT_LEN, MAX_NOTE_PACKED_LEN) would always be PRIVATE_LOG_CIPHERTEXT_LEN

That's why I thought it didn't make much sense to do that, and asked if I was missing something. About off-chain messages, would they go though this path? Do they emit logs somehow? Not very familiar with how they work today

'0x0000000000000000000000000000000000000000000000000000000000000000',
'0x0000000000000000000000000000000000000000000000000000000000000000',
'0x0000000000000000000000000000000000000000000000000000000000000000', // content end (MAX_NOTE_PACKED_LEN = 8)
'0x0000000000000000000000000000000000000000000000000000000000000000', // content end (8 storage fields)
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this 8? Wouldn't this be higher, up to PRIVATE_LOG_CIPHERTEXT_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.

Right now, if you look at aztec-nr, the max length/capacity for notes is 8. That's why this test was using this particular value

This test could test other values though, but I thought it made sense to test what we would receive from contracts today (8)

/// which in turn implies that you need to update the ORACLE_VERSION constant in this file and in
/// `noir-projects/aztec-nr/aztec/src/oracle/version.nr`.
export const ORACLE_INTERFACE_HASH = '9fb918682455c164ce8dd3acb71c751e2b9b2fc48913604069c9ea885fa378ca';
export const ORACLE_INTERFACE_HASH = '34238ab576c377b4ffc1ddbf557560f07a66baf879920191897bc784fa5a97ee';
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing version bump?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, was left unstaged 😅

@nchamo nchamo changed the title feat!(pxe): pass BoundedVec sizes as oracle params for validation request deserialization feat!(pxe): pass BoundedVec sizes as oracle params Mar 5, 2026
@nchamo nchamo changed the title feat!(pxe): pass BoundedVec sizes as oracle params feat(pxe)!: pass BoundedVec sizes as oracle params Mar 5, 2026
@nchamo nchamo requested a review from benesjan March 6, 2026 11:46
Copy link
Contributor

@benesjan benesjan left a comment

Choose a reason for hiding this comment

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

@nchamo given that we can still do breaking changes isn't this unnecessary, now?

Maybe instead we could just include the relevant capsule-related constants in the ORACLE_INTERFACE_HASH preimage such that this doesn't go unnoticed next time.

WDYT?

@nchamo
Copy link
Contributor Author

nchamo commented Mar 6, 2026

@benesjan , we already know that we want to increase the note max length from 8 to 10 in the future. It's just not a priority right now

I would argue that it would be best to make this breaking change now that we can, instead of trying to introduce a breaking change later

@benesjan
Copy link
Contributor

benesjan commented Mar 6, 2026

@nchamo wouldn't that breaking change be for v5? and hence not a problem?

@nchamo
Copy link
Contributor Author

nchamo commented Mar 6, 2026

If I understood you correctly @benesjan , you are suggesting that instead of preventing future breaking changes, we instead improve our tooling to detect them (which would have been very helpful in #20840). And I think you also assume that when we make the change 8 => 10 (or any other future changes to max note length), we will implement them at a time where we are comfortable introducing breaking changes

I think that approach could leave the door open to problems in the future, since we can't guarantee when changes like this would happen. I would suggest going ahead with this PR, since it implements a small change that would prevent all those potential issues

@benesjan
Copy link
Contributor

benesjan commented Mar 6, 2026

You understood it correctly.

since we can't guarantee when changes like this would happen.

In this case we would be the ones deciding to update the constant so I don't think this is the case. Or do you think some new use case appears that will make the time of this happening arbitrary?

IDK it seems unlikely that that would be urgent and could not wait for v5.

I think it's quite likely that these kind of tasks will be picked up before big breaking releases (just like was done with alpha where Mike starting revising all the constants).

@nchamo
Copy link
Contributor Author

nchamo commented Mar 6, 2026

I think the change from 8 => 10 will likely happen in v5 and it would be safe to do it then. But if we come up with some other improvement later on (10 => 11 for example), I don't know if we'll be comfortable doing a breaking change then. That's the risk I see right now

I personally think the risk/reward of detecting vs preventing breaking changes guides me towards preventing, specially when the fix is already implemented and it's quite small/easy

@benesjan
Copy link
Contributor

benesjan commented Mar 6, 2026

I am worried that this will just introduce additional complexity and then it will never be needed.

Let's wait for @nventuro to chime in.

@nventuro
Copy link
Contributor

nventuro commented Mar 7, 2026

It's not a lot of complexity, and 8->10 is something we can achieve with less than a day's work, with massive upside. Yes, we can change the oracles now, but the window will close in ~1 week, at which point we either delay changes or deal with awkward backwards compat.

Having robust oracle interfaces that can tolerate change is a win in my opinion, I like this change.

@nchamo nchamo marked this pull request as ready for review March 7, 2026 12:50
Copy link
Contributor

@benesjan benesjan left a comment

Choose a reason for hiding this comment

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

Overall the PR is very nice and clean. I was too much of a Karen about not merging this before. Sorry about that hehe

Just added few more nits.

) {}

static fromFields(fields: Fr[] | FieldReader): NoteValidationRequest {
static fromFields(fields: Fr[] | FieldReader, maxNotePackedLen: number): NoteValidationRequest {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
static fromFields(fields: Fr[] | FieldReader, maxNotePackedLen: number): NoteValidationRequest {
static fromFields(fields: Fr[], maxNotePackedLen: number): NoteValidationRequest {

Because of the error below I think we should drop the FieldReader from here because the pattern of passing field reader on the input is used when we do "nested deserialization" and in that case it would be legit to have fields of another object after the validation request.

(applies to EventValidationRequest as well).

if (reader.remainingFields() !== 0) {
throw new Error(
`Error converting array of fields to NoteValidationRequest. Hint: check that MAX_NOTE_PACKED_LEN is consistent with private_notes::MAX_NOTE_PACKED_LEN in Aztec-nr.`,
`Error converting array of fields to NoteValidationRequest: ${reader.remainingFields()} remaining fields (maxNotePackedLen=${maxNotePackedLen}).`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Here I would instead put into the error message the total expected number of fields for NoteValidationRequest and how much was received. Mentioning "remaining fields" is a bit unclear.

(also applies to the EventValidationRequest)


const MAX_PUBLIC_LOG_LEN_FOR_NOTE_COMPLETION = MAX_NOTE_PACKED_LEN;
const MAX_LOG_CONTENT_LEN = Math.max(MAX_PUBLIC_LOG_LEN_FOR_NOTE_COMPLETION, PRIVATE_LOG_CIPHERTEXT_LEN);
const MAX_LOG_CONTENT_LEN = PRIVATE_LOG_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.

But please correct me if I'm wrong or I'm missing something

Nial mentioned that this limit might not make sense as there is no reason for notes to not be longer in case they are delivered via offchain messages.

Could you expand on why this change was done here? I didn't review your other PRs that I assumed led to this so I don't have context here. Thanks

@nchamo nchamo requested a review from benesjan March 9, 2026 12:30
@AztecBot AztecBot requested a review from a team March 9, 2026 12:40
@nchamo nchamo removed the request for review from a team March 9, 2026 14:21
Copy link
Contributor

@benesjan benesjan left a comment

Choose a reason for hiding this comment

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

LGTM!

@nchamo nchamo enabled auto-merge (squash) March 10, 2026 10:35
@AztecBot AztecBot requested a review from a team March 10, 2026 10:35
@nchamo nchamo merged commit f92d2d0 into merge-train/fairies Mar 10, 2026
20 checks passed
@nchamo nchamo deleted the feat/oracle-param-bounded-vec-sizes branch March 10, 2026 10:35
github-merge-queue bot pushed a commit that referenced this pull request Mar 10, 2026
BEGIN_COMMIT_OVERRIDE
refactor: deduplicate MembershipWitness struct (#21244)
feat(pxe)!: pass BoundedVec sizes as oracle params (#21176)
refactor!: cleaning up public call and tx phase related oracles (#21209)
END_COMMIT_OVERRIDE
@ludamad
Copy link
Collaborator

ludamad commented Mar 11, 2026

backport-later handled: cherry-picked to v4-next in #21341

@AztecBot
Copy link
Collaborator

❌ Failed to cherry-pick to v4-next due to conflicts. Dispatching ClaudeBox to resolve. View backport run.

benesjan added a commit that referenced this pull request Mar 12, 2026
…rams

PR #21176 added maxNotePackedLen and maxEventSerializedLen parameters to
this oracle. Pinned protocol contracts call with the old 3-param signature,
so the legacy mapping needs an adapter that supplies the frozen default
values (8 and 10 respectively).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
benesjan added a commit that referenced this pull request Mar 12, 2026
…rams

PR #21176 added maxNotePackedLen and maxEventSerializedLen parameters to
this oracle. Pinned protocol contracts call with the old 3-param signature,
so the legacy mapping needs an adapter that supplies the frozen default
values (8 and 10 respectively).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
benesjan added a commit that referenced this pull request Mar 12, 2026
…rams

PR #21176 added maxNotePackedLen and maxEventSerializedLen parameters to
this oracle. Pinned protocol contracts call with the old 3-param signature,
so the legacy mapping needs an adapter that supplies the frozen default
values (8 and 10 respectively).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ludamad pushed a commit that referenced this pull request Mar 12, 2026
…rams

PR #21176 added maxNotePackedLen and maxEventSerializedLen parameters to
this oracle. Pinned protocol contracts call with the old 3-param signature,
so the legacy mapping needs an adapter that supplies the frozen default
values (8 and 10 respectively).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
nventuro added a commit that referenced this pull request Mar 13, 2026
## Summary
- Backports 5 PRs from `merge-train/fairies` related to oracle changes:
  - #21101 - feat: improve oracle name prefixes
- #21349 - fix: skip oracle version check for pinned protocol contracts
  - #21244 - refactor: deduplicate MembershipWitness struct
  - #21176 - feat(pxe)!: pass BoundedVec sizes as oracle params
- #21209 - refactor!: cleaning up public call and tx phase related
oracles

## Test plan
- CI passes on backport branch

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Nicolás Venturo <nicolas.venturo@gmail.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Co-authored-by: Nicolas Chamo <nicolas@chamo.com.ar>
Co-authored-by: benesjan <benesjan@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants