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 syscall #17720

Merged
merged 16 commits into from
Jul 7, 2021
Merged

Add ecrecover syscall #17720

merged 16 commits into from
Jul 7, 2021

Conversation

s-medvedev
Copy link
Contributor

Problem

The EVM implementation requires an ecrecover function, but its computation in the BPF code requires too many instructions (6 million).

Summary of Changes

Implement an ecrecover system call.

Fixes #

@s-medvedev
Copy link
Contributor Author

@jackcmay Please review ecrecover system call implementation. The value of ecrecover_base_cost will be specified soon.

@jackcmay
Copy link
Contributor

jackcmay commented Jun 3, 2021

How about program space binding for both C and Rust?

Tests?

@jackcmay
Copy link
Contributor

jackcmay commented Jun 3, 2021

For CI it looks like not all the Cargo.lock file changes were commited

@seanyoung
Copy link
Contributor

I'm not sure it should be called ecrecover, since that means something specific in ethereum. This is a general function for recovering the public key from a secp256k1 signature, which might be useful in other situations than ethereum compatibility. How about something like sol_secp256k1_recover?

@codecov
Copy link

codecov bot commented Jun 7, 2021

Codecov Report

Merging #17720 (63f4607) into master (6fb194f) will decrease coverage by 0.0%.
The diff coverage is 11.5%.

@@            Coverage Diff            @@
##           master   #17720     +/-   ##
=========================================
- Coverage    82.6%    82.6%   -0.1%     
=========================================
  Files         437      438      +1     
  Lines      122557   122640     +83     
=========================================
+ Hits       101309   101326     +17     
- Misses      21248    21314     +66     

sdk/src/feature_set.rs Outdated Show resolved Hide resolved
sdk/bpf/c/inc/solana_sdk.h Outdated Show resolved Hide resolved
sdk/bpf/c/inc/solana_sdk.h Outdated Show resolved Hide resolved
sdk/bpf/c/inc/solana_sdk.h Outdated Show resolved Hide resolved
@mergify mergify bot added the community Community contribution label Jun 10, 2021
@jackcmay
Copy link
Contributor

Looks good, we need to decide on compute cost.

@jackcmay jackcmay marked this pull request as ready for review June 17, 2021 18:11
@jon-chuang
Copy link
Contributor

@s-medvedev are you able to provide benchmarks for the bpf and the native cases? (probably one step of what you need to estimate the compute cost)

Separately, as far as I know libsecp256k1 doesn't use assembly. This could mean it's 1.2-2x slower than an optimised implementation. If this is a concern.

@s-medvedev
Copy link
Contributor Author

@s-medvedev are you able to provide benchmarks for the bpf and the native cases? (probably one step of what you need to estimate the compute cost)

We compare bpf implementation and native implementation via syscall. The both implementation use libsecp256k1 library.
BPF implementation takes 3'000'000 instructions for computations. It takes approximately 0.014237809 seconds on our CI node.

[2021-06-14T07:41:41.929294200Z DEBUG solana_runtime::message_processor] Program HkpHRyDL129CgrhkdvqR36XMu5pQKGt2oCybomsk8CDs invoke [1]
[2021-06-14T07:41:41.943518531Z DEBUG solana_runtime::message_processor] Program log: Total memory occupied: 34800
[2021-06-14T07:41:41.943532009Z DEBUG solana_runtime::message_processor] Program HkpHRyDL129CgrhkdvqR36XMu5pQKGt2oCybomsk8CDs consumed 3266457 of 200000000 compute units

Separately, as far as I know libsecp256k1 doesn't use assembly. This could mean it's 1.2-2x slower than an optimised implementation. If this is a concern.

@jon-chuang Yes, you are right. libsecp256k1 performs this operation very slowly. We compare alternative libraries and get next results:

Library Time (sec)
libsecp256k1 0.0001925
k256 0.0001561
secp256k1 0.00004772

Note: Core Intel i7-7700HQ used for measurements

