feat(bond): concurrent taker bonds, first-to-lock wins (Phase 0+1)#733
Conversation
Replace the supersede-on-retake model in Phase 1's take handlers with concurrent taker bonds: multiple `Requested` taker bonds may coexist on an order, and the first to reach `Locked` wins. Losers' hold invoices are cancelled at lock-time (not at retake-time) and their takers receive `Action::Canceled` from Mostro instead of an LND `FAILURE_REASON_INCORRECT_PAYMENT_DETAILS` when they try to pay a silently-superseded invoice. Companion to the spec PR (concurrent-bonds revision of docs/ANTI_ABUSE_BOND.md, PR #732). Phase 0 additive changes: - New migration `20260511180000_bond_taker_context.sql` adds nullable `taker_*` columns to the `bonds` table (taker_identity, taker_trade_index, taker_invoice, taker_fiat_amount, taker_amount, taker_fee, taker_dev_fee). The take handler stashes deferred take-flow context here while the bond races to `Locked`, instead of mutating the order's taker fields and clobbering concurrent racers. - `Bond` struct in `bond/model.rs` gains the matching 7 Option fields. - New DB helper `find_active_bond_by_taker(order_id, pubkey)` in `bond/db.rs`, scoped by pubkey so the take handlers' idempotent retry check and `cancel_order_by_taker` find exactly the sender's bond without disturbing concurrent rows. Phase 1 behaviour changes: - `bond::supersede_prior_taker_bonds` is removed. - `take_buy_action` / `take_sell_action` no longer release prior Requested bonds at retake-time. The pre-persist check becomes three guards: reject if any bond on the order is already Locked (PendingOrderExists); idempotent retry when the sender already has a Requested bond (re-emit the same bolt11); otherwise create a fresh bond row. Taker fields stay off the orders row during the bond window — they live on the bond's `taker_*` columns instead. - `request_taker_bond` accepts a new `TakerContext` struct carrying the per-take identity / trade index / invoice / pricing snapshot, and writes them to the bond row. - `on_bond_invoice_accepted` becomes the cancel-the-losers chokepoint. The conditional `Requested → Locked` UPDATE gains a `NOT EXISTS (… another bond Locked on this order …)` guard so exactly one bond wins per order; a losing race releases its own hold invoice via `release_bond` and DMs `Action::Canceled` to the taker without settling. On a win, the handler iterates every other still-Requested bond on the order, releases each, DMs each taker `Action::Canceled`, then promotes the winning bond's `taker_*` snapshot onto the order before `resume_take_after_bond`. - `cancel_order_by_taker` (in `cancel.rs`) is scoped to release only the sender's own bond via `find_active_bond_by_taker`. If other concurrent takers' bonds remain, the order stays in Pending and is not republished; only the sender's take is cancelled. When the sender is the last bonded taker, the full reset-and-republish flow runs as before. Tests: 259 passing. New unit tests cover the lock-race NOT EXISTS guard, concurrent Requested bonds coexisting on the same order, the `find_active_bond_by_taker` pubkey-scoped lookup, and round-trip of the `taker_*` columns through the DB. `cargo fmt`, `cargo clippy --all-targets --all-features -D warnings`, and `cargo test` all green. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThis PR implements concurrent taker bonds with atomic lock-race enforcement on the Mostro P2P platform. It adds taker-context columns to the bond schema, introduces a ChangesConcurrent Taker Bonds Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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.
🧹 Nitpick comments (2)
src/app/bond/flow.rs (1)
286-286: 💤 Low valueMinor: empty markdown link
[issue raised in the Phase 1 review](#).The placeholder
(#)URL leaves a non-functional link in the doc comment. Either point it at the actual issue/PR or drop the link formatting and keep the prose.🤖 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 `@src/app/bond/flow.rs` at line 286, The doc comment in src/app/bond/flow.rs contains a placeholder empty markdown link "(#)" in the text "the [issue raised in the Phase 1 review](#)"; update that doc comment by either replacing "(#)" with the real issue/PR URL or removing the link formatting and leaving plain text ("the issue raised in the Phase 1 review") so the documentation no longer contains a non-functional link.src/app/bond/db.rs (1)
135-140: 💤 Low valueConsider
ORDER BY created_at ASCfor deterministic selection.The current take-flow invariants ensure at most one active bond per
(order_id, taker_pubkey), soLIMIT 1is safe. However, sibling helpers (find_active_bonds,find_active_bonds_for_order) all sort bycreated_at ASC; adding the same ordering here is a one-line change that makes the selection deterministic if a future code path ever inserts a second active row for the same taker before the first terminates.♻️ Proposed change
sqlx::query_as::<_, Bond>( "SELECT * FROM bonds \ WHERE order_id = ? AND pubkey = ? AND state IN (?, ?) \ AND parent_bond_id IS NULL \ + ORDER BY created_at ASC \ LIMIT 1", )🤖 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 `@src/app/bond/db.rs` around lines 135 - 140, The SQL query passed to sqlx::query_as::<_, Bond> lacks an explicit ordering, so add "ORDER BY created_at ASC" to the query string used in that sqlx::query_as::<_, Bond>(...) call (place it before the existing "LIMIT 1") to make selection deterministic consistent with find_active_bonds/find_active_bonds_for_order.
🤖 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.
Nitpick comments:
In `@src/app/bond/db.rs`:
- Around line 135-140: The SQL query passed to sqlx::query_as::<_, Bond> lacks
an explicit ordering, so add "ORDER BY created_at ASC" to the query string used
in that sqlx::query_as::<_, Bond>(...) call (place it before the existing "LIMIT
1") to make selection deterministic consistent with
find_active_bonds/find_active_bonds_for_order.
In `@src/app/bond/flow.rs`:
- Line 286: The doc comment in src/app/bond/flow.rs contains a placeholder empty
markdown link "(#)" in the text "the [issue raised in the Phase 1 review](#)";
update that doc comment by either replacing "(#)" with the real issue/PR URL or
removing the link formatting and leaving plain text ("the issue raised in the
Phase 1 review") so the documentation no longer contains a non-functional link.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b4517645-846a-4969-9ccd-49b186ea0ee1
📒 Files selected for processing (8)
migrations/20260511180000_bond_taker_context.sqlsrc/app/bond/db.rssrc/app/bond/flow.rssrc/app/bond/mod.rssrc/app/bond/model.rssrc/app/cancel.rssrc/app/take_buy.rssrc/app/take_sell.rs
![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: a5f15b368e
ℹ️ 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 Err(e) = bond::release_bond(pool, bond).await { | ||
| warn!( | ||
| bond_id = %bond.id, | ||
| "taker_cancel: failed to release sender's bond: {}", e | ||
| ); |
There was a problem hiding this comment.
If release_bond fails here (for example, transient LND connectivity), the function only logs and continues into the reset/republish path. That leaves the sender’s bond active while the order is back in Pending; if that stale bond later reaches Accepted, on_bond_invoice_accepted now promotes its stored taker_* context and can resume the trade flow, effectively resurrecting a take the user already canceled. The taker-cancel path should abort (or at least skip republish) when sender-bond release is not confirmed.
Useful? React with 👍 / 👎.
The taker_* columns were originally added by a follow-up ALTER TABLE migration. Since the bond feature is not in production yet (no node has flipped enabled = true), there is no value in preserving the historical schema — fold the columns directly into the Phase 0 CREATE TABLE for a clean single-migration definition. - Migration `20260511180000_bond_taker_context.sql` removed. - Phase 0 migration `20260423120000_anti_abuse_bond.sql` now declares `taker_identity`, `taker_trade_index`, `taker_invoice`, `taker_fiat_amount`, `taker_amount`, `taker_fee`, `taker_dev_fee` directly in the `bonds` CREATE TABLE. - Test pool setups in `bond/db.rs` and `bond/flow.rs` no longer apply a second migration step. No schema change vs. the previous commit; just a cleaner shape on fresh DBs. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
…lder link - `find_active_bond_by_taker` in `src/app/bond/db.rs` now appends `ORDER BY created_at ASC` before the `LIMIT 1`, making the row selection deterministic and consistent with `find_active_bonds` / `find_active_bonds_for_order`. Under the concurrent-bonds model the `(order_id, pubkey)` pair is effectively unique (one taker has at most one active bond per order, enforced by the idempotent-retry check in the take handlers), but the order clause guarantees stable behaviour if that invariant is ever violated. - `release_bond` doc comment in `src/app/bond/flow.rs` drops the placeholder markdown link `[…](#)` and leaves plain text so the rendered docs no longer contain a dead anchor. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
1 participant
Summary
Implements the concurrent taker bonds model in Phase 0 + Phase 1 code, replacing the supersede-on-retake behaviour that was on
mainuntil spec PR #732 landed.Spec PR #732 (concurrent-bonds rewrite of
docs/ANTI_ABUSE_BOND.md) is already merged. This code PR brings the implementation in line with the new spec. PR #734 is a small spec follow-up that folds thetaker_*columns into the Phase 0 schema description (matching the same reorganisation done here on disk).Motivation
Under the supersede model, a slightly slower taker (e.g. someone who took the order but is still opening their Lightning wallet) gets their bond invoice cancelled the moment another taker presses "take". When they finally hit "pay", LND returns
FAILURE_REASON_INCORRECT_PAYMENT_DETAILS— a cryptic Lightning-level error with no Mostro-level explanation. This was observed in testing and is the kind of UX failure that costs trust without offering any anti-abuse benefit.Under concurrent bonds, every taker keeps a payable invoice and the race resolves at actual payment time. The first taker to lock their bond wins; the others receive a clean
Action::Canceledfrom Mostro. A malicious taker who never pays still cannot block the order book — their hold invoice expires on its own LND-side TTL.Phase 0 (schema)
migrations/20260423120000_anti_abuse_bond.sqlnow declares 7 new nullable columns directly in thebondsCREATE TABLE:taker_identity,taker_trade_index,taker_invoice,taker_fiat_amount,taker_amount,taker_fee,taker_dev_fee. No separate ALTER TABLE migration — the bond feature isn't in production yet, so the schema lands clean from Phase 0.Bondstruct (src/app/bond/model.rs) gains the matching 7Optionfields.new_requestedkeeps themNoneso non-bond callers behave identically.find_active_bond_by_taker(order_id, pubkey)insrc/app/bond/db.rs, scoped by pubkey for the idempotent-retry check and the scoped taker self-cancel.Phase 1 (behavioural)
bond::supersede_prior_taker_bonds(function + tests +mod.rsexport).take_buy_action/take_sell_actionpre-persist check shrinks to:PendingOrderExistsif any bond on the order is alreadyLocked.Requestedbond on this order, re-send the same bolt11.Requestedrows.Taker fields (
buyer_pubkey/seller_pubkey, identities, trade index, buyer invoice, per-take pricing) stop being persisted on theordersrow during the bond window — they go on the bond'staker_*columns.request_taker_bondaccepts a newTakerContextstruct carrying the per-take snapshot and writes them to the bond row. Bond amount is derived fromtaker_ctx.amount(notorder.amount) so concurrent racers on a market-priced range order each post a bond sized to their own quote.on_bond_invoice_acceptedbecomes the cancel-the-losers chokepoint:Requested → LockedUPDATE gainsNOT EXISTS (SELECT 1 FROM bonds … state='locked' AND id != ?)so exactly one bond wins per order.release_bond+Action::CanceledDM to its taker; hold invoice cancelled before settle.Requestedbonds on the order, release each + DMAction::Canceled, then promote the winning bond'staker_*snapshot onto the order beforeresume_take_after_bond.cancel_order_by_takerscoped to release only the sender's own bond viafind_active_bond_by_taker. If other concurrent takers' bonds remain, the order stays inPendingand is not republished (cancel is effectively a per-taker release + DM); if the sender was the last bonded taker, the full reset-and-republish flow runs as before.Files
migrations/20260423120000_anti_abuse_bond.sql(taker_* columns added to existing CREATE TABLE)src/app/bond/{model,db,flow,mod}.rssrc/app/{take_buy,take_sell,cancel}.rsCommits
feat(bond): switch Phase 1 to concurrent taker bonds, first-to-lock winsfeat(bond): inline taker_* columns into Phase 0 CREATE TABLE— schema reorganisation per maintainer feedback (no behavioural change)Ready to squash on merge if preferred.
Tests
concurrent_requested_bonds_coexist,lock_race_guard_admits_only_one_winner,find_active_bond_by_taker_scopes_to_pubkey,taker_context_columns_roundtrip.cargo fmt,cargo clippy --all-targets --all-features -D warnings, andcargo testall green.Test plan
NOT EXISTSlock-race guardcancel.rsdoesn't regress single-taker pathsAction::Canceled(not an LND error)🤖 Generated with Claude Code