feat: advertise this client's reassembly ceiling in authenticated peer state - #879
feat: advertise this client's reassembly ceiling in authenticated peer state#879Chessing234 wants to merge 2 commits into
Conversation
The private-media bit tells a sender we understand encrypted media. It says nothing about how much of it we can hold, so a sender has to infer the ceiling from the packet type — and that inference is wrong for this client. FragmentManager rejects any stream above MAX_FRAGMENTS_PER_ID whatever the type, while a sender that reads the bit as "large reassembler" plans far more than 256 fragments and sees no failure. Add TLV 0x03: a 2-byte big-endian count, optional, absent on every already-released client. An unreadable one (zero, wrong width, duplicated) is rejected rather than skipped like an unknown type — skipping reads as "said nothing", and "said nothing" is what sends the peer back to the guess. `local()` builds our own state so the two mesh services cannot drift and the advertised number is pinned to the constant the reassembler enforces.
Both built the local peer state inline and identically; they now share the factory, which is also what makes the advertisement testable without standing up a mesh service.
![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: 2d24d9a401
ℹ️ 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".
| * Wire format: | ||
| * `[version=0x01][type=0x01][len=1...8][minimal LE capabilities]` | ||
| * `[type=0x02][len=32][Ed25519 public key]` | ||
| * `[type=0x03][len=2][reassembly fragment ceiling, big-endian]` (optional) |
There was a problem hiding this comment.
Adding TLV 0x03 changes the authenticated peer-state wire contract, but the published interop spec in docs/PRIVATE_MEDIA_V1.md still defines 0x21 as only the capabilities and Ed25519-key TLVs and says unknown TLVs are skipped. Since this field is meant for cross-client negotiation with other senders, leaving the spec stale makes the new canonical bytes and rejection rules ambiguous for clients implementing the same contract; please update the relevant spec alongside this wire-format change.
AGENTS.md reference: AGENTS.md:L90-L91
Useful? React with 👍 / 👎.
|
correction on the numbers in the body: "784 → 790" came from an incremental gradle run, where result files left by earlier filtered runs get counted alongside the current ones. re-measured with the delta is unchanged and still exactly the 6 tests added here — |
1 participant
The Android half of the per-peer fragment-ceiling negotiation in permissionlesstech/bitchat#1671, which closes the
TODO(#1434)iOS left when the blanket 256-fragment preflight was narrowed.This is a live interop bug, not a future-proofing exercise
Three facts on current
main, both sides:PRIVATE_MEDIA(bit 8) —PeerCapabilities.LOCAL_SUPPORTEDis exactly that bit, sent in both the announce and authenticated peer state — and it implements the encrypted0x20path (NoisePayloadType.FILE_TRANSFER).FragmentManagerrejects any inbound stream whosetotalexceedsMAX_FRAGMENTS_PER_ID(256), regardless of packet type.fileTransfermigration fallback.At
MAX_FRAGMENT_SIZE = 469, 256 fragments is about 120 KiB. So an iOS→Android private image or voice note above roughly that size is planned at more than 256 fragments and dropped by our reassembler, with no failure visible to either side. The capability bit says what we understand; it has never said how much we can hold, and that is the gap.The change
TLV
0x03onAuthenticatedPeerState: a 2-byte big-endian count of fragments this client will reassemble for one packet, stated inside the established Noise session. Byte-identical to the field iOS #1671 reads.0x03(zero, wrong width, duplicated) is rejected rather than skipped like an unknown type. Skipping would read as "said nothing", and "said nothing" is precisely what sends the sender back to the type guess this field exists to replace.SecureIdentityStateManager's record stays three fields, and the ceiling is a per-session statement — the same reasoning as iOS, where it is pinned to the Noise session generation and deliberately not inherited across a rekey.AuthenticatedPeerState.local()builds our own state.MeshCoreandBluetoothMeshServicehad the same construction inlined twice; sharing it means the advertised number is pinned to the constantFragmentManagerenforces rather than restated by hand next to a capability set.Scope
Advertising only. This client already caps its own sends at
MAX_FRAGMENTS_PER_ID, which is below iOS's ceiling, so nothing it sends is at risk today and honouring a peer's advertised ceiling on the send path is a separate concern for a follow-up.Verification
Local run of the CI job (
testDebugUnitTest lintDebug, JDK 21):local(), and downgrading the unreadable-0x03rejection to a skip, each fail only their intended test — 2 failures, both named, nothing else moved.01 01 02 00 01 02 20 <key> 03 02 01 00.CI has not run. Fork PRs on this repo sit at
action_requireduntil a maintainer approves the workflow, so the local run above is the evidence, not a green check. Same for #866/#867/#875.Coordination
Answers the wire-constant question left open on iOS #1434 for this field specifically:
0x03, 2 bytes, big-endian, and the value Android sends is 256 because that is what its reassembler actually enforces. The field is additive in both directions, so this can land before, after, or without #1671 — but the benefit only appears once a sender reads it.