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

Use SecureField in interaction. #642

Merged
merged 1 commit into from
Jun 27, 2024
Merged

Conversation

alonh5
Copy link
Contributor

@alonh5 alonh5 commented May 26, 2024

This change is Reviewable

@codecov-commenter
Copy link

codecov-commenter commented May 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.50%. Comparing base (1f38b04) to head (34a1233).

Additional details and impacted files
@@           Coverage Diff           @@
##              dev     #642   +/-   ##
=======================================
  Coverage   89.50%   89.50%           
=======================================
  Files          75       75           
  Lines        9640     9640           
  Branches     9640     9640           
=======================================
  Hits         8628     8628           
  Misses        932      932           
  Partials       80       80           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@alonh5 alonh5 force-pushed the 05-26-use_securefield_in_interaction branch from b1c52c7 to aecb1e4 Compare May 26, 2024 13:56
@alonh5 alonh5 force-pushed the 05-21-modify_mask_points_for_interaction branch from 66b6bf8 to f873404 Compare May 26, 2024 14:19
@alonh5 alonh5 force-pushed the 05-26-use_securefield_in_interaction branch from aecb1e4 to ec7f140 Compare May 26, 2024 14:19
@alonh5 alonh5 force-pushed the 05-21-modify_mask_points_for_interaction branch from f873404 to 33fc002 Compare May 26, 2024 14:48
@alonh5 alonh5 force-pushed the 05-26-use_securefield_in_interaction branch from ec7f140 to 942e4d3 Compare May 26, 2024 14:48
@alonh5 alonh5 force-pushed the 05-21-modify_mask_points_for_interaction branch from 33fc002 to 7c247f4 Compare May 27, 2024 07:35
@alonh5 alonh5 force-pushed the 05-26-use_securefield_in_interaction branch from 942e4d3 to fe5d057 Compare May 27, 2024 07:35
@alonh5 alonh5 force-pushed the 05-21-modify_mask_points_for_interaction branch from 7c247f4 to cba4dae Compare May 27, 2024 07:46
@alonh5 alonh5 force-pushed the 05-26-use_securefield_in_interaction branch 3 times, most recently from 90b274e to 7141a2d Compare May 27, 2024 11:35
@alonh5 alonh5 force-pushed the 05-21-modify_mask_points_for_interaction branch from cba4dae to 43176a4 Compare May 27, 2024 11:39
@alonh5 alonh5 force-pushed the 05-26-use_securefield_in_interaction branch from 7141a2d to f54597b Compare May 27, 2024 11:39
@alonh5 alonh5 marked this pull request as ready for review May 27, 2024 12:33
@alonh5 alonh5 force-pushed the 05-21-modify_mask_points_for_interaction branch from 43176a4 to 120236e Compare May 28, 2024 12:14
@alonh5 alonh5 force-pushed the 05-26-use_securefield_in_interaction branch from f54597b to f478872 Compare May 28, 2024 12:14
@alonh5 alonh5 force-pushed the 05-21-modify_mask_points_for_interaction branch from 120236e to ae9d116 Compare May 28, 2024 12:51
@alonh5 alonh5 force-pushed the 05-26-use_securefield_in_interaction branch from f478872 to 2745ab2 Compare May 28, 2024 12:51
Copy link
Collaborator

@spapinistarkware spapinistarkware left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 11 files at r1, 1 of 11 files at r2, all commit messages.
Reviewable status: 2 of 12 files reviewed, 3 unresolved discussions (waiting on @alonh5 and @shaharsamocha7)


crates/prover/src/core/utils.rs line 94 at r3 (raw file):

/// Securely combines the given values using the given random alpha and z.
/// Alpha and z should be secure field elements for soundness.
pub fn shifted_secure_combination<F: ExtensionOf<BaseField>>(

Is this needed?


crates/prover/src/core/air/mod.rs line 82 at r3 (raw file):

        trace: &ColumnVec<&CircleEvaluation<B, BaseField, BitReversedOrder>>,
        elements: &InteractionElements,
    ) -> ColumnVec<SecureEvaluation<B>>;

I don't think it is always a bunch of secure columns in general.
It should be exactly like trace, just a bunch of basefield columns.

Code quote:

ColumnVec<SecureEvaluation<B>>

crates/prover/src/core/prover/mod.rs line 74 at r3 (raw file):

    let interaction_elements = air.interaction_elements(channel);
    let interaction_trace_polys = air.interact(&trace, &interaction_elements);
  1. This is not using the precomputed twiddles.
  2. Do we need to change the interface? Can't interact() reutrn teh evaluation?

Copy link
Contributor Author

@alonh5 alonh5 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 of 12 files reviewed, 3 unresolved discussions (waiting on @shaharsamocha7 and @spapinistarkware)


crates/prover/src/core/utils.rs line 94 at r3 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

Is this needed?

Yes the values could be M31 (in the prover constraint evaluation) and QM31 (in the verifier oods constraint evaluation).


crates/prover/src/core/air/mod.rs line 82 at r3 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

I don't think it is always a bunch of secure columns in general.
It should be exactly like trace, just a bunch of basefield columns.

Done.


