Reorganisation of `main` branch #42

storojs72 · 2023-12-11T17:18:55Z

The idea of this PR is following: keep in main branch our cryptographic building blocks (for every feature) with their unit-tests and keep rest of all developments, related to e2e verification in appropriate branches (pasta, grumpkin, zeromorph, gas-optimizing, etc.) together with specific related stuff (e2e contract, data loader, proof/public parameters JSONs, integration testing with cloud Anvil node), without attempting to generalise/stabilise the whole codebase in one place.

Solidity has limited support (no generics, for example) of tools that allow creating unified codebase for scattered functionalities, so attempting to combine all our developments within single codebase seems to be wasting of efforts. Instead it is better to maintain every "feature" in its own branch, until it would be possible to run the whole e2e verification without OutOfGas exception. When it happens, the next step would be deriving the initial version of e2e contract (by combining all features together - which should happen one time), turning it into Yul / assmebly one and start countdown of "stable" solution versions. And the e2e contract can appear in main branch. Until that, since main is usually considered as a source of "stable" codebase - we can keep fundamental cryptographic building blocks in main - since they are more stable - at reference side, in Arecibo, usually new blocks appear (grumpkin arithmetics, zeromorph, etc.) instead of significant changing the existing ones.

This approach will allow better modularity and isolation of unstable e2e verification contracts and steps libraries as well as avoiding spending time on temporary combining codebase in the "middle" of its evolution.

storojs72 · 2023-12-11T18:09:25Z

!test

tchataigner

Thanks! I have only a discussion to have on the ci workflow. Otherwise the reorg seems to be as you previously specified :)

tchataigner · 2023-12-12T10:27:07Z

.github/workflows/end2end.yml

+      #- name: Install Foundry
+      #  uses: foundry-rs/foundry-toolchain@v1
+      #  with:
+      #    version: nightly

-      - name: Set up Python
-        uses: actions/setup-python@v4
-        with:
-          python-version: 3.8
+      #- name: Set up Python
+      #  uses: actions/setup-python@v4
+      #  with:
+      #    python-version: 3.8

-      - name: Deploy main contract
-        run: |
-          echo "CONTRACT_ADDRESS=$(forge script script/Deployment.s.sol:NovaVerifierDeployer --fork-url $ANVIL_URL --private-key $ANVIL_PRIVATE_KEY --broadcast --non-interactive | sed -n 's/.*Contract Address: //p' | tail -1)" >> $GITHUB_OUTPUT
-        id: deployment
+      #- name: Deploy main contract
+      #  run: |
+      #    echo "CONTRACT_ADDRESS=$(forge script script/Deployment.s.sol:NovaVerifierDeployer --fork-url $ANVIL_URL --private-key $ANVIL_PRIVATE_KEY --broadcast --non-interactive | sed -n 's/.*Contract Address: //p' | tail -1)" >> $GITHUB_OUTPUT
+      #  id: deployment

-      - name: Load proof and public parameters
-        run: |
-          python loader.py pp-verifier-key.json pp-compressed-snark.json ${{steps.deployment.outputs.CONTRACT_ADDRESS}} $ANVIL_URL $ANVIL_PRIVATE_KEY
+      #- name: Load proof and public parameters
+      #  run: |
+      #    python loader.py pp-verifier-key.json pp-compressed-snark.json ${{steps.deployment.outputs.CONTRACT_ADDRESS}} $ANVIL_URL $ANVIL_PRIVATE_KEY

-      - name: Check proof verification status
-        run: |
-          [[ $(cast call ${{steps.deployment.outputs.CONTRACT_ADDRESS}} "verify(uint32,uint256[],uint256[],bool)(bool)" "3" "[1]" "[0]" "true" --private-key $ANVIL_PRIVATE_KEY --rpc-url $ANVIL_URL) == true ]] && exit 0 || exit 1
+      #- name: Check proof verification status
+      #  run: |
+      #    [[ $(cast call ${{steps.deployment.outputs.CONTRACT_ADDRESS}} "verify(uint32,uint256[],uint256[],bool)(bool)" "3" "[1]" "[0]" "true" --private-key $ANVIL_PRIVATE_KEY --rpc-url $ANVIL_URL) == true ]] && exit 0 || exit 1