To determine the cost regardless of computer performance, we decided to compare the time to compute ecrecover with the computation of a Keccak hash of a buffer with a certain length.
Based on this, measurements show that the libsecp256k1 library is comparable to calculating the hash from a buffer with a length of 50,000 bytes, secp256k1 - from a buffer with a length of 13,000 bytes.

You can see benchmark code in neonlabsorg/Solana-EVM#73

@jackcmay jackcmay mentioned this pull request Jun 23, 2021
Comment on lines 29 to 30
Secp256k1RecoverError::InvalidHash => 1,
Secp256k1RecoverError::InvalidRecoveryId => 1,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't match to From<u64> for Secp256k1RecoverError . is it intentional?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not intentional. Fixed.

@ryoqun
Copy link
Member

ryoqun commented Jun 25, 2021

Alternatively, we can introduce another special native program like secp256k1 program so that the operation could be easily pipeline-able?

This assumes the given signature is deterministic at the time of transaction (I think so?) and there needs some way to pass the recovered public key from the ecrecover program to the interested program. secp256k1 program was easy because it just needs pass/fail as outcome...

@jackcmay
Copy link
Contributor

jackcmay commented Jul 6, 2021

Alternatively, we can introduce another special native program like secp256k1 program so that the operation could be easily pipeline-able?

This assumes the given signature is deterministic at the time of transaction (I think so?) and there needs some way to pass the recovered public key from the ecrecover program to the interested program. secp256k1 program was easy because it just needs pass/fail as outcome...

My understanding is that in this use case ecrecover can be called by a Solidty program and that information is unknown at the transaction level. @s-medvedev can you confirm?

@jackcmay
Copy link
Contributor

jackcmay commented Jul 6, 2021

@s-medvedev are you able to provide benchmarks for the bpf and the native cases? (probably one step of what you need to estimate the compute cost)

We compare bpf implementation and native implementation via syscall. The both implementation use libsecp256k1 library.
BPF implementation takes 3'000'000 instructions for computations. It takes approximately 0.014237809 seconds on our CI node.

[2021-06-14T07:41:41.929294200Z DEBUG solana_runtime::message_processor] Program HkpHRyDL129CgrhkdvqR36XMu5pQKGt2oCybomsk8CDs invoke [1]
[2021-06-14T07:41:41.943518531Z DEBUG solana_runtime::message_processor] Program log: Total memory occupied: 34800
[2021-06-14T07:41:41.943532009Z DEBUG solana_runtime::message_processor] Program HkpHRyDL129CgrhkdvqR36XMu5pQKGt2oCybomsk8CDs consumed 3266457 of 200000000 compute units

Separately, as far as I know libsecp256k1 doesn't use assembly. This could mean it's 1.2-2x slower than an optimised implementation. If this is a concern.

@jon-chuang Yes, you are right. libsecp256k1 performs this operation very slowly. We compare alternative libraries and get next results:

Library Time (sec)
libsecp256k1 0.0001925
k256 0.0001561
secp256k1 0.00004772
Note: Core Intel i7-7700HQ used for measurements

To determine the cost regardless of computer performance, we decided to compare the time to compute ecrecover with the computation of a Keccak hash of a buffer with a certain length.
Based on this, measurements show that the libsecp256k1 library is comparable to calculating the hash from a buffer with a length of 50,000 bytes, secp256k1 - from a buffer with a length of 13,000 bytes.

You can see benchmark code in neonlabsorg/Solana-EVM#73

@s-medvedev Does switching from libsecp256k1 to secp256k1 require any api changes that might prevent us from switching the underlying implementation between the two later?

@s-medvedev
Copy link
Contributor Author

@jackcmay Yes that's right. Solidity contracts can use the ecrecover function and pass any information to it. Uniswap contracts use this functionality.

@s-medvedev
Copy link
Contributor Author

@jackcmay These two libraries differ slightly in interface, but nothing critical. The ecrecover system call can be implemented using these libraries without changing the interface.
See: https://github.com/neonlabsorg/ecrecover-bench/blob/1fffb9fad35371270d46d4dbba38393022866a38/src/ecrecover.rs#L165 & https://github.com/neonlabsorg/ecrecover-bench/blob/1fffb9fad35371270d46d4dbba38393022866a38/src/ecrecover.rs#L317

