Repository navigation
Derive ownership through dhke, and speed it up - #325
Conversation
There was a problem hiding this comment.
🔵 Needs a closer look
The cryptographic derivation and handwritten precomputation table warrant final human validation despite comprehensive tests.
0 open findings
What changed in this PR
Aligns note ownership and secret-key derivation with DHKE semantics while accelerating ownership checks through constant-time fixed-base multiplication.
Changes:
- Rejects identity and small-order stealth-address points during ownership and nullifier derivation.
- Adds a constant-time precomputed generator multiplication table.
- Updates API consumers, tests, and changelog documentation.
| File | Description |
|---|---|
core/src/keys.rs |
Adds DHKE-based shared-secret hashing. |
core/src/keys/secret.rs |
Updates ownership and note-key derivation. |
core/src/keys/view.rs |
Updates and accelerates ownership checks. |
core/src/keys/generator.rs |
Implements fixed-base generator multiplication. |
core/src/keys/generator/table.rs |
Provides the precomputed point table. |
core/src/note.rs |
Makes nullifier generation fallible. |
core/tests/small_order.rs |
Tests small-order and mixed-order points. |
core/tests/keys.rs |
Adapts key tests to the fallible API. |
core/tests/note_test.rs |
Adapts note tests to the fallible API. |
circuits/tests/common/mod.rs |
Updates circuit test setup. |
circuits/tests/torsion.rs |
Updates torsion tests. |
core/CHANGELOG.md |
Documents performance and API changes. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
c81441b to
0957c7e
Compare
|
Force-pushed: rebased on the amended stack. The two new |
0957c7e to
e3d9c85
Compare
|
Force-pushed: rebased on the amended stack. No changes. |
moCello
left a comment
moCello
left a comment
There was a problem hiding this comment.
COMMENT — The dhke derivation agrees with value decryption for every R, and the table multiplication is correct and constant-time. Approval waits only on the commit split below.
- The commit
core: Derive the shared secret of owns and gen_note_sk through dhkealso editscircuits/tests/common/mod.rsandcircuits/tests/torsion.rs. The repo convention gives each commit exactly one crate. Move the circuits test changes to acircuits:commit that follows thecore:commit. (CAUTION)
One note below; it asks for no change on this branch.
e3d9c85 to
23f08e2
Compare
|
Split as requested. |
moCello
left a comment
moCello
left a comment
There was a problem hiding this comment.
APPROVE — dhke now derives one shared secret for owns, gen_note_sk and value decryption, and it gives the old result for every R of prime order. The table multiplication gives the same point as the double-and-add for every scalar, and its digit recoding, row reads and point formulas do not depend on the secret. Two NOTEs ask for doc changes.
| /// Computes `s · G` for the generator `G`, the same point as | ||
| /// `GENERATOR_EXTENDED * s`, in constant time. |
There was a problem hiding this comment.
NOTE: The doc offers mul_generator as GENERATOR_EXTENDED * s and omits that its extended coordinates differ, which rkyv archives of stealth addresses and keys store.
Context: StealthAddress holds R as a JubJubExtended, and note_pk as a NotePublicKey that wraps one. rkyv archives their extended coordinates as they are. PublicKey holds A and B the same way, and ViewKey holds B. PublicKey::gen_stealth_address keeps GENERATOR_EXTENDED * for this reason, but only the message of 23f08e2 and the pull request description say so. No test in core pins the archived coordinates. A later change that moves gen_stealth_address to mul_generator for speed passes every test in core. By the message of 23f08e2, it then changes the genesis state that Rusk builds from seeded notes.
Fix: Add one sentence to the mul_generator doc. It states that the result has other extended coordinates than GENERATOR_EXTENDED * s, so callers use it only to compare points. A point that an rkyv archive stores keeps the double-and-add. A known-answer test of the archived bytes of a gen_stealth_address output enforces the same rule, at the cost of a fixture.
There was a problem hiding this comment.
Addressed in #327: the mul_generator doc now states that its extended coordinates differ from GENERATOR_EXTENDED * s, so it is for comparing points only, and a point an rkyv archive stores keeps the GENERATOR_EXTENDED * double-and-add. I left the known-answer fixture on the archived bytes as an optional follow-up — glad to add it if you want the extra guard.
| /// `a · R` is derived through [`dhke`](dusk_jubjub::dhke), which ignores | ||
| /// a small-order component of `R`. Returns `None` if `R` is the identity | ||
| /// or of small order: no key owns such a stealth address. |
There was a problem hiding this comment.
NOTE: dhke also returns none for an R that is not on the curve, but the docs and the changelog name only an identity or small-order R as the None case.
Context: dhke in dusk-jubjub 0.16 replaces an off-curve R with the identity. So gen_note_sk and Note::gen_nullifier return None for such an R, and both owns return false. Before this change, gen_note_sk and both owns panicked for an R whose coordinates are all zero, because a · R then has a zero Z. An off-curve R reaches these functions through StealthAddress::from_raw_unchecked, and the doc of StealthAddress::from_bytes_checked says that the rkyv path validates layout only. The docs of hash_shared_secret, ViewKey::owns, SecretKey::owns and Note::gen_nullifier name the same two cases as this doc. The two [#317] entries of core/CHANGELOG.md for the None and reject cases also name only these two.
Fix: Add "not on the curve" as a third case in this doc and in the six places above. The comment in Note::decrypt_value and the [#311] entry for Error::InvalidEncryption name the same two cases, and they predate this change. Add the same words there, so that one wording states when dhke gives none.
There was a problem hiding this comment.
Addressed in #327: an off-curve R is now named as a third case, with the identity and small order, in gen_note_sk, SecretKey::owns, ViewKey::owns, Note::gen_nullifier, hash_shared_secret, the decrypt_value comment, and the [#311] and [#317] changelog entries.
61a80ca to
fe6bf54
Compare
`ViewKey::owns`, `SecretKey::owns` and `SecretKey::gen_note_sk` computed `R · a` directly. For an `R` with a small-order component `T`, that is `a · R0 + (a mod 8) · T`, so which `note_pk` a wallet recognises for such an `R` depends on `a mod 8`, and for an identity or small-order `R` on `a mod 8` alone. Derive the shared secret with `dusk_jubjub::dhke`, as value decryption already does. It clears the cofactor, so a small-order component of `R` is ignored, and returns none for an identity or small-order `R`, which no key owns then. For an `R` of prime order the result is `a · R` as before, so notes created by honest senders keep their owner, note key and nullifier. `owns` returns false on none. `gen_note_sk` returns an `Option`, and so does `Note::gen_nullifier`, which needs the note key. None is the single outcome, carries no information and depends only on the public `R`, so an `Option` describes it better than an `Error` variant. A note that `owns` recognises always has both. The circuits tests adapt to the `Option` in the next commit. The tests craft `note_pk = H(a · R0 + g · T) · G + B` for `g` in `0..8`, for keys of every `a mod 8`, and the same for `R` of every small order. Multiplying `R` by `a` directly recognises `g = a mod 8`, and `g = a · j mod 8` for `R = j · T`; through `dhke`, only `g = 0` for `R0 + T`, and nothing for a small-order `R`.
`SecretKey::gen_note_sk` and `Note::gen_nullifier` now return an `Option`, empty for an `R` that is the identity or of small order. The sender owns every test note, so the tests expect both.
`ViewKey::owns` and `SecretKey::owns` compute `H(a · R) · G + B` and `(H(a · R) + b) · G` for every note a wallet scans, with the double-and-add of `GENERATOR_EXTENDED * s`: 252 doublings and 252 additions, nearly half the time of an `owns`, the shared secret `a · R` taking the rest. Multiply by `G` with a table of its multiples instead. Written in 64 signed radix-16 digits, `s · G` sums one table entry per digit: the table holds `1` to `8` times `2^(32j) · G` for `j < 8`, and Horner's rule over the eight digit positions takes 64 additions of table entries and 28 doublings. The table is 64 affine points, 4 KiB, built at compile time from coordinates in the source, which a test recomputes from the generator. It needs no build script nor new dependency, and is no_std. The scalar derives from the secret `a`, so the multiplication runs in constant time: the digits are computed without branching, each lookup reads a whole row and keeps the entry it needs by a constant-time selection, and the point formulas are complete. It computes the same point as the double-and-add, which a test checks for 1,000 random scalars and edge cases, and `owns` only compares that point, so no result changes. Over the 12,963 notes on mainnet, both `owns` take 36% less CPU time per note, with the old and new version timed back to back on every note. Stealth address generation keeps the double-and-add. It returns its points in the extended coordinates the computation leaves, which a different algorithm changes for the same point, and an rkyv archive of a note carries them as they are: Rusk's genesis state, built from notes derived from a seed, would change with them.
fe6bf54 to
35a68a1
Compare
Stacked on #324.
ViewKey::owns,SecretKey::ownsandSecretKey::gen_note_skderive the shared secret withdhke, as value decryption already does. A small-order component ofRis ignored, and no key owns a stealth address whoseRis the identity or of small order.gen_note_skandNote::gen_nullifiernow return anOption(breaking). Notes created by honest senders keep their owner, note key and nullifier. All 12,963 notes on mainnet and all 496 on testnet have anRof prime order, anddhkegives each of them the same shared point as before.ownsmultiplies the generator with a precomputed table of its multiples, with entries selected in constant time. Over the 12,963 notes on mainnet it takes 36% less CPU time per note. The table holds 64 points (4 KiB), is built at compile time, works in no_std and adds no dependency. Results don't change.Resolves #317
Resolves #319