-
Notifications
You must be signed in to change notification settings - Fork 24
Porting feature #1108 and #905 from mlkem-native to mldsa-native #413
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
base: main
Are you sure you want to change the base?
Conversation
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.
Two comments.
In general this sort of PR is an ideal candidate to identify further gaps compared to mlkem-native. Please let me know what differences you have encountered, so we can open issue for those that should be 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 see that the format script has diverged from mlkem-native.
Can we consolidate that by either fixing it in this PR, or opening an issue.
I see there is already #158 for the magic constant, but there are 3 more difference:
- check-constracts
- shell linting
- copyright checking
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.
Hello, Matthias,
Thank you for your reviewing, open issue:
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 do see a diff in run_cbmc_single_step
compared to mlkem-native that looks unintentional to me. Can you consolidate that please?
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.
While at it - can you fix this line:
log.error(
"Could not find function {self.args.start_with}. Running all proofs"
)
log is not defined.
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.
Hello, Matthis, thank you for your reviewing!
Both issues you mentioned been fixed as follow:
-
For the first part, I aligned the error handling in mldsa with the approach used in mlkem, which involves three main updates:
- aligned the way
capture_output
is handled. - aligned the handling of
subprocess.TimeoutExpired
. - Added an additional checking before decoding
p.stderr
.
- aligned the way
-
For the second part, I created a corresponding log instance for the
run_cbmc
,to solve the not defined error, -
And I throught that the same issue exists in the mlkem and need to be fixed. If my fix at here is acceptable, I will create a separate issue and apply the same fix in the mlkem-native repository.
d2a700f
to
daf8962
Compare
- This commit is ported from: - mlkem-native, commit: 1f40536 - `autogen` prints high-level status updates in addition to the low-level per-file status updates - `autogen` uses color coding like `format` does - `lint` omits GH summary content if GITHUB_STEP_SUMMARY is not set. - format uses color-coded error messages - The color definitions across the scripts have been harmonized Note: We did not port the functional changes from commit 1f40536 due to the divergence between the two project repositories. However, if `synchronize_backends`, `gen_citations_for` or other functions are introduced in the future, we will consider incorporating those changes into mldsa-native. Signed-off-by: willieyz <[email protected]>
- This commit is ported from: - mlkem, commit: 615d972 - This commit adds support for the use of wildcard pattern `*` in invocations of `tests cbmc`. Example: ``` tests cbmc -p "*poly*" ``` This will run all tests for functions containing "poly". - As before, multiple patterns can be provided, e.g. ``` tests cbmc -p "*keccak*" "*once*" ``` Signed-off-by: willieyz <[email protected]>
daf8962
to
04ff52a
Compare
This PR port the following two PR from mlkem-native to here (mldsa-native):