fix(orders): rehydrate kind-14 decryption coverage at startup - #292
Conversation
- Startup built the trade-key map but never seeded global_dm_keys, so after any restart every kind-14 for a previous session's trade was dropped undecrypted (no-matching-p-tag map=0). - The first create/take then rebuilt the relay filter from the session-only map, silently unsubscribing all older trades. - New seed_global_dm_coverage() derives and merges all known keys into the coverage map; startup and node switch now share it, so the two entry points cannot diverge again. - Merge semantics are union, never replace: a concurrently derived session key survives the seed; regression test pins it.
- New contract subsection: delivery (relay #p filter) and decryption (global_dm_keys) are independent layers and both must cover a trade.
- Relays deliver the startup backlog newest-first; applying it blindly walked finished trades backwards (Canceled resurfacing as WaitingPayment on every start) and re-emitted action requests. - New hard-terminal guard on the four status-syncing dispatch arms: once canceled/expired/settled/completed, no kind-14 moves the trade — no book/DB write, no TradeUpdate emission, so the auto-navigation listener cannot fire for dead trades. - SettledHoldInvoice and Dispute stay open on purpose: they still progress to Success / admin resolutions. - Local status resolves DB-first (authoritative across restarts), book as fallback; tests cover the set and the guard. - Contract documents the replay-skip semantics.
a session with blocked replays read as if the guard never fired. - Now goes through blog_debug with the orders tag and short_id, matching the other status lines.
|
Warning Review limit reached
Next review available in: 27 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 (2)
WalkthroughThe change prevents Kind-14 replay messages from reopening hard-terminal trades. It also makes global decryption coverage merge-preserving across startup, node refresh, subscriptions, and newly derived trade keys. Tests and contract documentation cover both behaviors. ChangesKind-14 synchronization
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
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 |
![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
🤖 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/orders.rs`:
- Around line 2412-2429: Update the Action::Canceled handler in
rust/src/api/orders.rs#L2412-L2429 to call status_sync_blocked_by_terminal
before any cancellation DB, session, book, UI, or TradeUpdate side effects,
returning when the replay is blocked; add a handler-level regression test in
rust/src/api/orders.rs#L4509-L4537 for a replayed cancellation against a locally
terminal non-canceled trade that asserts no write and no emission; retain the
“skipped entirely” contract in
specs/004-mostro-p2p-client/contracts/orders.md#L301-L307 after this guard is
applied.
🪄 Autofix
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: c51c9b72-5834-4505-ac52-54c2463e027c
📒 Files selected for processing (2)
rust/src/api/orders.rsspecs/004-mostro-p2p-client/contracts/orders.md
- A stale timeout-cancel replayed over an order that was later re-taken and completed could overwrite Success with Canceled and emit. - The Canceled arm now applies the terminal guard before any side effect; the never-active wipe path is unaffected (starts from non-terminal waiting states). - Handler-level regression test: dispatching a replayed Canceled against a Success trade writes nothing and emits nothing. - Contract notes Canceled follows the same replay-skip semantics.
![ermeme[bot]](https://avatars.githubusercontent.com/in/3690068?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review summary
Thanks for the focused fix. The startup/node-switch seeding path and the Action::Canceled terminal replay guard look good, and the previous CodeRabbit cancellation finding is addressed on the current head.
I found one remaining blocker: BuyerTookOrder / HoldInvoicePaymentAccepted still performs the peer-key side effect before the hard-terminal replay guard. A stale replay over a completed/canceled trade can therefore update the session peer/shared key and spawn the incoming chat subscription before the status update is skipped. That violates the new "skipped entirely" replay contract and can resurrect chat coverage for a finished trade.
Validated locally:
cargo test(253 passed, 8 ignored)
GitHub checks on 2cb6f6f2869447adce55f57f382359498d1a3ca8 are green: Flutter analyze/test, Rust build/test/clippy/wasm, and Web wasm smoke test.
- BuyerTookOrder/HoldInvoicePaymentAccepted checked the guard after on_peer_pubkey_received, so a stale replay over a finished trade still re-derived the peer key, recreated session state and respawned the chat subscription on every start. - The guard now runs right after the order id is known, before any side effect; the legit re-take of a timeout-canceled order still passes (its wiped row resolves to the book's pending). - Handler-level test: a replayed BuyerTookOrder over a Success trade creates no session, keeps the book terminal and emits nothing.
There was a problem hiding this comment.
Review summary
Approved. The previous blocker is fixed on the current head: the terminal replay guard now runs before on_peer_pubkey_received(), so stale BuyerTookOrder / HoldInvoicePaymentAccepted messages over hard-terminal trades skip peer/session/chat side effects as well as DB/book/TradeUpdate writes.
I also re-checked the startup/node-switch seeding path and the Action::Canceled replay guard. The added handler-level regression tests cover both the canceled replay and the peer-key side-effect replay case.
Validated locally:
cargo test(254 passed, 8 ignored)
GitHub checks on 8211396b71f9bec32827fad8ee5f1d063113aec2 are green: Flutter analyze/test, Rust build/test/clippy/wasm, and Web wasm smoke test.
1 participant
Closes #277 — cause 1 was already fixed by the
wire_status_appliesguard(verified in the logs attached to the investigation), cause 3 is fixed here,
and cause 2 (dead-subscription watchdog) moves to its own issue: #291 .
Since
66dad8c(2026-07-30, #253 review round), the kind-14 decryption map(
global_dm_keys) was only seeded on node switch — never at startup. Theevent loop decrypts against that map, so after any app restart every daemon
message for a trade from a previous session was dropped undecrypted
(
no-matching-p-tag map=0), even though the relay filter included its p-tagand delivered it. On top of that, the first create/take of the session rebuilt
the relay filter from the session-only map, silently unsubscribing every
older trade (
sub created p_count=24at startup →sub replaced p_count=1).Consequences, reproduced deterministically across five consecutive sessions:
trades from previous sessions went deaf; statuses froze at whatever the public
Kind 38383 shows (masked
in-progress, displayed as "Active"); add-invoicerequests never reached the maker, so the daemon canceled orders by timeout;
and every flow waiting for a daemon reply on a deaf trade died with
NoDaemonResponse. The regression also broke #253's own goal across restarts(a solver assignment after a restart was undecryptable).
Fix
seed_global_dm_coverage(): derives every known trade key and merges itinto the coverage map, returning the full pubkey set for the relay filter.
Startup and node switch now share it, so the two entry points cannot diverge
again. Merge semantics are union, never replace — a concurrently derived
session key survives the seed (regression test included).
issue — relays deliver the startup backlog newest-first, and applying it
blindly walked finished trades backwards (a canceled trade resurfacing as
WaitingPaymenton every start) and re-emitted action requests to the UI(a hazard for feat(navigation): auto-open invoice screens on daemon request #289's auto-navigation). The four status-syncing dispatch arms
now skip any sync that would move a trade out of
Canceled/CanceledByAdmin/CooperativelyCanceled/Expired/Success/ SettledByAdmin/CompletedByAdmin— no book/DB write, noTradeUpdateemission.
SettledHoldInvoiceandDisputedeliberately stay open (theystill progress). Skips are observable via
blog_debug.Verification (before → after, same identity, real daemon)
no-matching-p-tag: ~30 per session → 0-1.src=kind14status syncs after restart: 0 in five sessions → 34-55.p_count=1(collapse) →p_count=N+1.Canceled↔WaitingPayment→ stableCanceled.939cc777, fix(orders): order status gets out of sync (stale InProgress, dead subscription, missing kind-14 catch-up) #277-style) finally resolved toCanceledonce its backlog decrypted.Changes
rust/src/api/orders.rs: seeding helper + both entry points wired to it;is_hard_terminal/current_local_status/status_sync_blocked_by_terminalguards on the four dispatch arms; five new unit tests.
specs/004-mostro-p2p-client/contracts/orders.md: new "Kind-14 delivery &decryption coverage" section (the invariants this regression violated) and
the replay-skip semantics.
Out of scope (follow-up)
Non-terminal statuses can still be regressed by the newest-first replay
(e.g. an
Activetrade re-persisted asWaitingPayment); the complete fix isordering by event
created_at. Follow-up issue to be filedSummary by CodeRabbit
Bug Fixes
Documentation