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

SignEcdsa is too slow #2

Open
lastagile opened this issue Sep 15, 2021 · 23 comments
Open

SignEcdsa is too slow #2

lastagile opened this issue Sep 15, 2021 · 23 comments

Comments

@lastagile
Copy link

SignEcdsa takes couple seconds, any way to make it quicker?

@alexberdreal
Copy link

Hello! Have you found the solution for this?

@xJonathanLEI
Copy link
Contributor

Same issue here. Calling Sign() takes more than 7 seconds on my machine.

@xJonathanLEI
Copy link
Contributor

It seems that turning on -O3 helps. Signing time drops from 7 seconds to 180 milliseconds on my machine. It's apparently still not ideal, but better than nothing.

@rbdm-qnt
Copy link

was this ever solved?

@xJonathanLEI
Copy link
Contributor

@rbdm-qnt Nope but (shameless plug) if you use Rust then you can use starknet-crypto lol.

@rbdm-qnt
Copy link

@rbdm-qnt Nope but (shameless plug) if you use Rust then you can use starknet-crypto lol.

And is that one faster?

@xJonathanLEI
Copy link
Contributor

Yeah it is. See the linked page for benchmark.

@rbdm-qnt
Copy link

Yeah it is. See the linked page for benchmark.

1ms or less vs 7 seconds/180 ms? I don't understand, there must be something terribly wrong with the c++ version? Even the python version takes 70-80ms only. Unfortunately I don't use Rust, my application is in C++ and I need a high performance version for that language

@xJonathanLEI
Copy link
Contributor

Well you can use the Rust lib from C++ as well. You'll need to create a wrapper crate that exposes the C api but it's possible.

Honestly that's likely your best bet imo, as this repo doesn't seem to be maintained.

@rbdm-qnt
Copy link

Well you can use the Rust lib from C++ as well. You'll need to create a wrapper crate that exposes the C api but it's possible.

Honestly that's likely your best bet imo, as this repo doesn't seem to be maintained.

Wouldn't that have a big overhead? Never done that

@xJonathanLEI
Copy link
Contributor

xJonathanLEI commented Mar 12, 2023

Wouldn't that have a big overhead? Never done that

Nope it's just like linking to any other library. Rust does not have a runtime. Might be helpful:

@rbdm-qnt
Copy link

Wouldn't that have a big overhead? Never done that

Nope it's just like linking to any other library. Rust does not have a runtime. Might be helpful:

* https://parallel-rust-cpp.github.io/cpp_abi.html

* https://stackoverflow.com/questions/69418715/calling-rust-from-c-using-cxx-bridge

Thanks, I'll try it! Would you be interested in providing a C++ bind in the lib? Also any more documentation on how to install/operate the lib would be greatly appreciated (especially for a Rust noob :))

@xJonathanLEI
Copy link
Contributor

I would love to have such instructions implemented as an example (specifically with cxx). Created an issue to track it: xJonathanLEI/starknet-rs#325

However I'm currently too busy to put it together. Would be awesome if someone can help. Or if you make it work, feel free to drop a PR for that. Thanks!

@rbdm-qnt
Copy link

I would love to have such instructions implemented as an example (specifically with cxx). Created an issue to track it: xJonathanLEI/starknet-rs#325

However I'm currently too busy to put it together. Would be awesome if someone can help. Or if you make it work, feel free to drop a PR for that. Thanks!

Could you please at least provide an example of how to use the Rust code itself? I'm trying to understand how to create some C bindings, but can't figure out these Rust types like FieldElement and Fp256Parameters.

@xJonathanLEI
Copy link
Contributor

@rbdm-qnt
Copy link

rbdm-qnt commented Mar 15, 2023

@rbdm-qnt xJonathanLEI/starknet-rs#328

I remade the whole thing in C++ from scratch. I thought the 180ms latency reported must have been a mistake. I followed python's implementation (which takes 75ms) and remade it in cpp17, and it takes around 100ms. I think the problem is possibly the handling of GMP types, or maybe some bottleneck in cryptopp algorithms, but it's quite puzzling. I can't understand how's its possible for Rust to run this in 1ms.

