fix(pxe): backward & forward compatible deserialization of validation requests#20957
fix(pxe): backward & forward compatible deserialization of validation requests#20957nchamo wants to merge 7 commits intomerge-train/fairiesfrom
Conversation
| import { MAX_NOTE_PACKED_LEN } from './note_validation_request.js'; | ||
|
|
||
| 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); |
There was a problem hiding this comment.
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
yarn-project/pxe/src/contract_function_simulator/noir-structs/event_validation_request.ts
Outdated
Show resolved
Hide resolved
yarn-project/pxe/src/contract_function_simulator/noir-structs/event_validation_request.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Regarding the approaches you described in the PR description I am not sure approach 2 is preferable because it has a lot of assumptions.
Why not go with approach 1? Is it because we no longer want to break oracles? (which approach 1 would probably break as keeping older and newer version of the oracle seem too messy)
yarn-project/pxe/src/contract_function_simulator/noir-structs/note_validation_request.ts
Outdated
Show resolved
Hide resolved
yarn-project/pxe/src/contract_function_simulator/noir-structs/note_validation_request.ts
Outdated
Show resolved
Hide resolved
yarn-project/pxe/src/contract_function_simulator/noir-structs/note_validation_request.ts
Outdated
Show resolved
Hide resolved
…ult for old contracts
benesjan
left a comment
benesjan
left a comment
There was a problem hiding this comment.
I think this approach is nice so would go ahead with it.
Pretty cool how flexible are capsules.
Feels like we could use this approach in future oracle backwards compatibility issues as well.
Please request me for a review again once you polish it 😊
nchamo
changed the title
| oracle::capsules::store( | ||
| contract_address, | ||
| EVENT_BOUNDED_VEC_CAPACITY_SLOT, | ||
| [MAX_EVENT_SERIALIZED_LEN as Field], |
There was a problem hiding this comment.
waaaaat? Doesn't the PR description say that we can just pass these values as params to the oracle? that seems much saner.
|
Closing in favor of #21176 |
3 participants
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
BoundedVecwhen communicating with PXE. And, on PXE's side, we also used that value to deserialize theBoundedVec.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 using capsules and we are setting the defaults to the values before #20840 was introduced. This makes this approach backwards and forwards compatible
We are taking a different approach in #21176, that would only help is in the to prevent this from happening again, but it would be a breaking change
Fixes #14617
Fixes F-375