SIMD-0075: Secp256r1 Precompile (Supersedes SIMD-0048)

[iceomatic]

I've updated the SIMD to include all the things discussed in conversation on github and the core-technology channel over the past few days.

This includes:

• More specification & detail on the implementation
• New security considerations
• Citations to relevant documentation

It has become quite a bit more opinionated and therefore requires more discussion.

[iceomatic]

Tagging participants of the former discussion around SIMD-0048 for visibility
@samkim-crypto, @Lichtso , @mvines, @ripatel-fd , @sakridge

[ripatel-fd]

This looks great. I'll review in depth ASAP.

I'd also like to point out https://github.com/guidovranken/cryptofuzz as a dynamic analysis security tool.
This fuzzer has found many differences in behavior between secp256k1 ECDSA implementations and can probably be applied to secp256r1 too.

[iceomatic]

I'd also like to point out https://github.com/guidovranken/cryptofuzz as a dynamic analysis security tool. This fuzzer has found many differences in behavior between secp256k1 ECDSA implementations and can probably be applied to secp256r1 too.

Noted, thx! Will look into incorporating it into testing 🫡

[iceomatic]

I have re-written the Implementation section of the SIMD to include the pivot into using the OpenSSL crate instead of the p256 crate. Ive also added detailed implementation details as to how sig verify will be accomplished using OpenSSL.

TL;DR: Its ~3x faster than p256, is already a dependency in the labs/anza client, and uses underying C code, rather than native rust.

The idea here is to:

A. Make implementation comparison/parity between labs/anza and Firedancer easier
B. Use an rely on a more reputable and well maintained crate rather than an "unknown" and somewhat new one
C. Have faster/more efficient sigverify

Let me know what you think

[t-nelson]

this is a good start! please take a look at rfc2119 and make a pass at this document to correct usage of these key words. there are several SHOULDs that need to be MUSTs

[iceomatic]

All fixes/resolutions to open comments should be done. I'm unsure if the pseudo-coded structs and the processing logic are okay in the new format, so please leave feedback wrt to changes if you have any 👍

[0xSol]

Thanks @iceomatic for updating the SIMD and addressing fixes/resolutions in comments.
@ripatel-fd, @sakridge, @t-nelson, could you please take another look, and comment / approve as appropriate? Thanks.

[ptaffet-jump]

Some clarifications for the pseudocode

[ptaffet-jump]

I know existing precompiles have this check, but I think it's unnecessary.

[iceomatic]

So in the case of count == 0 the program should pass, irrespective of the instruction data?

[ptaffet-jump]

That would make sense to me, but I'd be interested in hearing from the folks who made this design decision for the other precompiles to make sure I'm not missing something.

[iceomatic]

Will leave this unresolved until we get some more input from the other contributors 🫡

[samkim-crypto]

For this issue, I think it makes sense either way. This predates me but solana-labs/solana#19300 has the justification for the count, which I think is reasonable.

[tlambertz]

That would make sense to me, but I'd be interested in hearing from the folks who made this design decision for the other precompiles to make sure I'm not missing something.

This is done as a safeguard for smart-contracts. There have been a number of contracts that wanted to verify a single signature, and thus omitted the check for count.
They just used instruction introspection to assert that the correct data is present in the call to secp verify.

An attacker could thus call secp verify with a count of 0, and pretend to have a valid signature for arbitrary data.

This is obviously a bug in the smart-contract using secp, but having this count=0 check made for an easy fix to prevent this issue from cropping up. It makes no sense to call with a zero count and non-zero data after all.

[iceomatic]

I guess as there's no document or SIMD detailing otherwise, I'd we just stay in line with #19300.

Thoughts @ptaffet-jump ?

[iceomatic]

Thanks @ptaffet-jump for the input, all comments with the exception of the count == 0 && length_of_data > 1 behaviour should be resolved 👍

[samkim-crypto]

Looks good to me!

It is worth noting (I apologize if this has already been discussed before) that there is going to be an inconsistency in the way ecdsa malleability is handled with the existing secp256k1 implementation, which does not handle malleability.

[iceomatic]

Looks good to me!

It is worth noting (I apologize if this has already been discussed before) that there is going to be an inconsistency in the way ecdsa malleability is handled with the existing secp256k1 implementation, which does not handle malleability.

Yep. In the description of the SIMD we made our case as to why it would be advantageous to handle malleability, but if contributors disagree I can happily remove that check to put it in line with the other precompiles
@samkim-crypto

[iceomatic]

With the approval from Anza we're just missing an approval on the FD side now 👍