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.

Sign up for GitHub

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

Rust: Add conversion between raw types and safe wrappers #248

Closed
wants to merge 5 commits into from
+98 −0
Closed

Rust: Add conversion between raw types and safe wrappers #248

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a47b956
add conversion between SecretKey and blst_scalar
dknopik Jan 10, 2025
e2b35fe
add conversion between Signature and $sig_aff
dknopik Jan 10, 2025
273616b
add reference versions of conversion
dknopik Jan 10, 2025
19e5f34
add remaining conversions
dknopik Jan 21, 2025
f9dbf4a
use `TryFrom` from `core` instead of `std`
dknopik Jan 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 98 additions & 0 deletions bindings/rust/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -750,6 +750,13 @@ macro_rules! sig_variant_impl {
pub fn from_bytes(sk_in: &[u8]) -> Result<Self, BLST_ERROR> {
SecretKey::deserialize(sk_in)
}

/// # Safety
///
/// passed [`blst_scalar`] must be ok as per [`blst_sk_check`]
pub unsafe fn from_scalar_unchecked(value: blst_scalar) -> Self {
SecretKey { value }
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd argue against this (as well as TryFrom below). Secret key interfaces should discourage copying of the secret key material, not encourage it(*). Well, one can say that this is inconsistent with the fact that SecretKey has Clone (auto-derived) trait. I'd argue that it was a mistake, unfortunately removing it constitutes a breaking change :-( As alternative I'd suggest a reference transmute. This naturally entails adding #repr(transparent) to the structure definition.

(*) Granted, Rust common practices don't appear to be really conducive to handling secret key material.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds reasonable. Would you rather change these methods to do the transmutation or leave it to the user?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm torn on this. I appreciate the fact that suggested from_scalar_unchecked is marked unsafe, because it makes the user pay attention. But it's impossible to mark impl<'a> From<&'a blst_scalar> for &'a SecretKey unsafe and as result it will appear safe to the user. On the other hand it's a convenience method that doesn't make a copy of the secret key material, hence there is no strong reason for making the user pay as much attention... So I suppose it's appropriate to internalize the unsafe transmute...

However, as it turns out it's impossible to have both From<&'a blst_scalar> and TryFrom<&'a blst_scalar> for &'a SecretKey. So I suppose one can add

        impl<'a> core::convert::TryFrom<&'a blst_scalar> for &'a SecretKey {
            type Error = BLST_ERROR;

            fn try_from(value: &'a blst_scalar) -> Result<Self, Self::Error> {
                unsafe {
                    if !blst_sk_check(value) {
                        return Err(BLST_ERROR::BLST_BAD_ENCODING);
                    }
                    Ok(transmute::<_, _>(value))
                }
            }
        }

And leave the option to perform the raw unchecked transmute to the user.

}

#[cfg(feature = "serde-secret")]
Expand All @@ -775,6 +782,25 @@ macro_rules! sig_variant_impl {
}
}

impl<'a> From<&'a SecretKey> for &'a blst_scalar {
fn from(sk: &'a SecretKey) -> Self {
&sk.value
}
}

impl core::convert::TryFrom<blst_scalar> for SecretKey {
type Error = BLST_ERROR;

fn try_from(value: blst_scalar) -> Result<Self, Self::Error> {
unsafe {
if !blst_sk_check(&value) {
return Err(BLST_ERROR::BLST_BAD_ENCODING);
}
}
Ok(SecretKey { value })
}
}

#[repr(transparent)]
#[derive(Default, Debug, Clone, Copy)]
pub struct PublicKey {
Expand Down Expand Up @@ -900,6 +926,24 @@ macro_rules! sig_variant_impl {
}
}

impl From<PublicKey> for $pk_aff {
fn from(pk: PublicKey) -> Self {
pk.point
}
}

impl<'a> From<&'a PublicKey> for &'a $pk_aff {
fn from(pk: &'a PublicKey) -> Self {
&pk.point
}
}

impl From<$pk_aff> for PublicKey {
fn from(point: $pk_aff) -> Self {
Self { point }
}
}

#[repr(transparent)]
#[derive(Debug, Clone, Copy)]
pub struct AggregatePublicKey {
Expand Down Expand Up @@ -1017,6 +1061,24 @@ macro_rules! sig_variant_impl {
}
}

impl From<AggregatePublicKey> for $pk {
fn from(pk: AggregatePublicKey) -> Self {
pk.point
}
}

impl<'a> From<&'a AggregatePublicKey> for &'a $pk {
fn from(pk: &'a AggregatePublicKey) -> Self {
&pk.point
}
}

impl From<$pk> for AggregatePublicKey {
fn from(point: $pk) -> Self {
Self { point }
}
}

#[repr(transparent)]
#[derive(Debug, Clone, Copy)]
pub struct Signature {
Expand Down Expand Up @@ -1468,6 +1530,24 @@ macro_rules! sig_variant_impl {
}
}

impl From<Signature> for $sig_aff {
fn from(sig: Signature) -> Self {
sig.point
}
}

impl<'a> From<&'a Signature> for &'a $sig_aff {
fn from(sig: &'a Signature) -> Self {
&sig.point
}
}

impl From<$sig_aff> for Signature {
fn from(point: $sig_aff) -> Self {
Self { point }
}
}

#[repr(transparent)]
#[derive(Debug, Clone, Copy)]
pub struct AggregateSignature {
Expand Down Expand Up @@ -1609,6 +1689,24 @@ macro_rules! sig_variant_impl {
}
}

impl From<AggregateSignature> for $sig {
fn from(sig: AggregateSignature) -> Self {
sig.point
}
}

impl<'a> From<&'a AggregateSignature> for &'a $sig {
fn from(sig: &'a AggregateSignature) -> Self {
&sig.point
}
}

impl From<$sig> for AggregateSignature {
fn from(point: $sig) -> Self {
Self { point }
}
}

impl MultiPoint for [PublicKey] {
type Output = AggregatePublicKey;

Expand Down