simplepir #279
simplepir #279
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @mihotel108, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request implements a simple Private Information Retrieval (PIR) protocol. It includes client and server implementations, along with utilities for generating random data, performing inner product calculations, and transmitting data over a network. The PR addresses issue #251.
Highlights
- CMake Build Configuration: A CMakeLists.txt file is added to build the SIMPLEPIR demo, linking necessary libraries like pthread and setting the C++ standard to C++11.
- Data Transmission: The
data_transmit.handdata_transmit.cppfiles implementSenderandReceiverclasses for sending and receiving__uint128_tdata over TCP sockets. This includes handling socket creation, connection, data size transmission, and data transmission/reception. - Random Data Generation: The
generate_rand.handgenerate_rand.cppfiles provide functions to generate 128-bit random numbers and vectors, used for creating the database and other parameters in the PIR protocol. - Inner Product Calculation: The
inner_product.handinner_product.cppfiles implement a fast inner product calculation modulo q, optimized with loop unrolling and modular reduction to improve performance. - PIR Client Implementation: The
pir_client.handpir_client.cppfiles define thePIRClientclass, which handles client-side operations such as receiving hints from the server, generating queries, receiving answers, and recovering the desired value. - PIR Server Implementation: The
pir_server.handpir_server.cppfiles define thePIRServerclass, responsible for setting up the database, generating hints, answering queries, and sending the results back to the client. - PIR Test Program: The
pir_test.cppfile sets up and runs a simple PIR experiment, creating server and client instances, generating random data, and executing the PIR protocol steps in separate threads.
Changelog
Click here to see the changelog
- experiment/pir/pps/simple/build/CMakeLists.txt
- Added a CMakeLists.txt file to build the SIMPLEPIR demo executable.
- Specifies the C++ standard and links the pthread library.
- experiment/pir/pps/simple/include/data_transmit.cpp
- Implemented
SenderandReceiverclasses for data transmission over TCP sockets. - Includes functions for sending and receiving data size and the data itself.
- Defines an overload for the << operator to print
__uint128_tvalues.
- Implemented
- experiment/pir/pps/simple/include/data_transmit.h
- Defined the
SenderandReceiverclasses. - Declared the
sendDataandreceiveDatamethods for data transmission.
- Defined the
- experiment/pir/pps/simple/include/generate_rand.cpp
- Implemented functions to generate 128-bit random numbers and vectors.
- Uses
std::random_deviceandstd::mt19937_64for random number generation.
- experiment/pir/pps/simple/include/generate_rand.h
- Declared functions for generating random 128-bit numbers and vectors.
- Includes necessary headers for random number generation.
- experiment/pir/pps/simple/include/inner_product.cpp
- Implemented a fast inner product calculation modulo q.
- Optimized with loop unrolling and modular reduction for performance.
- experiment/pir/pps/simple/include/inner_product.h
- Declared the
fast_inner_product_modqfunction. - Includes necessary headers for vector operations.
- Declared the
- experiment/pir/pps/simple/include/pir_client.cpp
- Implemented the
PIRClientclass for client-side PIR operations. - Includes methods for setup, query generation, answer reception, and value recovery.
- Implements matrix transpose for LWE matrix.
- Implemented the
- experiment/pir/pps/simple/include/pir_client.h
- Defined the
PIRClientclass with methods for PIR operations. - Includes parameters for the PIR protocol and data structures for storing intermediate values.
- Includes precompute_discrete_gaussian and sample_batch functions for gaussian sampling.
- Defined the
- experiment/pir/pps/simple/include/pir_server.cpp
- Implemented the
PIRServerclass for server-side PIR operations. - Includes methods for database setup, hint generation, query processing, and answer generation.
- Implemented the
- experiment/pir/pps/simple/include/pir_server.h
- Defined the
PIRServerclass with methods for PIR operations. - Includes parameters for the PIR protocol and data structures for storing the database and other values.
- Defined the
- experiment/pir/pps/simple/pir_test.cpp
- Sets up and runs a simple PIR experiment.
- Creates server and client instances, generates random data, and executes the PIR protocol steps in separate threads.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Trivia time!
What is the maximum value that can be stored in a __uint128_t variable?
Click here for the answer
The maximum value that can be stored in a `__uint128_t` variable is 2^128 - 1, or 340,282,366,920,938,463,463,374,607,431,768,211,455.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The pull request introduces a simple PIR (Private Information Retrieval) implementation. The code includes client and server components with basic functionalities like data transmission, random number generation, and inner product calculation. Overall, the code provides a starting point for understanding PIR concepts, but there are several areas that could be improved for correctness, efficiency, and security.
Summary of Findings
- Insecure random number generation: The code uses
std::random_deviceandstd::mt19937_64for random number generation, which might not be cryptographically secure. For security-sensitive applications, consider using a more robust random number generator. - Missing error handling: The code uses
assertfor error checking, which can lead to program termination in production. Replaceassertwith proper error handling mechanisms like exceptions or conditional checks with informative error messages. - Potential integer overflow: The code performs calculations with
__uint128_t, but there's a risk of integer overflow if the intermediate results exceed the maximum value of__uint128_t. Consider using modular arithmetic or other techniques to prevent overflow. - Hardcoded paths: The code uses hardcoded paths like
/root/pir/simple/include/..., which makes it difficult to deploy and reuse the code in different environments. Use relative paths or a configuration system to specify file locations.
Merge Readiness
The pull request introduces a basic PIR implementation, but it's not ready for merging due to several critical and high severity issues. These issues include insecure random number generation, missing error handling, potential integer overflows, and hardcoded paths. Addressing these issues is crucial for the code's correctness, security, and maintainability. I am unable to approve this pull request, and recommend that it not be merged until these issues are addressed. Users should have others review and approve this code before merging.
|
|
||
| void Sender::sendData(const std::vector<__uint128_t> &data) { | ||
| int sockfd = socket(AF_INET, SOCK_STREAM, 0); | ||
| assert(sockfd >= 0); |
There was a problem hiding this comment.
Using assert for error checking can lead to program termination in production environments. Consider using a more robust error handling mechanism, such as throwing an exception or returning an error code. What happens if sockfd is less than 0?
| assert(sockfd >= 0); | |
| if (sockfd < 0) { | |
| perror("Socket creation failed"); | |
| exit(EXIT_FAILURE); | |
| } |
| inet_pton(AF_INET, ip_.c_str(), &serv_addr.sin_addr); | ||
|
|
||
| int res = connect(sockfd, (struct sockaddr *)&serv_addr, sizeof(serv_addr)); | ||
| assert(res >= 0); |
There was a problem hiding this comment.
Similar to the socket creation, connect can also fail. Add error handling to check the return value of connect and handle potential connection errors. What happens if res is less than 0?
| assert(res >= 0); | |
| if (res < 0) { | |
| perror("Connection failed"); | |
| close(sockfd); | |
| exit(EXIT_FAILURE); | |
| } |
|
|
||
| Receiver::Receiver(int port) : port_(port) { | ||
| sockfd_ = socket(AF_INET, SOCK_STREAM, 0); | ||
| assert(sockfd_ >= 0); |
There was a problem hiding this comment.
As with the sender, add error handling to check the return value of socket and handle potential socket creation errors. What happens if sockfd_ is less than 0?
| assert(sockfd_ >= 0); | |
| if (sockfd_ < 0) { | |
| perror("Socket creation failed"); | |
| exit(EXIT_FAILURE); | |
| } |
| serv_addr.sin_port = htons(port_); | ||
|
|
||
| int res = bind(sockfd_, (struct sockaddr *)&serv_addr, sizeof(serv_addr)); | ||
| assert(res >= 0); |
There was a problem hiding this comment.
Add error handling to check the return value of bind and handle potential binding errors. What happens if res is less than 0?
| assert(res >= 0); | |
| if (res < 0) { | |
| perror("Bind failed"); | |
| close(sockfd_); | |
| exit(EXIT_FAILURE); | |
| } |
| assert(res >= 0); | ||
|
|
||
| res = listen(sockfd_, 5); | ||
| assert(res >= 0); |
There was a problem hiding this comment.
Add error handling to check the return value of listen and handle potential listening errors. What happens if res is less than 0?
| assert(res >= 0); | |
| if (res < 0) { | |
| perror("Listen failed"); | |
| close(sockfd_); | |
| exit(EXIT_FAILURE); | |
| } |
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| #include "/root/pir/simple/include/pir_client.h" |
There was a problem hiding this comment.
Avoid using absolute paths. Instead, use relative paths to improve portability. For example, change /root/pir/simple/include/pir_client.h to pir_client.h.
| #include "/root/pir/simple/include/pir_client.h" | |
| #include "pir_client.h" |
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| #include "/root/pir/simple/include/pir_server.h" |
There was a problem hiding this comment.
Avoid using absolute paths. Instead, use relative paths to improve portability. For example, change /root/pir/simple/include/pir_server.h to pir_server.h.
| #include "/root/pir/simple/include/pir_server.h" | |
| #include "pir_server.h" |
| #include <thread> | ||
| #include <vector> | ||
|
|
||
| #include "/root/pir/simple/include/generate_rand.h" |
There was a problem hiding this comment.
Avoid using absolute paths. Instead, use relative paths to improve portability. For example, change /root/pir/simple/include/generate_rand.h to generate_rand.h.
| #include "/root/pir/simple/include/generate_rand.h" | |
| #include "generate_rand.h" |
| #include "/root/SimplePIR/data_transmit.h" | ||
| #include "/root/SimplePIR/generate_rand.h" | ||
| #include "/root/SimplePIR/inner_product.h" | ||
| #include "/root/SimplePIR/regev_lwe.h" |
There was a problem hiding this comment.
Avoid using absolute paths. Instead, use relative paths to improve portability. For example, change /root/SimplePIR/data_transmit.h to data_transmit.h.
| #include "/root/SimplePIR/data_transmit.h" | |
| #include "/root/SimplePIR/generate_rand.h" | |
| #include "/root/SimplePIR/inner_product.h" | |
| #include "/root/SimplePIR/regev_lwe.h" | |
| #include "data_transmit.h" | |
| #include "generate_rand.h" | |
| #include "inner_product.h" | |
| #include "regev_lwe.h" |
| #include "/root/SimplePIR/data_transmit.h" | ||
| #include "/root/SimplePIR/generate_rand.h" | ||
| #include "/root/SimplePIR/inner_product.h" | ||
|
|
There was a problem hiding this comment.
Avoid using absolute paths. Instead, use relative paths to improve portability. For example, change /root/SimplePIR/data_transmit.h to data_transmit.h.
| #include "/root/SimplePIR/data_transmit.h" | |
| #include "/root/SimplePIR/generate_rand.h" | |
| #include "/root/SimplePIR/inner_product.h" | |
| #include "data_transmit.h" | |
| #include "generate_rand.h" | |
| #include "inner_product.h" |
There was a problem hiding this comment.
请使用bazel 的方式来组织和管理代码，可参考psi repo下其他算法的实现风格和bazel 的基本使用方式。不要使用cmake/make 。
|
recheck |
There was a problem hiding this comment.
不要上传这种和源码无关的配置文件，此外，目前的代码组织方式还是基于CMake，请参考psi repo里任意一个算法实现的目录，用BUILD.bazel 来组织和管理代码。
Fixed #251