feat: Delayed merge implementation#22775
feat: Delayed merge implementation#22775federicobarbacovi wants to merge 42 commits intomerge-train/barretenbergfrom
Conversation
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
federicobarbacovi
force-pushed
the
fb/delayed_merge_impl
branch
from
ce5fe78 to
de5b622
Compare
Co-authored-by: Copilot <copilot@github.com>
federicobarbacovi
added
ci-full
…y merge in favor of translator-merge
ledwards2225
left a comment
ledwards2225
left a comment
There was a problem hiding this comment.
Looks good! Basically reviewed everything but want to spend a bit more time tomorrow. Submitting this now so you can see some of my comments. Mostly minor suggestions
| update_native_verifier_accumulator(queue_entry, verifier_transcript); | ||
| #endif | ||
| goblin.prove_merge(prover_accumulation_transcript); | ||
| // Delayed merge: keep one subtable per folded circuit and prove the batched merge after the tail kernel. |
There was a problem hiding this comment.
Can't comment on unchanged lines so this is intended for the code above: it seems like HN_TAIL is no longer needed since AFAIK the only reason for it to exist was to add the blinding that's now handled by the batched merge
There was a problem hiding this comment.
This is correct, agreed to tackle this in a follow up as it touches Noir as well
| * @brief Create a mock batch merge proof which has the correct structure but is not necessarily valid | ||
| * | ||
| */ | ||
| bb::HonkProof create_mock_batch_merge_proof(bool is_zk = true); |
There was a problem hiding this comment.
It seems this is_zk param is always true. Are you just maintaining it based on the idea of doing goblin resets or something where we'd prefer not to use ZK?
…e in batch verifier Co-authored-by: Copilot <copilot@github.com>
…ification in is_hiding_kernel branch
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
…b/delayed_merge_impl
Co-authored-by: Copilot <copilot@github.com>
ledwards2225
left a comment
ledwards2225
left a comment
There was a problem hiding this comment.
LGTM. Did another pass though everything, added some tests and tweaked some out of date comments. Fixed what I think was a bug where we were adding the same random ops 3 times instead of generating them uniquely. Overall quite happy with how simple the changes are here. I'll leave it to you to decide if there's more testing/tweaking/assessing you want to do.
| BB_ASSERT_EQ(current_stdlib_verifier_accumulator.has_value(), false); | ||
|
|
||
| // Perform batch merge verification | ||
| auto [batch_pairing_points, batch_merged_table_commitments] = |
There was a problem hiding this comment.
nice! the changes in this file are so clean now
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
…b/delayed_merge_impl
3 participants
.