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

Replace explicit loop counters #76

Merged
merged 6 commits into from
Jul 29, 2024
Merged

Replace explicit loop counters #76

merged 6 commits into from
Jul 29, 2024

Conversation

rillian
Copy link
Contributor

@rillian rillian commented Jul 18, 2024

Address more clippy lints

  • Replace some explicit loop counters with zip and enumerate iterator wrappers, which are preferred style since they can optimize better.
  • Annotate some functions to silence "too many arguments" warnings.

rillian added 4 commits July 18, 2024 07:48
Clippy doesn't like index variables and issues a warning here,
recommending an equally-cumbersome functional equivalent.

Instead, combine the sequences mnd ap them to the evaluated
values directly. This may also optimize better than manual indexing
because it's easier for the compiler to eliminate initialization
and bounds checks.
Address these clippy lints by annotating the functions as expected.
Clippy is correct this is unfriendly API design, but for lower-level
cryptography it's better to match the mathematical notation of
the literature.
Use the `exp_iter` util instead of a manual accumulator for the
sequence of exponents of y in the r1cs prover. Addresses a
`clippy::needless_range_loop` lint, and should optimize better.

Fixup iterator patch
@rillian rillian requested a review from ankeleralph July 18, 2024 21:57
@rillian rillian self-assigned this Jul 18, 2024
@rillian rillian mentioned this pull request Jul 18, 2024
@@ -682,6 +683,9 @@ impl<'g, G: AffineRepr, T: BorrowMut<Transcript>> Prover<'g, G, T> {
.chain(s_L2.iter())
.zip(s_R1.iter().chain(s_R2.iter()));
for (i, (sl, sr)) in sLsR.enumerate() {
// y^i -> y^(i+1)
let exp_y = exp_y_iter.next()
.expect("exponentional iterator shouldn't terminate");

Choose a reason for hiding this comment

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

reported by reviewdog 🐶
[semgrep] expect or unwrap called in function returning a Result

Source: https://semgrep.dev/r/trailofbits.rs.panic-in-function-returning-result.panic-in-function-returning-result


Cc @thypon @bcaller

Copy link
Contributor Author

@rillian rillian Jul 19, 2024

Choose a reason for hiding this comment

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

The current FrExp impl is non-terminating, so the guard here is only to catch incompatible code changes. I think that's better as a panic than a error result, but don't feel strongly about it.

bulletproofs/src/r1cs/prover.rs Show resolved Hide resolved
@claucece
Copy link
Member

yes!!

Copy link
Collaborator

@ankeleralph ankeleralph left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@@ -682,6 +683,9 @@ impl<'g, G: AffineRepr, T: BorrowMut<Transcript>> Prover<'g, G, T> {
.chain(s_L2.iter())
.zip(s_R1.iter().chain(s_R2.iter()));
for (i, (sl, sr)) in sLsR.enumerate() {
// y^i -> y^(i+1)
let exp_y = exp_y_iter.next()
.expect("exponentional iterator shouldn't terminate");

Choose a reason for hiding this comment

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

reported by reviewdog 🐶
[semgrep] expect or unwrap called in function returning a Result

Source: https://semgrep.dev/r/trailofbits.rs.panic-in-function-returning-result.panic-in-function-returning-result


Cc @thypon @bcaller

}
r_vec.resize_with(padded_n, || {
let exp_y = exp_y_iter.next()
.expect("exponentional iterator shouldn't terminate");

Choose a reason for hiding this comment

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

reported by reviewdog 🐶
[semgrep] expect or unwrap called in function returning a Result

Source: https://semgrep.dev/r/trailofbits.rs.panic-in-function-returning-result.panic-in-function-returning-result


Cc @thypon @bcaller

// y^i -> y^(i+1)
let exp_y = exp_y_iter
.next()
.expect("exponentional iterator shouldn't terminate");

Choose a reason for hiding this comment

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

reported by reviewdog 🐶
[semgrep] expect or unwrap called in function returning a Result

Source: https://semgrep.dev/r/trailofbits.rs.panic-in-function-returning-result.panic-in-function-returning-result


Cc @thypon @bcaller

r_vec.resize_with(padded_n, || {
let exp_y = exp_y_iter
.next()
.expect("exponentional iterator shouldn't terminate");

Choose a reason for hiding this comment

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

reported by reviewdog 🐶
[semgrep] expect or unwrap called in function returning a Result

Source: https://semgrep.dev/r/trailofbits.rs.panic-in-function-returning-result.panic-in-function-returning-result


Cc @thypon @bcaller

@rillian rillian merged commit 88c7f94 into main Jul 29, 2024
3 of 4 checks passed
@rillian rillian deleted the clippy branch July 29, 2024 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants