-
Notifications
You must be signed in to change notification settings - Fork 65
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
feat: add frost-secp256k1-evm
crate
#749
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #749 +/- ##
==========================================
+ Coverage 82.18% 83.40% +1.21%
==========================================
Files 31 37 +6
Lines 3188 4061 +873
==========================================
+ Hits 2620 3387 +767
- Misses 568 674 +106 ☔ View full report in Codecov by Sentry. |
@conradoplg could you please take a look and enable CI? after the review I will make |
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 |
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 |
Also for reference, see the Taproot interoperability test |
I don't want to use
I wrote library in Solidity that can verify
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 |
I would also like to know how necessary After some reverse engineering I was able to port 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 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. |
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 |
@StackOverflowExcept1on We've discussed this internally and we're uncomfortable including new ciphersuites into this repo that do not have a formal specification and have not been peer reviewed. At the very least it would be good to know that something has been battle tested before including it :) If you do publish independently from your own repo, we'd be happy to link to it as an additional community resource for the ethereum ecosystem. Happy to re-visit this decision if/when you can get this peer reviewed and/or it's in wide use :) Thank you for your contribution and happy to help out further with reviews, etc... |
ERC-7816 was created recently: https://ethereum-magicians.org/t/erc-7816-a-schnorr-signature-scheme-for-evm-applications/21636. It also has documentation and reference implementation: https://github.com/verklegarden/schnorr-on-evm/blob/main/ERC.md. |
@StackOverflowExcept1on This is useful context, thank you. It looks like this ERC is currently at the "Idea" stage. Is that correct? |
@jackgavigan Yes, this ERC has not been accepted/audited yet. |
Resolves #715
Note: This is testing branch, where I've simply renamed
frost-secp256k1
tofrost-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