perf: bound NWC response replay and memoize the countdown state lookup - #718
Conversation
- Every NWC request subscribed to the wallet's kind-23195 responses with no since, replaying the wallet's entire response history from the relay per request - and the balance tick sends one request per minute. Responses are always newer than their request; a one-minute since absorbs clock skew and kills the replay. - The trade-detail countdown re-copied and re-sorted the order's message history on every 1-second tick to find the state message. The result is now memoized per history-list instance (the storage index emits a new list only on real changes). Closes the app-side scope of plan items 5.5 and 5.2; bounding the dart_nostr fork's internal registries remains a fork-repo follow-up, and the legacy SharedPreferences.getInstance sites stay untouched because migrating them to SharedPreferencesAsync moves the backing store and would drop existing read cursors.
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: 31067098d1
ℹ️ 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".
|
Warning Review limit reachedNext included review available in 58 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 (5)
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 |
Catrya
left a comment
Catrya
left a comment
There was a problem hiding this comment.
Request changes — one line needs a bigger number. Everything else is comment-level.
On CI: the red check is not from this PR. Full suite on this branch is 1282 pass / 1 fail, and the single failure is dispute_chat_duplicate_envelope_test.dart, which fails identically on pristine main; #708 already fixes it. flutter analyze is clean.
I confirmed the countdown hot path is real: trade_detail_screen.dart:1015 watches countdownTimeProvider purely as a 1 s rebuild ticker, and mostroMessageHistoryProvider hands back the same list instance between ticks, so the memo lands exactly where intended.
Blocking: one minute is too tight a clock-skew budget
Codex flagged this as P1 and it is right. Some detail worth adding.
since is computed from the phone's clock, but the relay filters against the created_at signed by the wallet service — two independent machines. If the wallet's clock runs more than 60 s behind the phone's, or (more common in consumer hands) if the phone's clock runs ahead because automatic time is off, the response arrives with a created_at older than since, the relay drops it, and completer.future.timeout(requestTimeout) (nwc_client.dart:488, 30 s) fires.
The failure mode is silent and total: nothing degrades, everything stops — balance, pay_invoice, all of it — and from the outside it looks like the wallet never answered, when it did.
There is also a precedent in this repo that argues against the number: ChatCursorStore.cursorOverlap is 10 minutes, chosen for exactly this. The 60 s in NostrEventExtensions.chatMaxClockSkewSecs is a validation tolerance for rejecting envelopes, not a relay-side filter that discards silently. So this PR applies the tightest skew budget in the codebase to the one place where the counterparty is a third-party server rather than Mostro.
One-line fix: raise it to 10 minutes, matching cursorOverlap. It kills the replay just as effectively — the point is not to re-receive the wallet's entire history, and ten minutes of it is nothing — while leaving a realistic clock margin.
The memo P2 is valid but largely theoretical
Codex notes that _findMessageForStateUncached is not pure — it calls DateTime.now() in the future-timestamp guard — so memoizing freezes a result whose validity depends on time.
Correct in principle, but worth knowing that the guard is close to dead code: MostroStorage.addMessage back-fills message.timestamp ??= DateTime.now().millisecondsSinceEpoch at write time, so a stored message cannot carry a timestamp more than an hour in the future. The only route would be background_notification_service.dart:383, which uses the Nostr event's createdAt, and that would need Mostro's clock to be over an hour fast. It is also self-limiting: a new message produces a new list instance and the memo recomputes.
I would leave this as a comment rather than a change. If you do want it closed, the clean move is to drop the DateTime.now() guard from the memoized function — it buys nothing today — rather than adding time-based invalidation to the memo.
No tests
The PR changes two behaviors and adds no test — the suite count is identical to main. That departs from the standard of this series: #708, #711, #712 and #715 all ship RED-first tests.
The NWC filter's since in particular deserves a pin: it is one line that nothing would catch if someone later edits or drops it, and it is the change with the most expensive failure mode.
Small accuracy note on the PR body
It says the memo works because "the storage index (#715) emits a new list instance only on real changes". The dependency is weaker than that: on main, Riverpod already caches the AsyncValue, so the 1 s ticks reuse the same list instance and the memo hits regardless. What #715 adds is fewer spurious invalidations — today BaseStorage.watch emits a fresh list on any write to the orders store, including writes for a different order. The optimization does not depend on #715 to work.
What is right
Both hot paths are real and correctly identified. Expando is the right tool here: its keys are weak, so entries are collected along with the list and nothing leaks — a static Map would have leaked. The memo caches negative results properly via containsKey rather than ??, which is the usual mistake in this pattern. And the e-tag verification in the handler is untouched, so since relaxes no security check — it only trims the replay.
Review feedback on #718. Blocking (P1): the kind-23195 `since` was computed from the phone's clock while the relay filters on the `created_at` signed by the wallet service. One minute of skew is the tightest budget in the codebase, applied to the one place where the counterparty is a third-party server. A wallet running more than 60 s behind (or a phone running ahead) had its live response dropped by the relay, timing out every NWC operation, payments included — silently, and indistinguishably from the wallet never answering. The window is now a named `NwcClient.responseReplayWindow` of 10 minutes, matching `ChatCursorStore.cursorOverlap`, the skew budget this codebase already uses for relay-side `since` filters. It bounds the replay just as effectively. The filter moves into a testable `responseFilter()`; the e-tag verification in the handler is untouched. P2: the countdown memo froze a result that depends on `DateTime.now()` — a message timestamped over an hour ahead was skipped and the skip cached for the lifetime of the list instance. The lookup now reports whether the future-timestamp guard discarded a candidate and leaves those results uncached, so a later tick re-evaluates once the clock catches up. The guard is kept rather than dropped, so no bogus timestamp can drive a countdown. The lookup moves out of the private widget into `StateMessageFinder` so both behaviors are testable; #718 shipped no tests, which departed from the rest of the series. Tests: 11 new — the `since` bound and its skew margin pinned against `cursorOverlap`, and the memo's hit/recompute/no-freeze behavior.
|
Thanks @Catrya — all three points addressed in 5e029a3. Blocking (P1) — one minute is too tight. Agreed, and the P2 memo. Closed, but not by dropping the No tests. Fair, and it did depart from #708/#711/#712/#715. Both behaviors are now testable and pinned — 11 tests:
PR body accuracy. You're right and I've stopped claiming it — the memo works on
|
2 participants
Summary
Final Phase-5 PR, closing the app-side scope of plan items 5.5 and 5.2.
since, replaying the wallet's entire response history from the relay per request — and the balance tick (post-perf: timer hygiene for settings poll, countdown and NWC health #690) sends one request per minute. Responses are always newer than their request in real time, butsinceis the phone's clock while the relay filters on the wallet service's signedcreated_at— so the window isNwcClient.responseReplayWindow= 10 minutes, matchingChatCursorStore.cursorOverlap, the skew budget already used for relay-sidesincefilters here. It kills the replay while leaving a realistic clock margin. The e-tag verification in the handler is untouched.Expando: Riverpod hands back the same list instance between ticks, so ticks hit the memo and a new instance recomputes. (perf: serve mostro message queries from a single in-memory index #715 is not a prerequisite — it only reduces spurious invalidations from unrelated writes to theordersstore.) Results made time-dependent by the future-timestamp guard are deliberately left uncached, so nothing freezes.Deliberately out of scope (documented)
dart_nostrfork's internal registries (allDataEntitiesRegister/eventsRegistry) — fork-repo follow-up, as with the reconnect backoff.SharedPreferences.getInstance()sites toSharedPreferencesAsync— the two APIs use different backing stores on Android; a blind swap would drop existing chat/dispute read cursors. Needs a small migration, not a cleanup.Test plan
flutter analyze— no new issues🤖 Generated with Claude Code
https://claude.ai/code/session_018fTxqxhpdL5siTgKZqwtur