Skip to content
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

Merged
merged 16 commits into from
Jan 13, 2025
Merged

Add ECRecover EVM precompile #504

merged 16 commits into from
Jan 13, 2025

Conversation

Vindaar
Copy link
Collaborator

@Vindaar Vindaar commented Dec 30, 2024

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 in Fp to a scalar in Fr. 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 the R coordinate with an even y coordinate or an odd one (if y 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)

@@ -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.} =
Copy link
Owner

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.

Copy link
Collaborator Author

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
Copy link
Owner

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?

Copy link
Collaborator Author

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
Copy link
Owner

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

constantine/signatures/ecdsa.nim Outdated Show resolved Hide resolved
constantine/signatures/ecdsa.nim Outdated Show resolved Hide resolved
constantine/signatures/ecdsa.nim Outdated Show resolved Hide resolved
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₂`
Copy link
Owner

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:
image
but there are the Strauss or the Yao algorithms to accelerate this, especially given that it's a bottleneck for TLS:

Copy link
Owner

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

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.

Copy link
Collaborator Author

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

@Vindaar
Copy link
Collaborator Author

Vindaar commented Jan 5, 2025

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. 😆

@Vindaar
Copy link
Collaborator Author

Vindaar commented Jan 5, 2025

CI is failing also on Linux, because of an outdated OpenSSL version on ubuntu-latest (= Ubuntu 22.04).

Ubuntu 22.04 ships OpenSSL version 3.0.2:
https://packages.ubuntu.com/jammy/openssl

while support for Keccak256 was only added in version 3.2 according to this issue comment in July 2024:
openssl/openssl#19304 (comment)

"Funnily" enough, Keccak is not mentioned anywhere in the actual changelog:
https://github.com/openssl/openssl/blob/openssl-3.2/CHANGES.md

According to the OpenSSL docs:
https://docs.openssl.org/3.2/man7/EVP_MD-KECCAK/

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.

Base automatically changed from ecdsaSecp256k1 to master January 5, 2025 14:08
mratsim
mratsim previously approved these changes Jan 7, 2025
Copy link
Owner

@mratsim mratsim left a 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

@@ -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),
Copy link
Owner

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)

Copy link
Collaborator Author

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?

Vindaar and others added 16 commits January 12, 2025 15:14
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.
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).
@Vindaar
Copy link
Collaborator Author

Vindaar commented Jan 12, 2025

Rebased, force pushed and taken out the ECDSA test for the time being.

@mratsim mratsim merged commit 9dd1205 into master Jan 13, 2025
12 checks passed
@mratsim mratsim deleted the ecRecover branch January 13, 2025 21:16
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.

2 participants