logs.gokuls.in

4 pull requests merged across 1 repo

Offline-Protocol/offline-protocol-sdk

The problem

Two devices in the same room — one plugged into the wall, one at 20% — carried an identical share of everyone else's traffic. The forwarding governor's dials (jitter, fan-out, send budget) had no idea what device they were running on.

Separately, the relay *role* was a label with no referent in either direction. It promoted on connection count plus battery, so a device surrounded by peers that never needed it announced itself a relay having forwarded nothing, while a device carrying the whole room's traffic over two links announced nothing at all. Apps were showing users a prediction, and frequently a wrong one.

What changed

Capability bias. Battery and charging state now scale three existing dials continuously. The weaker device's jitter window opens *later* (shifted, not widened), so the capable neighbour holding the same frame usually transmits first and the weak one stands down having spent no airtime — the saving is the forward that never happens. It also fans out to fewer neighbours and refills its send budget more slowly.

Deliberately a bias and not a threshold: a switch that turns forwarding off makes the network's shape depend on a state machine whose failure mode is a *partition*. Scaling means everyone still forwards, and a misjudged scale costs only redundancy. Charging devices, RelayPriority::Always devices, and devices with no battery reading are all exempt (unknown means full effort, matching the forwarding gate).

Observed relay standing. relay_promoted / relay_demoted / is_relay() now report frames actually carried in a rolling window (3 frames per 60s to begin, 2 consecutive quiet windows to end — asymmetric so a mesh with nothing to say for a minute doesn't churn). Config opt-out and the battery floor still demote *immediately*; those are refusals, and the window measures quiet.

That leaves the router's RelayManager role state machine with no consumers, so it's deleted along with RelayConfig::relay_threshold.

Breaking changes

  • relay_promoted fires without a battery feed, inverting the old \"no feed, no events\" behaviour. RelayPromoted.battery_level is therefore Option<u8> (number | null in RN).
  • isRelay() reports observed forwarding, so it answers false on a capable device that has had nothing to carry — including any device with a working internet relay, since the mesh is only offered frames nothing else can deliver (the known #326 gap, showing through truthfully).
  • relay_demoted no longer reports \"connections below relay threshold\"; sustained quiet reports \"no traffic carried for other devices recently\".
  • relayThreshold removed from Rust config, UDL, Swift, Kotlin, Python and RN.
  • ProtocolConfigExtended removed from UDL and FFI.

Inherited follow-ups from #363's review (all three closed)

  • (a) 32-bit relay_threshold as usize wrap — closed by *deleting the field*, so the cast is gone rather than patched.
  • (b) updateRelayConfig silently dropped an unrecognised priority while setRelayPriority threw — it now throws too. Dropping it applied the rest of the update and left the priority at its old value, indistinguishable from having set it at the call site.
  • (c) Dead ProtocolConfigExtended — carried relay/dors sections that no constructor on any binding accepted. Deleted.

Also: RelayPriority::Always now exempts a device from the *forwarding* battery floor (30% → hard 15%), mirroring the charging exemption. It previously moved only the cosmetic role label while the soft floor still stopped the device forwarding — weaker than \"always\" implies.

Two implementation traps worth reviewing

1. The jitter handicap is a fixed constant, never a multiple of the density-scaled span. The span already grows with degree; compounding the two pushes a weak device in a crowded room past RELAY_QUEUE_MAX_OVERDUE (5s), where forwards are *abandoned* rather than merely late. Bias must cost redundancy, never delivery.

2. TokenBucket::resize refills against the old rate, then clamps. It runs every tick, so refilling would let a battery oscillating anywhere on the ramp mint a fresh burst on every crossing.

Pre-existing finding, deliberately out of scope

Past roughly 167 neighbours the density-scaled jitter alone exceeds the 5s overdue cut-off (~155 with bias). The mesh is deliberately venue-bounded at ~100 nodes, so this is unreachable today — but it is the first thing that breaks if that ceiling rises. The test pins the venue case analytically rather than by sampling one message id: since the delay is min + handicap + hash % span, a sampled id makes the assertion a coin toss (it passed in isolation and flaked only under the warm parallel full-suite run).

Verification

  • cargo clippy --workspace -- -D warnings clean. *(Note: --all-targets has ~1455 pre-existing errors on main from test-code unwraps — the CI/CLAUDE.md gate is without that flag.)*
  • cargo test --workspace green, three consecutive full runs (1329 lib tests + doctests) to confirm the flake fix holds.
  • cargo fmt --all -- --check, RUSTDOCFLAGS=\"-D warnings\" cargo doc --workspace --no-deps clean.
  • All four binding sets regenerated together via ./scripts/generate-bindings.sh.
  • 233 Swift bridge tests, JS CI harness (incl. a new guard for the rejection behaviour), bindings/react-native tsc, and examples/react-native-app tsc — the last typechecked by no CI job.

New test coverage: 11 governor unit tests (bias ordering, overdue bound, charging/eager/unfed exemption, fan-out floor of 1, budget scaling, no-burst-on-resize; activity promote/demote/hysteresis/reset/force) plus 4 engine tests driving real process() ticks.

Rollout note

This is behavioural, not a wire or storage change — memory-only state, no persistence, no migration. Rollback is a clean revert. Item 1's device pass should watch for relay_promoted on a device that is actually forwarding.

The core problem

The SDK has a complete, tested, battery-aware relay policy — DORS energy scoring, relay promotion/demotion, and the message-forwarding battery floor. Every one of those reads the device's charge out of the per-transport TransportMetrics map.

Nothing in production ever wrote to that map. set_battery_level stored the number in an FFI-local RwLock that only a small is_relay() heuristic read, and update_transport_metrics — the only other candidate — has been a documented no-op for several releases. Consequences on every real device:

  • evaluate_relay_role early-returned on the unknown battery level, so relay_promoted / relay_demoted could never fire (QUICKSTART.md documents events that could not arrive).
  • DORS energy scoring skipped its battery term entirely.
  • battery_allows_relaying always took its "unknown means willing" branch, so the floor that stops a dying phone carrying other people's traffic never applied.

The fix

TransportManager::set_device_battery(level, is_charging) merges into the two snapshot loops that build the metrics map (get_available_transports and snapshot_status_and_available) — the single chokepoint every consumer resolves battery through. Merge is fill-if-absent: a transport reporting its own battery keeps it, along with its own charging state.

Charging state is plumbed because it is load-bearing, not cosmetic: a charging device is deliberately excused the soft min_battery_for_relay floor, so reporting the level alone would strip relay duty from exactly the devices that should keep it.

Apps must call setBatteryState on start and on each platform battery notification. Without it the policies stay in their unknown-level branch — the same behaviour as before, now documented rather than silent.

The other four defects

#DefectFix
2RN relay config crossed the bridge and was parsed by nothing — only relayPriority was read, so allowRelay / minBatteryForRelay / relayThreshold were ornamental on mobileNew update_relay_config / get_relay_config over UniFFI, applied at create time and at runtime by both bridges
3RelayPriority had two vocabularies: an FFI low/medium/high feeding a heuristic unrelated to the real relay policy, and the engine's never/auto/always that the config field already usedEnum renamed to the engine's vocabulary; is_relay() now reports the engine's actual role
4update_dors_config silently reset low_battery_threshold / relay_min_battery_level / relay_optimal_connection_count to 20/30/4 on every callThe three fields reached the FFI shape and both bridges
5get_topology() mapped battery_level into rssi — a node at 80% surfaced as −80 dBmNetworkNode gained battery_level + connection_count; rssi is now None

Note the JS NetworkNode type already declared battery_level, and docs/configuration.md already documented allowRelay / minBatteryForRelay and the auto/always vocabulary. The docs were describing behaviour that did not exist.

Breaking change

RelayPriority is spelled never / auto / always. Both native bridges and JS setRelayPriority still accept the legacy low / medium / high on input; what changes is what getRelayPriority returns — update anything comparing against 'medium'. Direct Swift/Kotlin/Python users of the enum need the new case names.

is_relay() reports the engine's real relay role rather than the old FFI-local guess (BLE peer count + FFI battery). The two could disagree; now they cannot. Deleting the heuristic outright stays item 12's scope — this only re-bases it on the single source of truth.

Testing

  • 9 new tests. Core: host-feed-driven role transitions (incl. the charging re-promote), the forwarding floor via the host feed, update_relay_config reaching both the forwarding gate and the role policy, metrics-merge semantics (fill, don't overwrite), clamping. FFI: battery reaching the engine, relay config reaching the engine, set_relay_priority preserving sibling fields, DORS round-trip, topology battery/rssi.
  • Mutation-checked. Reverted the metrics merge and the DORS field mapping to their buggy forms and confirmed the new tests fail; restored via scratchpad copies, not git checkout.
  • The DORS test asserts against TransportSelector::config() (a new read-only accessor), not get_dors_config() — the latter returns the FFI's own stored copy and would have passed vacuously.
  • cargo clippy --workspace -- -D warnings, cargo test --workspace (2110 pass), cargo fmt --all --check, RUSTDOCFLAGS="-D warnings" cargo doc — all green. The docs gate caught two intra-doc links to a private item; fixed.
  • Bridges: full iOS typecheck harness incl. OfflineProtocolModule.swift (proven non-vacuous with an injected error), swift test (233 pass), Android CI-equivalent Gradle build + unit tests.
  • All four binding sets regenerated together via ./scripts/generate-bindings.sh.

One unrelated flake appeared in a full run (test_legacy_nostr_message_received_leaves_the_watermark_alone) — confirmed pre-existing by reproducing on a stashed clean tree.

Follow-ups this does not do

  • Item 12 (B2) still owns deleting the is_relay() heuristic in favour of the governor.
  • Native platform battery observers are the app's job; this ships the API, not a BatteryManager / UIDevice subscription.

Follow-up to #361, from its review.

The gap

A delivery acknowledgement names a message id and nothing else tied it to a delivery. handle_ack_message settled on that id alone, so an acknowledgement from any party that had merely seen the frame settled the message: the outbox entry dropped, the retries stopped, and the app was told message_delivered for a message that arrived nowhere.

Refusing delivery is something the carriers and the relay could always do. A false confirmation is the strictly worse one, because it is the version the sender never finds out about.

Every device that carries a frame across the mesh knows its id, and #361 hands frames to exactly those devices — so this belongs with it rather than after it.

Not a security fix, and the code says so

The review that asked for this framed it as security hardening. That framing was wrong and the code does not repeat it.

An acknowledgement carries no signature: Message has no signature field, empty content is not a security-gated prefix (is_security_gated_prefix), and the receive loop handles ACKs and continues before the control gate would run anyway. So sender is self-declared, and a forger who saw the frame saw its recipient too and can simply write that name.

What this removes is the *unattributed* answer — a carrier or relay settling a message under its own name — and the divergence between the two settlement paths. Binding an acknowledgement to its sender for real needs signed acks, which is a negotiated wire change rather than a follow-up: unsigned acks must keep working for old peers, so during any transition a forger just omits the signature. Left out deliberately, noted in place.

Why it is safe on a path every delivery passes through

This was the question worth answering before adding a rejection here. Identity namespaces are not uniform — local_id is the profile before initialize_mls and the off1… address after, and UserId::new accepts either — so a naive check could plausibly have dropped legitimate acknowledgements fleet-wide.

It cannot, because delivery already implies the equality: a device processes a frame only when its own local_id equals the frame's recipient (receive_message forwards anything else), and the acknowledgement it builds carries that same local_id as its sender. A genuine confirmation therefore matches in either namespace.

The check fails open when neither outbox holds the id — the ordinary shape of a duplicate answer arriving after the message already settled. That fail-open needs our own outbox entry to be gone, which nothing a peer sends can arrange.

The ordering is load-bearing

The guard runs before anything is removed. The settle work starts by taking the pending ACK, and a frame rejected after that point would have already cost the message its retry record — turning a refused acknowledgement into exactly the silent loss this exists to prevent. Pinned by its own assertion.

Two consequences in settle_parked_dm_from_ack

  • Its own sender gate is now provably dead (the caller applies the same test against the same record) and is removed, rather than left as the kind of unreachable that reads as load-bearing to the next person.
  • Its doc claimed gate 1 excluded connection requests. That was never true: the caller retires their typed tracking a few lines earlier and unconditionally, so is_parkable_plain_dm's check has nothing left to find by the time it runs. The outcome is the wanted one — an already-untracked request, carried to its recipient and answered, was delivered — but the caller's ordering is what decides it, not the gate. Documented as such.

Scope

No FFI, UDL, wire-format, config, or persisted-state changes; no bindings regeneration. PendingAck is untouched — the recipient is read from the outbox record both paths already use, so the two cannot drift.

Verification

cargo test --workspace (2117 tests, 0 failures — was 2115), cargo clippy --workspace -- -D warnings, cargo fmt --all -- --check, RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps — all clean.

2 new tests, each mutation-checked against a sabotage of the line it covers:

  • test_an_acknowledgement_from_a_third_party_does_not_settle_an_in_flight_dm — deleting the guard fails this and #361's own test_a_parked_dm_is_not_settled_by_anyone_elses_acknowledgement, so removing the dead gate cost no coverage. Taking the pending ACK before the guard fails only the retry-record assertion, proving it pins the ordering specifically.
  • test_settling_a_parked_dm_redrives_the_recipients_other_parked_dms — pins the sibling re-drive the parked settle path performs, unpinned since #361. Removing the re-drive fails it.

The gap

Reachability was decided from local carrier status alone. TransportManager::can_reach_without_carrying answers yes for every recipient while any carrier that does its own routing is up — Internet, Nostr, Reticulum — so an online device never handed anything to its neighbors.

That is right for the case forwarding was built for, where nobody has infrastructure. In a mixed neighborhood it cost delivery both ways:

  • Outbound — a DM to a peer reachable only across the mesh went to the relay, earned the recipient_unreachable verdict, and parked waiting for someone who was never coming online. The park loop never consulted the mesh.
  • Inbound (the half that bites) — an online recipient answered a mesh-delivered message over the relay, where an offline sender could not see it. The sender retransmitted a message that had been delivered and read, and eventually reported it failed.

A third piece the issue did not name

Parking removes the pending ACK. handle_ack_message settles delivery only inside Some(pending) = remove_ack(..), so an acknowledgement for a parked message was dropped. Offering parked messages to the mesh without fixing that would deliver messages the sender never learns about — probed until the outbox lifetime expired, delivered and read the whole time. That is worse than not offering, so the three pieces here are one atom.

What changed

The send-time check is untouched, so the ordinary online case still costs the mesh nothing (a_device_with_infrastructure_does_not_spend_the_mesh_on_its_own_traffic still pins it). What changed is what happens after the relay contradicts it:

  • park_unreachable_dm offers the frame to the mesh — the relay's verdict is the only per-peer reachability fact this device receives. On every park, not just the first, so a recipient out of range at the first park is still reached later. Media chunks are offered from handle_recipient_unreachable_for_message, where they leave the park path; they keep their pending ACK, so nothing else is needed for them.
  • route_ack answers over the mesh when the message arrived over a mesh carrier and the sender is not a direct neighbor, whatever our own carriers say. With infrastructure up the answer goes both ways; the sender's ack handling already absorbs the duplicate.
  • settle_parked_dm_from_ack settles a parked DM from an acknowledgement with no pending ACK to match. Gated on: parkable plain DM, still in the outbox, recipient holds a live park counter, and the answer comes from that recipient. An acknowledgement is unauthenticated (no internal prefix, handled before the security gate), so the gates are things it cannot choose — bounding the forgery to parties who saw the frame *and* know the relay refused it, i.e. the carriers and the relay, who can already deny delivery outright.

TransportManager grows mesh_can_address / has_infrastructure_carrier — the two halves can_reach_without_carrying was already composed of — plus is_mesh_transport.

Scope

No FFI, UDL, wire-format, config, or persisted-state changes; no bindings regeneration. Group per-member sends and group delivery ACKs inherit both halves, since they ride the DM ladder.

Not covered (documented, not fixed)

  • Nostr and Reticulum report no per-recipient verdict, so nothing contradicts the initial answer on a device whose only infrastructure is one of those.
  • Carrier status is bridge-reported and means "this carrier is up", not "the relay connection is authenticated" — the issue's secondary point. A bridge that reports a connection it never authenticates produces no verdicts either, and its messages settle by ACK timeout as before.

Verification

cargo clippy --workspace -- -D warnings, cargo test --workspace (2115 tests, 0 failures), cargo fmt --all -- --check, RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps — all clean.

7 new tests, each mutation-checked against a sabotage of the line it covers (park offer removed → 2 unit tests + the e2e fail; settle arm removed → settle test + e2e fail; route_ack reverted → answer test fails; media offer removed → media test fails):

  • test_unreachable_verdict_offers_a_parked_dm_to_the_mesh
  • test_a_later_park_offers_the_dm_to_whoever_is_around_then
  • test_a_parked_dm_is_settled_by_an_acknowledgement_carried_back
  • test_a_parked_dm_is_not_settled_by_anyone_elses_acknowledgement
  • test_an_answer_to_a_carried_message_goes_back_over_the_mesh_even_when_online
  • test_unreachable_media_chunk_is_offered_to_the_mesh
  • an_online_sender_reaches_a_recipient_only_the_mesh_can_see (neighborhood simulator, full loop: send → relay verdict → carried two hops → delivered → answer carried back → settled)

The e2e test asserts zero transmissions before the verdict, so it cannot pass on the pre-existing send-failure path — the vacuity trap this suite has hit before.