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.

Sign up for GitHub

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

Improve documentation for BN curve parameter ATE_LOOP_COUNT #275

Merged
merged 2 commits into from
May 12, 2021
Merged

Improve documentation for BN curve parameter ATE_LOOP_COUNT #275

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
872e7a4
improve documentation for BN curve ATE_LOOP_COUNT
weikengchen May 12, 2021
0df71d6
fix
weikengchen May 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions ec/src/models/bn/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,16 @@ pub enum TwistType {
}

pub trait BnParameters: 'static {
// The absolute value of the BN curve parameter `X` (as in `q = 36 X^4 + 36 X^3 + 24 X^2 + 6 X + 1`).
const X: &'static [u64];
// Whether or not `X` is negative.
const X_IS_NEGATIVE: bool;

// The absolute value of `6X + 2`.
const ATE_LOOP_COUNT: &'static [i8];
// Whether or not `6X + 2` is negative. `6X + 2` is negative often when `X` is negative.
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't 6X + 2 negative exactly when X is negative? (Because X is an integer and 2 < 6.) That implies that this field should probably just be removed and X_IS_NEGATIVE used instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I'm confused because this doc appears to contradict the following sentence in the PR description:

This PR is to clarify that ATE_LOOP_COUNT is 6 X + 2 instead of |6 X + 2|, a possible confusion.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right.

Let me clarify this PR's documentation as well as the documentation in the code (you are right, 2 < 6 so X must be negative).

I think there are two possible future plans:

  1. Not too breaking, which can be done now:
  • Use X_IS_NEGATIVE to decide whatever ATE_LOOP_COUNT_IS_NEGATIVE is deciding, since they should be equivalent for BN curves.
  1. Breaking:
  • Remove ATE_LOOP_COUNT_IS_NEGATIVE and update the BN254 implementation accordingly.

const ATE_LOOP_COUNT_IS_NEGATIVE: bool;

const TWIST_TYPE: TwistType;
const TWIST_MUL_BY_Q_X: Fp2<Self::Fp2Params>;
const TWIST_MUL_BY_Q_Y: Fp2<Self::Fp2Params>;
Expand Down