Did you mean to keep it commented? We could have some conditions to make it run when !test is commented, but only on feature/* branches?

Also, should we have this flow possible on both issues and PRs or only PRs?

For the main branch we don't need integration testing - though it is relevant for other branches. Let's involve @samuelburnham to provide some input on this - as he initially helped with that stuff.

We could have some conditions to make it run when !test is commented, but only on feature/* branches

That seems to be quite good approach

I can tackle running on feature/* branches, sounds like a great plan.

Also, should we have this flow possible on both issues and PRs or only PRs?

PRs only I think, if we want to run on branches without an open PR it can be done via workflow_dispatch from https://github.com/lurk-lab/solidity-verifier/blob/main/.github/workflows/ci.yml#L8

PRs only sounds great :D

tchataigner · 2023-12-12T11:23:17Z

README.md

+The idea is actually to gather required cryptographic building blocks (Pasta / Grumpkin curve operations, Poseidon, KeccakTranscript, Sumcheck protocol, etc.) in `main` branch, 
+evaluate them and check that they work as expected via test vectors provided by "trusted" reference Rust implementation ([Arecibo](https://github.com/lurk-lab/arecibo)).
+Since reference proving system is under active development, the original end-to-end verification flow is a subject of changes, that is why, full e2e contracts are located in various branches,
+depending on the Nova cryptographic feature. See [pasta](https://github.com/lurk-lab/solidity-verifier/tree/pasta) [grumpkin](https://github.com/lurk-lab/solidity-verifier/tree/grumpkin), [zeromorph](https://github.com/lurk-lab/solidity-verifier/tree/zeromorph), [gas-optimizing](https://github.com/lurk-lab/solidity-verifier/tree/gas-optimizing) branches for more details.


Suggested change

depending on the Nova cryptographic feature. See [pasta](https://github.com/lurk-lab/solidity-verifier/tree/pasta) [grumpkin](https://github.com/lurk-lab/solidity-verifier/tree/grumpkin), [zeromorph](https://github.com/lurk-lab/solidity-verifier/tree/zeromorph), [gas-optimizing](https://github.com/lurk-lab/solidity-verifier/tree/gas-optimizing) branches for more details.

depending on the Nova cryptographic feature. See [pasta](https://github.com/lurk-lab/solidity-verifier/tree/pasta), [grumpkin](https://github.com/lurk-lab/solidity-verifier/tree/grumpkin), [zeromorph](https://github.com/lurk-lab/solidity-verifier/tree/zeromorph), [gas-optimizing](https://github.com/lurk-lab/solidity-verifier/tree/gas-optimizing) branches for more details.

- Repository structure - Overview of the different feature branches

huitseeker

Workflows and their triggers are different. Workflows live on the main branch of the repo and trigger according to the conditions on their "on:" section.
https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows
Iow, if we want some jobs to not run on e.g. the master branch's pushes, it should be enough to exclude the master branch pushes from their triggers. No need to comment everything.

Please note there is a branches-ignore attribute you can set on most events: https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#running-your-pull_request-workflow-based-on-the-head-or-base-branch-of-a-pull-request

- Fix NatSpec comment order over Pallas.sol and Vesta.sol - Added Solidity comments over PoseidonNeptuneU24Optimized.sol and Sponge.sol - Added README in poseidon folder

- Solidity doc for all cryptographic blocks file - README in src/blocks to explain each file and their content - Fixed typo in library name

storojs72 · 2023-12-13T14:56:34Z

@huitseeker, executing integration testing via commenting in the PR is quite convenient as we can manage whether to run it or not. With specific branch protection rules, we can manage whether integration test passing is required or not. For example for main it is not required before merging (since we have no e2e contract in main). For pasta and grumpkin branches, it is required, since we have e2e contracts there. For zeromorph and gas-optimised branches - it is not yet required, but will be required later, when relevant e2e contracts appear.

storojs72 · 2023-12-13T15:17:02Z

I think, we can follow this simple policy for now (using branch protection rules):

require unit tests passing for main and all derived branches (as they don't have e2e contracts);
require unit tests and integration tests passing for pasta and grumpkin branches (as they have e2e contracts). In order to invoke executing integration tests, one of core contributors (from our organisation) should comment !test (also it should work for PRs from external contributors).

When we finish composing the e2e contracts for zeromorph and gas-optimizing or whatever feature in future, we add new branch protection rule for correspondent branch and enjoy.

huitseeker · 2023-12-13T15:26:03Z

@storojs72 I'm not criticizing the merge policy or the tests you want to run on any particular branch. I trust you on that part.
I'm criticizing the specific manner in which you run those tests.

There are two parts to each CI workflow, which correspond to actual distinct areas a workflow file:

describing the steps of a particular job (that's the jobs: section in GH actions),
describing the events that lead to those steps being run (that's the on: section in GH actions),

In order to change when tests are run, the natural and efficient way to do things is to modify part 2.
If order to remove tests that will no longer ever be needed on any branch at all, it makes sense to remove workflow steps from part 1.

I do not understand the logic that leads to commenting steps in part 1, I'd love an explanation.

storojs72 · 2023-12-13T15:31:19Z

@huitseeker, I finally got that. Yes, commenting steps of integration testing was not a good idea. In 52b17a3 it is actually reverted and only one thing is added - skipping the integration-tests-e2e job execution for main branch (even if it is somehow triggered)

huitseeker

This now makes sense to me, thanks!

samuelburnham · 2023-12-13T16:14:22Z

.github/workflows/end2end.yml

@@ -21,6 +21,7 @@ jobs:
      && github.event.issue.state == 'open'
      && contains(github.event.comment.body, '!test')
      && (github.event.comment.author_association == 'MEMBER' || github.event.comment.author_association == 'OWNER')
+      && github.ref != 'refs/heads/main'


Have you tested this works? I'm worried this will never return true, because issue_comment actions are always triggered from the default branch and we haven't checked out the base branch of the PR at this point.

I'm also afraid of this scenario.

Another way of fetching informations on concerned branch might be to use github.event.issue.pull_request to get info on the source branch.

@tchataigner I've managed to find a workaround in #45. A nifty alternative is to run gh pr view <pr-number> --json baseRefName --jq '.baseRefName', but GitHub certainly doesn't make this easy to figure out 😄

You are both right. We have #45 for tackling with this, and probably we will need merge it first

#45 has been merged and it looks like we have this issue resolved

tchataigner

I think the e2e CI will not work with the newly added condition.

tchataigner · 2023-12-13T17:18:26Z

.github/workflows/end2end.yml

@@ -21,6 +21,7 @@ jobs:
      && github.event.issue.state == 'open'
      && contains(github.event.comment.body, '!test')
      && (github.event.comment.author_association == 'MEMBER' || github.event.comment.author_association == 'OWNER')
+      && github.ref != 'refs/heads/main'


I'm also afraid of this scenario.

Another way of fetching informations on concerned branch might be to use github.event.issue.pull_request to get info on the source branch.

- NatSpec for implementation of Sponge over the Optimized Poseidon library.

README update & Solidity NatSpec comments

storojs72 added 6 commits December 8, 2023 17:53

Remove e2e contract and integrative components

04cf9e9

Update README

98a2458

Add Bn256/Grumpkin curves arithmetics

9391349

Move constants (JSON) files to 'src/blocks/poseidon' path

b5a640b

Add optimized Poseidon library

3f12ba3

Add Zeromorph library

5057fa1

storojs72 requested review from huitseeker and tchataigner December 11, 2023 17:18

Stub for e2e integration testing

603e9e8

tchataigner reviewed Dec 12, 2023

View reviewed changes

tchataigner added 6 commits December 12, 2023 13:12

chore(doc): update README for main

b7842d2

- Repository structure - Overview of the different feature branches

chore(doc): SPDX License identifier

b14191a

chore(doc): comment Bn256/Grumpkin files

65495cc

chore(doc): fixed solidity doc for Bn256/Grumpkin files

7b92ee3

chore(doc): README for Grumpkin Bn256

679e154

chore(doc): pasta solidity doc & README

44ca19f

huitseeker requested changes Dec 13, 2023

View reviewed changes

tchataigner added 3 commits December 13, 2023 12:14

chore(doc): new doc + fix

13fcd9e

- Fix NatSpec comment order over Pallas.sol and Vesta.sol - Added Solidity comments over PoseidonNeptuneU24Optimized.sol and Sponge.sol - Added README in poseidon folder

chore(doc): doc for all cryptographic blocks

2626cad

- Solidity doc for all cryptographic blocks file - README in src/blocks to explain each file and their content - Fixed typo in library name

chore(doc): doc for Utilities.sol

836f496

storojs72 mentioned this pull request Dec 13, 2023

CI adaptation #43

Closed

Skip execution of e2e integration testing on main branch

135c917

storojs72 force-pushed the cleanup-main branch from 603e9e8 to 135c917 Compare December 13, 2023 14:50

chore(doc): merge latest cleanup commit

0f7d7e9

chore(doc): fix fmt

75b64c4

storojs72 requested a review from huitseeker December 13, 2023 15:17

storojs72 requested review from tchataigner and samuelburnham December 13, 2023 15:17

Requested changes and adjustments

52b17a3

storojs72 force-pushed the cleanup-main branch from eb74e10 to 52b17a3 Compare December 13, 2023 15:20

huitseeker approved these changes Dec 13, 2023

View reviewed changes

samuelburnham reviewed Dec 13, 2023

View reviewed changes

tchataigner requested changes Dec 13, 2023

View reviewed changes

tchataigner mentioned this pull request Dec 14, 2023

README update & Solidity NatSpec comments #44

Merged

ci: Fail e2e tests when PR base is main (#45)

de5db03

storojs72 requested a review from tchataigner December 14, 2023 12:15

tchataigner and others added 9 commits December 14, 2023 13:34

chore: merge cleanup-main

7f1848d

Merge branch 'cleanup-main' into feature/readme-doc-cleanup

2885503

Add Sponge library that supports optimized Poseidon

512e8b5

Merge remote-tracking branch 'origin/cleanup-main' into cleanup-main

c5351ac

Merge branch 'cleanup-main' into feature/readme-doc-cleanup

637ded6

chore(doc): followed review comments

9db542e

chore(doc): added NatSpec comments

c3baa6c

- NatSpec for implementation of Sponge over the Optimized Poseidon library.

chore(doc): fixed fmt

315b998

Merge pull request #44 from tchataigner/feature/readme-doc-cleanup

07d04f0

README update & Solidity NatSpec comments

tchataigner approved these changes Dec 15, 2023

View reviewed changes

storojs72 enabled auto-merge December 15, 2023 13:12

storojs72 added this pull request to the merge queue Dec 15, 2023

Merged via the queue into main with commit acb74cd Dec 15, 2023
1 check passed

samuelburnham mentioned this pull request Feb 6, 2024

ci: Port changes from main #48

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reorganisation of `main` branch #42

Reorganisation of `main` branch #42

storojs72 commented Dec 11, 2023

storojs72 commented Dec 11, 2023

tchataigner left a comment •

edited

Loading

tchataigner Dec 12, 2023

storojs72 Dec 12, 2023

samuelburnham Dec 12, 2023

tchataigner Dec 12, 2023

tchataigner Dec 12, 2023

huitseeker left a comment

storojs72 commented Dec 13, 2023 •

edited

Loading

storojs72 commented Dec 13, 2023

huitseeker commented Dec 13, 2023

storojs72 commented Dec 13, 2023

huitseeker left a comment

samuelburnham Dec 13, 2023

tchataigner Dec 13, 2023

samuelburnham Dec 13, 2023

storojs72 Dec 13, 2023

storojs72 Dec 14, 2023

tchataigner left a comment

tchataigner Dec 13, 2023

	depending on the Nova cryptographic feature. See [pasta](https://github.com/lurk-lab/solidity-verifier/tree/pasta) [grumpkin](https://github.com/lurk-lab/solidity-verifier/tree/grumpkin), [zeromorph](https://github.com/lurk-lab/solidity-verifier/tree/zeromorph), [gas-optimizing](https://github.com/lurk-lab/solidity-verifier/tree/gas-optimizing) branches for more details.
	depending on the Nova cryptographic feature. See [pasta](https://github.com/lurk-lab/solidity-verifier/tree/pasta), [grumpkin](https://github.com/lurk-lab/solidity-verifier/tree/grumpkin), [zeromorph](https://github.com/lurk-lab/solidity-verifier/tree/zeromorph), [gas-optimizing](https://github.com/lurk-lab/solidity-verifier/tree/gas-optimizing) branches for more details.

Reorganisation of main branch #42

Reorganisation of main branch #42

Conversation

storojs72 commented Dec 11, 2023

storojs72 commented Dec 11, 2023

tchataigner left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huitseeker left a comment

Choose a reason for hiding this comment

storojs72 commented Dec 13, 2023 • edited Loading

storojs72 commented Dec 13, 2023

huitseeker commented Dec 13, 2023

storojs72 commented Dec 13, 2023

huitseeker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchataigner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Reorganisation of `main` branch #42

Reorganisation of `main` branch #42

tchataigner left a comment •

edited

Loading

storojs72 commented Dec 13, 2023 •

edited

Loading