Optimize EOA signature verification

[k06a]

Optimize EOA signature verification. Since Berlin HF isContract() would cost 2000 gas.

Prev gas:


EOA: 2000 + 3500

SC: 2000



Now gas:


EOA: 3500

SC: (signature.length is 64 or 65 ? 3500 : 0) + 2000

[Amxx]

Hello @k06a,

First of all, thank you for this PR, we noticed it a while back and failed to let you know. For that I am sorry.
The idea itself is sound, it would result in interesting saving in the EOA case, which is the one we expect to happen the most often. However this PR breaks one thing:

• ERC1271 doesn't assume anything about the wallet's signature verification primitive, might be secp256k1 ecrecover like for EOAs, but it might be something different, particularly in the future.
• Some wallets may used another crypto primitive that also uses 64 or 65 bytes long signatures.
• If these signature return an invalid value for ecrecover, and we fallback to 1271, that is good, but if signature doesn't follow the right format, our ecrecover (which include some checks) could revert.

Unfortunatelly, we have no way to catch an internal library call :/

We will investigate other options (like having a non reverting ecrecover that returns an "error" enum + the recovered address).

What do you think ?

[Amxx]

[duplicate]

[Amxx]

We should ensure that, if signer is an ERC1271 implementing contract, the initial ecrecover doesn't revert and the contract call is performed.

[k06a]

@Amxx WDYT about ECDSA-tryRecover introduced in the latest commit?

[k06a]

@Amxx nice changes! I believe this tryRecover will be very useful for developers.

[frangio]

The problem I see with this try pattern is that I don't know how we'll migrate it from revert strings to custom errors, once we do that in a couple of months ideally and minimizing breaking changes.

As far as I know custom errors cannot be passed around as values. Perhaps this is something we should raise with the Solidity team.

[k06a]

@frangio possibly we could return ABI-encoded version (bytes) of error after migration to custom errors.

[Amxx]

@frangio
Maybe we can

• replace revert strings with an enum, making the return type of tryRecover address, uint8.
• in the recover functions, replace the bytes(error).length > 0 with error != 0
• have a error enum → string private function that we call

(address recovered, RecoveryError error) = tryRecover(hash, v, r, s)
if (error != RecoveryError.NoError) {
    revert(_recoveryErrorString(error));
}

In the futur we can remove the _recoveryErrorString private function in favor of custom errors.

[frangio]

LGTM! Thanks everyone.

[matias904]

Good