Skip to content

Conversation

jakemas
Copy link
Contributor

@jakemas jakemas commented Sep 18, 2025

Added in minimal framework for multi level build to support new config param mlkem-approach

@jakemas
Copy link
Contributor Author

jakemas commented Sep 18, 2025

The change appears to affect run time of ML-DSA-44 CBMC proof to 28m:

[CI / CBMC / CBMC (ML-DSA-44) / Run tests (pull_request)](https://github.com/pq-code-package/mldsa-native/actions/runs/17839953452/job/50727187298?pr=464)
CI / CBMC / CBMC (ML-DSA-44) / Run tests (pull_request)Successful in 28m

@jakemas jakemas marked this pull request as ready for review September 18, 2025 23:38
@jakemas jakemas requested a review from a team as a code owner September 18, 2025 23:38
*
*****************************************************************************/
#ifndef MLD_CONFIG_PARAMETER_SET
/* Map legacy MLDSA_MODE to new parameter set for backward compatibility */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can break backwards compatibility here, so let's choose MLD_CONFIG_PARAMETER_SET as the single source of truth and not allow any other configuration.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can MLDSA_MODE be removed entirely?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree - MLDSA_MODE should be removed entirely.

@hanno-becker
Copy link
Contributor

The change appears to affect run time of ML-DSA-44 CBMC proof to 28m:

@jakemas It's already like this on main, so not related to your PR.

Copy link
Contributor

@hanno-becker hanno-becker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jakemas Can you clean up the history and add short yet expressive commit messages?

Also, I don't think we need to worry about backwards compatibility at this point. If you introduce MLD_CONFIG_PARAMETER_SET as the new source of truth for the parameter set, that should supersede all others. Can MLDSA_MODE be removed?

@rod-chapman
Copy link
Contributor

Agree with Hanno - one parameter variable should be sufficient.

@jakemas
Copy link
Contributor Author

jakemas commented Sep 19, 2025

@hanno-becker Yes of course, I just wanted to get some review before squashing etc.

I thought the same thing about removing MLDSA_mode completely, that is exactly what I was doing with the non-stripped back version of this PR -- #453 but with CI acting as it is, I thought it was my changes causing these issues.

Ok, I will keep iterating and working on this, just wanted a check in, particularly with strange CI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Config: Add MLD_CONFIG_PARAMETER_SET + MLD_CONFIG_NAMESPACE_PREFIX
4 participants