helper for unsafe code #722

curiecrypt · 2023-02-01T13:20:41Z

Content

This PR includes the helper functions that store all usage of unsafe code in multi_sig.rs.

Pre-submit checklist

Branch
- Commit sequence broadly makes sense
- Key commits have useful messages
PR
- No clippy warnings in the CI
- Self-reviewed the diff
- Useful pull request description
- Reviewer requested

Issue(s)

Closes #690

github-advanced-security

clippy found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

github-actions · 2023-02-01T13:27:34Z

Test Results

    3 files ±  0   28 suites ±0 5m 9s ⏱️ +15s
437 tests +10 437 ✔️ +10 0 💤 ±0 0 ❌ ±0
505 runs +10 505 ✔️ +10 0 💤 ±0 0 ❌ ±0

Results for commit c83d6bd. ± Comparison against base commit f013bb7.

♻️ This comment has been updated with latest results.

mithril-stm/src/multi_sig.rs

iquerejeta

A first round of comments. Tomorrow I'll look into the unsafe_helpers module.

iquerejeta · 2023-02-08T17:53:15Z

mithril-stm/src/multi_sig.rs

+        // use blst::{
+        //     blst_fp12, blst_fp12_finalverify, blst_p1_affine_generator, blst_p2_affine_generator,
+        //     BLST_ERROR,
+        // };


mithril-stm/src/multi_sig.rs

iquerejeta · 2023-02-08T17:59:54Z

mithril-stm/src/multi_sig.rs

 use blst::{
    blst_p1, blst_p1_affine, blst_p1_compress, blst_p1_from_affine, blst_p1_to_affine,
    blst_p1_uncompress, blst_p2, blst_p2_affine, blst_p2_from_affine, blst_p2_to_affine,
-    blst_scalar, p1_affines, p2_affines,
+    blst_scalar, p1_affines, p2_affines, BLST_ERROR,
 };


Can't any of these imports be moved to the unsafe_helpers module?

iquerejeta · 2023-02-09T08:24:29Z

mithril-stm/src/multi_sig.rs

+        bytes
+    }
+
+    pub(crate) fn uncompress_p1(bytes: &[u8]) -> blst_p1 {


We could do a length check for the input. if it's not 48 bytes, return error. I also thought of using an array, [u8; 48], but that might complicate the function call, and it's not necessary.

Meaning the output of the function should be aResult<blst_p1, ()> rather than a point (where () should be some error type that we have in the lib).

iquerejeta · 2023-02-09T08:27:25Z

mithril-stm/src/multi_sig.rs

+        unsafe {
+            let mut point = blst_p1_affine::default();
+            let mut out = blst_p1::default();
+            blst_p1_uncompress(&mut point, bytes.as_ptr());


This function returns a BLST_ERROR. It should not be ignored.

mithril-stm/src/multi_sig.rs

iquerejeta · 2023-02-09T08:53:45Z

mithril-stm/src/multi_sig.rs

+        }
+    }
+
+    pub(crate) fn p2_affine_to_vk(grouped_vks: &p2_affines, scalars: &[u8]) -> BlstVk {


This function signature is very confusing. It's name p2_affine_to_vk but you give as input &p2_affines and &[u8], and you output a BlstVk. This function should already take as input the result of &grouped_vks.mult(scalars, 128), or have a different name (if possible, I'd suggest the former).

iquerejeta · 2023-02-09T08:54:14Z

mithril-stm/src/multi_sig.rs

