refactor: simple code cleanups in mst implementation #269

teddav · 2024-02-26T17:08:22Z

Just cleaned a few things while reviewing the MST. All tests are passing, but some changes (ex: removing some condition) might require to add more tests. Let me know if you think that's the case

alxkzmn · 2024-03-18T04:27:06Z

@ntampakas is it possible to let the workflows from forks to run and use the AWS credentials? Or is there a better way to deal with external contributions?

zk_prover/src/merkle_sum_tree/entry.rs

sifnoc · 2024-03-04T11:55:43Z

zk_prover/src/merkle_sum_tree/utils/csv_parser.rs

@@ -50,7 +50,8 @@ pub fn parse_csv_to_entries<P: AsRef<Path>, const N_CURRENCIES: usize, const N_B
            balances_big_int.push(balance);
        }

-        let entry = Entry::new(username, balances_big_int.try_into().unwrap())?;
+        let entry = Entry::new(username, balances_big_int.try_into().unwrap());


I've reviewed the potential for errors during the try_into operation. It appears that converting BigUint in this line does not produce any errors.
However, for the record, this should be checked again if the balance type in the Entry struct changes.

ntampakas · 2024-03-19T07:25:38Z

@ntampakas is it possible to let the workflows from forks to run and use the AWS credentials? Or is there a better way to deal with external contributions?

Hey Alex!
Probably you are referring to benchmark workflow that uses a self-hosted runner. Is this correct?
AFAIN, a forked repo, cannot execute workflows on the upstream side. What is the exact use case?

alxkzmn · 2024-03-19T09:40:41Z

@ntampakas I'm referring to the Rust workflow for tests that runs on push, it is also using the self-hosted runner because of the past performance issues with the github runner.

ntampakas · 2024-03-19T10:31:38Z

@ntampakas I'm referring to the Rust workflow for tests that runs on push, it is also using the self-hosted runner because of the past performance issues with the github runner.

Correct! That also initiates a self hosted runner.
Unfortunately, there is not an easy solution here; anyone who forks should create their own self hosted runner(s) using short-lived credentials (recommended) or AWS access keys.

teddav · 2024-03-20T21:03:32Z

I just talked with @sifnoc who explained to me why you moved to a self-hosted runner. Seems the change was made in PR #199 and time went from 15' to about 4'. I checked the runs before the PR and they were indeed around 15min but unfortunately I can't see the details because Github doesn't keep that information for that long, so I don't know what part was slow. But looking at more recent ones, it seems like running is actual test is the slow part, while installing the different components is fast. So my suggestion: why not just split the tests in different jobs (which will be run in parallel). Currently they are all run into 1 big job called "build". And I see 4 tasks being run:

Test Zk Prover
Test Zk Prover examples
Test backend
Test backend example
Let's split them and see. If it's unequal we can figure out how to split them differently.

teddav · 2024-03-20T21:17:20Z

i just pushed the change. i created a new workflow. I guess we can keep both running in parallel for now just to check the difference, and then decide which one to keep. We could also cache dependencies to make things even faster.
I dont know how caching works for cargo build/cargo test (like caching the target directory), but I found this https://github.com/Swatinem/rust-cache that could be interesting

alxkzmn · 2024-03-21T03:06:38Z

How about switching between self-hosted runner and GitHub runner based on where the changes are coming from? E.g., if it's a fork, run in GitHub, but if it's in Summa org, use the current setup? Here's the way to detect the repo name: https://github.com/orgs/community/discussions/25217#discussioncomment-3246901

sifnoc

Thank you for the initiative to optimize the github action flow.

We had a specific reason for running tests sequentially. It's because the interface file for the verifier contract in the Backend, specifically backend/src/contracts/generated/inclusion_verifier.rs, is generated from the ABI file, which in turn is generated from the contract. This process is similar to what is done in the script update_verifier_contract.sh in the Backend.

However, now that the circuits and contract have matured for V1, your suggestion to test in a parallelized manner seems very appropriate.

Also, following Alexs` found, we can add condition to the workflow file that run test on self-host runner.

.github/workflows/tests.yml

sifnoc

Thank you for fixing this. Now, the test finishes in about 6 minutes without using the est instance.

alxkzmn

Let's figure out which testing workflow we're using before we merge this PR. If the parallel tests work well, should they be merged into the old workflow?

zk_prover/src/merkle_sum_tree/utils/build_tree.rs

.github/workflows/tests.yml

alxkzmn · 2024-04-29T07:43:24Z

@teddav Do I get it right that now the tests would either run on both AWS and GitHub when pushed from Summa org or only on GitHub when pushed from a fork? In case of Summa org I was expecting the tests to only run in AWS, not on GitHub.

teddav · 2024-04-29T09:43:14Z

@alxkzmn yes you're right. It's useless to run on both. Even though now the AWS worker seems a bit overkill even for Summa's team. It seems like everything could run fine just on Github

sifnoc · 2024-07-11T12:09:40Z

This PR has been thoroughly discussed and is ready for merging. I appreciate the effort put into this PR, @teddav .

Based on the discussions, it seems that the next step for this PR would be to remove the requirement of an AWS instance in the workflow.

alxkzmn requested review from sifnoc and alxkzmn March 18, 2024 03:56

sifnoc approved these changes Mar 18, 2024

View reviewed changes

sifnoc mentioned this pull request Mar 18, 2024

Removed MST approach on Polynomial Interpolation approach #277

Merged

sifnoc requested changes Mar 21, 2024

View reviewed changes

.github/workflows/tests.yml Outdated Show resolved Hide resolved

.github/workflows/tests.yml Outdated Show resolved Hide resolved

.github/workflows/tests.yml Outdated Show resolved Hide resolved

.github/workflows/tests.yml Outdated Show resolved Hide resolved

teddav force-pushed the simple-code-cleanups-mst-implementation branch from cfc4c16 to 598db04 Compare April 8, 2024 15:12

sifnoc approved these changes Apr 9, 2024

View reviewed changes

alxkzmn requested changes Apr 16, 2024

View reviewed changes

zk_prover/src/merkle_sum_tree/utils/build_tree.rs Outdated Show resolved Hide resolved

.github/workflows/tests.yml Outdated Show resolved Hide resolved

teddav added 4 commits April 24, 2024 18:23

refactor: simple code cleanups in mst implementation

d3b4f5d

split ci tests workflow in smaller jobs

e791089

fix tests

a826485

update parallel tests + run tests on aws only for summa team

ead3c67

teddav force-pushed the simple-code-cleanups-mst-implementation branch from 598db04 to 6f05b33 Compare April 24, 2024 16:38

merge github workflows

4799f26

teddav force-pushed the simple-code-cleanups-mst-implementation branch from 6f05b33 to 4799f26 Compare April 24, 2024 16:39

alxkzmn self-requested a review April 29, 2024 06:57

sifnoc merged commit 23587ad into summa-dev:master Jul 11, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: simple code cleanups in mst implementation #269

refactor: simple code cleanups in mst implementation #269

teddav commented Feb 26, 2024

alxkzmn commented Mar 18, 2024

sifnoc Mar 4, 2024

ntampakas commented Mar 19, 2024

alxkzmn commented Mar 19, 2024

ntampakas commented Mar 19, 2024

teddav commented Mar 20, 2024

teddav commented Mar 20, 2024 •

edited

Loading

alxkzmn commented Mar 21, 2024

sifnoc left a comment

sifnoc left a comment

alxkzmn left a comment

alxkzmn commented Apr 29, 2024

teddav commented Apr 29, 2024

sifnoc commented Jul 11, 2024

refactor: simple code cleanups in mst implementation #269

refactor: simple code cleanups in mst implementation #269

Conversation

teddav commented Feb 26, 2024

alxkzmn commented Mar 18, 2024

sifnoc Mar 4, 2024

Choose a reason for hiding this comment

ntampakas commented Mar 19, 2024

alxkzmn commented Mar 19, 2024

ntampakas commented Mar 19, 2024

teddav commented Mar 20, 2024

teddav commented Mar 20, 2024 • edited Loading

alxkzmn commented Mar 21, 2024

sifnoc left a comment

Choose a reason for hiding this comment

sifnoc left a comment

Choose a reason for hiding this comment

alxkzmn left a comment

Choose a reason for hiding this comment

alxkzmn commented Apr 29, 2024

teddav commented Apr 29, 2024

sifnoc commented Jul 11, 2024

teddav commented Mar 20, 2024 •

edited

Loading