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

(WIP) Add pre-compile: verification for ed25519 sig made via GPG #2

Open
wants to merge 11 commits into
base: optimism
Choose a base branch
from

Conversation

nkrishang
Copy link

@nkrishang nkrishang commented Dec 25, 2024

A pre-compile contract at 0xed that uses gopengpg to verify signatures produced on signing a bytes message with an ed25519 key via GPG.

Usage

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

contract ED25519Verifier {
    
    function verify(bytes memory message, bytes memory publicKey, bytes memory signature) external view returns (bool) {
        // Pack the input in the format expected by the precompile:
        // message_len (32 bytes) || message || pubkey_len (32 bytes) || pubkey || sig_len (32 bytes) || signature
        bytes memory input = abi.encodePacked(
            bytes32(message.length),
            message,
            bytes32(publicKey.length),
            publicKey,
            bytes32(signature.length),
            signature
        );
        
        (bool success, bytes memory result) = address(0xed).staticcall(input);
        require(success, "ED25519 verification failed");
        
        return result.length > 0 && result[0] == 1;
    }
}

Copy link
Collaborator

@zobront zobront left a comment

Choose a reason for hiding this comment

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

This looks great. Definitely seems clear that this is the better path than using EIP665 and implementing the GPG stuff in Solidity.

I left a few comments on areas that will require some tweaks.

Before testing in Foundry, I would recommend adding a test to contracts_test.go. You can hardcode 1 correct signature and one incorrect signature into a JSON file, and should be able to plug that in to verify the signature verification is working as expected.

@@ -170,6 +170,7 @@ const (
Bls12381MapG2Gas uint64 = 75000 // Gas price for BLS12-381 mapping field element to G2 operation

P256VerifyGas uint64 = 3450 // secp256r1 elliptic curve signature verifier gas price
GpgEd25519VerifyGas uint64 = 2000 // GPG Ed25519 signature verification gas price
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should do some benchmarking to make sure this price is still appropriate, given the extra computation.

Copy link
Collaborator

@zobront zobront Dec 28, 2024

Choose a reason for hiding this comment

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

Plan for this:

  • run other precompile benchmarks to get MGas/s on local machine
  • run our precompile to get a sense of runs/s with varying inputs
  • create pricing formula that leads to MGas/s equal or greater than compared precompiles for all inputs
  • if the resulting gas price is very low, round up to be safe

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd also like to get a sense of whether there are codepaths in the Go libs that are very time intensive that could be abused as a DOS vector. We can poke around a bit, but also will leave a note for auditors to explore this.

core/vm/contracts.go Outdated Show resolved Hide resolved
core/vm/contracts.go Outdated Show resolved Hide resolved
if len(input) < int(offset+32+pubKeyLen) {
return nil, errPubKeyTooShort
}
pubKey := input[offset+32 : offset+32+pubKeyLen]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a specific length the pubKey is supposed to be that we should verify?

Copy link
Author

Choose a reason for hiding this comment

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

Similar to the comment on signature length. The armored public key length is non-deterministic (exported via gpg --export --armor <your-key-id>).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed, no way to do this. I'm going to leave this comment open as a reminder that I need to talk to the Tea team to figure out a reasonable upper bound to make sure that (a) there is sufficient gas to handle it, and (b) that there is no DOS risk based on whatever gas params we choose.

@nkrishang
Copy link
Author

I've added tests and benchmarks in core/vm/contracts_test.go. Please see the comments on signature length and public key length. An open question I'll mention is whether to create a max message size limit.

Overall, enshrining GPG specific code in a precompile like this means using it with public keys and signatures in the GPG specific armored format, instead of raw 32 bytes public keys and 64 bytes signatures. So far, I haven't landed on a solution to go back and forth from armored format (pKey or signature) to hex and back.

core/vm/contracts.go Outdated Show resolved Hide resolved
@@ -170,7 +170,7 @@ const (
Bls12381MapG2Gas uint64 = 75000 // Gas price for BLS12-381 mapping field element to G2 operation

P256VerifyGas uint64 = 3450 // secp256r1 elliptic curve signature verifier gas price
GpgEd25519VerifyGas uint64 = 2000 // GPG Ed25519 signature verification gas price
GpgEd25519VerifyGas uint64 = 7900 // GPG Ed25519 signature verification gas price
Copy link
Author

Choose a reason for hiding this comment

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

Assuming Ecrecover is fairly priced (as per EIP-152 Blake2 precompile) at 3000 gas/op for ~50,100 ns/op.

GpgEd25519Verify should be priced proportionately 7900 gas/op for ~134,000 ns/op as a good first estimate for gas cost.

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