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: Lazy initialize the KZG struct when running tests #6311

Merged
merged 4 commits into from
Aug 28, 2024

Conversation

kevaundray
Copy link
Contributor

Issue Addressed

get_kzg creates a new KZG struct each time its called, which can slow down tests since each test case will call get_kzg.

Proposed Changes

This PR uses LazyLock to instantiate it once using LazyLock and then other tests will just take a thread-safe reference to it.

Additional Info

There is another get_kzg that has been defined in kzg_utils -- That only calls get_kzg once, so its not an issue. Longer term, it might make sense to put a get_kzg method with LazyLock in crypto/get_kzg.rs, so that tests all over the workspace can access it. The main issue I see with this, is that the trusted_setup file is not in crypto/kzg.rs, so the solution won't be as clean as what it looks like in the tests modules.

@kevaundray kevaundray changed the base branch from stable to unstable August 27, 2024 13:11
@kevaundray kevaundray force-pushed the kw/lazy-initialize-kzg branch from d2c63bb to c8aec80 Compare August 27, 2024 13:20
@kevaundray kevaundray force-pushed the kw/lazy-initialize-kzg branch from bd89823 to e2d00f9 Compare August 27, 2024 13:23
kevaundray added a commit to kevaundray/lighthouse that referenced this pull request Aug 27, 2024
kevaundray added a commit to kevaundray/lighthouse that referenced this pull request Aug 27, 2024
@jimmygchen jimmygchen added test improvement Improve tests ready-for-review The code is ready for review labels Aug 27, 2024
Copy link
Member

@jimmygchen jimmygchen 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! 🎉

@jimmygchen jimmygchen added ready-for-merge This PR is ready to merge. das Data Availability Sampling and removed ready-for-review The code is ready for review labels Aug 28, 2024
@jimmygchen
Copy link
Member

@mergify queue

Copy link

mergify bot commented Aug 28, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 5a96687

mergify bot added a commit that referenced this pull request Aug 28, 2024
@mergify mergify bot merged commit 5a96687 into sigp:unstable Aug 28, 2024
28 checks passed
AgeManning pushed a commit to AgeManning/lighthouse that referenced this pull request Sep 3, 2024
* use LazyLock when initializing KZG struct

* update get_kzg to return Arc<KZG> and not a Result<Arc<KZG>>

* Revert orthogonal changes to `kzg_verify_cell_kzg_proof_batch`

* add back map_err
chong-he pushed a commit to chong-he/lighthouse that referenced this pull request Nov 26, 2024
* use LazyLock when initializing KZG struct

* update get_kzg to return Arc<KZG> and not a Result<Arc<KZG>>

* Revert orthogonal changes to `kzg_verify_cell_kzg_proof_batch`

* add back map_err
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
das Data Availability Sampling ready-for-merge This PR is ready to merge. test improvement Improve tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants