feat(chat): migrate P2P chat to the gift-wrap-free envelope - #247
Conversation
Implements https://mostro.network/protocol/chat.html (issue #246), replacing the simplified NIP-59 gift wrap whose random ephemeral authors allowed unattributable third-party flooding of any active conversation. - crypto/chat_keys.rs: HKDF-SHA256 split of the trade-key ECDH secret into K_conv (encryption / p tag) and K_sign (outer author), verified against the spec test vector. - nostr/gift_wrap.rs: mostro_wrap / mostro_unwrap — outer kind 14 signed with K_sign carrying a NIP-44 encrypted kind 1 inner event signed by the sender's trade key; full crypto-side validation with a test per rejection path. NIP-59 wrap/unwrap stays for dispute admin chat only. - api/messages.rs: subscription pinned to authors=[pub(K_sign)], bounded by a persisted per-order since cursor (clamped to the local clock) + limit; cheapest-check-first pipeline with outer-id LRU and a 30/min (burst 60) token bucket before any crypto work; flood breaker; inner signature verified and checked against the order's two trade keys; durable replay dedup on the inner event id; attachments ride the same envelope. - Chat history now persists to the messages table (write-through store); message_exists added to the Storage trait (web stub answers false, #233). - No dual-read window: kind 1059 is no longer accepted for peer chat. mostro-core 0.14.1 still ships the superseded envelope; this stays local until the canonical implementation lands upstream.
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughPeer chat migrated from NIP-59 gift-wrap events to HKDF-derived Kind 14/NIP-44 envelopes. Sending, receiving, durable persistence, replay deduplication, rate limiting, cursor tracking, reconnect handling, tests, and protocol documentation were updated. ChangesPeer chat migration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant TradeKeys
participant OrderFlow
participant ChatPublisher
participant Relay
participant ChatSubscriber
participant MessageStore
OrderFlow->>TradeKeys: derive K_conv and K_sign
OrderFlow->>ChatPublisher: send text or file pointer
ChatPublisher->>Relay: publish signed Kind 14 envelope
Relay->>ChatSubscriber: deliver author-filtered event
ChatSubscriber->>MessageStore: validate, deduplicate, and persist inner message
Possibly related issues
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8adaf6574f
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let Some(db) = crate::db::app_db::db() { | ||
| if let Err(e) = db.mark_messages_read(trade_id).await { | ||
| log::warn!("[messages] mark_messages_read failed trade={trade_id}: {e}"); |
There was a problem hiding this comment.
On native storage this call updates only the denormalized messages.is_read column, while list_messages reconstructs each ChatMessage exclusively from the unchanged JSON in messages.data. After marking a room read and restarting, hydration therefore restores every previously unread message with is_read: false, causing unread badges to reappear. The update must also rewrite the JSON or reads must overlay the column value.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b9f0ff3. mark_messages_read now also rewrites the JSON blob (json_set(data, '$.is_read', json('true')) — json('true') keeps it a JSON boolean so deserialization doesn't break). mark_messages_read_survives_rehydration closes and reopens the DB and asserts the flag survives.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/src/api/messages.rs (1)
878-894: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftSubscription is never closed and can be spawned repeatedly for the same order.
client.subscribe(filter, None)uses an auto-generated id that is never unsubscribed on any of the exit paths (idle timeout, flood trip, shutdown, subscribe-loopreturn). Combined withorders.rs::on_peer_pubkey_received, which spawns this task unconditionally every time aBuyerTookOrder/HoldInvoicePaymentAcceptedgift wrap arrives (including replays/reconnect backfills), a single order can accumulate several live relay subscriptions and tasks that all process the same events and race on cursor writes.Consider an explicit
SubscriptionIdper order plus a guard (e.g. anorder_id → activeset) so a second spawn is a no-op, andclient.unsubscribe(&id)before returning.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/src/api/messages.rs` around lines 878 - 894, Update the incoming chat subscription flow around subscribe_incoming_chat and orders.rs::on_peer_pubkey_received to assign an explicit SubscriptionId per order and guard against duplicate active tasks for the same order. Make repeated BuyerTookOrder or HoldInvoicePaymentAccepted notifications a no-op while a task is active, and ensure client.unsubscribe(&id) runs on every exit path, including idle timeout, flood protection, shutdown, and subscription failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/src/api/messages.rs`:
- Around line 977-1017: The incoming-chat cursor in the event-processing flow
must not advance until the message has been durably stored. Move the
accepted_ts/cursor update and store_chat_cursor call to occur after
message_store().add_message succeeds, or update add_message to report
persistence failure and only advance on success; preserve duplicate handling and
cursor clamping.
---
Outside diff comments:
In `@rust/src/api/messages.rs`:
- Around line 878-894: Update the incoming chat subscription flow around
subscribe_incoming_chat and orders.rs::on_peer_pubkey_received to assign an
explicit SubscriptionId per order and guard against duplicate active tasks for
the same order. Make repeated BuyerTookOrder or HoldInvoicePaymentAccepted
notifications a no-op while a task is active, and ensure client.unsubscribe(&id)
runs on every exit path, including idle timeout, flood protection, shutdown, and
subscription failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4689856f-e731-460c-83e3-740a76832ca4
⛔ Files ignored due to path filters (1)
rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
CLAUDE.mdrust/Cargo.tomlrust/src/api/messages.rsrust/src/api/orders.rsrust/src/crypto/chat_keys.rsrust/src/crypto/mod.rsrust/src/db/indexeddb.rsrust/src/db/mod.rsrust/src/db/sqlite.rsrust/src/nostr/gift_wrap.rsspecs/004-mostro-p2p-client/contracts/messages.md
grunch
left a comment
grunch
left a comment
There was a problem hiding this comment.
Strict review — blocking changes required
Reviewed exact head 8adaf6574fed215e452f6402b254ccd82344d743 after reading all existing review threads and the completed automated reviews.
I confirmed the existing blockers and did not duplicate them inline: native message persistence fails on the messages.trade_id -> trades.id foreign key; catch-up can be dropped by the live token bucket; read state is not serialized durably; the cursor advances before durable storage; and subscriptions/tasks have no single-owner/unsubscribe lifecycle.
The new-envelope crypto and signature checks look sound against the normative test vector, but the migration and lifecycle are not safe to ship yet. In addition to the inline blockers below, the Rust check is red: CI reproduced the foreign-key failure and mark_as_read_updates_count failed (183 passed, 1 failed), while the same exact-head suite passed locally (184 passed). That inconsistent result also shows the global message-store tests are not isolated.
Please address all unresolved blocking threads and add integration coverage for mixed-version peers, idle/restart recovery, catch-up above the burst size, and persistence using the real TradeInfo.id / order-id relationship.
| // locally (the cursor only advances on accepted messages). | ||
| let mut cursor = load_chat_cursor(&order_id).await.unwrap_or(0); | ||
| let mut filter = nostr_sdk::Filter::new() | ||
| .kind(nostr_sdk::Kind::PrivateDirectMessage) |
There was a problem hiding this comment.
Blocking: this is a hard cutover even though #246 requires a dual-read migration window. The pre-migration client listens for kind 1059 and publishes the old envelope; this head listens only for kind 14 here and publishes only the new envelope. During a staggered rollout, an updated peer and an older peer therefore cannot receive either direction of chat, including chats for already-active trades.
Please use a write-new/read-both transition (or another explicit version-negotiation strategy), retain the legacy receiver until the documented deprecation point, and test both mixed-version directions. Removing compatibility in the same release that introduces the new wire format breaks the linked issue's acceptance criterion.
There was a problem hiding this comment.
Fixed in b9f0ff3 — write-new/read-both, as requested. Outbound is always the new envelope; inbound additionally accepts kind 1059 from pre-migration peers until LEGACY_CHAT_DEPRECATION_TS (2026-12-31T00:00:00Z), after which the legacy subscription is simply not created. The legacy path decrypts with the shared secret as the recipient key (as v1 wraps it) and is bounded by the same outer-id LRU, live-stream budget, pre-decryption size cap, durable dedup and retention quota. Mixed-version coverage: legacy_gift_wrap_is_accepted_during_the_window exercises v1→v2 delivery, durable replay dedup, and rejection of a stranger-authored rumor.
|
|
||
| /// Token bucket sizing per the spec: ~30 messages/minute sustained with a | ||
| /// burst of 60, refused **before** any cryptographic work. | ||
| const RATE_CAPACITY: f64 = 60.0; |
There was a problem hiding this comment.
Blocking: the rate limiter bounds CPU rate but not durable storage growth, so the isolation invariant is still bypassable. A counterparty sending continuously at or below RATE_PER_SEC consumes no rejected-event streak, never trips FLOOD_TRIP_REJECTIONS, and every accepted payload is appended to the in-memory vector and SQLite without any per-trade message-count or byte quota. Over a long-running trade this can fill memory/disk; a full database then affects order/dispute persistence, exactly what chat isolation is supposed to prevent. The normative spec explicitly calls for caps on both message count and total bytes per trade.
Enforce bounded retention/quota before accepting or persisting, keep the durable inner-id replay set consistent with whatever retention window the cursor can still fetch, and test a sustained valid-rate stream—not only an over-rate burst.
There was a problem hiding this comment.
Fixed in b9f0ff3. Per-trade retention quotas are enforced before accepting/persisting any incoming message on both envelopes: MAX_STORED_MESSAGES_PER_TRADE = 1000 and MAX_STORED_BYTES_PER_TRADE = 5 MiB (MessageStore::quota_exceeded). A counterparty writing forever at a legitimate rate now hits the cap and further messages are dropped and logged, leaving order/dispute persistence untouched. The durable inner-id dedup set is the messages table itself, so it stays consistent with what the cursor can still fetch. Covered by quota_bounds_messages_and_bytes_per_trade (sustained valid-rate growth up to the byte boundary, not an over-rate burst).
| Err(e) => log::warn!("[messages] send_message trade={trade_id}: {e}"), | ||
| Ok(ctx) => { | ||
| sender_pubkey = ctx.trade_keys.public_key().to_hex(); | ||
| match publish_chat_payload(&ctx, &content).await { |
There was a problem hiding this comment.
A sender can report success for a message that every receiver running this code must reject. send_message accepts arbitrarily large non-empty content and publishes it here, while mostro_unwrap rejects any encrypted outer content larger than MAX_CONTENT_BYTES before decrypting. Since the NIP-44 ciphertext also includes the signed inner-event JSON and encoding overhead, a sufficiently long UI message is published and stored locally as sent but is silently discarded by the recipient.
Validate the final serialized/encrypted envelope size before publishing (or enforce a conservative plaintext limit), return a stable MessageTooLarge error, and add boundary tests that round-trip the largest accepted payload and reject the next byte.
There was a problem hiding this comment.
Fixed in b9f0ff3. mostro_wrap rejects post-encryption anything whose ciphertext exceeds MAX_CONTENT_BYTES with a stable MessageTooLarge: marker, send_message propagates it to the caller instead of storing a phantom 'sent' message, and a cheap plaintext bound short-circuits before any crypto. Boundary tests in oversized_message_is_refused_at_send_time: 60 KiB fails with the marker, 30 KiB round-trips. The exact largest-accepted byte is deliberately not pinned — NIP-44 padding plus JSON escaping make it content-dependent — so the guarantee tested is 'never publish what a receiver must reject'.
| return Err(anyhow!("outer event is dated too far in the future")); | ||
| } | ||
|
|
||
| if outer.content.len() > MAX_CONTENT_BYTES { |
There was a problem hiding this comment.
The 64 KiB check bounds only ciphertext, not the raw event that must be parsed and verified first. The protocol reference explicitly requires the caller to cap the raw wire event as well. With the current nostr-sdk defaults, a valid kind-14 relay message can be roughly 5 MiB and carry up to 2,000 tags. A peer holding K_sign can therefore keep content small, add a large set of irrelevant tags, and force JSON parsing, event-ID construction, and signature verification before this check and before the per-chat token bucket sees the event; the initial 60-event burst can make this hundreds of MiB of work.
Configure a raw event-size limit for kind 14 at the relay/client boundary and a tight tag-count limit appropriate to this envelope, then retain this payload limit for decryption. Add a test with small ciphertext plus oversized extra tags/raw JSON.
There was a problem hiding this comment.
Fixed in b9f0ff3. mostro_unwrap now bounds the raw event before any signature work: MAX_OUTER_TAGS = 8 (the envelope defines exactly one p tag plus optional NIP-13 nonce) rejects junk-tag padding ahead of hashing/verification, in addition to the existing pre-decryption ciphertext cap. Covered by junk_tag_padding_is_rejected_before_verification (small ciphertext + 2000 tags). Relay-side max_event_size-style limits remain an operator concern noted in the spec's relay considerations.
| // timestamp tweaking — it would break `since`-based sync. | ||
| let now = Timestamp::now(); | ||
|
|
||
| let inner = EventBuilder::text_note(message) |
There was a problem hiding this comment.
Two intentional identical messages sent in the same second collapse to one durable identity. The inner event uses second-resolution created_at, fixed empty tags, the same sender key/kind, and the text as content. Two rapid sends of the same text therefore produce the same signed inner event ID even though NIP-44 gives the outer envelopes different ciphertext/IDs; the receiver's inner-ID dedup drops the second one as a replay.
Give each inner chat message a signed uniqueness value (for example, a protocol-defined random nonce tag) and cover two identical same-second sends in the round-trip/dedup tests. Otherwise a normal double-send such as “yes” can be silently lost.
There was a problem hiding this comment.
Fixed in b9f0ff3. Each inner event now carries a signed 8-byte random nonce tag ([\"u\", <hex>]), so two identical same-second sends produce distinct inner ids and the receiver's dedup no longer eats the second one — covered by identical_same_second_sends_keep_distinct_identities. Agreed this wants a protocol-level definition: the spec's inner event shows empty tags, so I'd propose the nonce tag upstream in MostroP2P/protocol as part of the mostro-core migration.
…durability Addresses every review thread (Codex, CodeRabbit, grunch) and the red CI: - CI: mark_as_read_updates_count asserted on the global unread counter and raced with parallel tests — now asserts per-trade read state. - Persistence: messages.trade_id lost its FK to trades(id) (chat keys are per ORDER id; a taker's trades row is a fresh UUID, so every taker save_message failed). One-off table rebuild migrates v2 databases. - mark_messages_read now rewrites the is_read flag inside the JSON blob too — it survives rehydration instead of resurrecting unread badges. - The since cursor advances only after the message is durably stored; add_message reports persistence success. - Replay dedup fails closed: a storage lookup error drops the event. - Catch-up exemption: the token bucket meters only the live stream (post-EOSE); stored backlog is bounded by the filter limit instead, so history above the burst size is never dropped. - Lifecycle: one chat task per order (spawn guard), explicit subscription ids unsubscribed on every exit path, no 30-min idle death, and resubscription of persisted active trades when the pool comes online. - Dual-read migration window (write-new/read-both): inbound kind 1059 from pre-migration peers is accepted until LEGACY_CHAT_DEPRECATION_TS (2026-12-31T00:00:00Z), bounded by the same LRU/budget/size/dedup/quota; decryption uses the shared secret as the recipient key, as v1 sends it. - Per-trade retention quotas (1000 messages / 5 MiB) bound durable growth at a legitimate rate — the isolation invariant now covers storage. - Send-side size validation: stable MessageTooLarge error before publishing anything every receiver must reject. - Raw-event bound: tag-count cap before signature verification defeats junk-tag padding around the ciphertext cap. - Inner events carry a signed uniqueness nonce so two identical same-second sends keep distinct ids (dedup no longer eats double-sends). - Web: IndexedDB now implements chat messages + settings KV (durable replay dedup and cursor on web, fail-closed); the rest stays on #233. 193 rust tests green (17 new), clippy clean on touched files, wasm check green, flutter analyze/test green.
Review round 1 addressed — b9f0ff3All 11 inline threads (Codex ×3, CodeRabbit ×1, strict review ×7) are fixed and replied to individually, plus the two non-inline findings:
Also in this round, per the strict review: dual-read migration window (write-new/read-both until 2026-12-31T00:00:00Z, mixed-version test), no idle death + startup/reconnect resubscription, per-trade retention quotas (1000 msgs / 5 MiB), Verification: 193 Rust tests green (17 new), clippy clean on touched files, wasm check green, Flagged as follow-ups (not in this PR): live-relay integration harness for idle/restart recovery, a browser-level reload-replay test, and proposing the inner-event nonce tag upstream in MostroP2P/protocol. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
rust/src/db/indexeddb.rs (1)
157-186: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueOptional: full-store scans and per-message transactions.
list_messagesreads every persisted message and filters in Rust, andmark_messages_readopens one readwrite transaction per unread message (eachsave_messagecall). Fine at the retention caps (1000 msgs / 5 MiB per trade), but atrade_idindex on themessagesstore plus a single batched transaction would keep hydration cheap as trade count grows.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/src/db/indexeddb.rs` around lines 157 - 186, Optimize list_messages and mark_messages_read by adding and using a trade_id index on the messages store instead of scanning all persisted messages, and batch updates for all unread messages in one readwrite transaction rather than calling save_message per message. Preserve filtering, created_at sorting, and read-state behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/src/api/messages.rs`:
- Around line 949-978: Regenerate or validate the Flutter Rust Bridge bindings
for the changed subscribe_incoming_chat API in rust/src/api/messages.rs by
running ./scripts/frb-generate.sh --check. If the check reports differences,
regenerate and include the updated bridge files; otherwise leave generated code
unchanged.
- Around line 1342-1373: Update handle_legacy_chat_event so consecutive_rejected
is reset only after the sender passes the allowed_signers check; reject
unauthorized or invalid senders through state.reject(order_id) before returning
so flood protection remains effective. Replace the random UUID fallback for
missing rumor ids with immediate rejection and return, while preserving
message_store().is_known handling for rumors with a valid id.
In `@rust/src/db/schema.rs`:
- Around line 10-24: Update SQLITE_DROP_MESSAGES_FK_SQL to drop any pre-existing
messages_v3 before creating it, allowing interrupted migrations to be rerun.
Keep PRAGMA foreign_keys OFF/ON outside the transaction, and wrap the table
creation, data copy, drop, rename, and index statements in a transaction so the
rebuild is crash-safe.
---
Nitpick comments:
In `@rust/src/db/indexeddb.rs`:
- Around line 157-186: Optimize list_messages and mark_messages_read by adding
and using a trade_id index on the messages store instead of scanning all
persisted messages, and batch updates for all unread messages in one readwrite
transaction rather than calling save_message per message. Preserve filtering,
created_at sorting, and read-state behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 99c91a9f-7214-4947-8ecd-389157f04151
📒 Files selected for processing (9)
CLAUDE.mdrust/src/api/messages.rsrust/src/api/nostr.rsrust/src/api/orders.rsrust/src/db/indexeddb.rsrust/src/db/schema.rsrust/src/db/sqlite.rsrust/src/nostr/gift_wrap.rsspecs/004-mostro-p2p-client/contracts/messages.md
🚧 Files skipped from review as they are similar to previous changes (2)
- CLAUDE.md
- specs/004-mostro-p2p-client/contracts/messages.md
…fe migration - handle_legacy_chat_event: the rejected-streak reset moved after the allowed-signers check — anyone can wrap to the public legacy address, so unauthorized/malformed senders now count toward the flood breaker instead of resetting it. A rumor without an id is rejected outright: a fabricated fallback id would make the same rumor accepted again on every replay. - messages FK migration: rebuild wrapped in a transaction, stray messages_v3 from an interrupted attempt dropped first, foreign_keys pragma kept outside the transaction (SQLite ignores it inside one). Migration test now seeds a leftover messages_v3. - FRB bindings verified unchanged (subscribe_incoming_chat is pub(crate), not bridge surface): ./scripts/frb-generate.sh --check clean.
A database created before `messages` moved to a JSON `data` blob stores one column per field (`sender_pubkey`, `content_encrypted`, …) and also carries the FK to `trades(id)`. The FK is the only thing `migrate()` checked, so the v2→v3 rebuild fired and its `INSERT INTO messages_v3 SELECT id, trade_id, data, … FROM messages` failed with "no such column: data". That error propagates out of `SqliteStorage::open()`, so `initDb` failed entirely and the app fell back to memory-only mode — losing orders, trades, identity and the outbox on every launch, not just chat history. Detect the v1 table (present, no `data` column) and drop it: the `content_encrypted` rows are ciphertext the current chat code cannot read, so there is nothing to convert. The v2→v3 rebuild is now additionally gated on `data` existing, so it can never run against a schema without it. Regression test builds the v1 table by hand and asserts `open()` succeeds, the rebuilt table is v3 (JSON `data`, no FK), and `save_message` works.
The app is v2-native: it sends NIP-44 signed kind-14 events and nothing else. Protocol v1 (NIP-59 gift wrap) is being removed from this codebase, not implemented — outbound gift wrap already left the P2P chat in MostroP2P#247. Nothing read the node's `protocol_version` tag, though, so pointing the app at a v1 node produced no diagnosis at all: the daemon never decrypts a kind-14 event, never answers and never complains, and every send surfaced as a generic timeout indistinguishable from an unreachable relay. The reference node at mostro 0.18.0 advertises `["protocol_version","1"]` today, so this is not hypothetical. The tag is now parsed from the Kind 38385 event and daemon sends fail fast with an `UnsupportedNodeProtocol:` marker, which Dart maps to a localized message in the create- and take-order screens (new `nodeProtocolUnsupported` string in all five locales) telling the user to pick another node. An absent tag counts as supported: nodes predating it exist, the app has always spoken v2, and refusing them would break setups that work today. Only an explicit version other than 2 is a mismatch — including future versions, which are not assumed compatible. The decision is a pure function over the tag list, tested without touching the process-wide state: v2 supported, v1 not, absent supported, unknown future version not, plus malformed and valueless tags. 205 Rust tests, 190 Dart tests, clippy clean, wasm32 check passes.
…bscriptions The chat side (subscribe_incoming_chat) was already fixed via MostroP2P#247. This brings the two remaining per-trade subscriptions in orders.rs to parity: - subscribe_gift_wraps and subscribe_single_order called client.subscribe(_, None) with auto-generated ids and never unsubscribed, so the relay-side subscription survived the event loop's exit (30-min idle timeout, shutdown, or completed trade). Every create/take spawned a new one — they accumulated over a long session (bandwidth, duplicate notifications, relay pressure on mobile). Fix, mirroring the subscribe_incoming_chat pattern: - Deterministic ids (trade_subscription_id / single_order_subscription_id) via subscribe_with_id, so a repeat subscribe for the same trade/order replaces in place instead of stacking. Full pubkey hex, not an 8-char prefix, to rule out collisions. - unsubscribe(&sub_id) at each loop's single exit point, so the relay-side subscription never outlives the task. The limit(0) live-only semantics of subscribe_gift_wraps are unchanged — only the subscription id changes, not the filter. Central subscription registry deferred per the issue (nostr-sdk re-establishes subscriptions on reconnect). Unit tests cover the deterministic-id logic (idempotent per pubkey, no collision, expected format); the unsubscribe-on-exit is covered by inspection at the single exit point, mirroring the proven subscribe_incoming_chat cleanup, since it needs a live relay to exercise.
1 participant
Closes #246.
What
Migrates the peer-to-peer chat to the envelope specified in Peer-to-peer Chat (MostroP2P/protocol#52), replacing the simplified NIP-59 gift wrap. The outer event is now a kind 14 signed with
K_signandp-tagged topub(K_conv)— both HKDF-SHA256 derivations of the trade-key ECDH secret — carrying a NIP-44 encrypted kind 1 inner event signed by the sender's trade key. Third parties become cryptographically unable to publish into a conversation, and everything else is dropped at the relay by theauthorsfilter.Changes
rust/src/crypto/chat_keys.rs(new):derive_chat_keys— ECDH (generate_shared_key, raw x-coordinate) + HKDF-SHA256 split with domain separation (mostro:chat:conv:v1/mostro:chat:sign:v1) and the retry-byte fallback. Verified against the spec's test vector.rust/src/nostr/gift_wrap.rs:mostro_wrap/mostro_unwrapimplementing the envelope and the crypto-side validation steps (author, exactly-oneptag, tag-count bound before signature work, absolute future-timestamp bound, size, outer signature, NIP-44 self-decryption, inner signature verification, allowed-signer check, inner kind, relative timestamp bound). Send side rejects with a stableMessageTooLargeanything a receiver would discard, and each inner event carries a signed uniqueness nonce so identical same-second sends keep distinct ids.wrap/unwrap(NIP-59) remain for dispute admin chat.rust/src/api/messages.rs:kind 14 + authors=[pub(K_sign)], bounded by a persisted per-ordersincecursor (advanced only past durably stored messages, clamped tomin(ts, local_now)against cursor poisoning) plus alimit.mostro_unwrap→ durable inner-id dedup (fail-closed: lookup errors drop the event) → per-trade retention quotas (1000 msgs / 5 MiB). Sustained violation trips a flood breaker that halts chat processing while the trade stays fully operational.LEGACY_CHAT_DEPRECATION_TS(2026-12-31T00:00:00Z), bounded by the same LRU/budget/size/dedup/quota. Unauthorized senders on the legacy path count toward the flood breaker.resubscribe_active_chats()rebuilds listeners for persisted active trades whenever the relay pool comes online (startup and reconnect).author = mostro_pubkey; chat pinspub(K_sign); anything else is ignored.messagestable, hydrated per trade;message_existson theStoragetrait is the durable replay dedup. Themessages → tradesFK is dropped (chat keys are per order id; a taker's trades row is a fresh UUID) with a crash-safe, re-runnable table-rebuild migration.mark_messages_readrewrites the JSON blob so read state survives restarts.specs/004messages contract andCLAUDE.mdtransport section updated.Review history
trade_id-index/batching nitpick is deliberately deferred to Web: IndexedDB storage backend is a stub — nothing persists across a reload #233 (full IndexedDB backend): per-trade quotas cap the store at 1000 messages, so the full scan stays small, and the index belongs with the real schema work.Manual testing
Prereqs: a running
mostrod+ relay reachable by both clients (local regtest stack or the test node), and two devices/profiles of this app build — call them Alice (maker) and Bob (taker). Watch logs withflutter runconsoles; chat lines are tagged[messages].incoming-chat subscription active order=<id> author=<pub(K_sign)> since=0 legacy=true.nak req -k 14 <relay>), verify the chat events are kind 14, authored by the samepub(K_sign)both logs printed, with exactly oneptag — and that no event field contains either trade pubkey.resubscribing chat order=<id>, then the missed messages appear once, no duplicates, and history from before the restart is still there (persistence).since=<recent ts>(cursor persisted) and no backlog is re-downloaded.p-tagged to the conversation address from a third key (e.g. withnak). The clients must not log rejections (the relay-sideauthorsfilter drops them) and the trade UI must stay fully responsive; a dispute can still be opened.flutter run -d chrome— messages persist across a reload (IndexedDB) and no message is accepted twice after reload.Test plan (automated)
cargo test— 193 passed (30+ new: spec test vector, envelope rejection paths incl. junk-tag padding and send-size boundary, nonce uniqueness, token bucket + EOSE gating, LRU, quotas, payload parsing, durable dedup + FK migration + read-state rehydration via SQLite, legacy dual-read mixed-version path)cargo clippy --all-targets— clean on touched files./scripts/frb-generate.sh --check— bridge surface unchangedflutter analyze/flutter test— clean / 173 passed