feat(bond): Phase 1.5 — dedicated PayBondInvoice action + WaitingTakerBond status#736
Conversation
…rBond status Switch the bond protocol layer from the Phase 1 reuse of `Action::PayInvoice` / `Status::Pending` to the dedicated `Action::PayBondInvoice` and `Status::WaitingTakerBond` variants shipped by `mostro-core` 0.11.0. Pure protocol clean-up: no new slashing, no schema change, no behavioural change to the concurrent-bonds race that already landed on `main`. See `docs/ANTI_ABUSE_BOND.md` §6.5 for the spec. Changes: - **Cargo.toml**: bump `mostro-core` from 0.10.0 to 0.11.0. - **`src/app/bond/flow.rs::request_taker_bond`**: emit `Action::PayBondInvoice` (not `PayInvoice`); after the DM ships, flip the order from `Pending` to `WaitingTakerBond` and republish via NIP-33. The flip is best-effort and skipped when the order is already in `WaitingTakerBond` (concurrent takers). - **`src/app/bond/flow.rs::on_bond_invoice_accepted`**: defense-in- depth status check widens to accept both `Pending` and `WaitingTakerBond` as valid pre-trade states before resuming the take. `resume_take_after_bond` (via `show_hold_invoice` / `set_waiting_invoice_status`) drives the order out to `WaitingPayment` / `WaitingBuyerInvoice` exactly as before. - **`src/app/bond/flow.rs::on_bond_invoice_canceled`** + new `maybe_drop_waiting_taker_bond` helper: when the last active bond on an order is released and the order is parked at `WaitingTakerBond`, flip back to `Pending` and republish so the orderbook reflects the empty-bond state. No-op if other bonds are still racing or the order has already moved on. - **`src/nip33.rs::create_status_tags`**: add the load-bearing `WaitingTakerBond → (true, Status::Pending)` mapping (§2 principle 8). This is what keeps the order advertised under NIP-69's `pending` bucket during the bond window — the non-blockability invariant. - **`src/nip33.rs`**: `create_fiat_amt_array` and `create_source_tag` also recognise `WaitingTakerBond` as pre-trade so range-order min/max advertising and the mostro: source URL stay consistent with the NIP-69 mapping. - **`src/util.rs::get_ratings_for_pending_order`**: include maker ratings on the published event during `WaitingTakerBond` too — the wire status is `pending` and clients browsing the orderbook expect the rating attached either way. - **`src/app/take_buy.rs` / `take_sell.rs`**: entry status check widens to accept either `Pending` or `WaitingTakerBond`. The idempotent-retry path also emits `PayBondInvoice` (not `PayInvoice`). The locked-bond gate inside the bond block is unchanged. - **`src/app/cancel.rs::cancel_action_generic`**: the "Pending → maker / taker pre-trade cancel" guard widens to accept `WaitingTakerBond`, otherwise every cancel during the bond window would fall through to `NotAllowedByStatus` (regression vs Phase 1). Inside the branch, the existing two-route logic (maker self-cancel publishes `Canceled` + fans out bond releases; taker self-cancel releases only the sender's bond and only republishes Pending when no others remain) is unchanged. - **`src/app/trade_pubkey.rs`**: entry check widens for consistency with the other pre-trade actions. Tests (264 passing, +5 new): - `nip33::tests::waiting_taker_bond_maps_to_pending_on_wire` — the load-bearing NIP-69 invariant test. - `nip33::tests::pending_and_waiting_taker_bond_publish_the_same_wire_status` — regression guard for bucket equivalence. - `app::bond::flow::tests::maybe_drop_waiting_taker_bond_noop_when_other_bonds_active` — taker self-cancel with surviving racers leaves status alone. - `app::bond::flow::tests::maybe_drop_waiting_taker_bond_noop_when_order_not_in_waiting_status` — defensive no-op for already-transitioned orders. - `app::bond::flow::tests::maybe_drop_waiting_taker_bond_noop_when_order_missing` — defensive no-op for missing orders so the LND-cancel callback never propagates a hard error. `cargo fmt`, `cargo clippy --all-targets --all-features -D warnings`, and `cargo test` all green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
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 (2)
WalkthroughThis PR implements Phase 1.5 anti-abuse bond lifecycle semantics by introducing a dedicated bond invoice action, parking orders in a new ChangesPhase 1.5 Bond Lifecycle
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Suggested Reviewers
🚥 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 |
![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: bab26639f9
ℹ️ 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 review |
✅ Actions performedReview triggered.
|
![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 (2)
229-247: ⚖️ Poor tradeoffVerify error-handling contract for partial failures.
The status update happens after the DM ships (line 226), which is correct for atomicity. However, if
update_order_eventsucceeds butupdated.update(pool)fails (line 233), the order event is republished on the wire asWaitingTakerBondbut the DB still showsPending. This mismatch would self-heal on the nextrequest_taker_bondcall (idempotent guard at line 229), but could briefly confuse clients.Consider whether the warn-and-continue behavior is the right tradeoff here, or if the failure should propagate to prevent the bond from being created in an inconsistent state.
🤖 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` around lines 229 - 247, The current flow republished the WaitingTakerBond event via update_order_event but only logs a warning if updated.update(pool) fails, causing a wire/DB mismatch; change this to avoid proceeding with an inconsistent state by either (preferred) propagating the error from request_taker_bond so bond creation is aborted (replace the warn with an Err return using the function's error type) or (alternative) attempt a compensating action: republish a Pending event or retry the DB persist until success; locate the logic around request_taker_bond, get_keys, update_order_event and the updated.update(pool) call and implement one of these fixes so a republished WaitingTakerBond is never left unpersisted.
827-867: ⚖️ Poor tradeoffRace condition: check-then-act on active bonds.
Lines 850-852 query for active bonds, then line 855 republishes if the list is empty. Between these two operations, another concurrent taker could lock their bond (via
on_bond_invoice_accepted), making the "empty" check stale. The order would be dropped back toPendingeven though a winner exists.However, this race is self-healing: the winning bond's
on_bond_invoice_accepted(line 702) will immediately promote the taker context and drive the trade forward, republishing with the correct status. The brief flicker toPendingis a cosmetic glitch, not a data-corruption or user-impact issue.Fixing this would require a transactional check-and-update or advisory locking, which is high effort for minimal reward given the self-healing property.
🤖 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` around lines 827 - 867, The check-then-act in maybe_drop_waiting_taker_bond races with concurrent bond acceptance: find_active_bonds_for_order() is called, then update_order_event() may run on stale state; fix by performing the re-check-and-update under a database transaction or row lock so the decision is atomic — e.g., begin a transaction, SELECT the Order row FOR UPDATE (or otherwise lock it), re-run find_active_bonds_for_order() (or count active bonds) within that transaction, and only call update_order_event() and persist the change if the order is still Status::WaitingTakerBond and there are zero active bonds; alternatively implement the conditional update in update_order_event() to fail unless the order status and active-bond count match expectations so on_bond_invoice_accepted() wins any concurrent race.
🤖 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/flow.rs`:
- Around line 229-247: The current flow republished the WaitingTakerBond event
via update_order_event but only logs a warning if updated.update(pool) fails,
causing a wire/DB mismatch; change this to avoid proceeding with an inconsistent
state by either (preferred) propagating the error from request_taker_bond so
bond creation is aborted (replace the warn with an Err return using the
function's error type) or (alternative) attempt a compensating action: republish
a Pending event or retry the DB persist until success; locate the logic around
request_taker_bond, get_keys, update_order_event and the updated.update(pool)
call and implement one of these fixes so a republished WaitingTakerBond is never
left unpersisted.
- Around line 827-867: The check-then-act in maybe_drop_waiting_taker_bond races
with concurrent bond acceptance: find_active_bonds_for_order() is called, then
update_order_event() may run on stale state; fix by performing the
re-check-and-update under a database transaction or row lock so the decision is
atomic — e.g., begin a transaction, SELECT the Order row FOR UPDATE (or
otherwise lock it), re-run find_active_bonds_for_order() (or count active bonds)
within that transaction, and only call update_order_event() and persist the
change if the order is still Status::WaitingTakerBond and there are zero active
bonds; alternatively implement the conditional update in update_order_event() to
fail unless the order status and active-bond count match expectations so
on_bond_invoice_accepted() wins any concurrent race.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0201bbba-0143-46e3-b97b-0a28b1525f41
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
Cargo.tomlsrc/app/bond/flow.rssrc/app/cancel.rssrc/app/take_buy.rssrc/app/take_sell.rssrc/app/trade_pubkey.rssrc/nip33.rssrc/util.rs
…ry coverage Two findings from the bot review of PR #736: P1: request_taker_bond was reading order.status from the caller's snapshot, then unconditionally persisting a cloned order as WaitingTakerBond. The bond subscriber is armed before the DM ships, so a fast Accepted callback (taker pays instantly) could transition the order to WaitingPayment / WaitingBuyerInvoice before this block runs; a concurrent maker-cancel could move it to Canceled; another take from a different pubkey could already have flipped it to WaitingTakerBond. The stale write would revert any of those. Replace the blind write with a compare-and-swap UPDATE (`SET status = ? WHERE id = ? AND status = 'pending'`). When `rows_affected == 0` we exit cleanly — somebody else owns the order's status. When we win, we publish the NIP-33 event and then patch the persisted `event_id` with a targeted UPDATE rather than saving the full cloned row, so concurrent field writes aren't clobbered. P2: db::find_order_by_date — the query backing job_expire_pending_older_orders — filtered on `status = 'pending'` only. Phase 1.5 added the WaitingTakerBond pre-trade status; orders parked there past their expires_at would never expire, leaving the bond HTLCs tying up taker funds until CLTV expiry. Widen the query to `status IN ('pending', 'waiting-taker-bond')`. Tests (267 total, +3 new): - pending_to_waiting_taker_bond_cas_refuses_to_overwrite_concurrent_transition — the CAS SQL refuses to flip an already-transitioned order. - pending_to_waiting_taker_bond_cas_flips_when_status_unchanged — companion happy-path assertion. - test_find_order_by_date_includes_waiting_taker_bond — verifies the expiry query picks up both Pending and WaitingTakerBond expired orders, and excludes fresh / post-trade ones. `cargo fmt`, `cargo clippy --all-targets --all-features -D warnings`, `cargo test` — all green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Second codex review finding: the check-then-act sequence in
maybe_drop_waiting_taker_bond (read status → list active bonds →
publish+persist Pending) raced with concurrent
on_bond_invoice_accepted: between the bond listing and the persist,
a new bond could be created and locked, transitioning the order to
WaitingPayment, and our path would then overwrite WaitingPayment with
Pending.
Replace the three-step check-then-act with a single conditional
UPDATE that checks both predicates atomically:
UPDATE orders SET status = 'pending'
WHERE id = ? AND status = 'waiting-taker-bond'
AND NOT EXISTS (
SELECT 1 FROM bonds WHERE order_id = ? AND state IN ('requested', 'locked')
)
If `rows_affected == 0`, either the status moved on (winner promoted
to WaitingPayment, maker cancelled, etc.) or a concurrent bond is
still racing — both safe no-ops. If we win, we re-fetch the order to
build NIP-33 tags from a fresh snapshot, publish via
`update_order_event`, and patch the persisted `event_id` only — same
non-clobbering pattern used by the P1 CAS in `request_taker_bond`.
The codex finding's other note about `request_taker_bond`'s
warn-and-continue on publish failure was already addressed by the P1
fix (replaced the unconditional `updated.update(pool)` with a
targeted `event_id`-only UPDATE that runs only after the CAS
succeeds). The remaining failure modes (Nostr publish fails or
event_id patch fails) cause at most a one-event lag on the
orderbook, self-healed by the next status transition — and Pending
and WaitingTakerBond map to the same NIP-69 `pending` bucket
externally, so there's no observable regression.
Test: `maybe_drop_does_not_revert_concurrent_lock` — inserts a
`Locked` bond on an order in `WaitingTakerBond`, calls the helper,
and verifies the status stayed at `WaitingTakerBond` rather than
reverting to `Pending`.
268 tests pass (1 new); `cargo fmt`, `cargo clippy -D warnings`
green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![mostronatorcoder[bot]](https://avatars.githubusercontent.com/in/2922838?s=60&v=4){kind=small}
There was a problem hiding this comment.
I did a strict pass and I can't approve this PR yet.
The protocol cleanup itself looks good, and the compare-and-swap fix for Pending -> WaitingTakerBond addresses the earlier stale-write race correctly. I also agree with the dedicated PayBondInvoice action and the NIP-33 wire mapping strategy.
However, I see one blocking issue that changes behavior in a bad way:
- Orders in
WaitingTakerBondno longer participate in the pending-expiry job.
find_order_by_date() still selects only status == 'pending', but this PR introduces WaitingTakerBond as the normal pre-trade parked state while bonds are outstanding. That means an order can sit past expires_at and remain effectively available on the wire until the bond invoices resolve, instead of expiring on schedule.
This is especially problematic because WaitingTakerBond is intentionally published to Nostr as the same pending bucket, so from the outside the order still looks takeable while the daemon-side expiry sweep is no longer considering it.
I think the expiry path should include both pre-trade statuses (Pending and WaitingTakerBond) so Phase 1.5 preserves the previous expiration semantics.
Once that is fixed, I think this PR is in good shape.
There was a problem hiding this comment.
Re-reviewed the current PR head (85f0f196050a43c4178155127e96b9ed0532c4d1).
My previous blocking point about the expiry path no longer applies: the current head already updates find_order_by_date() to include both pending and waiting-taker-bond, which preserves the intended expiration semantics.
I also re-checked the current head for the earlier stale-write race around Pending -> WaitingTakerBond; the compare-and-swap fix is present, the WaitingTakerBond -> Pending drop-back path is implemented, and the related wire-mapping / take-entry / cancel-entry adjustments are consistent.
I ran the current test suite on the current head and it passes (268 passed).
Approving the PR as it stands. Sorry for the earlier stale review on an older state of the branch.
|
Testing this PR, what i did below:
Same tests repeated for a BUY order, same results. |
3 participants
Summary
Implements Phase 1.5 of the anti-abuse bond rollout (
docs/ANTI_ABUSE_BOND.md§6.5): retire the Phase 1 reuse ofAction::PayInvoice/Status::Pendingfor bond DMs in favour of dedicatedAction::PayBondInvoiceandStatus::WaitingTakerBondvariants. Pure protocol clean-up — no new slashing, no schema change, no behavioural change to the concurrent-bonds race already onmain.After this lands:
WaitingTakerBond(distinct from "no taker yet"), but the wire-published NIP-69 bucket stayspendingso the order keeps advertising as takeable.cancel_actionrecognisesWaitingTakerBondas a pre-trade state.PayBondInvoicefollowed by onePayInvoicefor the trade hold invoice, both visible as distinct action types on the wire.Changes
Protocol
mostro-core0.10.0 → 0.11.0 soAction::PayBondInvoiceandStatus::WaitingTakerBondare reachable.src/app/bond/flow.rs::request_taker_bond: emitAction::PayBondInvoice(notPayInvoice); after the DM ships, flip the order fromPendingtoWaitingTakerBondviaupdate_order_eventand persist. The flip is gated on current status so concurrent takers don't re-publish.src/app/take_buy.rs/take_sell.rsidempotent-retry path: also emitsPayBondInvoicewhen re-sending the same bolt11 to a taker whose bond is stillRequested.Status transitions
request_taker_bondflipsPending → WaitingTakerBondpost-DM.resume_take_after_bond(unchanged) drivesWaitingTakerBond → WaitingPayment/WaitingBuyerInvoicevia the existing trade-flow helpers.on_bond_invoice_canceled+ newmaybe_drop_waiting_taker_bondhelper: when the last active bond on an order is released and the order is inWaitingTakerBond, flip back toPendingand republish. No-op if other bonds are still racing.NIP-69 mapping (load-bearing —
docs/ANTI_ABUSE_BOND.md§2 principle 8)src/nip33.rs::create_status_tagsaddsStatus::WaitingTakerBond → (true, Status::Pending). This is what keeps the order advertised under thependingbucket during the bond window; a regression here would let a non-paying taker park the order off the orderbook.create_fiat_amt_arrayandcreate_source_tagalso recogniseWaitingTakerBondso range-order min/max advertising and themostro:source URL stay consistent with the wire bucket.src/util.rs::get_ratings_for_pending_order: include maker ratings on the published event duringWaitingTakerBondtoo.Take + cancel guards
src/app/take_buy.rs/take_sell.rsentry status check widens to accept eitherPendingorWaitingTakerBond. The locked-bond gate, idempotent-retry, and fresh-row creation logic inside the bond block are unchanged.src/app/cancel.rs::cancel_action_generic"pre-trade cancel" guard widens fromif Pendingtoif Pending | WaitingTakerBond— without this every cancel during the bond window would fall through toNotAllowedByStatus.src/app/trade_pubkey.rsentry check widens for consistency.Acceptance criteria (
docs/ANTI_ABUSE_BOND.md§6.5.4)Action::PayInvoice-for-bond workaround is retired —mostrodnow only emitsPayInvoicefor trade hold invoices andPayBondInvoicefor bonds.WaitingTakerBondmaps to NIP-69pendingand re-take from a different pubkey is accepted (concurrent-bonds race, unchanged frommain).bond::supersede_prior_taker_bondsis already removed (landed in PR feat(bond): concurrent taker bonds, first-to-lock wins (Phase 0+1) #733).Tests
5 new tests (264 total, all green):
nip33::tests::waiting_taker_bond_maps_to_pending_on_wire— load-bearing NIP-69 mapping invariant.nip33::tests::pending_and_waiting_taker_bond_publish_the_same_wire_status— bucket-equivalence regression guard.app::bond::flow::tests::maybe_drop_waiting_taker_bond_noop_when_other_bonds_active— taker self-cancel with surviving racers leaves status alone.app::bond::flow::tests::maybe_drop_waiting_taker_bond_noop_when_order_not_in_waiting_status— defensive no-op for already-transitioned orders.app::bond::flow::tests::maybe_drop_waiting_taker_bond_noop_when_order_missing— defensive no-op for missing orders so the LND-cancel callback never propagates a hard error.Existing tests covering the concurrent-bonds race (
lock_race_guard_admits_only_one_winner,concurrent_requested_bonds_coexist),find_active_bond_by_takerscoping,release_bondidempotency, andtaker_bond_requiredgating remain green.cargo fmt --all -- --check,cargo clippy --all-targets --all-features -D warnings,cargo test— all green.Test plan
WaitingTakerBond → PendingNIP-33 mapping (src/nip33.rs::create_status_tags)request_taker_bondmaybe_drop_waiting_taker_bondas the "last bond released" chokepointAction::PayBondInvoice(notPayInvoice); the order's daemon-side status isWaitingTakerBond; the published NIP-33 event hass=pending.PayBondInvoice; A's bond remains payable; A does not receiveAction::Canceledyet.PayBondInvoiceand, once the bond locks, onePayInvoicefor the trade hold invoice — two distinct action types on the wire, no memo parsing.IsNotYourOrder.Status::Canceled, every concurrent prospective taker receivesAction::Canceled.WaitingTakerBondtoPendingand is republished.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Chores
mostro-coredependency from 0.10.0 to 0.11.0.