Repository navigation
Conversation
Allow the API send endpoint to pay an external bolt11 invoice when the `to` field is a `lnbc...` payment request (optionally `lightning:`-prefixed). - Decode the invoice and take the amount from it; reject amountless invoices - Enforce the same min/max and admin-approval thresholds as internal sends - Idempotency lock + dedup keyed on the invoice payment hash - Balance check reserves ~2% for routing fees; response reports the real fee - Large invoices reuse the existing Telegram admin-approval flow - Response adds `payment_hash` and `fee` (sats) for invoice payments Docs: add provider-facing docs/invoice-payments-api.md and update referral-api.md / README for invoice support.
Balance and approval safety: - Re-check the *available* balance (wallet minus pot reservations) when an API transaction is approved, for both the internal-transfer and bolt11 paths. Pot balances are DB-side reservations that never leave the lnbits wallet, so the raw balance let an approved payment spend pot-reserved sats that the submission-time check had excluded. - Hold a per-sender lock across the balance-check-then-pay window in sendToInvoice, so concurrent requests cannot each pass the available balance check and together overdraw. Invoice approval de-duplication: - Persist the bolt11 payment hash on PendingTransaction. - Add InvoiceApprovalLock, a Bunt record tying a payment hash to its outstanding approval, and reject a second request for an invoice that already has one waiting. The existing MemoCache lock only spans 5 minutes while an approval stays actionable for 24h, so it could not cover the approval window on its own. Other: - Return a generic error when an invoice payment fails; the raw error can carry the internal lnbits host/port. Details stay in the logs. - Include the caller-supplied memo in the sender's Telegram confirmation, labelled separately from the invoice description. - Build the API approval keyboard per request instead of mutating a shared ReplyMarkup. These requests originate from concurrent HTTP handlers, which the Telegram lockInterceptor does not serialise, so concurrent approvals could overwrite each other's callback data.
lnbits answers POST /api/v1/payments once the payment is handed to its
backend, not when it settles. Follow each outgoing bolt11 payment to a
terminal state before reporting it.
- /api/v1/send returns settled (200), failed (400) or pending (202) with
the payment hash; add GET /api/v1/send/payment/{payment_hash} to poll
- admin-approved invoice payments also wait, and record failed/in_flight
- add settlement_timeout config, budgeted against the server write timeout
- accept lnbits 1.x RFC3339 payment timestamps and payment status
Security:
- reject replayed HMAC signatures and future timestamps
- validate payment_hash before it reaches an lnbits URL
- share one lnbits rate limiter between analytics and settlement checks
Review feedback: - use integer math for the 2% routing fee reserve - fail closed when the invoice approval lock cannot be read or written - return an error, not 202, when the Telegram approval request cannot be delivered; mark the pending transaction failed and deactivate the button Concurrent payments: - replace the per-sender lock with balance reservations, so several invoices from one wallet can be paid at once without spending pot funds - reservations cover API invoice/internal sends and both approval paths, and are released once lnbits accepts the payment Approval expiry: - the Telegram approve button now expires with the pending transaction (24h), matching what /api/v1/send/status reports
0xMadie
left a comment
0xMadie
left a comment
There was a problem hiding this comment.
BackendBanda Code Review — PR #72: bolt11 Lightning invoice payments
Review effort: 4/5 | Reviewer: backend-banda | Date: 2026-10-14
Summary
Solid, well-structured feature. The core correctness concerns (double-spend, false settlement confirmation, replay attacks) are all addressed. A few things need fixing or a decision before merge.
✅ What's good
HMAC single-use enforcement (middleware.go)
signatureSet.claim()with in-memory map + TTL = correct approach. Eviction on each call avoids unbounded growth. The TTL is2 * toleranceseconds — wide enough to cover the full valid window.- Future-timestamp rejection (
age < -tolerance) closes the captured-request replay window. Breaking change called out clearly in PR description.
Settlement polling (settlement.go)
WaitForOutgoingPayment→ polling loop with exponential backoff (500ms → 3s cap × 1.5). Correctly stays inside the 90s write timeout viasettlementBudgettrimming against elapsed time since request start.classifyPaymentis appropriately conservative:paid:falsewith no status →PaymentStateUnknown, never failure. This avoids falsely telling callers their sats came back.- Settlement preimage zero-value guard (
strings.Trim(preimage, "0") == "") is correct — lnbits returns all-zero preimage for unsettled payments. - Test coverage of all lnbits response shapes including public-key response (no
status) →PaymentStateUnknown. Good.
Balance reservation under concurrency (balance_reservation.go)
- Per-sender
sync.Mutexserialises the check-then-reserve step.once.DoinRelease()prevents double-release. The concurrent stress test (50 goroutines × 100 sat from 1000 balance → exactly 10 granted) is clean and passes. - Reservation released immediately after
lnbits.Payaccepts, not after settlement polling: correct — lnbits already reflects the deduction once accepted, so holding longer would double-count.
Idempotency (two-layer)
- Short MemoCache lock (concurrent requests, ~5 min) + durable
InvoiceApprovalLockin Bunt (24h, for re-submitted approvals) — the gap between them is documented and addressed. - Invoice payment hash validated as
^[0-9a-f]{64}$before use in lnbits URL. ✅
Approval flow (api_approval.go)
- Per-request
apiApprovalMarkupavoids the race between concurrent approval messages overwriting shared callback data. - Expiry check at approve-time (
time.Now().After(approvalData.expiresAt())) with backwards-compat fallback for old approvals (CreatedAt + PendingTxExpiry). Correct. - Failed Telegram delivery → immediate error + rollback (
abandonPendingTransaction), not a silent 202. ✅
🔴 Must-fix before merge
1. signatureSet is process-local — multi-instance deployments allow replay
var usedSignatures = &signatureSet{...} lives in Go process memory. If the bot runs as more than one instance (e.g. two pods, or a restart mid-tolerance-window), a captured signature can be replayed against the second instance.
For a single-process bot this is fine; for any scaled deployment it is not. Either:
- Document explicitly that the bot must run as a single instance, and add a startup log warning if that changes, OR
- Move the seen-signature store to Bunt (same pattern as
InvoiceApprovalLock) with a TTL-based cleanup.
Action needed: at minimum, a comment in signatureSet stating the single-instance assumption. If multi-instance is in the roadmap, make it Bunt-backed now.
2. MemoCache TTL for the concurrent invoice lock is undocumented / unclear
s.MemoCache.SetNX(lockKey, "locked") is used without a TTL here (in sendToInvoice). The comment says "short lock...expires after 5 minutes" but I don't see where that TTL is set. If MemoCache entries never expire, the lock is permanent: a request that dies after SetNX but before defer Delete (e.g. server crash, panic) would permanently block that invoice from being paid.
Please confirm: does MemoCache have a global TTL? If not, add an explicit TTL to this SetNX call (e.g. 10 minutes), or document the GC/restart recovery path.
3. SendPaymentStatus (GET /api/v1/send/payment/{hash}) has a zero-timeout single lookup
settlement := s.Bot.Client.WaitForOutgoingPayment(*fromUser.Wallet, paymentHash, 0)With timeout=0, WaitForOutgoingPayment does at most one rate-limited check. If lnbits is slow or the rate limiter is busy, this could return PaymentStateUnknown for a payment that would resolve within 1s. The caller gets unknown and has to retry anyway — so the endpoint is not saving them a round trip.
Suggest: a short timeout (e.g. 5s) so a single poll has a chance to land, while still returning promptly. Or document that callers should expect unknown and retry with backoff.
🟡 Non-blocking observations (address in follow-up or now, author's call)
4. lnbc... / lightning: prefix lowercased before invoice check
strings.ToLower(req.To) before lightning.IsInvoice — fine for the prefix, but bolt11 invoices themselves use bech32 which is case-insensitive. decodepay handles upper/lower, so this is safe. Just noting it's intentional.
5. Approval-flow invoice path: reservation acquired in approveAPITransactionHandler but not in executeApprovedInvoicePayment?
In the non-invoice approval path, ReserveBalance is called before the transfer and deferred-released. In executeApprovedInvoicePayment, I don't see an equivalent reservation — the approval handler reserves, but does it hold through the pay call? Please verify the reservation is not released before lnbits.Pay returns, or add one inside executeApprovedInvoicePayment.
6. payment_hash in pending transaction ID is included in plain text
id := fmt.Sprintf("pending-%s-invoice-%s-%d", ...) — the payment hash is part of the bunt key. Payment hashes are public on the Lightning network so this is not a secret, but if bunt keys are ever logged or exposed, it ties a username to a specific payment. Minor.
7. Test for SendPaymentStatus endpoint is missing
The settlement_test.go covers classifyPayment well. The HTTP handler itself (SendPaymentStatus) has no test. At minimum an integration-style test that exercises the auth check and the hash validation would catch regressions.
Verdict
Approve with blocking comments resolved. Issues 1 and 2 need a documented answer or a code fix; issue 3 needs at minimum a comment. Non-blocking items can go in a follow-up.
go build ./... and go test ./... are reported clean; I cannot run them locally (no repo access) but the test coverage on the new packages is solid.
⚠️ Note for merge: this PR changes the HMAC auth contract for ALL existing/send,/userbalance,/usertransactions, and status callers. Any client that retries with the same timestamp+signature will receive401 Request already processed. Make sure existing integrations (CeyFi, DCA bot) sign fresh timestamps on every request including retries before merging to main.
— backend-banda (T-0025)
3 participants
Summary
Adds external bolt11 Lightning invoice payments to the existing
/api/v1/sendendpoint. Whentois alnbc...payment request (optionallylightning:-prefixed), the bot pays it externally via lnbits instead of doing an internal transfer.What changed
Invoice payments
tomatching a bolt11 invoice routes tosendToInvoice; the amount comes from the invoice (amountless invoices are rejected).Settlement confirmation
settlement_timeout, default 60s, kept inside the 90s server write timeout).200 settled(withpreimageand realfee),400 failed(withpayment_hash), or202 pending(do not retry).GET /api/v1/send/payment/{payment_hash}to resolve a pending payment; approved payments that stay unresolved are recorded asin_flight, neverexpired.Concurrent payments
Security / review fixes
payment_hashis validated (64 hex chars) before it reaches an lnbits URL.202./api/v1/send/status.lnbits 1.x
timeaccepts RFC3339 strings; paymentstatus(success/pending/failed) is read. Branch is up to date withmain(lnbits 1.x account API)./send,/userbalance,/usertransactions,/referral/lookup, status endpoints) now reject a reused signature (401 Request already processed). Clients must sign every request, including retries, with a freshX-Timestamp.Docs
docs/invoice-payments-api.mddocs/referral-api.mdandREADME.md.api.send.settlement_timeout(seconds, default 60).Testing
go build ./...cleango test ./internal/lnbits/ ./internal/utils/passtelegrampackage tests run from the repo root (i18n loadstranslations/relative to the working directory).🤖 Generated with Claude Code