Skip to content
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

Add Dilithium Documentation #13

Merged
merged 1 commit into from
Jul 18, 2023
Merged

Add Dilithium Documentation #13

merged 1 commit into from
Jul 18, 2023

Conversation

atreiber94
Copy link
Collaborator

No description provided.

@atreiber94 atreiber94 requested a review from lieser April 4, 2023 16:16
@atreiber94 atreiber94 changed the title Dilithium Documentation Add Dilithium Documentation Apr 4, 2023
cryptodoc/src/06_pubkey_key_generation.rst Outdated Show resolved Hide resolved
cryptodoc/src/08_signatures.rst Outdated Show resolved Hide resolved
cryptodoc/src/08_signatures.rst Outdated Show resolved Hide resolved
@reneme reneme mentioned this pull request May 2, 2023
Copy link
Owner

@sehlen-bsi sehlen-bsi left a comment

Choose a reason for hiding this comment

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

Done with this first review. After the changes have been applied/reviewed, it can be merged and I'll revisit the hint generation in the next few weeks separately.


**Polynomial Operations**

Operations between polynomials, polynomial vectors, and polynomial matrices are provided in ``src/lib/pubkey/dilithium/dilithium_common/dilithium_polynomials.h``, including NTT, multiplication, and Montgomery reduction.
Copy link
Owner

Choose a reason for hiding this comment

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

(Sorry for asking off-topic questions again) Question: why is there not a shared codebase for Kyber and Dilithium?

cryptodoc/src/08_signatures.rst Outdated Show resolved Hide resolved
cryptodoc/src/08_signatures.rst Outdated Show resolved Hide resolved
cryptodoc/src/08_signatures.rst Outdated Show resolved Hide resolved
cryptodoc/src/06_pubkey_key_generation.rst Outdated Show resolved Hide resolved
cryptodoc/src/06_pubkey_key_generation.rst Outdated Show resolved Hide resolved
cryptodoc/src/08_signatures.rst Outdated Show resolved Hide resolved
cryptodoc/src/08_signatures.rst Show resolved Hide resolved
cryptodoc/src/08_signatures.rst Show resolved Hide resolved
@atreiber94
Copy link
Collaborator Author

Applied the suggestions (except for off-topic comment and the one referring #71)

@atreiber94 atreiber94 requested a review from FAlbertDev July 12, 2023 14:50
In order to take care of this, Dilithium computes a "hint".
The corresponding simple algorithm is :math:`\mathsf{MakeHint}_q` specified in Figure 3 of [Dilithium-R3]_.

To see that Botan's hint computation on inputs ``(w0 - c*s2 + c*t0, w1)`` is equivalent to the specification of [Dilithium-R3]_, we look at the hint creation in Figure 3, L. 23 of [Dilithium-R3]_.
Copy link
Owner

Choose a reason for hiding this comment

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

But now I think we need to mention here that Botan's implementation dffers from Figure 3 in the spec and mention the Dilithium reference implementation as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in 5382773

Copy link
Collaborator

@FAlbertDev FAlbertDev left a comment

Choose a reason for hiding this comment

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

Good job! I think the reduction of in-text source references creates a much more fluent and readable text.

cryptodoc/src/06_pubkey_key_generation.rst Show resolved Hide resolved
cryptodoc/src/06_pubkey_key_generation.rst Outdated Show resolved Hide resolved
Comment on lines 766 to 769
Dilithium uses a simple technique to reduce the size of the public key.
Given the public matrix :math:`A` and :math:`t = As_1 + s_2`, the public key only contains the "low order" bits of :math:`t`.
However, when computing with :math:`t`, a carry can occur, influencing the high order bits.
In order to take care of this, Dilithium computes a "hint".
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Dilithium uses a simple technique to reduce the size of the public key.
Given the public matrix :math:`A` and :math:`t = As_1 + s_2`, the public key only contains the "low order" bits of :math:`t`.
However, when computing with :math:`t`, a carry can occur, influencing the high order bits.
In order to take care of this, Dilithium computes a "hint".
Dilithium uses a simple technique to reduce the size of the public key.
Given the public matrix :math:`A` and :math:`t = As_1 + s_2`, the public key only contains the "high-order" bits t_1 of :math:`t`. When computing the high bits of the sum Az-ct within the verification algorithm, t_1 and a hint vector h is sufficient. This hint vector contains the carry bits describing how the lower bits t_0 of t affect the high bits of the computed sum.

IMO the current description is a little bit unclear. My description is also not very clean, though. Maybe we just want to write something like: "Section 2.4 of [Dilithium-R3]_ describes Dilithium's public key compression using 1-bit hints."

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think your description is nice, I may just polish it a bit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Polished in e9acce5. The intro is now a bit more detailed, but so is the section.

@reneme reneme dismissed sehlen-bsi’s stale review July 18, 2023 09:27

Changes were addressed. Dismiss to unblock. Feel free to open a ticket if further changes are required.

Co-Authored-By: Fabian Albert <[email protected]>
@reneme
Copy link
Collaborator

reneme commented Jul 18, 2023

Solved conflicts, rebased an squashed. Waiting for CI, then @FAlbertDev may merge.

@FAlbertDev FAlbertDev merged commit 61b88cf into main Jul 18, 2023
@reneme reneme deleted the dilithium_doc branch July 18, 2023 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants