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

chore: Split SumcheckRound into Prover/Verifier classes #1373

Merged
merged 4 commits into from
Aug 7, 2023

Conversation

ledwards2225
Copy link
Contributor

Description

Split SumcheckRound into SumcheckRoundProver and SumcheckRoundVerifier in support of the recursive verifier work. We do not want to inadvertently instantiate "prover functionality" with stdlib types when instantiating UltraVerifier_<flavor::UltraRecursive>. Sumcheck itself was split in a previous PR but SumcheckRound was overlooked.

Checklist:

  • I have reviewed my diff in github, line by line.
  • Every change is related to the PR description.
  • I have linked this pull request to the issue(s) that it resolves.
  • There are no unexpected formatting changes, superfluous debug logs, or commented-out code.
  • The branch has been merged or rebased against the head of its merge target.
  • I'm happy for the PR to be merged at the reviewer's next convenience.

@ledwards2225 ledwards2225 changed the title Split SumcheckRound into Prover/Verifier classes chore: Split SumcheckRound into Prover/Verifier classes Aug 2, 2023
@ludamad ludamad added the crypto cryptography label Aug 2, 2023
template <typename... T>
static constexpr void add_tuples(std::tuple<T...>& tuple_1, const std::tuple<T...>& tuple_2)
{
[&]<std::size_t... I>(std::index_sequence<I...>) { ((std::get<I>(tuple_1) += std::get<I>(tuple_2)), ...); }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick: This took me a bit to realize this is a lambda being called - maybe we could assign the lambda?

auto add_tuples_helper = ....
add_tuples_helper(std::make_index_sequence<sizeof...(T)>{})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was indeed gross and I've removed the lambda altogether and simplified it to a single fold expression

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Scratch that. Thought I was clever but most of the compilers didn't like it. Went with your original suggestion

Copy link
Collaborator

@ludamad ludamad left a comment

Choose a reason for hiding this comment

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

LGTM think I can review this one as it's a refactor (I'll rely on others for new crypto)

@ludamad ludamad added crypto cryptography and removed crypto cryptography labels Aug 2, 2023
@ledwards2225 ledwards2225 force-pushed the lde/split_sumcheck_round branch 2 times, most recently from e318a42 to 5a0d94a Compare August 2, 2023 15:47
@ledwards2225 ledwards2225 self-assigned this Aug 2, 2023
@ledwards2225 ledwards2225 force-pushed the lde/split_sumcheck_round branch 4 times, most recently from fbcec75 to da5027c Compare August 7, 2023 12:27
@ledwards2225 ledwards2225 force-pushed the lde/split_sumcheck_round branch from da5027c to a742f9d Compare August 7, 2023 13:22
@ledwards2225 ledwards2225 merged commit 8b1d48a into master Aug 7, 2023
@ledwards2225 ledwards2225 deleted the lde/split_sumcheck_round branch August 7, 2023 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto cryptography
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants