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

feat(utils): use multiple sources to download snark artifacts #273

Closed
wants to merge 10 commits into from

Conversation

sripwoud
Copy link
Member

@sripwoud sripwoud commented Apr 25, 2024

@sripwoud sripwoud added the tests 🧪 Adding missing or correcting existing tests label Apr 25, 2024
@sripwoud sripwoud self-assigned this Apr 25, 2024
@sripwoud sripwoud requested a review from cedoor April 25, 2024 16:35
@sripwoud sripwoud marked this pull request as draft April 25, 2024 17:58
@sripwoud sripwoud changed the title test(utils): increase timeout for get snark artifacts tests feat(utils): use 2 CDN providers for more reliability to download snark artifacts Apr 25, 2024
@sripwoud sripwoud changed the title feat(utils): use 2 CDN providers for more reliability to download snark artifacts feat(utils): use 2 CDN providers to download snark artifacts Apr 25, 2024
@sripwoud sripwoud changed the title feat(utils): use 2 CDN providers to download snark artifacts feat(utils): use multiple sources to download snark artifacts Apr 25, 2024

const getPackageVersions = async (proof: Proof) =>
fetch(`${ARTIFACTS_BASE_URL}/${proof}-artifacts`)
fetch(`https://registry.npmjs.org/@zk-kit/${proof}-artifacts`)
Copy link
Member Author

Choose a reason for hiding this comment

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

spotted a mistake there, unrelated to this PR though

@sripwoud sripwoud force-pushed the test/artifacts-timeout branch from 05babc6 to 71ba422 Compare April 29, 2024 09:44
@sripwoud
Copy link
Member Author

sripwoud commented Apr 29, 2024

@cedoor After further testing, I can confirm that unpkg is throttling downloads.
As I reimplemented the poseidon-proof to fetch all the artifacts concurrently, it would persistently fail with connection errors even before reaching the jest timeout.
I can also confirm that with the retry fetch mechanism we can avoid that issue.
I am happy to say that as a result (concurrent download + fetch retry to avoid connection errors), it can speed up the poseidon-proof tests up to around x4. (was requiring 180s of timeout, now 45s is enough):
https://github.com/privacy-scaling-explorations/zk-kit/actions/runs/8876339349/job/24367631912?pr=273#step:9:605

So deciding to combine multiple CDNs with a fetch retry will both make our tests faster and the artifacts fetching more reliable.

I would wait for #275 to merge first before continuing there, as both our PRs touch the same functions though.


const ARTIFACTS_BASE_URL = "https://unpkg.com/@zk-kit"
const getBaseUrls = (proof: Proof, version: Version) => [
Copy link
Member Author

Choose a reason for hiding this comment

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

my initial idea was to shuffle this randomly, but as privacy-scaling-explorations/snark-artifacts#23 told us how to rank them by download speed, we should follow that order

@sripwoud
Copy link
Member Author

Will reopen in here privacy-scaling-explorations/snark-artifacts#65

@sripwoud sripwoud closed this May 26, 2024
@sripwoud sripwoud deleted the test/artifacts-timeout branch July 23, 2024 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests 🧪 Adding missing or correcting existing tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(utils): use multiple sources to download snark artifacts
1 participant