feat: add secret and evaluation key serialization#23
Merged
Conversation
- Expose Context serialize/deserialize for standard-form keys; re-prepare after load - Depend on cedoor/poulpy branch feat/serialization-key-types for Reader/Writer support - Add key round-trip integration test; check off README milestone item for #13
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR updates PoulPy git dependencies to a fixed commit, adds deterministic seed-based key generation with Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The PR involves multiple heterogeneous changes across core APIs (encryption signature, key generation, serialization), new public structs and methods, versioned blob formats with strict validation, caching mechanisms, and fundamental architectural shifts in scratch allocation. While individual changes follow established patterns, the breadth of affected files and significance of API modifications demand careful review of serialization formats, seed-based generation correctness, prepared-form caching semantics, and backward compatibility. Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/key_serialization.rs (1)
3-23: Exercise deserialization in a freshContext.This currently round-trips through the same instance that generated the keys, so it doesn't really prove the "load later and rebuild prepared state" path. Deserializing in a second
Context::new(Params::unsecure())would cover the contract this PR adds.Suggested change
#[test] fn secret_and_evaluation_keys_roundtrip_through_blob_format() { - let params = Params::unsecure(); - let mut ctx = Context::new(params); + let mut ctx = Context::new(Params::unsecure()); let (sk, ek) = ctx.keygen(); let sk_blob = ctx.serialize_secret_key(&sk).expect("serialize sk"); let ek_blob = ctx.serialize_evaluation_key(&ek).expect("serialize ek"); - let sk2 = ctx + let mut ctx2 = Context::new(Params::unsecure()); + let sk2 = ctx2 .deserialize_secret_key(&sk_blob) .expect("deserialize sk"); - let ek2 = ctx + let ek2 = ctx2 .deserialize_evaluation_key(&ek_blob) .expect("deserialize ek"); - let a = ctx.encrypt::<u32>(11, &sk2); - let b = ctx.encrypt::<u32>(22, &sk2); - let c = ctx.add(&a, &b, &ek2); - assert_eq!(ctx.decrypt(&c, &sk2), 33); + let a = ctx2.encrypt::<u32>(11, &sk2); + let b = ctx2.encrypt::<u32>(22, &sk2); + let c = ctx2.add(&a, &b, &ek2); + assert_eq!(ctx2.decrypt(&c, &sk2), 33); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/key_serialization.rs` around lines 3 - 23, The test secret_and_evaluation_keys_roundtrip_through_blob_format currently deserializes keys into the same Context instance that generated them; change it to create a fresh Context::new(Params::unsecure()) for deserialization to exercise loading into a new runtime. Specifically, after calling ctx.serialize_secret_key and ctx.serialize_evaluation_key, instantiate a new Context (e.g., let mut ctx2 = Context::new(Params::unsecure())) and call ctx2.deserialize_secret_key and ctx2.deserialize_evaluation_key to get sk2/ek2, then use ctx2.encrypt, ctx2.add and ctx2.decrypt to validate the round-trip.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Cargo.toml`:
- Around line 12-16: Replace the mutable branch refs for the poulpy dependencies
with fixed commit SHAs: update each dependency entry (poulpy-core,
poulpy-schemes, poulpy-hal, poulpy-cpu-ref, poulpy-cpu-avx) to use rev =
"<commit-sha>" instead of branch = "feat/serialization-key-types" so Cargo pulls
a specific tested commit; ensure the same SHA is used for all poulpy crates that
must match, and keep optional = true on poulpy-cpu-avx if needed.
In `@src/context.rs`:
- Around line 371-382: The documentation for deserialize_secret_key says
truncated blobs should return io::ErrorKind::InvalidData but the implementation
currently returns io::ErrorKind::UnexpectedEof for the empty/truncated
early-return and a later truncated-path; update those error constructions in the
deserialize_secret_key function to use io::ErrorKind::InvalidData (and keep the
same error messages) so the runtime behavior matches the doc comment, and ensure
both the initial bytes.split_first() branch and the later truncated-read branch
use InvalidData consistently.
- Around line 359-455: The blob header must be extended to include a key-kind
discriminator and a stable params/backend fingerprint and validated on load:
update serialize_secret_key and serialize_evaluation_key to write a header (e.g.
[KEY_BLOB_VERSION][KEY_KIND][PARAMS_FINGERPRINT][payload]) instead of just the
version, and update deserialize_secret_key and deserialize_evaluation_key to
read and verify KEY_BLOB_VERSION, check the key-kind matches (secret vs
evaluation), compute a stable fingerprint from this Context's Params and backend
(module) and compare it to the header fingerprint, and return InvalidData on
mismatch; also introduce a stable fingerprint helper (e.g. a fn
params_fingerprint(&self) -> [u8;N]) referenced by the four functions and bump
or document KEY_BLOB_VERSION to reflect the new header format.
---
Nitpick comments:
In `@tests/key_serialization.rs`:
- Around line 3-23: The test
secret_and_evaluation_keys_roundtrip_through_blob_format currently deserializes
keys into the same Context instance that generated them; change it to create a
fresh Context::new(Params::unsecure()) for deserialization to exercise loading
into a new runtime. Specifically, after calling ctx.serialize_secret_key and
ctx.serialize_evaluation_key, instantiate a new Context (e.g., let mut ctx2 =
Context::new(Params::unsecure())) and call ctx2.deserialize_secret_key and
ctx2.deserialize_evaluation_key to get sk2/ek2, then use ctx2.encrypt, ctx2.add
and ctx2.decrypt to validate the round-trip.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 04045adc-871a-49eb-a911-94b6d7175f92
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
Cargo.tomlREADME.mdsrc/context.rssrc/keys.rstests/key_serialization.rs
- Mirror poulpy-schemes test_suite layouts (n_glwe 256, rank 2) for faster integration tests - Use Params::test() in key_serialization and bdd_parallel tests
- Add `serialize_keys` example for Params::test key blobs - Gitignore generated `tests/fixtures/` - bdd_parallel: shared `Params`, keygen per context for 1 vs 2 eval threads - key_serialization: rename round-trip test, inline `Params::test()` - Mark roadmap item #19 complete in README
- Remove `tests/fixtures/` gitignore entry - Point module docs at `./out` and describe out-of-tree / inspection use - Remove stale reference to test `include_bytes!` paths
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/context.rs (2)
469-476:⚠️ Potential issue | 🟡 MinorAlign empty/truncated blob errors with documented
InvalidData.Docs on Line 469 and Line 518 say
InvalidData, but Line 474 and Line 523 returnUnexpectedEof.Minimal fix
return Err(io::Error::new( - io::ErrorKind::UnexpectedEof, + io::ErrorKind::InvalidData, "empty secret key blob", ));return Err(io::Error::new( - io::ErrorKind::UnexpectedEof, + io::ErrorKind::InvalidData, "empty evaluation key blob", ));Also applies to: 518-525
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context.rs` around lines 469 - 476, The documented return kind is io::ErrorKind::InvalidData but the functions return UnexpectedEof for empty/truncated blobs; update the error construction in deserialize_secret_key (where bytes.split_first() is None) to use io::ErrorKind::InvalidData with the same message ("empty secret key blob") and make the equivalent change in the other deserialization function around lines 518–525 (e.g., deserialize_public_key or its counterpart) so both empty/truncated-blob branches return InvalidData instead of UnexpectedEof.
457-549:⚠️ Potential issue | 🟠 MajorMake key blobs self-describing before payload parse.
Line 459/509 only writes a version byte, so a wrong key kind or a same-sized blob from incompatible params can still be attempted under this context. Add a key-kind discriminator + stable params fingerprint in the header and validate both during deserialize.
Suggested direction
+const KEY_KIND_SECRET: u8 = 1; +const KEY_KIND_EVAL: u8 = 2; +const PARAMS_FINGERPRINT_LEN: usize = 32; + +fn params_fingerprint(&self) -> [u8; PARAMS_FINGERPRINT_LEN] { + // stable hash over params + backend identifier +} pub fn serialize_secret_key(&self, sk: &SecretKey) -> io::Result<Vec<u8>> { let mut out = Vec::new(); out.push(KEY_BLOB_VERSION); + out.push(KEY_KIND_SECRET); + out.extend_from_slice(&self.params_fingerprint()); sk.sk_glwe.write_to(&mut out)?; sk.sk_lwe.write_to(&mut out)?; Ok(out) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context.rs` around lines 457 - 549, The blobs written by serialize_secret_key and serialize_evaluation_key currently only push KEY_BLOB_VERSION and thus are ambiguous; change both serializers to write a small header containing (version byte KEY_BLOB_VERSION, a 1-byte key-kind discriminator e.g. "S" for Secret vs "E" for Evaluation, and a deterministic params fingerprint derived from self.params) before the payload, and update deserialize_secret_key and deserialize_evaluation_key to first parse and validate that header (check version == KEY_BLOB_VERSION, key-kind matches the expected kind in each function, and fingerprint matches the current Context params) and return io::Error::InvalidData if any check fails; keep the existing payload read_from logic and adjust error messages to reflect header validation failures.
🧹 Nitpick comments (1)
examples/serialize_keys.rs (1)
88-91: Avoid panic path in anio::Result-returning CLI.Use error propagation instead of
expect(...)so failures return consistently rather than abort.Suggested change
- let sk_blob = ctx.serialize_secret_key(&sk).expect("serialize secret key"); - let ek_blob = ctx - .serialize_evaluation_key(&ek) - .expect("serialize evaluation key"); + let sk_blob = ctx.serialize_secret_key(&sk)?; + let ek_blob = ctx.serialize_evaluation_key(&ek)?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/serialize_keys.rs` around lines 88 - 91, The code is using expect(...) on ctx.serialize_secret_key and ctx.serialize_evaluation_key which panics on error; change these to propagate errors instead (use the ? operator or map_err to convert to the CLI's error type) so failures return an io::Result rather than aborting. Update the surrounding function (e.g., main) signature to return Result<(), Box<dyn std::error::Error>> or io::Result<()> as appropriate, replace the expect calls for sk_blob and ek_blob with error-propagating calls using ?, and ensure any error type conversions are handled where needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/serialize_keys.rs`:
- Around line 93-94: The secret and evaluation key files are created with
std::fs::write which uses default umask-permissions; change creation so files
are created with owner-only permissions (0o600). Replace the
std::fs::write(&secret_key, sk_blob) and std::fs::write(&evaluation_key,
ek_blob) calls with explicit file creation using std::fs::OpenOptions (or
similar) and on Unix use std::os::unix::fs::OpenOptionsExt::mode(0o600) before
writing sk_blob and ek_blob to the files (or alternatively write then call
std::fs::set_permissions with Permissions::from_mode(0o600)); apply this for
both secret_key and evaluation_key to ensure owner-only access.
---
Duplicate comments:
In `@src/context.rs`:
- Around line 469-476: The documented return kind is io::ErrorKind::InvalidData
but the functions return UnexpectedEof for empty/truncated blobs; update the
error construction in deserialize_secret_key (where bytes.split_first() is None)
to use io::ErrorKind::InvalidData with the same message ("empty secret key
blob") and make the equivalent change in the other deserialization function
around lines 518–525 (e.g., deserialize_public_key or its counterpart) so both
empty/truncated-blob branches return InvalidData instead of UnexpectedEof.
- Around line 457-549: The blobs written by serialize_secret_key and
serialize_evaluation_key currently only push KEY_BLOB_VERSION and thus are
ambiguous; change both serializers to write a small header containing (version
byte KEY_BLOB_VERSION, a 1-byte key-kind discriminator e.g. "S" for Secret vs
"E" for Evaluation, and a deterministic params fingerprint derived from
self.params) before the payload, and update deserialize_secret_key and
deserialize_evaluation_key to first parse and validate that header (check
version == KEY_BLOB_VERSION, key-kind matches the expected kind in each
function, and fingerprint matches the current Context params) and return
io::Error::InvalidData if any check fails; keep the existing payload read_from
logic and adjust error messages to reflect header validation failures.
---
Nitpick comments:
In `@examples/serialize_keys.rs`:
- Around line 88-91: The code is using expect(...) on ctx.serialize_secret_key
and ctx.serialize_evaluation_key which panics on error; change these to
propagate errors instead (use the ? operator or map_err to convert to the CLI's
error type) so failures return an io::Result rather than aborting. Update the
surrounding function (e.g., main) signature to return Result<(), Box<dyn
std::error::Error>> or io::Result<()> as appropriate, replace the expect calls
for sk_blob and ek_blob with error-propagating calls using ?, and ensure any
error type conversions are handled where needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 797db3e2-eab3-450c-9f08-89206acdef38
📒 Files selected for processing (5)
README.mdexamples/serialize_keys.rssrc/context.rstests/bdd_parallel.rstests/key_serialization.rs
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/key_serialization.rs
- Refresh poulpy git lock to bfd0be57; align with removal of GLWE/LWE secret binary I/O upstream. - Add KeygenSeeds, keygen_with_seeds, and keygen_from_seeds; SecretKey::from_lattice_seed for lattice ChaCha seed only. - Add EvaluationKey::serialize and ::deserialize; share EVALUATION_KEY_BLOB_VERSION with Context helpers. - Re-export BDDKey and CGGI; trim serialize_keys example to evaluation key only; add keygen_seeds and adjust key_serialization tests.
- Wire format: version byte, T::BITS, Poulpy GLWE blob - Context helpers mirror evaluation key API - Integration tests for round-trip and wrong-T rejection
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/context.rs (1)
503-534:⚠️ Potential issue | 🟠 MajorEvaluation-key blob still lacks strong context discrimination.
At Line 510 onward, deserialization validates version and parsing, but the blob header is not self-describing beyond version. This leaves room for accepting non-equivalent blobs when structural parsing succeeds under current layouts. Please add a key-kind discriminator and stable params/backend fingerprint in the header and validate both during load.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context.rs` around lines 503 - 534, The deserialize_evaluation_key function currently only checks EVALUATION_KEY_BLOB_VERSION and then parses bytes; add a self-describing header to the serialized blob (a key-kind discriminator string/enum and a stable fingerprint of the Params + backend/layout) and validate them before parsing: update deserialize_evaluation_key to read and verify the discriminator (e.g., "EVAL_KEY") and a params/backend fingerprint (compute using Params::stable_fingerprint or create a deterministic hash of self.params and backend/layout infos) and return io::ErrorKind::InvalidData if either mismatches; ensure the same discriminator/fingerprint are written by the corresponding serialize_evaluation_key so EvaluationKey construction only proceeds when the header matches the current Context and Params.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/context.rs`:
- Around line 20-30: Run rustfmt to fix import list wrapping and signature
formatting: reformat the long use block (the poulpy_core::layouts::{...} and
poulpy_hal::{ api::ModuleNew, layouts::{Module, ReaderFrom}, source::Source, }
imports) so items wrap per rustfmt style and also reflow the multi-line function
signature around lines ~543-545 (the impl of ModuleNew::new / Context::new or
the long function there) to match rustfmt's default wrapping; then run cargo fmt
--all and ensure cargo fmt --all -- --check passes.
In `@tests/keygen_seeds.rs`:
- Around line 33-39: The test function
secret_key_from_lattice_seed_matches_full_keygen_lattice_part in
tests/keygen_seeds.rs is misformatted causing cargo fmt --all -- --check to
fail; re-run the Rust formatter (cargo fmt) and commit the formatted file so the
function, its statements and assertions (Context::new, keygen_with_seeds,
SecretKey::from_lattice_seed, and the assert/ assert_eq lines) follow rustfmt
style; ensure no other style violations remain and re-run cargo fmt --all --
--check before pushing.
---
Duplicate comments:
In `@src/context.rs`:
- Around line 503-534: The deserialize_evaluation_key function currently only
checks EVALUATION_KEY_BLOB_VERSION and then parses bytes; add a self-describing
header to the serialized blob (a key-kind discriminator string/enum and a stable
fingerprint of the Params + backend/layout) and validate them before parsing:
update deserialize_evaluation_key to read and verify the discriminator (e.g.,
"EVAL_KEY") and a params/backend fingerprint (compute using
Params::stable_fingerprint or create a deterministic hash of self.params and
backend/layout infos) and return io::ErrorKind::InvalidData if either
mismatches; ensure the same discriminator/fingerprint are written by the
corresponding serialize_evaluation_key so EvaluationKey construction only
proceeds when the header matches the current Context and Params.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2833336a-2ece-4a2c-9a48-58ab534c18ef
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
examples/serialize_keys.rssrc/ciphertext.rssrc/context.rssrc/keys.rssrc/lib.rstests/ciphertext_serialization.rstests/key_serialization.rstests/keygen_seeds.rs
✅ Files skipped from review due to trivial changes (1)
- tests/key_serialization.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/serialize_keys.rs
- src/keys.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/context.rs`:
- Around line 591-603: The GLWE-layout check is ineffective because FheUint is
allocated from self.params.glwe_layout before read_from, so fields
n/base2k/k/rank remain identical and the comparison is a tautology; to fix this,
change the serialization/deserialization format to include either the GLWE
layout (n, base2k, k, rank) or a Params fingerprint in the blob and then, in the
reader that uses FheUint::alloc_from_infos/read_from (referenced symbols:
FheUint, read_from, self.params.glwe_layout), deserialize that
layout/fingerprint first and validate it against self.params.glwe_layout (or
compute/compare the fingerprint) before allocating or accepting the ciphertext
bytes so mismatched GLWE parameters are detected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8123186c-6c0f-48e3-99e8-bb8dac9eb580
📒 Files selected for processing (2)
src/context.rstests/keygen_seeds.rs
✅ Files skipped from review due to trivial changes (1)
- tests/keygen_seeds.rs
- Repin poulpy to upstream poulpy-fhe/poulpy@b598566 (was cedoor fork branch). - Update prepared key/secret types to the new DeviceBuf storage (src/keys.rs). - Workaround upstream FheUint::encrypt_sk -> FheUintPrepared::prepare bug: cache the prepared form on Ciphertext and route Context::encrypt via FheUintPrepared::encrypt_sk + FheUint::from_fhe_uint_prepared. - Context::eval_binary now requires the prepared cache (panics with a shared NO_PREPARED_CACHE message); chaining limitation documented in src/ciphertext.rs. - Drop the defensive max(1<<22) floor in eval_binary; consolidate encrypt's scratch into a single 4 MiB arena with a TODO(poulpy-bug) pointing at squid#24 for the revert plan. - Trim tests/bdd_parallel.rs to the single parallel-vs-single-thread parity test; assert ground-truth plaintext on both sides. - Rename tests/key_serialization.rs -> tests/evaluation_key_serialization.rs (only EvaluationKey is serialized; secret material is reproduced from KeygenSeeds, already covered by tests/keygen_seeds.rs). - Update .cursor rules to reflect per-call scratch allocation (no more long-lived arena on Context). - README: link issue #24 under Milestone 2.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
src/context.rs (3)
520-525:⚠️ Potential issue | 🟡 MinorAlign truncated-blob errors with the documented contract.
The doc comments say malformed/truncated blobs return
io::ErrorKind::InvalidData, but the empty/truncated branches here still returnUnexpectedEof. Either normalize these paths toInvalidDataor update the docs to mention both kinds.Also applies to: 579-595
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context.rs` around lines 520 - 525, The code in deserialize_evaluation_key currently returns io::ErrorKind::UnexpectedEof for empty/truncated blobs (e.g., the early return when bytes.split_first() fails); update those branches to return io::ErrorKind::InvalidData instead so they match the documented contract; search the same file for the similar truncated-blob handling (the other branch around lines 579–595) and make the same replacement there, ensuring the error messages remain descriptive but use io::ErrorKind::InvalidData consistently.
514-555:⚠️ Potential issue | 🟠 MajorHarden the evaluation-key blob header before deserializing.
This still only checks a version byte before
BDDKey::read_fromloads bytes into storage allocated fromself.params.bdd_layout. A same-sized blob from a different key kind, params set, or backend can therefore be accepted and re-prepared under the currentContext. Add at least a key-kind discriminator plus a stable params/backend fingerprint and validate both before reading the payload.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context.rs` around lines 514 - 555, The deserializer (deserialize_evaluation_key) only checks a single version byte and then calls BDDKey::read_from into storage derived from self.params.bdd_layout, which allows blobs from other key-kinds/params/backends to be accepted; before calling BDDKey::read_from validate a header containing a key-kind discriminator and a stable params/backend fingerprint (e.g. include and check a constant KIND byte/string and a hash of Params layout + backend id), compare that fingerprint against self.params (and backend identifier) and reject mismatches with io::ErrorKind::InvalidData; keep the existing EVALUATION_KEY_BLOB_VERSION check but extend the header parsing in deserialize_evaluation_key and only proceed to allocate/read BDDKey and prepare BDDKeyPrepared if both discriminator and fingerprint match.
609-621:⚠️ Potential issue | 🟠 MajorThis GLWE-parameter check cannot detect a mismatched blob.
fhe_uintis allocated fromself.params.glwe_layoutbeforeread_from, son/base2k/k/rankalready match the current context. Comparing those fields back toself.params.glwe_layoutis therefore a tautology and won't catch ciphertexts serialized under a different layout when the payload size happens to line up. Validate a serialized layout/fingerprint before allocation instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context.rs` around lines 609 - 621, The current check is tautological because FheUint::alloc_from_infos(&self.params.glwe_layout) is called before fhe_uint.read_from(...), so comparing fhe_uint.n()/base2k()/max_k()/rank() against self.params.glwe_layout always matches; instead, first deserialize or peek the serialized GLWE layout/fingerprint from the reader (e.g., read a header or fixed-size layout struct) and validate those fields against self.params.glwe_layout before calling FheUint::alloc_from_infos or allocating fhe_uint; update the logic around FheUint::alloc_from_infos, read_from, and the reader usage so you validate the incoming layout/fingerprint first and only then allocate and read the payload.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ciphertext.rs`:
- Around line 45-49: The Ciphertext<T> struct must store only the standard-form
ciphertext: remove the prepared field (Option<FheUintPrepared<...>>) and keep
only pub(crate) inner: FheUint<Vec<u8>, T> so Ciphertext does not carry
cached/prepared state; update any constructors/serializers/deserializers to
produce a Ciphertext with only inner. Allocate and use FheUintPrepared
transiently inside Context::eval_binary (and any other Context methods that need
prepared state) so preparation is performed within Context and never returned or
stored on Ciphertext; adjust call sites that previously accessed
Ciphertext::prepared to call Context methods that prepare the operand and
perform the operation. Ensure no API surfaces expose FheUintPrepared outside
Context::eval_binary.
In `@src/keys.rs`:
- Around line 14-19: The PR removed the standard GLWE/LWE-based persistence path
for SecretKey and only persists KeygenSeeds via Context::keygen_with_seeds /
Context::keygen_from_seeds; restore the original contract by implementing
SecretKey::serialize and SecretKey::deserialize that serialize/deserialize the
raw GLWE/LWE fields and rebuild any prepared/internal structures on load, and
add matching Context helper methods (e.g., Context::serialize_secret_key and
Context::deserialize_secret_key) that call those SecretKey methods; ensure the
deserialize path reconstructs the prepared form instead of relying on
KeygenSeeds so keys generated outside this crate or existing serialized keys
remain compatible.
- Around line 46-51: The KeygenSeeds struct currently derives Debug which can
leak secret material (fields lattice, bdd_mask, bdd_noise); remove Debug from
the derive list on KeygenSeeds to prevent accidental logging, and if you need to
retain debug printing implement a custom Debug impl that redacts all secret
fields (e.g. show "<redacted>" or similar) rather than printing the raw byte
arrays; ensure no other code depends on the auto-derived Debug for KeygenSeeds
and update callers if you add a redacting Debug implementation.
---
Duplicate comments:
In `@src/context.rs`:
- Around line 520-525: The code in deserialize_evaluation_key currently returns
io::ErrorKind::UnexpectedEof for empty/truncated blobs (e.g., the early return
when bytes.split_first() fails); update those branches to return
io::ErrorKind::InvalidData instead so they match the documented contract; search
the same file for the similar truncated-blob handling (the other branch around
lines 579–595) and make the same replacement there, ensuring the error messages
remain descriptive but use io::ErrorKind::InvalidData consistently.
- Around line 514-555: The deserializer (deserialize_evaluation_key) only checks
a single version byte and then calls BDDKey::read_from into storage derived from
self.params.bdd_layout, which allows blobs from other key-kinds/params/backends
to be accepted; before calling BDDKey::read_from validate a header containing a
key-kind discriminator and a stable params/backend fingerprint (e.g. include and
check a constant KIND byte/string and a hash of Params layout + backend id),
compare that fingerprint against self.params (and backend identifier) and reject
mismatches with io::ErrorKind::InvalidData; keep the existing
EVALUATION_KEY_BLOB_VERSION check but extend the header parsing in
deserialize_evaluation_key and only proceed to allocate/read BDDKey and prepare
BDDKeyPrepared if both discriminator and fingerprint match.
- Around line 609-621: The current check is tautological because
FheUint::alloc_from_infos(&self.params.glwe_layout) is called before
fhe_uint.read_from(...), so comparing fhe_uint.n()/base2k()/max_k()/rank()
against self.params.glwe_layout always matches; instead, first deserialize or
peek the serialized GLWE layout/fingerprint from the reader (e.g., read a header
or fixed-size layout struct) and validate those fields against
self.params.glwe_layout before calling FheUint::alloc_from_infos or allocating
fhe_uint; update the logic around FheUint::alloc_from_infos, read_from, and the
reader usage so you validate the incoming layout/fingerprint first and only then
allocate and read the payload.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ef3289e5-bdc9-4abe-952f-3d9774cd2090
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
.cursor/rules/modules.mdc.cursor/rules/poulpy-concepts.mdc.cursor/rules/project.mdcCargo.tomlREADME.mdexamples/add_u32.rssrc/ciphertext.rssrc/context.rssrc/keys.rstests/bdd_parallel.rstests/ciphertext_serialization.rstests/evaluation_key_serialization.rstests/keygen_seeds.rs
✅ Files skipped from review due to trivial changes (4)
- .cursor/rules/poulpy-concepts.mdc
- .cursor/rules/project.mdc
- .cursor/rules/modules.mdc
- Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/ciphertext_serialization.rs
- tests/keygen_seeds.rs
- Enable poulpy-cpu-avx/enable-avx only via target-specific deps on x86_64 - Select FFT64Avx as BE only with backend-avx on x86_64; scalar elsewhere - CI: RUSTFLAGS for all-features clippy; trim README; sync backend module docs
- Write n, base2k, k, and rank after plaintext bit width - Compare header layout to context Params before alloc/read_from - Single wire format with version byte 1 (no legacy reader) - Add test rejecting deserialize across mismatched Params
- Remove derived Debug that leaked lattice and BDD seed bytes - Add manual Debug impl showing <redacted> for all secret fields
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #13
Closes #19 (created a new lighter param set for tests)
Needs poulpy-fhe/poulpy#147
Summary by CodeRabbit
Release Notes
New Features
Breaking Changes
encrypt(value, &secret_key, &evaluation_key)Tests