Ns/rename pp crs #1760
Ns/rename pp crs #1760
Conversation
nsarlin-zama
added
the
data_PR
nsarlin-zama
force-pushed
the
ns/rename_pp_crs
branch
2 times, most recently
from
5926757 to
808cc7c
Compare
nsarlin-zama
force-pushed
the
ns/rename_pp_crs
branch
from
808cc7c to
e0c703e
Compare
nsarlin-zama
force-pushed
the
ns/rename_pp_crs
branch
from
e0c703e to
e93770b
Compare
| impl Named for CompactPkeCrs { | ||
| const NAME: &'static str = "zk::CompactPkeCrs"; | ||
| } |
There was a problem hiding this comment.
isn't this an issue for versioning if the struct is transparent ?
There was a problem hiding this comment.
No, the name is included outside of the versioned data so it has no influence on the versioning. If one day we want to modify the struct to not be transparent we will have to do a CompactPkeCrsVersions enum that starts at v1:
enum CompactPkeCrsVersions {
V0(CompactPkePublicParams),
V1(CompactPkeCrs)
}There was a problem hiding this comment.
ok my bad I meant if we want to safe deserialize a CompactPkeCrs from the params, it won't work because of the named impl right ?
There was a problem hiding this comment.
yes, the idea is to deserialize it as a CompactPkePublicParams and then convert it to a CompactPkeCrs using the From impl
There was a problem hiding this comment.
could we somehow force people to always deserialize the CRS via some from/into tricks ?
There was a problem hiding this comment.
The problem is that since they have been serialized with NAME = "CompactPkePublicParams" it means that the output of safe_deserialize needs to be something with NAME= "CompactPkePublicParams".
We could have CompactPkeCrs::NAME = "CompactPkePublicParams", but since it's just a matter of calling .into for the user, I don't think it's worth having this discrepancy.
There was a problem hiding this comment.
no yeah I agree, was just thinking out loud
There was a problem hiding this comment.
or maybe a different way to write safe serialize/deserialize with a serialized type S that has Into where T is the actual type we want to use later, but that would maybe defeat the purpose of the safe serialize
| // "wasm bindgen is fragile and prefers the actual type vs. Self" | ||
| #[allow(clippy::use_self)] | ||
| #[wasm_bindgen] | ||
| impl CompactPkePublicParams { |
There was a problem hiding this comment.
do we want to keep this on the js side ?
There was a problem hiding this comment.
I think we at least need to keep the deserialize code for backward compatibility ?
There was a problem hiding this comment.
ah is it only deserialization ?
There was a problem hiding this comment.
could it be put on th Crs and a "deserialize_from_parameters" kind of thing ?
There was a problem hiding this comment.
and on the struct itself because I guess we need it a "deserialize_into_crs"
There was a problem hiding this comment.
you want to keep both ? Maybe it's better to keep it simple and have only the Crs::deserialize_from_parameters ?
There was a problem hiding this comment.
sure, I'm just trying to think of users manipulating the old struct and not finding any function on it
|
or maybe make it borrow from the CRS ? edit: this achieves nothing except avoiding copies |
|
maybe I'm wrong here, I don't have a precise enough view of the CRS and params |
BREAKING_CHANGE: - All the zk API (build_with_proof, verify, verify_and_expand,...) now take a `CompactPkeCrs` instead of a `CompactPkePublicParams`. Serialized `CompactPkePublicParams` from previous versions can be converted into a `CompactPkeCrs` using `params.into()`
nsarlin-zama
force-pushed
the
ns/rename_pp_crs
branch
from
e93770b to
0fac299
Compare
closes: https://github.com/zama-ai/tfhe-rs-internal/issues/800#issue-2603108396
PR content/description
Use the CompactPkeCrs instead of the CompactPkePublicParams. It simplifies a bit the workflow for users, they generate a crs and are directly able to use it and serialize it without having to extract the params.
Having a release 0.11 in Cargo.toml was necessary to be able to update the data repo.
Check-list: