Skip to content

feat: Do not deserialize Brillig even with bincode #13143

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
aakoshh wants to merge 1 commit into from
Closed

feat: Do not deserialize Brillig even with bincode #13143

aakoshh wants to merge 1 commit into from

Conversation

@aakoshh
Copy link
Contributor

@aakoshh aakoshh commented Mar 28, 2025 •
edited
Loading

Followup for #12841

I missed this opportunity: we don't need the Brillig parts in Barretenberg, and I think it's okay to ignore the last field in the struct with bincode.

Nope

Unfortunately this doesn't work:

17:30:59 command failed: /home/aztec-dev/aztec-packages/barretenberg/cpp/build/bin/bb prove --scheme ultra_honk --init_kzg_accumulator  --output_format fields --write_vk -o /tmp/tmp.nateEgQBpw -b ./target/program.json -w ./target/witness.gz (exit: 1)
17:30:59 --- stdout ---
17:30:59 --- stderr ---
17:30:59 Scheme is: ultra_honk
17:30:59 Some input bytes were not read

That means if we want to ignore the brillig part we have to use msgpack, or add another kind of format to nargo which is binpack without brillig 😞

Or we have to have a multi-stage serialization scheme where we treat the brillig part as opaque bytes, and don't further deserialise in Barretenberg.

@aakoshh aakoshh closed this Mar 28, 2025
@aakoshh aakoshh mentioned this pull request Mar 28, 2025
Merged
aakoshh added a commit that referenced this pull request Mar 28, 2025
@aakoshh
feat(noir): Allow missing optional fields in msgpack (#13141)
493dede 
Followup for #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 #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
#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.
AztecBot pushed a commit to AztecProtocol/barretenberg that referenced this pull request Mar 29, 2025
@aakoshh
feat(noir): Allow missing optional fields in msgpack (#13141)
be402b1 
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TomAFrench TomAFrench Awaiting requested review from TomAFrench

@ludamad ludamad Awaiting requested review from ludamad

@asterite asterite Awaiting requested review from asterite

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@aakoshh