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

feat: add frost-secp256k1-evm crate #749

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

StackOverflowExcept1on
Copy link
Contributor

@StackOverflowExcept1on StackOverflowExcept1on commented Oct 8, 2024

Resolves #715
Note: This is testing branch, where I've simply renamed frost-secp256k1 to frost-secp256k1-evm for code review purposes.

TODO:

  • check_sign_with_test_vectors
  • check_sign_with_test_vectors_dkg
  • check_sign_with_test_vectors_with_big_identifiers

Copy link

codecov bot commented Oct 9, 2024

Codecov Report

Attention: Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.

Project coverage is 81.79%. Comparing base (5f4ac6e) to head (d8511fd).
Report is 60 commits behind head on main.

Files with missing lines Patch % Lines
frost-secp256k1-evm/src/keys/repairable.rs 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #749      +/-   ##
==========================================
- Coverage   82.18%   81.79%   -0.39%     
==========================================
  Files          31       34       +3     
  Lines        3188     3345     +157     
==========================================
+ Hits         2620     2736     +116     
- Misses        568      609      +41     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@StackOverflowExcept1on
Copy link
Contributor Author

@conradoplg could you please take a look and enable CI? after the review I will make frost-secp256k1-evm new directory and leave frost-secp256k1 untouched, and update the documentation everywhere.

@StackOverflowExcept1on StackOverflowExcept1on marked this pull request as ready for review October 9, 2024 16:52
@StackOverflowExcept1on
Copy link
Contributor Author

StackOverflowExcept1on commented Oct 9, 2024

For some reason CI can't deploy FROST book, but it builds successfully. Ping me when the review is ready and I'll add these commits to the PR.

@conradoplg
Copy link
Contributor

This is looking good. Don't worry about the Docs deployment job.

Would it be possible to add an interoperability test that shows that signatures produced can be verified by another library? Like we do for Ed25519.

@StackOverflowExcept1on
Copy link
Contributor Author

I see that helpers::verify_signature uses ed25519_dalek::Verifier. In that case, what should I do with k256 instead of ed25519_dalek? k256::schnorr supports Schnorr signatures, but this is BIP340 standard.

@conradoplg
Copy link
Contributor

I see that helpers::verify_signature uses ed25519_dalek::Verifier. In that case, what should I do with k256 instead of ed25519_dalek? k256::schnorr supports Schnorr signatures, but this is BIP340 standard.

I'm a bit confused, I'm not very familiar with the Ethereum ecosystem, sorry. If this is supposed to be BIP340, doesn't it need to be based on #730? Signatures produced by the frost-secp256k1-evm crate are supposed to be used where, exactly? Aren't there any other Rust library that produces (regular, non-FROST) equivalent signatures?

@conradoplg
Copy link
Contributor

Also for reference, see the Taproot interoperability test verify_signature(): https://github.com/ZcashFoundation/frost/pull/730/files#diff-7d8692f2e9ebda6cd9d035739032ea49be2b156ac782a7664e9d89a05f21132f

@StackOverflowExcept1on
Copy link
Contributor Author

StackOverflowExcept1on commented Oct 30, 2024

@conradoplg

If this is supposed to be BIP340, doesn't it need to be based on #730?

I don't want to use BIP340 and the current version of frost-secp256k1 with sha256 because there is no sha256 instruction in EVM. This means that to calculate sha256 need to make staticcall to address 0x02, and this requires memory preparation etc. More details here: https://evm.codes/precompiled#0x02. However, there is keccak256 instruction in EVM: https://evm.codes/#20.

Signatures produced by the frost-secp256k1-evm crate are supposed to be used where, exactly?

I wrote library in Solidity that can verify frost-secp256k1-evm signatures very cheaply. For only 10609 gas! While transferring ETH between addresses costs 21000 gas. Here is an example contract: https://github.com/StackOverflowExcept1on/frost-secp256k1-evm/blob/01f30739ad8a363a1bcb61afccaafbd222f0c501/src/Counter.sol#L22. So this library allows to write smart contract for multi-signature wallet, use FROST signatures for oracles and much more. The Counter contract consumes 31609 gas, which is $0.71 on Ethereum right now.

Aren't there any other Rust library that produces (regular, non-FROST) equivalent signatures?

I don't know of any such libraries or I'll have to write the signature check for regular Schnorr signatures and the challenge calculation from scratch using k256.

@StackOverflowExcept1on
Copy link
Contributor Author

I would also like to know how necessary hash_to_scalar is? It is quite expensive in Solidity. I managed to verify "FROST-secp256k1-KECCAK256-v1" for 10609 gas (4200 gas was spent on loading public key from storage, 2112 gas was spent on transaction data, 3000 gas on ecrecover and the remaining 1297 gas is for preparing memory for ecrecover and hash_to_scalar).

After some reverse engineering I was able to port hash_to_scalar to Solidity. Right now it allocates about 280 bytes of memory just to calculate the challenge, then does keccak256 twice, xor, addmod, mulmod...

