Contain queries in FRI.

[alonh5]

This change is 

[alonh5]

Current dependencies on/for this PR:

• Create ColumnDomainsVisitor. #376
• Create MaskEvaluator. #375
• Move max_constraint_log_degree_bound to AirExt trait. #373
• Move CP evaluation to AirExt trait. #372
• Type def ColumnVec. #371
• max_constraint_log_degree_bound in Air trait. #370
• Create FibonnaciAir. #369
• Contain queries in FRI. #355 👈
• Get opening positions from FRI. #354
• Get degree bounds from component. #351
• dev

This stack of pull requests is managed by Graphite.

[alonh5]

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @andrewmilson, @shaharsamocha7, and @spapinistarkware)

---

src/core/fri.rs line 330 at r1 (raw file):

            last_layer_domain,
            last_layer_poly,
            queries: None,

Not sure about this but I couldn't find another way to contain the queries inside the FRI protocol. Would be happy to hear what you think.

[andrewmilson]

Reviewable status: 0 of 2 files reviewed, 4 unresolved discussions (waiting on @alonh5, @shaharsamocha7, and @spapinistarkware)

---

src/core/fri.rs line 330 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Not sure about this but I couldn't find another way to contain the queries inside the FRI protocol. Would be happy to hear what you think.

LGTM but do you mind documenting it?

---

