Cache P-256 curve parameters per thread in p256_verify_impl - #2525
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR optimizes the EIP-7951 p256_verify precompile implementation by reducing per-call Crypto++ curve setup overhead and slightly reordering validation to fail faster on the infinity-key sentinel.
Changes:
- Cache
DL_GroupParameters_EC<ECP>as a function-localthread_localinp256_verify_implto avoid reconstructing P-256 parameters on every call. - Reorder public key checks to reject
(qx, qy) == (0, 0)before invokingec.VerifyPoint(...).
Verdict: CORRECT
🤖 Generated with Claude Code
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
There was a problem hiding this comment.
Reviewed per REVIEW.md. No findings.
- The
thread_local(rather thanstatic) cache is correct: Crypto++'sECPwrites through amutable Point m_Rscratch member in its constAdd/Doublemethods, so a single shared instance across execution threads would be a data race — the code comment documents this accurately. Per-thread caching eliminates the per-call ASN.1 decode of the secp256r1 parameters without changing behavior. - No dangling references into the now-persistent object:
MultiplyreturnsPointby value, andauto const r_prime = ec.Add(p1, p2)deducesPointand copies out of the scratch before the next use. - A block-scope
thread_localin this header-definedinlinefunction is ODR-safe (function-local statics in inline functions are unique across TUs), and the function has no suspension points, so the thread-bound references cannot outlive their thread even under fiber scheduling. - Reordering the (0, 0) infinity check ahead of
VerifyPointis behavior-preserving: P-256 has b ≠ 0, so (0, 0) fails the curve equation, meaning both orders reject the identical input set — and the diff comment explains the divergence from the EIP-7951 pseudocode order. - Existing coverage applies: the
p256Verify.jsonvector suite (precompiles_test.cpp,TYPED_TEST(TraitsTest, p256_verify)) exercises both rejection paths across trait instantiations. - PR hygiene: single commit, single file, scope is exactly what the title says.
Verdict: CORRECT
🤖 Generated with Claude Code
khordadi
force-pushed
the
amir/p256-curve-params
branch
from
August 28, 2026 15:03
344960d to
59861a0
Compare
Baltoli
previously approved these changes
Aug 28, 2026
Baltoli
requested review from
dhil,
goodlyrottenapple,
mkolosick and
ryankeleti
August 28, 2026 15:08
ryankeleti
previously approved these changes
Aug 28, 2026
goodlyrottenapple
previously approved these changes
Aug 28, 2026
mkolosick
requested changes
Aug 28, 2026
khordadi
dismissed stale reviews from goodlyrottenapple, ryankeleti, and Baltoli
via
August 28, 2026 15:31
87834e2
khordadi
force-pushed
the
amir/p256-curve-params
branch
from
August 28, 2026 15:31
59861a0 to
87834e2
Compare
mkolosick
approved these changes
Aug 28, 2026
ryankeleti
approved these changes
Aug 28, 2026
DL_GroupParameters_EC<ECP> construction decodes the secp256r1 domain parameters (OID lookup, hex-decoding the curve constants, Montgomery conversion) on every call to p256_verify_impl, costing ~11.6us per invocation. Cache the parameters in a thread_local instead. thread_local rather than a shared static because Crypto++'s ECP writes mutable scratch state under Add and the small-scalar Multiply fallback, execution is fiber-parallel across OS threads, and the small-scalar path is reachable from attacker-controlled r and s; a single shared object would be a data race. This matches the thread_local secp256k1_context already used by ecrecover_impl. Also check for the (0, 0) infinity public key before evaluating the curve equation: both orders reject identically ((0, 0) is not on the curve since b != 0 for P-256), and doing the cheap check first skips the more expensive point validation. Measured with all four variants compiled into one binary over the 782 geth/wycheproof vectors (identical verdicts on every vector): full valid verify 695.3 -> 684.2us (-1.6%), early-reject inputs 14.3 -> 2.7us (5.3x), infinity inputs 3.25 -> 3.04us from the reorder. No consensus-visible behavior change. Raised by @guidovranken in #1646. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
khordadi
force-pushed
the
amir/p256-curve-params
branch
from
August 28, 2026 20:29
87834e2 to
ccc4c64
Compare
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
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.
Fixes #1646.
DL_GroupParameters_EC<ECP>was constructed on every call; it now lives in a function-localthread_local. Not a shared global: Crypto++'sECPwrites mutable scratch state underAdd/Multiply, so one object shared across execution threads would be a data race. This matches thethread_local secp256k1_contextpattern inecrecover_impl.The issue's remaining suggestion (
constexpr empty_result) was already resolved by the zkvm precompiles refactor (1ce42ff).No consensus-visible behavior change; all 19
p256_verifytest instantiations pass (782 geth/wycheproof vectors each). The zkvm build shadows this header and is unaffected.🤖 Generated with Claude Code