wallet: attribute round operator fees to onchain withdrawal activity - #993
Conversation
In this commit, we fix the client-side operator fee reconciliation to count locally owned outputs from the BUILT VTXOs rather than the intent requests. Under the seal-time fee handshake, an intent's amount is the pre-fee target: the server's quote shaves the operator fee off the leaf at seal time, and the FSM intentionally no longer subtracts it client side. Summing intent amounts therefore cancels the round's inputs exactly, computeClientOperatorFee returns zero for every fee-charging round, and the FeePaidMsg emission is silently suppressed — no boarding_fee_paid or refresh_fee_paid row was ever landing in the fee ledger. The built ClientVTXO carries the sealed leaf value (the quote residual extracted by leafNonAnchorAmount), so input minus output over the sealed amounts yields the true fee. Foreign directed-send recipient slots never materialize as owned VTXOs, so their intent amount remains the only local record of their value and still counts as-is. A new regression test pins the shape observed on regtest: a leave round forfeiting 149,745 sats into a 10,000 sat leave plus a 139,204 sat sealed change VTXO must book the 541 sat operator fee, and a fully funded foreign recipient slot must leave that fee unchanged.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces database migration 15, which adds a round_uuid TEXT column to the ledger_entries table to mirror the raw 16-byte round_id BLOB. This allows joining ledger rows against round-adjacent tables in portable SQL, enabling the retrieval of settlement fees. The PR also updates the ledger logic to stamp this UUID on new inserts, includes a Go post-migration step to backfill existing rows, and adjusts the wallet history logic to handle and display these fees correctly. The review feedback suggests optimizing the settlement fee query in vtxo.sql using a correlated scalar subquery to avoid full table scans, defensively clamping the adjusted AmountSat in history.go to prevent sign-flipping, and adding a context cancellation check inside the backfill loop in post_migration_checks.go.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| rounds.confirmation_height AS settlement_height, | ||
| CAST(COALESCE(round_fees.fee_sat, 0) AS BIGINT) AS settlement_fee_sat | ||
| FROM vtxos | ||
| LEFT JOIN rounds ON vtxos.forfeit_round_id = rounds.round_id | ||
| LEFT JOIN ( | ||
| SELECT round_uuid, | ||
| CAST(SUM(amount_sat) AS BIGINT) AS fee_sat | ||
| FROM ledger_entries | ||
| WHERE round_uuid IS NOT NULL | ||
| AND event_type IN ('boarding_fee_paid', 'refresh_fee_paid') | ||
| GROUP BY round_uuid | ||
| ) AS round_fees ON round_fees.round_uuid = vtxos.forfeit_round_id |
There was a problem hiding this comment.
Using a LEFT JOIN with a grouped subquery on ledger_entries requires the database to scan, filter, and aggregate the entire ledger_entries table (or a large portion of it) before performing the join. As the ledger grows, this will become a significant performance bottleneck.
Since we only need the sum of fees for the specific forfeit_round_id of each returned VTXO, we can use a correlated scalar subquery instead. This allows the database planner to perform a highly efficient index lookup on idx_client_ledger_round_uuid for each returned row, avoiding the full table scan and aggregation.
rounds.confirmation_height AS settlement_height,
CAST(COALESCE(
(SELECT SUM(amount_sat)
FROM ledger_entries
WHERE round_uuid = vtxos.forfeit_round_id
AND event_type IN ('boarding_fee_paid', 'refresh_fee_paid')
), 0) AS BIGINT) AS settlement_fee_sat
FROM vtxos
LEFT JOIN rounds ON vtxos.forfeit_round_id = rounds.round_idThere was a problem hiding this comment.
Taken — the fee lookup is now a correlated scalar subquery, and the migration's partial index was widened to a composite (round_uuid, event_type) so the seek fully covers the fee event-type filter with no residual step.
| sweepAll := entry.GetRequest().GetOnchainAddress().GetSweepAll() | ||
| if sweepAll && entry.AmountSat < 0 { | ||
| entry.AmountSat += settle.feeSat | ||
| } |
There was a problem hiding this comment.
To prevent any potential data inconsistency or extreme fee scenario from flipping the sign of entry.AmountSat (which represents an outflow and must remain negative or zero), we should defensively clamp the adjusted amount to at most 0.
sweepAll := entry.GetRequest().GetOnchainAddress().GetSweepAll()
if sweepAll && entry.AmountSat < 0 {
entry.AmountSat += settle.feeSat
if entry.AmountSat > 0 {
entry.AmountSat = 0
}
}There was a problem hiding this comment.
Added — both netting sites (applyCooperativeLeaveForfeited and the unilateral applyUnrollStatus path) now clamp the adjusted amount at zero so a pathological fee can never flip an outflow row's sign.
| for _, rawID := range roundIDs { | ||
| if len(rawID) != 16 { | ||
| continue | ||
| } |
There was a problem hiding this comment.
For long-running loops that perform database operations, it is a good practice to check if the context has been cancelled (ctx.Err()) at the start of each iteration to abort early and avoid unnecessary database writes.
| for _, rawID := range roundIDs { | |
| if len(rawID) != 16 { | |
| continue | |
| } | |
| for _, rawID := range roundIDs { | |
| if err := ctx.Err(); err != nil { | |
| return err | |
| } | |
| if len(rawID) != 16 { | |
| continue | |
| } |
There was a problem hiding this comment.
Added — the backfill loop now checks ctx.Err() per iteration and aborts early; the round_uuid IS NULL guard already makes the re-run a no-op.
![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: 69dede7b59
ℹ️ 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".
| // A zero settlement fee (old daemon, fee-free round) leaves any | ||
| // already-carried fee untouched rather than clobbering it. | ||
| if settle.feeSat > 0 { | ||
| entry.FeeSat = settle.feeSat |
There was a problem hiding this comment.
When two cooperative on-chain sends are queued before TriggerRoundRegistration, PendingRoundAssembly.processEvent appends both leave requests into the same round, and the daemon reports the same round-level FeeSat on every forfeited VTXO from that round. Copying that total directly onto each pending EXIT means every send completed from the same round displays the full round fee (and sweep-all rows subtract the full fee from each amount), overstating fees and understating delivered amounts. This needs a per-leave allocation or a guard that only stamps the total when the round has a single leave row.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed real, and deliberately out of scope for this series: per-operation attribution needs a signal the settlement doesn't carry today (a per-leave outflow readback or a leave count on the settlement). Tracked as #994 together with the two sibling multi-op corners (mixed-round fee typing, change-less directed sends). The whole distortion is bounded by one round's fee.
In this commit, we move the boarding fee leg's credit side from vtxo_balance to wallet_balance so the chart of accounts nets correctly against the amounts the round actually books. The boarding vtxo_received leg carries the SEALED (post-fee) VTXO value under the seal-time fee handshake, which means vtxo_balance already lands on the true VTXO holding without any fee adjustment. Crediting the fee from vtxo_balance on top of that would understate the VTXO layer by the fee; crediting it from wallet_balance instead completes the gross wallet outflow — deposit in, sealed value plus fee out — leaving every account at its true balance. Refresh fees keep their vtxo_balance credit: a refresh fee is carved out of forfeited VTXO value, and the paired gross send/receive legs cancel, so the fee leg is the only real movement there. The fee_ledger doc's boarding and refresh walkthroughs are updated to describe the sealed-amount legs and the per-flow credit accounts.
5729f6a to
e760978
Compare
|
Ran an adversarial review pass over the branch; the series is updated with the findings addressed (fixups squashed in place, one new commit at the tip). Fixed:
Accepted with follow-up (#994): per-operation fee attribution when one round composes multiple client ops — whole-round fee netting on concurrent sweep rows, the boarding-typed fee credit for mixed rounds, and the possible fee suppression for change-less directed sends where the quote residual lands in a foreign leaf. All bounded by one round's fee. |
|
@claude review this |
|
Claude finished @Roasbeef's task in 2m 48s —— View job Review — round operator fee attribution (#993)I read the full diff across the three layers (fee emission, joinability, wallet surface) plus the ledger idempotency and index invariants. Overall this is a careful, well-documented series: the root-cause diagnosis (intent amounts cancelling to a zero fee under the seal-time handshake, and the BLOB/TEXT join gap) is convincing, the accounting rationale for moving the boarding fee credit to Findings below, grouped by confidence. The prior-reviewer items are triaged rather than repeated. Correctness1. Round-level fee stamped per-leave (multi-op rounds) — real, already accepted. 2. Live-upgrade sweep-all rows can double-count the fee. Defensive / robustness3. Netting can flip the outflow sign. entry.AmountSat += settle.feeSat
if entry.AmountSat > 0 {
entry.AmountSat = 0
}4. Performance5. Settlement-fee subquery scans the full ledger. Things I checked that are fine
None of the above are blockers; the two I'd prioritize are the clamp (#3, trivial) and confirming the #994 follow-up scope covers #2's upgrade window. Nice work on the race analysis. |
In this commit, we add migration 000015, which grows ledger_entries a round_uuid TEXT column mirroring the raw 16-byte round_id BLOB in the canonical lowercase UUID form that rounds.round_id and vtxos.forfeit_round_id already store. The two subsystems historically persisted the same identifier in different encodings, and no BLOB to TEXT conversion exists in the SQL dialect subset shared by SQLite and Postgres (hex() vs encode()), so nothing could join ledger rows against the round-adjacent tables in plain SQL. The TEXT mirror closes that gap once, for every present and future join. New inserts stamp the column in the LedgerStoreDB adapter via roundUUIDText, so no ledger actor or message change is needed. Existing rows are converted by a Go post-migration step registered for version 15 — the string formatting is the part SQL cannot express portably — wired into both store constructors through the previously dormant makePostStepCallbacks machinery. The per-round backfill UPDATE guards on round_uuid IS NULL, so a crash-interrupted run re-executes as a no-op. The generated sqlc code for the accounting queries is regenerated alongside: the insert gains the new column, the ledger list queries return it, and the two backfill helper queries land.
In this commit, we extend the ListVTXOsByStatus settlement join with the forfeit round's operator fee. The query already LEFT JOINs the round that forfeited each VTXO to surface the settling commitment txid and confirmation height; a second LEFT JOIN against a fee-totals subquery (SUM of boarding_fee_paid and refresh_fee_paid grouped by round_uuid, keyed on forfeit_round_id) now rides along in the same single query, so the read costs no extra roundtrip. The figure is round-level: every VTXO forfeited in the same round reports the same total, so consumers must attribute it once, never sum it across VTXOs. It reads zero for fee-free rounds and for ledger rows predating the round_uuid backfill. vtxo.Settlement grows a FeeSat field populated by the by-status row converter on both the full and light read paths, and the settlement store test now proves the sum covers exactly the settling round's fee event types — the vtxo_sent row in the same round and a fee booked against an unrelated round contribute nothing.
In this commit, we add fee_sat to the VTXOSettlement message and populate it in descriptorToProto from the settlement the store join now carries. A FORFEITED VTXO's settlement thereby reports where the forfeit round confirmed AND what the round cost, giving wallet-layer consumers a single carrier for completing a cooperative-leave row with its on-chain coordinates and fee. The field mirrors the store contract: a round-level total repeated on every VTXO the round forfeited, zero against fee-free rounds and ledgers predating fee-by-round attribution.
In this commit, we complete the fee plumbing the activity surface was missing: when a cooperative-leave EXIT row completes off its forfeited source VTXO, the settlement now carries the forfeit round's operator fee, and applyCooperativeLeaveForfeited stamps it onto the row's fee_sat. The projection pass then persists it into the canonical activity store, so both the live table and the durable row agree. Sweep-all sends need one more step. A sweep's pending amount is the gross drained balance with the fee still baked in — the binding fee is unknown until the round seals — so displaying a fee next to that gross amount would double-count it. The onchain request now records a sweep_all marker (set by leaveEntryStub from the prepared intent), and completion nets the settled fee back out of the amount. Every completed EXIT thus reads the same way: amount is the value delivered to the destination, fee is the cost on top, and their sum is the true outflow. A bounded send's amount is already the exact destination value, so it is left untouched. Verified end-to-end on an arktest regtest topology: a 10,000 sat bounded send completes as amount -10,000 / fee 541, and a sweep-all of a 139,204 sat wallet completes as amount -138,816 / fee 388, with the destination wallet receiving exactly those amounts on chain.
In this commit, we keep the newly emitted boarding_fee_paid ledger rows out of the wallet activity view. The daemon's unified history typed them 'boarding' all along, but the mapping was dead code while the round actor's fee computation suppressed every fee row; with fee emission fixed, each boarding round's fee leg surfaced as a phantom PENDING DEPOSIT row for the fee amount (observed as a 255 sat 'ledger-N' deposit on regtest). Only the wallet_utxo_created subtype is a user-facing deposit. The fee already reaches the user through the real deposit row's fee_sat attribution, so the accounting leg is classified out of the wallet surface entirely.
In this commit, we bring the per-package agent docs in line with the fee-attribution changes: the db package map documents migration 000015 and the round_uuid mirror column, the ledger doc describes the per-flow fee credit accounts and the joinable round linkage, and the swapwallet doc records the cooperative-leave EXIT fee stamping and sweep-all amount netting invariant.
In this commit, we add a read path for the unilateral exit cost the ledger already records: handleExitCost books an onchain_fee_paid leg after the final sweep confirms, keyed by the exited VTXO's outpoint-derived idempotency key. ExitIdempotencyKey exports that key derivation, and LedgerStoreDB.GetConfirmedExitCost sums the fee legs under it — the partial unique index scopes on (key, event_type, accounts), so the send leg sharing the same key never contributes. An exit that has not confirmed (or predates exit-cost accounting) reads zero.
In this commit, we add exit_cost_sat to GetUnrollStatusResponse and stamp it in the daemon on both status paths (live registry and the persisted-job fallback) whenever a job reads COMPLETED. Unlike the estimate breakdown a detailed probe projects via enrichExitFees, this is the settled figure from the ledger's confirmed onchain_fee_paid exit leg, so it needs no fee-rate estimation or lineage resolution and is cheap enough for the plain per-row status lookups the activity surface issues. The stamp is best-effort: a missing ledger store or a read failure logs at debug and never fails the status query.
In this commit, we close the remaining FEE 0 gap on the activity surface: a completed unilateral exit now carries the settled exit cost the daemon reports on GetUnrollStatus. applyUnrollStatus stamps it onto the row's fee_sat and nets it back out of the gross VTXO amount, so a unilateral EXIT reads the same way as a completed cooperative leave: amount is the value delivered on chain, fee is the cost on top, and their sum is the gross VTXO value that left Ark custody. A zero cost (old daemon, or an exit predating exit-cost accounting) leaves the row exactly as before, preserving prior behavior for historical exits.
In this commit, we harden the EXIT completion paths against the ordering gap between the status that completes a row and the ledger commit that carries its fee. The forfeit status (VTXO actor) and the round's FeePaidMsg (durable ledger actor) are independent fire-and-forget Tells from the same round-actor turn, and the unroll path likewise only guarantees the ExitCostMsg is enqueued before the terminal handoff. A reconcile pass landing in that window would complete the row at fee 0, durably project it, and drop the pending record — freezing the exact fee-0 symptom this series eliminates. Two guards close the window. First, a COMPLETE projection carrying a zero fee retains its pending record for a bounded number of passes (feeZeroClearGracePasses) before clearing, so the row stays derivable long enough for a fee that commits milliseconds later to be re-read and re-projected; genuinely fee-free rounds pay only a few redundant decorations. Second, mergeActivityContext treats a recorded fee as sticky: no producer legitimately moves a settled fee back to zero, so a later fee-0 projection (racing pass, transient ledger read error) restores the stored fee together with its coupled netted amount instead of regressing the row to gross/fee-0. The reconciler scenario test drives the race end to end: pass one completes at fee 0 and retains the record, pass two observes the late-committed fee, heals the stored row, and clears.
e760978 to
6b7bff8
Compare
|
Addressed the consolidated review (fixups squashed in place, series shape unchanged):
|
1 participant
In this PR, we fix #988: onchain withdrawals (both bounded sends and
--send-allsweeps) now display the operator fee they actually paid, and a sweep's amount is netted so every completed EXIT row reads the same way: amount is the value delivered to the destination, fee is the cost on top, and their sum is the true outflow.The investigation turned up two distinct gaps rather than the single display-plumbing gap the issue suggested. First, the fee ledger itself was silently missing every fee row: under the seal-time fee handshake, an intent's amount is the pre-fee target while the sealed leaf carries the server's quote residual, so
computeClientOperatorFee(which summed intent amounts) computed exactly zero for every fee-charging round and theFeePaidMsgemission never fired. Noboarding_fee_paidorrefresh_fee_paidrow ever landed on disk. Second, even with the rows present, nothing could join them back to the wallet surface: the ledger storesround_idas a raw 16-byte BLOB whilerounds.round_idandvtxos.forfeit_round_idstore the TEXT UUID, and the BLOB/TEXT conversion isn't expressible in the SQL dialect subset shared by SQLite and Postgres.Fee emission
We fix the fee computation to count locally owned outputs from the built VTXOs (the sealed leaf values) rather than the intent requests, while foreign directed-send recipient slots keep their intent amounts. With the fee legs flowing again, the boarding fee's credit side moves from
vtxo_balancetowallet_balance: the boardingvtxo_receivedleg books the sealed (post-fee) VTXO value, so the fee completes the gross wallet outflow and every account nets to its true balance. Refresh fees keep theirvtxo_balancecredit, since those are carved out of forfeited VTXO value.Joinability
Migration 000015 adds a
round_uuidTEXT column toledger_entries, mirroring the raw round id in the canonical form the round tables already use. New inserts stamp it in the db adapter; existing rows are backfilled by a Go post-migration step (the string conversion is the part SQL can't do portably), wired through the previously dormantmakePostStepCallbacksmachinery. With the mirror in place,ListVTXOsByStatusgrows a fee-totals join keyed onforfeit_round_id, so a forfeited VTXO's settlement carries the forfeit round's operator fee in the same single query that already surfaces the settling txid and height.Wallet surface
The settlement fee rides the existing completion path:
waverpc.VTXOSettlementgainsfee_sat, andapplyCooperativeLeaveForfeitedstamps it onto the EXIT row when the forfeited source VTXO completes the leave. A sweep-all row also nets the fee back out of its gross pending amount (the request now records asweep_allmarker, since the binding fee is unknown until the round seals). The newly emitted boarding fee legs are classified out of the activity view so they can't fabricate a phantom deposit row; the deposit's fee still surfaces via the existing per-round attribution.Verified end-to-end on an arktest regtest topology: boarding 150,000 sats books a 255 sat fee row, a 10,000 sat bounded send completes as amount -10,000 / fee 541, and a sweep of the remaining 139,204 sats completes as amount -138,816 / fee 388, with the destination wallet receiving exactly those amounts on chain and the ledger's
vtxo_balancenetting to zero after the drain.Unilateral exits
The same convention now covers unilateral exits. The ledger has always booked the confirmed exit cost (the
onchain_fee_paidleg unroll emits after the final sweep confirms, keyed by the exited outpoint), soGetUnrollStatusgains anexit_cost_satfield read straight from that leg — the settled figure, not the estimate breakdown the detailed probe projects — andapplyUnrollStatusstamps it onto the completed EXIT row, netting the gross VTXO amount down to the value delivered on chain.One deliberate non-goal: activity rows already persisted as COMPLETE before this change keep their zero fee. The stored sweep rows carry the fee baked into their amount with no marker to tell them apart from bounded sends, so retro-stamping would double-count; historical ledger rows do gain the
round_uuidbackfill, so their fees are queryable even though the old activity rows aren't rewritten. The same reasoning covers a sweep-all row that was still PENDING across the upgrade: its stub predates thesweep_allmarker and is indistinguishable from a bounded send, so it completes with the fee stamped but the gross amount un-netted (amount + fee overstates the outflow by the fee, for that one row).See each commit message for a detailed description w.r.t the incremental changes.
Fixes #988.