Skip to content

Remove UnsafeCell (Attempt 2)#88

Closed
davxy wants to merge 15 commits intoparitytech:masterfrom
davxy:remote-unsafe-cell2
Closed

Remove UnsafeCell (Attempt 2)#88
davxy wants to merge 15 commits intoparitytech:masterfrom
davxy:remote-unsafe-cell2

Conversation

@davxy
Copy link
Member

@davxy davxy commented Feb 29, 2024

Superseeds #87

  • UnsafeCell removal
  • Expose parallel feature

@davxy davxy requested a review from burdges February 29, 2024 09:34
#[repr(transparent)]
pub struct SecretScalar<F: PrimeField>(UnsafeCell<[F; 2]>);
#[derive(Zeroize)]
pub struct SecretSplit<F: PrimeField>([F; 2]);
Copy link
Member 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
Member 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. ;)


/// 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
Member 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
Member 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

@davxy davxy closed this Dec 10, 2024
@davxy davxy deleted the remote-unsafe-cell2 branch December 10, 2024 10:15
@burdges
Copy link
Collaborator

burdges commented Dec 11, 2024

Appears I never fixed this. I'd basically come around to a reasonable compromize, but I never made the changes?!? fuck

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