Feat/ed25519-verify#7196
Conversation
Coverage Report for CI Build 25802863122Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage decreased (-3.2%) to 82.754%Details
Uncovered Changes
Coverage Regressions9829 previously-covered lines in 165 files lost coverage.
Coverage Stats
💛 - Coveralls |
| }; | ||
|
|
||
| const ED25519VERIFY_API: SpecialAPI = SpecialAPI { | ||
| input_type: "(buff), (buff 64), (buff 32)", |
There was a problem hiding this comment.
| input_type: "(buff), (buff 64), (buff 32)", | |
| input_type: "(buff 1048576), (buff 64), (buff 32)", |
| output_type: "bool", | ||
| signature: "(ed25519-verify message signature public-key)", | ||
| description: "The `ed25519-verify` function verifies that the provided signature of the message | ||
| was signed with the private key that generated the public key.", |
There was a problem hiding this comment.
Please add the description of the outputs here as well.
|
|
||
| // Verify the signature | ||
| self.key | ||
| .verify(msg, &ed25519_sig) |
There was a problem hiding this comment.
I think we should use verify_strict here, to avoid issues with signature malleability.
| args: &[SymbolicExpression], | ||
| context: &TypingContext, | ||
| ) -> Result<TypeSignature, StaticCheckError> { | ||
| check_argument_count(3, args)?; |
There was a problem hiding this comment.
Same comment from #7187: If #7179 merges before this, it could use the new functions described #7179 (comment).
| ) -> Result<Value, VmExecutionError> { | ||
| // (ed25519-verify message signature public-key) | ||
| // message: (buff MAX_VALUE_SIZE), signature: (buff 64), public-key: (buff 32) | ||
| check_argument_count(3, args)?; |
There was a problem hiding this comment.
If #7179 merges before this, it could use the new functions described #7179 (comment).
| // message: (buff MAX_VALUE_SIZE), signature: (buff 64), public-key: (buff 32) | ||
| check_argument_count(3, args)?; | ||
|
|
||
| runtime_cost(ClarityCostFunction::Ed25519verify, exec_state, 0)?; |
There was a problem hiding this comment.
I think this should pass in the message length.
| /// SHA256 hashing the seed bytes until a private key is found. | ||
| /// | ||
| /// If `seed` is a valid private key, it will be returned without hashing. | ||
| /// The returned private key's compress_public flag will be `true`. |
There was a problem hiding this comment.
I think this is copy/pasted from secp256k1. This private key doesn't have a compress_public flag.
|
|
||
| #[tag(t_prop)] | ||
| #[test] | ||
| fn prop_ed25519_verify_accepts_valid_signatures( |
There was a problem hiding this comment.
Can you also add proptests where the message/signature/pubkey is tampered with and verify that it is always caught? Also after changing to use the strict verification, test the non-strict cases.
3 participants
Description
This patch adds the ed25519-verify clarity function as well as an infrastructure for ed25519 curves.
It expects "(buff 1024), (buff 64), (buff 32)" and returns bool (if the signature is verified)
The message buffer can be between 0 and 1024 bytes.
The signature is 64 bytes.
The public key is 32 bytes.
Cost has been profiled to 7880 units and veries very little based on the size of the message (can be between 0 and 1024) so probably it could be kept as fixed and not linear as soon as the clarity6 costs table is pushed.
Applicable issues
Additional info (benefits, drawbacks, caveats)
Checklist
docs/property-testing.md)changelog.d/README.md)rpc/openapi.yamlfor RPC endpoints,event-dispatcher.mdfor new events)clarity-benchmarkingrepo