Organize errors.

[alonh5]

This change is 

[alonh5]

• Organize errors. #523 👈
• dev

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @alonh5 and the rest of your teammates on Graphite

[shaharsamocha7]

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @alonh5 and @spapinistarkware)

---

src/core/commitment_scheme/mod.rs line 377 at r1 (raw file):

                let values = queried_values.take(1 << subdomain.log_size).collect_vec();
                if values.len() != 1 << subdomain.log_size {
                    return Err(VerificationError::InvalidStructure(

Do we want to return also the values/values len?

Code quote:

VerificationError::InvalidStructure(

---

src/core/prover/mod.rs line 146 at r1 (raw file):

    let (trace_oods_values, composition_oods_value) =
        proved_values_to_mask(air, proof.commitment_scheme_proof.proved_values.clone())
            .map_err(|_| VerificationError::InvalidStructure("Proved values".to_string()))?;

I think that this returned error string isn't really indicative. Maybe we should be able to infer in which phase of the algorithm it failed?

Code quote:

"Proved values".to_string()

---

src/core/prover/mod.rs line 157 at r1 (raw file):

}

fn proved_values_to_mask(

Document

Code quote:

fn proved_values_to_mask(

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 73.68421% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 95.09%. Comparing base (8a75935) to head (5ed50a5).
Report is 6 commits behind head on dev.

Files	Patch %	Lines
src/core/commitment_scheme/verifier.rs	60.00%	3 Missing and 1 partial ⚠️
src/core/prover/mod.rs	81.25%	0 Missing and 3 partials ⚠️
src/fibonacci/mod.rs	75.00%	0 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #523      +/-   ##
==========================================
- Coverage   95.17%   95.09%   -0.08%     
==========================================
  Files          63       63              
  Lines        9227     9241      +14     
  Branches     9227     9241      +14     
==========================================
+ Hits         8782     8788       +6     
- Misses        387      389       +2     
- Partials       58       64       +6     

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

[alonh5]

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

---

src/core/prover/mod.rs line 146 at r1 (raw file):

Previously, shaharsamocha7 wrote…

I think that this returned error string isn't really indicative. Maybe we should be able to infer in which phase of the algorithm it failed?

It's not exactly failing in a certain phase, the structure of the passed proved values (oods values) isn't as expected. I modified the error message a bit, I think being more specific than this might be an overkill. WDYT?

---

src/core/prover/mod.rs line 157 at r1 (raw file):

Previously, shaharsamocha7 wrote…

Document

Done.

---

src/core/commitment_scheme/mod.rs line 377 at r1 (raw file):

Previously, shaharsamocha7 wrote…

Do we want to return also the values/values len?

values here is is only the values of a specific subdomain in the iteration. I can calculate the actual total difference in queried values just for this error. Thinking of the possible reasons this error could occur I'm not sure if this extra information will actually help the user.

[shaharsamocha7]

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

---

src/core/prover/mod.rs line 146 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

It's not exactly failing in a certain phase, the structure of the passed proved values (oods values) isn't as expected. I modified the error message a bit, I think being more specific than this might be an overkill. WDYT?

maybe proved_values or (sampled)

---

src/core/commitment_scheme/mod.rs line 377 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

values here is is only the values of a specific subdomain in the iteration. I can calculate the actual total difference in queried values just for this error. Thinking of the possible reasons this error could occur I'm not sure if this extra information will actually help the user.

E.g. if the values vector is empty that would give an extra information.
Maybe only print the len()?

[alonh5]

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

---

src/core/prover/mod.rs line 146 at r1 (raw file):

Previously, shaharsamocha7 wrote…

maybe proved_values or (sampled)

Done.

---

src/core/commitment_scheme/mod.rs line 377 at r1 (raw file):

Previously, shaharsamocha7 wrote…

E.g. if the values vector is empty that would give an extra information.
Maybe only print the len()?

WDYT?

[shaharsamocha7]

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

---

src/core/commitment_scheme/verifier.rs line 143 at r4 (raw file):

    value: SecureField,
) -> Result<SparseCircleEvaluation<SecureField>, VerificationError> {
    let queried_values_len = queried_values.len();

I would calculate that only if the error has happened.

Code quote:

let queried_values_len = queried_values.len();

---

src/core/commitment_scheme/verifier.rs line 152 at r4 (raw file):

                if values.len() != 1 << subdomain.log_size {
                    return Err(VerificationError::InvalidStructure(format!(
                        "Insufficient number of queried values ({queried_values_len})"

not sure that this value help to understand the error message by itself.
Either also print expected number or values, or remove at all (your call)

Code quote:

{queried_values_len}

[alonh5]

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

---

src/core/commitment_scheme/verifier.rs line 152 at r4 (raw file):

Previously, shaharsamocha7 wrote…

not sure that this value help to understand the error message by itself.
Either also print expected number or values, or remove at all (your call)

To calculate the expected number I would need to iterate over the domains again, calculate and sum the sized of each one. I think this is an overkill for values that won't give much information anyways, I'll just remove it from the error.

[shaharsamocha7]

Reviewed 1 of 1 files at r5.
Reviewable status: all files reviewed (commit messages unreviewed), all discussions resolved (waiting on @spapinistarkware)

[shaharsamocha7]

Reviewed 4 of 4 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @spapinistarkware)