Secp256r1 signature recovery and Ed25519 verification #486
Conversation
Dentosal
added
breaking
Dentosal
changed the title
|
|
||
| let signature = signature.normalize_s().unwrap_or(signature); | ||
|
|
||
| // TODO: this is a hack to get the recovery id. The signature should be normalized |
There was a problem hiding this comment.
Hmm, we can create an issue in their repository to support normalized signatures. Even we can try to propose a change for it=)
There was a problem hiding this comment.
Yep, I can make a PR to support that. However, it's not really blocking this PR since the version used here works, and the VM doesn't use this function so there is not performance penaly for now. I'm simply uncertain whether we're going to end up using this library for the signatures. If we end up adding more EC opcodes as proposed in the spec issue, we're likely going to write our own EC library anyway.
| b: Word, | ||
| c: Word, | ||
| ) -> Result<(), RuntimeError> { | ||
| let pub_key = Bytes32::from(read_bytes(memory, a)?); |
There was a problem hiding this comment.
It is sad that ed25519 can't recover pub_key. It affects our API(two curves recover the pub key return it into register a, but here we need to get it from the user)=(
There was a problem hiding this comment.
On the other hand, having the public key readily available during signature verification enables efficient batch verification (while it is impossible to do batch recovery).
There was a problem hiding this comment.
I don't find it any more sad than division by zero existing. We cannot alter reality to match our APIs. We just have to make sure the documentation is clear about the API differences.
There was a problem hiding this comment.
Yeah, it's just the nature of the curve.
| @@ -0,0 +1,33 @@ | |||
| //! ED25519 signature verification | |||
|
|
|||
There was a problem hiding this comment.
should we rexport the dalek lib, (maybe even under test-helpers)? That way applications using ed25519 don't have to worry about importing the correct version of dalek or pontentially compiling it twice.
Closes #464 and FuelLabs/fuel-specs#484.
This PR also removes the apparently unused
KeystoreandSignertraits fromfuel-crypto.The PR creates some technical debt:
ed25519-dalekcrate is near it's 2.0 release, and once that's released, we should update to it. Also, secp256r1 signing (that isn't used by any instruction currently) is somewhat inefficient due to limitations of thep256crate, and that must be addressed if we're going to have opcodes for doing ec signatures.