feat(libsignal): let a consumer opt out of counter leasing - #1211
Merged
Conversation
Counters are leased in batches so the send path needs one durable flush per batch, and a reload fast-forwards past the whole lease. That only works because there is somewhere to persist the ceiling. A consumer whose storage is a component export has nowhere: into_components has to materialize the reservation before handing the record over, so the whole batch burns on every export rather than once per batch. Four consecutive DM sends land on the wire at counters 0, 64, 128, 192, and the peer buffers 63 skipped keys for each of them. A consumer whose persistence is already synchronous and durable before the ciphertext reaches the wire gets nothing from the lease and pays all of that. SessionRecord::waive_counter_lease and its SenderKeyRecord counterpart are how it says so. Nothing is inferred: the same record shape can be persisted by a consumer that wants the lease and by one that does not, so deriving the policy from the representation would turn a storage change into a silent change of guarantee. The two records now share a CounterLease enum whose Waived variant carries neither ceiling nor pending flag, so a record cannot hold a reservation the send path would gate on while having waived the lease that reservation implements. That covers creation, not just export: a waived record reserves nothing, so no ciphertext is gated on a flush that no longer protects anything. Waiving gives up a real guarantee. Without the lease, a crash between the encrypt and the write can reissue a counter and with it the (key, IV) pair. A record loaded from a snapshot written under the lease still carries a reservation that may already have been published, so waiving materializes it once, across archived states too, and then runs consecutively. The default is untouched.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds ChangesCounter lease waiver
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Sender
participant CounterLease
participant SenderKeyRecord
participant SessionRecord
Sender->>SenderKeyRecord: encrypt and reserve counter
SenderKeyRecord->>CounterLease: update reservation
SenderKeyRecord->>SessionRecord: export components
SessionRecord->>CounterLease: apply or waive lease
SessionRecord-->>Sender: rebuilt sender state
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
| Filename | Overview |
|---|---|
| wacore/libsignal/src/protocol/counter_lease.rs | Introduces the shared leased/waived state machine and centralizes rise-only reservation and transient flush-gate behavior. |
| wacore/libsignal/src/protocol/state/session.rs | Migrates direct-session reservation state to CounterLease and adds waiver materialization across current and archived sessions. |
| wacore/libsignal/src/protocol/sender_keys.rs | Migrates sender-key reservation state to CounterLease and adds the fallible group-record waiver entry point. |
| wacore/libsignal/src/protocol/session_cipher.rs | Delegates every direct-message reservation attempt to the lease state so waived records remain unreserved. |
| wacore/libsignal/src/protocol/group_cipher.rs | Delegates group iteration reservations to the lease state and adds component-export waiver coverage. |
| wacore/libsignal/tests/counter_lease.rs | Adds direct-session tests covering waived counters, default leasing, wire gates, persisted ceilings, and archived-state materialization. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
Load[Load session or sender-key record] --> Choice{Consumer waives lease?}
Choice -->|No| EncryptLeased[Encrypt and reserve a batch when ceiling is reached]
EncryptLeased --> Gate{Reservation raised?}
Gate -->|Yes| Flush[Durably flush reservation before wire]
Gate -->|No| SendLeased[Send using existing durable lease]
Flush --> SendLeased
Choice -->|Yes| Materialize[Advance existing chains past persisted ceiling]
Materialize --> Waived[Set lease to Waived]
Waived --> EncryptWaived[Encrypt without reserving or wire gating]
EncryptWaived --> Persist[Consumer persists synchronously and durably]
Persist --> SendWaived[Send ciphertext]
Reviews (2): Last reviewed commit: "fix(libsignal): keep the lease when a wa..." | Re-trigger Greptile
![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/libsignal/src/protocol/sender_keys.rs`:
- Around line 583-591: The waive_counter_lease flow currently commits
lease.waive() before materializing the ceiling, leaving inconsistent state when
fast_forward_sender_chain rejects the gap. Update waive_counter_lease to call
fast_forward_sender_chain_or_drop with the ceiling first, and only waive the
lease after successful materialization, preserving the record when
materialization fails.
🪄 Autofix
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 (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7d0c619e-cae8-4ab2-a9c0-e2239fa73e65
📒 Files selected for processing (8)
wacore/libsignal/src/protocol/consts.rswacore/libsignal/src/protocol/counter_lease.rswacore/libsignal/src/protocol/group_cipher.rswacore/libsignal/src/protocol/mod.rswacore/libsignal/src/protocol/sender_keys.rswacore/libsignal/src/protocol/session_cipher.rswacore/libsignal/src/protocol/state/session.rswacore/libsignal/tests/counter_lease.rs
📦 Binary size report
.text per crate
Baseline: |
Two problems on the same path. The doc link from the public waive_counter_lease pointed at the crate-private CounterLease, which fails the rustdoc gate. The guarantee a consumer gives up belongs on the public surface that offers the choice anyway, so it moved to SessionRecord::waive_counter_lease and the group counterpart links there. More importantly, the sender-key waiver dropped the ceiling before advancing past it. A chain too stale to advance returned the error with the lease already gone, leaving the record free to reissue exactly the iterations that ceiling said may already be on the wire. Materialize first, waive only on success.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
0 issues found across 4 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: Introduces an opt-in durability tradeoff (waiving the lease can reissue a key/IV pair on crash). The implementation appears correct but the security implications and interface design warrant human review.
Re-trigger cubic
This was referenced Aug 5, 2026
jlucaso1
added a commit
to oxidezap/whatsapp-rust-docs
that referenced
this pull request
Aug 5, 2026
* docs(signal-protocol): document counter-lease waiver opt-out whatsapp-rust#1211 added SessionRecord::waive_counter_lease and SenderKeyRecord::waive_counter_lease, letting a consumer whose own persistence is already synchronous and durable before the wire opt out of the batched counter lease that otherwise burns a full reservation on every component export. Document the API, its trade-off, and the per-record-type failure behavior, and cross-link it from the Record components section it primarily benefits. Ref: oxidezap/whatsapp-rust#1211 * docs(signal-protocol): fix trusted-reload comparison in lease waiver SignalStoreCache's trusted reload is a matching live-cache incarnation, not synchronous durability -- its warm sends use the coalesced write-behind. Only a direct Device store's trusted-reload rationale ties to synchronous durability. Corrects an inaccurate comparison flagged by review. * docs(signal-protocol): split dense lease-waiver sentence AGENTS.md asks for one idea per sentence. Break the run-on covering eligibility, the motivating case, and why into_components() re-burns the reservation into separate sentences. --------- Co-authored-by: Claude <noreply@anthropic.com>
This was referenced Aug 6, 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
Counters are leased in batches (
SENDER_CHAIN_RESERVATION_BATCH= 64) so the send path needs one durable flush per batch, and a reload fast-forwards past the whole lease so a published counter is never re-derived. That only works because there is somewhere to persist the ceiling: the local field 100 thatdeserialize_for_storereads back. A consumer whose storage is a component export has nowhere to put it, andinto_componentshas to materialize the reservation before handing the record over — so the whole batch burns on every export instead of once per batch. Four consecutive DM sends land on the wire at counters 0, 64, 128, 192, and the peer buffers 63 skipped keys for each one.A consumer whose persistence is already synchronous and durable before the ciphertext reaches the wire gets nothing from the lease and pays all of that.
SessionRecord::waive_counter_leaseand itsSenderKeyRecordcounterpart are how it says so, per record load. Nothing is inferred from the stored representation.Changes
CounterLease, shared by both records.Waivedcarries neither a ceiling nor a pending flag, so a record cannot hold a reservation the send path would gate on while having waived the lease that reservation implements. The incoherent state is unrepresentable in the type rather than avoided by convention, which is what the design note asked for.message_encryptandgroup_encryptreserve through the lease, so a waived record reserves nothing and no ciphertext is gated on a flush that no longer protects anything. Fixing onlyinto_componentswould have lefthas_pending_reservation()/is_wire_gated()true under a waiver.CounterLease::reserve. Both call sites used to duplicateif spent >= ceiling, and a bare call could lower a ceiling a durable snapshot already carried. Reservations now only ever rise, by construction.waive_counter_leasesignatures differ only in the way their exports already differ: the session drops an unadvanceable chain fail-closed, the sender key propagates the error.into_componentsadvances every state the ceiling covered, so the waiver has to as well; otherwise a later promotion has nothing left telling it those counters may already be on the wire. This was a real hole in the first draft of this branch, caught by the test named for it.SENDER_CHAIN_RESERVATION_BATCH, the note above the reservation inmessage_encrypt, and the outbound-advance note ingroup_encryptall stated the fast-forward guarantee without qualification. Each now says the waived case does not reserve.Trade-off
Without the lease, a crash between the encrypt and the write can reissue a counter, and with it the (key, IV) pair it derives. That is acceptable only for a consumer whose persistence is synchronous and durable before the ciphertext reaches the wire, which is precisely what it declares by waiving. The rationale lives on
CounterLeasealone, not repeated at the call sites.The default is untouched. A consumer that asks for nothing keeps the lease, the wire gate and the fast-forward exactly as before.
Numbers
0, 64, 128, 192; peer buffers 189 skipped keys0, 1, 2, 3; zero skipped0, 64, …, 448; receiver buffers 441 skipped keys0..7; zero skippedBoth leased figures are the measured symptom, asserted by the tests that pin the default. The 441 matches what the reporting consumer measured.
Validation
Every existing lease test passes unedited, including
tests/counter_lease.rs,crash_mid_lease_skips_spent_iterations_and_peer_decryptsandlease_amortizes_the_wire_gate. No contract change was needed there.New coverage, happy and failure side by side:
a_waived_lease_keeps_exported_counters_consecutive/the_default_lease_still_burns_a_batch_per_exporta_waived_lease_keeps_group_iterations_consecutive/the_default_group_lease_still_burns_a_batch_per_exporta_waived_lease_never_gates_the_wire/the_default_lease_still_gates_the_wirewaiving_materializes_a_previously_reserved_ceiling_onceand its group twinwaiving_burns_the_ceiling_into_archived_states_too— verified to fail (archived index 1 instead of 64) with the archived-state burn removedCounterLeaseitself for the waived no-ops and the rise-only reservationFull matrix left to CI.