function hashToScalar(bytes memory domain, bytes memory message) public pure returns (uint256) {
    bytes32 b0 = keccak256(
        abi.encodePacked(
            //136 bytes for keccak256 block size
            uint256(0),
            uint256(0),
            uint256(0),
            uint256(0),
            uint64(0),
            //actual message
            message,
            //len_in_bytes
            uint16(48),
            //reserved 0
            uint8(0),
            //domain
            domain,
            uint8(domain.length)
        )
    );
    bytes32 b_vals = keccak256(abi.encodePacked(b0, uint8(1), domain, uint8(domain.length)));
    bytes32 tmp = b0 ^ b_vals;
    bytes32 b_vals2 = keccak256(abi.encodePacked(tmp, uint8(2), domain, uint8(domain.length)));

    uint256 F_2_192_ = 0x0000000000000001000000000000000000000000000000000000000000000000;

    uint256 d0 = uint256(b_vals >> 64);
    uint256 d1 = uint256(((b_vals & bytes32(uint256(0xffffffffffffffff))) << 128) | (b_vals2 >> 128));

    return addmod(mulmod(d0, F_2_192_, Secp256k1.N), d1, Secp256k1.N);
}

uint256 challenge = hashToScalar(
    "FROST-secp256k1-KECCAK256-v1chal",
    abi.encodePacked(
        uint8(signatureRYCompressed), signatureRX, uint8(publicKeyYCompressed), publicKeyX, uint256(messageHash)
    )
);

@conradoplg
Copy link
Contributor

I would also like to know how necessary hash_to_scalar is? It is quite expensive in Solidity. I managed to verify "FROST-secp256k1-KECCAK256-v1" for 10609 gas (4200 gas was spent on loading public key from storage, 2112 gas was spent on transaction data, 3000 gas on ecrecover and the remaining 1297 gas is for preparing memory for ecrecover and hash_to_scalar).

After some reverse engineering I was able to port hash_to_scalar to Solidity. Right now it allocates about 280 bytes of memory just to calculate the challenge, then does keccak256 twice, xor, addmod, mulmod...

function hashToScalar(bytes memory domain, bytes memory message) public pure returns (uint256) {
    bytes32 b0 = keccak256(
        abi.encodePacked(
            //136 bytes for keccak256 block size
            uint256(0),
            uint256(0),
            uint256(0),
            uint256(0),
            uint64(0),
            //actual message
            message,
            //len_in_bytes
            uint16(48),
            //reserved 0
            uint8(0),
            //domain
            domain,
            uint8(domain.length)
        )
    );
    bytes32 b_vals = keccak256(abi.encodePacked(b0, uint8(1), domain, uint8(domain.length)));
    bytes32 tmp = b0 ^ b_vals;
    bytes32 b_vals2 = keccak256(abi.encodePacked(tmp, uint8(2), domain, uint8(domain.length)));

    uint256 F_2_192_ = 0x0000000000000001000000000000000000000000000000000000000000000000;

    uint256 d0 = uint256(b_vals >> 64);
    uint256 d1 = uint256(((b_vals & bytes32(uint256(0xffffffffffffffff))) << 128) | (b_vals2 >> 128));

    return addmod(mulmod(d0, F_2_192_, Secp256k1.N), d1, Secp256k1.N);
}

uint256 challenge = hashToScalar(
    "FROST-secp256k1-KECCAK256-v1chal",
    abi.encodePacked(
        uint8(signatureRYCompressed), signatureRX, uint8(publicKeyYCompressed), publicKeyX, uint256(messageHash)
    )
);

You could use any hash-to-scalar method as long it's secure. There was no particular reason for us using that one for the secp256k1, I think the rationale was to simply use something that was already specified in a RFC.

Thinking about this, I think this ciphersuite would be more suited to live in a separate crate maintained by you in a separate repository. You can simply import frost-core (and even enable the internals feature if needed). My rationale is that this seems a much more specific use case (compared to BIP-340) and we don't have the expertise to properly evaluate and maintain this. Does this make sense?

We're happy to help you building it by answering any questions that you have. We can also link to your crate when it's finished.

@StackOverflowExcept1on
Copy link
Contributor Author

My rationale is that this seems a much more specific use case (compared to BIP-340) and we don't have the expertise to properly evaluate and maintain this. Does this make sense?

The only thing missing at the moment is standardization, but that could be the new ERC-XXXX. Signature verification is cheap even in Ethereum Mainnet, making it attractive even with the current hash_to_scalar implementation for decentralized applications. Changing the hash from sha256 to keccak256 requires no knowledge to maintain this in the ZCashFoundation repository. Perhaps we should rename it to frost-secp256k1-keccak256? At least it will just be frost-secp256k1 with keccak256 hash and nothing to do with EVM until the Ethereum community comes up with some better ERC-XXXX.

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.

Support frost-secp256k1-evm (frost-secp256k1 with keccak256)
2 participants