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.

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

Replace usize with u32 in serialized/deserialized structs to avoid cross-platform issues #17

Merged
merged 17 commits into from
Sep 1, 2023

Conversation

olegnn
Copy link
Contributor

@olegnn olegnn commented Aug 24, 2023

No description provided.

@olegnn olegnn marked this pull request as draft August 24, 2023 21:52
@@ -27,7 +27,7 @@ use oblivious_transfer_protocols::{
pub struct Phase2<F: PrimeField, const KAPPA: u16, const STATISTICAL_SECURITY_PARAMETER: u16> {
pub id: ParticipantId,
/// Number of threshold signatures being generated in a single batch.
pub batch_size: usize,
pub batch_size: u64,
Copy link
Member

Choose a reason for hiding this comment

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

u16 or u32 would be enough.

@@ -14,7 +14,7 @@ use oblivious_transfer_protocols::ParticipantId;
pub struct Phase1<F: PrimeField, const SALT_SIZE: usize> {
pub id: ParticipantId,
/// Number of threshold signatures being generated in a single batch.
pub batch_size: usize,
pub batch_size: u64,
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as above.

@@ -21,7 +21,7 @@ use rayon::prelude::*;
#[derive(Clone, Debug, PartialEq, Eq, CanonicalSerialize, CanonicalDeserialize)]
pub struct RandomCommitment<G: AffineRepr> {
/// Maximum size of the witness vectors
pub max_size: usize,
pub max_size: u64,
Copy link
Member

Choose a reason for hiding this comment

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

u32 is sufficient and update type in init function signature as well.

@@ -18,7 +18,7 @@ use ark_std::{rand::RngCore, vec::Vec, UniformRand};
/// of attributes
#[derive(Clone, Debug)]
pub struct Credential<E: Pairing> {
pub max_attributes_per_commitment: usize,
pub max_attributes_per_commitment: u64,
Copy link
Member

@lovesh lovesh Aug 24, 2023

Choose a reason for hiding this comment

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

u32 is a good enough upper bound on the number of attributes. Also update type of max_attributes_per_commitment in issue_*

#[zeroize(skip)]
pub max_attributes_per_commitment: usize,
pub max_attributes_per_commitment: u64,
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as above regarding number of attributes

@@ -29,7 +29,7 @@ pub struct Credential<E: Pairing> {
/// of attributes
#[derive(Clone, Debug)]
pub struct CredentialWithoutOpenings<E: Pairing> {
pub max_attributes_per_commitment: usize,
pub max_attributes_per_commitment: u64,
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

@@ -264,17 +264,17 @@ impl<E: Pairing> Signature<E> {
),
DelegationError,
> {
if update_key.start_index > insert_at_index {
if update_key.start_index as usize > insert_at_index {
Copy link
Member

Choose a reason for hiding this comment

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

Update type of user_public_key and max_attributes_per_commitment in Signature::new* accordingly.

@@ -90,7 +90,7 @@ pub struct MaskedInputs<F: PrimeField>(#[serde_as(as = "Vec<ArkObjectBytes>")] p
Clone, Debug, PartialEq, CanonicalSerialize, CanonicalDeserialize, Serialize, Deserialize,
)]
pub struct Party1<F: PrimeField, const KAPPA: u16, const STATISTICAL_SECURITY_PARAMETER: u16> {
pub batch_size: usize,
pub batch_size: u64,
Copy link
Member

Choose a reason for hiding this comment

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

u32

@@ -109,7 +109,7 @@ pub struct Party1<F: PrimeField, const KAPPA: u16, const STATISTICAL_SECURITY_PA
Clone, Debug, PartialEq, CanonicalSerialize, CanonicalDeserialize, Serialize, Deserialize,
)]
pub struct Party2<F: PrimeField, const KAPPA: u16, const STATISTICAL_SECURITY_PARAMETER: u16> {
pub batch_size: usize,
pub batch_size: u64,
Copy link
Member

Choose a reason for hiding this comment

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

u32

@@ -236,7 +236,7 @@ impl OTExtensionReceiverSetup {

pub fn decrypt_random(&self, message_size: usize) -> Vec<Message> {
Copy link
Member

Choose a reason for hiding this comment

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

Change message_size to be u32 in decrypt* and encrypt*

@lovesh
Copy link
Member

lovesh commented Aug 24, 2023

Thanks for these changes. Can you please make the following changes

  1. Update message_count to u32 here , here, here, here and here.
  2. Change here and here to u32.
  3. We use this pattern of generating affine or projective elements by hashing with a label and a counter at a few places in the codebase. Can you move it to a macro in utils crate and just use the macro keeping the counter as u32 in the macro.

@olegnn olegnn changed the title Replace usize with u64 in serialized/deserialized structs to avoid cross-platform issues Replace usize with u32 in serialized/deserialized structs to avoid cross-platform issues Aug 25, 2023
#[macro_export]
macro_rules! affine_group_from_slices {
($($arg: expr),+) => {
$crate::hashing_utils::affine_group_elem_from_try_and_incr::<_, D>(&$crate::concat_slices!($($arg),+))
Copy link
Member

Choose a reason for hiding this comment

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

The counter should be in this maacro so that there is only 1 place where we convert it to bytes like this

macro_rules! affine_group_from_slices {
  (label: expr, prefix: expr, counter: expr) => {
   
  }
}

and use it like affine_group_from_slices!(label, b" : h_", i)

srs: &SetCommitmentSRS<E>,
) -> Result<(Self, Option<UpdateKey<E>>), DelegationError> {
let k = commitments.len();
let k = commitments
Copy link
Member

Choose a reason for hiding this comment

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

Thanks.

update_key: &UpdateKey<E>,
set_comm_srs: &SetCommitmentSRS<E>,
) -> Result<(Self, Option<UpdateKey<E>>), DelegationError> {
let rho = E::ScalarField::rand(rng);
let (new_sig, comm, o, new_uk) = self.signature.change_rel(
attributes.clone(),
self.attributes.len(),
self.attributes
Copy link
Member

Choose a reason for hiding this comment

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

Thanks.

}

/// Produces an iterator emitting `n` items `u32::to_le_bytes` of the counter starting from zero.
pub fn n_bytes_iter(n: u32) -> impl_indexed_iter!(<Item = [u8; 4]>) {
Copy link
Member

Choose a reason for hiding this comment

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

n_bytes_iter -> le_bytes_iter

@lovesh lovesh marked this pull request as ready for review September 1, 2023 08:56
@lovesh lovesh merged commit c39cafd into main Sep 1, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants