-
Notifications
You must be signed in to change notification settings - Fork 12
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
Reorganisation of main
branch
#42
Conversation
!test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I have only a discussion to have on the ci workflow. Otherwise the reorg seems to be as you previously specified :)
.github/workflows/end2end.yml
Outdated
#- 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 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PRs only sounds great :D
README.md
Outdated
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
603e9e8
to
135c917
Compare
@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 |
I think, we can follow this simple policy for now (using branch protection rules):
When we finish composing the e2e contracts for |
eb74e10
to
52b17a3
Compare
@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. There are two parts to each CI workflow, which correspond to actual distinct areas a workflow file:
In order to change when tests are run, the natural and efficient way to do things is to modify part 2. I do not understand the logic that leads to commenting steps in part 1, I'd love an explanation. |
@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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This now makes sense to me, thanks!
.github/workflows/end2end.yml
Outdated
@@ -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' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are both right. We have #45 for tackling with this, and probably we will need merge it first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#45 has been merged and it looks like we have this issue resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the e2e CI will not work with the newly added condition.
.github/workflows/end2end.yml
Outdated
@@ -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' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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, sincemain
is usually considered as a source of "stable" codebase - we can keep fundamental cryptographic building blocks inmain
- 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.