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.

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

Switch to a more general supraseal_c2::generate_groth16_proofs interface. #315

Merged
merged 3 commits into from
Dec 11, 2023

Conversation

dot-asm
Copy link

@dot-asm dot-asm commented Sep 20, 2023

Verified to work in supranational/supra_seal#39

@vmx
Copy link

vmx commented Sep 20, 2023

The CI failure is unrelated. I'm working on fixing it.

@vmx vmx force-pushed the supraseal-facelift branch from c033439 to f278980 Compare October 4, 2023 15:50
@vmx
Copy link

vmx commented Oct 4, 2023

@dot-asm I dared to rebase your PR on current master and push it. It should now pass the CI.

Copy link

@vmx vmx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for taking so long for the review. The general change looks good to me (it's much nicer code than the old one). I've just two minor things, see the inline comments.

let (r_s, s_s) = randomization.unwrap_or((
vec![E::Fr::ZERO; num_circuits],
vec![E::Fr::ZERO; num_circuits],
));

// Make sure all circuits have the same input len.
let n = provers[0].a.len();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the n isn't used anywhere else, I'd just move the provers[0].a.len(); directly into the assert_eq below.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A fixup commit is pushed.

src/groth16/prover/supraseal.rs Show resolved Hide resolved
@dot-asm
Copy link
Author

dot-asm commented Oct 5, 2023

Sorry for taking so long for the review.

No need to apologize, I understand :-)

for i in 0..num_circuits {
input_assignments_ref.push(input_assignments_no_repr[i].as_ptr());
aux_assignments_ref.push(aux_assignments_no_repr[i].as_ptr());
impl<Scalar> Into<supraseal_c2::Assignment<Scalar>> for &ProvingAssignment<Scalar>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somehow one of my comments didn't make it to GitHub, so let's try it again:

In Rust you usually implement From instead of Into. Quoting the From documentation:

One should always prefer implementing From over Into because implementing From automatically provides one with an implementation of Into thanks to the blanket implementation in the standard library.

I would also move it to the top-level and not inside the function.

As I needed to try it you myself if it actually work, here's the code:

impl<Scalar> From<&ProvingAssignment<Scalar>> for supraseal_c2::Assignment<Scalar>
where
    Scalar: PrimeField,
{
    fn from(assignment: &ProvingAssignment<Scalar>) -> Self {
        assert_eq!(assignment.a.len(), assignment.b.len());
        assert_eq!(assignment.a.len(), assignment.c.len());

        Self {
            a_aux_density: assignment.a_aux_density.bv.as_raw_slice().as_ptr(),
            a_aux_bit_len: assignment.a_aux_density.bv.len(),
            a_aux_popcount: assignment.a_aux_density.get_total_density(),

            b_inp_density: assignment.b_input_density.bv.as_raw_slice().as_ptr(),
            b_inp_bit_len: assignment.b_input_density.bv.len(),
            b_inp_popcount: assignment.b_input_density.get_total_density(),

            b_aux_density: assignment.b_aux_density.bv.as_raw_slice().as_ptr(),
            b_aux_bit_len: assignment.b_aux_density.bv.len(),
            b_aux_popcount: assignment.b_aux_density.get_total_density(),

            a: assignment.a.as_ptr(),
            b: assignment.b.as_ptr(),
            c: assignment.c.as_ptr(),
            abc_size: assignment.a.len(),

            inp_assignment_data: assignment.input_assignment.as_ptr(),
            inp_assignment_size: assignment.input_assignment.len(),

            aux_assignment_data: assignment.aux_assignment.as_ptr(),
            aux_assignment_size: assignment.aux_assignment.len(),
        }
    }
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed up. (Please excuse my distrust for compilers, expressed as preference to give the compiler exactly as much as required, it's kind of an occupational hazard:-)

@DrPeterVanNostrand
Copy link

Hi @dot-asm, the From interface looks good to me (and I think the rest of the code too), but just to verify that I understand the PR... this change allows each witness assignment to carry unique denisity trackers, whereas previously, all witnesses were required to have their densities be constructed in an identical fashion?

@dot-asm
Copy link
Author

dot-asm commented Dec 8, 2023

this change allows each witness assignment to carry unique denisity trackers, whereas previously, all witnesses were required to have their densities be constructed in an identical fashion?

Well, the original goal was to streamline the interface and implementation, and the recognition that independent densities would be effectively free was kind of a collateral. Secondly, upon cutting the supraseal-c2 crate the original interface, the one this PR switches from, is just an adapter on top of the new interface, the one this PR switches to. So it's also kind of a cut-out-the-middle-man situation going on here.

@DrPeterVanNostrand
Copy link

@dot-asm Cool, thanks for the background info. The PR looks good to me

@vmx vmx force-pushed the supraseal-facelift branch from 55be1e9 to 925bd4a Compare December 11, 2023 16:18
@cryptonemo cryptonemo merged commit 30e30c2 into filecoin-project:master Dec 11, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants