fix: keep amt and fa tags consistent during the taker-bond window

[Forte11Cuba]

Closes #927

A range order being taken published a contradictory event: the range in fa next to the taker's quote in amt, while still showing s: pending. The order was fine in the DB — the event was built from the in-memory struct that take_sell/take_buy mutate before the WaitingTakerBond republish.

Part 1 nip33: fa and amt now share a single publishes_as_pending() predicate. While a range order publishes as pending, amt stays "0" — byte-identical to the Pending event it replaces, so no client changes needed.

Part 2 scheduler sweep: if the LND cancel signal is missed, the order no longer sits at WaitingTakerBond for up to 24 h. A job runs every 300 s, releases demonstrably stale Requested taker bonds and drops the order back to Pending. One deviation from the issue: calling only maybe_drop_waiting_taker_bond would never fire (its CAS no-ops while a requested bond exists — exactly the stranded state), so the sweep releases those bonds first via the existing release_bond. The per-order step re-checks bond state and age at action time, since a concurrent take can land between the scan and the release.

Part 3 order creation: half-specified ranges (min xor max) are now rejected with InvalidAmount. They used to slip past both check_fiat_amount and check_range_order_limits and get persisted with a fixed sats price and no fiat amount.

Tested manually on regtest (Polar with a real LND node): took a range order of 5000-10000 CUP and let the bond expire without paying. The event kept amt: "0" and the full range in fa during the whole bond window, and since LND's cancel signal genuinely never arrived, the sweep recovered the order back to Pending in about 6 minutes. Also adds 6 new unit tests; full suite passes (1250 tests) and clippy is clean.

Summary by CodeRabbit

• New Features

  • Added an automatic Lightning-only recovery sweep for orders stuck waiting for a taker bond.
  • Eligible orders are returned to pending status and republished when the bond window expires without a cancellation signal.

• Bug Fixes

  • Orders with incomplete amount ranges are now rejected.
  • Range orders now display consistent amount information while awaiting a taker bond.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 2553ec2c-1c0c-4ef5-9192-84cd162934c5

📥 Commits

Reviewing files that changed from the base of the PR and between d2e114d and f5387fb.

📒 Files selected for processing (6)

• src/app/bond/db.rs
• src/app/bond/flow.rs
• src/app/bond/mod.rs
• src/app/order.rs
• src/nip33.rs
• src/scheduler.rs

Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.

---

Walkthrough

The changes reject partial range orders, keep NIP-33 range publication consistent during WaitingTakerBond, and add a Lightning-only scheduler sweep that releases stale requested taker bonds and returns stranded orders to Pending.

Changes

Taker bond recovery and order state handling

Layer / File(s)	Summary
Pending order validation and publication src/app/order.rs, src/nip33.rs	order_action rejects half-specified ranges. Pending range orders publish amt: "0" while retaining their fa range.
Stale bond detection and recovery src/app/bond/db.rs, src/app/bond/flow.rs	The database query finds stale WaitingTakerBond orders. The sweep re-checks bond state and age, releases stale requested bonds, and returns eligible orders to Pending. Tests cover stale, fresh, locked, bondless, and role-filtered cases.
Lightning scheduler integration src/app/bond/mod.rs, src/scheduler.rs	The reconciliation function is exported and scheduled every 300 seconds when Cashu mode is disabled.

Estimated code review effort: 4 (Complex) | ~45 minutes

Merge Risk: 🟠 High · up to f5387

The new stale-bond recovery can release and overwrite a taker bond after a valid payment has locked it, potentially allowing an order or trade to proceed without the intended anti-abuse bond. The cleanup transition needs an atomic state-and-age guard before this PR is ready to merge.

Sequence Diagram(s)

sequenceDiagram
  participant Scheduler
  participant BondFlow
  participant BondDatabase
  participant OrderStore
  participant Nostr
  Scheduler->>BondFlow: Run reconciliation every 300 seconds
  BondFlow->>BondDatabase: Find stale WaitingTakerBond orders
  BondDatabase-->>BondFlow: Return eligible orders
  BondFlow->>OrderStore: Re-check bond state and age
  BondFlow->>OrderStore: Release stale Requested bond
  BondFlow->>OrderStore: Set order to Pending
  BondFlow->>Nostr: Republish order as Pending

