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

Remove UnsafeCell (Attempt 2) #88

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Conversation

davxy
Copy link
Collaborator

@davxy davxy commented Feb 29, 2024

Superseeds #87

  • UnsafeCell removal
  • Expose parallel feature

@@ -48,145 +82,180 @@ impl<R: RngCore+CryptoRng> XofReader for Rng2Xof<R> {
// TODO: We split additively right now, but would a multiplicative splitting
// help against rowhammer attacks on the secret key?
#[repr(transparent)]
pub struct SecretScalar<F: PrimeField>(UnsafeCell<[F; 2]>);
#[derive(Zeroize)]
pub struct SecretSplit<F: PrimeField>([F; 2]);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure if we want to make this public so that the user can eventually keep it and manually invoke resplit if required.

If you want to make it private then some methods can be removed as well... as are not used

@davxy
Copy link
Collaborator Author

davxy commented Mar 8, 2024

@burdges do you think this is good to be merged?

@burdges
Copy link
Collaborator

burdges commented Mar 13, 2024

I see, 2af5bd2 reverts 4c7eba5. I'll go through the finished change, not the individual commits. ;)

@@ -86,7 +84,7 @@ pub struct SecretKey<K: AffineRepr> {
pub(crate) thin: ThinVrf<K>,

/// Secret key represented as a scalar.
pub(crate) key: SecretScalar<<K as AffineRepr>::ScalarField>,
pub(crate) key: SecretScalar<K::ScalarField>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this working purely because <K: AffineRepr> exists in the struct? Is this a new rust feature?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

K is bound only by the AffineRepr trait. There are no ambiguities for the ScalarField associated type

@davxy
Copy link
Collaborator Author

davxy commented Mar 13, 2024

I see, 2af5bd2 reverts 4c7eba5. I'll go through the finished change, not the individual commits. ;)

Yeah. I decided to revert because that modification was using the secret for the computation: res = (A+B)*c = secret*c).
The original code was better as it doesn't reconstruct the secret to compute the result res = (A*c + B*c).

Doing the computation with the reconstructed secret makes the splitting trick pointless

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.

2 participants