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

Jump to bottom

feat: add banderwagon #271

Merged
merged 24 commits into from
Sep 23, 2023
Merged

feat: add banderwagon #271

merged 24 commits into from
Sep 23, 2023

Conversation

advaita-saha
Copy link
Collaborator

Fixes #263
Adds all the features mentioned in the issue

Other than that, the following have been changed :

  • Status codes have been shifted to a common codecs_status_codes.nim file
  • trySetFromCoordX added to Twisted Edwards curve, both projective & affine

mratsim
mratsim reviewed Sep 13, 2023
View reviewed changes
Copy link
Owner

@mratsim mratsim left a comment

Choose a reason for hiding this comment

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

Looks good so far

constantine/math/config/curves_declaration.nim Outdated Show resolved Hide resolved
constantine/serialization/codecs_banderwagon.nim Outdated Show resolved Hide resolved
constantine/serialization/codecs_banderwagon.nim Outdated Show resolved Hide resolved
constantine/serialization/codecs_banderwagon.nim Show resolved Hide resolved
constantine/serialization/codecs_banderwagon.nim Outdated Show resolved Hide resolved
constantine/math/constants/banderwagon.nim Outdated Show resolved Hide resolved
constantine/math/constants/banderwagon.nim Outdated Show resolved Hide resolved
@advaita-saha advaita-saha marked this pull request as ready for review September 13, 2023 12:23
mratsim
mratsim reviewed Sep 14, 2023
View reviewed changes
constantine/math/constants/banderwagon.nim Outdated Show resolved Hide resolved
constantine/math/constants/banderwagon.nim Outdated Show resolved Hide resolved
constantine/math/constants/banderwagon.nim Outdated Show resolved Hide resolved
mratsim
mratsim approved these changes Sep 17, 2023
View reviewed changes
Copy link
Owner

@mratsim mratsim left a comment

Choose a reason for hiding this comment

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

LGTM, we only need tests now

constantine/math/elliptic/ec_twistededwards_affine.nim Outdated Show resolved Hide resolved
constantine/math/elliptic/ec_twistededwards_projective.nim Outdated Show resolved Hide resolved
@advaita-saha
Copy link
Collaborator Author

@mratsim While reviewing please have a close look at the precompute for primeMinus1div2 as I am not very sure how good the implementation I did for that

mratsim
mratsim approved these changes Sep 23, 2023
View reviewed changes
Copy link
Owner

@mratsim mratsim left a comment

Choose a reason for hiding this comment

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

Excellent

constantine/math/config/precompute.nim Show resolved Hide resolved
constantine/math/config/precompute.nim Outdated Show resolved Hide resolved
constantine/math/elliptic/ec_twistededwards_affine.nim
P.y.neg()
P.y += one

# (1 - ax²)
Copy link
Owner

Choose a reason for hiding this comment

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

a future TODO, a = 5 and we should shortcut when a <= 15 to use the addition chains here:

constantine/constantine/math/arithmetic/finite_fields.nim

Lines 475 to 479 in 7b64f85

elif b == 5:
var t {.noInit.}: typeof(a)
t.double(a)
t.double()
a += t

@mratsim mratsim merged commit f925853 into mratsim:master Sep 23, 2023
6 checks passed
@advaita-saha advaita-saha deleted the banderwagon branch October 15, 2023 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mratsim mratsim mratsim approved these changes

Assignees
No one assigned
Labels
verkle trees 🌴
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add Banderwagon
2 participants
@advaita-saha @mratsim