fix(send): correct group phash and mark full SKDM target set (WA Web parity) - #678
Merged
Merged
Conversation
…parity)
Three related group-SKDM correctness fixes, the phash ones validated against a
real WA Web capture sent to the production server (raw identifiers redacted as
PII; reproduced locally to confirm the formula).
1. phash base64: use standard alphabet ('+'/'/', `BASE64_STANDARD_NO_PAD`) to
match whatsmeow (`RawStdEncoding`) and WA Web (`WABase64.encodeB64`). URL-safe
('-'/'_') diverged on ~22% of phashes (any output hitting base64 index 62/63).
2. has_key marks the FULL SKDM target set: `PreparedGroupStanza.skdm_devices`
now carries the whole `distribution_list`, not just the devices that
encrypted successfully. Mirrors WA Web `markHasSenderKey(x, M)`. Previously
devices that failed to encrypt (406 / no bundle) stayed `has_key=false` and
were re-targeted on every send, re-fetching dead prekeys -> retry storm. The
retry-receipt path (`mark_forget_sender_key`) repairs any that are actually
alive and keyless.
3. phash over the full participant device set + the sending device, on EVERY
group send (matching WA Web `phashV2([].concat(A, [B]))`):
- was computed over the SKDM target subset only and omitted the self device;
- was omitted entirely on fully-warm sends (no distribution).
`resolve_skdm_targets` now returns `(all_devices, needs_skdm)` so the caller
can pass the full set; `prepare_group_stanza` gains `all_devices_for_phash`
and computes the phash via `build_group_phash_set` (full set + self,
hosted-filtered) regardless of whether any SKDM is distributed. Status
broadcasts keep their prior behavior (group-only change).
Validation against a real WA Web capture (prod server):
- cold send: the recipient `<to>` set plus the sending device reproduced the
exact on-wire phash; the recipient set alone produced a different hash, so the
sending device is part of the hashed set.
- two later warm sends (no `<participants>`) still carried a phash, confirming a
phash is sent on every group send.
- the `<ack class="message">` never echoes a phash, so the server validates it
silently; a correct phash is what keeps it from triggering a re-fanout.
Tests: a phash device-set test (self inclusion + hosted filtering, fictitious
vectors), and a prepare_group_stanza test proving a device whose SKDM
encryption fails is still marked has_key.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughSplit resolved device targets into (all_devices_for_phash, needs_skdm); propagate both through status and group send paths, update prepare_group_stanza signature and phash computation (standard Base64), and adjust stanza SKDM marking and tests/benchmarks. ChangesSKDM Targeting and Phash Computation Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Benchmark Results4 regression(s) detected (>2% threshold):
63 unchanged benchmark(s)
|
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wacore/benches/send_receive_benchmark.rs`:
- Around line 736-740: bench_group_send's warm-path calls run_group_send which
invokes prepare_group_stanza with all_devices_for_phash set to None, causing
distribution_list to remain None and no phash to be computed; change the call in
run_group_send (the invocation that passes skdm_target_devices and
all_devices_for_phash) to pass the resolved distribution set (the variable
holding the selected device list/distribution_list) into the
all_devices_for_phash parameter instead of None so prepare_group_stanza can
compute and emit the group phash (this aligns with build_group_phash_set which
expects the full device set to append the sender and filter hosted devices).
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f931f8dd-0435-4bf5-ba8f-dc76ad659106
📒 Files selected for processing (4)
src/send.rswacore/benches/send_receive_benchmark.rswacore/src/messages.rswacore/src/send.rs
The warm group-send benches (setup_group_send_*, force_skdm=false) called prepare_group_stanza with all_devices_for_phash=None, so distribution_list stayed None and no phash was computed. Real warm sends now always compute a phash (full device set + self), so the bench under-measured that cost. Pass the participant device set on the warm path (mirroring the real caller, which supplies the full set when not distributing); the cold/force_skdm path keeps None and resolves the set itself.
This was referenced Jun 1, 2026
This was referenced Aug 11, 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.
Three related correctness fixes on the group SKDM/send path. The phash ones are validated against a real WhatsApp Web capture sent to the production server (raw LIDs redacted as PII; reproduced locally to confirm the formula).
1. phash base64: standard alphabet
participant_list_hashused URL-safe base64 (-/_). whatsmeow (RawStdEncoding) and WA Web (WABase64.encodeB64) use the standard alphabet (+//). Any phash whose first 6 hash bytes hit base64 index 62/63 diverged from the server (~22% of phashes). One-line fix plus a cross-impl test vector.2. has_key marks the full SKDM target set
After a group send we marked only the devices that encrypted successfully (
skdm_encrypted_devices). WA Web'smarkHasSenderKey(x, M)marks the whole target setM. A device that fails to encrypt (406 / no bundle) used to stayhas_key=false, so it was re-targeted on every send, re-fetching dead prekeys, which is a big part of the cold-send retry storm.PreparedGroupStanza.skdm_devicesnow carries the fulldistribution_list. The retry-receipt path (mark_forget_sender_key) still repairs any device that is actually alive and missing the key.3. phash over the full device set + self, on every send
WA Web computes
phashV2([].concat(A, [B]))whereAis the full participant device set andBis the sending device, and it sends a phash on every group send. We were:resolve_skdm_targetsnow returns(all_devices, needs_skdm);prepare_group_stanzatakesall_devices_for_phashand computes the phash via a newbuild_group_phash_sethelper (full set + sending device, hosted-filtered) regardless of whether any SKDM is distributed. Status broadcasts keep their prior behavior (this is group-only; WA Web's status path does not augment the set with self).Validation (real WA Web capture, prod server)
<to>device set plus the sending device reproduced the exact phash on the wire; the recipient set alone produced a different hash, so the sending device is part of the hashed set.<participants>node) still carried a phash, confirming a phash is sent on every group send.<ack class="message">never echoes a phash, so the server validates it silently; a correct phash is what keeps it from triggering a re-fanout.Tests
participant_list_hashstandard-base64 cross-impl vectors (whatsmeow/WA Web parity).build_group_phash_set: self inclusion changes the hash; hosted devices are dropped (fictitious vectors, no PII).prepare_group_stanza: a device whose SKDM encryption fails is still markedhas_key(the full target set).cargo clippy --all-targets -- -D warningsclean; wacore + whatsapp-rust + sqlite-storage test suites green.