-
Notifications
You must be signed in to change notification settings - Fork 995
Improve EcRecover precompile performance #9053
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
Improve EcRecover precompile performance #9053
Conversation
…k1JNI Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
- Use LibSecp256k1JNI by default and fallback to SECP256K1 signature algorithm which uses LibSecp256k1 native lib (as well as Java fallback). - Add --Xecrecover-precompile-native-enabled to disable LibSecp256k1JNI. - ECRecoverBenchmark switches between LibSecp256k1JNI and Java. Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
ethereum/evmtool/src/main/java/org/hyperledger/besu/evmtool/benchmarks/ECRecoverBenchmark.java
Outdated
Show resolved
Hide resolved
garyschulte
left a comment
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.
Few comments - nothing major. It would be good to mark the prior native path with deprecation IMO. There isn't a need to keep that code path around once the single-entrypoint-ecrecover is proven.
ethereum/evmtool/src/main/java/org/hyperledger/besu/evmtool/benchmarks/ECRecoverBenchmark.java
Outdated
Show resolved
Hide resolved
| logger.info("Using the native implementation of ecrecover precompile"); | ||
| } else { | ||
| ECRECPrecompiledContract.disableNative(); | ||
| if (SignatureAlgorithmFactory.getInstance().isNative()) { |
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 is a little confusing because disableNative should force isNative to be false AFAICT? is this if statement necessary in that case?
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.
Yeh there's two layers of disabling because SECP256K1 is used in both ecrecover and in transactions.
I agree it's a little odd, but with the benefit of supporting a fallback native option if the new one isn't supported. Maybe doesn't make sense if it's the same native lib backend?
The alternative is to never attempt to use LibSecp256K1 if we wish to disable LibSecp256K1JNI...but I think I'd have to disable that only in the precompile and leave it enabled for the other use cases.
Note, there already exists --Xsecp-native-enabled.
A simpler approach would be to only disable the JNI wrapper if this is set to false, so for K1 ecrecover it's only ever new JNI lib or Java (and remove the new option).
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.
maybe a comment here could clarify for future-us why we are double-disabling.
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.
Removed double layer disabling and now driving LibSecp256K1JNI enable/disable via SignatureAlgorithm.isNative
ethereum/evmtool/src/main/java/org/hyperledger/besu/evmtool/benchmarks/P256VerifyBenchmark.java
Outdated
Show resolved
Hide resolved
| return useNative; | ||
| } | ||
|
|
||
| /** Disable native. Note SECP256K1 must additionally be disabled to fully disable native */ |
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.
If we are on an r1 chain, does the same apply?
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.
Might need to chat to you about this.
Is the current expectation for an "r1 chain" that you would use R1 for the precompile as well? Doesn't seem obvious either way to me. If you're transactions are R1, maybe you still want access to a K1 precompile to support various contracts?
IOW, the spec for this precompile is K1, would we want a different precompile for R1?
Can discuss more in the context of the upcoming R1 variant of this PR, but for now I guess it might be safer to only enable the K1JNI lib if the signature algorithm is set to K1?
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.
yeah, ecrecover on a secp256r1 chain will need to use an r1-friendly ecrecover
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.
Agree for ecrecover outside the scope of the precompile.
but for the precompile, that would break any public network-based contracts that use k1 precompile though?
Feels like we might need a separate R1 ecrecover precompile?
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.
Perhaps we can subclass and configure or compose ecrecover precompile for both r1 and k1. I am keen to avoid any complexity with precompile registry per-hardfork specs. For example, ideally "london-is-london" and we don't have to define a separate protocol schedule with different precompile config for an r1 chain.
It might even be worth pushing the optimized signature recovery into SignatureAlgorithm in the same way it was done previously for the native implementation. In essence we leave ECRECPrecompiledContract as-is and choose the native implementation (or not) in SignatureAlgorithm
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.
@daniellehrner - can you shed light on the original requirements for ecrecover for r1 chains?
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.
Have isolated these changes to K1 so this PR does not impact R1 chains.
I have not pushed the impl to SignatureAlgorithm as suggested. I attempted it, but you still end up with the same amount of complexity in ECRECPrecompiledContract due to different interface requirements between the precompile and SignatureAlgorithm.
We can revisit when writing the upcoming R1 EcRecover PR, however for it to be worth the complexity, I think we need to redesign the SigAlg ecrecover interface across the board (not just for the precompile).
evm/src/main/java/org/hyperledger/besu/evm/precompile/ECRECPrecompiledContract.java
Show resolved
Hide resolved
- remove TODOs - Handle exception Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
Remove --Xecrecover-precompile-native-enabled Can now only disable for testing purposes Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
…improvements Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
…improvements Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
| final Bytes input, @NotNull final MessageFrame messageFrame) { | ||
| final int size = input.size(); | ||
| final Bytes d = size >= 128 ? input : Bytes.wrap(input, MutableBytes.create(128 - size)); | ||
| final Bytes32 h = Bytes32.wrap(d, 0); |
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 second wrap to Bytes32 was just not needed?
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.
It's moved both inside computeK1Native and computeDefault, also renamed from h -> messageHash
I could have calculated outside and passed it in but thought the unary method signature of computeX(safeInput) was clearer than computeX(safeInput, messageHash)
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.
ok that makes sense
|
|
||
| // SECP256K1#PublicKey#recoverFromSignature throws an Illegal argument exception | ||
| // when it is unable to recover the key. There is not a straightforward way to | ||
| // check the arguments ahead of time to determine if the fail will happen and |
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.
| // check the arguments ahead of time to determine if the fail will happen and | |
| // check the arguments ahead of time to determine whether the fail will happen. |
macfarla
left a comment
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.
think this looks ok to me if it has synced perf-devnet-1
…improvements Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
c4c01ee to
03f698d
Compare
…improvements Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
|
Testing update: SNAP synced perf-devnet-1 and FULL sync hoodi. |
- Use LibSecp256k1JNI by default and fallback to SECP256K1 signature algorithm which uses LibSecp256k1 native lib as well as Java fallback. In reality, the backend lib (bitcoin-core-secp256k1) is the same so should always use LibSecp256k1JNI if either available. - ECRECPrecompiledContractTest and ECRecoverBenchmark only switches between LibSecp256k1JNI and Java (LibSecp256k1 is effectively ignored for this precompile in reality). - R1 chains will use SECP256R1 SignatureAlgorithm (native/java) as before. - If --Xsecp-native-enabled=true (default) and LibSecp256k1JNI is available then use that. - If --Xsecp-native-enabled=false or native lib not available, then use Java. --------- Signed-off-by: Simon Dudley <simon.dudley@consensys.net> Co-authored-by: garyschulte <garyschulte@gmail.com>
- Use LibSecp256k1JNI by default and fallback to SECP256K1 signature algorithm which uses LibSecp256k1 native lib as well as Java fallback. In reality, the backend lib (bitcoin-core-secp256k1) is the same so should always use LibSecp256k1JNI if either available. - ECRECPrecompiledContractTest and ECRecoverBenchmark only switches between LibSecp256k1JNI and Java (LibSecp256k1 is effectively ignored for this precompile in reality). - R1 chains will use SECP256R1 SignatureAlgorithm (native/java) as before. - If --Xsecp-native-enabled=true (default) and LibSecp256k1JNI is available then use that. - If --Xsecp-native-enabled=false or native lib not available, then use Java. --------- Signed-off-by: Simon Dudley <simon.dudley@consensys.net> Co-authored-by: garyschulte <garyschulte@gmail.com> Signed-off-by: jflo <justin+github@florentine.us>
- Use LibSecp256k1JNI by default and fallback to SECP256K1 signature algorithm which uses LibSecp256k1 native lib as well as Java fallback. In reality, the backend lib (bitcoin-core-secp256k1) is the same so should always use LibSecp256k1JNI if either available. - ECRECPrecompiledContractTest and ECRecoverBenchmark only switches between LibSecp256k1JNI and Java (LibSecp256k1 is effectively ignored for this precompile in reality). - R1 chains will use SECP256R1 SignatureAlgorithm (native/java) as before. - If --Xsecp-native-enabled=true (default) and LibSecp256k1JNI is available then use that. - If --Xsecp-native-enabled=false or native lib not available, then use Java. --------- Signed-off-by: Simon Dudley <simon.dudley@consensys.net> Co-authored-by: garyschulte <garyschulte@gmail.com> Signed-off-by: georgereuben <reubengeorge101@gmail.com>
- Use LibSecp256k1JNI by default and fallback to SECP256K1 signature algorithm which uses LibSecp256k1 native lib as well as Java fallback. In reality, the backend lib (bitcoin-core-secp256k1) is the same so should always use LibSecp256k1JNI if either available. - ECRECPrecompiledContractTest and ECRecoverBenchmark only switches between LibSecp256k1JNI and Java (LibSecp256k1 is effectively ignored for this precompile in reality). - R1 chains will use SECP256R1 SignatureAlgorithm (native/java) as before. - If --Xsecp-native-enabled=true (default) and LibSecp256k1JNI is available then use that. - If --Xsecp-native-enabled=false or native lib not available, then use Java. --------- Signed-off-by: Simon Dudley <simon.dudley@consensys.net> Co-authored-by: garyschulte <garyschulte@gmail.com> Signed-off-by: georgereuben <reubengeorge101@gmail.com>
In draft awaiting testing.
- Add --Xecrecover-precompile-native-enabled to disable LibSecp256k1JNI.If
--Xsecp-native-enabled=true(default) and LibSecp256k1JNI is available then use that.If
--Xsecp-native-enabled=falseor native lib not available, then use Java.Removed `--Xecrecover-precompile-native-enabled` in second iteration following review. Expand for out-of-date details
Flag combinations showing interaction between LibSecp256k1JNI used for precompile-only versus SECP256K1 used for precompile fallback as well as other key operations apart from EcRecover. The default for both flags is `true`:
default:
JNI lib used for ecrecover, but Java used for other secp operations:
Disable JNI lib, fallback to SECP256K1 for ecrecover:
Disable both, use Java for ecrecover:
Testing
fullsnap sync of perf-devnet-1