Merged
Conversation
Fixes the issue found by @federicobarbacovi in `secp256k1_ecdsa_mul`. The issue was that the point at infinity flag `_is_infinity` was not being set explicitly when adding base points due to the skew flags. This issue arises when in the following edge case: ### Description of the issue Choose secp256k1 scalars $s_1, u_1, u_2 \in \mathbb{F}_r$ such that 1. the public key is computed as $P = s_1 \cdot G$ for generator $G \in \mathbb{G}$ 2. $u_1 \cdot G + u_2 \cdot P = \mathcal{O}$ where $\mathcal{O}$ is the point at infinity. In the `secp256k1_ecdsa_mul` function, the scalars $u_1$ and $u_2$ are split in endomorphism scalars: $$u_1 = u_{1, \textsf{low}} + \lambda \cdot u_{1, \textsf{high}}, \quad u_2 = u_{2, \textsf{low}} + \lambda \cdot u_{2, \textsf{high}}.$$ Each of the scalars $u_{1, \textsf{low}}, u_{1, \textsf{high}}, u_{2, \textsf{low}}, u_{2, \textsf{high}}$ has a skew bit: let them be $b_1, b_2, b_3, b_4$. We handle the skew bit in the accumulator $A$ as: 1. First compute addition $B = A + G \quad \text{or} \quad B = A - G$ based on the skew being positive or negative 2. Set the accumulator to $B$ if the skew is non-zero, else keep the accumulator unchanged: $A := (1 - b) \cdot A + b \cdot B$ While performing the second step, we conditionally selected only the `x` and `y` coordinates of $A$ or $B$ but didn't fetch the `_is_infinity` flag. This leads to the flag `_is_infinity` set to `false` even when the result of the scalar multiplication is point at infinity. ### Fix The fix is to correctly set the `_is_infinity` flag when performing conditional selection operation. So we just define a new `conditional_select` function on the `biggroup` (and `biggroup_goblin`) class. --------- Co-authored-by: federicobarbacovi <federicobarbacovi@users.noreply.github.com>
Fixes the issue found by @federicobarbacovi in `secp256k1_ecdsa_mul`. The issue was that while adding the stagger points, we were using incomplete addition formula (via `chain_add_start`, `chain_add`, `chain_add_end` functions). This leads to unsatisfiable circuit for certain sets of scalars $s_1, u_1, u_2 \in \mathbb{F}_r$ such that: 1. the public key is computed as $P = s_1 \cdot G$ for generator $G \in \mathbb{G}$ 2. $u_1 \cdot G + u_2 \cdot P = \mathcal{O}$ where $\mathcal{O}$ is the point at infinity. In particular, when the skew terms of each of the scalars $u_{1, \textsf{low}}, u_{1, \textsf{high}}, u_{2, \textsf{low}}, u_{2, \textsf{high}}$ is 0, we know that the accumulator would reach the point at infinity before handling any skew terms. In this case, that happens when performing point additions due to the stagger terms. We know the stagger offsets of the scalars are set as: | Term | Stagger bits | Point to add | |-------------|--------------|-------------| | $u_{1, \textsf{low}}$ | 2 | $P_3 := 2G$ | |$u_{1, \textsf{high}}$ | 3 | $P_1 := 3λG$ | |$u_{2, \textsf{low}}$ | 0 | — | | $u_{2, \textsf{high}}$ | 1 | $P_2 := λG$ | For the regression test case in this PR, we first add $P_1, P_2$: $A \longleftarrow (A +P_1)$ $A \longleftarrow (A + P_2)$ While adding $P_3$, we reach the point at infinity: $A + P_3 = \mathcal{O}$. To fix this, we simply use complete addition formula for adding each of $P_1, P_2, P_3$. This increases the circuit size for `secp256k1_ecdsa_mul` by 730 gates ($\approx 2$ percent increase in circuit size from 39957 to 40687 gates).
### 🧾 Audit Context Cleanup and audit of the lookup tables used in `biggroup` class. This PR does not change any logic, its mainly structural changes, cleanup and documentation. ### 🛠️ Changes Made - Added a README to explain how lookup tables are being used in biggroup, and a brief summary of how ROM tables work in barretenberg circuits. - Removed unused methods/structs like: `lookup_table_base`, `batch_lookup_table_base`, `create_endo_pair_five_lookup_table`. - Function comments + fix clang warnings + avoid hard-coded values. - No change to circuit, so no VK changes expected. ### ✅ Checklist - [x] Audited all methods of the relevant module/class - [x] Audited the interface of the module/class with other (relevant) components - [x] Documented existing functionality and any changes made (as per Doxygen requirements) - [x] Resolved and/or closed all issues/TODOs pertaining to the audited files - [x] Confirmed and documented any security or other issues found (if applicable) - [x] Verified that tests cover all critical paths (and added tests if necessary) - [ ] Updated audit tracking for the files audited (check the start of each file you audited) ### 📌 Notes for Reviewers NA
Can merge once approved
Comments and minor updates to the methods of cycle_group. Changes include: - New standalone tests for `dbl()`, `unconditional_add()`/`subtract()`, `operator+`/`-` (with gate count pinning to prevent unintended circuit changes) - New helper methods `constant_infinity()` and `is_constant_point_at_infinity()` for improved clarity - Remove member `_is_constant` in favor of method `is_constant()` which simply checks that `x.is_constant() && y.is_constant() && _is_infinity.is_constant()` - Reorganize `dbl()` and `_unconditional_add_or_subtract()` to remove hard to follow nested conditional logic - Simplify logic in `_variable_base_batch_mul_internal()` that was splitting up lookup gates and ECC add gates to ensure consecutive adds - this is no longer necessary due to our circuit block structure. (Note: this is what causes the VK change. No change in number of gates, just ordering)
Collaborator
Author
|
🤖 Auto-merge enabled after 4 hours of inactivity. This PR will be merged automatically once all checks pass. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
BEGIN_COMMIT_OVERRIDE
fix: secp256k1 ecdsa mul - fix handling of point at infinity (#16679)
fix: secp256k1 ecdsa mul handling of stagger point additions (#16685)
chore: biggroup audit of lookup and rom tables (#16895)
fix(bb): oversight that disabled batch commits (#17278)
docs(bb): add initial cli reference (#17244)
chore!: cycle group #6 (#17174)
fix: mac bb publish (#17276)
END_COMMIT_OVERRIDE