no_std fuel-crypto #578

Dentosal · 2023-09-01T06:31:37Z

Work towards #536.

This PR removes secp256k1 dependency from fuel-crypto when not depending on stdlib, and uses currently un-audited k256 crate, which is a pure-rust implementation. There's an audit underway for the it, though.

The API doesn't change much, but generating new private secp256k1 keys now requires the rng to implement the rand::CryptoRng trait, meaning they cannot be generated using rng.gen() but require SecretKey::random(&mut rng) instead.

.github/workflows/ci.yml

Voxelot · 2023-09-07T06:57:49Z

fuel-crypto/src/secp256/backend/k1/k256.rs

+///
+/// The compression scheme is described in
+/// <https://github.com/FuelLabs/fuel-specs/blob/master/src/protocol/cryptographic-primitives.md>
+pub fn sign<SK: Into<k256::SecretKey>>(secret: SK, message: &Message) -> [u8; 64] {


maybe we should use an actual trait interface in front of these backends so that it's less likely users of this crate get unexpected errors when switching between std and nostd?

I considered it, but I really don't see what extra value using a trait here would bring. Any errors would already be caught by the type system, or at least the tests. I wont mind wrapping them in a trait here if you feel like it's better like that.

Also, the types used by public_key and sign functions are not the same for both modules, as they use Into<...> with a type from the corresponding backing crate, although that could be changed as well.

It would probably be better if we had our own types on the public interfaces, and then implemented conversion to various backends as needed based on what's compiled in. Otherwise if one of these crypto libs changes their types or interfaces it could cause a lot of breaking changes in other unrelated places.

Agreed, although I find it unlikely to actually cause breakage, but it's true that upgrading an underlying crypto library is a breaking change in the current implementation. I'll fix that.

Ok, I've fixed this now for secp256k1. For secp256r1 we currently have only a single implementation, which still exposes some types from the underlying crypto lib under #[cfg(feature = "test-helpers")], which I think is fine.

Which commit has this fix? I'm not seeing it

f52fe82 and 053fa23. Forgot to push those, sorry.

fuel-crypto/benches/signature.rs

bvrooman

I am not very familiar with the cryptography concepts in this PR, but nothing sticks out as an obvious issue. Overall, it looks good.

Just one small suggestion related to implementing Distribution for SecretKey.

bvrooman · 2023-09-08T17:36:46Z

fuel-vm/src/checked_transaction.rs

@@ -712,9 +712,9 @@ mod tests {
        let tx = TransactionBuilder::script(vec![], vec![])
            .gas_price(gas_price)
            .gas_limit(gas_limit)
-            .add_unsigned_message_input(rng.gen(), rng.gen(), rng.gen(), input_amount, vec![0xff; 10])


Instead of replacing all the instances of rng.gen(), is it possible to do something like:
impl Distribution<SecretKey> for Standard { ... }
instead? I believe this should allow you to keep the existing syntax in these places.

I don't hate this syntax. It's explicit.

No. We had that before, but sadly the elliptic-curve crate (used by k256) requires us to accept only rngs with CryptoRngCore trait. At least I couldn't figure out how to make impl Distribution work with this.

MitchTurner

LGTM. IDK if I feel comfortable approving because a bit of the crypto stuff is too unfamiliar for me to scrutinize closely.

bvrooman · 2023-09-14T00:09:09Z

No additional questions from me. Again, I'm not familiar with all the cryptography concepts, so I will have to go by the tests and your judgement here.

Voxelot · 2023-09-14T01:58:45Z

fuel-crypto/src/tests/signature.rs

 use rand::{
    rngs::StdRng,
    SeedableRng,
 };

+#[cfg(feature = "std")]


I get that the secret key generation doesn't work in no_std, but do we have tests in other places to ensure that the no_std backends work the same as std?

Signature verification is probably the most critical thing to support from a no_std perspective, as that is the only logic we'll really be using in a fraud-proof mode. So we should make sure that signature verification tests work in both std and no_std.

The test for backend equivalency is in fuel-crypto/src/secp256/backend.rs. Only the secp256k1 has different implementations currently.

I agree that it makes sense to add some tests to the backends themselves.

I added standalone test cases for k256 module that also run in no_std mode, in e3943de

Voxelot · 2023-09-14T02:02:04Z

fuel-vm/Cargo.toml

@@ -19,7 +19,7 @@ derivative = "2.2"
 dyn-clone = { version = "1.0", optional = true }
 ethnum = "1.3"
 fuel-asm = { workspace = true }
-fuel-crypto = { workspace = true }
+fuel-crypto = { workspace = true, default-features = false }


Would this make the VM use the no-std backend by default in all cases? Maybe we should also enable the std feature on fuel-crypto if it's enabled on fuel-vm so we can take advantage of the more optimized implementation in native mode?

ie.

std = ["fuel-crypto/std", "fuel-tx/std"]

Using the no-std version by default could also impact our gas pricing if the performance between the native secp256k1 lib is very different from the pure-rust impl.

I'll check that the features are correct, as that seems like a potential issue. It's somewhat hard to verify that the correct crypto backend is getting used, as they have the same external API.

Using the no-std version by default could also impact our gas pricing if the performance between the native secp256k1 lib is very different from the pure-rust impl.

It certainly does, the no_std version is about 5x slower. The gas pricing shouldn't care about no_std performance, as the standard option would be to run the nodes with native secp256k1.

Added feature propagation in dc6962a

Addresses breaking fuel-vm changes from FuelLabs/fuel-vm#582, FuelLabs/fuel-vm#578, FuelLabs/fuel-vm#588 and FuelLabs/fuel-vm#587. Waiting for a new fuel-vm release. --------- Co-authored-by: Green Baneling <XgreenX9999@gmail.com>

Dentosal added 2 commits September 1, 2023 05:55

WIP

9df7764

WIP: use k256 crate for no_std compliant cryptography

b5c74f4

Dentosal added fuel-crypto Related to the `fuel-crypto` crate. needs-investigation no changelog Skips the CI changelog check labels Sep 1, 2023

Dentosal self-assigned this Sep 1, 2023

Dentosal added 7 commits September 1, 2023 09:31

Merge branch 'master' into dento/nostd_crypto

2812411

Re-add secp256k1 crate support, add backend selection

6b7f326

Fix clippy issues with features on/off

4eda0ee

Merge branch 'master' into dento/nostd_crypto

f1c6df6

Fmt toml files

dcd67d9

Cleanup for CI

f9465cb

Add changelog entry

637368b

Dentosal added breaking A breaking api change and removed needs-investigation no changelog Skips the CI changelog check labels Sep 7, 2023

Doc-tests cleanup

b68c1c5

Dentosal marked this pull request as ready for review September 7, 2023 04:25

Dentosal requested a review from a team September 7, 2023 04:25

Dentosal added 3 commits September 7, 2023 07:27

Rename secp module to secp256

1640c4e

Fix warnings with no_std target

73bd6c1

Run checks for no_std fuel-crypto

94ad309

Dentosal mentioned this pull request Sep 7, 2023

no_std support for fuel-tx and fuel-vm #582

Merged

Remove commented-out code

460b4c9

Voxelot reviewed Sep 7, 2023

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

Clean up todo comments

fc68acf

Voxelot reviewed Sep 7, 2023

View reviewed changes

fuel-crypto/benches/signature.rs Show resolved Hide resolved

bvrooman reviewed Sep 8, 2023

View reviewed changes

MitchTurner reviewed Sep 8, 2023

View reviewed changes

Dentosal added 2 commits September 11, 2023 08:36

Restore CI checking no_std compatibility with other crates

0a180b4

Merge branch 'master' into dento/nostd_crypto

88225a4

Dentosal requested review from Voxelot and xgreenx September 11, 2023 12:30

Dentosal added 2 commits September 12, 2023 08:00

Unify interfaces of different secp256k1 backends

f52fe82

Do not export secp256k1 backend as-is

053fa23

bvrooman previously approved these changes Sep 12, 2023

View reviewed changes

Dentosal dismissed bvrooman’s stale review via 053fa23 September 13, 2023 05:43

Fix imports on no_std

b2ac87d

Dentosal requested review from bvrooman and MitchTurner September 13, 2023 22:57

bvrooman previously approved these changes Sep 14, 2023

View reviewed changes

Voxelot reviewed Sep 14, 2023

View reviewed changes

Use C deps on fuel-crypto when stdlib is enabled

dc6962a

Dentosal dismissed bvrooman’s stale review via dc6962a September 14, 2023 13:58

Add no_std test cases for k256 crypto backend

e3943de

Voxelot approved these changes Sep 15, 2023

View reviewed changes

Dentosal added this pull request to the merge queue Sep 15, 2023

Merged via the queue into master with commit e18be55 Sep 15, 2023
36 checks passed

Dentosal deleted the dento/nostd_crypto branch September 15, 2023 05:59

Dentosal mentioned this pull request Sep 16, 2023

fuel-vm nostd update FuelLabs/fuel-core#1367

Merged

xgreenx mentioned this pull request Sep 27, 2023

Release 0.38.0 #590

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

no_std fuel-crypto #578

no_std fuel-crypto #578

Dentosal commented Sep 1, 2023 •

edited

Loading

Voxelot Sep 7, 2023

Dentosal Sep 7, 2023

Dentosal Sep 7, 2023

Voxelot Sep 11, 2023 •

edited

Loading

Dentosal Sep 11, 2023

Dentosal Sep 12, 2023

Voxelot Sep 12, 2023

Dentosal Sep 13, 2023

bvrooman left a comment •

edited

Loading

bvrooman Sep 8, 2023

MitchTurner Sep 8, 2023

Dentosal Sep 11, 2023 •

edited

Loading

MitchTurner left a comment

bvrooman commented Sep 14, 2023

Voxelot Sep 14, 2023 •

edited

Loading

Dentosal Sep 14, 2023

Dentosal Sep 14, 2023

Voxelot Sep 14, 2023 •

edited

Loading

Voxelot Sep 14, 2023

Dentosal Sep 14, 2023

Dentosal Sep 14, 2023 •

edited

Loading

no_std fuel-crypto #578

no_std fuel-crypto #578

Conversation

Dentosal commented Sep 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Voxelot Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvrooman left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dentosal Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

MitchTurner left a comment

Choose a reason for hiding this comment

bvrooman commented Sep 14, 2023

Voxelot Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Voxelot Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dentosal Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

Dentosal commented Sep 1, 2023 •

edited

Loading

Voxelot Sep 11, 2023 •

edited

Loading

bvrooman left a comment •

edited

Loading

Dentosal Sep 11, 2023 •

edited

Loading

Voxelot Sep 14, 2023 •

edited

Loading

Voxelot Sep 14, 2023 •

edited

Loading

Dentosal Sep 14, 2023 •

edited

Loading