Skip to content

feat: Client IVC API#10217

Merged
codygunton merged 31 commits into
masterfrom
cg/api-civc
Dec 3, 2024
Merged

feat: Client IVC API#10217
codygunton merged 31 commits into
masterfrom
cg/api-civc

Conversation

@codygunton
Copy link
Copy Markdown
Contributor

@codygunton codygunton commented Nov 26, 2024

  • Establish API in purely virtual class
    • This is just a first pass. I will continue to work on this before showing dev rel and others to get buy-in.
  • Implement some API functions for ClientIVC: prove, verify, prove_and_verify
  • Support for constructing CIVC proof for input a single circuit
    • This is interpreted as a "compiletime stack"
    • Produces ECCVM and Translator proofs from dummy/empty data; future optimization could avoid.
    • Add one_circuit to CIVC to encode whether the MH part of the CIVC proof should be a hiding circuit (which takes a folding proof) or a proof for the single circuit.
  • Run almost all ACIR tests against ClientIVC
  • fold_and_verify and mega honk flows go away in bb, but remain until bb.js alignment.
  • Delete large log file that should not be track (accounts for big negative diff).

@codygunton codygunton self-assigned this Nov 26, 2024
@AztecProtocol AztecProtocol deleted a comment from github-actions Bot Nov 29, 2024
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Nov 30, 2024

Changes to circuit sizes

Generated at commit: 6abb1875eeb12d0b31075349aa2d583fa2652259, compared to commit: da1470d074f4884e61b51e450a661432c6f0a10f

🧾 Summary (100% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
parity_root 0 ➖ 0.00% -8 ✅ -0.00%
rollup_block_root 0 ➖ 0.00% -9 ✅ -0.00%
rollup_block_merge 0 ➖ 0.00% -8 ✅ -0.00%
rollup_root 0 ➖ 0.00% -8 ✅ -0.00%
rollup_merge 0 ➖ 0.00% -8 ✅ -0.00%
private_kernel_empty -350 ✅ -36.34% -358 ✅ -0.04%
private_kernel_reset -5,459 ✅ -5.74% -5,038 ✅ -0.78%
private_kernel_inner -7,834 ✅ -17.56% -6,125 ✅ -9.88%
private_kernel_init -5,673 ✅ -21.54% -4,623 ✅ -12.12%
private_kernel_tail_to_public -2,249 ✅ -12.85% -4,091 ✅ -13.39%
private_kernel_tail -1,200 ✅ -22.53% -2,667 ✅ -17.57%
rollup_base_public -107,265 ✅ -23.61% -1,144,794 ✅ -22.96%
rollup_base_private -54,054 ✅ -24.55% -989,829 ✅ -27.51%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
parity_root 4,782 (0) 0.00% 3,636,536 (-8) -0.00%
rollup_block_root 4,192 (0) 0.00% 2,739,373 (-9) -0.00%
rollup_block_merge 11,544 (0) 0.00% 1,858,073 (-8) -0.00%
rollup_root 11,528 (0) 0.00% 1,858,059 (-8) -0.00%
rollup_merge 3,252 (0) 0.00% 1,826,915 (-8) -0.00%
private_kernel_empty 613 (-350) -36.34% 901,640 (-358) -0.04%
private_kernel_reset 89,662 (-5,459) -5.74% 637,479 (-5,038) -0.78%
private_kernel_inner 36,767 (-7,834) -17.56% 55,848 (-6,125) -9.88%
private_kernel_init 20,668 (-5,673) -21.54% 33,520 (-4,623) -12.12%
private_kernel_tail_to_public 15,253 (-2,249) -12.85% 26,461 (-4,091) -13.39%
private_kernel_tail 4,127 (-1,200) -22.53% 12,509 (-2,667) -17.57%
rollup_base_public 347,034 (-107,265) -23.61% 3,841,978 (-1,144,794) -22.96%
rollup_base_private 166,132 (-54,054) -24.55% 2,608,307 (-989,829) -27.51%

Copy link
Copy Markdown

@maramihali maramihali left a comment

Choose a reason for hiding this comment

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

Nice work, left some small request for clarification/docs and a question.

"node": ">=18"
}
} No newline at end of file
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

extra lines in the two json files could be removed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The formatter added those for me, not sure why--maybe formatting config has changed since the last time those files changed?

* @param precomputed_vk
*/
void ClientIVC::accumulate(ClientCircuit& circuit,
const bool _one_circuit,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

will you add a comment explaining the point of this boolean?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure

// proof
if (!initialized) {
OinkProver<Flavor> oink_prover{ proving_key };
if (_one_circuit) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

would be helpful to begin with a comment saying this is handling the edge case where there's a single circuit in the IVC schme

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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


namespace bb {

acir_format::WitnessVector get_witness(std::string const& witness_path)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

it would be great to have docs for each of this getter but not part of the focus of this PR.

return content;
}

class ClientIVCAPI : public API {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

it would be great overall if this class had a bit more documentation


std::vector<AcirProgram> folding_stack;

// TODO(https://github.com/AztecProtocol/barretenberg/issues/1162): Efficiently unify ACIR stack parsing
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nit: we should uniformly either use - or _ in input type

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think I did achieve this, but I see that some of the comments about deprecation weren't updated to refer to the _ version of things. I suspect this is what you're referring to?

return verified;
};

void gates([[maybe_unused]] const API::Flags& flags,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

add a TODO for this three functions

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

or actually I guess they are not client ivc related but as a next step we should create an ultra honk api class as well?

const std::filesystem::path& output_dir) override
{
if (!flags.output_type || *flags.output_type != "fields_msgpack") {
throw_or_abort("No output_type or output_type not supported");
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

why do flags need to be dereferenced?

Copy link
Copy Markdown
Contributor Author

@codygunton codygunton Dec 3, 2024

Choose a reason for hiding this comment

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

That's one way of accessing the value in a std::optional when the optional actually does contain a value.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In any case, this validation is going to be replaced with the more robust functionality in https://github.com/CLIUtils/CLI11

@codygunton codygunton merged commit cc54a1e into master Dec 3, 2024
@codygunton codygunton deleted the cg/api-civc branch December 3, 2024 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants