perf: default the orders transport to v2 (NIP-44 kind 14) - #704
Conversation
The gift wrap transport (kind 1059 / NIP-59) is obsolete in the Mostro protocol, but resolveTransport still fell back to it whenever the node's info event had not arrived. At every cold start the app therefore opened a useless kind-1059 REQ on every relay and then closed and re-opened it as kind 14 once protocol_version resolved - a free resubscription (and its replay) per launch. The default and unknown versions now resolve to v2; only an explicit protocol_version 1 selects the legacy path. Deleting the 1059/NIP-59 branches entirely (filters, wrap, unwrap, background isolate) is the follow-up once the team confirms no v1 nodes remain (plan item 3.6).
|
Warning Review limit reachedNext included review available in 25 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (13)
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: d7ecbefc32
ℹ️ 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".
| case null: | ||
| // v2 is the live transport. Defaulting to it when the node info has | ||
| // not arrived yet avoids a useless kind-1059 REQ at every cold start | ||
| // followed by a CLOSE + re-REQ once protocol_version resolves. | ||
| return Transport.nip44; |
There was a problem hiding this comment.
When a user sends an order during cold start against a supported protocol_version=1 node, MostroService.publishOrder passes mostroInstance?.protocolVersion as null, so this branch now emits an unsupported kind-14 message that the v1 node ignores. The later instance listener only replaces the receive subscription; it cannot retry the lost outbound action. Limit the v2 default to the initial orders subscription optimization, or wait for transport discovery before selecting the outbound envelope.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 80dffa5 — but by waiting for transport discovery, not by preserving the v1 default on the send path.
The concern is real: an outbound envelope on the wrong transport is dropped by the node and nothing retries it. Keeping null → giftWrap for sends does not fix it though, it just moves the loss: on a v2 node (the mandatory case now) a cold-start send would go out as kind 1059 and be ignored exactly the same way. Either default is a guess, and one of the two nodes always loses.
So the send path no longer guesses. OpenOrdersRepository.awaitMostroInstance() returns the cached kind-38385 info event, or waits for it (bounded, 3s, well under the 10s orphan-session cleanup timer) and logs + falls back to the previous defaults on timeout. Applied at every wrapForTransport call site: MostroService.publishOrder, DisputeRepository, and the three restore requests. Same call also removes the PoW-0 guess those paths had, which had the same root cause.
The v2 default in resolveTransport stays as the last-resort fallback for the case where the info event genuinely never arrives.
Tests: test/data/repositories/open_orders_repository_await_instance_test.dart pins the three outcomes (cached, resolves on arrival, null on timeout). Full suite green (1196), flutter analyze clean.
Catrya
left a comment
Catrya
left a comment
There was a problem hiding this comment.
The behaviour change is correct and I verified the win is real. _initMostroInstanceListener guards on if (newTransport == _appliedOrdersTransport) return; (subscription_manager.dart:87), so with this change a cold start resolves nip44, the node's info event (protocol_version=2) resolves nip44 too, and the CLOSE + re-REQ on every relay no longer happens. That was the only recurring v1 cost in the app, and it is gone.
I also checked the one thing that could have quietly broken it: MostroInstance.protocolVersion is a non-nullable int defaulting to 1 (mostro_instance.dart:81), which would have collapsed the "not advertised" vs "advertised 1" distinction the resolver depends on. It does not apply here — subscription_manager.dart:124 passes infoEvent?.protocolVersion where infoEvent is a NostrEvent?, so it goes through the nullable raw-tag getter (mostro_instance.dart:186). The distinction is preserved and the commit message is accurate.
I agree with flipping the default: case too. A node advertising 3 is far more likely to speak v2 than v1, so assuming v2 is the better guess.
flutter analyze is clean apart from the two pre-existing stale-mock errors, the full suite shows no new failures against main, and transport_test.dart covers all five branches of the switch.
What blocks this is that the rule changed in one function while three places around it still state — and in one case still execute — the old rule.
1. The catch fallback still selects v1 (this is code, not a comment)
// subscription_manager.dart:118-129
/// Resolves the transport for the orders subscription from the connected
/// node's advertised `protocol_version` (§2, §4.1). Defaults to v1 gift wrap
/// when the node info is not yet available or unreadable. // no longer true
Transport _resolveOrdersTransport() {
try {
final infoEvent = ref.read(orderRepositoryProvider).mostroInstance;
return resolveTransport(infoEvent?.protocolVersion);
} catch (e) {
logger.w('Failed to resolve orders transport, defaulting to v1: $e');
return Transport.giftWrap; // still v1
}
}
If this branch ever fires, the app opens a kind-1059 REQ on every relay and stays pinned to it for the whole session against a node that will never answer there — silently, with only a logger.w. That is precisely the failure this PR exists to remove, left alive inside the PR's own blast radius. It should return Transport.nip44, with the log wording updated to match.
Given that advertising protocol_version is mandatory, this fallback has no remaining justification: it exists to cover "I don't know what the node speaks", and that case now has one correct answer.
2. resolveTransport's own docstring contradicts the code
transport.dart:18-25 still reads:
/// - `null` → [Transport.giftWrap]. The tag is absent or the node info has not
/// been fetched yet; during the migration window this is the common legacy
/// case, so it resolves to v1 without noise.
/// - any other value → [Transport.giftWrap], logged at `warn`. We do not speak
/// that protocol, so we degrade to v1 (version-skew guard) ...
Both statements are now the opposite of what the function does. This is not an incidental comment: the enum Transport docstring five lines above says the enum exists "so the send path, the receive subscription filters and the message version field cannot drift out of sync". A file whose only job is documenting the negotiation rule, documenting it backwards, is exactly the drift it was written to prevent — and CLAUDE.md points readers here as the source of truth.
Worth writing down why the version-skew guard was dropped, too, rather than just deleting the rationale.
3. subscription_manager.dart:78-79
▎ "the transport resolved at subscription time (typically v1 at cold start)"
No longer the case.
Non-blocking
The restore flow still subscribes to both transports unconditionally. restore_manager.dart:222-240:
// The node info (kind 38385) that advertises protocol_version may not have
// loaded yet when restore starts, so we subscribe to v1 gift wrap (kind 1059)
// and v2 NIP-44 direct (kind 14) simultaneously rather than resolving a
// single transport — the node answers on whichever it speaks.
final v1Filter = NostrFilter(kinds: [1059], p: [_tempTradeKey!.public], limit: 0);
final v2Filter = NostrFilter(kinds: [14], authors: [mostroPubkey], p: [...], limit: 0);
The justification in that comment is the exact reasoning this PR invalidates — "node info may not have loaded yet" now resolves to v2. The runtime cost is small (one extra filter per relay, restore only, limit: 0 so no history replay), which is why I am not blocking on it, but it is the one remaining place that opens a kind-1059 REQ against a v2 node, and leaving it makes the full v1 removal in plan item 3.6 harder to reason about. Note also that the v1 filter has no authors pin while the v2 one does, so it accepts gift wraps to the temp key from anyone.
MostroInstance.protocolVersion default. It still defaults to 1 (mostro_instance.dart:81) with a docstring saying that matches legacy-daemon behaviour, which no longer matches how the app actually negotiates. The About screen (about_screen.dart:341) would therefore show "Protocol Version: 1" for a node the app is speaking v2 to. Harmless functionally, but it is wrong information shown to exactly the person debugging this. Separate layer, so a follow-up issue is fine.
For the record, I did trace every remaining kind-1059 site: the orders filter (subscription_manager.dart:601) is now correctly gated behind an explicit protocol_version = 1, and the rest — mostro_service.dart:130, restore_manager.dart:1158, the legacy pre-migration rows in chat_room_notifier.dart:398 / dispute_chat_notifier.dart:366, and the FCM kind guard in background_notification_service.dart:246 — are decode-only paths with no relay subscription behind them. After the two fixes above, no v1 REQ is opened against a v2 node.
Review follow-ups on the v2 transport default. - Send paths (order actions, dispute creation, the three restore requests) now await the node's kind-38385 info event via OpenOrdersRepository.awaitMostroInstance() instead of reading whatever happens to be cached. Guessing the transport is unrecoverable: the node drops the envelope it does not speak and nothing retries the action. The wait is bounded (3s) and falls back to the previous defaults, so an unreachable node degrades rather than blocks the UI. Fixes the PoW-0 guess on the same paths. - SubscriptionManager._resolveOrdersTransport()'s catch fallback returned Transport.giftWrap, which would pin a whole session to a kind-1059 REQ a v2 node never answers. It now matches resolveTransport and returns v2. - resolveTransport's docstring documented the old rule (null and unknown versions → v1) and the reason the version-skew guard existed. Rewritten to the rule the code implements, with why the guard was dropped. Same for the two stale comments in subscription_manager and restore_manager. - Restore opens a single filter for the resolved transport instead of subscribing to kinds 1059 and 14 unconditionally; the node info has already been polled for by then. Both filters remain only when the info event never arrived. Documents why the v1 filter has no authors pin (NIP-59 signs the outer wrap with an ephemeral key). - MostroInstance.protocolVersion is nullable, so the About screen no longer reports "1" for a node the app speaks v2 to.
|
Thanks for the trace — all three blockers plus both non-blocking items are addressed in 80dffa5. 1. The catch fallback still selected v1 — fixed. 2. 3. Non-blocking: restore's dual subscription — fixed rather than deferred, since your point that it makes the full v1 removal harder to reason about is the real cost. Non-blocking: Also, from the Codex review: the send path no longer guesses the transport at all. Every Tests: new |
subscription_filter_diff_test's warm-up race test matched a chat REQ by "any filter with kind 14". The orders subscription is kind 14 too now that the transport defaults to v2, so the orders REQ was counted as a second chat REQ and the test failed on the merge with main. Chat filters are keyed on the peer-derived K_sign pubkeys and the orders filter on the node's pubkey, so the author separates them regardless of kind.
2 participants
Summary
Item 3.6 of the performance plan. The gift wrap transport (kind 1059 / NIP-59) is obsolete in the Mostro protocol, but
resolveTransportstill defaulted to it whenever the node's kind-38385 info event had not arrived yet. Every cold start therefore:protocol_versionresolved — a free resubscription (and its history replay) per launch, plus a live dead-transport subscription window.Changes
resolveTransport:null(info not yet fetched) and unknown versions resolve toTransport.nip44; only an explicitprotocol_version: 1selects the legacy path. The transport-change listener still re-subscribes if a legacy node later advertises v1.Follow-up (needs team confirmation): once no v1 nodes remain supported, delete the 1059/NIP-59 branches entirely —
buildOrdersFilter's 1059 arm,wrapForTransport's gift wrap arm,decryptNIP59Event/createNIP59Event, and the background isolate's mirror paths. That removes a second decrypt pipeline from the hot path.Test plan
transport_test.dartupdated pins + mostro/subscriptions/data suites — greenflutter test— all greenflutter analyze— no new issues🤖 Generated with Claude Code
https://claude.ai/code/session_018fTxqxhpdL5siTgKZqwtur