-
-
Notifications
You must be signed in to change notification settings - Fork 47
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.
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
Add ECRecover
EVM precompile
#504
Conversation
09bf117
to
664d985
Compare
constantine/ecdsa_secp256k1.nim
Outdated
@@ -47,22 +47,55 @@ func signatures_are_equal*(a, b: Signature): bool {.libPrefix: prefix_ffi.} = | |||
proc sign*(sig: var Signature, | |||
secretKey: SecretKey, | |||
message: openArray[byte], | |||
nonceSampler: NonceSampler = nsRandom) {.libPrefix: prefix_ffi, genCharAPI.} = | |||
nonceSampler: NonceSampler = nsRandom, | |||
H: type CryptoHash = sha256) {.libPrefix: prefix_ffi, genCharAPI.} = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exportc procedures cannot have a generic type input.
It's fine for the file to be eth_ecdsa_signatures.nim
and specialize everything for Ethereum.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, right! Yeah, let's go with that. It's trivial to add another at some point in the future after all.
@@ -48,6 +50,8 @@ type | |||
cttEVM_PointNotOnCurve | |||
cttEVM_PointNotInSubgroup | |||
cttEVM_VerificationFailure | |||
cttEVM_InvalidSignature | |||
cttEVM_InvalidV # `v` of signature `(r, s, v)` is invalid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a point in separating InvalidV from InvalidSignature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In practice, I'd say no. Here I was just a bit hasty in mapping the errors from the test vectors to enum elements. Will go with MalformedSignature
as below for both.
return cttEVM_InvalidV | ||
let v = input[63] | ||
if v notin [byte 0, 1, 27, 28]: | ||
return cttEVM_InvalidSignature |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rename MalformedSignature
as InvalidSignature
is ambiguous whether it's:
- a real signature but not for this public key + message
- something that isn't a signature in the first place
var point1 {.noinit.}, point2 {.noinit.}: ECJac | ||
point1.scalarMul(u1, G) # `p₁ = u₁ * G` | ||
point2.scalarMul(u2, R) # `p₂ = u₂ * R` | ||
Q.sum(point1, point2) # `Q = p₁ + p₂` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comes up quite often for ECDSA and should be a dedicated proc, unsure about the name yet dualScalarMul
, simul
, lincomb
.
The multiscalarmul_vartime proc has too much overhead for just 2:
but there are the Strauss or the Yao algorithms to accelerate this, especially given that it's a bottleneck for TLS:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also the scalarMulEndo
routines like this
constantine/constantine/math/elliptic/ec_scalar_mul_vartime.nim
Lines 252 to 331 in 6b65b0e
func scalarMulEndo_wNAF_vartime*[scalBits: static int; EC]( | |
P: var EC, | |
scalar: BigInt[scalBits], | |
window: static int) {.tags:[VarTime, Alloca], meter.} = | |
## Endomorphism-accelerated windowed vartime scalar multiplication | |
## | |
## P <- [k] P | |
## | |
## This uses windowed-NAF (wNAF) | |
## This MUST NOT be used with secret data. | |
## | |
## This is highly VULNERABLE to timing attacks and power analysis attacks | |
# Signed digits divides precomputation table size by 2 | |
# Odd-only divides precomputation table size by another 2 | |
const precompSize = 1 shl (window - 2) | |
static: doAssert window < 8, "Window of size " & $window & " is too large and precomputation would use " & $(precompSize * sizeof(EC)) & " stack space." | |
# 1. Compute endomorphisms | |
const M = when P.F is Fp: 2 | |
elif P.F is Fp2: 4 | |
else: {.error: "Unconfigured".} | |
const G = when EC isnot EC_ShortW_Aff|EC_ShortW_Jac|EC_ShortW_Prj: G1 | |
else: EC.G | |
var endos {.noInit.}: array[M-1, EC] | |
endos.computeEndomorphisms(P) | |
# 2. Decompose scalar into mini-scalars | |
const L = EC.getScalarField().bits().ceilDiv_vartime(M) + 1 | |
var miniScalars {.noInit.}: array[M, BigInt[L]] | |
var negatePoints {.noInit.}: array[M, SecretBool] | |
miniScalars.decomposeEndo(negatePoints, scalar, EC.getScalarField().bits(), EC.getName(), G) | |
# 3. Handle negative mini-scalars | |
if negatePoints[0].bool: | |
P.neg() | |
for m in 1 ..< M: | |
if negatePoints[m].bool: | |
endos[m-1].neg() | |
# 4. EC precomputed table | |
var tabEC {.noinit.}: array[M, array[precompSize, EC]] | |
for m in 0 ..< M: | |
var P2{.noInit.}: EC | |
if m == 0: | |
tabEC[0][0] = P | |
P2.double(P) | |
else: | |
tabEC[m][0] = endos[m-1] | |
P2.double(endos[m-1]) | |
for i in 1 ..< tabEC[m].len: | |
tabEC[m][i].sum_vartime(tabEC[m][i-1], P2) | |
var tab {.noinit.}: array[M, array[precompSize, affine(EC)]] | |
tab.batchAffine(tabEC) | |
# 5. wNAF precomputed tables | |
const NafLen = L+1 | |
var tabNaf {.noinit.}: array[M, array[NafLen, int8]] | |
for m in 0 ..< M: | |
# tabNaf returns NAF from least-significant to most significant bits | |
let miniScalarLen = tabNaf[m].recode_r2l_signed_window_vartime(miniScalars[m], window) | |
# We compute from most significant to least significant | |
# so we pad with 0 | |
for i in miniScalarLen ..< NafLen: | |
tabNaf[m][i] = 0 | |
# 6. Compute | |
var isInit = false | |
for i in 0 ..< NafLen: | |
if isInit: | |
P.double() | |
for m in 0 ..< M: | |
if isInit: | |
P.accumNAF(tab[m], tabNaf[m], NafLen, i) | |
else: | |
isInit = P.initNAF(tab[m], tabNaf[m], NafLen, i) |
actually are implemented in 2 steps:
- splitting a scalar in 2 or 4
- and computing a dual or quadruple scalar multiplications efficiently, including in constant-time.
So it should be possible to isolate those internals in a separate PR and redirect the publicly exposed dualScalarMul
to use these and minimize extra code that needs to be written.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue for it here: #507
Rebased onto the ECDSA PR branch & made the ECDSA file for secp256k1 specific to Ethereum now. The CI should pass now (aside from the maybe failing Windows test case like in the other where it gets terminated at some point?). edit: or maybe not, yet. 😆 |
CI is failing also on Linux, because of an outdated OpenSSL version on Ubuntu 22.04 ships OpenSSL version 3.0.2: while support for Keccak256 was only added in version 3.2 according to this issue comment in July 2024: "Funnily" enough, Keccak is not mentioned anywhere in the actual changelog: According to the OpenSSL docs: version 3.2 is indeed the first to support it (changing to version 3.1 in the docs links leads to a page not found). NOTE: Even Ubuntu 24.04 ships with OpenSSL 3.0.13. Edit: Ubuntu 24.10 is the first version to ship with 3.3. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, the PR needs a rebase
constantine.nimble
Outdated
@@ -607,6 +607,9 @@ const testDesc: seq[tuple[path: string, useGMP: bool]] = @[ | |||
("tests/t_ethereum_verkle_primitives.nim", false), | |||
("tests/t_ethereum_verkle_ipa_primitives.nim", false), | |||
|
|||
# Signatures | |||
("tests/ecdsa/t_ecdsa_verify_openssl.nim", false), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be commented out until Ubuntu 25.04 is available in CI (probably June)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the non LTS releases ever part of the Github Actions CI images?
Done so that future public key recovery can simply call `verifyImpl` to verify public key is found (signImpl changed to match).
ECDSA over secp256k1 commonly uses both SHA256 (e.g. Bitcoin) and Keccak256 (e.g. Ethereum). Other combinations may also exist. We default to SHA256 for the time being.
ECRecover provides the message hash and not the message. We need an API to pass that directly to the internal ECDSA procedure. We export the impl `vartime` routine for that purpose. We could alternatively also import that file using `{.all.}`.
We extend the CttEVMStatus enum by two further elements. One for an invalid signature in ECRecover and another for an invalid `v` value.
Co-authored-by: Mamy Ratsimbazafy <[email protected]>
Co-authored-by: Mamy Ratsimbazafy <[email protected]>
Co-authored-by: Mamy Ratsimbazafy <[email protected]>
Given that we generate a C API from the code, we need to differentiate the function names for the types. The default takes a message and this variant takes a digest (as used in Ethereum's precompile for ECRecover).
Rebased, force pushed and taken out the ECDSA test for the time being. |
This builds on top of the ECDSA implementation in PR #490 and adds public key recovery. Based on that implementing the
ECRecover
precompile is quite straightforward (recover an Ethereum address based on a given message digest and signature).We base our implementation of the precompile on Geth's implementation here:
https://github.com/ethereum/go-ethereum/blob/341647f1865dab437a690dc1424ba71495de2dd8/core/vm/contracts.go#L243-L272
and to a lesser extent appendix F in the Ethereum Yellow Paper.
For the general ECDSA public key recovery API I went with a variable time implementation. To my understanding this is essentially needed regardless, due to the fact that the signing process converts the coordinate of the point
R
inFp
to a scalar inFr
. While unlikely for Secp256k1 (because the subgroup order and the curve prime are both 256 bit integers), if the x coordinate is larger than the subgroup order, we reduce it. This reduction needs to be reversed in the recovery process requiring a loop.In addition we have an
evenY
boolean argument, which decides if we recover the public key associated with theR
coordinate with an eveny
coordinate or an odd one (ify
is even then-y
is odd in a finite field of odd size).(I'll fix the CI failure in the other PR and will rebase this one on top of the other once that is done)