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

SIMD-0048: Native Program for secp256r1 signature verification #48

Merged
merged 16 commits into from
Jun 29, 2023

Conversation

0xRigel
Copy link
Contributor

@0xRigel 0xRigel commented May 15, 2023

General Goal:

Add support for secp256r1 signature verification, so we can enable the usage of Passkeys, WebAuthn and other account abstractions

Notes

Would love to get the ball rolling on this :) Have talked briefly to @mvines & @samkim-crypto about it, and would greatly appreciate any additional inputs/ideas

@0xRigel 0xRigel changed the title SIMD: Support for secp256r1 signature verification SIMD: Syscall for secp256r1 signature verification May 15, 2023
@0xRigel 0xRigel changed the title SIMD: Syscall for secp256r1 signature verification SIMD-0048: Syscall for secp256r1 signature verification May 15, 2023
## Alternatives Considered
We have discussed the following alternatives:

1.) Realising signature verification with a native program/instruction similar to `KeccakSecp256k11111111111111111111111111111` instead of a syscall. As the signature is known/generated by the user upfront for a tx, an implementation as a native program would also work.
Copy link
Contributor

Choose a reason for hiding this comment

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

@Lichtso - from the perspective of ABIv2, a native program/instruction would be strongly preferred over another syscall right? Or does it not really matter?

Copy link
Contributor

Choose a reason for hiding this comment

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

In program-runtime-v2 we will migrate all syscalls to built-ins, but that is still over a year away.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would there be any benefit / significant man hours saved if I went with the native program approach?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we probably should hold here until @Lichtso and the runtime team decides what we're going to do in general about the "Precompile" instructions that currently exist for runtime-v2. We've had some Discord discussions about that over the last day or so.

Generally I feel like adding a syscall isn't the best approach if we decide to retain the notion of Precompiles in runtime-v2, since that limits our future ability to optimize the performance of this signature verification should it gain widespread usage. Others might argue that would be a premature optimization though..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, okay 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just want to ping in here again :) @Lichtso would you have anything against an implementation as a native program / do you see any up or downsides for either option?

@0xRigel
Copy link
Contributor Author

0xRigel commented Jun 4, 2023

@mvines is greatly in favor of implementing this as a native program as to maximise optimisation for future usage. Does anyone else have any two-cents on this? If not, I will modify/rewrite the PR to reflect this.

@samkim-crypto
Copy link
Contributor

Yes, I think the secp256r1 verification have potential to have widespread usage, so we should do what we can to leave any potential for optimization, which would mean the native program route.

I think @Lichtso's opinion should weigh heavily here though, so it might be good to wait until he has some input.

@Lichtso
Copy link
Contributor

Lichtso commented Jun 9, 2023

Sorry for the late reply. So we have three options:

  1. Precompile: Preferred method for potential optimizations, but prevents being used from CPI
  2. Syscall: Also fine, will be later be moved into a built-in (together with other syscalls) in program-runtime-v2
  3. Built-in: A bit too much management overhead to have a standalone built-in with only one method, so would advise against it

@mvines
Copy link
Contributor

mvines commented Jun 14, 2023

I suggest we proceed with the Precompile route for now. These signatures are always created off-chain so there's not really a use-case for a program to be able to provide the input (eg, Syscall approach)

@0xRigel
Copy link
Contributor Author

0xRigel commented Jun 14, 2023

Excellent 👍 Will rework the SIMD to reflect this. Thanks for all the input ❤️

@0xRigel 0xRigel changed the title SIMD-0048: Syscall for secp256r1 signature verification SIMD-0048: Native Program for secp256r1 signature verification Jun 22, 2023
@0xRigel
Copy link
Contributor Author

0xRigel commented Jun 22, 2023

Rewrote the SIMD to reflect our consensus on the route of making it a precompile/native program 👍

mvines
mvines previously approved these changes Jun 22, 2023
Copy link
Contributor

@mvines mvines left a comment

Choose a reason for hiding this comment

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

This proposal looks good to me, I don't see any concerns from the Labs validator client side. Would love to see a bit of feedback from somebody else though.

If I recall correctly, @sakridge implemented the secp256k1 program so just tagging him in the event he wishes to review and comment

@0xRigel
Copy link
Contributor Author

0xRigel commented Jun 22, 2023

This proposal looks good to me, I don't see any concerns from the Labs validator client side. Would love to see a bit of feedback from somebody else though.

If I recall correctly, @sakridge implemented the secp256k1 program so just tagging him in the event he wishes to review and comment

Great! Thx 👍

Copy link

@sakridge sakridge left a comment

Choose a reason for hiding this comment

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

looks reasonable to me

@romeo4934
Copy link

Amazing! Passkeys and multisig are the future of self-custody ❤️

@mvines
Copy link
Contributor

mvines commented Jun 29, 2023

Enough time has passed with approvals and no additional comments, merging

