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

Make ECDSA signature verification customizable #2019

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

Conversation

tomaka
Copy link
Contributor

@tomaka tomaka commented Nov 1, 2024

Close #2016

This adds a new variant to the runtime-call-enums, which this time concerns ECDSA signatures verification.
It works exactly the same way as SignatureVerification, except that you have to provide a public key on success. The possible errors are also more details than just "success" or "failure".

I went with panicking if the public key provided by the API user is invalid. This only concerns the case when resume instead of verify_and_resume is called. I don't want to expose the private types of libsecp256k1, and adding yet another type for parsing the public key seems cumbersome.

I haven't tested this code because I don't think that this is used in any of the relay chains, and I also don't know which parachain uses it and how to easily test this.
However, assuming that the code worked before (which isn't 100% sure for the same reasons), it should still work as the changes were pretty straight forward.

Work time: 1h

@tomaka
Copy link
Contributor Author

tomaka commented Nov 1, 2024

cc @xlc @ermalkaleci

@ermalkaleci
Copy link
Contributor

@xlc I am not if we can utilize this since when you do the mocking we have only the address and not the public key

@ermalkaleci
Copy link
Contributor

@tomaka is this public key passed on resume verified or I can just pass some dummy data?

let sig = expect_pointer_constant_size!(0, 65);
let msg = expect_pointer_constant_size!(1, 32);
let is_v2 = matches!(
let (message_ptr, message_size) = expect_pointer_size_raw!(1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomaka I tried but got unreachable at this line

message_ptr: expect_pointer_constant_size_raw!(1, 32),
message_size: 32,

seems to fix the panic

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

although I still don't know how we can create a mocked signature with valid public

@tomaka
Copy link
Contributor Author

tomaka commented Nov 1, 2024

is this public key passed on resume verified or I can just pass some dummy data?

The public key needs to be a valid ECDSA public key, but it is not checked against the signature.
The problem here is that there are 4 host functions, 2 of which return the public key in raw format and 2 return the public key in compressed format. So what I did is in the public API you're supposed to pass a raw public key, and it gets converted to a compressed one if relevant. But for that to work, the public key has to be valid, it's not just passing it through to the Wasm code.
I can of course adjust the public API if necessary, I'm just trying to find a right balance of flexible but not too cumbersome to use.

@xlc
Copy link
Contributor

xlc commented Nov 1, 2024

@ermalkaleci the idea is to embed a valid public key in the signature and we can just read and return it

@ermalkaleci
Copy link
Contributor

@ermalkaleci the idea is to embed a valid public key in the signature and we can just read and return it

Yes but we have only the address, not public key.

@xlc
Copy link
Contributor

xlc commented Nov 1, 2024

it is possible to get public key from a valid signature so for most of the use cases, we can find signature from old tx and use it to get public key. so mocking wouldn’t work for address never signed a tx but that’s fine. it also means automatic mock would be a bit complicated. maybe we require user to supply a valid signature

@ermalkaleci
Copy link
Contributor

Yes that's my point.

@albertov19
Copy link

Hey! My understanding is that this will end up fixing this other Issue right? AcalaNetwork/chopsticks#845

What is the status :) Thanks !

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.

Make ECDSA signature verification host call customizable
4 participants