stellar / rs-soroban-env

Rust environment for Soroban contracts.
Apache License 2.0
59 stars 40 forks source link

Add `secp256r1` host function for signature verification #1376

Closed jayz22 closed 4 months ago

jayz22 commented 4 months ago

What

Resolves https://github.com/stellar/rs-soroban-env/issues/807 by adding a new host function verify_sig_ecdsa_secp256r1 for ECDSA signature verification using secp256r1 curve. The function accepts following inputs:

The function is gated behind protocol 21 (min_supported_protocol = 21).

Several key interface decisions have been extensively discussed in https://github.com/stellar/stellar-protocol/discussions/1435 PR with the associated XDR changes: https://github.com/stellar/stellar-xdr/pull/178, https://github.com/stellar/rs-stellar-xdr/pull/355 SDK PR adapting to this change: https://github.com/stellar/rs-soroban-sdk/pull/1246

Metering and Calibration

Two new cost types have been newly added:

A prevous cost type ComputeEcdsaSecp256k1Sig has been renamed to DecodeEcdsaCurve256Sig, which represents the cost of deserializing both the secp256k1 and secp256r1 signatures.

Calibration:

Calibration results has been posted and discussed in https://github.com/stellar/stellar-protocol/discussions/1435#discussioncomment-8881673

Testing

Unit tests have been added to test against various forms of invalid inputs.

In addition, two set of test vectors has been added in integration test:

anupsdf commented 4 months ago

CI cackle is pointing out that ERROR:base64ctuses unsafe. We need to check if this will cause any determinism issue.

jayz22 commented 4 months ago

CI cackle is pointing out that ERROR:base64ctuses unsafe. We need to check if this will cause any determinism issue.

Resolved. The unsafe base64ct usage comes from a default feature pem in the p256 crate, which we are not using. I've cleaned up the dependency to include only necessary minimal features in this commit.

graydon commented 4 months ago

I'm a bit nervous about the way you're tunneling through the protocol gate to access the host function before it's available, though I understand why and suspect it's the best option for now. In the change I'm getting started on now (protocol-qualification of all the tests and un-gating the next stuff) we can remove this sort of thing.

jayz22 commented 4 months ago

I'm a bit nervous about the way you're tunneling through the protocol gate to access the host function before it's available, though I understand why and suspect it's the best option for now. In the change I'm getting started on now (protocol-qualification of all the tests and un-gating the next stuff) we can remove this sort of thing.

Just making sure I understand your concern of "tunneling through the protocol gate". The host function protocol gate (min_supported_protocol=21 and the generated runtime checks) is in place that prevents accessing the function via the host interface (either v20 wasm contract, or a v20 contract in native mode). I'm not changing any of that. The actual implementation code and helper methods are guarded behind #[cfg(feature = "next")]. These exist to switch access to the v21 cost type definitions. Once the curr/next xdr changes are merged, we can remove them.

I believe you are talking about the tests that are directly calling the new method via VmCallerEnv. I think it's reasonable to have them be protocol dependent. However it isn't totally necessary since those tests are almost purely for testing the correctness of the secp256r1 logic, which is not protocol dependent. Although I understand there are parts which are protocol decisions (such as 32 bytes msg_digest and uncompressed sec1 encoding). Once your protocol-qualification of testing changes are ready, we can adapt to that.

graydon commented 4 months ago

I'm a bit nervous about the way you're tunneling through the protocol gate to access the host function before it's available, though I understand why and suspect it's the best option for now. In the change I'm getting started on now (protocol-qualification of all the tests and un-gating the next stuff) we can remove this sort of thing.

Just making sure I understand your concern of "tunneling through the protocol gate". The host function protocol gate (min_supported_protocol=21 and the generated runtime checks) is in place that prevents accessing the function via the host interface (either v20 wasm contract, or a v20 contract in native mode). I'm not changing any of that. The actual implementation code and helper methods are guarded behind #[cfg(feature = "next")]. These exist to switch access to the v21 cost type definitions. Once the curr/next xdr changes are merged, we can remove them.

I believe you are talking about the tests that are directly calling the new method via VmCallerEnv. I think it's reasonable to have them be protocol dependent. However it isn't totally necessary since those tests are almost purely for testing the correctness of the secp256r1 logic, which is not protocol dependent. Although I understand there are parts which are protocol decisions (such as 32 bytes msg_digest and uncompressed sec1 encoding). Once your protocol-qualification of testing changes are ready, we can adapt to that.

Yes, exactly. If you notice in the observations recorded for the test in this PR, they don't contain the calls to the new host function at all, because you're actually bypassing the point where tracing machinery would normally hook in to observe the call.

I think it will probably be something we can roll back when we have protocol-qualified testing, and is ok for now. Just something I need to remember to come back to.