Loading

Possibly related PRs

• MostroP2P/mostro#767: Introduced related maker/taker bond lifecycle and scheduler logic.

Suggested reviewers: grunch, arkanoider

Poem

A rabbit checks the bond window bright

Stale holds release in moonlit light
Ranges keep their zero amt
Pending paths stay consistent
The scheduler hops every five minutes
And stranded orders find new beginnings

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 78.57% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 28 functions across 6 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the primary fix: keeping the NIP-33 amt and fa tags consistent during the taker-bond window.
Linked Issues check	✅ Passed	The changes satisfy issue #927: they align amt and fa for WaitingTakerBond range orders, add regression coverage, reconcile stale taker bonds, reject partial ranges, and preserve existing order behavi…
Out of Scope Changes check	✅ Passed	The changes are within issue #927 scope. The scheduler, database query, validation fix, exports, and tests directly support the stated objectives.

Full details: Linked Issues check

Explanation

The changes satisfy issue #927: they align amt and fa for WaitingTakerBond range orders, add regression coverage, reconcile stale taker bonds, reject partial ranges, and preserve existing order behavior.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[21Mill]

Reviewed at f5387fb. Verified locally: full suite green (1250 passed / 0 failed / 2 ignored) and clippy --all-targets --all-features -- -D warnings clean, as you describe. Part 1 and part 3 look right to me; part 3 in particular closes a genuine hole — check_range_order_limits only validates when both bounds are Some, so a half-range really did slip past everything.

Part 2 has a problem that I think blocks it, and it comes from one factual premise.

---

1. src/app/bond/flow.rs:1307 — the stale cutoff is derived from a setting that never reaches the bond invoice.

stale_cutoff = now - hold_invoice_expiration_window - 60. But that setting is never applied to a bond hold invoice: request_taker_bond calls create_hold_invoice (flow.rs:172), which sets only hash, memo, value and cltv_expiry and leaves expiry at 0 (src/lightning/mod.rs:452), so LND falls back to its documented default — "Payment request expiry time in seconds. Default is 86400 (24 hours)", per the AddHoldInvoiceRequest proto. The bond invoice is payable for 24h, not for 360s.

Two consequences:

• With the shipped config, a taker who pays their bond invoice 7 minutes after taking finds it already cancelled and the order yanked back to Pending — while the invoice was still open and payable at LND.
• The sweep is not a fallback. maybe_drop_waiting_taker_bond is driven by LND's cancel, which arrives at the 24h expiry — so on a healthy subscription the signal is not missed, it simply has not happened yet. This sweep becomes the primary mechanism on every take, shortening the bond window from 24h to ~6 minutes for everyone. Your regtest run reads the same way: the cancel signal had not "genuinely never arrived", it was 23h54m away.

Shortening the window may well be the right behaviour — it is what the config entry describes — but it is a live change for every take rather than the belt-and-braces recovery the doc comment describes.

2. src/app/bond/flow.rs:1339 and :1352 — the "inverse race cannot happen" premise is false, and the release has no state guard.

The doc justifies the unguarded release with "its hold invoice expired at least the grace margin ago, so LND no longer accepts payment on it". Per finding 1 that does not hold, which makes this sequence reachable:

1. the sweep reads the bond via find_active_bonds_for_order and sees Requested;
2. the taker pays; on_bond_invoice_accepted wins its Requested -> Locked CAS, promotes the taker context and resumes the take;
3. the sweep calls release_bond with its stale snapshot, cancelling the now-accepted HTLC.

Re-reading the state would not save it either: release_bond short-circuits only on state.is_terminal() (flow.rs:513), and Locked is not terminal. And the write is worse than the cancel — Bond::update is UPDATE bonds SET <every column> WHERE id = ? with no state predicate (src/app/bond/crud.rs:160-171), so it overwrites the concurrent Locked and restores the snapshot's locked_at. End state: a live trade backed by a bond whose sats were refunded to the taker.

Worth noting release_bond's own doc already says the real cancel comes "when the hold invoice's CLTV expires and LND auto-cancels" (flow.rs:496) — the contradiction is thirty lines up in the same file.

3. src/nip33.rs:318 — the amt fix is scoped to range orders, leaving the same contradiction for market-priced single orders.

create_amt_value returns "0" only under publishes_as_pending(order) && order.is_range_order(). A single order created at market price has amount == 0 while Pending, and take_sell / take_buy mutate it to the live quote before the republish (src/app/take_sell.rs:174-179), from the same in-memory struct. Its WaitingTakerBond event therefore carries s: pending with a concrete amt where the Pending event it replaces advertised "0" — the same self-contradicting event, just without the range beside it to make it obvious. I confirmed it against this branch:

assertion `left == right` failed
  left: "199399"
 right: "0"

Your test covers the fixed-price single order ("Single-amount order in WaitingTakerBond -> real amount"), which is correct — that one was never "0". The market-priced one is the gap. Gating on the persisted amount rather than on is_range_order() would cover both; note the current approach cannot, since once amount is mutated nothing on the struct distinguishes the two.

---

Checked and sound: the find_stale_waiting_taker_bond_orders SQL, including the bondless arm (a WaitingTakerBond order always has a taker bond row, since create_bond at flow.rs:188 precedes the status CAS at :271); child slash rows cannot pollute it (role=maker); the >= cutoff / < cutoff boundary is consistent between finder and sweep step; the scheduler gating under !is_cashu_enabled() and the resubscribe_active_bonds-before-start_scheduler ordering in main.rs.

publishes_as_pending() is the right abstraction and keeping amt byte-identical to the Pending event is the right call — the disagreement is only about how wide the predicate should be.

One cross-reference: this PR would make hold_invoice_expiration_window its first real consumer, and give it a third meaning (the taker bond window). #955 covers the setting's current state — it has no reader, and the deadline its config entry describes is enforced by expiration_seconds. Probably worth settling that before this lands on top of it.

[SIDHARTH20K4]

I've tested this PR thoroughly on Fedora with rustc 1.94.0.

Core Fix in nip33.rs - GOOD

The fix correctly addresses the main issue. publishes_as_pending() properly treats WaitingTakerBond as Pending for wire publishing, ensuring amt and fa stay consistent during the bond window.

Test Results

• Full test suite: 1250 passed / 0 failed / 2 ignored
• Clippy: Clean (--all-targets --all-features -- -D warnings)
• Regression test: amt_value_is_zero_while_range_order_publishes_as_pending passes

cargo test
# Result: test result: ok. 1250 passed; 0 failed; 2 ignored

cargo clippy --all-targets --all-features -- -D warnings
# Result: no warnings

Additional Finding: Market Orders

I added a test for market orders to verify the previous reviewer's finding:

#[test]
fn market_order_amt_stays_zero_during_bond_window() {
    // Market order: amount = 0 means "market price"
    let mut order = make_pending_order();
    order.amount = 0;
    order.min_amount = None;
    order.max_amount = None;
    order.fiat_amount = 1000;

    // Pending state - shows "0"
    assert_eq!(super::create_amt_value(&order), "0");

    // When taken - amount gets mutated to a quote
    order.status = Status::WaitingTakerBond.to_string();
    order.amount = 199_399;

    // The bug: this returns "199399", but Pending showed "0".
    // It should remain "0" for consistency.
    assert_eq!(super::create_amt_value(&order), "0");
}

Result

The test currently fails:

thread 'nip33::tests::market_order_amt_stays_zero_during_bond_window' panicked
assertion `left == right` failed
  left: "199399"
 right: "0"

Conclusion

The core fix in nip33.rs looks good and passes the existing regression test and full test suite.

However, the additional market-order test exposes a separate consistency issue: during the WaitingTakerBond state, a market order's amt changes from "0" to the taker's quoted amount ("199399").

If the intended behavior is that amt remains consistent with what was published while the order was Pending, then this case still needs to be addressed.