Conversation
…m/opcode-in-both-rows
…m/opcode-in-both-rows
maramihali
changed the base branch from
mm/translator-detects-no-op
to
mm/opcode-in-both-rows
maramihali
force-pushed
the
mm/zk-civc
branch
from
f67adb9 to
e4f39eb
Compare
maramihali
requested review from
IlyasRidhuan,
fcarreiro and
jeanmon
as code owners
| mega_builder.queue_ecc_no_op(); | ||
|
|
||
| mega_builder.queue_ecc_random_op(); | ||
| mega_builder.queue_ecc_random_op(); | ||
| mega_builder.queue_ecc_random_op(); |
There was a problem hiding this comment.
could move this into a function in goblin for the avm_mode
There was a problem hiding this comment.
Do these need to be "random" instead of no-ops? If that's the case then does feel like a design flaw somewhere but maybe not critical. Does seem like it might be better off as a method in goblin with an explanation
There was a problem hiding this comment.
random ops are handled differently than no-ops in Translator, particularly because the op wire will not be populated with the opcode at even indices and 0 at odd indices but rather with two random values and translator expects, we can make it so we don't handle random ops at the beginning of the trace in avm_mode but seemed an unnecesary complication. Isolated to a function in goblin
|
If you want to check if this affects the AVM you can run the |
|
@fcarreiro what I mentioned above is an insignificant amount of work (6 extra ultra ops on top of the 7000 from the AVM recursive verifier), double checked by running the tests I dont see a change in times |
Oh I think then I misunderstood. I see a special |
ledwards2225
left a comment
ledwards2225
left a comment
There was a problem hiding this comment.
Woo! The end of an era. Nice work. Left some comments about things I think could be clarified
...rg/cpp/src/barretenberg/stdlib/translator_vm_verifier/translator_recursive_verifier.test.cpp
Outdated
Show resolved
Hide resolved
barretenberg/cpp/src/barretenberg/translator_vm/translator.test.cpp
Outdated
Show resolved
Hide resolved
barretenberg/cpp/src/barretenberg/translator_vm/translator_circuit_builder.cpp
Outdated
Show resolved
Hide resolved
| // information about the op queue content linked to the circuits being proven | ||
| static constexpr size_t RESULT_ROW = 8; | ||
| static constexpr size_t NUM_NO_OPS_START = 1; | ||
| static_assert(NUM_NO_OPS_START == 1); |
There was a problem hiding this comment.
Is the intent of these static asserts just to signal that these are not parameters that can be tweaked?
There was a problem hiding this comment.
yes, or that a change to these parameter might require further changes, similar to how we have static_asserts for relation degrees in flavors, but maybe they are a bit overkill (especially the no-op one I think can be removed), I tried my best to not hardcode constants in the code though
| mega_builder.queue_ecc_no_op(); | ||
|
|
||
| mega_builder.queue_ecc_random_op(); | ||
| mega_builder.queue_ecc_random_op(); | ||
| mega_builder.queue_ecc_random_op(); |
There was a problem hiding this comment.
Do these need to be "random" instead of no-ops? If that's the case then does feel like a design flaw somewhere but maybe not critical. Does seem like it might be better off as a method in goblin with an explanation
At last! Add randomness to the UltraOp version of the op queue and ensure proper handling in Translator. This makes the Translator and Merge proof actually zero knowledge. The caveat is, because Translator is zero-knowledge by default, we have to ensure these changes don't affect the AVM recursive verifier. There, we only ever accumulate one circuit so we don't want to do an artificial append merge with offset. As such, I opted for translator not expecting to process random ops at the end of the op queue when the op queue is derived from the goblin recursive verifier circuit. We however add the three random ops at the beginning of the op queue by default as otherwise we'd have to alter Translator relations based on whether Goblin is used in AVM or CIVC. Closes AztecProtocol/barretenberg#1360
At last! Add randomness to the UltraOp version of the op queue and ensure proper handling in Translator. This makes the Translator and Merge proof actually zero knowledge. The caveat is, because Translator is zero-knowledge by default, we have to ensure these changes don't affect the AVM recursive verifier. There, we only ever accumulate one circuit so we don't want to do an artificial append merge with offset. As such, I opted for translator not expecting to process random ops at the end of the op queue when the op queue is derived from the goblin recursive verifier circuit. We however add the three random ops at the beginning of the op queue by default as otherwise we'd have to alter Translator relations based on whether Goblin is used in AVM or CIVC. Closes AztecProtocol/barretenberg#1360
3 participants
At last! Add randomness to the UltraOp version of the op queue and ensure proper handling in Translator. This makes the Translator and Merge proof actually zero knowledge.
The caveat is, because Translator is zero-knowledge by default, we have to ensure these changes don't affect the AVM recursive verifier. There, we only ever accumulate one circuit so we don't want to do an artificial append merge with offset. As such, I opted for translator not expecting to process random ops at the end of the op queue when the op queue is derived from the goblin recursive verifier circuit. We however add the three random ops at the beginning of the op queue by default as otherwise we'd have to alter Translator relations based on whether Goblin is used in AVM or CIVC.
Closes AztecProtocol/barretenberg#1360