docs: harden cashu spec after review (fee crash-safety, reuse guard, action ownership)#794
Conversation
Fold the spec-review findings into the Cashu series: - H-1: fee token now persisted in the same atomic CAS write (cashu_fee_token + cashu_fee_redeemed_at, TA-1f migration) so a crash between lock and redeem can't strand the fee; scheduler retry job specified. The old text claimed the redeem was retryable while also mandating no persistence. - H-2: cross-order fee-token reuse guard — cashu_fee_proofs table keyed by Y = hash_to_curve(secret), inserted in the CAS transaction, closes the concurrent-validation TOCTOU (fee token is not order-bound). - H-3: action->owner matrix added to CF-5 so every blocked dispatch_cashu action has an unblocking track or an explicit permanently-blocked decision (NewOrder/FiatSent/RateUser were ownerless). - M-1: CF-4 test text no longer contradicts the no-status-predicate design of find_locked_cashu_orders. - M-2: daemon-side remaining-locktime guards (FiatSent margin, dispute alert, TA-3 monitor) specified as cross-track obligations in 4B. - M-3: mint keyset unit (sat) verified at connect and per proof. - LOW: README status for doc 02, CF-0 formal PR spec, locked-decision 6 amended for the 0.14 exception, new Track A 10 fixes the 9->11 gap. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 Run ID: 📒 Files selected for processing (2)
WalkthroughDocumentation-only updates across the Cashu escrow engineering plan refine the frozen-baseline rules, CF-0/CF-2/CF-4/CF-5 specs, Track A fee-token lock requirements, remaining-locktime guards, and README status/guidance. ChangesCashu Escrow Documentation Updates
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/cashu/01-fundamentals.md`:
- Around line 539-559: The action-ownership matrix is inconsistent for NewOrder
because it is assigned to TA-2 here even though TA-2 is reserved for the take_*
escrow-request work. Update the matrix entry in the relevant section so NewOrder
either points to the correct owning track/action handler or is marked as
permanently blocked, and keep the row aligned with the existing tracking symbols
like handle_message_action_no_ln and the TA-1/TA-2 ownership scheme.
🪄 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: caee221f-d80d-4135-bc84-e03538f28da5
📒 Files selected for processing (3)
docs/cashu/01-fundamentals.mddocs/cashu/02-track-a-lock.mddocs/cashu/README.md
![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: d6b29b080b
ℹ️ 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".
| retries pending redeems. This is the crash-safety + TOCTOU half; it is | ||
| **not** optional. |
There was a problem hiding this comment.
This makes the persistence/anti-reuse half mandatory, but the same PR breakdown still says Track A has "two core PRs plus one optional hardening PR" just above. In a nonzero-fee deployment, treating TA-1f as optional allows TA-1 to ship without the crash-safe fee token storage and cashu_fee_proofs guard that §4A now relies on, so a crash between lock and redeem or a concurrent token reuse can still leave an Active order with an uncollectable fee. Update the breakdown/sequencing to make TA-1f required or fold it into TA-1.
Useful? React with 👍 / 👎.
| | `Release` | **Track B** | cashu release handler | | ||
| | `RateUser` | **Track B** | `no_ln` once terminal states are reachable | | ||
| | `Cancel` | **Track C** | cashu cancel handler | | ||
| | `Dispute`, `AdminCancel`, `AdminSettle`, `AdminTakeDispute` | **Track D** | cashu dispute handlers | |
There was a problem hiding this comment.
The matrix states every blocked action must have an owner/permanent decision, and CF-5 blocks "any admin/dispute action", but the existing dispatcher has an Action::AdminAddSolver path in handle_message_action_no_ln and this new admin row omits it. In Cashu mode that leaves solver management either blocked forever or routed by an unowned future edit, which can break Track D dispute setup. Add AdminAddSolver here with an explicit final routing, likely no_ln, or a permanent-block decision.
Useful? React with 👍 / 👎.
The CF-5 ownership matrix assigned NewOrder to TA-2, but TA-2's spec in 02 only covered the take_* escrow-request work — the two docs were inconsistent. Keep TA-2 as the owner (permanently blocking NewOrder would recreate the H-3 gap) and make it explicit on both sides: the matrix row points to 02 §6 TA-2, and TA-2's scope now includes routing NewOrder to handle_message_action_no_ln, bundled with the take flow so creatable and takeable ship together (avoids the orphan-order hazard).
- 02 §6 intro still said 'two core PRs plus one optional hardening PR' (pre-TA-1f text): now three core PRs + optional TA-3, with an explicit rule that a nonzero-fee node must not enable cashu before TA-1f. - CF-5 ownership matrix omitted AdminAddSolver (present in the no-LN dispatcher, blocked by CF-5's admin rule): added with Track D as owner and no_ln as final routing.
1 participant
Summary
Strict review of the Cashu spec series (
docs/cashu/) surfaced six substantive findings plus doc-hygiene issues. This PR folds all of them into the spec before implementation starts. Docs-only — no code.HIGH
H-1 · Fee token was unrecoverable after a crash. §4A ordered validate fee → CAS → redeem while explicitly mandating that the fee token is not persisted — yet claimed the redeem was "retryable". A crash between CAS and redeem left a correctly-
Activeorder whose fee existed only in the dead process's memory, with the replay guard (correctly) refusing a second submission — closing the recovery path forever. Fix: TA-1f now ships a migration (cashu_fee_token,cashu_fee_redeemed_at) and extends the CF-4 CAS so the fee token is written in the same atomic UPDATE; a cashu-mode scheduler job retries pending redeems (the analogue of the LN payment-retry job). The optional locktime refinement no longer costs a migration.H-2 · Fee token reusable across orders (TOCTOU). Unlike the escrow token (whose 2-of-3 embeds per-order trade keys), nothing binds the fee token to an order — it's P2PK to the node-wide
P_M.verify_fee_token's unspent check only stops sequential reuse: two concurrentAddCashuEscrowfor two same-fee orders could both validate the same token before the first redeem, leaving one orderActivewith an uncollectable fee. Fix: acashu_fee_proofs (y PRIMARY KEY, order_id, created_at)registry (Y = hash_to_curve(secret)), inserted in the CAS transaction; UNIQUE violation ⇒InvalidCashuToken.H-3 · No track owned unblocking
NewOrder. CF-5 blocksNewOrder/FiatSent/RateUserand G-1 says each track unblocks "the actions it owns" — but no track owned those three. Followed literally, all four tracks could ship and a cashu node still couldn't create a single order. (The first Cashu attempt hit exactly this bug in code.) Fix: an action→owner matrix in CF-5 covering every blocked action, withAddInvoice/AddBondInvoiceas explicit permanently-blocked decisions and the rule that no action may be left ownerless.MEDIUM
find_locked_cashu_orders; the test now asserts the design instead.FiatSentrejected underescrow_settlement_margin_days(default 3, Track B), dispute-near-locktime solver alert (Track D), proactive buyer warnings (TA-3).connectnow requiressatkeysets andverify_escrow_tokenchecks the unit per proof (tokens may mix keysets).LOW / hygiene
CF-0…CF-5.release.rs).Test plan
Summary by CodeRabbit