perf: zero-copy ciphertext serialization and DRY send pipeline - #502
Merged
Conversation
- Add into_serialized(self) -> Box<[u8]> on SignalMessage, PreKeySignalMessage, SenderKeyMessage, SenderKeyDistributionMessage — moves the internal buffer instead of copying via .to_vec() - Extract extract_ciphertext() helper in wacore::send, replacing 4 duplicated CiphertextMessage match blocks across peer, group, and retry stanza builders - Add encode_and_pad() that encodes protobuf + pads in a single pre-sized allocation, replacing encode_to_vec() + pad_message_v2() two-step - DRY: extract random_pad_len() shared between pad_message_v2 and encode_and_pad - Use resize() instead of vec! + extend_from_slice for padding bytes - Hash sorted phash strings incrementally instead of join() to avoid the large concatenated string allocation
📝 WalkthroughWalkthroughThis pull request refactors ciphertext extraction and message serialization handling across the signal encryption layer. It introduces ownership-based serialization methods ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@wacore/src/send.rs`:
- Around line 619-623: The code silently skips unsupported ciphertexts when
extract_ciphertext(encrypted_payload) returns None; add a warning log
immediately before the continue to surface partial fanout failures. At the
extract_ciphertext call site, emit a warning (using the existing logging
facility, e.g. warn! or log::warn!) that includes identifying context such as a
short debug/hex of encrypted_payload and the device/recipient identifiers
available in the surrounding scope, then continue; this makes unsupported
ciphertext variants observable without changing control flow.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b92091c8-432b-4fb5-9644-3653e32138d9
📒 Files selected for processing (4)
src/features/signal.rswacore/libsignal/src/protocol/protocol.rswacore/src/messages.rswacore/src/send.rs
Comment on lines
+619
to
623
| let Some((enc_type, is_prekey, serialized_bytes)) = | ||
| extract_ciphertext(encrypted_payload) | ||
| else { | ||
| continue; | ||
| }; |
There was a problem hiding this comment.
Add diagnostics before skipping unsupported ciphertext variants.
This branch silently drops a device encryption attempt. A warning log here would make partial fanout failures observable during debugging/incident response.
Suggested patch
- let Some((enc_type, is_prekey, serialized_bytes)) =
- extract_ciphertext(encrypted_payload)
- else {
- continue;
- };
+ let Some((enc_type, is_prekey, serialized_bytes)) =
+ extract_ciphertext(encrypted_payload)
+ else {
+ log::warn!(
+ "Unsupported ciphertext variant for device {}; skipping encryption output",
+ signal_address
+ );
+ continue;
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let Some((enc_type, is_prekey, serialized_bytes)) = | |
| extract_ciphertext(encrypted_payload) | |
| else { | |
| continue; | |
| }; | |
| let Some((enc_type, is_prekey, serialized_bytes)) = | |
| extract_ciphertext(encrypted_payload) | |
| else { | |
| log::warn!( | |
| "Unsupported ciphertext variant for device {}; skipping encryption output", | |
| signal_address | |
| ); | |
| continue; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wacore/src/send.rs` around lines 619 - 623, The code silently skips
unsupported ciphertexts when extract_ciphertext(encrypted_payload) returns None;
add a warning log immediately before the continue to surface partial fanout
failures. At the extract_ciphertext call site, emit a warning (using the
existing logging facility, e.g. warn! or log::warn!) that includes identifying
context such as a short debug/hex of encrypted_payload and the device/recipient
identifiers available in the surrounding scope, then continue; this makes
unsupported ciphertext variants observable without changing control flow.
|
Bencher Report| Branch | perf/into-serialized-and-encode-pad |
| Testbed | ubuntu-latest |
⚠️ WARNING: Truncated view!The full continuous benchmarking report exceeds the maximum length allowed on this platform.
🚨 1 Alert
🐰 View full continuous benchmarking report in Bencher
This was referenced Apr 9, 2026
3 tasks
This was referenced May 30, 2026
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.
Summary
Zero-copy ciphertext
into_serialized(self) -> Box<[u8]>onSignalMessage,PreKeySignalMessage,SenderKeyMessage,SenderKeyDistributionMessageBox<[u8]>buffer directly intoNodeContent::Bytesinstead of copying via.to_vec()Box<[u8]>implementsInto<Vec<u8>>(free conversion), soNodeBuilder::bytes()accepts it without allocationDRY:
extract_ciphertext()helperCiphertextMessagematch blocks acrossprepare_peer_stanza,prepare_group_retry_stanza,encrypt_for_devices, andsrc/features/signal.rs(&'static str enc_type, bool is_prekey, Box<[u8]> bytes)Encode + pad fusion
encode_and_pad()encodes protobuf and pads in a single pre-sizedVec::with_capacity(encoded_len + pad_len)random_pad_len()extracted as shared helper (DRY betweenpad_message_v2andencode_and_pad)pad_message_v2usesresize()instead ofvec!+extend_from_sliceIncremental phash
participant_list_hashhashes sorted ad_strings incrementally instead ofjoin()into one large concatenated stringsort_unstableinstead ofsortTest plan
cargo fmt --allcargo clippy --all --tests— zero warningscargo test --workspace --exclude e2e-tests— all 1119 tests passSummary by CodeRabbit