Skip to content
This repository has been archived by the owner on Apr 9, 2024. It is now read-only.

feat(acir): Add length to BlackBoxFuncCall serialization #285

Closed
wants to merge 1 commit into from

Conversation

vezenovm
Copy link
Contributor

Related issue(s)

Works towards resolving #234.

Description

After #269 we moved BlackBoxFuncCall to struct variants with explicit function signatures. The inputs are now separated which is a welcome change. However, the way BlackBoxFuncCalls are currently being written still follows the old method when everything was handled in a flattened inputs vector. Now that we are splitting up the inputs into separate input fields this requires us to have hardcoded function signatures when serializing our function inputs. This is fine for our existing blackbox funcs as any current functions that have variable inputs have only one and it can be serialized as the last input of the function signature.

For recursion we can have multiple function inputs that all vary in size. VerifyProof will require a key, proof, public_inputs, key_hash, and input_aggregation_object. The key hash is a single element that can be computed by the respective backend. Every other function input can vary depending on the proving system. Thus the way we currently read in a flattened vector is not sufficient.

Summary of changes

I added a new struct FunctionInputIO that stores the length of a given function input. Singular function inputs
are not affected as the function signature determines whether we should fetch the first element of a vector when reading in the function inputs or return a vector with a single element.

Dependency additions / changes

(If applicable.)

Test additions / changes

I included one more black box func that has multiple vector based function inputs in the serialization_roundtrip test.

Checklist

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt with default settings.
  • I have linked this PR to the issue(s) that it resolves.
  • I have reviewed the changes on GitHub, line by line.
  • I have ensured all changes are covered in the description.

Additional context

(If applicable.)

@sirasistant
Copy link
Contributor

sirasistant commented May 16, 2023

Check out this PR https://github.com/noir-lang/acvm/pull/281/files
In this pr we already split the logic for reading each BB call independently, maybe we should just do that and have full custom serialization/deserialization per BB call. If the value is a fixed length array, just write N elements, if variable, write a variable one with write_inputs.
What do you think @vezenovm ? I think at this point and taking into account that some functions will also have constants, etc, it should end up being cleaner

@vezenovm
Copy link
Contributor Author

vezenovm commented May 16, 2023

In this pr we already split the logic for reading each BB call independently

Oh I missed this in that PR. I will check it out further. I am not sure how I feel about serializing each BB call independently when we still could just call read_inputs and read_outputs a single time. But I see your point about it being more clear exactly what is happening in each function.

if variable, write a variable one with write_inputs.

Either way the variable one will still need to know the size of the inputs when being read back. We would have to change the writes to also use custom method like you did for read in #281. I guess at that point I would just have a custom write/read method for VerifyProof that will handle the fact that we have multiple variable inputs and would include the size of the vector when writing/reading

@vezenovm
Copy link
Contributor Author

One more relevant comment in the PR review for #281 : #281 (comment)

@vezenovm
Copy link
Contributor Author

I think at this point and taking into account that some functions will also have constants, etc, it should end up being cleaner

This should be clear from the function signature in the BlackBoxFuncCall enum. If we wanted to we could turn FunctionInputIO into an enum as well and avoid having custom read/write methods

@vezenovm vezenovm mentioned this pull request May 16, 2023
5 tasks
@vezenovm
Copy link
Contributor Author

Closed as this issue is resolved by #286

@vezenovm vezenovm closed this May 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants