feat: merge-train/barretenberg#20012
Merged
Merged
Conversation
Fixes two issues found by @guidovranken in our ecdsa circuit. #### Issue 1: Point-at-Infinity in ecdsa verification ecdsa verification can lead to a point at infinity for carefully crafted signature $(r, s)$ with message $z$: $$u_1 \cdot G + u_2 \cdot P = \mathcal{O}$$ where $u_1 = z/s \bmod n$, $u_2 = r/s \bmod n$, and $\mathcal{O}$ is the point at infinity ($n$ is the scalar field modulus of secp256k1). We do not allow the output of ecdsa mul to be point at infinity in the ecdsa circuit, so by design such signatures will not be able to generate a satisfying proof. However, the boolean output of the ecdsa verification must also be false in those cases. The bug is that the output of the ecdsa verification is still `true` even if the ecdsa mul is a point at infinity. The fix is to update the boolean output of the ecdsa verification to ensure the mul result is not a point at infinity. #### Issue 2: NAF Reconstruction $2^{136}$ Overflow In `compute_naf()` function in biggroup, if the input scalar is even (`skew = 1`) and its least-significant $2L$ bits (where $L = 68$ limb size) are all 0 then there is an overflow in reconstructing the lower half of the scalar. For a 256-bit scalar $a$ ($N = 256$) with skew $\mathfrak{s}_a \in \{0, 1\}$ we reconstruct it as $$ \begin{aligned} a &= \underbrace{\sum_{i=0}^{N-2} (1 - a'_{i+1}) \cdot \textcolor{grey}{2^{i}}}_{\textsf{positive part}} - \underbrace{\left(\mathfrak{s}_a + \sum_{i=0}^{N-2} a'_{i+1} \cdot \\textcolor{grey}{2^{i}} \right)}_{\textsf{negative part}}. \\ \end{aligned} $$ where $a'\_1, a'\_2, \dots, a'\_{N-1}$ are inverted bits of $a$ (that is, $a'\_{i+1} = 1 - a'\_{i+1}$). See biggroup README for more details. We reconstruct the positive and the negative parts separately, and each of those parts is a bigfield element (non-native). So while reconstructing the negative part's lower $2L$ bits, we have: $$ \textsf{negative}_{\textsf{lo}} := \mathfrak{s}_a + \sum_{i=0}^{2L-1} a'_{i+1} \cdot \textcolor{grey}{2^i} $$ This overflows when $a'\_{i+1} = 1$ for all $i \in [0, 2L)$. So we need to detect such overflow and then carry it over the higher limb of the negative part. More details on the issues: https://hackmd.io/JZ_uFRZPTvq4rOVcwNuX9A?view
## Summary - Re-land of #19800 with fixes for WASM build and bootstrap.sh field names - Add type-specific JSON structs (VkJson, ProofJson, PublicInputsJson) with named fields per review feedback - Auto-detect JSON vs binary format in verify command - Update bootstrap.sh to use new field names (.vk, .proof, .public_inputs, .hash) - Suppress nlohmann/json sign-conversion warnings for WASM builds using pragma ## Size Impact | Build | Without nlohmann/json | With nlohmann/json | Difference | |-------|----------------------|-------------------|------------| | Native `bb` | 63.18 MB | 63.20 MB | **+19 KB (0.03%)** | | WASM `bb` | 13.49 MB | 13.52 MB | **+30 KB (0.22%)** | ## Test plan - [x] Native build passes - [x] WASM build passes (with `--target bb`) - [x] `regenerate_recursive_inputs` passes - [x] JSON prove and verify works end-to-end
Collaborator
Author
|
🤖 Auto-merge enabled after 4 hours of inactivity. This PR will be merged automatically once all checks pass. |
Collaborator
Author
Flakey Tests🤖 says: This CI run detected 1 tests that failed, but were tolerated due to a .test_patterns.yml entry. |
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
4 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
chore: secp fixes (#19931)
feat: support JSON input files for bb verify command (v2) (#19966)
END_COMMIT_OVERRIDE