Skip to content

chore: fix stale code generated files#827

Open
ivokub wants to merge 24 commits intomasterfrom
fix/stale-code-generation
Open

chore: fix stale code generated files#827
ivokub wants to merge 24 commits intomasterfrom
fix/stale-code-generation

Conversation

@ivokub
Copy link
Copy Markdown
Collaborator

@ivokub ivokub commented Mar 30, 2026

Description

I wanted to see if there are any stale generated files -- when we run go generate then we only create new files, but don't remove any existing implementation, so any leftovers from before will be kept.

So I deleted all files with "DO NOT EDIT" header and ran go generate ./... again, which indicated that the following files were stale:

  • ecc/bw6-761/internal/fptower/e6_direct.go
  • ecc/bw6-761/internal/fptower/e6_direct_test.go
  • ecc/grumpkin/marshal.go
  • ecc/grumpkin/marshal_test.go
  • ecc/secp256k1/hash_to_curve/g1.go
  • ecc/secp256k1/hash_to_g1.go
  • ecc/secp256k1/hash_to_g1_test.go
  • ecc/secp256r1/hash_to_curve/g1.go
  • ecc/secp256r1/hash_to_g1_test.go
  • ecc/secp256r1/multiexp.go
  • ecc/secp256r1/multiexp_affine.go
  • ecc/secp256r1/multiexp_jacobian.go
  • ecc/secp256r1/multiexp_test.go

for the direct e6 we have implemented it by hand, so the fix was to remove the header.

For the rest it seems it comes from the fact that we use early returns based on curve names, not by the features the curves provide.

Now instead of feature gating code generation based on curve names, we do it based on the features what the curves provide (HasG2, IsCompressiblePoint etc. or field suite MiMC etc based on configuration). This ensures that all the implementations are always up-to-date and we don't rely on manually-generated and copied implementation.

I implemented also code generation for a=-3 and a!=0 case. It seems with the dedicated formulas there is no regression compared to current hand-written implementation.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How has this been tested?

Generated files are consistent.

Checklist:

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • I did not modify files generated from templates
  • golangci-lint does not output errors locally
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Note

High Risk
Touches core elliptic-curve arithmetic (doubling/tripling/scalar-mul) and serialization across multiple curves; small mistakes could cause incorrect group operations or invalid point acceptance in security-critical code.

Overview
Regenerates a large set of ECC generated files and updates curve formulas/metadata across bls12-*, bls24-*, bn254, bw6-*, grumpkin, secp256k1, and secp256r1.

Arithmetic changes primarily clarify and standardize Jacobian/extended-Jacobian doubling constants (e.g., explicit M = 3*X²) and documentation around Triple, plus minor cleanup (scoping temps) across G1/G2 implementations.

Serialization and tests are refreshed: encoder/decoder error messages are curve-specific, compressed/uncompressed handling is tightened, several tests are adjusted for correct Jacobian initialization (FromAffine) and deterministic field ops, and secp256k1 gains a full generated marshal.go + tests. secp256r1 additionally updates formulas to support nonzero a (including adding a Triple implementation and switching scalar multiplication to NAF mixed-add), and adds new batch/subgroup-related tests/bench adjustments.

Written by Cursor Bugbot for commit 87b1bfe. This will update automatically on new commits. Configure here.

@ivokub ivokub marked this pull request as ready for review March 31, 2026 10:23
@ivokub
Copy link
Copy Markdown
Collaborator Author

ivokub commented Mar 31, 2026

@yelhousni one more thing I'm thinking about doing -- instead of manually defining the constants in ecc/{curve}/{curve}.go files, perhaps it would be better to code-generate these files as well? We could move the constants into curve configuration (also whatever comments we have etc about security, the polynomials defining the fields etc.) and then generate. One, this allows us to use the configuration in code generation and secondly it ensures everything is neatly uniform. What do you think?

Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

@ivokub ivokub requested review from gbotrel and yelhousni and removed request for gbotrel March 31, 2026 23:16
@ivokub ivokub removed the request for review from yelhousni March 31, 2026 23:30
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.

1 participant