feat: centralize encoding in shortint #1826
Conversation
The plaintext encoding in shortint was duplicated all over the code This commit centralize the encoding used for shortint, so that if an encoding fix is needed there should be one place to do it.
| ciphertext_modulus: self.ciphertext_modulus, | ||
| message_modulus: self.message_modulus, | ||
| carry_modulus: self.carry_modulus, | ||
| padding_bit, |
There was a problem hiding this comment.
Shouldn't we hardcode PaddingBit::Yes here
Or otherwise store it in CudaServerKey
| let decoded = ShortintEncoding { | ||
| ciphertext_modulus: self.ct.ciphertext_modulus(), | ||
| message_modulus: self.message_modulus, | ||
| carry_modulus: self.carry_modulus, | ||
| padding_bit: PaddingBit::Yes, | ||
| } |
There was a problem hiding this comment.
We could add a method encoding to Ciphertext
| @@ -241,11 +249,13 @@ pub(crate) fn unchecked_create_trivial_with_lwe_size( | |||
| pbs_order: PBSOrder, | |||
| ciphertext_modulus: CiphertextModulus, | |||
| ) -> Ciphertext { | |||
There was a problem hiding this comment.
This function could take an ShortintEncoding as input
I don't know this is a better though
There was a problem hiding this comment.
I don't think it's better as to create the ciphertext, the metadata are needed
| @@ -92,6 +92,13 @@ where | |||
| assert_eq!(accumulator.polynomial_size(), polynomial_size); | |||
| assert_eq!(accumulator.glwe_size(), glwe_size); | |||
|
|
|||
There was a problem hiding this comment.
This function could take an ShortintEncoding as input
I don't know this is a better though
| @@ -175,6 +179,14 @@ where | |||
| assert_eq!(accumulator.polynomial_size(), polynomial_size); | |||
| assert_eq!(accumulator.glwe_size(), glwe_size); | |||
|
|
|||
| // Value of the shift we multiply our messages by | |||
| #[derive(Copy, Clone, Debug, PartialEq, Eq)] | ||
| pub(crate) enum PaddingBit { | ||
| No = 0, | ||
| Yes = 1, | ||
| } | ||
|
|
||
| fn compute_delta( | ||
| ciphertext_modulus: CiphertextModulus, | ||
| message_modulus: MessageModulus, | ||
| carry_modulus: CarryModulus, | ||
| padding_bit: PaddingBit, | ||
| ) -> u64 { | ||
| match ciphertext_modulus.kind() { | ||
| CiphertextModulusKind::Native => { | ||
| (1u64 << (u64::BITS - 1 - padding_bit as u32)) / (carry_modulus.0 * message_modulus.0) | ||
| * 2 | ||
| } | ||
| CiphertextModulusKind::Other | CiphertextModulusKind::NonNativePowerOfTwo => { | ||
| ciphertext_modulus.get_custom_modulus() as u64 | ||
| / (carry_modulus.0 * message_modulus.0) | ||
| / if padding_bit == PaddingBit::Yes { 2 } else { 1 } | ||
| * 2 | ||
| } | ||
| } | ||
| } | ||
|
|
||
| pub(crate) struct ShortintEncoding { | ||
| pub(crate) ciphertext_modulus: CiphertextModulus, | ||
| pub(crate) message_modulus: MessageModulus, | ||
| pub(crate) carry_modulus: CarryModulus, | ||
| pub(crate) padding_bit: PaddingBit, | ||
| } | ||
|
|
||
| impl ShortintEncoding { | ||
| pub(crate) fn delta(&self) -> u64 { | ||
| compute_delta( | ||
| self.ciphertext_modulus, | ||
| self.message_modulus, | ||
| self.carry_modulus, | ||
| self.padding_bit, | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| impl ShortintEncoding { | ||
| pub(crate) fn from_parameters( | ||
| params: impl Into<ShortintParameterSet>, | ||
| padding_bit: PaddingBit, | ||
| ) -> Self { | ||
| let params = params.into(); | ||
| Self { | ||
| ciphertext_modulus: params.ciphertext_modulus(), | ||
| message_modulus: params.message_modulus(), | ||
| carry_modulus: params.carry_modulus(), | ||
| padding_bit, | ||
| } | ||
| } | ||
|
|
||
| pub(crate) fn encode(&self, value: Cleartext<u64>) -> Plaintext<u64> { | ||
| // TODO store delta in the encoder directly ? | ||
| let delta = compute_delta( | ||
| self.ciphertext_modulus, | ||
| self.message_modulus, | ||
| self.carry_modulus, | ||
| self.padding_bit, | ||
| ); | ||
|
|
||
| Plaintext(value.0.wrapping_mul(delta)) | ||
| } | ||
|
|
||
| pub(crate) fn decode(&self, value: Plaintext<u64>) -> Cleartext<u64> { | ||
| assert!(self.ciphertext_modulus.is_native_modulus()); | ||
| let delta = self.delta(); | ||
|
|
||
| // The bit before the message | ||
| let rounding_bit = delta >> 1; | ||
|
|
||
| // Compute the rounding bit | ||
| let rounding = (value.0 & rounding_bit) << 1; | ||
|
|
||
| Cleartext(value.0.wrapping_add(rounding) / delta) | ||
| } | ||
| } |
There was a problem hiding this comment.
Could be in an encoding module
| let encoding = ShortintEncoding { | ||
| ciphertext_modulus: parameters.ciphertext_modulus, | ||
| message_modulus: parameters.message_modulus, | ||
| carry_modulus: parameters.carry_modulus, | ||
| padding_bit: PaddingBit::Yes, | ||
| }; |
There was a problem hiding this comment.
Could be a method of CompactPublicKeyEncryptionParameters
| let delta = ShortintEncoding { | ||
| ciphertext_modulus: self.parameters.ciphertext_modulus, | ||
| message_modulus: self.parameters.message_modulus, | ||
| carry_modulus: self.parameters.carry_modulus, | ||
| padding_bit: PaddingBit::Yes, | ||
| } | ||
| .delta(); |
There was a problem hiding this comment.
Could be a method of the type of self.parameters
| pub(crate) fn from_parameters( | ||
| params: impl Into<ShortintParameterSet>, | ||
| padding_bit: PaddingBit, | ||
| ) -> Self { |
There was a problem hiding this comment.
When using the classical or multibit PBS, I think we should force the PaddingBit to be set.
But it looks like it would need bigger changes
Maybe we could think of removing no padding bit encoding and wop from shortint
There was a problem hiding this comment.
Yeah I also think the no padding bit 'support' should go away as we don't really test it, its not something we use either
The plaintext encoding in shortint was duplicated all over the code
This commit centralize the encoding used for shortint, so that if an encoding fix is needed there should be one place to do it.