pm: add diagnostics to insufficient sender reserve errors - #4003
Conversation
The same errInsufficientSenderReserve sentinel was returned from two unrelated checks in faceValue(): faceValue below the orchestrator's ticketEV (sender reserve genuinely too low) and faceValue unable to cover the ticket redemption tx cost (usually a too low -maxFaceValue). Operators debugging the second case were misled into inspecting the sender's reserve when the fix was raising -maxFaceValue. Wrap the sentinel with the values involved (faceValue, ticketEV, max float, redemption cost at current and avg gas price) and name the knob responsible per site. errors.Is compatibility is preserved via %w. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughInsufficient sender reserve errors now include the failed validation and relevant numeric values while preserving ChangesSender reserve error reporting
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![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 `@pm/recipient.go`:
- Line 297: Update the insufficient-reserve classification in the byoc job
orchestrator consumer to use errors.Is against a shared exported sentinel or
helper for errInsufficientSenderReserve, rather than comparing err.Error() text.
Apply the change to the relevant classification logic and extend byoc job
orchestrator tests to verify wrapped insufficient-reserve errors still produce
the 503 response; preserve existing behavior for unrelated errors.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d508ad59-42a2-403f-93ed-fa18d724c476
📒 Files selected for processing (3)
CHANGELOG_PENDING.mdpm/recipient.gopm/recipient_test.go
| } | ||
| if faceValue.Cmp(r.cfg.EV) < 0 { | ||
| return nil, errInsufficientSenderReserve | ||
| return nil, fmt.Errorf("%w: faceValue %v is less than ticketEV %v (sender max float: %v); the sender's reserve is too low for this orchestrator's -ticketEV", errInsufficientSenderReserve, faceValue, r.cfg.EV, maxFloat) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo files =="
git ls-files | rg '(^|/)(recipient\.go|job_orchestrator\.go)$|\.go$' | sed -n '1,120p'
echo "== recipient.go relevant block =="
if [ -f pm/recipient.go ]; then
wc -l pm/recipient.go
sed -n '260,330p' pm/recipient.go
fi
echo "== job_orchestrator.go occurrences =="
if [ -f pm/byoc/job_orchestrator.go ]; then
wc -l pm/byoc/job_orchestrator.go
rg -n 'insufficient|Insufficient|503|400|errors\.Is|errInsufficientSenderReserve|SenderReserve' pm/byoc/job_orchestrator.go
else
fd -i 'job_orchestrator.go' . | sed -n '1,20p'
fiRepository: livepeer/go-livepeer
Length of output: 6313
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== all occurrences of errInsufficientSenderReserve and insufficient sender reserve =="
rg -n 'errInsufficientSenderReserve|insufficient sender reserve|InsufficientSenderReserve|SenderReserve|job_orchestrator' .
echo "== go module / package summary =="
sed -n '1,80p' pm/byoc/job_orchestrator.go 2>/dev/null || true
sed -n '1,80p' pm/recipient.go 2>/dev/null || trueRepository: livepeer/go-livepeer
Length of output: 4579
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== job_orchestrator.go relevant blocks =="
sed -n '130,180p' byoc/job_orchestrator.go
echo "== job_orchestrator_test.go relevant error case =="
sed -n '580,630p' byoc/job_orchestrator_test.go
rg -n 'if err\.Error\(\) == "insufficient sender reserve"|ErrorIs|insufficient sender reserve' byoc/job_orchestrator.go byoc/job_orchestrator_test.go pm/recipient.go pm/recipient_test.go | sed -n '1,120p'Repository: livepeer/go-livepeer
Length of output: 4623
Preserve the downstream insufficient-reserve status contract.
pm/recipient.go now wraps errInsufficientSenderReserve, so err.Error() == "insufficient sender reserve" no longer matches these production errors. byoc/job_orchestrator.go:159 therefore classifies 503 reserve failures as 400. Update this consumer to use errors.Is with an exported/shared sentinel or helper, and update byoc/job_orchestrator_test.go to exercise the wrapped error.
Also applies to lines 297 and 313 in pm/recipient.go.
🤖 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 `@pm/recipient.go` at line 297, Update the insufficient-reserve classification
in the byoc job orchestrator consumer to use errors.Is against a shared exported
sentinel or helper for errInsufficientSenderReserve, rather than comparing
err.Error() text. Apply the change to the relevant classification logic and
extend byoc job orchestrator tests to verify wrapped insufficient-reserve errors
still produce the 503 response; preserve existing behavior for unrelated errors.
There was a problem hiding this comment.
Pull request overview
This PR improves operator diagnostics for insufficient sender reserve failures in PM ticket parameter generation by wrapping the sentinel error with contextual details so operators can identify which check failed and why.
Changes:
- Add contextual
fmt.Errorf("%w: ...")wrapping for the twofaceValue()insufficient-reserve branches. - Update tests to assert via
ErrorIsand useErrorContainsto target the intended failure branch. - Add a
CHANGELOG_PENDING.mdentry describing the improved diagnostics.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
pm/recipient.go |
Wrap errInsufficientSenderReserve with detailed diagnostics for the EV-check and tx-cost-check branches. |
pm/recipient_test.go |
Update assertions to validate wrapping (ErrorIs) and branch selection (ErrorContains). |
CHANGELOG_PENDING.md |
Document the improved error diagnostics for orchestrator operators. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if faceValue.Cmp(r.cfg.EV) < 0 { | ||
| return nil, errInsufficientSenderReserve | ||
| return nil, fmt.Errorf("%w: faceValue %v is less than ticketEV %v (sender max float: %v); the sender's reserve is too low for this orchestrator's -ticketEV", errInsufficientSenderReserve, faceValue, r.cfg.EV, maxFloat) | ||
| } |
| if faceValue.Cmp(txCost) < 0 && faceValue.Cmp(r.txCostWithGasPrice(avgGasPrice)) < 0 { | ||
| return nil, errInsufficientSenderReserve | ||
| return nil, fmt.Errorf("%w: faceValue %v cannot cover the ticket redemption tx cost (at current gas price: %v, at avg gas price: %v); increase -maxFaceValue or the sender's reserve", errInsufficientSenderReserve, faceValue, txCost, r.txCostWithGasPrice(avgGasPrice)) | ||
| } |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4003 +/- ##
===================================================
- Coverage 35.53014% 35.50354% -0.02660%
===================================================
Files 174 174
Lines 45111 45111
===================================================
- Hits 16028 16016 -12
- Misses 27814 27820 +6
- Partials 1269 1275 +6
... and 7 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
What
faceValue()inpm/recipient.goreturns the same bareinsufficient sender reserveerror from two unrelated checks:faceValue < ticketEVafter clamping to the sender's max float: the sender's reserve really is too low for the orchestrator's-ticketEV.faceValue < txCost(current gas)andfaceValue < txCost(avg gas): the ticket could never pay for its own redemption. This is typically caused by a too low-maxFaceValue(on Arbitrum the floor isredeemGas 1.2M x 3 gwei hardcoded avg = 3.6e15 wei) and fires no matter how large the sender's reserve is.Debugging case 2 with the current message points operators at the sender's reserve, which is a dead end. I lost a debugging session to exactly this: an orchestrator with
-maxFaceValue=1e13can never pass check 2, but the error suggested funding the gateway's reserve.Changes
errInsufficientSenderReservewith%wat both sites, including the values involved (faceValue, ticketEV, sender max float, redemption cost at current and avg gas price) and naming the knob responsible per site.assert.EqualErrortoassert.ErrorIs(plusErrorContainsto pin the branch where the test targets a specific check).errors.Iscompatibility is preserved, so no behavior change for callers.Example messages
Before (both cases):
insufficient sender reserveAfter:
insufficient sender reserve: faceValue 999999999 is less than ticketEV 1000000000 (sender max float: 999999999); the sender's reserve is too low for this orchestrator's -ticketEVinsufficient sender reserve: faceValue 10000000000000 cannot cover the ticket redemption tx cost (at current gas price: 12000000000000, at avg gas price: 3600000000000000); increase -maxFaceValue or the sender's reserve🤖 Generated with Claude Code
Summary by CodeRabbit