Anyway, now I really must understand how to use your implementation in my C++ application. Please, if you could provide any basic example of usage of the Sign method (even just in Rust, doesn't have to be in C++) it would save me a lot of time, cos I'm a total Rust noob

@xJonathanLEI
Copy link
Contributor

Please, if you could provide any basic example of usage of the Sign method

Hey man my last comment was exactly just that. It's a CMake example:

https://github.com/xJonathanLEI/starknet-rs/tree/master/examples/starknet-cxx

@rbdm-qnt
Copy link

Please, if you could provide any basic example of usage of the Sign method

Hey man my last comment was exactly just that. It's a CMake example:

https://github.com/xJonathanLEI/starknet-rs/tree/master/examples/starknet-cxx

Thank you, that's great! Is this file autogenerated? Cause I can't find it in the directories "starknet_cxx_bridge/lib.h"

@xJonathanLEI
Copy link
Contributor

I can't find it in the directories "starknet_cxx_bridge/lib.h"

Yes it's generated by cxx. It should live somewhere inside the build folder once you build it. Pretty sure you can just go into definition if you use an LSP. (Not 100% sure cuz I wrote the example without an LSP)

@rbdm-qnt
Copy link

rbdm-qnt commented Mar 16, 2023

I can't find it in the directories "starknet_cxx_bridge/lib.h"

Yes it's generated by cxx. It should live somewhere inside the build folder once you build it. Pretty sure you can just go into definition if you use an LSP. (Not 100% sure cuz I wrote the example without an LSP)

I was able to make the example work and integrate it into my codebase with some work, thank you! I have another question, why does the Sign method return 1 string? Canonically, ecdsa_sign methods return r and s values, so 2 strings, as far as I know

EDIT: looked into the rust code, it was just a formatting issue, i modified the lib.rs binding to return the individual elements of the signature with a delimiter in between them

@xJonathanLEI
Copy link
Contributor

Yep yep it was just to make the bindings example simpler. Glad you figured it out! It's great that you found the lib helpful!

@rbdm-qnt
Copy link

rbdm-qnt commented Mar 16, 2023

Yep yep it was just to make the bindings example simpler. Glad you figured it out! It's great that you found the lib helpful!

I'm having trouble getting valid values for ecsda_signature; in the python version I'm able to generate rfc6979 key with an empty seed value (in bytes b"") or values like "\x01" or above. In Rust these values error out, as well as an empty string, and I have to use "0x1" or above. I'm thinking this might be the reason why the signatures are invalid? For instance, I'm generating a signature with the same exact values in both Python and C++/Rust, and then verifying the outputs

EDIT: one of the things I noticed is that, if I regenerate ecsda_signature, in python both r and s are always different, while in Rust r is always the same and s is different; i tried verifying either of them or a mix of r from python and s from rust and anything coming out of Rust seems invalid

EDIT 2: the issue seems to be in the ecdsa_sign method itself, im 99% sure its about the k arg. it most probably gets treated differently than in python. Could the problem be that I'm trying to compare this Rust Sign method with a RFC6979 compliant signature? Is it not? And if so, which method implements RFC6979 signature?

EDIT 3: I think my best bet is bridging over the generate_k method from rust, use that to generate k and give it to the sign method? I'm trying to add it to the lib.rs file but no luck compiling it so far, it says the method can't be found in starknet_crypto

EDIT 4: That was correct, i was able to expose it using and it matches python. Now in the final output, R matches python, S doesn't, the verify still fails, but i'm getting closer.

CONCLUSION: i ended up reimplementing the sign method in c++ following Starkware Python implementations as i couldn't make sense of this one, and couldn't get it to verify. It was still useful, but now i also understand the reason behind the speed difference: most of the latency is in this step

@xJonathanLEI
Copy link
Contributor

@rbdm-qnt Hi. When using correctly all the Rust methods have been tested to generate exactly the same output as their Python counterparts. I've also verified that using ecdsa_sign from C++ gives the exact same output as in Rust. Everything should be fine. The lib is used in many places for the cryptographic stuff so it should work.

You mentioned an empty string doesn't work. That's because of how the wrapper itself (lib.rs) handles input. Everything gets converted into FieldElement at the end of the day and starknet-crypto only every deals with FieldElements, not strings. On the wrapper level, the parsing logic doesn't treat an empty string as a valid value while the Python version does (according to you; I haven't verified this).

I'm not sure how Python treats the empty string. Maybe it treats it as a 0 value? In that case, you can should be able to input 0x0 to the wrapper and it should parse correctly. Of course, this builds on the assumption that Python treats it as zero.

In any case I'm happy you made it work, but if you still consider it a bug after reading this comment (maybe I misunderstood something in your text), please file a bug under starknet-rs. Thanks. Let's not continue spamming the crypto-cpp folks lol

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

No branches or pull requests

4 participants