Skip to content
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.

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

feat: Make public inputs the start of the UH and MH proof #12266

Merged
merged 52 commits into from
Mar 12, 2025

Conversation

lucasxia01
Copy link
Contributor

@lucasxia01 lucasxia01 commented Feb 25, 2025

Closes AztecProtocol/barretenberg#1089.

Removes the circuitSize, numPublicInputs, and pubInputsOffset metadata from the proof. The result is that the public inputs are at the beginning of the proof.

Followup work includes:

Base automatically changed from lx/transcript-allow-hashing-without-sending to master February 25, 2025 22:18
@lucasxia01 lucasxia01 marked this pull request as ready for review February 26, 2025 14:45
@Maddiaa0
Copy link
Member

Question, if there is no offset anymore can it just be removed? What was it for ?

@lucasxia01
Copy link
Contributor Author

Question, if there is no offset anymore can it just be removed? What was it for ?

No its still there in the vk, just not redundantly in the proof. The pub_inputs_offset basically represents the number of ecc_ops. I'm not sure if it needs to exist anymore if we don't use MegaHonk with an unstructured trace.

@ledwards2225
Copy link
Contributor

Question, if there is no offset anymore can it just be removed? What was it for ?

TBC there are two "public input offset" variables floating around. One describes the offset of the pub inputs stored in the proof (previously 3, now 0). That one can probably go away. The other one describes the index at which the public inputs are stored in the wires. For UH it is always 1 (since we have a "zero-row" to accommodate shifts). For MH its something else. Its possible this one can also be removed but that's not effected by this PR

Copy link
Contributor

@ledwards2225 ledwards2225 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delightful, thanks for doing this. I'm assuming this will effect some noir devs who are manually splitting pub inputs from the proof using this secret magic 3? @saleel Is there any reason to hold off on this until docs are updated or something?

// Commitments
p.w1 = bytesToG1ProofPoint(proof[0x60:0xe0]);
p.w1 = bytesToG1ProofPoint(proof[0x0:0x80]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank god for GPT for things like this..

@saleel
Copy link
Member

saleel commented Mar 5, 2025

Delightful, thanks for doing this. I'm assuming this will effect some noir devs who are manually splitting pub inputs from the proof using this secret magic 3? @saleel Is there any reason to hold off on this until docs are updated or something?

🙏
Looks like this will this also change the output from the exposed wasm methods? If so, I think we would need to update bb.js classes

@@ -253,39 +250,40 @@ export class UltraHonkBackend {
gunzip(compressedWitness),
);

// Write VK to get the VK
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra call is not ideal since its more work just to fetch the number of public inputs. The vk or maybe just the number of public inputs should be returned by proof generation perhaps.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the bb.js interface just needs to be reworked similarly to the bb interface so that prove can output both a proof and a vk...

@codygunton
Copy link
Contributor

Will #11024 eventually be handled by your series of works?

@lucasxia01
Copy link
Contributor Author

Will #11024 eventually be handled by your series of works?

@codygunton yep, Saleel brought that up as well and that's on my radar as a followup in the immediate future.

@lucasxia01 lucasxia01 added the ci-full Run all master checks. label Mar 6, 2025
@lucasxia01 lucasxia01 merged commit b6294ad into master Mar 12, 2025
6 checks passed
@lucasxia01 lucasxia01 deleted the lx/public-inputs-at-proof-start branch March 12, 2025 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-full Run all master checks.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move public inputs to the front of proof
6 participants