-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add Secure Array #59
base: main
Are you sure you want to change the base?
Add Secure Array #59
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
|
MacOs test failing: |
Great work! Once this PR is ready, do we then still need a separate const SecureVector{Backend, DataT} = SecureArray{Backend, 1, DataT}
const SecureMatrix{Backend, DataT} = SecureArray{Backend, 2, DataT} ? |
We can do it, but I have to adapt a bit a code. |
Then let's maybe discuss this on Friday? I believe that if it will allow is to have just one and not three different implementations for 1D, 2D, and nD arrays, it is probably going to be worth it. The only reason that might be against it is if the nD code is so much more difficult to understand that it becomes very hard to add new features even to 1D arrays. |
Codecov fails, because it tries to count also
And counts them as missing. I don't think it is a problem, but we could also explicitly exclude such lines from codecov, but it would be dirtier. As clear from this comment, I have also parallelized loops over ciphertexts. Since now |
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.
First round of reviews. I haven't checked src/unencrypted.jl
or src/openfhe.jl
at all yet.
Co-authored-by: Michael Schlottke-Lakemper <[email protected]>
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've finally taken a look at the remaining files as well, though I cannot really vouch for most of the code (since I don't fully understand the implementation for shifts/rotations). I mainly focused on getting the code look "Julian". Whether the implementation is correct or can be simplified, I don't know and I rely on you having implemented good tests :-)
@@ -5,7 +5,7 @@ function simple_real_numbers(context) | |||
public_key, private_key = generate_keys(context) | |||
|
|||
init_multiplication!(context, private_key) | |||
init_array_rotation!(context, private_key, [-1, 2], (8,)) | |||
init_rotation!(context, private_key, (8,), -1, 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.
Can we add a special case for 1D rotations that allow a user to pass the shape
argument also just as an integer? That is, such that one can do
init_rotation!(context, private_key, (8,), -1, 2) | |
init_rotation!(context, private_key, 8, -1, 2) |
in this case? or would this be too confusing/complicated to implement?
function get_index(context::SecureContext{<:OpenFHEBackend}, shape::Tuple{Integer}, | ||
rotation_index) |
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.
get_index
is very close to getindex
but does something completely different. Also, it is customary in Julia to not add types in a function signature unless really needed for dispatch. Thus I suggest to change this interface to something like the following:
function get_index(context::SecureContext{<:OpenFHEBackend}, shape::Tuple{Integer}, | |
rotation_index) | |
function compute_rotation_indices(context, shape, shifts) |
cc = get_crypto_context(context) | ||
OpenFHE.EvalRotateKeyGen(cc, private_key.private_key, -shifts) | ||
# Get indexes for precompilation | ||
indexes = get_index(context, shape, rotation_index) |
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.
indexes = get_index(context, shape, rotation_index) | |
indices = get_index(context, shape, rotation_index) |
indices
is the correct plural of index
# number of ciphertexts in array | ||
n_ciphertexts = Int(ceil(array_length/capacity)) | ||
# store all indexes to enable | ||
indexes = Int[] |
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.
indexes = Int[] | |
indices = Int[] |
as above
indexes | ||
end | ||
|
||
function get_index(context::SecureContext{<:OpenFHEBackend}, shape::NTuple{N, Integer}, |
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.
What's different to the get_index
from above? IMHO it would be good to comment on that, especially since some of the code seems to be duplicated here
function PlainArray(data::Vector{<:Real}, context::SecureContext{<:OpenFHEBackend}, | ||
shape::Tuple) |
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 lost track, but is there also a
function PlainArray(data::Array{<:Real, N}, context) where {N}
...
end
constructor that allows one to just pass in an array and the shape is figured out automatically?
function subtract(sa::SecureArray{<:OpenFHEBackend}, pa::PlainArray{<:OpenFHEBackend}) | ||
cc = get_crypto_context(sa) | ||
ciphertexts = Vector{OpenFHE.Ciphertext}(undef, length(sa.data)) | ||
for i in range(1, length(sa.data)) |
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 a highly unusual style for loop iterations. Can you please use
for i in range(1, length(sa.data)) | |
for i in 1:length(sa.data) |
here and everywhere else? Even more Julian would be to use
for i in range(1, length(sa.data)) | |
for i in eachindex(sa.data) |
But please do not iterate over ranges, this is not Python 😅
first = abs(shift) + 1 | ||
last = length(sv) | ||
mask_rotated[first:last] .= 1 | ||
function rotate(sa::SecureArray{<:OpenFHEBackend, 1}, shift) |
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.
Here you use shift
again. I suggest to use a consistent way of differentiating between the user-visible "shift" and the internal, FHE ciphertext "shift". Maybe "shift" or "shifts" for everything that behaves like the "shift" in Base.circshfit
and "rotation_index" for everything related to the actual ciphertext shift (or whatever OpenFHE is using for this).
I introduce
SecureArray
andPlainArray
, so that an array is stored in a vector of ciphertexts/plaintexts, enabling de facto arbitrary big arrays in fixed ring dimension. All operations should work now. 1D rotations are also now supported. The only part still missing is nD rotations for n>1. But I already have ideas, and it should not be too difficult.Please don't merge before I introduce nD rotations.
TODO for me: