fix: correlate create_order replies by request_id and stop stale relay replay#172
Merged
Conversation
- send a random request_id nonce in the outgoing new-order message; the daemon echoes it in both the NewOrder confirmation and any CantDo rejection (mostrod publish_order / enqueue_cant_do_msg) - the pending create_order waiter now records its nonce and is only resolved by a reply that echoes it: stale relay-replayed events (no or foreign request_id) leave the waiter in place for the genuine reply instead of falsely rejecting or falsely confirming a live create - late-reconciliation and no-waiter paths are unchanged
- add a since(now - 60s) cutoff to the per-trade kind-14 reply filter: freshly derived keys have no stored history, so this guards the key reuse cases — a mnemonic re-imported without last-trade-index resync re-derives keys whose full reply history sits on the relays, and any future counter regression does the same; replayed replies are what used to falsely resolve waiting create_order calls - since is a lower bound on created_at, not a delivery deadline: live and late-arriving replies pass; offline catch-up stays on the global feed, which deliberately has no cutoff (documented on both filters) - defense in depth with the request_id correlation from the previous commit, for relays that ignore since
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds per-attempt ChangesOrder confirmation correlation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant App as create_order
participant Pending as PendingCreate
participant Relay
participant Daemon
App->>Pending: register trade key and request_id
App->>Relay: subscribe live kind-14 replies
App->>Daemon: publish NewOrder(request_id)
Relay->>App: NewOrder or CantDo(request_id)
App->>Pending: take_matching_create(trade_key, request_id)
alt matching request_id
Pending-->>App: resolve waiter or retain detached record
else stale request_id
Pending-->>App: preserve pending record
end
Possibly related issues
Possibly related PRs
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}
Contributor
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 90-92: The fixed 60-second lookback in reply_subscription_since
can miss valid replies when the client clock is ahead of the daemon. Update the
since calculation to use daemon-aligned time or widen the window, and make sure
create_order consumes that adjusted timestamp so the reply subscription does not
time out on skewed hosts.
🪄 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
Run ID: b29023a5-2ac6-4f9e-8e06-422bded05b5f
📒 Files selected for processing (2)
rust/src/api/orders.rsrust/src/mostro/actions.rs
…skew - 60s could silently drop the daemon's live reply on devices whose clock runs ahead of the daemon's: since is a delivery-level filter (relays match live events against it too) and the reply's created_at is stamped by the daemon's clock, so every create would time out on a skewed device with no diagnostic trail - 600s comfortably covers realistic mobile clock skew; widening is nearly free because request_id correlation is what protects the waiter — the cutoff only trims stored-history replay on reused keys
![ermeme[bot]](https://avatars.githubusercontent.com/in/3690068?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
Verdict: Request Changes
The request-id correlation is good for the live waiter, but the no-waiter fallback in Action::NewOrder still accepts any replayed confirmation and can rebind the local order to the wrong daemon UUID after the 10s timeout. That leaves a stale replay path that bypasses the new guard, so the fix is incomplete.
- limit(0) makes the per-trade subscription live-only: no stored history, and unlike since it never drops the genuine reply under client clock skew - removes reply_subscription_since and its test; request_id correlation remains the primary protection (also resolves the CodeRabbit note) - same pattern as mostro-cli and MostriX waiter subscriptions; offline catch-up stays on the global feed, which replays history with no cutoff
- collapse PENDING_CONFIRMATIONS, PENDING_MAKER_KEYS and PENDING_LOCAL_IDS into one PendingCreate record per attempt, keyed by its fresh trade key - NewOrder/CantDo side effects (trade-key binding, waiter, local→daemon id bridge) now require the echoed request_id, closing the paths where a stale replay could consume state belonging to a live or concurrent create - the 10s timeout detaches only the waiter channel: a genuine late reply still reconciles, and the record dies with the per-trade subscription - the Kind 38383 path matches by content fingerprint and skips records with a live waiter; the arbitrary any-entry fallback is gone - absorb resolve_maker_order (unused from Dart) and regenerate the bridge; widen gift-wrap dedup to 512 entries; add correlation tests
- gate the pre-dispatch local→daemon id reconciliation on ownership: the stored id must be the local UUID of this trade key's own pending create - without the gate, a stale replay carrying an old order id for a reused trade index could rebind a confirmed order — daemon→daemon — corrupting the order book, trade row and trade-key mapping - cold start is unaffected: the pending map is empty after a restart and the Kind 38383 fingerprint path owns maker recovery there - extract the decision into may_reconcile_stored_id with a unit test
There was a problem hiding this comment.
Reviewed the current head. The request_id correlation is wired through create_order, stale relay replays no longer consume the pending create, and the per-trade subscription avoids replaying stored history. I also ran targeted Rust tests for the new correlation helpers and they passed. No blocking issues found.
There was a problem hiding this comment.
I re-reviewed the current head strictly, with the failure mode in mind where stale relay-replayed events could consume pending create-order state.
What I checked:
- The outgoing
new_orderpayload now carriesrequest_idandtrade_indexon the wire; the correlation nonce is not reconstructed later. take_matching_create(...)only consumes the pending record when the echoedrequest_idmatches exactly. Missing or mismatched ids leave the record in place.detach_create_waiter(...)only clears the waiter channel on timeout. It does not delete the record, so a late genuine reply can still reconcile.take_pending_create_by_content_key(...)ignores live waiters (tx.is_some()), which prevents the content-key path from stealing an in-flight create.may_reconcile_stored_id(...)only allows rebinding when the stored id is the pending local UUID. That prevents stale replayed identifiers from rewriting a confirmed order.
The targeted tests cover the important edges:
- late genuine reply after timeout
- stale / mismatched
request_idignored - concurrent creates do not cross-consume
- content-key lookup skips live waiters
I do not see a blocking correctness issue in the current head. The key invariant is consistent: only a reply that echoes the exact request_id may resolve or consume the pending create.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #168
Problem
create_orderwaits for the daemon's reply, but resolved its waiter bytrade pubkey alone. When the per-trade kind-14 subscription opens,
relays replay the key's stored history, and any old reply addressed to
that key could consume the waiter before the genuine reply arrived:
CantDo(about an unrelated old order) rejected a create thedaemon actually accepted — the order existed on the daemon while the
app reported an error, so the user retried and created duplicates
NewOrderconfirmation (replayed from a previous session)confirmed a create the daemon actually rejected — the app navigated
to My Order showing an old, unrelated order as "just created", while
the genuine
CantDowas discarded as staleKey reuse (fixed in #171 ) was the fuel; this PR fixes the correlation
itself, which still matters for re-imported mnemonics (no
last-trade-index resync yet) and any future counter regression.
Fix — two independent layers
fix(orders): correlate create_order confirmations by request_idnew-ordernow carries a randomrequest_idnonce;mostrod echoes it in both the
NewOrderconfirmation(
publish_order) and everyCantDorejection (enqueue_cant_do_msg)that echoes it (
take_matching_confirmation); stale events — no orforeign
request_id— leave the waiter in place for the genuinereply; late-reconciliation / no-waiter paths are unchanged
validate_responsestill getsNonebecause it short-circuitson
CantDobefore comparing request_ids; correlation lives at thewaiter arms (documented in code). This closes the gap flagged by the
existing "does not yet track outstanding request_ids" comment.
fix(orders): stop replaying stored history into per-trade subscriptionssince(now - 60s); freshlyderived keys have no history, so this guards the key-reuse cases
(mnemonic re-import, counter regression) where the key's full reply
history sits on the relays
sinceis a lower bound oncreated_at, not a delivery deadline:live and late-arriving replies pass, and reconnects replay anything
newer than the cutoff
(
subscribe_node_filters), which deliberately has no cutoff andkeeps replaying full history — documented on both filters
Testing
cargo test(98 passed): wrap/unwrap roundtrip proving the noncetravels in the message;
request_id_matchescases; waiter leftintact by stale/foreign ids and consumed exactly once by the echoed
nonce; 60s-window helper.
cargo clippyclean on touched files.CantDo(OutOfRangeSatsAmount) resolves the waiter → realerror shown
NoDaemonResponseafter 10s, nothing persistedNewOrderresolves the waiter → correct order confirmedharmlessly ("no waiting caller"); the per-trade feed receives only
the fresh reply
No FRB regeneration needed: no
rust/src/api/signatures changed.Summary by CodeRabbit