docs(signal-protocol): document DSM destination trait and chain key buffer reuse - #449
Conversation
…uffer reuse (whatsapp-rust#1137) Adds two notes to the Signal Protocol doc for PR #1137's allocation cuts: the DsmDestination trait that lets a Jid name the DeviceSentMessage destination without an intermediate String, and write_chain_key's in-place reuse of the persisted chain key buffer across ratchet advances.
|
Warning Review limit reached
Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
| Filename | Overview |
|---|---|
| advanced/signal-protocol.mdx | Adds focused documentation for destination JID encoding and chain-key buffer reuse without changing site structure or runtime behavior. |
Reviews (2): Last reviewed commit: "docs(signal-protocol): address Codex rev..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f06670f037
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - **Own other devices** (your other linked devices) receive a `DeviceSentMessage` wrapper containing the message plus the destination JID, so your other devices can display the sent message in the correct chat | ||
|
|
||
| <Note> | ||
| **Destination JID encoding via `DsmDestination` ([#1137](https://github.com/oxidezap/whatsapp-rust/pull/1137)).** The `DeviceSentMessage` wrapper writes its destination JID as a length-prefixed protobuf field, which needs the encoded length before the bytes. `wacore::messages::MessageUtils::encode_dm_plaintexts` and `dm_plaintexts_from_encoded` used to take `destination_jid: &str`, so the caller rendered the `Jid` into a `String` purely to measure and copy it. Both now take `impl DsmDestination` — a trait implemented directly on `Jid` (so it can measure and write its own wire form without an intermediate `String`) and on `str` plus the standard string wrappers (`String`, `Box<str>`, `Rc<str>`, `Arc<str>`, `Cow<str>`), carried through references of any depth via two blanket impls. The DM send path now passes `to_jid: &Jid` directly instead of `&to_jid.to_string()`. |
There was a problem hiding this comment.
The sentence beginning Both now take... combines the signature change, supported wrapper types, blanket implementations, allocation behavior, and call-site impact into one dense statement. This makes the API change difficult to scan and violates the project's one-idea-per-sentence convention. Split these details into concise sentences.
AGENTS.md reference: AGENTS.md:L25-L25
Useful? React with 👍 / 👎.
|
|
||
| ### Chain key buffer reuse | ||
|
|
||
| As of [#1137](https://github.com/oxidezap/whatsapp-rust/pull/1137), advancing a chain key no longer allocates a fresh buffer for its persisted 32-byte key material on every step. `SessionState` stores each chain key's bytes as `Option<bytes::Bytes>` on the underlying protobuf `ChainKey` field; since `Bytes` is immutable, writing the ratcheted key used to be an unconditional `Bytes::copy_from_slice(..)`, and the ratchet advances three times per message round trip (twice sending, once receiving). |
There was a problem hiding this comment.
The claim that a message round trip always advances chain keys three times only describes a particular two-device send scenario. As the DM fanout section documents, encryption runs for every resolved recipient device and every own linked device, so the number of sending advances varies with device topology and can be less or greater than two. Qualify this as a benchmark scenario or describe the cost per destination device.
Useful? React with 👍 / 👎.
| } | ||
| ``` | ||
|
|
||
| Reuse only happens when both guards pass: `try_into_mut()` succeeds solely when the `Bytes` is uniquely owned (no other clone observing the old key), and the length check keeps a differently-sized buffer (e.g. from a legacy record) from being partially overwritten. Either guard failing falls back to the original allocating behavior. In steady state a checked-out session record is uniquely owned — the cache takes it out of its `Arc` via `try_unwrap` (see [Session object cache](#session-object-cache) above) — so the fallback is rare. |
There was a problem hiding this comment.
For a legacy record with a differently sized buffer, BytesMut::copy_from_slice does not partially overwrite the destination; like slice copy_from_slice, it requires equal lengths and panics on a mismatch. The guard therefore prevents that panic and selects the allocating fallback, so the current explanation gives readers the wrong safety model.
Useful? React with 👍 / 👎.
- Split the DsmDestination note into one-idea-per-sentence, per this repo's style convention (AGENTS.md). - Qualify "three advances per round trip" as the harness benchmark's single-device scenario, not a fixed count — real sends touch as many sessions as DM device fanout resolves. - Correct the length-guard rationale: a mismatched length would panic in copy_from_slice, not partially overwrite the buffer.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
2 participants
Summary
Documents two of the four allocation cuts from whatsapp-rust#1137 that touch behavior already described in
advanced/signal-protocol.mdx:DsmDestinationtrait —MessageUtils::encode_dm_plaintexts/dm_plaintexts_from_encodednow takeimpl DsmDestinationinstead ofdestination_jid: &str, so the DM send path passes&Jiddirectly instead of stringifying it first. Added as a note under DM device fanout, next to the existing description of theDeviceSentMessagewrapper.write_chain_keyreuses the persisted chain key's existingBytesbuffer in place when it is uniquely owned and the right length, instead of allocating on every ratchet advance. Added as a new "Chain key buffer reuse" subsection under Performance optimizations, alongside the existing session-cache/Arcdocumentation it builds on.The other two cuts in #1137 (the
send_raw_bytes_burstout-parameter and the streaming reporting-token HMAC) are internal topub(crate)/private functions with no existing doc surface to update, so they're left out here — this PR only touches content that was already documenting the affected code paths.Test plan
🤖 Generated with Claude Code
Generated by Claude Code
Summary by cubic
Updates
advanced/signal-protocol.mdxto document two allocation cuts fromwhatsapp-rust#1137.Notes
DsmDestinationusage in DM encoding (pass&Jidwithout aString) and chain keyBytesbuffer reuse; clarifies the single-device benchmark context and that the length check avoids acopy_from_slicepanic.Written for commit 0aa5807. Summary will update on new commits.