From a66c591f8e0a0c4d81982b5248da236cfaa081f9 Mon Sep 17 00:00:00 2001 From: jack <212554440+jackjackbits@users.noreply.github.com> Date: Mon, 6 Jul 2026 17:45:57 +0200 Subject: [PATCH] Courier: deposit in parallel when the only route is a send queue (#1368) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Deposit with couriers in parallel when the only route is a send queue The courier path was nearly unreachable: NostrTransport claims any favorite with a known npub as "reachable" regardless of connectivity, and the mesh favorite exchange shares npubs, so for essentially every courier-eligible recipient the router picked Nostr's reachable branch. With no internet the message just sat in the relay send queue — in the flagship scenario (internet shutdown, mutual friend standing right there) the courier walked away carrying nothing. Add Transport.canDeliverPromptly(to:), defaulting to reachability for radio-backed transports; NostrTransport answers honestly by mirroring the relay manager's connection state (fail-closed behind Tor). When the chosen transport can't hand the message off promptly, the router now also deposits a sealed copy with connected couriers. Double delivery is harmless: receivers dedup by message ID, and delivered/read acks never downgrade the carried status. When relays are up, sends are trusted and no courier quota is spent. Co-Authored-By: Claude Fable 5 * Track DM-relay connectivity, not any-relay, for prompt delivery Codex review: NostrRelayManager.isConnected is true when any relay is up, including geohash/custom relays — but private messages target the default (gift-wrap-capable) relay set and queue when none of those are connected. A lone geohash relay would have suppressed the parallel courier deposit while the DM sat in the queue. Publish a DM-scoped connectivity flag and drive canDeliverPromptly from it. Co-Authored-By: Claude Fable 5 --------- Co-authored-by: jack Co-authored-by: Claude Fable 5 --- bitchat/Nostr/NostrRelayManager.swift | 7 ++ bitchat/Services/MessageRouter.swift | 19 +++-- bitchat/Services/NostrTransport.swift | 27 ++++++- bitchat/Services/Transport.swift | 9 +++ .../EndToEnd/CourierEndToEndTests.swift | 77 ++++++++++++++++++- .../Services/NostrTransportTests.swift | 52 ++++++++++++- 6 files changed, 182 insertions(+), 9 deletions(-) diff --git a/bitchat/Nostr/NostrRelayManager.swift b/bitchat/Nostr/NostrRelayManager.swift index 53d794fa..e1de49d6 100644 --- a/bitchat/Nostr/NostrRelayManager.swift +++ b/bitchat/Nostr/NostrRelayManager.swift @@ -137,6 +137,10 @@ final class NostrRelayManager: ObservableObject { @Published private(set) var relays: [Relay] = [] @Published private(set) var isConnected = false + /// Whether a relay that carries private messages is connected. DMs + /// target the default (gift-wrap-capable) relay set, so a connected + /// geohash/custom relay alone must not count — sends would still queue. + @Published private(set) var isDMRelayConnected = false private let dependencies: NostrRelayManagerDependencies private var allowDefaultRelays: Bool = false @@ -1087,6 +1091,9 @@ final class NostrRelayManager: ObservableObject { private func updateConnectionStatus() { isConnected = relays.contains { $0.isConnected } + // Relay URLs are normalized before entries are created, so direct + // set membership is sound. + isDMRelayConnected = relays.contains { $0.isConnected && Self.defaultRelaySet.contains($0.url) } } /// A relay that drops before sending EOSE must not stall initial-load diff --git a/bitchat/Services/MessageRouter.swift b/bitchat/Services/MessageRouter.swift index a15b9b10..e0417851 100644 --- a/bitchat/Services/MessageRouter.swift +++ b/bitchat/Services/MessageRouter.swift @@ -128,6 +128,14 @@ final class MessageRouter { SecureLogger.debug("Routing PM via \(type(of: transport)) (reachable) to \(peerID.id.prefix(8))… id=\(messageID.prefix(8))…", category: .session) transport.sendPrivateMessage(content, to: peerID, recipientNickname: recipientNickname, messageID: messageID) enqueue(message, for: peerID) + // "Reachable" without prompt delivery means the send only joined + // a queue (Nostr with relays down): also hand a sealed copy to + // any connected couriers rather than waiting for internet that + // may never come. Double delivery is harmless — receivers dedup + // by message ID, and delivered/read acks never downgrade. + if !transport.canDeliverPromptly(to: peerID) { + attemptCourierDeposit(content: content, messageID: messageID, for: peerID) + } } else { var unsent = message unsent.sendAttempts = 0 @@ -137,11 +145,12 @@ final class MessageRouter { } } - /// Last resort when no transport can reach the peer: seal the message to - /// their known static key and hand it to connected mutual favorites who - /// may physically encounter them. The queued copy above stays retained, - /// so direct delivery still wins if the peer reappears first (receivers - /// dedup by message ID). + /// Last resort when no transport can deliver promptly — the peer is + /// unreachable, or only reachable through a send queue waiting on + /// internet: seal the message to their known static key and hand it to + /// connected mutual favorites who may physically encounter them. The + /// queued copy above stays retained, so direct delivery still wins if + /// the peer reappears first (receivers dedup by message ID). private func attemptCourierDeposit(content: String, messageID: String, for peerID: PeerID) { guard let recipientKey = courierDirectory.noiseKey(peerID) else { return } for transport in transports { diff --git a/bitchat/Services/NostrTransport.swift b/bitchat/Services/NostrTransport.swift index d6b98a61..7a5c052b 100644 --- a/bitchat/Services/NostrTransport.swift +++ b/bitchat/Services/NostrTransport.swift @@ -14,7 +14,13 @@ final class NostrTransport: Transport, @unchecked Sendable { let registerPendingGiftWrap: @MainActor (String) -> Void let sendEvent: @MainActor (NostrEvent) -> Void let scheduleAfter: @Sendable (TimeInterval, @escaping @Sendable () -> Void) -> Void + /// Emits whether a relay that carries private messages is up + /// (fail-closed behind Tor). A connected geohash/custom relay alone + /// doesn't count: DM sends target the default relay set and would + /// still queue. + let relayConnectivity: @MainActor () -> AnyPublisher + @MainActor static func live(idBridge: NostrIdentityBridge) -> Dependencies { Dependencies( notificationCenter: .default, @@ -26,7 +32,8 @@ final class NostrTransport: Transport, @unchecked Sendable { sendEvent: { NostrRelayManager.shared.sendEvent($0) }, scheduleAfter: { delay, action in DispatchQueue.main.asyncAfter(deadline: .now() + delay, execute: action) - } + }, + relayConnectivity: { NostrRelayManager.shared.$isDMRelayConnected.eraseToAnyPublisher() } ) } } @@ -49,6 +56,10 @@ final class NostrTransport: Transport, @unchecked Sendable { // Reachability Cache (thread-safe) private var reachablePeers: Set = [] + // Mirror of the relay manager's connection state, cached here because + // canDeliverPromptly is called synchronously off the main actor. + private var relaysConnected = false + private var relayConnectivityCancellable: AnyCancellable? private let queue = DispatchQueue(label: "nostr.transport.state", attributes: .concurrent) @MainActor @@ -72,6 +83,12 @@ final class NostrTransport: Transport, @unchecked Sendable { queue.sync(flags: .barrier) { self.reachablePeers = Set(reachable) } + + relayConnectivityCancellable = self.dependencies.relayConnectivity() + .sink { [weak self] connected in + guard let self else { return } + self.queue.async(flags: .barrier) { self.relaysConnected = connected } + } } deinit { @@ -133,6 +150,14 @@ final class NostrTransport: Transport, @unchecked Sendable { return reachablePeers.contains(where: { $0.toShort() == short }) } } + + func canDeliverPromptly(to peerID: PeerID) -> Bool { + // A known npub makes a peer "reachable", but with no relay + // connection a send only joins the local queue. Answering honestly + // here lets the router hand a sealed copy to a courier in parallel + // instead of waiting for internet that may never come. + isPeerReachable(peerID) && queue.sync { relaysConnected } + } func peerNickname(peerID: PeerID) -> String? { nil } func getPeerNicknames() -> [PeerID: String] { [:] } diff --git a/bitchat/Services/Transport.swift b/bitchat/Services/Transport.swift index afd1680e..5cd095a6 100644 --- a/bitchat/Services/Transport.swift +++ b/bitchat/Services/Transport.swift @@ -54,6 +54,11 @@ protocol Transport: AnyObject { // Connectivity and peers func isPeerConnected(_ peerID: PeerID) -> Bool func isPeerReachable(_ peerID: PeerID) -> Bool + /// Whether a send to this peer is likely to leave the device promptly. + /// Distinct from reachability: Nostr claims any favorite with a known + /// npub as reachable even with no relay connection, where a send only + /// joins a queue waiting for internet that may never come. + func canDeliverPromptly(to peerID: PeerID) -> Bool func peerNickname(peerID: PeerID) -> String? func getPeerNicknames() -> [PeerID: String] @@ -111,6 +116,10 @@ protocol Transport: AnyObject { } extension Transport { + // Reachability implies prompt delivery for transports that hand packets + // straight to the radio; queue-backed transports override this. + func canDeliverPromptly(to peerID: PeerID) -> Bool { isPeerReachable(peerID) } + // Noise identity hooks default to inert for transports that do not carry // Noise sessions (e.g. NostrTransport). func noiseSessionPublicKeyData(for peerID: PeerID) -> Data? { nil } diff --git a/bitchatTests/EndToEnd/CourierEndToEndTests.swift b/bitchatTests/EndToEnd/CourierEndToEndTests.swift index 5acef6b3..a8cc3079 100644 --- a/bitchatTests/EndToEnd/CourierEndToEndTests.swift +++ b/bitchatTests/EndToEnd/CourierEndToEndTests.swift @@ -544,7 +544,16 @@ private final class CourierCaptureTransport: Transport { func isPeerConnected(_ peerID: PeerID) -> Bool { snapshots.contains { $0.peerID == peerID && $0.isConnected } } - func isPeerReachable(_ peerID: PeerID) -> Bool { isPeerConnected(peerID) } + // Nostr-style reachability: claimed for peers with no live link (known + // npub), where prompt delivery additionally needs a relay connection. + var reachablePeers: Set = [] + var promptDelivery = true + func isPeerReachable(_ peerID: PeerID) -> Bool { + isPeerConnected(peerID) || reachablePeers.contains(peerID) + } + func canDeliverPromptly(to peerID: PeerID) -> Bool { + isPeerReachable(peerID) && promptDelivery + } func peerNickname(peerID: PeerID) -> String? { nil } func getPeerNicknames() -> [PeerID: String] { [:] } @@ -652,4 +661,70 @@ struct MessageRouterCourierTests { #expect(transport.directSends == ["m3"]) #expect(transport.courierSends.isEmpty) } + + /// A peer can be "reachable" through a transport that cannot deliver + /// promptly (Nostr claims any favorite with a known npub, even with no + /// relay connection). The queued send must not shadow the courier: a + /// sealed copy goes to connected couriers in parallel, and receivers + /// dedup by message ID if both arrive. + @Test @MainActor + func queuedReachableSendAlsoDepositsWithCourier() { + let bobKey = Data(repeating: 0xB0, count: 32) + let bobID = PeerID(publicKey: bobKey) + let carolKey = Data(repeating: 0xC0, count: 32) + let carolID = PeerID(publicKey: carolKey) + + let transport = CourierCaptureTransport() + transport.snapshots = [ + TransportPeerSnapshot(peerID: carolID, nickname: "carol", isConnected: true, noisePublicKey: carolKey, lastSeen: Date()) + ] + transport.reachablePeers = [bobID] + transport.promptDelivery = false + + let directory = CourierDirectory( + noiseKey: { peerID in peerID == bobID ? bobKey : nil }, + isTrustedCourier: { $0 == carolKey } + ) + let router = MessageRouter(transports: [transport], courierDirectory: directory) + var carried: [String] = [] + router.onMessageCarried = { messageID, _ in carried.append(messageID) } + + router.sendPrivate("hi bob", to: bobID, recipientNickname: "bob", messageID: "m4") + + #expect(transport.directSends == ["m4"]) + #expect(transport.courierSends.count == 1) + #expect(transport.courierSends.first?.messageID == "m4") + #expect(transport.courierSends.first?.couriers == [carolID]) + #expect(carried == ["m4"]) + } + + /// When the reachable transport can deliver promptly (relays up), the + /// send is trusted and no courier quota is spent. + @Test @MainActor + func promptlyDeliverableReachablePeerSkipsCourier() { + let bobKey = Data(repeating: 0xB0, count: 32) + let bobID = PeerID(publicKey: bobKey) + let carolKey = Data(repeating: 0xC0, count: 32) + let carolID = PeerID(publicKey: carolKey) + + let transport = CourierCaptureTransport() + transport.snapshots = [ + TransportPeerSnapshot(peerID: carolID, nickname: "carol", isConnected: true, noisePublicKey: carolKey, lastSeen: Date()) + ] + transport.reachablePeers = [bobID] + + let directory = CourierDirectory( + noiseKey: { peerID in peerID == bobID ? bobKey : nil }, + isTrustedCourier: { $0 == carolKey } + ) + let router = MessageRouter(transports: [transport], courierDirectory: directory) + var carried: [String] = [] + router.onMessageCarried = { messageID, _ in carried.append(messageID) } + + router.sendPrivate("hi bob", to: bobID, recipientNickname: "bob", messageID: "m5") + + #expect(transport.directSends == ["m5"]) + #expect(transport.courierSends.isEmpty) + #expect(carried.isEmpty) + } } diff --git a/bitchatTests/Services/NostrTransportTests.swift b/bitchatTests/Services/NostrTransportTests.swift index 9a359fb1..e8fd19b5 100644 --- a/bitchatTests/Services/NostrTransportTests.swift +++ b/bitchatTests/Services/NostrTransportTests.swift @@ -6,6 +6,7 @@ // For more information, see // +import Combine import Foundation import Testing import BitFoundation @@ -85,6 +86,51 @@ struct NostrTransportTests { #expect(didRefresh) } + @Test("Prompt delivery requires both a known npub and a relay connection") + @MainActor + func canDeliverPromptlyTracksRelayConnectivity() async throws { + let keychain = MockKeychain() + let idBridge = NostrIdentityBridge(keychain: keychain) + let recipient = try NostrIdentity.generate() + let noiseKey = Data((0..<32).map(UInt8.init)) + let peerID = PeerID(hexData: noiseKey) + let relationship = makeRelationship( + peerNoisePublicKey: noiseKey, + peerNostrPublicKey: recipient.npub, + peerNickname: "Alice" + ) + let connectivity = CurrentValueSubject(false) + + let transport = NostrTransport( + keychain: keychain, + idBridge: idBridge, + dependencies: makeDependencies( + loadFavorites: { [noiseKey: relationship] }, + relayConnectivity: { connectivity.eraseToAnyPublisher() } + ) + ) + + // Reachable (npub known) but relays down: the peer must not be + // treated as promptly deliverable, or the router would skip the + // courier and let the message rot in the Nostr send queue. + #expect(transport.isPeerReachable(peerID)) + #expect(!transport.canDeliverPromptly(to: peerID)) + + connectivity.send(true) + let deliverable = await TestHelpers.waitUntil( + { transport.canDeliverPromptly(to: peerID) }, + timeout: 5.0 + ) + #expect(deliverable) + + connectivity.send(false) + let undeliverable = await TestHelpers.waitUntil( + { !transport.canDeliverPromptly(to: peerID) }, + timeout: 5.0 + ) + #expect(undeliverable) + } + @Test("Private message resolves short peer ID and emits decryptable packet") @MainActor func sendPrivateMessageResolvesShortPeerID() async throws { @@ -375,7 +421,8 @@ struct NostrTransportTests { currentIdentity: @escaping @MainActor () throws -> NostrIdentity? = { nil }, registerPendingGiftWrap: @escaping @MainActor (String) -> Void = { _ in }, sendEvent: @escaping @MainActor (NostrEvent) -> Void = { _ in }, - scheduleAfter: @escaping @Sendable (TimeInterval, @escaping @Sendable () -> Void) -> Void = { _, _ in } + scheduleAfter: @escaping @Sendable (TimeInterval, @escaping @Sendable () -> Void) -> Void = { _, _ in }, + relayConnectivity: @escaping @MainActor () -> AnyPublisher = { Just(false).eraseToAnyPublisher() } ) -> NostrTransport.Dependencies { NostrTransport.Dependencies( notificationCenter: notificationCenter, @@ -385,7 +432,8 @@ struct NostrTransportTests { currentIdentity: currentIdentity, registerPendingGiftWrap: registerPendingGiftWrap, sendEvent: sendEvent, - scheduleAfter: scheduleAfter + scheduleAfter: scheduleAfter, + relayConnectivity: relayConnectivity ) }