search

privacy-scaling-explorations / zkevm-circuits

https://privacy-scaling-explorations.github.io/zkevm-circuits/
Other
819 stars 853 forks source link

Discussion: Be more agressive with panicking on errors #1093

Open ed255 opened 1 year ago

ed255 commented 1 year ago

There are many sanity checks that can be performed during the various stages of the proof input preparation, witness calculation, etc. I think we should encourage adding those kind of sanity checks with the philosophy of detecting issues as early as possible (which means detecting them closer to where they happen, which gives us better context).

For testing purposes, I think the best option is to have failures of such sanity checks panic because then we know exactly where they fail, and it forces us to not introduce new bugs via PRs (because unit tests must pass). On the other hand, these checks / panics may be inconvenient in testnet environments because:

  1. A panic will stop the prover, which may make the testnet to become stuck
  2. Checks have some overhead which may slow down things (this also affects benchmarks and production)

So maybe a good solution is to still encourage having these checks, and use flags to: a. Turn panics into log::error (Similar to this approach for unimplemented functionality https://github.com/privacy-scaling-explorations/zkevm-circuits/issues/1072 ) b. Disable checks altogether for performance reasons

As an example of where this can apply: RwLookups checks (they are currently disabled in the github workflows because we don't run tests with RUST_LOG=debug, and even then, they don't panic, so tests would still pass) https://github.com/privacy-scaling-explorations/zkevm-circuits/blob/98646613d517f473489d7b18d4ea8b53a43b5e11/zkevm-circuits/src/evm_circuit/execution.rs#L1231-L1244

Also in this new check I introduce in this PR (I think the overhead for this one is very small, but we still have the panic/log dilemma): https://github.com/privacy-scaling-explorations/zkevm-circuits/pull/1092

pinkiebell commented 1 year ago

I think it helps to have something like warn_panic or warn_error and re-use the warn-unimplemented feature flag, maybe rename it. And also add this flag to the zkevm-circuits crate and propagate to eth-types. That will make third-party e.g (zkevm-chain) integration easier.

ed255 commented 1 year ago

I think it helps to have something like warn_panic or warn_error and re-use the warn-unimplemented feature flag, maybe rename it. And also add this flag to the zkevm-circuits crate and propagate to eth-types. That will make third-party e.g (zkevm-chain) integration easier.

In the meantime (while we don't have the feature in the zkevm-circuits crate), I think that if you import eth-types with the warn-unimplemented feature, you should get the desired result (that is, zkevm-circuits won't panic on unimplemented things).