Add double-ratchet encryption for Nostr relay DMs#697
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3dc93c46a7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| val combined = if (existingBuffer.size >= end) { | ||
| existingBuffer.copyOf() | ||
| } else { | ||
| existingBuffer.copyOf(end) |
There was a problem hiding this comment.
Reset accumulator when a new write starts at offset 0
The accumulator keeps the previous buffer length even when a fresh write starts at offset == 0, so after any truncated larger packet, later smaller packets from the same device can never satisfy isContiguousFromStart (onlyRange.last + 1 will stay smaller than the stale buffer size). In practice this can permanently drop subsequent messages on that connection until disconnect/clear, because the pending state is never reinitialized for a new packet boundary.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 16d7dd8. A fresh write at offset 0 now starts a new accumulator when the previous pending write already had a zero-based range, and there is a regression test for a truncated packet followed by a smaller complete packet.
| connectionTracker.getSubscribedDevices().forEach { device -> | ||
| if (device.address == routed.relayAddress) return@forEach | ||
| if (connectionTracker.addressPeerMap[device.address] == senderID) return@forEach | ||
| candidateLimits += BlePacketBudget.packetLimitBytesForMtu(null) |
There was a problem hiding this comment.
Honor negotiated MTU for subscribed server connections
This path always assumes the default 182-byte payload budget for subscribed (server-side) devices instead of using the per-device MTU tracked in BluetoothConnectionTracker. If a peer negotiates a smaller MTU (or falls back to 23), fragment sizing can exceed the real ATT payload limit, which leads to notification failures/timeouts and dropped packets on those links.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 16d7dd8. Subscribed/server-side devices now use the tracked per-device packet limit from BluetoothConnectionTracker instead of the default MTU budget.
| val maxDataSize = (maxPacketSize - packetOverhead) | ||
| .coerceAtMost(MAX_FRAGMENT_SIZE) | ||
| .coerceAtLeast(64) |
There was a problem hiding this comment.
Remove minimum fragment size that violates link budget
Clamping maxDataSize to at least 64 bytes can produce fragments larger than maxPacketSize on low-MTU links, and it also makes the subsequent maxDataSize <= 0 safeguard unreachable. When the negotiated packet budget is below packetOverhead + 64, this logic generates frames that cannot fit on the link and causes repeated delivery failures instead of gracefully aborting fragmentation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 16d7dd8. Removed the 64-byte minimum clamp so fragment payload sizing respects the actual link budget; added a low-MTU regression test that verifies each fragment frame stays within budget.
061a4cb to
40e674f
Compare
c18c252 to
a5e4052
Compare
Summary
This adds double-ratchet encryption for Nostr relay private messages, backed by vendored
ndr-ffiartifacts from nostr-double-ratchetv0.0.97-4-gcc3b83a(cc3b83a4cbe8dfb245f9b7192244c21115bb16f9).Why double ratchet
What changed
ndr-ffiJNI/UniFFI bindings and provenance metadata.Vendored FFI note
This PR vendors prebuilt
ndr-ffiartifacts from nostr-double-ratchet so the app can use one shared Rust implementation across macOS/iOS/Android without requiring every app build to install Rust, UniFFI, Apple cross-compilation targets, and the Android NDK. The vendored artifacts are pinned to a source commit and include provenance metadata.The Android release artifacts are produced by the source repo build script and stripped with the NDK
llvm-strip --strip-unneededtool, removing non-runtime symbol tables from the shipped.sofiles. That keeps the vendored native payload smaller without changing runtime exports.If preferred before merge, this can be changed to build
ndr-ffifrom source during CI/app builds, or to reference thenostr-double-ratchetrepo as a pinned submodule/dependency. A native Swift/Kotlin implementation is also possible, but I avoided that here because duplicating the double-ratchet protocol across app codebases would increase security and compatibility risk compared with using the same tested Rust implementation everywhere.Verification
ANDROID_NDK_HOME=/Users/sirius/Library/Android/sdk/ndk/28.2.13676358 NDK_HOME=/Users/sirius/Library/Android/sdk/ndk/28.2.13676358 ./scripts/mobile/build-android.sh --releasefrom nostr-double-ratchet./gradlew testDebugUnitTest --rerun-tasks./gradlew assembleDebug