Skip to content
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

Concrete Keygen in Rust #1198

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Concrete Keygen in Rust #1198

wants to merge 7 commits into from

Conversation

youben11
Copy link
Member

No description provided.

@cla-bot cla-bot bot added the cla-signed label Jan 16, 2025
@youben11 youben11 force-pushed the feat/keygen-from-tfhers branch from 2fce9dc to c59d101 Compare January 16, 2025 14:31
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Benchmark

Benchmark suite Current: ea5d0fd Previous: 8704ff8 Ratio
v0 PBS table generation 58966332 ns/iter (± 1773888) 58734887 ns/iter (± 505931) 1.00
v0 PBS simulate dag table generation 37935830 ns/iter (± 490235) 37418545 ns/iter (± 362974) 1.01
v0 WoP-PBS table generation 53801528 ns/iter (± 577218) 53928255 ns/iter (± 1171398) 1.00

This comment was automatically generated by workflow using github-action-benchmark.

Copy link
Member

@BourgerieQuentin BourgerieQuentin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me, but I think we should not name that concrete_rust

.def(
"serialize",
[](KeysetInfo &keySetInfo) {
auto keySetInfoSerialize = [](KeysetInfo &keysetInfo) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are you using a lambda here?

f"{os.path.dirname(os.path.abspath(__file__))}"
"/../../../../tools/concrete-rust/target/release/concrete-rust-keygen"
)
os.system(f"{bin_path} {keyinfo_path} {keyset_path}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check returns?

///
/// # Returns
/// A Cap'n Proto message containing the generated keyset.
fn generate_keyset_message(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe outline some logics here?

///
/// # Panics
/// This function panics if it fails to open the keyset info file or create the keyset file.
pub fn generate_keyset(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think method with path should not be in the lib

@BourgerieQuentin
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants