-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
feat: prover version compatibility matrix #3618
base: main
Are you sure you want to change the base?
Conversation
All in all, this is already looking pretty reasonable. Left a bunch of comments, mostly aimed at maintainability. Looking forward to hear more! P.S. We might need someone with more knowledge in github actions to review it as well. |
@@ -0,0 +1,6 @@ | |||
{ |
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.
Is this automatically generated? I assume so. The version it matches to, is that core version or protocol version?
@@ -30,6 +32,30 @@ jobs: | |||
|
|||
mkdir -p prover_logs_${{matrix.compressor-mode}} | |||
|
|||
- name: Identify prover and core versions to run |
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.
How many jobs will this cause now? 8? And are they parallel or sequential? I think this kind of changes should be ran only on tag cutting mode. WDYT?
- name: Verify compatibility matrix | ||
run: | | ||
MATRIX_FILE="prover/compatibility_matrix.json" | ||
CORE_RANGE_START=24 |
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.
Who is going to keep those updated? AFAIU, we're already out of sync (v25 is in mainnet, but correct me if I'm wrong).
fi | ||
done | ||
|
||
for ((ver=$CORE_RANGE_START; ver<=$CORE_RANGE_END; ver++)); do |
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.
Some comments on what's going on in here wouldn't hurt. Applies across the entire set of bashery.
echo "Found core versions outside allowed range ($CORE_RANGE_START-$CORE_RANGE_END)" | ||
jq "[.[]] | flatten | map(tonumber) | map(select(. < $CORE_RANGE_START or . > $CORE_RANGE_END))" "$MATRIX_FILE" | ||
exit 1 | ||
fi |
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.
End of line (here & in matrix)
This pull request enhances our CI pipeline to ensure compatibility between the core and prover components. Specifically, it introduces the following verification steps:
Additionally, this PR:
Introduces a version compatibility matrix between prover and core and enforces its correctness in a separate GitHub workflow.