src/core/fri.rs line 348 at r2 (raw file):

        decommitted_values: Vec<SparseCircleEvaluation<SecureField>>,
    ) -> Result<(), VerificationError> {
        let queries = self.queries.clone().unwrap();

Suggestion

Code snippet:

let queries = self.queries.as_ref().unwrap();

---

src/core/fri.rs line 361 at r2 (raw file):

        SecureField: ExtensionOf<F>,
    {
        assert_eq!(queries.log_domain_size, self.expected_query_log_domain_size);

Can we add an assertion

Code snippet:

assert_eq!(queries.len(), self.config.n_queries);

---

src/core/fri.rs line 439 at r2 (raw file):

    }

    pub fn opening_positions(

Can you add documentation please? and add a test for this and D

Also, what do you think about renaming it column_opening_positions?

---

src/core/fri.rs line 450 at r2 (raw file):

        let queries = Queries::generate(channel, column_log_sizes[0], self.config.n_queries);
        self.queries = Some(queries.clone());
        get_opening_positions(&queries, &column_log_sizes)

Suggestion:

Code snippet:

let res = get_opening_positions(&queries, &column_log_sizes);
self.queries = Some(queries);
res

[alonh5]

Reviewable status: 0 of 2 files reviewed, 4 unresolved discussions (waiting on @andrewmilson, @shaharsamocha7, and @spapinistarkware)

---

src/core/fri.rs line 330 at r1 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

LGTM but do you mind documenting it?

Done.

---

src/core/fri.rs line 348 at r2 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

Suggestion

Done. I had make some methods get self by reference, does that look fine to you?

---

src/core/fri.rs line 361 at r2 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

Can we add an assertion

Same as in the previous PR (this assertion is problematic because the queries are deduped which could cause the actual query number to be less than the config).

---

src/core/fri.rs line 439 at r2 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

Can you add documentation please? and add a test for this and D

Also, what do you think about renaming it column_opening_positions?

Done.

---

src/core/fri.rs line 450 at r2 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

Suggestion:

Done.

[andrewmilson]

Reviewable status: 0 of 3 files reviewed, 6 unresolved discussions (waiting on @alonh5, @shaharsamocha7, and @spapinistarkware)

---

src/core/fri.rs line 348 at r2 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Done. I had make some methods get self by reference, does that look fine to you?

To keep it owned can do let queries = self.queries.take().expect("queries not sampled")

---

src/core/fri.rs line 361 at r2 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Same as in the previous PR (this assertion is problematic because the queries are deduped which could cause the actual query number to be less than the config).

Ahh yeah forgot about that. Thanks

---

src/core/fri.rs line 256 at r3 (raw file):

    last_layer_poly: LinePoly<SecureField>,
    /// The queries used for decommitment. Initialized when calling
    /// `self.column_opening_positions`.

Can do [`FriVerifier::column_opening_positions`] to make it link

---

src/core/fri.rs line 1228 at r3 (raw file):

    #[test]
    fn test_opening_positions() {

Personally I think this test should be removed since it's looking a bit like a change detector test https://testing.googleblog.com/2015/01/testing-on-toilet-change-detector-tests.html

I think it's better to just have the valid_mixed_degree_end_to_end_proof_passes_verification test and add an assert that verifier.column_opening_positions equals what prover.decommit returns.

[alonh5]

Reviewable status: 0 of 3 files reviewed, 6 unresolved discussions (waiting on @andrewmilson, @shaharsamocha7, and @spapinistarkware)

---

src/core/fri.rs line 348 at r2 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

To keep it owned can do let queries = self.queries.take().expect("queries not sampled")

Done.

---

src/core/fri.rs line 361 at r2 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

Ahh yeah forgot about that. Thanks

Done.

---

src/core/fri.rs line 256 at r3 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

Can do [`FriVerifier::column_opening_positions`] to make it link

Cool, thanks.

---

src/core/fri.rs line 1228 at r3 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

Personally I think this test should be removed since it's looking a bit like a change detector test https://testing.googleblog.com/2015/01/testing-on-toilet-change-detector-tests.html

I think it's better to just have the valid_mixed_degree_end_to_end_proof_passes_verification test and add an assert that verifier.column_opening_positions equals what prover.decommit returns.

Good idea. Thanks.

[andrewmilson]

Reviewable status: 0 of 3 files reviewed, 7 unresolved discussions (waiting on @alonh5, @shaharsamocha7, and @spapinistarkware)

---

src/core/fri.rs line 439 at r2 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Done.

Might be worth mentioning that the order of the SparseSubCircleDomains matches the order of column commitment.

---

src/core/fri.rs line 1067 at r5 (raw file):

        let bounds = LOG_DEGREES.map(CirclePolyDegreeBound::new).to_vec();
        let mut verifier = FriVerifier::commit(&mut test_channel(), config, proof, bounds).unwrap();
        let verifier_opening_positions = verifier.column_opening_positions(&mut test_channel());

Can we keep it arrange act assert.

Code snippet:

fn column_opening_positions_for_mixed_degree_are_valid() -> Result<(), VerificationError> {
    ...
    let mut verifier = FriVerifier::commit(&mut test_channel(), config, proof, bounds).unwrap();
    
    let verifier_opening_positions = verifier.decommit(decommitment_values);
    
    assert_eq!(prover_opening_positions, verifier_opening_positions);
    verifier.decommit(decommitment_values)
}

[alonh5]

Reviewable status: 0 of 3 files reviewed, 7 unresolved discussions (waiting on @andrewmilson, @shaharsamocha7, and @spapinistarkware)

---

src/core/fri.rs line 439 at r2 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

Might be worth mentioning that the order of the SparseSubCircleDomains matches the order of column commitment.

Done.

---

src/core/fri.rs line 1067 at r5 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

Can we keep it arrange act assert.

Done. Regarding the test name proposal, I think it's mainly testing the protocol end to end and secondarily testing the column opening positions, so I kept the name as is and included the verifier commitment in the "act" stage.

[andrewmilson]

Reviewed 1 of 3 files at r3, 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @alonh5, @shaharsamocha7, and @spapinistarkware)

---

src/core/fri.rs line 1067 at r5 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Done. Regarding the test name proposal, I think it's mainly testing the protocol end to end and secondarily testing the column opening positions, so I kept the name as is and included the verifier commitment in the "act" stage.

SGTM

[alonh5]

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

---

src/core/fri.rs line 1067 at r5 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

SGTM

Done.

[andrewmilson]

Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion (waiting on @shaharsamocha7 and @spapinistarkware)

[andrewmilson]

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

The base branch was changed.

[andrewmilson]

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

---

src/core/fri.rs line 457 at r8 (raw file):

    /// Samples queries and returns the opening positions for each unique column size.
    /// The order of the opening positions corresponds to the order of the column commitment.

nit: https://doc.rust-lang.org/rustdoc/how-to-write-documentation.html#documenting-components

Code snippet:

/// Samples queries and returns the opening positions for each unique column size.
///
/// The order of the opening positions corresponds to the order of the column commitment.

---

src/core/fri.rs line 476 at r8 (raw file):

/// Returns the column opening positions needed for verification.
/// The domain log sizes must be unique and in descending order.

nit: https://doc.rust-lang.org/rustdoc/how-to-write-documentation.html#documenting-components

Code snippet:

/// Returns the column opening positions needed for verification.
///
/// The domain log sizes must be unique and in descending order.

[andrewmilson]

Reviewed 1 of 2 files at r7, 1 of 1 files at r10, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @shaharsamocha7 and @spapinistarkware)