add non static call constraint for sstore

[DreamWuGit]

sstore op code will modify state, so if it is executed in static call , exception will occur , here constrain only in non static call , spec privacy-ethereum/zkevm-specs#188

[DreamWuGit]

the CI error looks weird to me (https://github.com/appliedzkp/zkevm-circuits/runs/6073405191?check_suite_focus=true), Does anybody know that ? @CPerezz @ChihChengLiang @miha-stopar ?

[ChihChengLiang]

@DreamWuGit Created an issue #470 and I'm investigating. For reviewers, Feel free to ignore that error and do the review.

[roynalnaruto]

Minor suggestions from my side, looks good otherwise :)

[AronisAt79]

the CI error looks weird to me (https://github.com/appliedzkp/zkevm-circuits/runs/6073405191?check_suite_focus=true), Does anybody know that ? @CPerezz @ChihChengLiang @miha-stopar ?

@DreamWuGit this happens because github.secrets are not shared with workflows triggered from PRs on forks.Therefore the invoked action is not permitted to access the https://api.github.com/repos/appliedzkp/zlevm-circuits/pull/467/requested_reviewers endpoint. We are working on alternatives with @ntampakas

[ntampakas]

We are trying to avoid GH Actions limitations on forked repos, by implementing two different workflows:

1. An action, triggered by the PR coming from the fork, that will create and upload an artifact that will contain all the necessary info (f.x.: PR_NUM, REPO, etc.)
2. A cron job that will be executed periodically on the base repo, after downloading the artifact mentioned above and assigning randomly selected reviewers to the PR in question.

This is the cleanest way to trigger workflows from PRs coming from forks, that bypasses the necessity to access the GH secret(s).
There are a couple of drawbacks:
Github does no guarantee that the workflow using the schedule event, will run at the pre-defined time.

[ntampakas]

Moved to #446

[icemelon]

overall LGTM. Just need to address the comments

[DreamWuGit]

@ntampakas I am curious why other PR not hit this issue ， even my other PR (i.e. #466 is OK) :）

[DreamWuGit]

@ntampakas I am curious why other PR not hit this issue ， even my other PR (i.e. #466 is OK) :）

after push new stuff, CI can pass now .

[CPerezz]

Following up on the previous message, here is the implementation we have concluded on with pros and cons.

1. Every PR (either from fork or not) creates and uploads an artifact on base repo, consisting of all PR info.

2. A second workflow runs periodically as a scheduled task, downloads and extracts the artifact and in general may run any action requiring access to GitHub token: In our case the action is to randomly select reviewer(s) and assign them in PR.

Pros: No need to distribute tokens to external accounts, as first artifact creation requires no write permission. Same concept can be used for providing benchmark results in comments.

Cons: Second workflow is executed as a cron job, so it will flood the actions history with workflow run records. It will make it a bit annoying to read the actions history (without filtering). As mentioned in the comment above, Github does no guarantee that the workflow using the schedule event, will run at the pre-defined time. Therefore reviewers assignment, might take more than expected.

If everyone agrees on this approach, i will shortly raise a PR for review. @ed255 @CPerezz @ChihChengLiang

@AronisAt79 @ntampakas could you move this thread to the issue/PR where it corresponds to?

[CPerezz]

LGTM.

Can we update and merge this??

[CPerezz]

@lispc I can't rebase myself in order to merge it.
Please do so and ping me on TG or here in the conversation once the rebase has passed the CI so that I can merge this PR 😄

[lispc]

@CPerezz

i don't fully understand "rebase myself in order to merge it".

I clicked the "update branch" button. And i think there is a "squash and merge" button, which is very helpful for things like this?

[CPerezz]

Oh @lispc I thought you couldn't merge the PR.

Ye, just squash and merge please! :)

[lispc]

nonono, I cannot merge this. I snapshot from another repo just to give a demo..

[CPerezz]

nonono, I cannot merge this. I snapshot from another repo just to give a demo..

OOOOH! No, what I meant was that when your branch is out-of-date you can rebase or merge main into it.
I wanted you to rebase. That was all ahhaha

Thanks!! Merging!