+        }
+    }
+
+    pub(crate) fn p1_affine_to_sig(grouped_sigs: &p1_affines, scalars: &[u8]) -> BlstSig {


Same comment as for p2_affine_to_vk function

Co-authored-by: Iñigo Querejeta Azurmendi <31273774+iquerejeta@users.noreply.github.com>

mithril-stm/src/multi_sig.rs

iquerejeta

Looks good 👍 just minor comment to revert one comment I said in the previous round, didn't realise it would require us to copy secret data.

iquerejeta · 2023-02-14T16:09:34Z

mithril-stm/src/multi_sig.rs

+// blst_p1_affine, blst_p1_compress, blst_p1_from_affine, blst_p1_to_affine,
+//     blst_p1_uncompress, blst_p2, blst_p2_affine, blst_p2_from_affine, blst_p2_to_affine,
+//     blst_scalar, p1_affines, p2_affines, BLST_ERROR,


iquerejeta · 2023-02-14T16:15:56Z

mithril-stm/src/multi_sig.rs

+        blst_scalar, blst_sk_to_pk_in_g1,
+    };
+
+    pub(crate) fn verify_pairing(vk: &VerificationKey, pop: &ProofOfPossession) -> bool {


I would include a little description of what this function does, as one could get confused. I would describe the relation that it is checking, i.e. e(g1, mvk) = e(k2, g2).

iquerejeta · 2023-02-14T16:18:08Z

mithril-stm/src/multi_sig.rs

+
+    pub(crate) fn scalar_to_pk_in_g1(sk: &SigningKey) -> blst_p1 {
+        unsafe {
+            let sk_scalar = std::mem::transmute::<BlstSk, blst_scalar>(sk.0.clone());


I don't like the fact that we have to clone the secret key here. I know I said about consistency in my previous comment, but it is true that this means that the value of the secret key needs to be cloned. I did not think about that. I'd rather revert to the previous state, where we transmute the pointers so that we don't have to clone secret data.

curiecrypt requested a review from iquerejeta February 1, 2023 13:20

github-advanced-security bot found potential problems Feb 1, 2023

View reviewed changes

mithril-stm/src/multi_sig.rs Fixed Show fixed Hide fixed

mithril-stm/src/multi_sig.rs Fixed Show fixed Hide fixed

mithril-stm/src/multi_sig.rs Fixed Show fixed Hide fixed

mithril-stm/src/multi_sig.rs Fixed Show fixed Hide fixed

mithril-stm/src/multi_sig.rs Fixed Show fixed Hide fixed

curiecrypt temporarily deployed to testing-preview February 1, 2023 13:44 — with GitHub Actions Inactive

curiecrypt marked this pull request as draft February 1, 2023 15:14

curiecrypt temporarily deployed to testing-preview February 2, 2023 12:06 — with GitHub Actions Inactive

curiecrypt temporarily deployed to testing-preview February 7, 2023 11:15 — with GitHub Actions Inactive

curiecrypt added 6 commits February 7, 2023 15:18

helper for transmute added

1d196e5

clippy errors corrected

267ce8b

herlpers made private

ffec0f0

helpers for unsafe code

4b7959e

unsafe code grouped in functions

cb148d7

error corrections

1c914fd

curiecrypt force-pushed the bls-transmute-helper branch from 7ec5fd0 to 1c914fd Compare February 7, 2023 12:20

curiecrypt temporarily deployed to testing-preview February 7, 2023 12:36 — with GitHub Actions Inactive

curiecrypt added 2 commits February 7, 2023 16:43

references and pointers corrected

bbc23f9

mod for unsafe helpers

9d25947

curiecrypt changed the title ~~helper for transmute added~~ helper for unsafe code Feb 7, 2023

curiecrypt temporarily deployed to testing-preview February 7, 2023 15:04 — with GitHub Actions Inactive

curiecrypt marked this pull request as ready for review February 7, 2023 15:04

iquerejeta requested changes Feb 8, 2023

View reviewed changes

iquerejeta requested changes Feb 9, 2023

View reviewed changes

Apply suggestions from code review

77801f0

Co-authored-by: Iñigo Querejeta Azurmendi <31273774+iquerejeta@users.noreply.github.com>

github-advanced-security bot found potential problems Feb 14, 2023

View reviewed changes

mithril-stm/src/multi_sig.rs Fixed Show fixed Hide fixed

mithril-stm/src/multi_sig.rs Fixed Show fixed Hide fixed

mithril-stm/src/multi_sig.rs Fixed Show fixed Hide fixed

comments resolved

5612aae

curiecrypt temporarily deployed to testing-preview February 14, 2023 16:06 — with GitHub Actions Inactive

curiecrypt requested a review from iquerejeta February 14, 2023 16:08

iquerejeta approved these changes Feb 14, 2023

View reviewed changes

final revisions for unsafe helpers

c83d6bd

curiecrypt temporarily deployed to testing-preview February 15, 2023 15:44 — with GitHub Actions Inactive

curiecrypt merged commit 4035e64 into main Feb 15, 2023

curiecrypt deleted the bls-transmute-helper branch February 15, 2023 15:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

helper for unsafe code #722

helper for unsafe code #722

curiecrypt commented Feb 1, 2023 •

edited

Loading

github-advanced-security bot left a comment

github-actions bot commented Feb 1, 2023 •

edited

Loading

iquerejeta left a comment

iquerejeta Feb 8, 2023

iquerejeta Feb 8, 2023

iquerejeta Feb 9, 2023

iquerejeta Feb 9, 2023

iquerejeta Feb 9, 2023

iquerejeta Feb 9, 2023

iquerejeta Feb 9, 2023

iquerejeta left a comment

iquerejeta Feb 14, 2023

iquerejeta Feb 14, 2023

iquerejeta Feb 14, 2023

helper for unsafe code #722

helper for unsafe code #722

Conversation

curiecrypt commented Feb 1, 2023 • edited Loading

Content

Pre-submit checklist

Issue(s)

github-advanced-security bot left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 1, 2023 • edited Loading

Test Results

iquerejeta left a 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

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

iquerejeta left a 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

curiecrypt commented Feb 1, 2023 •

edited

Loading

github-actions bot commented Feb 1, 2023 •

edited

Loading