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.

@@            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.

@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.

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.

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.

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 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?

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

@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.

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)
    )
);

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.

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.