zkevm-circuits
zkevm-circuits copied to clipboard
privacy-scaling-explorations
Unify testing-randomness usage across the entire workspace
LGTM for now to merge and prevent PR outdating. I'd really consider to use testing setup/teardown to reduce the verbosity and redundancy in the tests. Also, I think it would be benefitial to setup a final solution that unifies ALL the randomness used across all the tests so that we don't do different things on each PR/test. See: #641 Originally posted by @CPerezz in https://github.com/privacy-scaling-explorations/zkevm-circuits/pull/654#pullrequestreview-1073967710
As seen in this PR too, there are a lot of different ways on which we're using the randomness in our tests. And As mentioned in #641, it would be nice to have a Rand value for testing that is easy to see (No seeded RNG's) and that is constant across tests.
Therefore I think we could simply set a "random" passed by env-var and simply use it on each test we need to perform.
We can simply have a [cgf(test)] lazy_static block that fetches the env var when required.
Closing #641 in favor of this PR as it is a superset.
I'd say keep it simple before we get the challenge API.
@ChihChengLiang note that this has nothing to do with the RLC or the circuits. This is just a universal way for us to set random values for our tests (constructing a random Tx or Block for example.)
I believe we have 2 cases of randomness in tests:
- The randomness value for RLC. I think we could have a fixed constant for this used across all tests. This could be
0x1000 - Deterministic but random-looking values to generate inputs for tests. For this I think the easiest way is a seedable rng with a fixed seed.
The idea of getting the fixed random constant from an env var sounds good to me, as long as there's a sensible default value when the env var is not defined.
Thanks for the feedback @ed255
Regarding 1. Not going to make a PR as this will be superset by the new halo2 version removing the randomness requirements. As per 2. I like too the idea of using a seedable rng with a fixed seed passed via env or set as default. NOTE that this will make it a bit more difficult to see randomness-related values (the seed will derive in a consistent but likely large and very random number.
We have now the CHallenge-API. Are we still considering this an issue? Or should I close it?? cc: @ed255 @ChihChengLiang
We have now the CHallenge-API. Are we still considering this an issue? Or should I close it?? cc: @ed255 @ChihChengLiang
It makes sense to me to close this issue considering that with the Challenge-API we don't have the original issue with mock random values everywhere!
I agreee @ed255 but we're still using randomness in a lot of tests. I'd mark this as a good_first issue and leave it without a milestone for now. WDYT?
I agreee @ed255 but we're still using randomness in a lot of tests. I'd mark this as a
good_first issueand leave it without a milestone for now. WDYT?
Right, we still have rng used to fill test inputs non-deterministically. It makes sense to leave this as a good first isse :)