diff --git a/FABLE_5_FINDINGS.md b/FABLE_5_FINDINGS.md new file mode 100644 index 0000000..30d892a --- /dev/null +++ b/FABLE_5_FINDINGS.md @@ -0,0 +1,107 @@ +# Fable 5 findings + +Review of the issues Fable found after the Call to Play follow-up fixes. This +document records the assessment only; it is not an implementation plan. + +## 1. IP-based actor verification can reject legitimate peers + +**Assessment: real and must-fix.** + +`handle_call_to_play_events` requires the QUIC connection's source IP to match +the peer's advertised listener IP. That assumption is unreliable for a +multi-homed LAN machine: routing may select Ethernet, Wi-Fi, a VPN, or a bridge +address different from the advertised address. The receiver silently drops the +event, while the one-way sender sees a successful write and does not invoke its +resync fallback. + +The check is not strong authentication either. Two peers on one host share an +IP, and the project currently uses a shared application TLS certificate rather +than a cryptographic identity unique to each `peer_id`. A normal client still +stamps its own peer ID, so same-host impersonation requires a modified client, +but the IP comparison should not be presented as an authenticated identity +boundary. + +A sound correction must either bind a peer ID to a connection through a +same-connection protocol exchange, return an application-level +acknowledgement/rejection that can trigger resync, or deliberately rely on the +documented trusted-LAN model without the unreliable IP equality test. + +## 2. Evicted event IDs can prevent a revived call from healing + +**Assessment: real and must-fix together with finding 5.** + +Expiry removes call events from `events` but leaves their IDs in `event_ids`. +If one peer expires a call and later receives an orphan `AddTime`, a subsequent +handshake cannot restore the original `Create`: it is rejected forever as a +duplicate. The call can therefore be alive on its creator while remaining +invisible on the other peer. The ID set also grows without a bound for the +session. + +Pruning expired IDs alone is not sufficient with the current `insert_all` +behavior. A handshake commonly supplies `Create` followed by later actions; +per-event compaction can expire and remove `Create` before the merge reaches +the extending `AddTime`. Healing requires atomic batch semantics: validate and +deduplicate the batch, merge it with retained events, then compact once using +the complete history. + +Retained IDs should correspond to retained events and deliberate terminal +tombstones, not every event ever observed. + +## 3. `insert` can accept an event that compaction immediately removes + +**Assessment: correct, lower-severity correctness/semantics issue.** + +An action targeting a call that expired more than five minutes ago can be +inserted, removed by compaction in the same operation, and still return +`Ok(true)`. It is then emitted to the local UI and broadcast to peers even +though it has no durable effect. + +The presentation reducer normally hides the result, but the accepted signal is +misleading and the work is wasted. Store results should distinguish retained, +duplicate, rejected, and immediately obsolete events, and only retained events +should be emitted or broadcast. + +## 4. "Add 5 more minutes" can shorten the deadline + +**Assessment: real user-visible bug.** + +The action currently sets the deadline to `now + 5 minutes`. If a roster fills +early while the call still has more than five minutes remaining, the button +shortens the call despite saying that it adds time. + +The intended calculation is `max(now, current deadline) + 5 minutes`. That also +behaves naturally for an already expired call. + +## 5. Handshake history merge is quadratic + +**Assessment: correct, and part of the correctness fix for finding 2 rather +than merely a performance nit.** + +`insert_all` calls `insert` for every incoming event, and each insertion rebuilds +several maps over the growing store while holding its write lock. Merging a +large handshake history is therefore O(n²). + +Compacting once after an atomic batch merge removes that cost and is also what +allows `Create` plus a later `AddTime` to revive consistently. Findings 2 and 5 +should be treated as one store-semantics problem. + +## 6. The startup error points users at the wrong cause + +**Assessment: correct UX issue.** + +When `actorId` is still unavailable, the peer may simply be starting. Telling +the user to choose a game folder is misleading unless the application actually +knows that the folder is missing. The normal startup state should say that Call +to Play is connecting and ask the user to try again shortly; folder guidance +should be reserved for a known missing-folder condition. + +## Priority + +1. Replace the unreliable IP identity check or make delivery acknowledged. +2. Correct batch merge, compaction, and event-ID retention as one change. +3. Correct the deadline-extension calculation. +4. Stop reporting and broadcasting immediately obsolete events. +5. Correct the startup message. + +The existing QUIC connection must not be described as carrying an authenticated +per-peer identity until the protocol actually establishes one.