Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Absorb Ligero Public Parameters #71

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Absorb Ligero Public Parameters #71

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions poly-commit/src/linear_codes/ligero.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
use ark_crypto_primitives::{
crh::{CRHScheme, TwoToOneCRHScheme},
merkle_tree::{Config, LeafParam, TwoToOneParam},
sponge::Absorb,
};
use ark_ff::PrimeField;
use ark_std::{log2, marker::PhantomData};
Expand Down Expand Up @@ -96,6 +97,23 @@
}
}

impl<F, C, H> Absorb for LigeroPCParams<F, C, H>
where
F: PrimeField,
C: Config,
H: CRHScheme,
{
fn to_sponge_bytes(&self, dest: &mut Vec<u8>) {

Check failure on line 106 in poly-commit/src/linear_codes/ligero.rs

View workflow job for this annotation

GitHub Actions / Check no_std 

cannot find type `Vec` in this scope
Copy link

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

self.sec_param.to_sponge_bytes(dest);
self.rho_inv.to_sponge_bytes(dest);
}

fn to_sponge_field_elements<F2: PrimeField>(&self, dest: &mut Vec<F2>) {

Check failure on line 111 in poly-commit/src/linear_codes/ligero.rs

View workflow job for this annotation

GitHub Actions / Check no_std 

cannot find type `Vec` in this scope
self.sec_param.to_sponge_field_elements(dest);
self.rho_inv.to_sponge_field_elements(dest);
Copy link

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:

  1. Things that are in the vk but not absorbed here.
  2. 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.

Copy link
Author

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?)

Copy link
Author

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?

Copy link
Author

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^

Copy link

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.

}
}

impl<F, C, H> LinCodeParametersInfo<C, H> for LigeroPCParams<F, C, H>
where
F: PrimeField,
Expand Down
Loading