chore: add deserialization tests with corrupted inputs#3391
chore: add deserialization tests with corrupted inputs#3391nsarlin-zama merged 2 commits intomainfrom
Conversation
435a678 to
76777d0
Compare
IceTDrinker
left a comment
There was a problem hiding this comment.
Looks reasonable, I let the others take a look
thanks !
@IceTDrinker reviewed 21 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on nsarlin-zama, soonum, and SouchonTheo).
tests/corrupted_inputs_deserialization.rs line 117 at r1 (raw file):
} fn handle_ct_list(input: &[u8], conformance_params: &CompactCiphertextListConformanceParams) {
could be worth printing to see up to where a list progresses given most of them likely won't make it passed deserialization
same for the proven stuff below
tests/corrupted_inputs_deserialization.rs line 161 at r1 (raw file):
#[test] #[cfg(feature = "integer")]
I think we can get rid of that feature, to group once #3380 is merged to get rid of ZK and shortint features too I think
a472097 to
a19bae9
Compare
nsarlin-zama
left a comment
There was a problem hiding this comment.
- I added the strings feature to tfhe because one of the samples has a string
- also added a black_box to explicitly states that the ops should not be optimized away
@nsarlin-zama made 3 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on IceTDrinker, soonum, and SouchonTheo).
tests/corrupted_inputs_deserialization.rs line 117 at r1 (raw file):
Previously, IceTDrinker wrote…
could be worth printing to see up to where a list progresses given most of them likely won't make it passed deserialization
same for the proven stuff below
yes good idea
tests/corrupted_inputs_deserialization.rs line 161 at r1 (raw file):
Previously, IceTDrinker wrote…
I think we can get rid of that feature, to group once #3380 is merged to get rid of ZK and shortint features too I think
I removed it here so that it's less work to do it later
a19bae9 to
52ae14e
Compare
52ae14e to
a8f488d
Compare
e2e253a to
73f7792
Compare
IceTDrinker
left a comment
There was a problem hiding this comment.
Looks good to me, should we have someone else review the changes ?
Otherwise I can approve
@IceTDrinker partially reviewed 5 files and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on IceTDrinker, mayeul-zama, soonum, SouchonTheo, and tmontaigu).
tmontaigu
left a comment
There was a problem hiding this comment.
@tmontaigu partially reviewed 21 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on mayeul-zama, nsarlin-zama, soonum, and SouchonTheo).
tests/corrupted_inputs_deserialization.rs line 60 at r4 (raw file):
fn process_inputs(dir: &Path, mut handler: impl FnMut(&[u8])) -> u64 { let mut total_tests = 0; let entries: Vec<_> = std::fs::read_dir(dir)
minor, but I dont think we actually need to collect ?
tests/corrupted_inputs_deserialization.rs line 71 at r4 (raw file):
total_tests += 1; let mut input = Vec::new(); File::open(&path)
std::fs:read exists as a convenient wrapper of this
https://doc.rust-lang.org/std/fs/fn.read.html
tests/corrupted_inputs_deserialization.rs line 82 at r4 (raw file):
} macro_rules! test_type {
Minor since this is a test, but it can be done without macro
fn test_integer<FheType>(exp: &CompactCiphertextListExpander, i: usize)
where
FheType: HlExpandable
+ Tagged
+ Clone
+ Add<FheType, Output = FheType>
+ Sub<FheType, Output = FheType>
+ Mul<FheType, Output = FheType>,
{
let ct = match exp.get::<FheType>(i) {
Ok(Some(ct)) => ct,
Ok(None) => {
println!("No ct found at idx {}\n", i);
return;
}
Err(e) => {
println!("Error caught while trying to get ct at idx {}:\n{e}\n", i);
return;
}
};
let res = ct.clone() + ct.clone();
let res = res * ct.clone();
let _ = std::hint::black_box(res - ct);
}
73f7792 to
2d17498
Compare
nsarlin-zama
left a comment
There was a problem hiding this comment.
@nsarlin-zama made 3 comments and resolved 1 discussion.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on mayeul-zama, soonum, SouchonTheo, and tmontaigu).
tests/corrupted_inputs_deserialization.rs line 60 at r4 (raw file):
Previously, tmontaigu (tmontaigu) wrote…
minor, but I dont think we actually need to collect ?
done
tests/corrupted_inputs_deserialization.rs line 71 at r4 (raw file):
Previously, tmontaigu (tmontaigu) wrote…
std::fs:read exists as a convenient wrapper of this
https://doc.rust-lang.org/std/fs/fn.read.html
done
tests/corrupted_inputs_deserialization.rs line 82 at r4 (raw file):
Previously, tmontaigu (tmontaigu) wrote…
Minor since this is a test, but it can be done without macro
fn test_integer<FheType>(exp: &CompactCiphertextListExpander, i: usize) where FheType: HlExpandable + Tagged + Clone + Add<FheType, Output = FheType> + Sub<FheType, Output = FheType> + Mul<FheType, Output = FheType>, { let ct = match exp.get::<FheType>(i) { Ok(Some(ct)) => ct, Ok(None) => { println!("No ct found at idx {}\n", i); return; } Err(e) => { println!("Error caught while trying to get ct at idx {}:\n{e}\n", i); return; } }; let res = ct.clone() + ct.clone(); let res = res * ct.clone(); let _ = std::hint::black_box(res - ct); }
I'm always happy when I can avoid a macro :)
done
2d17498 to
d988f4c
Compare
tmontaigu
left a comment
There was a problem hiding this comment.
@tmontaigu reviewed 1 file and all commit messages, and resolved 2 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on mayeul-zama, soonum, and SouchonTheo).
d988f4c to
08749a5
Compare
e4f7eda to
e17b210
Compare
IceTDrinker
left a comment
There was a problem hiding this comment.
@IceTDrinker reviewed 5 files and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on mayeul-zama, soonum, and SouchonTheo).

closes: please link all relevant issues
PR content/description
This PR adds a test that runs known corrupted ct lists to check they are correctly deserialized (meaning errors are caught but does not trigger a panic).
The lfs part (pull, github cache, ...) has been copied from the backward data tests.
This change is