docs(comments): fix 10 stale/wrong comments on the encryption surface - #87
Merged
Conversation
An adversarial commenting pass over the on-disk encryption code (merged in #85). Comments only — no behavior change; full test suite, clippy, and fmt stay green. Wrong facts: - crypto/mod.rs: derive_kek's `# Errors` said BadKeyLength fires only for `kdf == Hkdf` with empty raw bytes; the empty-key check is unconditional (before the KDF match) and covers Raw and Passphrase. - superblock/mod.rs (x2): sealed-superblock comments said sensitive scalars 16..52 are zeroed, but page_size is cleartext at 48..52 — the zeroed range is 16..48. - python/src/db.rs: key coercion claimed an empty/bad raw key raises BadKeyLength via to_py_err; every CryptoError maps to InvalidEncryptionKey -> InvalidEncryptionKeyError (there is no BadKeyLength Python error). - transaction/recovery.rs: "Slot 0 was written last" — it is written first (i=0, highest counter superblock_count-1). - transaction/keys.rs (test): comment said txn_counter=3 after fresh_encrypted; it is 2. Stale: - page_io.rs: header still said "Two fsyncs per commit"; it is three (I28 pre-drain + data + superblock) — the header #84 missed. - transaction/commit.rs + lifecycle.rs: CommitCtx described as "ten pieces" of state; the cipher + crypto_header fields make it twelve. - python/src/errors.rs: exception-hierarchy comment omitted DecryptionFailedError from the FatalError tier. Three further candidates could not be adversarially verified (transient rate limit) and were left as-is rather than rewritten on an unverified claim.
🚦 Bench results: PR vs main
Per-scenario detail (4 metrics × cells)document-store
mutation-log
ycsb-a
ycsb-b
|
Xof
added a commit
that referenced
this pull request
Jul 1, 2026
Constructive commenting pass (/comment-run) over the on-disk encryption surface merged in #85 — the complement to the wrong-comment fixes in #87. Additive only: ADD/EXTEND, no existing comment text deleted or rewritten. Comments only; full test suite, clippy, and fmt stay green. Each captures a non-obvious "we do X because Y" a first-time reader could not infer from the code: - crypto/mod.rs: Argon2id's Version::V0x13 and 32-byte output are pinned to the on-disk format (like KEK_INFO for HKDF) — changing either silently breaks unwrap of every existing Argon2id slot. - superblock/mod.rs: the four bootstrap fields must stay cleartext because they ARE the AAD, and slot selection + AAD derivation run before any DEK is available (a sealed body would be a chicken-and-egg deadlock). - page_cache.rs: the encrypted cold-load DecryptionFailed is fatal/poisoning, a peer of ChecksumMismatch, not a retryable operational error. - page_io.rs: set_stride swallows a seek error to 0 because its signature is infallible (bootstrap call site) and 0 makes reads fail closed. - spillway.rs: spill writes are deliberately not fsynced — content never crosses a transaction boundary, so durability would be wasted I/O. The pass found 0 wrong comments (fixed in #87) and 0 bugs. Six of the eleven modules reviewed needed nothing added — the encryption code was already well-commented from its per-task review gates.
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
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.
Summary
An adversarial commenting pass over the on-disk encryption surface (the code merged in #85, which never had a dedicated comment review). Comments only — no behavior change. Ten findings, each surfaced by a per-module reviewer and then confirmed by a second agent that tried to refute it, so false positives were dropped before reaching this diff.
Fixes
Wrong facts (would mislead a reader):
crypto/mod.rs—derive_kek's# ErrorssaidBadKeyLengthfires "ifkdf == Hkdfand the raw key bytes are empty," but the empty-key check runs unconditionally, before the KDF match and applies to bothRawandPassphrase.superblock/mod.rs(×2) — the sealed-superblock comments said sensitive scalars at 16..52 are zeroed, butpage_sizeis written in cleartext at 48..52; the zeroed range is 16..48. (The serialize code and the plaintext-leak test were already correct — only the comments were off by 4.)python/src/db.rs— the key-coercion doc claimed an empty/wrong raw key "raises BadKeyLength via to_py_err," but everyCryptoErrormaps toChiselError::InvalidEncryptionKey→InvalidEncryptionKeyError; there is noBadKeyLengthPython error.transaction/recovery.rs— "Slot 0 was written last in the loop" — it's written first (i=0, at the highest countersuperblock_count-1).transaction/keys.rs(test) — a test comment computedtxn_counter=3afterfresh_encrypted; it's actually2(createleavessuperblock_count-1 = 1, one commit bumps to2).Stale (drifted from the code):
page_io.rs— module header still said "Two fsyncs per commit"; the commit protocol does three (I28 pre-drain + data + superblock), as the same file's ownfsyncdoc already states. (This is the header docs: fix 7 wrong comments from the commenting pass (stale facts from the freemap rewrite + extraction) #84 missed.)transaction/commit.rs+transaction/lifecycle.rs—CommitCtxis described as bundling "ten pieces of manager state"; the encryption work addedcipherandcrypto_header, making it twelve.python/src/errors.rs— the exception-hierarchy comment omittedDecryptionFailedErrorfrom theFatalErrortier.Not fixed (transparency)
Three further candidate findings could not be adversarially verified (their verify agents hit a transient rate limit). On manual inspection none was clearly wrong — two describe an unexercised "unknown crypto algorithm id" error path, one is accurate phase-language — so they were left as-is rather than rewritten on an unverified claim.
Verification
Comment-only;
cargo build, fullcargo test,cargo clippy --workspace -- -D warnings, andcargo fmt --checkall clean.