test: Integrate Fuzzing into Jellyfish #784
Conversation
|
I'm highly supportive on this effort. I'd say add fuzzing in CI as well (maybe as a weekly cron job as it runs for a very long time). |
alxiong
changed the title
|
okay, i fixed the rayon iterator issue in CI. but wasm32 target build will fail. I feel like including wdyt? |
That's a good idea. We don't mean to export it. Also don't forget to update the nix env |
|
Yeah I think adding as a crate was a "oh my god I will do anything to make this work" type of decision. Let me swap that out and get CI, then @0xkato and I will keep adding tests |
|
CI fixed. @0xkato to take over adding tests, I will circle back |
| if let Ok(input) = ExtensionAttackInput::arbitrary(&mut u) { | ||
| let forged_val = F::from(input.forged_val); | ||
| let attack_pos = input.attack_pos; | ||
| let forged_pos = attack_pos * 3 + 2; |
There was a problem hiding this comment.
instead of using hardcoded value, let's either add explanatory code doc on top of this whole function or name const arity = 3
especially explain the digest_leaf computation on H(0 || pos || val) where 0 is leaf_node_domain_separator... to help readers understand why +2
| let squeezed = RescueCRHF::<F>::sponge_no_padding(&data, 1).unwrap(); | ||
| let val = squeezed[0]; | ||
|
|
||
| let elems = vec![val; attack_pos as usize + 1]; |
There was a problem hiding this comment.
should we:
- construct this leaves vector with
Vec::with_capacity(next_multiple_of(attack_pos, arity)) - only mutate the
attack_posto be theval - add some sanity check on the constructed
mt'sheight,num_leavesetc.
| if let Some(random_lookup) = (0..input.elems.len()) | ||
| .into_iter() | ||
| .collect::<Vec<_>>() | ||
| .choose(&mut thread_rng()) | ||
| .cloned() |
There was a problem hiding this comment.
is this an overly-complicated way of doing rng.gen_range(0..len)?
| .cloned() | ||
| { | ||
| let commitment = mt.commitment(); | ||
| if let jf_merkle_tree::LookupResult::Ok(elem, proof) = mt.lookup(random_lookup as u64) { |
There was a problem hiding this comment.
Q1: i can't think of a reason why the result here won't be LookupResult::Ok, we didn't prune it or explicitly forget it.
Q2: putting it behind if let indicate that we want to skip the failure cases? should we panic in those case instead. I think we should ? to panic.
|
|
||
| #[derive(Arbitrary, Debug)] | ||
| struct MerkleTreeArbitraryInput { | ||
| // height: usize, |
| if let Some(random_lookup) = (0..input_elems_len) | ||
| .into_iter() | ||
| .collect::<Vec<_>>() | ||
| .choose(&mut rand::rng()) | ||
| .cloned() |
There was a problem hiding this comment.
same here, why not rand::rng().random_rang()?
| return; | ||
| } | ||
|
|
||
| let elems: Vec<Fr254> = input.elems.iter().map(|x| Fr254::from(*x)).collect(); |
There was a problem hiding this comment.
importing ark_std::UniformRand, then you can invoke Fr254::rand(&mut rng) which will produce a uniform field element (256bits) rather than converting from a single limb u64.
we can use arbitrary input's as seed to a SeedableRng which is further used to generate a random field element if Arbtrary doesn't work well with Fr::rand() api.
There was a problem hiding this comment.
so what are we testing here? just universal tree construction? or maybe this is just WIP?
Howdy gents.
Tobias and I have had some initial luck with getting libFuzzer integrated and want to spend some time looking at this further (i.e. a more comprehensive test harness, etc). We wanted to solicit your feedback as this would be a lot of work, and if there's no hope of it ever merging we might consider some other approaches.