feat: Cycle Group Fuzzer #12154
feat: Cycle Group Fuzzer #12154
Conversation
| Element(ScalarField s = ScalarField::one(), GroupElement g = GroupElement::one()) | ||
| : scalar(s) | ||
| , value(g){}; | ||
| // Element(GroupElement& el) |
| uint256_t temp; | ||
| // Generate a random mask_size-bit value | ||
| // We want to sample from log distribution instead of uniform | ||
| uint16_t* p = (uint16_t*)&temp; |
There was a problem hiding this comment.
FastRandom generates 32-bit values, you can generate faster
There was a problem hiding this comment.
Well, not really. It generates 31-bit values + a little bit more. I can change the parameters to p1, a, b=(4294967291, 38978, 1337) so that it would generate 32 bit values better though.
It's not a strong prime, unlike the current one. But I don't see how a strong one could benefit the generator
There was a problem hiding this comment.
I'm just too scared to look at these 13%...
In [1]: p = 3758096939
In [2]: p / 2**32
Out[2]: 0.8750001292210072| // Instruction arguments | ||
| ArgumentContents arguments; | ||
|
|
||
| template <typename T> static inline uint256_t fast_uint256_log(T& rng) |
There was a problem hiding this comment.
The name doesn't really explain well what's happening here. Either comments or a rename would help
| e.scalar = ScalarField::one().sqrt().second.invert(); | ||
| break; | ||
| case 5: | ||
| e.scalar = ScalarField::get_root_of_unity(13); |
There was a problem hiding this comment.
Just a small prime factor of the multiplicative group order. Could be 9 or 6.
| // this->scalar = 0; | ||
| // } | ||
| ScalarField scalar; | ||
| GroupElement value; |
There was a problem hiding this comment.
Why are you using group elements?
There was a problem hiding this comment.
All the AffineElement's operations first cast to GroupElement and then are cast back. Just cut out the middle man
There was a problem hiding this comment.
That's ok, but I don't think changing the projective coordinates affects the cycle group representation at all, so you have a mutation that only affect the native implementation. Since we're fuzzing the circuit one, it's probably wasted effort
There was a problem hiding this comment.
Yeah, thought of that too
| PUT_RANDOM_BYTE_IF_LUCKY(instruction.arguments.mulArgs.in); | ||
| PUT_RANDOM_BYTE_IF_LUCKY(instruction.arguments.mulArgs.out); | ||
| if (rng.next() & 1) { | ||
| instruction.arguments.mulArgs.scalar = ScalarField(Instruction::fast_uint256_log(rng)); |
There was a problem hiding this comment.
Why not change it in other ways, too? You are just substituting with a random value, but you could alternatively mutate it in the same way we usually mutate fields
| size_t ind = | ||
| rng.next() % static_cast<size_t>(instruction.arguments.batchMulArgs.add_elements_count); | ||
| instruction.arguments.batchMulArgs.scalars[ind] = | ||
| ScalarField(Instruction::fast_uint256_log(rng)); |
|
|
||
| cycle_group_t cg() const | ||
| { | ||
| const bool reconstruct = static_cast<bool>(VarianceRNG.next() % 2); |
There was a problem hiding this comment.
Your randomseed weight is zero, so rng might not change unless the call order is different
There was a problem hiding this comment.
What do you mean by that? Why rng might not change?
There was a problem hiding this comment.
Ah, right, I mixed up mutation weights and construction weights. No, it should work, sorry
| */ | ||
| class InstructionWeights { | ||
| public: | ||
| static constexpr size_t SET = 0; |
There was a problem hiding this comment.
Why do these instructions have a zero weight?
There was a problem hiding this comment.
Changed SET_INF weight to 2. The other two ops cost nothing, so I thought we can use them as much as possible. Like in bigfield fuzzer
This pr introduces a fuzzer for
stdlib::primitives::cycle_group