taikoxyz / zkevm-circuits

DEPRECATED in favor of https://github.com/taikoxyz/raiko! Taiko's fork of the PSE's ZK-EVM
Other
159 stars 125 forks source link

PI refactor #138

Open CeciliaZ030 opened 1 year ago

CeciliaZ030 commented 1 year ago

Description

Refactor the PI circuit in circuit-tool style.

CeciliaZ030 commented 1 year ago

I have one concern tho... @Brechtpd do you think for bytes: [Cell<F>; N] within FieldBytesGadget I should directly fill in in increment with r? Remember in the original accumulation column increment all the way and it was just one constraint like next = cur * r + byte which has low degree, but mine has block_accc[i+1] = ((byte0 * r) + byte1 * r) * ... + byte31 which causes high degree. I know we can reduce it with split_expression but that's also some amount of overhead. For simple circuit like this refactoring with circuit-tools i just worry about performance degrade in general 😭 and whether it's worthy to tradeoff performance over readability.

Brechtpd commented 1 year ago

I have one concern tho... @Brechtpd do you think for bytes: [Cell<F>; N] within FieldBytesGadget I should directly fill in in increment with r? Remember in the original accumulation column increment all the way and it was just one constraint like next = cur * r + byte which has low degree, but mine has block_accc[i+1] = ((byte0 * r) + byte1 * r) * ... + byte31 which causes high degree. I know we can reduce it with split_expression but that's also some amount of overhead. For simple circuit like this refactoring with circuit-tools i just worry about performance degrade in general 😭 and whether it's worthy to tradeoff performance over readability.

smtmfft commented 1 year ago

Does this support new PI format?? https://github.com/taikoxyz/taiko-mono/blob/85bef055c8778a473fff41318b06792c151efa52/packages/protocol/contracts/L1/libs/LibProving.sol#L166C42-L166C50 BTW: evidence: https://github.com/taikoxyz/taiko-mono/blob/85bef055c8778a473fff41318b06792c151efa52/packages/protocol/contracts/L1/TaikoData.sol#L120

smtmfft commented 1 year ago

For A5 integration, after functionality test, please merge this PR to feat/a5-testnet branch.