Fix uint64 underflow in sent amount calculation for commitment txs#172
Fix uint64 underflow in sent amount calculation for commitment txs#172aruokhai wants to merge 6 commits into
Conversation
When computing the net sent amount, the previous code subtracted vtxosToAdd directly from myVtxos without a bounds check, causing a uint64 underflow if outputs exceeded inputs — e.g. when boarding and spending vtxos occur in the same commitment tx. Include pendingBoardingTxs in totalIn and guard the subtraction. Closes #170 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughIn ChangesTxSent Underflow Fix
🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![arkanaai[bot]](https://avatars.githubusercontent.com/u/170820085?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔴 Protocol-Critical Review — Requesting Changes
This PR touches VTXO amount accounting and transaction record creation. Requires human sign-off before merge.
✅ The fix itself is correct
The underflow diagnosis is spot-on. The old code in handleCommitmentTx did:
amount -= v.Amount // uint64 subtraction with no guardWhen boarding inputs contribute to vtxosToAdd but aren't counted in myVtxos, the subtraction wraps to ~2^64. The fix correctly:
- Adds
pendingBoardingTxsamounts tototalIn - Accumulates
vtxosToAddinto a separatetotalOut - Guards with
if totalIn > totalOutbefore subtracting
Clean, minimal, easy to reason about. Good.
🔴 P0 — Same underflow exists in handleArkTx (client.go:1540-1544)
// client.go:1530-1544 (master)
inAmount := uint64(0)
for _, vtxo := range myVtxos {
inAmount += vtxo.Amount
}
outAmount := uint64(0)
for _, vtxo := range vtxosToAdd {
outAmount += vtxo.Amount
}
txsToAdd = append(txsToAdd, clientTypes.Transaction{
...
Amount: inAmount - outAmount, // ← SAME uint64 underflow, NO guard at all
Type: clientTypes.TxSent,This is worse than the commitment tx case — there isn't even an if amount > 0 pseudo-guard. If a user receives a payment in the same ark tx where they spend vtxos, and outAmount > inAmount, this silently records a bogus TxSent with a wrapped amount.
Please fix handleArkTx in this same PR. Same pattern: guard with if inAmount > outAmount before subtracting.
🟡 P1 — No test coverage
This is a money-accounting bug. It needs at least a table-driven unit test covering:
- Pure send (totalIn > totalOut) → correct TxSent amount
- Boarding + vtxo spend in same round (the original bug scenario)
- totalIn == totalOut → no TxSent recorded
- totalIn < totalOut → no TxSent recorded, no underflow
handleCommitmentTx and handleArkTx are internal methods that take concrete types — they should be testable via client_internal_test.go.
🟢 Minor observations (non-blocking)
totalIn < totalOutcase silently drops: When boarding + vtxo inputs < vtxo outputs, no transaction record is created at all. This is pre-existing behavior and not a regression, but worth a// TODOcomment noting that a TxReceived might be appropriate here.- No cross-repo API breakage: The change is internal to
handleCommitmentTx, no public API or type changes. No downstream SDK impact.
Summary
| Finding | Severity | Status |
|---|---|---|
Underflow fix in handleCommitmentTx |
— | ✅ Correct |
Same underflow in handleArkTx:1544 |
P0 | 🔴 Must fix |
| Missing test coverage | P1 | 🟡 Should add |
| Silent drop when totalIn < totalOut | P2 | 🟢 Pre-existing, note it |
- handleArkTx had the same uint64 underflow as handleCommitmentTx: no guard before inAmount - outAmount, so a receive-in-same-tx would wrap to a huge bogus TxSent amount. - Extracted committedSentAmount and arkSentAmount as pure functions so the guard logic is unit-testable without store mocking. - Added table-driven tests covering pure send, boarding+vtxo in same round, totalIn==totalOut, and totalIn<totalOut for both helpers. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Previously the spent-vtxos branch of handleCommitmentTx/handleArkTx only emitted a TxSent when our inputs exceeded our outputs; when a tx both spent one of our vtxos and credited us with more than we put in (e.g. a payment to us in the same round, a collaborative tx), it recorded nothing — leaving the balance change invisible in history. committedNetAmount/arkNetAmount now return a direction: TxSent when inputs win, TxReceived when outputs win, skip only on exact equality. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
✅ Follow-up Review — All Previous Findings Addressed
Previous review requested changes for 3 findings. All resolved in commits b71e494 and 528b632.
P0 ✅ handleArkTx underflow — Fixed
arkNetAmount (client.go:2122) now guards the subtraction identically to committedNetAmount. The raw inAmount - outAmount is gone.
P1 ✅ Test coverage — Added
Table-driven tests in client_internal_test.go:10-117 cover:
- Pure send (in > out) → correct
TxSentamount - Boarding + vtxo in same round → boarding counted as input, no underflow
- Net receive (out > in) →
TxReceived, no underflow - Equal in/out →
ok=false, nothing recorded
P2 ✅ Silent drop on net-receive — Now records TxReceived
netAmount (client.go:2134) returns TxReceived when totalOut > totalIn instead of dropping. This closes the accounting gap I flagged.
Code quality
netAmounthelper is clean: switch on three cases, no arithmetic without a guard, no possible underflow.committedNetAmountcorrectly includesboardingTxsintotalIn.arkNetAmountcorrectly excludes boarding (boarding doesn't apply in the ark tx path).- All helpers are unexported — zero cross-repo API impact.
- Pre-existing boarding-only path (client.go:1388) is untouched and out of scope.
There was a problem hiding this comment.
✅ Follow-up Review — All Previous Findings Addressed
Previous review requested changes for 3 findings. All resolved in commits b71e494 and 528b632.
P0 ✅ handleArkTx underflow — Fixed
arkNetAmount (client.go:2122) now guards the subtraction identically to committedNetAmount. The raw inAmount - outAmount is gone.
P1 ✅ Test coverage — Added
Table-driven tests in client_internal_test.go:10-117 cover:
- Pure send (in > out) → correct
TxSentamount - Boarding + vtxo in same round → boarding counted as input, no underflow
- Net receive (out > in) →
TxReceived, no underflow - Equal in/out →
ok=false, nothing recorded
P2 ✅ Silent drop on net-receive — Now records TxReceived
netAmount (client.go:2134) returns TxReceived when totalOut > totalIn instead of dropping. This closes the accounting gap I flagged.
Code quality
netAmounthelper is clean: switch on three cases, no arithmetic without a guard, no possible underflow.committedNetAmountcorrectly includesboardingTxsintotalIn.arkNetAmountcorrectly excludes boarding (boarding doesn't apply in the ark tx path).- All helpers are unexported — zero cross-repo API impact.
- Pre-existing boarding-only path (client.go:1388) is untouched and out of scope.
There was a problem hiding this comment.
Follow-up Review — All Previous Findings Addressed
Previous review requested changes for 3 findings. All resolved in commits b71e494 and 528b632.
P0 - handleArkTx underflow — Fixed
arkNetAmount (client.go:2122) now guards the subtraction identically to committedNetAmount. The raw inAmount - outAmount is gone.
P1 - Test coverage — Added
Table-driven tests in client_internal_test.go:10-117 cover:
- Pure send (in > out) -> correct TxSent amount
- Boarding + vtxo in same round -> boarding counted as input, no underflow
- Net receive (out > in) -> TxReceived, no underflow
- Equal in/out -> ok=false, nothing recorded
P2 - Silent drop on net-receive — Now records TxReceived
netAmount (client.go:2134) returns TxReceived when totalOut > totalIn instead of dropping. This closes the accounting gap I flagged.
Code quality
- netAmount helper is clean: switch on three cases, no arithmetic without a guard, no possible underflow.
- committedNetAmount correctly includes boardingTxs in totalIn.
- arkNetAmount correctly excludes boarding (boarding does not apply in the ark tx path).
- All helpers are unexported — zero cross-repo API impact.
- Pre-existing boarding-only path (client.go:1388) is untouched and out of scope.
Protocol-critical (VTXO amount accounting) — requires human approval before merge.
…nt and ark transactions
There was a problem hiding this comment.
✅ Incremental Review — Commit ebbf64e
Pure rename refactor: committedNetAmount → commitmentTxNetAmount, arkNetAmount → arkTxNetAmount. Minor doc comment cleanup. No logic changes.
All call sites, definitions, and test invocations updated consistently. Approval stands.
…itmentTxNetAmount and TestArkNetAmount to TestArkTxNetAmount
There was a problem hiding this comment.
✅ Incremental Review — Commit 96d3fe9
Test function renames only: TestCommittedNetAmount → TestCommitmentTxNetAmount, TestArkNetAmount → TestArkTxNetAmount. Aligns test names with the function renames from ebbf64e. No logic changes.
Approval stands.
Hoist commitmentTxNetAmount out of the if-init and wrap arkTxNetAmount's signature so both stay under the 100-char limit. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
✅ Incremental Review — Commit 4bb2694
Formatting-only change: golines max-len 100 line wrapping on commitmentTxNetAmount call site and arkTxNetAmount function signature. No logic changes.
Approval stands.
1 participant
Summary
Fixes #170
handleCommitmentTxcomputes the net sent amount by subtractingvtxosToAdd(our change outputs) frommyVtxos(our spent inputs). Because both areuint64, ifvtxosToAdd > myVtxosthe result silently wraps to a huge number, recording a bogusTxSententry.pendingBoardingTxsamounts tototalInso boarding inputs are correctly counted alongside spent vtxos.totalIn > totalOutcheck before subtracting.Boarding scenario (root cause highlighted)
The invariant
myVtxos >= vtxosToAddassumed this branch always represents a pure send, but it breaks whenever boarding and a vtxo spend land in the same commitment tx:myVtxos— existing vtxos being spent (e.g. small)pendingBoardingTxs— on-chain boarding UTXOs being settled (potentially large)vtxosToAdd— all new vtxos issued to us, including the one created from the boarding inputSo
vtxosToAddeasily exceedsmyVtxosalone, causing the underflow. The fix counts boarding amounts as part oftotalInso the net calculation reflects the full picture.Test plan
TxSentamount is unchangedTxSentreflects correct net amounttotalIn <= totalOut, no spuriousTxSentrecorded🤖 Generated with Claude Code
Summary by CodeRabbit