-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Absorb Ligero Public Parameters #71
base: main
Are you sure you want to change the base?
Conversation
C: Config, | ||
H: CRHScheme, | ||
{ | ||
fn to_sponge_bytes(&self, dest: &mut Vec<u8>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to import ark_std::vec::Vec
under the no_std
feature flag. If you want to test the library with no_std
you can install cross
(cargo install cross
) and then:
cargo cross build --workspace --no-default-features --target aarch64-unknown-none --exclude ark-pcs-bench-templates
LGTM 👌 |
|
||
fn to_sponge_field_elements<F2: PrimeField>(&self, dest: &mut Vec<F2>) { | ||
self.sec_param.to_sponge_field_elements(dest); | ||
self.rho_inv.to_sponge_field_elements(dest); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I am a bit doubtful about:
- Things that are in the vk but not absorbed here.
- Things that are not in the vk but should be.
Regarding 1: - I think the well-formedness check boolean check_well_formedness
should definitely be absorbed. Regarding leaf_hash_param
, two_to_one_hash_param
and col_hash_params
, are they not absorbed here because of practical reasons or theoretical ones? At least on a theoretical level, I think these can be as relevant as all the other params. It's true that implementing this would be a bit awkward in that we might have to enforce Absorb
on these, but that shouldn't be that difficult, right? In the end I think these are pretty trivial (or empty) in the Ligeros we actually instantiate. But maybe they are really out of what we can touch.
What do you think, @mmagician ?
Regarding 2: I'm actually quite surprised that we don't have the RS-code parameters in the key: num rows, num_cols and num_ext_cols. It's true that we have rho = num_cols/num_ext_cols, but of course the quotient doesn't determine both of them separately. I've actually looked at the code and the verifier never learns these parameters until they have a commitment (they are in commitment.metadata). I think in most UV PCS definitions the public parameters should determine a degree bound on the polynomials (in UV Ligero the degree bound is simply num rows * num_cols - 1). Suppose we want to use this implementation of the PCS in a protocol where the polynomials must satisfy certain degree bounds. The way things are, there is no way in the API to enforce it, since it is check
that reads these parameters (equivalently, the degree bound) from commitment.metadata. We could either not use the API or use it and miss the degree bounds, which is of course an issue.
Long story short (for point 2): the verifier key is missing three important parameters and I think that has genuine security implications when using the API. @mmagician I can talk to Hossein to fix it everywhere. One way or another, once those three usizes
are in the verifier key, they should also be absorbed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding 1: the hashing parameters are definitely not empty. They determine the type of hash function used for:
- converting leaf -> leafDigest,
- hashing two leaves together
- hashing the columns together
These could in principle be all the same or all different. In our cases, the first is usually just identity, and the other two can have implications on the speed, especially if we're using SNARK-friendly hash functions.
Bottom line: these parameters are important even in our usage. I don't know whether they should be absorbed though, I think you'll have a much better idea about this than I do.
My (probably naive) thinking was this: they don't form part of the instance, but rather the practical instantiation of the protocol. But not that you say it, it probably makes sense to absorb these details, too (also, the field size being used?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding 2: the reason for not including these was that a single verifier key can (right now) be used for multiple polynomials. If you add the cols/rows, that limits the polynomials to all be of the same size.
Maybe in practice that's ok?
The alternative would be to place max-col / max-rows, since we're anyway doing checks on the max polynomial size that can be committed to IIRC. Will having max-rows / max-cols solve the F-S issue, or do you think we need the actual number of rows & cols?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we tap into the wisdom of @mmaker here, especially on point 1 from above^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear, the issue of not having the parameters n_rows
and n_cols
in the key is not (only) about FS. Let me try to explain it more clearly: Suppose we want to use Ligero in a protocol where the committed polynomials must satisfy a certain degree bound (e.g. Aurora). Our current Ligero implementation cannot be used! Because there is no way to tell the verifier's check
method what degree bound to enforce (again, degree bound = n_rows * n_cols - 1, that's why this is all related to those parameters). The verifier checks out the metadata in the commitment and doesn't compare it to a "previously expected" set of parameters.
So at least we have to allow the constructor to optionally receive n_rows
and n_cols
for these kinds of situations.
Going back to the FS question, I think we should absorb n_rows
and n_cols
(at which point absorbing rho
or n_extended_cols
is equivalent) at some point for robustness of the FS (maybe @mmaker can confirm). Since this has to be done in open
and check
and both have access to the vk/ck and the commitment, they can pull it from one place or the other depending on whether we decide n_rows
and n_cols
should always be in vk/ck or they can sometimes be only in the commitment.
Description
closes: #XXXX
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
Pending
section inCHANGELOG.md
Files changed
in the Github PR explorer