zkevm-circuits icon indicating copy to clipboard operation
zkevm-circuits copied to clipboard

Add unit tests for math gadgets

Open ed255 opened this issue 3 years ago • 9 comments

In src/evm_circuit/util/math_gadget.rs we have several math gadgets that are used in opcode gadgets. While the opcode gadgets have unit tests (which indirectly test the math gadgets), we don't have unit tests for the math gadgets. Adding these unit tests would be very useful because it will allow us to test the math gadgets individually with their own edge cases.

Doing these unit tests may require creating some testing functionality because these gadgets use the ConstraintBuilder which is associated to EVM steps; but for unit testing them there's no actual step.

The list of math gadgets that require unit testing:

  • [ ] IsZeroGadget<F>
  • [ ] IsEqualGadget<F>
  • [ ] BatchedIsZeroGadget<F, const N: usize>
  • [ ] AddWordsGadget<F, const N_ADDENDS: usize, const CHECK_OVREFLOW: bool>
  • [ ] MulWordByU64Gadget<F>
  • [ ] RangeCheckGadget<F, const N_BYTES: usize>
  • [ ] LtGadget<F, const N_BYTES: usize>
  • [ ] LtWordGadget<F>
  • [ ] ComparisonGadget<F, const N_BYTES: usize>
  • [ ] PairSelectGadget<F>
  • [ ] ConstantDivisionGadget<F, const N_BYTES: usize>
  • [ ] MinMaxGadget<F, const N_BYTES: usize>
  • [ ] MulAddWordsGadget<F>
  • [ ] ShrWordsGadget<F>
  • [ ] MulAddWords512Gadget<F>
  • [ ] ModGadget<F>

ed255 avatar Jun 21 '22 13:06 ed255

Thinking about this:

  • What about a special opcode TEST=A5, only activated when compiled in a special way, with the only propose to test gadgets? e.g. The first word in the stack is the function to test, and there's 16 words in the stack as input and 16 words as output. It the cell step becomes too much big, we can also use other opcodes, we have A5-EF that are empy.

  • Another way could be putting this as precompiles, maybe is cleaner.

adria0 avatar Jun 22 '22 10:06 adria0

We are working on it.

xiaodino avatar Oct 09 '22 05:10 xiaodino

Some of the math gadgets that were also re-defined in the gadgets crate already have unit tests: https://github.com/privacy-scaling-explorations/zkevm-circuits/tree/main/gadgets/src

So @xiaodino can refer them :)

roynalnaruto avatar Oct 11 '22 14:10 roynalnaruto

@smtmfft IMO we should also refactor the math_gadget module into modules per gadget in it, mainly to lower the lines-of-code in the entire module. What do others think about this? If yes, I'm happy to do this in a separate PR, or this can be done within #865 as well.

roynalnaruto avatar Oct 31 '22 06:10 roynalnaruto

Hi, @roynalnaruto You mean make each math gadget has its own file?? I think that's better structure, If the code does not change too much, I think this test would still works for them and I can add them to those files later on.

smtmfft avatar Nov 01 '22 03:11 smtmfft

Hi, @roynalnaruto Which branch are you working on? Maybe I could create PRs to that one if necessary.

smtmfft avatar Nov 01 '22 03:11 smtmfft

Hi, @roynalnaruto , I might misunderstood sth. Is there already a math gadget refactory task which is being done by someone else, or I can do that one by one just like https://github.com/privacy-scaling-explorations/zkevm-circuits/tree/main/gadgets/src??

smtmfft avatar Nov 10 '22 00:11 smtmfft

@smtmfft I am at the moment not working on the math gadget or its refactoring. This issue is only for adding tests for each gadget in there, but I mentioned that along with that, it would be good to also refactor those gadgets into their own modules, so that the lines-of-code in math_gadget.rs are reduced and we can manage the entire module better.

roynalnaruto avatar Nov 11 '22 14:11 roynalnaruto

Cool, I will create a task issue and self assign if no body else has concern :)

smtmfft avatar Nov 11 '22 14:11 smtmfft

Unit tests added as part of #920

davidnevadoc avatar Mar 28 '23 21:03 davidnevadoc