pm: Defer ticket redemption when the sender cannot cover the face value - #4012
pm: Defer ticket redemption when the sender cannot cover the face value#4012rickstaa wants to merge 1 commit into
Conversation
livepeer/protocol#657 changed TicketBroker.redeemWinningTicket to require the sender's deposit and reserve to cover the full ticket face value. A redemption below it now reverts and leaves the ticket unused, where it previously consumed the ticket and paid out whatever remained (potentially dust). go-livepeer had no matching precondition, so it would keep building redemption transactions that cannot succeed. Mirror the broker's requirement before any RPC round-trip: availableFunds is served from the sender watcher cache, which the block watcher keeps current from DepositFunded and ReserveFunded, so a top-up is picked up on the next L1 block without a doomed transaction in between. The queue re-evaluates on every block, so a deferred ticket is redeemed as soon as the sender funds it, for as long as the ticket remains valid. Treat the underfunded case as an expected state rather than a failure: it is retryable and logged at V(5), since at one line per ticket per block it would otherwise flood the log for as long as a sender stays underfunded. Stop re-attempting the head of the queue within a single block event. Every iteration re-selects the earliest unredeemed ticket, so any outcome that leaves that ticket in place would re-attempt it once per queued ticket. The loop now stops and waits for the next block. Finally, do not infer that a ticket was consumed from an opaque error. CheckTx reports only that a transaction failed, without a revert reason, and isNonRetryableTicketErr matched that string to mark the ticket redeemed locally. Now that a reverted redemption leaves the ticket redeemable, ask the broker whether it was actually used before dropping it, so a still-valid ticket is not forfeited by a redemption that raced the sender's balance. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the probabilistic micropayments (pm) redemption flow to align with the TicketBroker behavior change from livepeer/protocol#657, ensuring tickets are deferred (not dropped) when the sender cannot cover full face value and reducing redundant per-block retry attempts.
Changes:
- Add an explicit, retryable
errInsufficientSenderFundsgate before RPC/tx submission, and treat reverted-but-unconsumed redemptions as retryable via an error wrapper. - Prevent N-attempts-per-block spinning on a blocked head-of-queue ticket by stopping the block handler loop early on retryable outcomes.
- Add/extend tests covering the new retry/defer semantics and queue behavior; update pending changelog.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pm/validator.go | Introduces errInsufficientSenderFunds sentinel and unconsumedRedemptionErr wrapper type. |
| pm/stub.go | Extends stub broker to model reverts that do not consume tickets. |
| pm/sendermonitor.go | Adds pre-RPC face-value funding gate and consults IsUsedTicket after CheckTx failure to avoid dropping unconsumed tickets. |
| pm/sendermonitor_test.go | Adds tests for pre-RPC gating, retryability after revert, top-up redemption, and isNonRetryableTicketErr cases. |
| pm/queue.go | Stops per-block loop on retryable head-of-queue failures; logs insufficient-funds deferrals at V(5); updates retryability logic. |
| pm/queue_test.go | Adds test ensuring only one redemption attempt per block when head-of-queue is retry-blocked. |
| CHANGELOG_PENDING.md | Adds orchestrator-facing changelog entry describing the new deferral behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // CheckTx only reports that the transaction failed, without a revert reason, so | ||
| // ask the broker whether the ticket was actually consumed rather than assuming | ||
| // it was. Since livepeer/protocol#657 a redemption that reverts leaves the | ||
| // ticket unused and still redeemable, and dropping it here would forfeit it. | ||
| if used, usedErr := sm.broker.IsUsedTicket(ticket.Ticket); usedErr == nil && !used { | ||
| return tx, unconsumedRedemptionErr{err} | ||
| } |
|
Closing: the framing here was wrong. go-livepeer already retries an underfunded sender — The one genuine defect (a reverted redemption being dropped locally) plus the efficiency items are tracked in #4013. |
Context
livepeer/protocol#657 patched the
TicketBrokergriefing issue disclosed via Immunefi.redeemWinningTicketnow requires the sender'sdeposit + reserveto cover the full ticket face value; below that it reverts withsender deposit and reserve insufficient to cover ticket face valueand leaves the ticket unused. Previously the ticket was consumed first and the transcoder could be paid dust.This is the go-livepeer side of Sidestream's follow-up: "we recommend updating offchain implementation to retry failed tickets in the next rounds" and "double-check that no known offchain implementation relies on this specific error message."
On the second point: nothing does. No go-livepeer code matches the old
"sender deposit and reserve are zero"string. The lookalike atcmd/livepeer_cli/wizard_ticketbroker.go:36is a locally-generated CLI status, not a revert match.What was actually wrong
Redemption is driven by
ticketQueue.handleBlockEvent, which fires on every new L1 block (~12s) and, on a retryable failure, leaves the ticket queued for the next one. So retries already existed. Three things were wrong around them:No client-side face-value precondition. go-livepeer kept building redemptions that cannot succeed. On the default config (
-gasLimit0) these die inEstimateGas, so they cost RPC round-trips rather than gas — but they cost them on every block, for as long as the sender is underfunded.The head of the queue was re-attempted N times per block. Each loop iteration re-selects the earliest unredeemed ticket, so a retryable failure re-selects the same ticket on every remaining iteration — N attempts per block, where N is that sender's queue length.
A reverted redemption dropped the ticket.
CheckTx(eth/client.go) returns only"transaction failed txHash=..."with no revert reason, andisNonRetryableTicketErrmatched that string to mark the ticket redeemed locally. That was correct before the patch — the ticket really was consumed. It is now a regression: a redemption that reverts leaves the ticket redeemable on-chain, and go-livepeer would forfeit it. Reachable when-gasLimitis set explicitly (skippingEstimateGas) or when the sender is drained between estimate and mining — the exact race the patch was written for.The fix
Gate before the RPCs, don't back off.
availableFundsis already served from the sender watcher's in-memory cache, which the block watcher keeps current fromDepositFunded/ReserveFunded. So the check costs abig.Intcompare, and the existing 12s cadence becomes the mechanism rather than the problem:Exponential backoff would have optimized attempt count — the wrong axis once the failure is free — while risking sleeping through the top-up or the ticket's expiry.
Supporting changes: stop the loop instead of re-selecting a blocked head-of-queue ticket; log the underfunded case at
V(5)rather thanErrorf, since at one line per ticket per block it would otherwise flood the log; and consultIsUsedTicketbefore marking a ticket redeemed after a failedCheckTx, so the contract decides whether a ticket is spent rather than an opaque error string.Net per sender per block: one comparison and one V-line, versus N × (2 RPCs + an error line).
Tests
errInsufficientSenderFundsis returned before any RPC — asserted by makingIsUsedTicketandSuggestGasPricefail distinguishably and observing that neither error surfacesisNonRetryableTicketErr, including wrapped errorsAll three behavioural changes were mutation-tested — reverting each one individually makes the corresponding test fail.
go test ./pm/... ./server/ ./eth/...passes; the new tests also pass under-race.Note for reviewers
The
pmpackage has pre-existing-racefailures onmaster(TestWatchPoolSizeChange,TestTicketQueueLoop,TestTicketQueueLoop_IsNonRetryableTicketErr_MarkAsRedeemed) caused by non-race-safe test stubs. Untouched here; flagging so they aren't mistaken for regressions.🤖 Generated with Claude Code