Make signing and key generation C independent

[Al-Kindi-0]

Describe your changes

Addresses #281

Checklist before requesting a review

• Repo forked and branch created from next according to naming convention.
• Commit messages and codestyle follow conventions.
• Relevant issues are linked in the PR description.
• Tests added for new functionality.
• Documentation/comments updated according to changes.

[bobbinth]

Thank you! Looks good! This is not a full review yet - but i wanted to leave a few comments, primarily about the non-math portion of the code.

[bobbinth]

Still not a full review (I haven't really reviewed the math module) - but I left a few comments inline.

Thinking more about how to generalize hash_to_point(), I think there are broadly two approaches:

Approach 1 is to use something like FlaconContext trait. This trait could look like so:

pub trait FlaconContext {
    fn hash_to_point(message: Word, nonce: Nonce) -> Polynomial<FalconFelt>;
}

Then, we could parameterize sign() and verify() methods with this trait like so:

impl PrivateKey {
    pub fn sign<R: Rng, C: FalconContext>(
        &self,
        message: Word,
        mut rng: &Rng,
        context: C,
    ) -> Signature {
    }
}

impl Signature {
    pub fn verify<C: FalconContext>(
        &self,
        message: Word,
        pubkey_com: Word,
        context: C
    ) -> bool {
    }
}

We can also use function pointers instead of traits - but I think traits are a bit cleaner.

---

Approach 2 would be use enums to indicate which hash-to-point function to use. For example:

pub enum HashToPoint {
    Shake256,
    Rpo256,
}

And then sign() method could look like so:

impl PrivateKey {
    pub fn sign<R: Rng>(
        &self,
        message: Word,
        mut rng: &Rng,
        hash_to_point: HashToPoint,
    ) -> Signature {
    }
}

And since there are only 2 options for HashToPoint, we could encode it into the signature itself (not sure if you think this is a good idea though). In this case, verify() function signature would not need to change and Signature struct could look like so:

pub struct Signature {
    pk: Polynomial<FalconFelt>,
    s2: Polynomial<FalconFelt>,
    nonce: NonceBytes,
    hash_to_point: HashToPoint,
}

The first approach is more general as we'd be able to add other hash-to-point implementations pretty easily, but the second approach has slightly better ergonomics (in my opinion) - so, I'm currently on the fence.

It is also possible that there might be another approach which is better than these two.

[Al-Kindi-0]

The first approach is more general as we'd be able to add other hash-to-point implementations pretty easily, but the second approach has slightly better ergonomics (in my opinion) - so, I'm currently on the fence.

I went with the second approach as it has better ergonomics as you suggested. Let me know if this is what you were thinking.

[bobbinth]

Thank you! I pushed several commits which refactored the structure of keys and signature, and also left some more comments inline.

A couple of additional comments:

1. Since this now can handle regular Falcon signatures, should we name the directory falcon512 (rather than rpo_falcon512)?
2. Is there a way we can add more tests for the canonical Falcon case? Specifically, I'm wondering if there are some tests vectors that test the whole signature generation.

[Al-Kindi-0]

A couple of additional comments:

1. Since this now can handle regular Falcon signatures, should we name the directory `falcon512` (rather than `rpo_falcon512`)?

I have changed it now.

2. Is there a way we can add more tests for the canonical Falcon case? Specifically, I'm wondering if there are some tests vectors that test the whole signature generation.

Yes, that should be possible. One thing making this a bit more cumbersome is the fact that the hash-to-point algorithm takes the message as Word and this makes adapting the test-vectors difficult. One solution is to generate our own test vectors using the C reference implementation.

[bobbinth]

Looks good! Thank you! I left some more comments inline - but we are very close.

One thing: I haven't really reviewed the math module and I think it is probably OK as is. But I wonder if we should create an issue to come back to it, add tests/comments and maybe do some basic cleanup.

[bobbinth]

Thank you! Looks good! I've added a few comments inline - but I think most of them can be addressed in a follow-up PR.

(maybe the one about the header format could be addressed in this one)

[bobbinth]

One thing I noticed: signature generation algorithm seems to get "stuck" sometimes (e.g., when executing dsa::rpo_falcon512::keys::tests::test_falcon_verification test runs for over 60 seconds) - this happened once in CI and twice in my local testing (I probably ran the test 20 - 30 times total).

I am not sure if this is related to the use of libm (I don't think so). Or maybe there is a bug somewhere in the random sampling algorithm which fails to make progress under some conditions (maybe this is related to ensuring that s2 polynomial has an inverse?).

[bobbinth]

@Al-Kindi-0 - let's also update the changelog and the docs in the README file.

[bitwalker]

One thing I noticed: signature generation algorithm seems to get "stuck" sometimes (e.g., when executing dsa::rpo_falcon512::keys::tests::test_falcon_verification test runs for over 60 seconds) - this happened once in CI and twice in my local testing (I probably ran the test 20 - 30 times total).

I'd bet the culprit can be found here

It isn't clear what the upper bound is on that inner loop, but I get the impression pathological cases could occur where it loops for a long time. If it's happening frequently enough, then there is probably something wrong there. Might be worth setting up a second test using proptest that provides a seed as input to the RNG so that we can replicate issues like this (i.e. if the proptest fails due to taking too long, we'll know exactly what input failed, rather than having to hope we can trigger it again). Sounds like it happens pretty easily though.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

[bobbinth]

Thank you! Looks good! I ran the tests some more, and with the reverted structured didn't run into hanging issues.

I did some very crude benchmarking and it seems like key generation takes about 2 seconds while signing takes about 20ms. This quite a bit slower than what the standardized implementation can achieve. Big part of this slowdown is likely because we are using RPO as the hash function, but there are inefficiencies in other parts of the code too. At any rate, I think this performance is acceptable (at least for now).

Also, I pushed a few commits with some small changes, the biggest one of which is splitting sign() into sign() and sign_with_rng(). I think this PR is good to merge (which I will do shortly). The next steps on this front are:

1. Integrate these changes into miden-vm (I will try to release v0.9.0 of this crate by end of Friday).
2. Add tests against the canonical implementation.

[Al-Kindi-0]

I think if we want a fair comparison with the reference implementation then we would need to use a rng based on SHAKE256 (in std to allow for some optimizations). I noticed that once we changed to RPO things got very slow i.e., by more than single digit percentage points.
In any case, we can come back to benchmarking and testing after we finish the blocking tasks.