-
Notifications
You must be signed in to change notification settings - Fork 205
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
Composite Prime Moduli Sampling and Scaling Factor Calculations (#910 phase 2) #929
base: dev
Are you sure you want to change the base?
Conversation
src/pke/include/cryptocontext.h
Outdated
@@ -2372,11 +2387,19 @@ class CryptoContextImpl : public Serializable { | |||
void ModReduceInPlace(Ciphertext<Element>& ciphertext) const { | |||
ValidateCiphertext(ciphertext); | |||
|
|||
GetScheme()->ModReduceInPlace(ciphertext, BASE_NUM_LEVELS_TO_DROP); | |||
if (isCKKS(m_schemeId)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any CKKS-specific code should be implemented inside the folder for the CKKS scheme (not in global cryptocontext code)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
Ciphertext<Element> ciphertext = EvalMult(ciphertext1, ciphertext2); | ||
algo->KeySwitchInPlace(ciphertext, evalKey); | ||
ModReduceInPlace(ciphertext, BASE_NUM_LEVELS_TO_DROP); | ||
uint32_t levelsToDrop = BASE_NUM_LEVELS_TO_DROP; | ||
if (cc->getSchemeId() == CKKSRNS_SCHEME) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this code here rather then in CKKS files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
@@ -66,6 +66,7 @@ | |||
#include <algorithm> | |||
#include <unordered_map> | |||
#include <set> | |||
#include <iostream> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not what i meant. I would consider the code for DEBUG_KEY
as legacy, and we prefer not to expand it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I remove all the code under DEBUG_KEY? I only added the header because of pre-commit complaints. Let me know.
src/pke/include/cryptocontext.h
Outdated
* GetCompositeDegree: get composite degree of the current scheme crypto context. | ||
* @return integer value corresponding to composite degree | ||
*/ | ||
uint32_t GetCompositeDegreeFromCtxt() const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetCompositeDegreeFromCtxt() should be in either protected or private section of CryptoContextImpl as it is used only inside the class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed it.
std::cout << __FUNCTION__ << "::" << __LINE__ << " numPrimes: " << numPrimes << " qBound: " << qBound | ||
<< std::endl; | ||
} | ||
numPrimes *= compositeDegree; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a question: this multiplication numPrimes *= compositeDegree;
used to be performed for scalTech == COMPOSITESCALINGAUTO || scalTech == COMPOSITESCALINGMANUAL
only. Now it is done for any scalTech. Is this correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it is a CKKS class, yes, because on single scaling mode it will be multiplied by 1. So, I figured we don't really need the overhead of branch prediction.
const auto cryptoParams = std::dynamic_pointer_cast<CryptoParametersRNS>(ciphertextVec[0]->GetCryptoParameters()); | ||
uint32_t levelsToDrop = cryptoParams->GetCompositeDegree(); | ||
|
||
for (size_t i = 0; i < lim; i = i + 2) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this for
loop should be split: the logic to calculate ciphertextVec's index is complicated as there are 2 additional conditions to check in every iteration. a possible solution could look like this (we can discuss the changes):
size_t ctrIndex = 0;
size_t i = 0;
for (; i < (inSize - 1); i += 2) {
ciphertextMultVec[ctrIndex] = algo->EvalMultAndRelinearize(ciphertextVec[i], ciphertextVec[i + 1], evalKeys);
algo->ModReduceInPlace(ciphertextMultVec[ctrIndex++], levelsToDrop);
}
if(i < inSize) {
ciphertextMultVec[ctrIndex] = algo->EvalMultAndRelinearize(ciphertextVec[i], ciphertextVec[0], evalKeys);
algo->ModReduceInPlace(ciphertextMultVec[ctrIndex++], levelsToDrop);
i += 2;
}
for (; i < lim; i += 2) {
ciphertextMultVec[ctrIndex] =
algo->EvalMultAndRelinearize(ciphertextMultVec[i - inSize], ciphertextMultVec[i + 1 - inSize], evalKeys);
algo->ModReduceInPlace(ciphertextMultVec[ctrIndex++], levelsToDrop);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That works except that the second operand inside the special case is ciphertextMultVec[i + 1 - inSize]
.
size_t ctrIndex = 0; | ||
|
||
auto algo = ciphertextVec[0]->GetCryptoContext()->GetScheme(); | ||
const auto cc = ciphertextVec[0]->GetCryptoContext(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc is not used here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought I had removed that... Thanks for picking up. Compiler should have complained. Fixed it now.
@@ -46,6 +46,32 @@ CKKS implementation. See https://eprint.iacr.org/2020/1118 for details. | |||
|
|||
namespace lbcrypto { | |||
|
|||
Ciphertext<DCRTPoly> AdvancedSHECKKSRNS::EvalMultMany(const std::vector<Ciphertext<DCRTPoly>>& ciphertextVec, | |||
const std::vector<EvalKey<DCRTPoly>>& evalKeys) const { | |||
if (ciphertextVec.size() < 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if ciphertextVec.size() == 1
then the loop below is not executed. possibly if (ciphertextVec.size() < 2)
would suffice? Also, return ciphertextMultVec.back();
will cause a segfault if ciphertextVec.size() == 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation is borrowed from the base-advancedshe.cpp
. This fix will be propagated to the original implementation in base-advancedshe.cpp
as follows:
if (ciphertextVec.size() < 2)
OPENFHE_THROW("Input ciphertext vector size should be 1 or more");
if (ciphertextVec.size() == 1) return ciphertextVec[0];
uint32_t levelsToDrop = BASE_NUM_LEVELS_TO_DROP; | ||
if (cryptoParams->GetScalingTechnique() == COMPOSITESCALINGAUTO || | ||
cryptoParams->GetScalingTechnique() == COMPOSITESCALINGMANUAL) { | ||
const auto cryptoRNSParams = std::dynamic_pointer_cast<CryptoParametersRNS>(cryptoParams); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cryptoRNSParams is not needed, you could re-use cryptoParams
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what happened here. Also just above, the line const auto cc = ciphertext1->GetCryptoContext();
is not needed either.
No description provided.