Commit be402b1
feat(noir): Allow missing optional fields in msgpack (#13141)
Followup for AztecProtocol/aztec-packages#12841
Changes code generation for msgpack so that it doesn't throw an error if
an optional field of a `struct` is not present in the data. This is to
allow @TomAFrench and @asterite to delete `Opcode::MemoryOp::predicate`
which is an optional field that we no longer use. Removing such a field
should be a non-breaking change, as the field was optional to begin
with, so while even if it's present in C++ it should already handle it
being empty.
Unfortunately the `msgpack-c` library as far as I can see [would throw
an
error](https://github.com/AztecProtocol/msgpack-c/blob/54e9865b84bbdc73cfbf8d1d437dbf769b64e386/include/msgpack/v1/adaptor/detail/cpp11_define_map.hpp#L33-L45)
if the optional field would not be present in the data as NIL.
For this to work the PR re-introduces the `Helpers` and enumerates
fields explicitly, instead of using `MSGPACK_FIELDS`. I changed the
unmerged noir-lang/noir#7716 to do codegen with
this change.
I rebased AztecProtocol/aztec-packages#13021 on
top of this PR to see if it works when msgpack is actually in use.
### Note for future migration path
@ludamad reached out that while the bytecode size increase shown in
noir-lang/noir#7690 doesn't seem too bad, and
compression compensates for the inclusion of string field names, it's
still wasteful to have to parse them, and it would be better to use
arrays.
I established in
[tests](https://github.com/noir-lang/noir/pull/7690/files#diff-2d66028e5a8966511a76d1740d752be294c0b6a46e0a567bc2959f91d9ce224bR169-R176)
that we what we call `msgpack-compact` format uses arrays for structs,
but still tags enums with types. We use a lot of enums, so there is
still quite a few strings. Ostensibly we could use [serde
attributes](https://serde.rs/container-attrs.html) to shorten names, but
it would probably be a nightmare and ugly.
Nevertheless if we generated C++ code to deal with arrays, we could save
some space.
And if we want to stick to `bincode`, we can use
`NOIR_SERIALIZATION_FORMAT=bincode`, which I back ported to the Noir
codegen PR, to generate `bincode` with a format byte marker. There is
also `bincode-legacy`, but unfortunately we only have one shot at
deserialising bincode in C++: if it fails, we can't catch the exception.
Therefore the path to be able to use the bincode format marker is:
1. Release `bb` which can handle the `msgpack` format (which has a
probe, doesn't have to throw)
2. Start producing msgpack data from `nargo`
3. Stop accepting unmarked bincode in `bb` and look for format byte == 1
to show that bincode is in use
4. Tell `nargo` which format to use
EDIT: Unfortunately if we use `binpack` with today's data types it
forces us to parse the Brillig part, as established by
AztecProtocol/aztec-packages#13143 which would
mean the Noir team can't make any changes to Brillig opcodes without
breaking `bb`. We would need to change the format again to use two tier
encoding, or use msgpack arrays.1 parent 1a86e16 commit be402b1
File tree
3 files changed
+1263
-84
lines changed- cpp/src/barretenberg/dsl/acir_format
- serde
3 files changed
+1263
-84
lines changedLines changed: 3 additions & 3 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
32 | 32 | | |
33 | 33 | | |
34 | 34 | | |
35 | | - | |
| 35 | + | |
36 | 36 | | |
37 | 37 | | |
38 | 38 | | |
| |||
67 | 67 | | |
68 | 68 | | |
69 | 69 | | |
70 | | - | |
| 70 | + | |
71 | 71 | | |
72 | 72 | | |
73 | 73 | | |
74 | 74 | | |
75 | | - | |
| 75 | + | |
76 | 76 | | |
77 | 77 | | |
78 | 78 | | |
| |||
0 commit comments