fix(pxe): respect slot boundaries in oracle (de)serialization#24211
fix(pxe): respect slot boundaries in oracle (de)serialization#24211nchamo wants to merge 5 commits into
Conversation
nchamo
changed the title
Flakey Tests🤖 says: This CI run detected 2 tests that failed, but were tolerated due to a .test_patterns.yml entry. |
| private constructor( | ||
| public readonly value: T | undefined, | ||
| public readonly template: T | undefined, | ||
| // `unknown` (not the descriptor shape) keeps this generic Option decoupled from the serialization layer's |
There was a problem hiding this comment.
This is so that we can do things like Option.none({ maxLength: ciphertext.maxLength });
I tried passing a Size type all the way from the registry to the Option, but in most cases, a size isn't needed. So I thought the extra type added more noise than it helped
| params: [{ name: 'pkMHash', type: FIELD }], | ||
| params: [ | ||
| { name: 'pkMHash', type: FIELD }, | ||
| { name: 'keyIndex', type: FIELD }, |
There was a problem hiding this comment.
The keyIndex was not used, but it was present on the oracle. So I added it so that tests passed
| /// | ||
| /// These constants must be kept in sync between this file and `noir-projects/aztec-nr/aztec/src/oracle/version.nr`. | ||
| export const ORACLE_INTERFACE_HASH = 'f5cd3321b32371186f30dfd11b246946fb425cadffb8e6564b897d5184e43fe9'; | ||
| export const ORACLE_INTERFACE_HASH = '04e973e69ac73ab958cec7908abcfc40fdac5615a10c890808928c962d91c652'; |
There was a problem hiding this comment.
This was needed because I added keyIndex to the registry. But nothing really changed, it was a parameter previously ignored
| * Most tests *round-trip*: serialize a value, deserialize the result, and assert it comes back unchanged (see the | ||
| * `roundTrip` helper at the bottom). The rest pin a specific encoding, or check that malformed wire input is rejected. | ||
| */ | ||
| describe('oracle type mappings', () => { |
There was a problem hiding this comment.
I moved many of the tests from oracle_registry.test.ts here
| }); | ||
| // Every input slot must be modelled by a param: oracles whose Noir decl passes an extra field must declare it | ||
| // (the handler can ignore it). Otherwise an under-declared shape would silently drop a field into nothing. | ||
| if (offset !== inputs.length) { |
There was a problem hiding this comment.
This new check asserts we are not messing up
| export const BLOCK_HEADER: TypeMapping<BlockHeader> = { | ||
| serialization: { fn: v => v.toFields() }, | ||
| deserialization: { fn: ([reader]) => BlockHeader.fromFields(reader.readFieldArray(BLOCK_HEADER_LENGTH)), slots: 1 }, | ||
| shape: Array<SlotShape>(BLOCK_HEADER_LENGTH).fill('scalar'), |
There was a problem hiding this comment.
Other cases like this one where we have to say "this is an array of many scalars" will go away in a PR I'm working on that introduces the helper STRUCT (like we have ARRAY or BOUNDED_VEC today). When we introduce that, this will be calculated automatically
3 participants
Motivation
When deserializing an oracle's inputs, every slot of a value was read through a single shared
FieldReadercursor with no notion of where one slot ends and the next begins. For a fixed single-field value this is fine, but a value spanning multiple fields had no boundary to stop at. The clearest symptom: aNoneOptionnested inside anARRAY/BOUNDED_VECdrained all remaining fields of the collection (it calledreader.skip(remaining)), swallowing the elements after it instead of skipping just its own zero-padding.The change
Each
TypeMappingnow declares ashape: the wire width of every slot it occupies. Deserialization slices a value's fields into oneFieldReaderper slot viasplitByShape, so slot boundaries are structural instead of being tracked inside a shared cursor. This replaces the old per-mappingslots: numbercount.shape, so aNoneskips only its own fields.Optionderives itsNonepadding from the inner'sshapeinstead of atemplate, so aNonetakes no argument (or just a size descriptor for variable-size types).