feat(bond): Phase 4.5 — re-prompt winner for payout invoice on payment failure#755
feat(bond): Phase 4.5 — re-prompt winner for payout invoice on payment failure#755grunch wants to merge 2 commits into
Conversation
…t failure When a slashed bond's counterparty payout exhausted `payout_max_retries` against a submitted invoice, the bond went straight to `Failed` and the winner was never asked for a new invoice: the scheduler only enumerates `PendingPayout` rows, and the retry loop hammered the *same* unroutable bolt11. The §8.2 "Failed resurrection" recovery existed but only fired if the client spontaneously resubmitted — Mostro never prompted — so in practice the counterparty share stranded and needed operator intervention (issue #750). Phase 4.5 changes the retry-exhaustion transition in `on_send_payment_failure`: - **Inside the claim window**: discard the unroutable invoice and re-arm the invoice-request sub-phase (`payout_invoice`, `payout_routing_fee_sats`, `payout_payment_hash`, `last_invoice_request_at` cleared; `payout_attempts` reset to 0; state stays `PendingPayout`). The next scheduler tick re-prompts the winner via `request_payout_invoice`. `slashed_at` is never touched, so the forfeit deadline does not move and the re-prompt/retry cycle is bounded by `payout_claim_window_days` (→ `Forfeited`). - **Past the claim window**: transition to `Failed` as before (terminal technical failure, operator review). Daemon-only: reuses `Action::AddBondInvoice` (Phase 3) and `Action::BondInvoiceAccepted` (Phase 3.5). No mostro-core bump, no migration. `claim_window_seconds` is threaded through `pay_counterparty` into `on_send_payment_failure` for unit-testability. Spec §9.5 added; phase overview, §8.1/§8.2 cross-refs, and §14.2/§14.3 release status updated to reflect Phases 0–4 merged on main. Closes #750 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR adds Phase 4.5 documentation and implements claim-window-aware payout retry exhaustion: indeterminate failures retain the invoice for reconciliation, terminal failures inside the claim window clear stale invoice fields and re-arm the payout loop, and terminal failures outside the window transition the bond to Failed. ChangesPhase 4.5 payout recovery mechanism
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 983043c359
ℹ️ 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}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/ANTI_ABUSE_BOND.md (1)
194-217:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMark Phase 4.5 as implemented here.
This PR adds the Phase 4.5 daemon logic and tests, but the overview still says Phase 4.5 is
pendingand "not yet implemented." If merged as-is, the spec will immediately contradict the code and mislead follow-up rollout work.🤖 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 `@docs/ANTI_ABUSE_BOND.md` around lines 194 - 217, Update the doc to reflect that Phase 4.5 is implemented: change the table row for "4.5" from "pending" to "implemented" (and optionally add the PR/commit reference), and update the paragraph that currently says "Phase 4.5 and Phases 5–8 are not yet implemented." to say that Phase 4.5 is merged/implemented while Phases 5–8 remain unimplemented; ensure the Status section still cites the `mostro-core` pin in Cargo.toml (0.11.5) and any relevant PR numbers for Phase 4.5 so the spec matches the code.
🤖 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.
Outside diff comments:
In `@docs/ANTI_ABUSE_BOND.md`:
- Around line 194-217: Update the doc to reflect that Phase 4.5 is implemented:
change the table row for "4.5" from "pending" to "implemented" (and optionally
add the PR/commit reference), and update the paragraph that currently says
"Phase 4.5 and Phases 5–8 are not yet implemented." to say that Phase 4.5 is
merged/implemented while Phases 5–8 remain unimplemented; ensure the Status
section still cites the `mostro-core` pin in Cargo.toml (0.11.5) and any
relevant PR numbers for Phase 4.5 so the spec matches the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 447cc29c-d0b5-4417-84e2-879f4323b9f5
📒 Files selected for processing (2)
docs/ANTI_ABUSE_BOND.mdsrc/app/bond/payout.rs
…lures Codex review on #755 flagged a P1: the Phase 4.5 re-arm path cleared `payout_payment_hash` on every exhausted retry, including non-terminal failures (status-stream timeout, stream EOF, send_payment RPC error) where the original payment may still be InFlight in LND. Clearing the hash disables the reconciliation branch in `pay_counterparty`, so a freshly-prompted invoice (different hash) could be paid while the original payment later settles — a double payout. Introduce `PaymentFailureKind { Terminal, Indeterminate }` and thread it into `on_send_payment_failure`: - Terminal (LND-confirmed Failed, or structurally unusable invoice): no payment is/will be in flight, so the invoice may be abandoned — re-arm in-window, or Failed out-of-window (unchanged Phase 4.5 behaviour). - Indeterminate (timeout / EOF / send RPC error): keep `payout_invoice` and `payout_payment_hash` so reconciliation polls LND to a definitive Succeeded/Failed before anything new is paid. Never re-arms, never flips to Failed. `payout_attempts` saturates at `payout_max_retries` so a long LND outage can't grow it unbounded. The send-status stream now tracks whether it ended on an explicit `PaymentStatus::Failed` (terminal) vs timeout/EOF (indeterminate); the reconciliation-Failed and decode-failure paths are terminal; the send_payment RPC error is indeterminate. Adds `send_payment_indeterminate_failure_keeps_invoice` test; updates the existing exhaustion tests to pass the failure kind. Spec §9.5 documents the double-payout guard. Co-Authored-By: Claude Opus 4.8 (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.
Looks good to me. I reviewed the current head commit 08b0b6961a932d17ba5c4cc3df41c8cb8b26d895.
This hardens the Phase 4.5 payout path in the right places:
- it keeps indeterminate
send_paymentfailures on the current invoice pluspayout_payment_hash, so reconciliation can continue without risking a double payout, - it only re-arms on terminal failures while still inside the claim window,
- it clears the stale payout hash on resurrection so the row always describes the current invoice,
- and it preserves the fixed
slashed_atanchor instead of extending the winner's deadline.
I also checked the important races around forfeit_bond, request_payout_invoice, apply_payout_invoice, and the success-path Slashed CAS. The invariants are documented well and the guarded transitions are consistent with the intended state machine.
Checks run:
cargo test payout -- --nocapturecargo clippy --all-targets --all-features -- -D warnings
Both passed.
![ermeme[bot]](https://avatars.githubusercontent.com/in/3690068?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
Verdict: Approve
Reviewed current head 08b0b6961a932d17ba5c4cc3df41c8cb8b26d895.
I checked the payout retry / re-arm logic and the new tests cover the important branches:
- terminal failure past the claim window →
Failed - terminal failure within the claim window → re-arm and re-prompt
- indeterminate failure → keep invoice/hash for reconciliation
Local verification:
cargo test payout -- --nocapturecargo test bond -- --nocapture
Both passed.
1 participant
Summary
Implements Phase 4.5 of the anti-abuse bond rollout (spec
docs/ANTI_ABUSE_BOND.md§9.5), fixing the payout failure mode reported in #750.When a slashed bond's counterparty payout exhausted
payout_max_retriesagainst a submitted invoice,on_send_payment_failuretransitioned the bond straight toFailedand the winner was never asked for a new invoice again:run_bond_payout_cycle) only enumeratesPendingPayoutbonds — aFailedbond is invisible to it, soAction::AddBondInvoiceis never re-sent.payout_invoice, so all retries hit the same unroutable bolt11 — Mostro never asks for a fresh one.A recovery path existed (the "
Failedresurrection" branch inapply_payout_invoice), but it only fires if the client spontaneously resends. Mostro never prompts, so in practice the counterparty share stranded and required manual operator intervention.What changed
src/app/bond/payout.rs—on_send_payment_failurenow decides what to do after the retry budget for a given invoice is exhausted, based on the forfeit window anchored onslashed_at:payout_invoice,payout_routing_fee_sats,payout_payment_hash,last_invoice_request_at; resetpayout_attempts = 0; keepstate = PendingPayout. The next scheduler tick seespayout_invoice IS NULLand re-prompts the winner viarequest_payout_invoice.slashed_atis never touched, so the forfeit deadline does not move and the re-prompt/retry cycle is bounded bypayout_claim_window_days(→Forfeited).invoice_request_attemptsis preserved (it's bounded by the forfeit window, not the retry budget).Failedas before (terminal technical failure, operator review). No point re-prompting past the deadline.claim_window_secondsis threaded fromprocess_one_bondthroughpay_counterpartyintoon_send_payment_failure(consistent with howmax_retriesis already passed), keeping the function unit-testable.This is daemon-only: it reuses
Action::AddBondInvoice(Phase 3) andAction::BondInvoiceAccepted(Phase 3.5). Nomostro-corebump, no schema migration.The spec was also updated: §9.5 added, the phase-overview table now marks Phases 0–4 as merged on
main(PRs #712, #719, #736, #737, #738, #743, #744), and §8.1/§8.2/§14.2/§14.3 carry forward-references and corrected release status.How to test
Automated
New/updated unit tests in
src/app/bond/payout.rs:send_payment_failure_within_window_reprompts_winner— aftermax_retriesfailures with an in-windowslashed_at, the row re-arms:statestaysPendingPayout,payout_invoice/payout_routing_fee_sats/payout_payment_hash/last_invoice_request_atcleared,payout_attempts = 0,slashed_atunchanged,invoice_request_attemptspreserved.send_payment_failure_past_window_flips_to_failed— same exhaustion but with an out-of-windowslashed_atstill terminates inFailed.apply_payout_invoice_resurrects_after_re_failure— updated to force the out-of-window branch where it needs aFailedrow.Manual (regtest / Polar)
slash_node_share_pctbetween 0 and 1 and a shortpayout_invoice_window_secondsso re-prompts are quick to observe.BondResolution) so a bond entersPendingPayout.Action::AddBondInvoice, reply from the winner with a bolt11 that cannot be routed (e.g. an invoice from a node with no path / no inbound liquidity).send_paymentfails up topayout_max_retries, then logsre-requesting a fresh invoice from the winner (still inside claim window, deadline unchanged)and the bond staysPendingPayoutwith its invoice cleared.Action::AddBondInvoiceis delivered to the winner. Reply with a routable invoice → the payout succeeds, the bond reachesSlashed, and the winner getsAction::BondInvoiceAcceptedthenAction::BondPayoutCompleted.slashed_at + payout_claim_window_days) the client renders does not shift across re-prompts.payout_claim_window_daysand confirm the bond eventuallyForfeited(no invoice in flight) — i.e. the loop is bounded.Closes #750
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests