Skip to content
This repository has been archived by the owner on Sep 14, 2023. It is now read-only.

feat: pre-compute test users #753

Merged
merged 13 commits into from
Mar 17, 2023
Merged

feat: pre-compute test users #753

merged 13 commits into from
Mar 17, 2023

Conversation

kratico
Copy link
Contributor

@kratico kratico commented Mar 15, 2023

Resolves #741

The test_users_public_keys file was generated with

import { testUser } from "./crypto/mod.ts"
import * as $ from "./deps/scale.ts"

const DEFAULT_TEST_USER_COUNT = 100_000

const publicKeys = []
for (let i = 0; i < DEFAULT_TEST_USER_COUNT; i++) {
  publicKeys.push(testUser(i).publicKey)
}

await Deno.writeFile(
  `./providers/frame/test_users_public_keys`,
  $.array($.sizedUint8Array(32)).encode(publicKeys),
)

Note that the star.yml workflow duration is not impacted by this change

image

Comment on lines 20 to 22
await Deno.readTextFile(
new URL(`./test_users/network_${networkPrefix}_user_balances.json`, import.meta.url),
),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Each network_<prefix>_user_balances.json weights ~7mb.
I'm not sure what would be best

  • keep them in Git
  • upload them the something like S3 and cache them in a CDN

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I didn't think about how large the files would be... Can you test to see how much time each part of the generation takes? (text encoding / hashing / sr25519 from seed / ss58 generation) If the text encoding and/or hashing are expensive, we could adopt a different approach for the seed generation, but if the majority of the time is the sr25519 or ss58, we might be out of luck.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that sr25519 from seed is taking the majority of time.
I did a quick implementation that uses sr25519 from secret and the result is similar.

image

image

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, ok. In that case, instead of pre-computing the full addresses, could you just pre-compute the public keys, and then save them into a file using $.array($.sizedUint8Array(32))? That way it can be the same file across chains, and it should be only around 3.2 MB.

Copy link
Contributor

Choose a reason for hiding this comment

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

And if the file is only around 3.2 megs, you can put that in git for now, like we do with the dprint formatter. Later, once I take a look at #709, that file can potentially be moved from git into an s3 cache.

Copy link
Contributor Author

@kratico kratico Mar 16, 2023

Choose a reason for hiding this comment

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

Ah, ok. In that case, instead of pre-computing the full addresses, could you just pre-compute the public keys, and then save them into a file using $.array($.sizedUint8Array(32))? That way it can be the same file across chains, and it should be only around 3.2 MB.

Great suggestion!
Now, there is a single 3.1MB file

@tjjfvi
Copy link
Contributor

tjjfvi commented Mar 15, 2023

And yeah, star is taking a while to cache... I think maybe the Cache deno dependencies step isn't doing as much as it should. I suspect given that the polkadot js dependencies in examples/polkadot_js_signer.ts aren't in deps, that they weren't included in the initial cache cache.

@tjjfvi
Copy link
Contributor

tjjfvi commented Mar 15, 2023

I just evicted the deno cache for main, we'll see if that fixes it

@tjjfvi
Copy link
Contributor

tjjfvi commented Mar 15, 2023

Ah, no, I think it's taking a long time again because @harrysolovay changed the specifiers back to @latest, so now star has to spin up chains again.

@harrysolovay
Copy link
Contributor

Ah, no, I think it's taking a long time again because @harrysolovay changed the specifiers back to @latest, so now star has to spin up chains again.

Correct: I didn't realize the pinning was intentional / for caching purposes. Apologies. Feel free to revert.

@kratico kratico force-pushed the feat/pre-compute-test-users branch from ed4a12f to 4830a63 Compare March 16, 2023 14:25
@kratico kratico marked this pull request as ready for review March 16, 2023 14:28
@kratico kratico requested a review from harrysolovay as a code owner March 16, 2023 14:28
Copy link
Contributor

@tjjfvi tjjfvi 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. Can you put the code you used to generate that file into _tasks/download.ts? (at which point it can be renamed to something like generate_artifacts.ts)

cspell.json Outdated Show resolved Hide resolved
@kratico kratico force-pushed the feat/pre-compute-test-users branch from 94c1b0b to 7711a8d Compare March 16, 2023 15:17
@kratico kratico requested a review from tjjfvi March 16, 2023 15:37
_tasks/downloads.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@tjjfvi tjjfvi left a comment

Choose a reason for hiding this comment

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

Perfect, just one nit

providers/frame/common.ts Outdated Show resolved Hide resolved
Co-authored-by: T6 <t6@t6.fyi>
tjjfvi
tjjfvi previously approved these changes Mar 16, 2023
@kratico kratico requested a review from tjjfvi March 16, 2023 17:42
providers/frame/common.ts Outdated Show resolved Hide resolved
kratico and others added 2 commits March 16, 2023 14:45
@kratico kratico added this pull request to the merge queue Mar 17, 2023
Merged via the queue into main with commit b0b6589 Mar 17, 2023
@kratico kratico deleted the feat/pre-compute-test-users branch March 17, 2023 00:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pre-compute test users
3 participants