feat(bond): anti-abuse bond phase 1 — taker lifecycle (lock + always release)#719
Merged
feat(bond): anti-abuse bond phase 1 — taker lifecycle (lock + always release)#719
Conversation
Wires the opt-in taker bond into the take flow with a strict "always release" guarantee. When [anti_abuse_bond] is enabled and apply_to matches the taker side, take_buy_action / take_sell_action defer the trade hold invoice and request a Lightning hold-invoice bond from the taker first; once the bond locks, a subscriber-driven continuation resumes the original take path. Every order exit (release_action, cancel_action's cooperative + unilateral branches, admin_settle_action, admin_cancel_action, scheduler::job_cancel_orders) cancels the bond hold invoice and marks the row Released. No slashing yet — that lands in Phase 2+. Phase 1 takes the alternative path documented in spec §6.2: orders stay in Status::Pending while the bond is outstanding, the bond bolt11 ships to the taker via the existing Action::PayInvoice (the bond hash makes it unambiguous), and bond state lives entirely in the bonds table. Dedicated AddBondInvoice / WaitingTakerBond variants will land with the matching mostro-core release in a later phase. Other Phase 1 details: - A guard at take time rejects a second take with PendingOrderExists when an active bond row already exists for the order, preventing duplicate bonds when two takers race. - main.rs calls bond::resubscribe_active_bonds on startup so a daemon restart never strands a taker who paid the bond just before shutdown. - New db helpers find_bond_by_hash, find_active_bonds and find_active_bonds_for_order back the subscriber, restart hook and exit-path release respectively. - Settings::get_bond / is_bond_enabled now return None / false when MOSTRO_CONFIG is not yet initialized, so the bond gate never panics in unit tests that don't bring up the full configuration. - Default-off: nodes that don't set [anti_abuse_bond].enabled = true see zero behavior change. Tests: cargo test (245 passed), cargo clippy --all-targets --all-features (clean).
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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. WalkthroughAdds Phase 1 taker anti-abuse bond: new bond flow module, DB helpers and queries, invoice subscriber/resubscribe logic, and wiring for request/supersede/release into take, cancel, admin, release, scheduler, and startup paths with non-fatal error handling and config gating. Changes
Sequence Diagram(s)sequenceDiagram
participant Taker
participant Server
participant LND
participant Database
rect rgba(100, 150, 200, 0.5)
note over Taker, Database: Taker Bond Request Phase
Taker->>Server: take_buy_action / take_sell_action (initiate take)
Server->>Database: Check/supersede prior active bonds
Server->>LND: Create hold invoice (bond)
Server->>Database: Insert Bond (Requested) with hash/payment_request
Server->>Taker: Enqueue Action::PayInvoice (bond)
Server->>LND: Subscribe to bond invoice events
Server->>Taker: Return (order stays Pending)
end
rect rgba(150, 200, 100, 0.5)
note over Taker, Database: Bond Accepted -> Resume Trade
Taker->>LND: Pay bond invoice
LND->>Server: Invoice Accepted event
Server->>Database: Transition Bond -> Locked
Server->>Server: Resume trade flow (create trade hold invoice / proceed)
end
rect rgba(200, 150, 100, 0.5)
note over Server, LND: Bond Release Path
Server->>Database: Find active bonds for order
Server->>LND: Cancel hold invoice (if hash present)
Server->>Database: Transition Bond -> Released
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
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. Review rate limit: 0/1 reviews remaining, refill in 52 minutes and 38 seconds.Comment |
![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: a6581f1c93
ℹ️ 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".
![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: 8
🧹 Nitpick comments (5)
src/app/cancel.rs (1)
149-157: Optional: consolidate the duplicated bond-release-with-warn boilerplate.The same
if let Err(e) = bond::release_bonds_for_order(...) { tracing::warn!(...) }shape now appears 4 times in this file and ~3 more times acrossadmin_cancel.rs,admin_settle.rs,release.rs, andscheduler.rs. A single helper would (a) prevent context-tag drift in future phases, (b) keep the call site one line, and (c) make Phase 2 slashing changes a one-touch edit.♻️ Sketch — add to
src/app/bond/flow.rs(ormod.rs) and call from each site/// Best-effort bond release; logs at warn level on failure and never propagates. pub async fn release_bonds_for_order_or_warn( pool: &sqlx::Pool<sqlx::Sqlite>, order_id: uuid::Uuid, context: &'static str, ) { if let Err(e) = release_bonds_for_order(pool, order_id).await { tracing::warn!("{}: bond release failed for {}: {}", context, order_id, e); } }Call sites become:
- if let Err(e) = bond::release_bonds_for_order(pool, order.id).await { - tracing::warn!( - "cooperative_cancel: bond release failed for {}: {}", - order.id, - e - ); - } + bond::release_bonds_for_order_or_warn(pool, order.id, "cooperative_cancel").await;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/cancel.rs` around lines 149 - 157, Consolidate repeated "best-effort bond release" boilerplate by adding a helper (e.g. release_bonds_for_order_or_warn) that calls release_bonds_for_order(pool, order_id).await and logs a tracing::warn! on Err without propagating; place this helper in the bond module (e.g. src/app/bond/flow.rs or mod.rs) and replace each repeated pattern (currently calling release_bonds_for_order(...) with an if let Err(e) { tracing::warn!(...) }) in cancel.rs, admin_cancel.rs, admin_settle.rs, release.rs, scheduler.rs with a one-line call to release_bonds_for_order_or_warn(pool, order.id, "cooperative_cancel" or appropriate context).src/app/bond/flow.rs (3)
222-228: Reuse oneLndConnectorfor the whole batch instead of one per bond.
release_bondopens a freshLndConnector::new()for each call, sorelease_bonds_for_orderwill pay the gRPC handshake cost N times even though all bonds in the batch can share one connection. For Phase 1 N is essentially 1, but it's cheap to fix and it removes a foot-gun before Phase 6 (range orders with potentially many child bonds).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/bond/flow.rs` around lines 222 - 228, The loop in release_bonds_for_order creates a new LndConnector per bond via release_bond; instead create one LndConnector once and reuse it for the batch: instantiate LndConnector::new() before iterating bonds in release_bonds_for_order, change or add an API like release_bond_with_connector(pool, bond, &connector) (or make release_bond accept a &LndConnector) and call that inside the loop, and ensure the connector is properly dropped/closed after the loop; update function signatures (release_bond or add release_bond_with_connector) and usages accordingly to avoid per-bond handshakes.
252-274: Spawned subscriber tasks have no shutdown / cancellation handle.
bond_invoice_subscribetokio::spawns two tasks per bond and never tracks theirJoinHandle. On daemon shutdown they're force-aborted, which is fine for Phase 1, but if a bond is canceled / released while its subscriber is mid-await, the subscriber will keep running and eventually log warnings about an unknown hash on the nextAccepted/Canceled. Not a Phase 1 blocker — flag for hardening alongside the Phase 3 scheduler work where you'll likely want a cancellation token per subscriber anyway.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/bond/flow.rs` around lines 252 - 274, The spawned subscriber tasks in bond_invoice_subscribe use tokio::spawn and are never tracked (no JoinHandle or cancellation), so they can outlive a bond and log unknown-hash warnings when handlers like on_bond_invoice_accepted/on_bond_invoice_canceled run; fix by creating and storing a per-subscriber cancellation handle (e.g., tokio_util::sync::CancellationToken or an AbortHandle) when spawning, pass a clone into the task and also select! on token.cancelled() inside the rx loop (or abort the JoinHandle) so tasks are explicitly cancelled when the bond is canceled/released, and keep the JoinHandle/cancellation handle in the bond/subscriber registry so you can cancel and await the task on release.
309-355: LGTM on the idempotency guards, but the state read-modify-write isn't atomic.Lines 322/326 keep the second
Accepteddelivery from re-running the resume, which is the important case. Just be aware that two near-simultaneous deliveries could both pass the guard and race on theupdate. With SQLite's single-writer model the secondupdatewill win and the resume will fire twice —show_hold_invoice/set_waiting_invoice_statuswill produce a duplicate Nostr event. Worth a follow-up using anUPDATE ... WHERE state = 'requested'+ check-affected-rows pattern to make the transition atomic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/bond/flow.rs` around lines 309 - 355, The current on_bond_invoice_accepted flow reads the bond, modifies state, then calls Bond::update which allows a race where two concurrent handlers both pass the in-memory guards and the second update overwrites the first causing resume_take_after_bond to run twice; to fix, change the update to an atomic conditional update (e.g. UPDATE bonds SET state='locked', locked_at=? WHERE id=? AND state='requested') and check the affected rows after the SQL exec in the code path that currently calls Bond::update (or replace Bond::update with a new method that performs this conditional update), and only call resume_take_after_bond when the conditional update reports 1 row affected (otherwise treat as idempotent/no-op).src/app/bond/db.rs (1)
86-110: Consider deriving the active-state list fromBondStaterather than hardcoding the SQL literals.
'requested', 'locked'here must matchBondState::Displayexactly, but nothing enforces the link. A future tweak to theDisplayimpl (or accidental case change) silently breaks the active-bond filter — and only at runtime.find_bonds_by_statealready bindsstate.to_string(); the active-state queries could either useBondState::Requested.to_string()/BondState::Locked.to_string()with two?placeholders, or expose a tinyBondState::is_activehelper used in code-side filtering for unit-testable coverage.Not blocking for Phase 1 — flag for hardening before slashing logic in Phase 2 starts depending on the same set.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/bond/db.rs` around lines 86 - 110, The SQL is hardcoding state literals ('requested','locked') in find_active_bonds_for_order (and the preceding active-bonds query); change this to derive values from the BondState enum instead: either rewrite the query to use two ? placeholders and bind BondState::Requested.to_string() and BondState::Locked.to_string() (mirroring find_bonds_by_state), or fetch by order_id and filter in Rust using a new BondState::is_active helper (implement is_active on BondState and use it to retain only active bonds). Update the query in find_active_bonds_for_order (and the other active-bonds query) to remove literal strings and reference the enum-derived strings or the post-query filter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/admin_cancel.rs`:
- Around line 203-211: The tracing::warn! invocation that logs bond release
failure should be collapsed into a single line to satisfy rustfmt; update the
macro call following the bond::release_bonds_for_order(pool, order.id).await
error path so it reads as one line (e.g. tracing::warn!("admin_cancel: bond
release failed for {}: {}", order.id, e);) keeping the same message and
variables.
In `@src/app/bond/flow.rs`:
- Around line 93-98: The call to Bond::new_requested currently spans multiple
lines causing cargo fmt failure; collapse it into a single-line call using the
same arguments (order.id, taker_pubkey.to_string(), BondRole::Taker, amount) so
the invocation of Bond::new_requested(...) fits on one line.
- Around line 161-164: The matches! usage should be single-line and you must
stop matching against BondState stringified Display values; parse bond.state
once into the BondState enum (e.g., via FromStr/TryFrom) and add a
BondState::is_terminal(&self) -> bool helper that returns true for
Released/Slashed/Failed, then replace the matches!(bond.state.as_str(), "...")
check in the current function and the similar check in on_bond_invoice_canceled
with a direct call like if bond_state.is_terminal() after parsing; this removes
coupling to BondState::Display and fixes the formatting issue if you keep any
matches! (otherwise remove matches! entirely).
- Around line 43-48: The multi-line import of DB functions causes cargo fmt
failure; collapse the import into a single-line use statement. Replace the
current block that references create_bond, find_active_bonds,
find_active_bonds_for_order, and find_bond_by_hash across multiple lines with a
single-line `use super::db::{create_bond, find_active_bonds,
find_active_bonds_for_order, find_bond_by_hash};` so rustfmt/cargo fmt passes
and the symbols (create_bond, find_active_bonds, find_active_bonds_for_order,
find_bond_by_hash) remain correctly imported.
- Around line 168-205: The code currently marks a bond Released after any LND
error from LndConnector::new()/cancel_hold_invoice; change the flow to
distinguish permanent "invoice not found / already canceled" errors from
transient transport/server errors: update the LND error mapping (the function
that currently converts gRPC errors into
MostroInternalErr/ServiceError::LnNodeError) to preserve error types or return a
typed enum/variant, then in the caller around cancel_hold_invoice only set
updated.state = BondState::Released and update the DB when the error variant
indicates NotFound/AlreadyCanceled; for transient errors (connection refused,
server error, timeouts) do not mutate the bond state or released_at,
propagate/return an error so the scheduler's retry logic (used by
find_active_bonds* which selects state IN ('requested','locked')) can retry, and
emit a structured log/event for operator visibility.
In `@src/app/cancel.rs`:
- Around line 324-332: The tracing::warn! macro calls that span multiple lines
cause cargo fmt --check to fail; find the four occurrences in cancel.rs where
bond::release_bonds_for_order(...) is awaited and the following tracing::warn!
is written across multiple lines (example site uses "maker_cancel: bond release
failed for {}: {}" with order.id and e) and collapse each tracing::warn!
invocation into a single-line call (e.g. tracing::warn!("maker_cancel: bond
release failed for {}: {}", order.id, e);) so all four sites are formatted
consistently.
In `@src/app/release.rs`:
- Around line 272-279: The tracing::warn! invocation is a single long line that
exceeds rustfmt width; update the warn! call around the
bond::release_bonds_for_order error branch so the macro arguments are split
across lines (e.g. put the format string and variables on separate lines or use
multi-line formatting) for tracing::warn!({ ... }) referencing order.id and the
error variable e, then run cargo fmt --all -- --check (or cargo fmt -- --check)
to verify formatting.
In `@src/app/take_buy.rs`:
- Around line 107-112: The code masks real DB errors by mapping any error from
find_active_bonds_for_order into MostroCantDo(CantDoReason::PendingOrderExists);
change it to propagate the original error from find_active_bonds_for_order (use
the ? operator or pass through the ServiceError-wrapping MostroInternalErr) and
only return Err(MostroCantDo(CantDoReason::PendingOrderExists)) when
existing.is_empty() is false (i.e., an active bond was actually found). Update
the call site that currently does .map_err(|_|
MostroCantDo(CantDoReason::PendingOrderExists)) so it returns the original DB
error and keep the PendingOrderExists branch solely for the non-empty result
check.
---
Nitpick comments:
In `@src/app/bond/db.rs`:
- Around line 86-110: The SQL is hardcoding state literals
('requested','locked') in find_active_bonds_for_order (and the preceding
active-bonds query); change this to derive values from the BondState enum
instead: either rewrite the query to use two ? placeholders and bind
BondState::Requested.to_string() and BondState::Locked.to_string() (mirroring
find_bonds_by_state), or fetch by order_id and filter in Rust using a new
BondState::is_active helper (implement is_active on BondState and use it to
retain only active bonds). Update the query in find_active_bonds_for_order (and
the other active-bonds query) to remove literal strings and reference the
enum-derived strings or the post-query filter.
In `@src/app/bond/flow.rs`:
- Around line 222-228: The loop in release_bonds_for_order creates a new
LndConnector per bond via release_bond; instead create one LndConnector once and
reuse it for the batch: instantiate LndConnector::new() before iterating bonds
in release_bonds_for_order, change or add an API like
release_bond_with_connector(pool, bond, &connector) (or make release_bond accept
a &LndConnector) and call that inside the loop, and ensure the connector is
properly dropped/closed after the loop; update function signatures (release_bond
or add release_bond_with_connector) and usages accordingly to avoid per-bond
handshakes.
- Around line 252-274: The spawned subscriber tasks in bond_invoice_subscribe
use tokio::spawn and are never tracked (no JoinHandle or cancellation), so they
can outlive a bond and log unknown-hash warnings when handlers like
on_bond_invoice_accepted/on_bond_invoice_canceled run; fix by creating and
storing a per-subscriber cancellation handle (e.g.,
tokio_util::sync::CancellationToken or an AbortHandle) when spawning, pass a
clone into the task and also select! on token.cancelled() inside the rx loop (or
abort the JoinHandle) so tasks are explicitly cancelled when the bond is
canceled/released, and keep the JoinHandle/cancellation handle in the
bond/subscriber registry so you can cancel and await the task on release.
- Around line 309-355: The current on_bond_invoice_accepted flow reads the bond,
modifies state, then calls Bond::update which allows a race where two concurrent
handlers both pass the in-memory guards and the second update overwrites the
first causing resume_take_after_bond to run twice; to fix, change the update to
an atomic conditional update (e.g. UPDATE bonds SET state='locked', locked_at=?
WHERE id=? AND state='requested') and check the affected rows after the SQL exec
in the code path that currently calls Bond::update (or replace Bond::update with
a new method that performs this conditional update), and only call
resume_take_after_bond when the conditional update reports 1 row affected
(otherwise treat as idempotent/no-op).
In `@src/app/cancel.rs`:
- Around line 149-157: Consolidate repeated "best-effort bond release"
boilerplate by adding a helper (e.g. release_bonds_for_order_or_warn) that calls
release_bonds_for_order(pool, order_id).await and logs a tracing::warn! on Err
without propagating; place this helper in the bond module (e.g.
src/app/bond/flow.rs or mod.rs) and replace each repeated pattern (currently
calling release_bonds_for_order(...) with an if let Err(e) { tracing::warn!(...)
}) in cancel.rs, admin_cancel.rs, admin_settle.rs, release.rs, scheduler.rs with
a one-line call to release_bonds_for_order_or_warn(pool, order.id,
"cooperative_cancel" or appropriate context).
🪄 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: 19d54fc9-33a9-44a3-b212-f1fb0b57779f
📒 Files selected for processing (13)
docs/ANTI_ABUSE_BOND.mdsrc/app/admin_cancel.rssrc/app/admin_settle.rssrc/app/bond/db.rssrc/app/bond/flow.rssrc/app/bond/mod.rssrc/app/cancel.rssrc/app/release.rssrc/app/take_buy.rssrc/app/take_sell.rssrc/config/settings.rssrc/main.rssrc/scheduler.rs
Tightens the Phase 1 taker-bond plumbing in response to PR feedback: - take_buy_action / take_sell_action no longer mask DB errors from find_active_bonds_for_order as PendingOrderExists; the original ServiceError::DbAccessError now propagates and PendingOrderExists is reserved for the actual "an active bond already exists" branch. - BondState::is_terminal / is_active helpers replace the stringified matches!(...) guards in release_bond and on_bond_invoice_canceled, so control flow no longer depends on Display values. State strings are parsed once via FromStr at the entry of each handler. - on_bond_invoice_accepted now drives the Requested → Locked transition through a conditional UPDATE WHERE state = 'requested', and only resumes the take continuation when the row count is 1. This closes the race where two concurrent subscriber firings (e.g. LND reconnect + restart-time resubscriber) both passed the in-memory guard and resume_take_after_bond ran twice. - Active-bond SQL no longer hardcodes the 'requested' / 'locked' literals; the queries bind BondState::Requested / Locked Display strings, mirroring find_bonds_by_state. - New release_bonds_for_order_or_warn helper consolidates the best-effort "release + log on err" boilerplate that was duplicated at eight call sites across release.rs, cancel.rs, admin_cancel.rs, admin_settle.rs, and scheduler.rs into a single line per site. - cargo fmt cleanup across the touched files. Deferred (not in Phase 1 scope): - Distinguishing transient vs permanent LND errors during bond release: Phase 1 explicitly chose "always release on exit" as the ironclad guarantee. A typed gRPC error layer is a cross-cutting refactor better paired with the Phase 3 payout retry work, where it belongs. - Reusing one LndConnector across release_bonds_for_order's batch: per-order batches are 1–2 bonds in practice; not worth the API churn at Phase 1. - Tracking subscriber JoinHandles for cancellation: mirrors the existing crate::util::invoice_subscribe pattern. Cleaning up subscriber lifecycle is a separate concern that should land for trade and bond invoices together. Tests: cargo test (246 passed), cargo clippy --all-targets --all-features (clean), cargo fmt --all --check (clean).
Three coordinated fixes for the Phase 1 taker bond flow: 1. on_bond_invoice_accepted no longer skips the take continuation forever after a transient resume failure. Previously the conditional UPDATE that drives Requested → Locked was the only path that could call resume_take_after_bond — if that resume failed (LND/DB/Nostr blip while creating the trade hold invoice), every subsequent Accepted firing saw `state = locked` and returned without retrying, leaving the order stuck in Pending with the bond still locked. The handler now decouples the state transition from the resume retry: after the conditional UPDATE, it re-reads the bond and the order, and runs resume_take_after_bond whenever the bond is Locked AND the order is still Pending. Multiple firings are idempotent because the order moves out of Pending on first success. 2. Resume is gated on order.status == Pending. If a maker / admin / scheduler cancellation wins the race after the bond invoice was paid but before the handler ran, the order has already been transitioned away from Pending; reactivating it would issue inconsistent PayInvoice / WaitingBuyerInvoice messages on a canceled trade. The new guard logs the skip and returns Ok cleanly. 3. release_bonds_for_order and resubscribe_active_bonds no longer short-circuit on Settings::is_bond_enabled(). An operator who toggles the feature off (or removes the [anti_abuse_bond] block) with bonds already locked in LND would otherwise strand taker funds — the cancel/release/admin/scheduler exits, and the restart resubscriber, would all skip release for those rows. Both functions now run on the *DB state*: a single indexed SELECT is the cost of safety, and it returns zero rows for nodes that never enabled the feature. The release_bonds_for_order_no_op_when_disabled unit test is replaced by release_bonds_for_order_runs_regardless_of_feature_flag, which asserts the new invariant directly. Tests: cargo test (246 passed), cargo clippy --all-targets --all-features (clean), cargo fmt --all --check (clean).
The previous release_bond unconditionally marked the bond Released
after warn-logging any cancel_hold_invoice error. For benign races
(invoice already canceled / not found) that's correct, but for
transient errors — LND unreachable, server returning Unavailable /
DeadlineExceeded / Internal — it stranded the taker's funds: the HTLC
stayed encumbered at LND while the DB row dropped out of
find_active_bonds*'s "active" set (state IN ('requested', 'locked')),
so no future code path retried the cancel.
This change:
- Modifies LndConnector::cancel_hold_invoice to preserve the gRPC
Status code in the error message via a stable `code=<Code>` prefix
(instead of stringifying the whole tonic::Status verbatim, which
loses structure to the call site).
- Adds a CancelOutcome classifier in bond/flow.rs that maps a
cancel error to AlreadyDone (NotFound / AlreadyExists / known
LND messages indicating the invoice is already gone) vs. Transient
(everything else, including LND unreachable). Anything we can't
classify confidently maps to Transient — the safer side is to
delay cleanup, never to falsely report a release on an HTLC LND
still has.
- release_bond now only marks Released when the cancel landed (Ok)
or was AlreadyDone. On Transient outcomes — including
LndConnector::new() failing because LND is unreachable — it emits
a structured warn (bond_id, order_id, outcome) and propagates the
error, leaving the bond in its current active state.
The recovery path for a left-active bond is implicit and already
wired: the LND subscriber spawned by bond_invoice_subscribe (and
re-attached by resubscribe_active_bonds on daemon restart) catches
the eventual InvoiceState::Canceled — emitted when the hold invoice's
CLTV expires and LND auto-cancels — and on_bond_invoice_canceled
then marks the bond Released. The structured warn gives operators a
visible signal in the meantime.
New unit tests cover the classifier across gRPC-code prefixes,
LND-specific message patterns, and the conservative Transient
fallback for unrecognised errors.
Tests: cargo test (249 passed), cargo clippy --all-targets
--all-features (clean), cargo fmt --all --check (clean).
Catrya
reviewed
Apr 28, 2026
Member
There was a problem hiding this comment.
-
If a taker accepts the order but, before paying the bond, decides to cancel because they don’t want to continue. When trying to cancel, Mostro returns the error
cant_do: is_not_your_orderand does not allow the action. The taker should be able to cancel at this stage if they choose not to proceed. -
If someone takes an order but has not yet paid the bond, the order remains in Pending, which is fine. However, if another user tries to take it, they cannot and receive
cant-do: pending-order-exists. The order should remain available so that whoever pays the bond first gets it. Otherwise, a malicious user could block orders without ever paying the bond.
Collaborator
I made some tests with Mostrix and I confirm this issue, but after bond payment cancel procedures seems working with Mostrix. Tested:
Missing failing bond payment tests. |
Member
|
This review was done by claude in ultrareview: Two 1. Phase 1 lets a taker bond-commit while the order remains in
Suggested fix: branch
2. Phase 1 promises that "on every exit the bond is cancelled and the row marked With Suggested fix: mirror the if let Ok(order_updated) =
crate::util::update_order_event(&keys, Status::Expired, order).await
{
let order_id = order_updated.id;
let _ = order_updated.update(pool).await;
bond::release_bonds_for_order_or_warn(pool, order_id, "pending_expiry").await;
}I’m manually testing everything now. If I find anything different, I’ll comment. |
Two interrelated taker-side gaps in the Phase 1 bond flow: - A taker who took a Pending order but hadn't paid the bond yet got IsNotYourOrder when trying to cancel, because cancel_action routed every Pending cancel through the maker-only path. cancel_action now recognises a bonded taker (event.sender matches an active bond's pubkey) and routes through cancel_order_by_taker, releasing the bond and republishing the order. - take_buy / take_sell rejected any new take while another taker had an active bond, letting a malicious user keep an order Pending by taking and never paying. The new bond::supersede_prior_taker_bonds helper releases still-Requested prior bonds (notifying the prior taker) so re-takes succeed; only a Locked prior bond — meaning the trade is already committed — still rejects with PendingOrderExists. For API-priced orders, the market-price recomputation also resets the local quote when superseding, so re-takes don't inherit a stale amount. take_sell additionally always assigns buyer_invoice from this take's payment_request, preventing a prior taker's invoice from leaking into the new take. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/app/cancel.rs (1)
349-362:⚠️ Potential issue | 🟠 MajorPending maker-cancel still does not notify the bonded taker.
This path cancels and releases bonds but only sends
Action::Canceledto the maker. The persisted taker should also receive cancellation notification.Suggested fix
enqueue_order_msg( request_id, Some(order.id), Action::Canceled, None, event.sender, None, ) .await; + // Notify taker as well when present. + if let (Ok(seller), Ok(buyer)) = (order.get_seller_pubkey(), order.get_buyer_pubkey()) { + let taker_pubkey = if seller == event.sender { buyer } else { seller }; + enqueue_order_msg( + None, + Some(order.id), + Action::Canceled, + None, + taker_pubkey, + None, + ) + .await; + } // Phase 1: a maker cancelling a still-Pending order may be racing // with a taker who just locked a bond. Release any active bond so // the taker is made whole.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/cancel.rs` around lines 349 - 362, The cancel path currently only notifies the maker via enqueue_order_msg and then releases bonds; after releasing bonds you must also notify the persisted taker that the order was canceled. Retrieve the persisted taker identifier from the order record (e.g., order.taker_id or the equivalent field) and call enqueue_order_msg with that taker, Some(order.id), Action::Canceled (matching the maker notification), and the same sender/context, ensuring the taker receives the cancellation; keep the bond::release_bonds_for_order_or_warn call as-is.src/scheduler.rs (1)
461-465:⚠️ Potential issue | 🟠 Major
Pending -> Expiredpath still misses bond release.When pending orders expire, bonds are not released in this path, which contradicts the “always release on exit paths” Phase 1 objective.
Suggested fix
if let Ok(order_updated) = crate::util::update_order_event(&keys, Status::Expired, order).await { - let _ = order_updated.update(pool).await; + let order_id = order_updated.id; + if order_updated.update(pool).await.is_ok() { + bond::release_bonds_for_order_or_warn(pool, order_id, "pending_expiry") + .await; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scheduler.rs` around lines 461 - 465, The Pending->Expired branch updates the order but never releases the associated bond; call the bond-release helper when transitioning to Status::Expired (e.g., invoke the existing release routine from crate::util or orders) in the same branch where update_order_event(...) returns Ok and/or immediately after order_updated.update(pool).await, handling/ logging any error from that release call so bonds are always released on this exit path; reference update_order_event, Status::Expired, and order_updated.update to locate and insert the release call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/bond/flow.rs`:
- Around line 127-141: The code currently sends the invoice via
enqueue_order_msg(...) before calling bond_invoice_subscribe(...), risking an
emitted invoice without an active subscriber; either subscribe first or undo the
persisted bond on subscribe failure. Move the bond_invoice_subscribe(hash,
request_id).await? call to before enqueue_order_msg(...) so the subscriber is
armed prior to emitting the invoice, or wrap bond_invoice_subscribe(...) in a
match/Result and on Err call the bond cleanup routine (e.g., cancel_bond(...),
release_bond(...), or delete the persisted bond row for bond_small/order.id) and
return the error; reference enqueue_order_msg, bond_invoice_subscribe, and the
bond cleanup function you have in this module.
- Around line 332-339: The current release_bonds_for_order_or_warn function
swallows all errors from release_bonds_for_order() by only logging a warning;
instead change its behavior to surface failures: update
release_bonds_for_order_or_warn to return Result<(), E> (or the crate's common
error type) and propagate the Err from release_bonds_for_order(), or implement
an explicit retry/schedule mechanism before returning Err. Locate
release_bonds_for_order_or_warn and the called release_bonds_for_order (and
release_bond) and either (A) change the caller chain to handle the propagated
Result so transient LND errors are retried/handled, or (B) implement a bounded
retry loop with backoff inside release_bonds_for_order_or_warn that attempts
release_bonds_for_order() N times and returns Err if still failing; ensure any
logged messages include the error and that the function signature is updated
accordingly.
In `@src/scheduler.rs`:
- Around line 414-427: The code currently ignores the Result from
order_updated.update(pool).await and proceeds to call
bond::release_bonds_for_order_or_warn(...).await even if the DB update failed;
change this to await and inspect the Result from
order_updated.update(pool).await (e.g., let res =
order_updated.update(pool).await), and only call
bond::release_bonds_for_order_or_warn(pool, order_id, "scheduler_timeout").await
when the update returned Ok; on Err, log or warn about the failed persistence
and skip the bond release so bonds are not released when the order state was not
saved.
---
Outside diff comments:
In `@src/app/cancel.rs`:
- Around line 349-362: The cancel path currently only notifies the maker via
enqueue_order_msg and then releases bonds; after releasing bonds you must also
notify the persisted taker that the order was canceled. Retrieve the persisted
taker identifier from the order record (e.g., order.taker_id or the equivalent
field) and call enqueue_order_msg with that taker, Some(order.id),
Action::Canceled (matching the maker notification), and the same sender/context,
ensuring the taker receives the cancellation; keep the
bond::release_bonds_for_order_or_warn call as-is.
In `@src/scheduler.rs`:
- Around line 461-465: The Pending->Expired branch updates the order but never
releases the associated bond; call the bond-release helper when transitioning to
Status::Expired (e.g., invoke the existing release routine from crate::util or
orders) in the same branch where update_order_event(...) returns Ok and/or
immediately after order_updated.update(pool).await, handling/ logging any error
from that release call so bonds are always released on this exit path; reference
update_order_event, Status::Expired, and order_updated.update to locate and
insert the release call.
🪄 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: c67fe4f4-93b5-4c2c-a9fd-8eaf07b02419
📒 Files selected for processing (13)
docs/ANTI_ABUSE_BOND.mdsrc/app/admin_cancel.rssrc/app/admin_settle.rssrc/app/bond/db.rssrc/app/bond/flow.rssrc/app/bond/mod.rssrc/app/bond/types.rssrc/app/cancel.rssrc/app/release.rssrc/app/take_buy.rssrc/app/take_sell.rssrc/lightning/mod.rssrc/scheduler.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- src/app/release.rs
- src/app/admin_settle.rs
- src/app/bond/mod.rs
- docs/ANTI_ABUSE_BOND.md
… expiry Two Phase 1 release-on-exit gaps the previous taker-cancel patch didn't cover: - cancel_pending_order_from_maker only DM'd the maker. A taker who had already requested or locked a bond on the still-Pending order got no signal even though the bond was being released — now we iterate find_active_bonds_for_order and Canceled-notify each bonded pubkey other than the cancelling maker. - job_expire_pending_older_orders flipped the order to Expired but never released attached bonds, so a slow taker whose bond was outstanding when the maker order's expires_at elapsed had the HTLC sit in LND until CLTV. Mirror the scheduler_timeout hook on the expiry path with release_bonds_for_order_or_warn(..., "pending_expiry"). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…on persist Two robustness fixes from the Phase 1 follow-up review: - request_taker_bond armed the LND subscriber AFTER shipping the bolt11. A taker who paid before the subscriber attached would slip through the Accepted event and the take would never resume, with the HTLC unwinding only at CLTV. Reorder so the subscriber is attached first; on subscribe failure roll back the persisted bond via release_bond and propagate the error so the take fails cleanly rather than leaving a Requested row with no listener. - Both scheduler jobs (job_cancel_orders and job_expire_pending_older_orders) ignored the Result from order_updated.update(pool) and unconditionally released bonds. On a persist failure the bond would be released while the order stayed in its pre-cancel/Pending status, so the next tick would re-publish a cancel with no funds left to refund. Match against the persist result and only release on Ok; on Err warn and let the next tick retry. CLTV remains the eventual safety net. Note: release_bonds_for_order_or_warn intentionally swallows transient LND errors and warns — the function name and doc spell out the best-effort contract, and release_bond's doc walks through the subscriber+CLTV recovery path. Strict callers can use release_bonds_for_order directly. Not changing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/cancel.rs`:
- Around line 418-445: The current flow blocks a bonded taker from
self-cancelling if find_active_bonds_for_order fails; change it so taker
self-cancel does not require a successful DB lookup: attempt to detect a bonded
taker first from the in-memory order fields (compare event.sender /
event.sender.to_string() against order.taker_pubkey or equivalent) and call
cancel_order_by_taker regardless of whether find_active_bonds_for_order returned
an error; if you still call find_active_bonds_for_order, treat DB errors as
non-fatal (log/ignore) and fall back to the order.taker_pubkey match before
returning MostroCantDo from this branch. Ensure you update the branch that
currently uses find_active_bonds_for_order, referencing the functions/variables
cancel_order_by_taker, find_active_bonds_for_order, event.sender, order,
order.taker_pubkey, and MostroCantDo.
- Around line 358-377: The code currently swallows errors from
find_active_bonds_for_order(...) which can hide failures to notify bonded
takers; change the if-let to a match on
crate::app::bond::db::find_active_bonds_for_order(pool, order.id).await so that
the Ok(active_bonds) arm retains the existing loop (using PublicKey::from_str,
comparing to event.sender and calling enqueue_order_msg), and the Err(err) arm
logs the error with clear context (include order.id and the error) instead of
dropping it silently; in both arms still call
bond::release_bonds_for_order_or_warn(pool, order.id,
"pending_maker_cancel").await so bond cleanup always runs.
- Around line 257-259: The taker bond release
(bond::release_bonds_for_order_or_warn) is executed at the fallible tail of
cancel_order_by_taker so early returns (the `?` exits) can leave the bond row
unreleased; create a deferred cleanup that always runs on function exit: after
you obtain the pool and order ID in cancel_order_by_taker, install a Drop-based
guard (or use scopeguard::defer) that calls
bond::release_bonds_for_order_or_warn(pool, order_id, "taker_cancel") unless
explicitly disarmed; then, on the normal successful path where you already
handle bond state, disarm the guard to avoid double-release. This guarantees
release on every exit while keeping the existing bond helper function
(bond::release_bonds_for_order_or_warn).
🪄 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: 82a00f2e-df1e-486a-a31c-7b4f522b3bf7
📒 Files selected for processing (3)
src/app/bond/flow.rssrc/app/cancel.rssrc/scheduler.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/scheduler.rs
- src/app/bond/flow.rs
Collaborator
These 2 scenarios are working now...
This message of cancel from maker now arrives to taker... |
- cancel_order_by_taker: split into a wrapper that always runs release_bonds_for_order_or_warn after the fallible inner flow, so early `?` exits can no longer strand a Requested/Locked bond row. - cancel_pending_order_from_maker: replace the silent if-let with a match that warns with order.id when the bond lookup fails, so bonded-taker notification failures stop being invisible. - cancel_action (Pending branch): stop propagating bond-lookup DB errors that block a legitimate taker self-cancel; log and fall back to the in-memory taker pubkey on the order before returning IsNotYourOrder. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
4 participants
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.
Summary
Implements Phase 1 of the anti-abuse bond rollout (issue #711, spec
in
docs/ANTI_ABUSE_BOND.md). Builds on the Phase 0 foundation thatshipped in #712.
This phase wires the opt-in taker bond into the take flow with a
strict always-release guarantee. When the operator enables
[anti_abuse_bond]andapply_tomatches the taker side, a Lightninghold-invoice bond is locked before the trade flow begins; on every
exit (happy path, unilateral cancel, cooperative cancel, admin
action, scheduler timeout) the bond is cancelled and the row marked
Released. No slashing yet — that lands in Phase 2+.Why this scope
The phase is intentionally inert without an opt-in. Default config
keeps
enabled = false, and nodes that don't add the[anti_abuse_bond]block at all behave exactly like before. Thislets operators flip the switch in staging and exercise hold-invoice
custody end-to-end without any user funds at risk if Mostro
mis-judges the situation.
Key design choice — alternative path from spec §6.2
mostro-core0.10.0 does not yet exposeAction::AddBondInvoice/Status::WaitingTakerBond. Phase 1 takesthe documented Alternative:
Status::Pendingwhile the bond is outstanding.Action::PayInvoice(the bond's payment hash uniquelydistinguishes it from the trade hold invoice that follows once the
bond locks).
bondstable.The dedicated action / status will land alongside the matching
mostro-corerelease in a later phase, at which point this can bemigrated transparently.
Files touched
src/app/bond/flow.rs(new) —request_taker_bond,release_bond,release_bonds_for_order, the LND subscriber thatflips bonds to
Locked, the resume-take continuation, andresubscribe_active_bondsfor daemon-restart resilience.src/app/bond/db.rs— newfind_bond_by_hash,find_active_bonds,find_active_bonds_for_orderhelpers.src/app/take_buy.rs,src/app/take_sell.rs— gate onbond::taker_bond_required(), persist the populated order, requestthe bond, return early.
PendingOrderExistsguard againstduplicate bonds when two takers race.
src/app/release.rs,src/app/cancel.rs(cooperative +unilateral, taker- and maker-side, including pending-order maker
cancels),
src/app/admin_settle.rs,src/app/admin_cancel.rs,src/scheduler.rs::job_cancel_orders.src/main.rs— callsbond::resubscribe_active_bondsnext tofind_held_invoiceson startup.src/config/settings.rs—get_bond/is_bond_enabledno longerpanic when
MOSTRO_CONFIGis uninitialized; returnsNone/falseso the bond gate is safe to call from any context(including unit tests that don't bring up the full configuration).
docs/ANTI_ABUSE_BOND.md— Phase 0 and Phase 1 marked completed;added Phase 1 implementation notes.
Compatibility statement
bondstable).Action/Statusvariants. The bond bolt11reuses
Action::PayInvoice; clients that don't know about bondssee one extra invoice to pay before the trade hold invoice — the
exact behavior we want for opt-in staging.
enabled = false. Existingsettings.tomlfiles behave identically.
Test plan
cargo test --all-features(245 passed)cargo clippy --all-targets --all-features(clean)cargo sqlx prepare -- --bin mostrod(no new compile-timequeries; existing offline data still valid)
enabled=true,apply_to=take)outstanding
Locked: subscriberre-attaches and the trade resumes once the bond is paid
[anti_abuse_bond]block):take → release flow identical to before this PR
References
docs/ANTI_ABUSE_BOND.md(§6 — Phase 1)Summary by CodeRabbit
New Features
Improvements
Documentation