@mvines mvines merged commit 42fc78c into solana-foundation:main Jun 29, 2023
Comment on lines +114 to +117
The crates `ecdsa` and `p256` are a good starting point for the implementation.
Due to a current dependency version conflict of `zeroize` between
`curve25519-dalek` and `solana-program`, using these crates will require a
fix/bump of `zeroize` inside `curve25519-dalek`. See issue [#26688](https://github.com/solana-labs/solana/issues/26688)
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally, we'd use the curve point encoding from secp256k1 here to minimize additional complexity

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted 👍

Copy link
Contributor

@ripatel-fd ripatel-fd left a comment

Choose a reason for hiding this comment

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

IMHO, this document does not clearly explain the changes made to the protocol. The cryptographic algorithm details are entirely unspecified, except for the words ECDSA and secp256r1 and pointers to a few third-party implementations. The usual standard for a SIMD is that it should be complete, unambiguous, and feature bit-level reproducibility, without relying on implementation code too much.

For sake of example, if Solana Labs added an arbitrary implementation of ECDSA signature verification from a third-party library, I would ask myself the following questions:

  • Can we swap out the third-party library for another without changing consensus rules (i.e. hard forking) in case the author abandons it? (Continuity risk)
  • Is the library compatible with another implementation written in a different language?
  • Is there a suite of test cases that covers all notable checks so we can quickly detect a non-compliant implementation?
  • Are there notable differences from a widely accepted specification?
  • Is this specification free of ambiguity or gaps? (I don't assume this to be the case for ECDSA, given that it is not a single protocol. It is more like a family of protocols rooting in the same math, but using wildly different parameters and encoding rules. (ASN.1-based, GPG/MPI, COSE, ...)) ... If there are gaps, what is the exact behavior left unspecified?
  • Does the implementation behave deterministically, e.g. across different hardware architectures? (such as differences in the reference implementation vs a vectorized one, which both might handle unspecified logic differently)

If the answer is unclear or "no" to any of these, it cannot be safely realized (yet).
I don't know the answers to these myself, but would argue that the document should provide them. In Ed25519, the usual suspects were point compression and small order checks. I'm not sure if they apply to ECDSA too.

Here are some examples of high-quality external resources on ECC algorithms:

It would be nice if the Bunkr team could link to similar documents and provide a test suite for the new program. We (Firedancer) are generally opposed to adding new cryptography to the protocol unless absolutely necessary. Even if the proposal is technically sound and well-defined, implementation bugs will pose a financial risk to its users or the chain at large. At least until we have a more robust way of handling implementation differences than halting validation blockchain-wide.

Finally, I'd like to consider our path to implementing this – Although "secp256k1" and "secp256r1" may appear close looking at their names, there is a huge practical difference. When me mention "secp256k1", we actually mean the specific Bitcoin/Ethereum signature scheme using ECDSA over the secp256k1 curve using ECRECOVER mode of verification. This specific signature scheme was subjected to years of scrutiny ensuring it runs safe in a deterministic/on-chain context. This is why it is comparatively safer to support. Who is going to support the effort of bringing a secp256r1-based protocol up to the same standards? Is it Solana Foundation and the Firedancer team who are going to have to call for cryptographic peer review and cover SIMD-0048 with an expensive bug bounty program?

@0xRigel
Copy link
Contributor Author

0xRigel commented Oct 23, 2023

Here's some clarification around the spec:

Curve Parameters: NIST P-256/secp256r1/prime256v1 curve parameters are outlined in the SEC2 document in section 2.7.2

Point Encoding: Point encoding is well outlined in SEC1 in Section 2.3.3 as either uncompressed or compressed SEC1 encoded points.

Signing & Verification Functions: Functions for signing messages and message verification are outlined in SEC1 in Section 4.1. Additionally the updated algorithm in RFC6979 specifically outlines a the Deterministic ECDSA procedure, albeit only for creating signatures (corrected). Test Vectors for P-256 are also present in RFC6979 in section A.2.5.

p256 rust crate: The p256 crate crate is an implementation of the NIST-P256 curve and is part of the widely used and well maintainedrust-crypto library. It's tests have parity with the vectors found in RFC6979

As a NIST certified curve, NIST-P256 and its algorithms are well documented and tested. Apart from the performance optimizations found in secp256k1, it relies on the same arithmetic and only differs in curve parameters. An implementation in C for instance can be found here.

A suitable testing suite to ensure general safety could be the Wycheproof Project

@ripatel-fd
Copy link
Contributor

@iceomatic Thanks, this helps somewhat. Can you please amend the SIMD itself with this information?

Additionally the updated algorithm in RFC6979 specifically outlines a the Deterministic ECDSA procedure

This does not implement the kind of determinism we need. This RFC specifies modifications to the signing operations. This SIMD only covers verification operations. We have to ensure that the verification function, given some input, returns the same output regardless of implementation used, even for malicious input. This is a requirement rarely seen outside blockchain runtimes. Therefore it is not well supported.

Here is a short list of ECDSA verification bugs found in software used in the wild. Most of these are produced from garbage/malicious input for which it would have been safe to return an unspecified result in the traditional context. (Garbage public key + garbage signature = ?). If any one of the below produced a discrepancy between Solana Labs and another client, it could potentially halt the chain.

@0xRigel
Copy link
Contributor Author

0xRigel commented Oct 23, 2023

Yes will do. I see your point regarding precise computational determinism. Obviously any mismatch there would be catastrophic. Trying to think of the best way to move forward.

Spitballing:
Has fd created a library for field arithmetic on secp256k1 yet? Might it be possible to use that, with adjusted curve parameters and without the optimisations around k1, and then instead of having a rust implementation, rely on a pre- compiled C version in both the Labs client, as well as fd?

@ripatel-fd
Copy link
Contributor

ripatel-fd commented Oct 30, 2023

Has fd created a library for field arithmetic on secp256k1 yet?

No, but the Ed25519 field element library should be reusable for secp256{r,k}1.
https://github.com/firedancer-io/firedancer/blob/main/src/ballet/ed25519/avx512/fd_r43x6.c

@jbrower95
Copy link

jbrower95 commented Nov 24, 2023

Hey folks. Just wanted to say that passkey signers powered by secp256r1, per the spec above, empower experiences like (https://defifortheworld.com), which are both safe and portable in ways that most current embedded-wallet web SDKs can't achieve.

Where can I find the latest discussion on this proposal?

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: SIMDs
Development

Successfully merging this pull request may close these issues.

8 participants