@jackcmay
Copy link
Contributor

jackcmay commented Jul 6, 2021

@s-medvedev Want to resolve the conflicts and get us to all green for merge?

@jackcmay jackcmay added the v1.7 label Jul 6, 2021
@jon-chuang
Copy link
Contributor

jon-chuang commented Jul 7, 2021

@s-medvedev , separately, would you mind sharing with me the bpf secp crate? I'd like to get the wallclock results when bpf_jit is set to true, as for ProgramTest it is in fact set to false by default.

@s-medvedev
Copy link
Contributor Author

@s-medvedev , separately, would you mind sharing with me the bpf secp crate? I'd like to get the wallclock results when bpf_jit is set to true, as for ProgramTest it is in fact set to false by default.

We used the libsecp256k1 library in BPF and got 3,000,000,000 instructions to recover one key. We then ported this as a system call to the Solana kernel level. The measurement results can be viewed above.
We could not use the secp256k1 library as a BPF. When we compile it in our code and try to deploy program into Solana, we get an error:

$ solana program deploy evm_loader/program/target/deploy/evm_loader.so
Error: ELF error: ELF error: Relocation failed, unknown type 1

@jon-chuang
Copy link
Contributor

I see, thanks @s-medvedev . I am guessing it was most likely run with JIT, as without JIT, for an ed25519 scalar mul, I was getting 1s and above.

@jackcmay
Copy link
Contributor

jackcmay commented Jul 7, 2021

@s-medvedev , separately, would you mind sharing with me the bpf secp crate? I'd like to get the wallclock results when bpf_jit is set to true, as for ProgramTest it is in fact set to false by default.

We used the libsecp256k1 library in BPF and got 3,000,000,000 instructions to recover one key. We then ported this as a system call to the Solana kernel level. The measurement results can be viewed above.
We could not use the secp256k1 library as a BPF. When we compile it in our code and try to deploy program into Solana, we get an error:

$ solana program deploy evm_loader/program/target/deploy/evm_loader.so
Error: ELF error: ELF error: Relocation failed, unknown type 1

I suspect this was due to rw data in the crate

@jackcmay jackcmay merged commit 1f288ce into solana-labs:master Jul 7, 2021
mergify bot pushed a commit that referenced this pull request Jul 7, 2021
Co-authored-by: Anton Lisanin <lisanin.anton@gmail.com>
(cherry picked from commit 1f288ce)

# Conflicts:
#	Cargo.lock
#	programs/bpf/Cargo.lock
#	programs/bpf/tests/programs.rs
#	programs/bpf_loader/Cargo.toml
#	programs/bpf_loader/src/syscalls.rs
#	sdk/program/Cargo.toml
jackcmay pushed a commit that referenced this pull request Jul 8, 2021
Co-authored-by: Anton Lisanin <lisanin.anton@gmail.com>
(cherry picked from commit 1f288ce)

# Conflicts:
#	Cargo.lock
#	programs/bpf/Cargo.lock
#	programs/bpf/tests/programs.rs
#	programs/bpf_loader/Cargo.toml
#	programs/bpf_loader/src/syscalls.rs
#	sdk/program/Cargo.toml
mergify bot added a commit that referenced this pull request Jul 8, 2021
* Add ecrecover syscall (#17720)

Co-authored-by: Anton Lisanin <lisanin.anton@gmail.com>
(cherry picked from commit 1f288ce)

# Conflicts:
#	Cargo.lock
#	programs/bpf/Cargo.lock
#	programs/bpf/tests/programs.rs
#	programs/bpf_loader/Cargo.toml
#	programs/bpf_loader/src/syscalls.rs
#	sdk/program/Cargo.toml

* resolve conflicts

Co-authored-by: s-medvedev <40623263+s-medvedev@users.noreply.github.com>
Co-authored-by: Jack May <jack@solana.com>
@brooksprumo brooksprumo mentioned this pull request Aug 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Community contribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants