fix: reassemble split file content in linear time instead of quadratic - #889
Open
Chessing234 wants to merge 3 commits into
Open
fix: reassemble split file content in linear time instead of quadratic#889Chessing234 wants to merge 3 commits into
Chessing234 wants to merge 3 commits into
Conversation
decode tolerates a CONTENT TLV split across several TLVs by appending each chunk with `contentBytes + value`. That reallocates and copies the whole accumulator every time, so k chunks totalling N bytes cost O(k*N). The split is the sender's choice, and a CONTENT TLV costs only five header bytes, so one content byte per chunk maximises the chunk count. The decoder runs on the mesh handler for any FILE_TRANSFER from a verified peer — verification binds an identity, it does not authorise anything, and a peer mints one by signing its own announcement. Measured on a ~1.2 MB payload split into 200k single-byte chunks, which is the scale BLE reassembly already admits: 2435ms of copying, repeatable for the cost of sending the packet again. Collecting the chunks and joining once brings the same payload to 65ms. The surrounding decoder was already hardened against exactly this shape for unknown tags, which are skipped without copying and logged once rather than per TLV. This closes the same hole on the tag that carries the bytes.
Pins ordering across chunks, the single-TLV round trip, an empty CONTENT TLV, rejection when CONTENT is absent, and correct reassembly at 200k chunks. Deliberately no wall-clock assertion: a threshold wide enough to be stable on slow CI is too wide to fail on the quadratic path, so it would only add flake. The 2435ms/65ms comparison was measured directly and is in the pull request instead.
The class doc described neither what encode writes nor what decode reads: it claimed 2-byte lengths for every TLV and an 8-byte FILE_SIZE, where the code uses 4 bytes for both CONTENT and FILE_SIZE, and it presented multi-TLV chunking as the way large files are carried when v2 writes exactly one CONTENT TLV and fragments at the transport layer. docs/file_transfer.md already has this right. Bringing the doc on the type into line with it, and recording why the multi-TLV tolerance path has to stay linear, since that is what this branch changes.
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
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.
What
BitchatFilePacket.decodetolerates a CONTENT TLV split across several TLVs by appending each chunk to the accumulator:a + ballocates a new array and copies the whole accumulator every time, so k chunks totalling N bytes cost O(k·N).The split is the sender's choice, and a CONTENT TLV costs only five header bytes — so one content byte per chunk maximises the chunk count.
decoderuns on the mesh handler for any FILE_TRANSFER from a verified peer, and verification binds an identity rather than authorising anything: a peer mints one by signing its own announcement (MessageHandler.kt:467, the public broadcast path).Measured
A ~1.2 MB payload split into 200k single-byte CONTENT TLVs — the scale BLE reassembly already admits — on this machine, same JVM, same input:
Repeatable for the cost of sending the packet again, and the cost grows with the square of the payload, so it gets worse as the reassembly ceiling rises.
Fix
Collect the chunks and join once — one allocation, one pass, O(N).
The surrounding decoder was already hardened against exactly this shape for unknown tags, which are skipped without copying and logged once rather than per TLV, with a comment explaining that per-TLV work is attacker-scaled. This closes the same hole on the tag that actually carries the bytes.
Evidence
BitchatFilePacketContentTest, 5 tests: ordering across chunks, the single-TLV round trip throughencode, an empty CONTENT TLV, rejection when CONTENT is absent, and correct reassembly at 200k chunks.Behaviour is otherwise unchanged — an absent CONTENT TLV is still
null, and a zero-length one still yields an empty file, both pinned.No wall-clock assertion in the suite, deliberately. A threshold wide enough to stay stable on slow CI is too wide to fail on the quadratic path, so it would only add flake. The numbers above were measured directly instead.
Measured the same way on
mainand here: 90 classes / 591 tests → 91 / 596. Lint unchanged frommain(305 errors, 333 warnings, 17 hints, all pre-existing/baselined).What I could not verify
Not run on a device or over a real BLE link — the measurement is a direct JVM call into
decode, not a packet arriving over the radio. The 1.2 MB figure comes from readingAppConstants.Fragmentation.MAX_FRAGMENT_TOTAL_BYTES; I have not confirmed by experiment that a payload of that shape survives reassembly end to end.Separately, the class doc above
BitchatFilePacketis out of date on the wire format — it says all TLV lengths are 2 bytes and FILE_SIZE is 8, while the code uses 4 for both CONTENT and FILE_SIZE. Left alone here to keep this diff to the one concern; happy to fix it in whatever change touches that next.