Skip to content

feat(ble): negotiate an explicit per-peer fragment ceiling - #1671

Open
Chessing234 wants to merge 3 commits into
permissionlesstech:mainfrom
Chessing234:feat/negotiated-fragment-ceiling
Open

feat(ble): negotiate an explicit per-peer fragment ceiling#1671
Chessing234 wants to merge 3 commits into
permissionlesstech:mainfrom
Chessing234:feat/negotiated-fragment-ceiling

Conversation

@Chessing234

Copy link
Copy Markdown
Contributor

Closes the TODO(#1434) left in broadcastPacket when the blanket 256-fragment preflight was narrowed.

The gap

Today the fragment bound for a directed transfer is inferred from the packet type: a directed fileTransfer is assumed to be the raw migration fallback aimed at current Android (256), and anything else is assumed to be aimed at a client whose reassembler is as large as ours. That proxy holds only while "implements the encrypted 0x20 path" and "has a large reassembler" describe the same population.

A client that adopts 0x20 with a smaller buffer breaks it in the silent direction: we plan fragments the peer drops, and the sender sees no local failure. .privateMedia says a peer understands encrypted media; it says nothing about how much of it that peer can hold.

The change

  • TLV 0x03 on AuthenticatedPeerStatePacket — a 2-byte big-endian count of fragments the peer will reassemble for one packet, stated inside the established Noise session. Absent means "not advertised", which is what every released client sends, so old peers decode unchanged.
  • An unreadable 0x03 (zero, wrong width, duplicated) is rejected rather than skipped like an unknown TLV. Skipping would read as "said nothing", and "said nothing" falls back to the permissive proxy — the opposite of what a peer sending the field is asking for.
  • BLEFragmentCeilingPolicy turns the inline type check into a value: packet type + directedness + whatever the peer advertised, out comes the bound and which of three reasons produced it. Framework-free, so every branch is tested directly.
  • An advertised ceiling replaces the proxy in both directions. Below 256 is the case the proxy gets wrong silently. Above 256 is a peer telling us the deployed-Android assumption behind that cap does not describe it.
  • Every decision is clamped to what we would reassemble ourselves, so one side raising its configuration cannot raise the other side's memory exposure.
  • We advertise our own bound too. The receiver's 10_000 guard is now a named constant used for both the guard and the advertisement, so they cannot drift.

Two things worth reviewing closely

The preflight is no longer restricted to fileTransfer. It could not stay restricted and still do its job: an advertised ceiling constrains encrypted media too, and that is the case the proxy misses. Peers that advertise nothing keep the exact old classification, so this is a no-op for every released client.

The rejection copy is now parameterized. It said "more than 256 mesh fragments", which stops being true once the ceiling is negotiated. The existing key is kept and only the numeral replaced, so all 30 locales keep their existing translation. One judgment call I want flagged: Turkish attached a vowel-harmonised ablative suffix to the numeral (256'dan), which no format specifier gets right for every value, so I rephrased that locale to a suffix-free construction (mesh parça sınırı: %lld). That is my edit, not a translator's — please have someone who reads Turkish check it.

Verification

  • Full SwiftPM suite: 2018 → 2034, green. The delta is exactly the 16 tests added here (4 wire-format, 9 policy, 3 session-store).
  • The rekey invariant is enforced twice over — beginAuthenticatedGeneration drops the observation, and the accessor refuses one pinned to a superseded generation. Removing either alone still passes; the test fails once both are gone, and it was the only failure in that run.
  • Two unrelated suites (ChatViewModelInitializationTests, BLEServiceCoreTests) failed once each on runs that took 47s and 63s while the machine was loaded, and did not reproduce in four subsequent clean runs of the full suite or in isolation. Timeout-sensitive, not this diff — but I am flagging them rather than filtering them out.
  • No local SwiftLint, so I checked the --strict rules by hand against .swiftlint.yml; one trailing_comma violation found and fixed before pushing.

What I did not verify

No device or simulator run, so the negotiation is verified by unit tests and by reading the wiring, not by two real handsets agreeing on a ceiling. Android does not send 0x03 yet, so in the field today every peer takes the unchanged fallback path — which is the safe way for this to land, but it also means the negotiated branch has no cross-platform exercise until an Android client advertises one.

The `.privateMedia` capability bit tells a sender that a peer understands
encrypted media. It says nothing about how much of it that peer can hold,
so senders have had to infer the ceiling from the packet type instead.

Add TLV 0x03 to the authenticated peer-state packet: a 2-byte big-endian
count of fragments the peer will reassemble for one packet. Absent means
"not advertised", which is what every already-released client sends, so
old peers keep decoding unchanged.

An unreadable 0x03 (zero, wrong width, duplicated) is rejected rather
than skipped like an unknown TLV. Skipping it would read as "said
nothing", and "said nothing" falls back to the permissive proxy — the
opposite of what a peer sending the field is asking for.

Name the receiver's own 10,000-fragment guard while here, so the number
we advertise and the number we enforce cannot drift apart.
Turn the inline type check in broadcastPacket into a value: given the
packet type, whether it is directed, and whatever the recipient
advertised, decide the fragment bound and say which of the three reasons
produced it.

An advertised ceiling replaces the proxy in both directions. Below 256 is
the case the proxy gets wrong silently — we plan fragments the peer drops
and see no local failure. Above 256 is a peer telling us the deployed
Android assumption behind that cap does not describe it.

Every decision is clamped to what we would reassemble ourselves, so one
side raising its configuration cannot raise the other side's memory
exposure.

No caller yet; this commit is the decision and its tests.
Advertise our own reassembler's bound in authenticated peer state, keep
each peer's advertised bound pinned to the generation that stated it, and
size outbound transfers against it.

Two behaviour changes fall out of this:

The preflight no longer only inspects directed `fileTransfer`. An
advertised ceiling constrains encrypted media too, and a small
reassembler behind the 0x20 path is exactly the case the type proxy
cannot see. Peers that advertise nothing keep the old classification, so
released clients are unaffected.

The rejection message carries the real limit instead of a hardcoded 256,
which is no longer always the number in force. The existing key keeps
every locale's translation with only the numeral parameterized.

Retires TODO(permissionlesstech#1434).
@Chessing234

Copy link
Copy Markdown
Contributor Author

one allocation note while this is open, since i only spotted it reading the rotation spec.

docs/PEER-ID-ROTATION.md §4.5 says the peer-ID binding proof will be "sent as a new TLV in AuthenticatedPeerStatePacket" but never assigns it a number. this PR takes 0x03 there, and the android half is permissionlesstech/bitchat-android#879. so whichever of those lands first should be treated as pinning 0x03, and the binding proof wants 0x04.

worth writing the number down somewhere either way — the packet's TLV space currently has its assignments only in the struct itself, which is how two features end up reaching for the same byte. (the 0x03 in that doc's §4.6 table is the v2 announce's own namespace, not this one — different packet, no conflict.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant