docs: payment circuit breaker spec - #863
Conversation
Spec for a damage-containment mechanism: if a bug lets an attacker pull sats out of the node, mostrod should notice that outflow is outrunning inflow and halt every outgoing payment on its own, so an operator has time to investigate instead of waking up to a drained node. Documentation only — no code changes. Rollout is split into four atomic PRs (§8), starting with an observe-only phase that ships dark. Key design decisions recorded: - The gate lives in LndConnector::send_payment, the single chokepoint every Lightning outflow passes through. Halting the scheduler jobs is not sufficient: the largest outflow (the trade payout) is driven by a user-supplied message on the release hot path, not by a job. - Two detection layers. A per-order invariant (outflow never exceeds inflow) checked synchronously before dispatch, and rolling-window velocity / net-outflow rules evaluated by a watcher job. Balance reconciliation against LND is deliberately out of scope; §4.3 records what that costs. - The latch is persistent and manual-reset only. A breaker that clears on restart is exactly the condition an attacker would induce. - Fail closed, and write-ahead: a reserved ledger row counts against every budget as if it had succeeded, so a double-pay bug is caught by the gate on the second attempt rather than in the post-mortem. - Tripping freezes outgoing payments and nothing else. No hold invoice is cancelled, no corrective transfer is attempted. Known gaps are stated rather than assumed: every rule derives from mostrod's own accounting, so a bug that moves funds without writing a ledger row is invisible to all of them; and Cashu mode has no equivalent chokepoint, so it is not covered by these phases.
|
Warning Review limit reached
Next review available in: 8 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughThe pull request adds a complete specification for a Lightning payment circuit breaker. It defines payment contracts, synchronous and asynchronous checks, persistent state, ledger accounting, operations, configuration, rollout phases, tests, and documentation navigation. ChangesPayment Circuit Breaker
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 43405f6a48
ℹ️ 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".
| 3. Query `Σ in` / `Σ out` (including `reserved`) for `intent.order_id`. If the | ||
| §4.1 invariant would be violated, trip and reject. | ||
| 4. Insert the ledger row as `reserved`. |
There was a problem hiding this comment.
When two payouts for the same order enter the gate concurrently, both can read the same inflow/outflow totals before either inserts its reserved row, so both pass the invariant and are dispatched. This leaves the stated double-payment containment vulnerable to a check-then-insert race; perform the aggregate check and reservation as one serialized SQLite transaction or equivalent atomic conditional operation, and cover simultaneous attempts in the test plan.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid — fixed in 4770837.
§5.3 now performs the aggregate check and the reservation as one serialized transaction (BEGIN IMMEDIATE, serialized per order_id), with dispatch happening only after it commits, and I added an explicit note that splitting or reordering those steps reintroduces the race.
Worth recording why this is not theoretical in this codebase: the trade payout runs on the release hot path while the dev-fee job pays out against the same order_id, so the two writers genuinely overlap — it is not limited to a retry storm.
§10 gained the matching test: two payouts for one order_id submitted simultaneously, asserting exactly one reserves and dispatches.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/PAYMENT_CIRCUIT_BREAKER.md`:
- Around line 85-87: Update the fenced blocks containing the payment formula and
example rows in the documentation to specify the text language, ensuring every
affected Markdown code fence includes a text identifier.
- Around line 321-354: Update the circuit-breaker configuration deserialization
validation in the relevant types from src/config/types.rs to require
check_interval_seconds and reserved_row_timeout_seconds to be strictly positive,
rejecting zero and negative values at startup alongside the existing cap and
tolerance checks.
- Around line 200-208: Update the send_payment gate sequence so the invoice
payment_hash is derived before inserting the reserved ledger row, ensuring every
reservation is reaper-reconcilable. Keep latch, amount, and invariant checks
before reservation, then decode the invoice and only create the reservation
after successful hash derivation; ensure decode or pre-LND failures cannot leave
a hashless reserved row.
- Around line 81-95: Clarify the per-order invariant’s synchronous fee treatment
by defining whether outflow includes routing fees before confirmation. Update
the invariant and equal-to-inflow boundary behavior to reserve the configured
fee cap, such as routing_fee_cap_sats(amount), or an explicitly defined separate
fee budget, so pre-dispatch approval cannot exceed inflow once fees are applied.
- Around line 246-247: Scope the new-order rejection logic to Lightning-backed
flows only, preserving Cashu order acceptance in the shared accept_event
validation path. Update the relevant order-creation check and its documentation
near the described sections, or explicitly document and enforce that a Lightning
breaker trip intentionally blocks all order creation.
- Around line 26-29: Replace every hardcoded path-and-line reference in
PAYMENT_CIRCUIT_BREAKER.md with a stable file-and-symbol reference, including
changing the send_payment citation to src/lightning/mod.rs::send_payment. Search
the entire document for all path:line patterns and update each without altering
the surrounding guidance.
- Around line 151-159: Ensure the payment flow uses one validated amount: either
remove PaymentIntent.amount_sats and use the send_payment amount throughout, or
validate amount_sats equals amount and amount is positive before any ledger
write. Apply the same correction to the additional PaymentIntent
construction/use site around the referenced section, preventing ledger checks
and LND from receiving inconsistent or nonpositive amounts.
- Around line 204-208: Make the §4.1 invariant check and reserved-ledger
insertion atomic by wrapping both operations in a single transaction and
serializing concurrent attempts for the same order, using the repository’s
existing transaction and locking conventions. Ensure dispatch occurs only after
the reservation transaction commits, and add a test that concurrently submits
two payments for one order and verifies only one reservation is accepted.
- Around line 224-230: Update the reaper flow around
LndConnector::lookup_payment_status so only definitive terminal LND statuses
transition a reserved row to confirmed or failed. Preserve reserved accounting
for Ok(None), ambiguous responses, and in-flight or other non-terminal statuses
by retaining them as reserved or moving them to the established
unknown/manual-review state, without allowing retries that could double-pay.
- Around line 249-257: The reset documentation promises a persisted audit trail,
but the described single-row circuit_breaker_state fields overwrite prior trips
and resets. Update the CircuitBreakerStatus and CircuitBreakerReset
documentation to specify an append-only trip/reset event record, or revise the
wording to describe reason and reset_note strictly as current-state metadata
without claiming historical auditability.
- Around line 108-118: The payment cap documentation and test plan use
inconsistent boundary semantics. Define that caps trigger only when values
exceed the configured limit, update the test plan so an exact threshold passes
and threshold plus one trips, and apply the same expectation to the referenced
429-429 case while preserving the gate’s amount > max_payment_sats behavior.
In `@docs/README.md`:
- Line 12: Update the Payment Circuit Breaker entry in the README index to
render PAYMENT_CIRCUIT_BREAKER.md as a Markdown link targeting that document,
while preserving the existing description and default-status note.
🪄 Autofix
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 Plus
Run ID: c3f72d25-8c9a-49b4-a5f5-bb2b17e683e0
📒 Files selected for processing (2)
docs/PAYMENT_CIRCUIT_BREAKER.mddocs/README.md
- Replace every path:line reference with path + symbol (AGENTS.md policy). Several were already stale: lookup_payment_status had moved 325 -> 333 and job_process_dev_fee_payment 1087 -> 820. - Label both bare fenced blocks as text (MD040). - Make the check-and-reserve step one serialized transaction, so the trade payout and the dev-fee job cannot both reserve against the same order_id. - Derive the payment hash before reserving, so no reservation can be left unreconcilable by the reaper. - Only definitive terminal LND statuses clear a reservation; Ok(None) parks in a new 'unknown' state that keeps counting, instead of being freed for retry. - Make inflows idempotent on the settled invoice hash via a partial unique index, so an 'already settled' retry cannot inflate the allowance. - Define routing fees inside the invariant, reserved at routing_fee_cap_sats. - Drop the duplicated PaymentIntent.amount_sats field. - Add an append-only circuit_breaker_event table for the audit trail. - State that caps are strict, scope the order-creation block to Lightning mode, validate the duration settings, and extend the test plan to match. - Link the guide from the docs index.
ToRyVand
left a comment
ToRyVand
left a comment
There was a problem hiding this comment.
Took the three reviewer asks in the description and checked them against current main. The core design holds up; I found two things in the inventory/caveat that need correcting before Phase 1 builds on them, plus one threshold interaction.
§3 — the central premise is correct and complete ✅
Enumerated every public method on LndConnector (src/lightning/mod.rs): create_hold_invoice, subscribe_invoice, settle_hold_invoice, cancel_hold_invoice, get_chain_height, get_hold_invoice_expiry_height, send_payment, lookup_payment_status, check_payment_status, get_node_info. send_payment is the only one that moves funds out — no keysend, no onchain send, no SendCoins. So "every Lightning outflow passes through send_payment" is accurate, and §2.1's gate placement is sound.
Actual invocation sites (filtering doc-comment mentions) are exactly three:
src/app/release.rs:613— trade payoutsrc/app/dev_fee.rs:993— dev feesrc/app/bond/payout.rs:669— bond payout
Inflow inventory also verified correct: settle_seller_hold_invoice (src/util.rs:1682), slash_one (src/app/bond/slash.rs:783), resolve_range_maker_bond_at_close (src/app/bond/slash.rs:1073). Excluding cancel_hold_invoice is right — it returns the HTLC to whoever locked it.
1. §3 bond payout row cites the wrong files
The table attributes the bond payout to src/app/bond/slash.rs and src/app/bond/flow.rs. Both files exist, but neither contains a send_payment invocation — grep for .send_payment( returns nothing in either. The real call site is src/app/bond/payout.rs:669.
Minor as a doc fix, but worth correcting precisely because §3 is the table the design rests on: an implementer adding the PaymentIntent parameter would go to slash.rs/flow.rs and find nothing, and payout.rs is also where the retry/CAS state machine lives that the new parameter has to thread through.
2. §4.1's range-order caveat identifies the right hazard but prescribes something that can't be done as written
The caveat says the ledger "must key on the parent/root order_id for both the slash inflow and the payout outflow." That phrasing assumes bonds.order_id already holds the parent on a child row. It doesn't.
From the slice-slash INSERT (src/app/bond/slash.rs:965-989):
.bind(Uuid::new_v4()) // id
.bind(slice.id) // order_id ← the CHILD's id
.bind(parent_bond.id) // parent_bond_id
.bind(slice.id) // child_order_id ← also the child's idThe comment right above it (slash.rs:961) states it outright: "order_id is the slice's". So on a slice-slash row, order_id == child_order_id == slice.id, and the root is reachable only by following parent_bond_id → parent bond row → that row's order_id.
It's also not uniform across the rows this path writes. The maker-refund INSERT in the same function (slash.rs:1220-1239) binds order_id = root.id with child_order_id = NULL. So two row shapes coexist in bonds, meaning the same figure differs between them:
| Row | order_id |
child_order_id |
|---|---|---|
| slice-slash child | child/slice order | child/slice order |
| maker-refund | root order | NULL |
Both are created PendingPayout and both produce a payout through payout.rs → send_payment, so both hit the ledger.
The consequence is the exact failure mode the caveat warns about, just not avoided by the stated remedy. The inflow is one settled parent HTLC (the child "shares the parent HTLC" per slash.rs:961), so it books against the root order; a child slash payout keyed naively on bonds.order_id books an outflow against the child order id, which has zero inflow → per-order invariant violated → breaker trips on a legitimate range-order payout.
Suggested correction: state the resolution rule rather than the column — for a bond row with parent_bond_id IS NOT NULL, resolve the ledger's order_id through the parent bond's order_id; rows with parent_bond_id IS NULL already carry the root directly. The §6 schema comment needs the same fix, since payment_ledger.order_id currently documents "Range-order bond rows key on the parent, never the child" and inherits the same assumption.
Happy to be wrong here if there's a normalization step between the bond row and the payout that I missed — but I couldn't find one on the payout.rs:669 path.
3. §7 — max_payment_sats default collides with max_order_amount, and its fee treatment is unspecified
max_payment_sats = 1000000 is exactly the default max_order_amount (settings.tpl.toml:52, config/types.rs:551). So a maximum-size legitimate trade payout lands precisely on the cap — allowed under the strict-> rule per §4.2, but with zero headroom.
What makes that matter is that §4.2 doesn't say which figure the cap is compared against. §4.1 goes out of its way to define the per-order invariant in fee-inclusive terms (amount + routing_fee_cap_sats(amount)), but the max_payment_sats row says only "single payment exceeds cap." With max_routing_fee = 0.002, routing_fee_cap_sats(1_000_000) = 2 000 sats, so:
- compared against the bare amount → 1 000 000 ≤ 1 000 000, passes with no margin
- compared fee-inclusive → 1 002 000 > 1 000 000, a legitimate maximum-size trade trips the breaker on day one
Both readings are defensible from the text, and §4.1's care about fee-inclusiveness nudges toward the second. Worth pinning down explicitly in §4.2, and either raising the default above max_order_amount or noting that it must be set relative to it.
Related and much softer: max_outflow_sats_per_hour = 5000000 / max_outflow_sats_per_day = 20000000 work out to 5 and 20 max-size trades respectively. §6's Phase-0-calibration warning covers this, so just noting the magnitude for whoever does the calibrating.
Everything else read cleanly to me. Two things I'd call out as good rather than as requests: §4.3 recording the excluded LND-balance layer and its cost ("if the draining bug lives in the accounting itself, the breaker is blind to it") instead of quietly dropping it, and the latch/audit split in §5.6 — keeping the first trip rather than letting a second overwrite the record that matters is the detail that gets missed in this kind of design.
Contributor, not a maintainer — technical review only, no merge signal implied.
2 participants
Summary
Adds
docs/PAYMENT_CIRCUIT_BREAKER.md— a spec for a damage-containment mechanism against a hypothetical fund-draining bug.The premise: if an attacker finds a bug that lets them pull sats out of the node, mostrod should notice on its own that more sats are leaving than entering, halt every outgoing payment, and stay halted until an operator clears it. This is containment, not prevention — it bounds how much leaves before a human can react. It is worth building because the worst realistic scenario is not "we lost sats" but "we lost sats all night while nobody was watching".
Documentation only. No code changes. Rollout is split into four atomic PRs (§8), the first of which ships dark in observe-only mode.
Design decisions worth reviewing
LndConnector::send_payment(src/lightning/mod.rs:227), the single chokepoint every Lightning outflow passes through. Halting the scheduler payment jobs is not sufficient — the largest outflow, the trade payout, is driven by a user-supplied Nostr message on thereleasehot path (src/app/release.rs:589), not by a job. §3 tabulates the full outflow surface.reservedledger row counts against every budget as if it had succeeded, so a double-pay bug is caught by the gate on the second attempt rather than in the post-mortem. Confirmation becomes an accuracy improvement, not a safety requirement (§5.4).send_paymentgains aPaymentIntentparameter. Deliberate: it makes it impossible to add a new outflow without stating what it is and which order funds it.Known gaps (stated in the spec, not papered over)
payment_ledgerrow is invisible to all of them. The gate placement is what mitigates this.Test plan
order_id, neverchild_order_id, or a legitimate child payout trips the breaker).🤖 Generated with Claude Code
https://claude.ai/code/session_01T38hb5pq2hu3e53U42fGWQ
Summary by CodeRabbit