From ac47af9101d25d017de37bda92683754da7e6d55 Mon Sep 17 00:00:00 2001 From: Taksh Date: Mon, 10 Aug 2026 20:09:00 +0530 Subject: [PATCH 01/11] fix: synchronize totalChecks increment in NostrEventDeduplicator isDuplicate() incremented totalChecks outside the lruLock that guards every other mutation on this class, so concurrent callers raced on the read-modify-write and lost increments. duplicateCount and evictionCount were already incremented under the lock; totalChecks was the one counter left outside it. getStats().totalChecks feeds hitRate and is the only denominator for duplicateCount, so a systematic undercount skews both. Moved the increment inside the existing synchronized block. Added a concurrency test that reproduces the race: run against the prior code it failed reliably (3/3 runs); against the fix it passes. --- .../android/nostr/NostrEventDeduplicator.kt | 4 +- .../nostr/NostrEventDeduplicatorTest.kt | 66 +++++++++++++++++++ 2 files changed, 68 insertions(+), 2 deletions(-) create mode 100644 app/src/test/kotlin/com/bitchat/android/nostr/NostrEventDeduplicatorTest.kt diff --git a/app/src/main/java/com/bitchat/android/nostr/NostrEventDeduplicator.kt b/app/src/main/java/com/bitchat/android/nostr/NostrEventDeduplicator.kt index 0638eafd..138ffb0a 100644 --- a/app/src/main/java/com/bitchat/android/nostr/NostrEventDeduplicator.kt +++ b/app/src/main/java/com/bitchat/android/nostr/NostrEventDeduplicator.kt @@ -79,9 +79,9 @@ class NostrEventDeduplicator( * @return true if the event is a duplicate (already seen), false if it's new */ fun isDuplicate(eventId: String): Boolean { - totalChecks++ - synchronized(lruLock) { + totalChecks++ + val existingNode = nodeMap[eventId] if (existingNode != null) { diff --git a/app/src/test/kotlin/com/bitchat/android/nostr/NostrEventDeduplicatorTest.kt b/app/src/test/kotlin/com/bitchat/android/nostr/NostrEventDeduplicatorTest.kt new file mode 100644 index 00000000..557ead2e --- /dev/null +++ b/app/src/test/kotlin/com/bitchat/android/nostr/NostrEventDeduplicatorTest.kt @@ -0,0 +1,66 @@ +package com.bitchat.android.nostr + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Test +import java.util.concurrent.CountDownLatch +import java.util.concurrent.Executors +import java.util.concurrent.TimeUnit + +class NostrEventDeduplicatorTest { + @Test + fun `isDuplicate flags a repeated event id`() { + val deduplicator = NostrEventDeduplicator(maxCapacity = 10) + + assertEquals(false, deduplicator.isDuplicate("event-1")) + assertEquals(true, deduplicator.isDuplicate("event-1")) + } + + @Test + fun `capacity evicts the least recently used event id`() { + val deduplicator = NostrEventDeduplicator(maxCapacity = 2) + + deduplicator.isDuplicate("a") + deduplicator.isDuplicate("b") + deduplicator.isDuplicate("c") // evicts "a" + + assertTrue(deduplicator.contains("b")) + assertTrue(deduplicator.contains("c")) + assertEquals(false, deduplicator.contains("a")) + } + + /** + * totalChecks used to be incremented outside the lruLock that guards every other + * mutation in this class, so concurrent callers could race on the read-modify-write + * and lose increments. Every other counter (duplicateCount, evictionCount) was + * already incremented under the lock, which is why only this one drifted. + */ + @Test + fun `totalChecks counts every call exactly once under concurrent access`() { + val deduplicator = NostrEventDeduplicator(maxCapacity = 10_000) + val threadCount = 8 + val checksPerThread = 2_000 + val executor = Executors.newFixedThreadPool(threadCount) + val start = CountDownLatch(1) + val done = CountDownLatch(threadCount) + + repeat(threadCount) { threadIndex -> + executor.submit { + start.await() + repeat(checksPerThread) { callIndex -> + deduplicator.isDuplicate("thread-$threadIndex-event-$callIndex") + } + done.countDown() + } + } + + start.countDown() + assertTrue(done.await(10, TimeUnit.SECONDS)) + executor.shutdown() + + assertEquals( + (threadCount * checksPerThread).toLong(), + deduplicator.getStats().totalChecks + ) + } +} From 76bf226f9886f9480fde32a055d802e27240769c Mon Sep 17 00:00:00 2001 From: wollow Date: Tue, 11 Aug 2026 14:52:16 +0300 Subject: [PATCH 02/11] fix(nostr): cap DM envelope backdating at 24 hours --- .../com/bitchat/android/nostr/NostrCrypto.kt | 4 +- .../bitchat/android/nostr/NostrProtocol.kt | 2 +- .../android/nostr/NostrProtocolTest.kt | 46 +++++++++++++++++++ docs/client-rewrite-contracts.md | 7 ++- 4 files changed, 55 insertions(+), 4 deletions(-) diff --git a/app/src/main/java/com/bitchat/android/nostr/NostrCrypto.kt b/app/src/main/java/com/bitchat/android/nostr/NostrCrypto.kt index b25499dd..972b90fa 100644 --- a/app/src/main/java/com/bitchat/android/nostr/NostrCrypto.kt +++ b/app/src/main/java/com/bitchat/android/nostr/NostrCrypto.kt @@ -317,9 +317,9 @@ object NostrCrypto { } /** - * Random timestamp up to maxPastSeconds in the past (default 2 days) + * Random timestamp up to maxPastSeconds in the past (default 24 hours for iOS compatibility) */ - fun randomizeTimestampUpToPast(maxPastSeconds: Int = 172800): Int { + fun randomizeTimestampUpToPast(maxPastSeconds: Int = 86400): Int { val now = (System.currentTimeMillis() / 1000).toInt() val offset = if (maxPastSeconds > 0) secureRandom.nextInt(maxPastSeconds + 1) else 0 return now - offset diff --git a/app/src/main/java/com/bitchat/android/nostr/NostrProtocol.kt b/app/src/main/java/com/bitchat/android/nostr/NostrProtocol.kt index 4376d1e9..926ea493 100644 --- a/app/src/main/java/com/bitchat/android/nostr/NostrProtocol.kt +++ b/app/src/main/java/com/bitchat/android/nostr/NostrProtocol.kt @@ -37,7 +37,7 @@ object NostrProtocol { val rumorId = rumorBase.computeEventIdHex() val rumor = rumorBase.copy(id = rumorId) - // 2. Seal the rumor (kind 13) signed by sender, timestamp randomized up to 2 days + // 2. Seal the rumor (kind 13) signed by sender, timestamp randomized within iOS's 24h lookback val sealedEvent = createSeal( rumor = rumor, recipientPubkey = recipientPubkey, diff --git a/app/src/test/kotlin/com/bitchat/android/nostr/NostrProtocolTest.kt b/app/src/test/kotlin/com/bitchat/android/nostr/NostrProtocolTest.kt index a5bd9561..3ef73733 100644 --- a/app/src/test/kotlin/com/bitchat/android/nostr/NostrProtocolTest.kt +++ b/app/src/test/kotlin/com/bitchat/android/nostr/NostrProtocolTest.kt @@ -3,6 +3,7 @@ package com.bitchat.android.nostr import com.google.gson.Gson import org.junit.Assert.assertEquals import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue import org.junit.Test class NostrProtocolTest { @@ -41,6 +42,47 @@ class NostrProtocolTest { assertNull(decrypted) } + @Test + fun createPrivateMessage_limitsEnvelopeTimestampsToIosLookback() { + val sender = NostrIdentity.generate() + val recipient = NostrIdentity.generate() + + repeat(20) { + val beforeCreation = (System.currentTimeMillis() / 1000).toInt() + val giftWrap = NostrProtocol.createPrivateMessage( + content = "bitchat1:test", + recipientPubkey = recipient.publicKeyHex, + senderIdentity = sender + ).single() + val afterCreation = (System.currentTimeMillis() / 1000).toInt() + val sealJson = NostrCrypto.decryptNIP44( + ciphertext = giftWrap.content, + senderPublicKeyHex = giftWrap.pubkey, + recipientPrivateKeyHex = recipient.privateKeyHex + ) + val seal = gson.fromJson(sealJson, NostrEvent::class.java) + + assertTimestampWithinIosLookback("gift wrap", giftWrap.createdAt, beforeCreation, afterCreation) + assertTimestampWithinIosLookback("seal", seal.createdAt, beforeCreation, afterCreation) + } + } + + private fun assertTimestampWithinIosLookback( + envelope: String, + createdAt: Int, + beforeCreation: Int, + afterCreation: Int + ) { + assertTrue( + "$envelope timestamp must be no more than 24 hours old", + createdAt >= beforeCreation - IOS_DM_LOOKBACK_SECONDS + ) + assertTrue( + "$envelope timestamp must not be in the future", + createdAt <= afterCreation + ) + } + private fun forgedGiftWrap( content: String, claimedSender: NostrIdentity, @@ -82,4 +124,8 @@ class NostrProtocolTest { content = giftWrapContent ).sign(wrapPrivateKey) } + + private companion object { + const val IOS_DM_LOOKBACK_SECONDS = 86400 + } } diff --git a/docs/client-rewrite-contracts.md b/docs/client-rewrite-contracts.md index 1abed243..1da0e33a 100644 --- a/docs/client-rewrite-contracts.md +++ b/docs/client-rewrite-contracts.md @@ -17,7 +17,7 @@ The remaining implementation work and milestone progress are tracked in | Inner payloads | Noise type bytes, private-message TLVs, peer-state TLVs, file-transfer TLVs, live-voice bursts, fragment header, sync request TLVs | `ClientRewriteWireContractTest`, `AuthenticatedPeerStateTest`, `PrivateMediaTransferPreparerTest`, `VoiceBurstPacketTest`, `FragmentManagerTest` | | Identity/security | Announcement extensions, capability bitfield endianness, Noise static-key binding, handshake identity binding, signatures | `IdentityAnnouncementTest`, `NoiseSessionManagerIdentityBindingTest`, `ClientRewritePrimitiveContractTest` | | Sync/routing | Stable packet IDs, GCS bitstream, replay collapse, TTL handling, relay choice, confirmed graph edges | `ClientRewritePrimitiveContractTest`, `GCSFilterTest`, `PacketRelayManagerTest`, `MeshGraphServiceTest`, `TransportBridgeServiceTest` | -| Nostr | Bech32, secp256k1 key derivation, NIP-01 event IDs/signatures, NIP-44 authenticated encryption, NIP-13 PoW, authenticated NIP-17 seals | `ClientRewriteNostrContractTest`, `NostrProtocolTest` | +| Nostr | Bech32, secp256k1 key derivation, NIP-01 event IDs/signatures, NIP-44 authenticated encryption, NIP-13 PoW, authenticated NIP-17 seals, 24-hour outbound envelope randomization | `ClientRewriteNostrContractTest`, `NostrProtocolTest` | | Application state | Peer unions, canonical private conversations, chronological history, delivery/read behavior, media migration policy | `AppStateStoreTest`, `PrivateChatManagerTest`, `MediaSendingManagerMigrationTest` | ## Golden-vector policy @@ -31,6 +31,11 @@ Round-trip tests remain useful but are not sufficient on their own: an encoder and decoder can share the same defect. Each critical wire format therefore has at least one literal vector. +NIP-17 receivers must use a lookback at least as long as the maximum timestamp +randomization used by senders. Android caps outbound seal and gift-wrap +randomization at 24 hours for iOS interoperability while retaining its 48-hour +receive lookback. + ## Rewrite acceptance gate From a configured Android development environment, run: From bc03d789720707fd106cb0b4127dd8c2229e67d9 Mon Sep 17 00:00:00 2001 From: wollow Date: Tue, 11 Aug 2026 17:57:03 +0300 Subject: [PATCH 03/11] changed to 23-hour-45-minute maximum backdating window --- .../com/bitchat/android/nostr/NostrCrypto.kt | 8 +++++--- .../com/bitchat/android/nostr/NostrProtocol.kt | 2 +- .../bitchat/android/nostr/NostrProtocolTest.kt | 16 ++++++++++++---- docs/client-rewrite-contracts.md | 6 +++--- 4 files changed, 21 insertions(+), 11 deletions(-) diff --git a/app/src/main/java/com/bitchat/android/nostr/NostrCrypto.kt b/app/src/main/java/com/bitchat/android/nostr/NostrCrypto.kt index 972b90fa..cab41ad0 100644 --- a/app/src/main/java/com/bitchat/android/nostr/NostrCrypto.kt +++ b/app/src/main/java/com/bitchat/android/nostr/NostrCrypto.kt @@ -21,7 +21,9 @@ import java.math.BigInteger * Includes secp256k1 operations, ECDH, and NIP-44 encryption */ object NostrCrypto { - + + internal const val NIP17_DEFAULT_MAX_PAST_SECONDS = 85_500 + private val secureRandom = SecureRandom() // NIP-44 v2 only @@ -317,9 +319,9 @@ object NostrCrypto { } /** - * Random timestamp up to maxPastSeconds in the past (default 24 hours for iOS compatibility) + * Random timestamp in the past, defaulting to 23h45m to leave slack inside iOS's 24h lookback. */ - fun randomizeTimestampUpToPast(maxPastSeconds: Int = 86400): Int { + fun randomizeTimestampUpToPast(maxPastSeconds: Int = NIP17_DEFAULT_MAX_PAST_SECONDS): Int { val now = (System.currentTimeMillis() / 1000).toInt() val offset = if (maxPastSeconds > 0) secureRandom.nextInt(maxPastSeconds + 1) else 0 return now - offset diff --git a/app/src/main/java/com/bitchat/android/nostr/NostrProtocol.kt b/app/src/main/java/com/bitchat/android/nostr/NostrProtocol.kt index 926ea493..42caefec 100644 --- a/app/src/main/java/com/bitchat/android/nostr/NostrProtocol.kt +++ b/app/src/main/java/com/bitchat/android/nostr/NostrProtocol.kt @@ -37,7 +37,7 @@ object NostrProtocol { val rumorId = rumorBase.computeEventIdHex() val rumor = rumorBase.copy(id = rumorId) - // 2. Seal the rumor (kind 13) signed by sender, timestamp randomized within iOS's 24h lookback + // 2. Seal the rumor with 15 minutes of slack inside iOS's 24-hour lookback. val sealedEvent = createSeal( rumor = rumor, recipientPubkey = recipientPubkey, diff --git a/app/src/test/kotlin/com/bitchat/android/nostr/NostrProtocolTest.kt b/app/src/test/kotlin/com/bitchat/android/nostr/NostrProtocolTest.kt index 3ef73733..29a41f6e 100644 --- a/app/src/test/kotlin/com/bitchat/android/nostr/NostrProtocolTest.kt +++ b/app/src/test/kotlin/com/bitchat/android/nostr/NostrProtocolTest.kt @@ -43,10 +43,15 @@ class NostrProtocolTest { } @Test - fun createPrivateMessage_limitsEnvelopeTimestampsToIosLookback() { + fun createPrivateMessage_reservesSlackInsideIosLookback() { val sender = NostrIdentity.generate() val recipient = NostrIdentity.generate() + assertEquals( + IOS_DM_LOOKBACK_SECONDS - TIMESTAMP_SAFETY_SLACK_SECONDS, + NostrCrypto.NIP17_DEFAULT_MAX_PAST_SECONDS + ) + repeat(20) { val beforeCreation = (System.currentTimeMillis() / 1000).toInt() val giftWrap = NostrProtocol.createPrivateMessage( @@ -74,8 +79,8 @@ class NostrProtocolTest { afterCreation: Int ) { assertTrue( - "$envelope timestamp must be no more than 24 hours old", - createdAt >= beforeCreation - IOS_DM_LOOKBACK_SECONDS + "$envelope timestamp must leave 15 minutes inside the iOS lookback", + createdAt >= beforeCreation - MAX_OUTBOUND_BACKDATE_SECONDS ) assertTrue( "$envelope timestamp must not be in the future", @@ -126,6 +131,9 @@ class NostrProtocolTest { } private companion object { - const val IOS_DM_LOOKBACK_SECONDS = 86400 + const val IOS_DM_LOOKBACK_SECONDS = 86_400 + const val TIMESTAMP_SAFETY_SLACK_SECONDS = 900 + const val MAX_OUTBOUND_BACKDATE_SECONDS = + IOS_DM_LOOKBACK_SECONDS - TIMESTAMP_SAFETY_SLACK_SECONDS } } diff --git a/docs/client-rewrite-contracts.md b/docs/client-rewrite-contracts.md index 1da0e33a..ed92ab79 100644 --- a/docs/client-rewrite-contracts.md +++ b/docs/client-rewrite-contracts.md @@ -17,7 +17,7 @@ The remaining implementation work and milestone progress are tracked in | Inner payloads | Noise type bytes, private-message TLVs, peer-state TLVs, file-transfer TLVs, live-voice bursts, fragment header, sync request TLVs | `ClientRewriteWireContractTest`, `AuthenticatedPeerStateTest`, `PrivateMediaTransferPreparerTest`, `VoiceBurstPacketTest`, `FragmentManagerTest` | | Identity/security | Announcement extensions, capability bitfield endianness, Noise static-key binding, handshake identity binding, signatures | `IdentityAnnouncementTest`, `NoiseSessionManagerIdentityBindingTest`, `ClientRewritePrimitiveContractTest` | | Sync/routing | Stable packet IDs, GCS bitstream, replay collapse, TTL handling, relay choice, confirmed graph edges | `ClientRewritePrimitiveContractTest`, `GCSFilterTest`, `PacketRelayManagerTest`, `MeshGraphServiceTest`, `TransportBridgeServiceTest` | -| Nostr | Bech32, secp256k1 key derivation, NIP-01 event IDs/signatures, NIP-44 authenticated encryption, NIP-13 PoW, authenticated NIP-17 seals, 24-hour outbound envelope randomization | `ClientRewriteNostrContractTest`, `NostrProtocolTest` | +| Nostr | Bech32, secp256k1 key derivation, NIP-01 event IDs/signatures, NIP-44 authenticated encryption, NIP-13 PoW, authenticated NIP-17 seals, 23h45m outbound envelope randomization | `ClientRewriteNostrContractTest`, `NostrProtocolTest` | | Application state | Peer unions, canonical private conversations, chronological history, delivery/read behavior, media migration policy | `AppStateStoreTest`, `PrivateChatManagerTest`, `MediaSendingManagerMigrationTest` | ## Golden-vector policy @@ -33,8 +33,8 @@ at least one literal vector. NIP-17 receivers must use a lookback at least as long as the maximum timestamp randomization used by senders. Android caps outbound seal and gift-wrap -randomization at 24 hours for iOS interoperability while retaining its 48-hour -receive lookback. +randomization at 23h45m, leaving 15 minutes of slack inside iOS's 24-hour +subscription window, while retaining its 48-hour receive lookback. ## Rewrite acceptance gate From acfbacf78fefb6664a523111f2c8c73a2e85b67f Mon Sep 17 00:00:00 2001 From: wollow Date: Tue, 11 Aug 2026 22:37:44 +0300 Subject: [PATCH 04/11] fix(nostr): reserve two-hour DM timestamp margin --- app/src/main/java/com/bitchat/android/nostr/NostrCrypto.kt | 4 ++-- .../main/java/com/bitchat/android/nostr/NostrProtocol.kt | 2 +- .../kotlin/com/bitchat/android/nostr/NostrProtocolTest.kt | 4 ++-- docs/client-rewrite-contracts.md | 6 +++--- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/src/main/java/com/bitchat/android/nostr/NostrCrypto.kt b/app/src/main/java/com/bitchat/android/nostr/NostrCrypto.kt index cab41ad0..1bbdc972 100644 --- a/app/src/main/java/com/bitchat/android/nostr/NostrCrypto.kt +++ b/app/src/main/java/com/bitchat/android/nostr/NostrCrypto.kt @@ -22,7 +22,7 @@ import java.math.BigInteger */ object NostrCrypto { - internal const val NIP17_DEFAULT_MAX_PAST_SECONDS = 85_500 + internal const val NIP17_DEFAULT_MAX_PAST_SECONDS = 79_200 private val secureRandom = SecureRandom() // NIP-44 v2 only @@ -319,7 +319,7 @@ object NostrCrypto { } /** - * Random timestamp in the past, defaulting to 23h45m to leave slack inside iOS's 24h lookback. + * Random timestamp in the past, defaulting to 22 hours to leave 2 hours of slack inside iOS's 24-hour lookback. */ fun randomizeTimestampUpToPast(maxPastSeconds: Int = NIP17_DEFAULT_MAX_PAST_SECONDS): Int { val now = (System.currentTimeMillis() / 1000).toInt() diff --git a/app/src/main/java/com/bitchat/android/nostr/NostrProtocol.kt b/app/src/main/java/com/bitchat/android/nostr/NostrProtocol.kt index 42caefec..7463a378 100644 --- a/app/src/main/java/com/bitchat/android/nostr/NostrProtocol.kt +++ b/app/src/main/java/com/bitchat/android/nostr/NostrProtocol.kt @@ -37,7 +37,7 @@ object NostrProtocol { val rumorId = rumorBase.computeEventIdHex() val rumor = rumorBase.copy(id = rumorId) - // 2. Seal the rumor with 15 minutes of slack inside iOS's 24-hour lookback. + // 2. Seal the rumor with 2 hours of slack inside iOS's 24-hour lookback. val sealedEvent = createSeal( rumor = rumor, recipientPubkey = recipientPubkey, diff --git a/app/src/test/kotlin/com/bitchat/android/nostr/NostrProtocolTest.kt b/app/src/test/kotlin/com/bitchat/android/nostr/NostrProtocolTest.kt index 29a41f6e..5faae233 100644 --- a/app/src/test/kotlin/com/bitchat/android/nostr/NostrProtocolTest.kt +++ b/app/src/test/kotlin/com/bitchat/android/nostr/NostrProtocolTest.kt @@ -79,7 +79,7 @@ class NostrProtocolTest { afterCreation: Int ) { assertTrue( - "$envelope timestamp must leave 15 minutes inside the iOS lookback", + "$envelope timestamp must leave 2 hours inside the iOS lookback", createdAt >= beforeCreation - MAX_OUTBOUND_BACKDATE_SECONDS ) assertTrue( @@ -132,7 +132,7 @@ class NostrProtocolTest { private companion object { const val IOS_DM_LOOKBACK_SECONDS = 86_400 - const val TIMESTAMP_SAFETY_SLACK_SECONDS = 900 + const val TIMESTAMP_SAFETY_SLACK_SECONDS = 7_200 const val MAX_OUTBOUND_BACKDATE_SECONDS = IOS_DM_LOOKBACK_SECONDS - TIMESTAMP_SAFETY_SLACK_SECONDS } diff --git a/docs/client-rewrite-contracts.md b/docs/client-rewrite-contracts.md index ed92ab79..05d5e1ad 100644 --- a/docs/client-rewrite-contracts.md +++ b/docs/client-rewrite-contracts.md @@ -17,7 +17,7 @@ The remaining implementation work and milestone progress are tracked in | Inner payloads | Noise type bytes, private-message TLVs, peer-state TLVs, file-transfer TLVs, live-voice bursts, fragment header, sync request TLVs | `ClientRewriteWireContractTest`, `AuthenticatedPeerStateTest`, `PrivateMediaTransferPreparerTest`, `VoiceBurstPacketTest`, `FragmentManagerTest` | | Identity/security | Announcement extensions, capability bitfield endianness, Noise static-key binding, handshake identity binding, signatures | `IdentityAnnouncementTest`, `NoiseSessionManagerIdentityBindingTest`, `ClientRewritePrimitiveContractTest` | | Sync/routing | Stable packet IDs, GCS bitstream, replay collapse, TTL handling, relay choice, confirmed graph edges | `ClientRewritePrimitiveContractTest`, `GCSFilterTest`, `PacketRelayManagerTest`, `MeshGraphServiceTest`, `TransportBridgeServiceTest` | -| Nostr | Bech32, secp256k1 key derivation, NIP-01 event IDs/signatures, NIP-44 authenticated encryption, NIP-13 PoW, authenticated NIP-17 seals, 23h45m outbound envelope randomization | `ClientRewriteNostrContractTest`, `NostrProtocolTest` | +| Nostr | Bech32, secp256k1 key derivation, NIP-01 event IDs/signatures, NIP-44 authenticated encryption, NIP-13 PoW, authenticated NIP-17 seals, 22h outbound envelope randomization | `ClientRewriteNostrContractTest`, `NostrProtocolTest` | | Application state | Peer unions, canonical private conversations, chronological history, delivery/read behavior, media migration policy | `AppStateStoreTest`, `PrivateChatManagerTest`, `MediaSendingManagerMigrationTest` | ## Golden-vector policy @@ -31,9 +31,9 @@ Round-trip tests remain useful but are not sufficient on their own: an encoder and decoder can share the same defect. Each critical wire format therefore has at least one literal vector. -NIP-17 receivers must use a lookback at least as long as the maximum timestamp +NIP-17 receivers should reserve safety slack beyond the maximum timestamp randomization used by senders. Android caps outbound seal and gift-wrap -randomization at 23h45m, leaving 15 minutes of slack inside iOS's 24-hour +randomization at 22h, leaving 2 hours of slack inside iOS's 24-hour subscription window, while retaining its 48-hour receive lookback. ## Rewrite acceptance gate From 87ccfe3f6ca8804fa0fe892c8e57d1b84227ac97 Mon Sep 17 00:00:00 2001 From: Taksh Date: Sat, 15 Aug 2026 13:58:41 +0530 Subject: [PATCH 05/11] Use the shared packet identity for duplicate detection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replay and duplicate detection keyed on a 32-bit contentHashCode over at most the first 64 bytes of the payload. Two packets from the same peer in the same millisecond that agreed on that prefix were the same packet as far as this cache was concerned, and a collision here is a dropped message: the second is discarded and nothing reports it. PacketIdUtil is the identity the rest of the stack already uses for this question — gossip sync membership, and the message IDs MessageHandler assigns — and iOS derives it identically: first 16 bytes of SHA-256 over type, senderID, timestamp and the whole payload. The security path now agrees with the sync path instead of carrying a weaker private notion of "same packet", and the FRAGMENT special case disappears because the full payload is covered either way. Peer scoping is deliberately kept. PacketIdUtil covers the packet's own senderID, which is not the peer it arrived from once relayed. --- .../bitchat/android/mesh/SecurityManager.kt | 33 ++++++++++++------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/app/src/main/java/com/bitchat/android/mesh/SecurityManager.kt b/app/src/main/java/com/bitchat/android/mesh/SecurityManager.kt index 3f684679..7efa7fb8 100644 --- a/app/src/main/java/com/bitchat/android/mesh/SecurityManager.kt +++ b/app/src/main/java/com/bitchat/android/mesh/SecurityManager.kt @@ -3,6 +3,7 @@ package com.bitchat.android.mesh import android.util.Log import com.bitchat.android.crypto.EncryptionService import com.bitchat.android.protocol.BitchatPacket +import com.bitchat.android.sync.PacketIdUtil import com.bitchat.android.protocol.MessageType import com.bitchat.android.model.RoutedPacket import com.bitchat.android.noise.AuthenticatedNoiseSession @@ -241,18 +242,28 @@ class SecurityManager(private val encryptionService: EncryptionService, private /** * Generate message ID for duplicate detection */ + /** + * Identity used for replay and duplicate detection. + * + * This was a 32-bit `contentHashCode()` over at most the first 64 bytes of + * the payload. Two packets from the same peer in the same millisecond that + * agreed on that prefix collided, and a collision here is a *dropped + * message* — the second packet is discarded as a duplicate and there is no + * signal that it happened. + * + * `PacketIdUtil` is the identity the rest of the stack already uses for + * exactly this question (gossip sync membership, message IDs in + * `MessageHandler`), and iOS derives it the same way: the first 16 bytes of + * SHA-256 over type, senderID, timestamp and the **whole** payload. Using + * it here makes the security path agree with the sync path instead of + * carrying a weaker private notion of "same packet". + * + * Peer scoping is kept: `PacketIdUtil` covers the packet's own senderID, + * while this key is scoped by the peer the packet was received from, and + * those are not the same thing for a relayed packet. + */ private fun generateMessageID(packet: BitchatPacket, peerID: String): String { - return when (MessageType.fromValue(packet.type)) { - MessageType.FRAGMENT -> { - // For fragments, include the payload hash to distinguish different fragments - "${packet.timestamp}-$peerID-${packet.type}-${packet.payload.contentHashCode()}" - } - else -> { - // For other messages, use a truncated payload hash - val payloadHash = packet.payload.sliceArray(0 until minOf(64, packet.payload.size)).contentHashCode() - "${packet.timestamp}-$peerID-$payloadHash" - } - } + return "$peerID-${PacketIdUtil.computeIdHex(packet)}" } /** From 4ef1b9c76576a7ddb5d857de17fcc3632bc39e5c Mon Sep 17 00:00:00 2001 From: Taksh Date: Sat, 15 Aug 2026 13:58:41 +0530 Subject: [PATCH 06/11] Pin the collision, the replay, and the peer scoping MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The collision case fails on main: two packets sharing a 64-byte prefix and a timestamp, where the second was silently dropped. The other two pass before and after on purpose. Replay of an identical packet must still be caught, and the same packet arriving from two different peers must still be tracked separately — strengthening the identity must not quietly weaken either. --- .../android/mesh/SecurityManagerTest.kt | 61 +++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/app/src/test/kotlin/com/bitchat/android/mesh/SecurityManagerTest.kt b/app/src/test/kotlin/com/bitchat/android/mesh/SecurityManagerTest.kt index 2a56e708..e43e11b5 100644 --- a/app/src/test/kotlin/com/bitchat/android/mesh/SecurityManagerTest.kt +++ b/app/src/test/kotlin/com/bitchat/android/mesh/SecurityManagerTest.kt @@ -598,4 +598,65 @@ class SecurityManagerTest { private fun String.hexToBytes(): ByteArray = chunked(2).map { it.toInt(16).toByte() }.toByteArray() + + // Duplicate-detection identity. + + /** + * Two distinct packets that agree on their first 64 bytes and share a + * timestamp. The old key hashed only that prefix, with a 32-bit + * `contentHashCode`, so these were "the same packet" and the second was + * silently dropped. + */ + private fun prefixSharingPair(): Pair { + val shared = ByteArray(64) { 0x7 } + val first = BitchatPacket( + version = 1u, + type = MessageType.NOISE_ENCRYPTED.value, + senderID = otherPeerID.hexToByteArrayForTest(), + recipientID = myPeerID.hexToByteArrayForTest(), + timestamp = 1_700_000_000_000uL, + payload = shared + byteArrayOf(0x01, 0x02, 0x03), + ttl = 7u + ) + val second = first.copy(payload = shared + byteArrayOf(0x0A, 0x0B, 0x0C)) + return first to second + } + + @Test + fun `packets differing only past the first 64 bytes are not treated as duplicates`() { + val (first, second) = prefixSharingPair() + + assertTrue(securityManager.validatePacket(first, otherPeerID)) + assertTrue( + "A distinct packet must not be dropped as a duplicate", + securityManager.validatePacket(second, otherPeerID) + ) + } + + @Test + fun `a genuine replay of the same packet is still rejected`() { + // The other half: strengthening the identity must not weaken replay + // protection, which is the reason this cache exists. + val (first, _) = prefixSharingPair() + + assertTrue(securityManager.validatePacket(first, otherPeerID)) + assertFalse( + "The identical packet must still be caught", + securityManager.validatePacket(first, otherPeerID) + ) + } + + @Test + fun `the same packet from two different peers is tracked separately`() { + // Peer scoping is deliberately kept: PacketIdUtil covers the packet's + // own senderID, which is not the peer it was received from once a + // packet has been relayed. + val (first, _) = prefixSharingPair() + + assertTrue(securityManager.validatePacket(first, otherPeerID)) + assertTrue(securityManager.validatePacket(first, unknownPeerID)) + } + + private fun String.hexToByteArrayForTest(): ByteArray = + chunked(2).map { it.toInt(16).toByte() }.toByteArray() } From 6bdcd46b33de4bbcb0c38aadee5688217a7c151a Mon Sep 17 00:00:00 2001 From: Taksh Date: Mon, 24 Aug 2026 21:32:36 +0530 Subject: [PATCH 07/11] fix(nostr): keep retrying relays instead of giving up on them forever A relay that fails is retried on an exponential backoff, then abandoned for the lifetime of the process. Nothing brings it back: the relay layer registers no connectivity callback, the periodic subscription validator only repairs subscriptions on sockets that are already open (and returns immediately when connectedRelayCount is 0, which is exactly the state after an outage), and connect() runs once from NostrClient.initialize(). The remaining paths that reset reconnectAttempts are a manual retry, a Tor state change, and a successful open. Two ways a relay died permanently: - Any error whose message mentioned DNS returned before scheduling anything at all. "Unable to resolve host" is what this device reports when it simply has no network, so a moment in a tunnel killed every relay at once, with no retry ever. - Otherwise the schedule stopped at MAX_RECONNECT_ATTEMPTS. With INITIAL=1s and MULTIPLIER=2 that is nine waits totalling about eight and a half minutes, after which the relay was dead. MAX_BACKOFF_INTERVAL was unreachable: attempt 9 asks for 256s and attempt 10 gave up, so the five-minute ceiling the constant defines never applied to anything. Let the backoff saturate at MAX_BACKOFF_INTERVAL and keep retrying there. A name-resolution failure now backs off like any other error. Steady state costs one connection attempt per relay per five minutes; the previous behaviour cost the user every internet DM, delivery receipt and geohash channel until they noticed and restarted the app. The schedule moves into RelayReconnectPolicy so it is unit-testable without OkHttp or a Context. --- .../android/nostr/NostrRelayManager.kt | 47 ++------- .../android/nostr/RelayReconnectPolicy.kt | 45 +++++++++ .../android/nostr/RelayReconnectPolicyTest.kt | 96 +++++++++++++++++++ 3 files changed, 151 insertions(+), 37 deletions(-) create mode 100644 app/src/main/java/com/bitchat/android/nostr/RelayReconnectPolicy.kt create mode 100644 app/src/test/kotlin/com/bitchat/android/nostr/RelayReconnectPolicyTest.kt diff --git a/app/src/main/java/com/bitchat/android/nostr/NostrRelayManager.kt b/app/src/main/java/com/bitchat/android/nostr/NostrRelayManager.kt index 772e2a60..81c1cac6 100644 --- a/app/src/main/java/com/bitchat/android/nostr/NostrRelayManager.kt +++ b/app/src/main/java/com/bitchat/android/nostr/NostrRelayManager.kt @@ -14,8 +14,6 @@ import okhttp3.* import java.util.UUID import java.util.concurrent.ConcurrentHashMap import java.util.concurrent.atomic.AtomicBoolean -import kotlin.math.min -import kotlin.math.pow /** * Manages WebSocket connections to Nostr relays @@ -48,12 +46,8 @@ class NostrRelayManager private constructor() { "wss://nostr21.com" ) - // Exponential backoff configuration (same as iOS) - private const val INITIAL_BACKOFF_INTERVAL = com.bitchat.android.util.AppConstants.Nostr.INITIAL_BACKOFF_INTERVAL_MS // 1 second - private const val MAX_BACKOFF_INTERVAL = com.bitchat.android.util.AppConstants.Nostr.MAX_BACKOFF_INTERVAL_MS // 5 minutes - private const val BACKOFF_MULTIPLIER = com.bitchat.android.util.AppConstants.Nostr.BACKOFF_MULTIPLIER - private const val MAX_RECONNECT_ATTEMPTS = com.bitchat.android.util.AppConstants.Nostr.MAX_RECONNECT_ATTEMPTS - + // Reconnect backoff lives in RelayReconnectPolicy. + // Track gift-wraps we initiated for logging private val pendingGiftWrapIDs = ConcurrentHashMap.newKeySet() @@ -1018,36 +1012,15 @@ class NostrRelayManager private constructor() { if (!desiredConnected.get() || !isNetworkActionAllowed(connectionToken) ) return - - // Check if this is a DNS error - val errorMessage = error.message?.lowercase() ?: "" - if (errorMessage.contains("hostname could not be found") || - errorMessage.contains("dns") || - errorMessage.contains("unable to resolve host")) { - - val relay = relaysList.find { it.url == relayUrl } - if (relay?.lastError == null) { - Log.w(TAG, "Nostr relay DNS failure; not retrying") - } - return - } - - // Implement exponential backoff for non-DNS errors + + // Every failure backs off and retries, including a name-resolution + // failure: "unable to resolve host" is what this device reports when it + // simply has no network, so treating it as permanent turns a walk + // through a tunnel into a dead relay layer for the rest of the process. val relay = relaysList.find { it.url == relayUrl } ?: return - relay.reconnectAttempts++ - - // Stop attempting after max attempts - if (relay.reconnectAttempts >= MAX_RECONNECT_ATTEMPTS) { - Log.w(TAG, "Max Nostr relay reconnection attempts reached") - return - } - - // Calculate backoff interval - val backoffInterval = min( - INITIAL_BACKOFF_INTERVAL * BACKOFF_MULTIPLIER.pow(relay.reconnectAttempts - 1.0), - MAX_BACKOFF_INTERVAL.toDouble() - ).toLong() - + relay.reconnectAttempts = RelayReconnectPolicy.nextAttempt(relay.reconnectAttempts) + val backoffInterval = RelayReconnectPolicy.backoffMs(relay.reconnectAttempts) + relay.nextReconnectTime = System.currentTimeMillis() + backoffInterval Log.d(TAG, "Scheduling Nostr relay reconnection") diff --git a/app/src/main/java/com/bitchat/android/nostr/RelayReconnectPolicy.kt b/app/src/main/java/com/bitchat/android/nostr/RelayReconnectPolicy.kt new file mode 100644 index 00000000..199e0dcf --- /dev/null +++ b/app/src/main/java/com/bitchat/android/nostr/RelayReconnectPolicy.kt @@ -0,0 +1,45 @@ +package com.bitchat.android.nostr + +import com.bitchat.android.util.AppConstants +import kotlin.math.min +import kotlin.math.pow + +/** + * Reconnect schedule for a Nostr relay socket. + * + * A phone loses its data connection constantly — airplane mode, a tunnel, a + * Wi-Fi to cellular handover, a dead zone — and every relay fails at once when + * it does. The schedule therefore has to survive an outage of arbitrary length + * and heal on its own, because nothing else will: the relay layer registers no + * connectivity callback, the periodic subscription validator only repairs + * subscriptions on sockets that are already open, and `connect()` runs once at + * startup. + * + * So the backoff grows exponentially and then *saturates* rather than + * terminating. Retrying forever at the ceiling costs one connection attempt per + * relay per [AppConstants.Nostr.MAX_BACKOFF_INTERVAL_MS]; giving up costs the + * user every internet DM, delivery receipt and geohash channel until they + * notice and restart the app. + */ +internal object RelayReconnectPolicy { + + /** + * Attempt count past which the interval stops growing. Beyond this the + * exponential term is already above the ceiling, so pinning it keeps the + * schedule at a steady [AppConstants.Nostr.MAX_BACKOFF_INTERVAL_MS] and + * keeps the exponent from running away over a long outage. + */ + const val SATURATION_ATTEMPTS: Int = AppConstants.Nostr.MAX_RECONNECT_ATTEMPTS + + /** Attempt number to record after a failure at [previousAttempts]. */ + fun nextAttempt(previousAttempts: Int): Int = + (previousAttempts.coerceAtLeast(0) + 1).coerceAtMost(SATURATION_ATTEMPTS) + + /** Delay before the reconnect for a given attempt number (1-based). */ + fun backoffMs(attempt: Int): Long { + val step = attempt.coerceAtLeast(1) + val exponential = AppConstants.Nostr.INITIAL_BACKOFF_INTERVAL_MS * + AppConstants.Nostr.BACKOFF_MULTIPLIER.pow(step - 1.0) + return min(exponential, AppConstants.Nostr.MAX_BACKOFF_INTERVAL_MS.toDouble()).toLong() + } +} diff --git a/app/src/test/kotlin/com/bitchat/android/nostr/RelayReconnectPolicyTest.kt b/app/src/test/kotlin/com/bitchat/android/nostr/RelayReconnectPolicyTest.kt new file mode 100644 index 00000000..f5e2581a --- /dev/null +++ b/app/src/test/kotlin/com/bitchat/android/nostr/RelayReconnectPolicyTest.kt @@ -0,0 +1,96 @@ +package com.bitchat.android.nostr + +import com.bitchat.android.util.AppConstants +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * The relay layer has no connectivity callback, its periodic validator only + * repairs subscriptions on sockets that are already open, and `connect()` runs + * once at startup. The backoff schedule is therefore the only thing that can + * bring relays back after the phone loses its data connection, so it has to + * saturate rather than terminate. + */ +class RelayReconnectPolicyTest { + + private val initial = AppConstants.Nostr.INITIAL_BACKOFF_INTERVAL_MS + private val ceiling = AppConstants.Nostr.MAX_BACKOFF_INTERVAL_MS + + @Test + fun `the first retry waits the initial interval`() { + assertEquals(initial, RelayReconnectPolicy.backoffMs(RelayReconnectPolicy.nextAttempt(0))) + } + + @Test + fun `the interval doubles per attempt until it reaches the ceiling`() { + var attempt = 0 + var previous = 0L + var sawCeiling = false + + repeat(RelayReconnectPolicy.SATURATION_ATTEMPTS) { + attempt = RelayReconnectPolicy.nextAttempt(attempt) + val delay = RelayReconnectPolicy.backoffMs(attempt) + + assertTrue("delay must never exceed the ceiling", delay <= ceiling) + if (delay == ceiling) { + sawCeiling = true + } else { + assertEquals("expected doubling below the ceiling", maxOf(initial, previous * 2), delay) + } + previous = delay + } + + assertTrue("the schedule must actually reach the ceiling", sawCeiling) + } + + @Test + fun `an outage longer than the schedule keeps retrying at the ceiling`() { + var attempt = 0 + // Far past the old give-up point; a real outage can last hours. + repeat(500) { attempt = RelayReconnectPolicy.nextAttempt(attempt) } + + assertEquals(RelayReconnectPolicy.SATURATION_ATTEMPTS, attempt) + assertEquals(ceiling, RelayReconnectPolicy.backoffMs(attempt)) + } + + @Test + fun `a long outage cannot run the attempt counter or the exponent away`() { + var attempt = 0 + repeat(10_000) { attempt = RelayReconnectPolicy.nextAttempt(attempt) } + + val delay = RelayReconnectPolicy.backoffMs(attempt) + assertTrue("delay must stay finite and bounded", delay in 1..ceiling) + } + + @Test + fun `a successful connection resets the schedule to the initial interval`() { + var attempt = 0 + repeat(6) { attempt = RelayReconnectPolicy.nextAttempt(attempt) } + assertTrue(RelayReconnectPolicy.backoffMs(attempt) > initial) + + // updateRelayStatus zeroes reconnectAttempts on a successful open. + attempt = 0 + + assertEquals(initial, RelayReconnectPolicy.backoffMs(RelayReconnectPolicy.nextAttempt(attempt))) + } + + @Test + fun `a nonsensical stored attempt count still yields a usable delay`() { + assertEquals(initial, RelayReconnectPolicy.backoffMs(RelayReconnectPolicy.nextAttempt(-5))) + assertTrue(RelayReconnectPolicy.backoffMs(0) in 1..ceiling) + assertTrue(RelayReconnectPolicy.backoffMs(Int.MAX_VALUE) in 1..ceiling) + } + + @Test + fun `the whole schedule stays under an hour of total wait before the ceiling`() { + var attempt = 0 + var total = 0L + repeat(RelayReconnectPolicy.SATURATION_ATTEMPTS) { + attempt = RelayReconnectPolicy.nextAttempt(attempt) + total += RelayReconnectPolicy.backoffMs(attempt) + } + + assertTrue("reaching the steady state must not take an hour", total < 60 * 60 * 1000L) + } +} From fa1727de2f9daebea1ce322653d2e326a6f40c34 Mon Sep 17 00:00:00 2001 From: callebtc <93376500+callebtc@users.noreply.github.com> Date: Mon, 7 Sep 2026 23:29:23 +0300 Subject: [PATCH 11/11] docs: simplify packet deduplication comment --- .../bitchat/android/mesh/SecurityManager.kt | 24 +------------------ 1 file changed, 1 insertion(+), 23 deletions(-) diff --git a/app/src/main/java/com/bitchat/android/mesh/SecurityManager.kt b/app/src/main/java/com/bitchat/android/mesh/SecurityManager.kt index 7efa7fb8..6fab9f03 100644 --- a/app/src/main/java/com/bitchat/android/mesh/SecurityManager.kt +++ b/app/src/main/java/com/bitchat/android/mesh/SecurityManager.kt @@ -239,29 +239,7 @@ class SecurityManager(private val encryptionService: EncryptionService, private return encryptionService.getCombinedPublicKeyData() } - /** - * Generate message ID for duplicate detection - */ - /** - * Identity used for replay and duplicate detection. - * - * This was a 32-bit `contentHashCode()` over at most the first 64 bytes of - * the payload. Two packets from the same peer in the same millisecond that - * agreed on that prefix collided, and a collision here is a *dropped - * message* — the second packet is discarded as a duplicate and there is no - * signal that it happened. - * - * `PacketIdUtil` is the identity the rest of the stack already uses for - * exactly this question (gossip sync membership, message IDs in - * `MessageHandler`), and iOS derives it the same way: the first 16 bytes of - * SHA-256 over type, senderID, timestamp and the **whole** payload. Using - * it here makes the security path agree with the sync path instead of - * carrying a weaker private notion of "same packet". - * - * Peer scoping is kept: `PacketIdUtil` covers the packet's own senderID, - * while this key is scoped by the peer the packet was received from, and - * those are not the same thing for a relayed packet. - */ + /** Deduplicates by peer and a hash of packet type, sender, timestamp, and full payload. */ private fun generateMessageID(packet: BitchatPacket, peerID: String): String { return "$peerID-${PacketIdUtil.computeIdHex(packet)}" }