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

perf(commitments): generate sapling point outside the method #4799

Merged
merged 2 commits into from
Jul 21, 2022
Merged

Conversation

oxarbitrage
Copy link
Contributor

Motivation

Fix #4783

Solution

Generate the sapling points once as a global for sapling (f79c438).

Orchard is already doing it buy the lazy_static is inside the method. Maybe this is ok but i changed it anyway for consistency (4cbe1fc).

Review

Anyone can review.

Reviewer Checklist

  • Code implements Specs and Designs
  • Tests for Expected Behaviour
  • Tests for Errors

@oxarbitrage oxarbitrage requested a review from a team as a code owner July 20, 2022 16:13
@oxarbitrage oxarbitrage requested review from upbqdn and removed request for a team July 20, 2022 16:13
@teor2345 teor2345 added C-bug Category: This is a bug I-slow Problems with performance or responsiveness I-integration-fail Continuous integration fails, including build and test failures A-cryptography Area: Cryptography related P-High 🔥 labels Jul 21, 2022
Copy link
Member

@upbqdn upbqdn left a comment

Choose a reason for hiding this comment

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

All looks good.

A suggestion regarding maintenance:

It is often desirable to keep variables close to where they are used. As described here https://users.rust-lang.org/t/lazy-static-inside-a-functions-scope/31700/2, statics are not reinitialized on subsequent function calls, so having V and R inside the function helps to keep the scope of the module a bit cleaner.

@teor2345 teor2345 merged commit 1b17c57 into main Jul 21, 2022
@teor2345 teor2345 deleted the issue4783 branch July 21, 2022 23:17
@teor2345
Copy link
Contributor

I admin-merged this PR because it is an urgent sync fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cryptography Area: Cryptography related C-bug Category: This is a bug I-integration-fail Continuous integration fails, including build and test failures I-slow Problems with performance or responsiveness
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Avoid calling find_group_hash with every new ValueCommitment
3 participants