crates/prover/src/core/prover/mod.rs line 74 at r3 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…
  1. This is not using the precomputed twiddles.
  2. Do we need to change the interface? Can't interact() reutrn teh evaluation?

Done.

@alonh5 alonh5 force-pushed the 05-26-use_securefield_in_interaction branch from b20b7cc to b4b6328 Compare June 23, 2024 09:18
@alonh5 alonh5 force-pushed the 05-21-modify_mask_points_for_interaction branch from cc96916 to 909c178 Compare June 23, 2024 09:22
@alonh5 alonh5 force-pushed the 05-26-use_securefield_in_interaction branch 2 times, most recently from ccd2fb8 to 88b9ad2 Compare June 23, 2024 10:20
@alonh5 alonh5 force-pushed the 05-21-modify_mask_points_for_interaction branch from 909c178 to 7443767 Compare June 23, 2024 10:57
@alonh5 alonh5 force-pushed the 05-26-use_securefield_in_interaction branch 2 times, most recently from 0dcbd87 to de6e3a6 Compare June 23, 2024 13:02
@alonh5 alonh5 force-pushed the 05-21-modify_mask_points_for_interaction branch from 7443767 to 60c196d Compare June 23, 2024 13:19
@alonh5 alonh5 force-pushed the 05-26-use_securefield_in_interaction branch 2 times, most recently from 88f8a6d to c6aaf57 Compare June 23, 2024 13:34
@alonh5 alonh5 force-pushed the 05-26-use_securefield_in_interaction branch from c6aaf57 to 6a16e22 Compare June 24, 2024 10:02
@alonh5 alonh5 changed the base branch from 05-21-modify_mask_points_for_interaction to dev June 24, 2024 10:02
Copy link
Collaborator

@spapinistarkware spapinistarkware left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 11 files at r2, 9 of 9 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @alonh5 and @shaharsamocha7)


crates/prover/src/examples/wide_fibonacci/component.rs line 135 at r4 (raw file):

        let trace_values = trace.iter().map(|eval| &eval.values[..]).collect_vec();
        let (alpha, z) = (elements[ALPHA_ID], elements[Z_ID]);
        let values = write_lookup_column(&trace_values, alpha, z);

For efficientcy, we probably want write_lookup_column() to already return a SecureColumn.
You can add a TODO for that.


crates/prover/src/examples/wide_fibonacci/component.rs line 139 at r4 (raw file):

        for (i, value) in values.into_iter().enumerate() {
            secure_column.set(i, value);
        }

I think we have a FromIterator impl for SecureColumn. so, you can use collect()

Code quote:

        for (i, value) in values.into_iter().enumerate() {
            secure_column.set(i, value);
        }

crates/prover/src/examples/wide_fibonacci/constraint_eval.rs line 95 at r4 (raw file):

            // Lookup constraints.
            let lookup_value =
                SecureCirclePoly::<CpuBackend>::eval_from_partial_evals(std::array::from_fn(|j| {

Both of us know what this function does, but I fear other people will be confused by it. Maybe we can consult with the team how to name this.

Code quote:

eval_from_partial_evals

@alonh5 alonh5 force-pushed the 05-26-use_securefield_in_interaction branch from 6a16e22 to 96ac8ba Compare June 26, 2024 10:23
Copy link
Contributor Author

@alonh5 alonh5 left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @shaharsamocha7 and @spapinistarkware)


crates/prover/src/examples/wide_fibonacci/component.rs line 135 at r4 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

For efficientcy, we probably want write_lookup_column() to already return a SecureColumn.
You can add a TODO for that.

Done.


crates/prover/src/examples/wide_fibonacci/component.rs line 139 at r4 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

I think we have a FromIterator impl for SecureColumn. so, you can use collect()

Done, thanks.


crates/prover/src/examples/wide_fibonacci/constraint_eval.rs line 95 at r4 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

Both of us know what this function does, but I fear other people will be confused by it. Maybe we can consult with the team how to name this.

Yes, I also think we should move it to be a method of secure field and then it will be more clear when it's next to from_m31_array(). I added a TODO also.

Copy link
Contributor Author

@alonh5 alonh5 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 11 of 13 files reviewed, 3 unresolved discussions (waiting on @shaharsamocha7 and @spapinistarkware)


crates/prover/src/examples/wide_fibonacci/constraint_eval.rs line 95 at r4 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Yes, I also think we should move it to be a method of secure field and then it will be more clear when it's next to from_m31_array(). I added a TODO also.

PR 679

@alonh5 alonh5 force-pushed the 05-26-use_securefield_in_interaction branch from 96ac8ba to 34a1233 Compare June 27, 2024 10:56
Copy link
Collaborator

@spapinistarkware spapinistarkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @shaharsamocha7)

Copy link
Contributor Author

alonh5 commented Jun 27, 2024

Merge activity

  • Jun 27, 8:38 AM EDT: @alonh5 started a stack merge that includes this pull request via Graphite.
  • Jun 27, 8:38 AM EDT: @alonh5 merged this pull request with Graphite.

@alonh5 alonh5 merged commit 3150e16 into dev Jun 27, 2024
14 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.

None yet

3 participants