feat: Delayed merge implementation#22775
Conversation
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
ce5fe78 to
de5b622
Compare
Co-authored-by: Copilot <copilot@github.com>
…y merge in favor of translator-merge
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
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
…22906) Fixes the merge-train/barretenberg merge-queue failure: http://ci.aztec-labs.com/1617122ce095114b ## Summary `compute_running_hash` in `batch_merge.test.cpp` (added by #22775) declared `bb::fr hash_challenge(0)` but never used it. Under `-Werror -Wall`, the ARM build fails with `[-Wunused-variable]`. The fix removes the dead declaration. ## Why ci-full on #22775 didn't catch it Clang's `-Wunused-variable` is suppressed when a variable's constructor has visible side effects. On x86 (`DISABLE_ASM=OFF`), `field<T>` constructors call `self_to_montgomery_form()` → `operator*=` → `asm_self_mul_with_coarse_reduction`, which contains an `__asm__()` block with a `"memory"` clobber. Clang treats inline asm as a side effect and silences the warning. On ARM, `CMakeLists.txt` auto-sets `DISABLE_ASM=ON` (line 50–57), so `bb::fr(0)` resolves to a pure `constexpr` arithmetic path with no observable side effect — clang then issues the warning. `ci-full` (label) runs only on amd64 (see `ci.sh` `full` target — single `bootstrap_ec2 ci-full` on x86), so the asm side-effect masked the warning. The merge queue (`ci.sh merge-queue`) is what adds the arm64 leg (`a1-fast arm64 ci-fast`), which is why the failure surfaced only at merge time. ClaudeBox log: https://claudebox.work/s/e0102d5ad296fd89?run=1
BEGIN_COMMIT_OVERRIDE
fix(ci): default S3_BUILD_CACHE_AWS_PARAMS in cache_s3_transfer{,_to}
(AztecProtocol#22898)
chore: low-hanging chonk prover fixes from profiling (AztecProtocol#22855)
chore: fuse N `add_scaled` into one `parallel_for` (AztecProtocol#22893)
feat: Delayed merge implementation (AztecProtocol#22775)
chore: numeric audit response (AztecProtocol#22856)
fix: harden BN254 G2 SRS ingress (AztecProtocol#22858)
fix: remove unused hash_challenge variable in batch_merge.test.cpp
(AztecProtocol#22906)
fix(bbup): remove jq dependency (AztecProtocol#22912)
chore: fix g2 test failing on merge-train (AztecProtocol#22920)
fix(ci): error on disabled-cache in CI hash calculation (AztecProtocol#22904)
END_COMMIT_OVERRIDE
Implementation of the delayed merge: we replace the single merges in the kernels with a final batch merge in the hiding kernel. Description of the batch merge protocol is the relevant md file