unroll: durable per-target unroll subsystem (4/5) - #263
Conversation
![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 implements a new unilateral-exit (unroll) subsystem using a durable actor pattern and a finite state machine to manage on-chain recovery. It introduces new RPCs, database migrations for job tracking, and integration with the VTXO manager. Feedback includes correcting the GetBalance logic to accumulate balances across multiple wallet backends instead of overwriting them. Recommendations were also made to enhance the robustness of the unroll actor by adding defensive checks for proof nodes, treating missing transactions during state reissues as errors to avoid stalls, and adopting non-blocking communication in the registry actor to prevent potential hangs.
| return | ||
| } | ||
|
|
||
| resp.OnchainWalletConfirmedSat = int64(walletBalance.Confirmed) |
There was a problem hiding this comment.
The OnchainWalletConfirmedSat field is overwritten by each wallet backend. If multiple backends are active (e.g., LND and a lightweight wallet), the balance will only reflect the last one processed. It should accumulate the balances instead.
| resp.OnchainWalletConfirmedSat = int64(walletBalance.Confirmed) | |
| resp.OnchainWalletConfirmedSat += int64(walletBalance.Confirmed) |
| return | ||
| } | ||
|
|
||
| resp.OnchainWalletConfirmedSat = int64(confirmed) |
There was a problem hiding this comment.
The balance should be accumulated to support multiple wallet backends correctly.
| resp.OnchainWalletConfirmedSat = int64(confirmed) | |
| resp.OnchainWalletConfirmedSat += int64(confirmed) |
| return | ||
| } | ||
|
|
||
| resp.OnchainWalletConfirmedSat = int64(confirmed) |
There was a problem hiding this comment.
The balance should be accumulated to support multiple wallet backends correctly.
| resp.OnchainWalletConfirmedSat = int64(confirmed) | |
| resp.OnchainWalletConfirmedSat += int64(confirmed) |
| } | ||
|
|
||
| pkScript := append( |
There was a problem hiding this comment.
If a proof node is missing during reissue, the actor will skip it and potentially get stuck in the AwaitingMaterialization state forever because the required confirmation will never arrive. It's safer to return an error here, similar to how EnsureReadyTransactions is handled.
if !ok {
return fmt.Errorf("proof node %s missing", txid)
}| continue | ||
| } | ||
|
|
There was a problem hiding this comment.
If the sweep transaction is missing during reissue while the FSM is in a sweep-related state, the actor will be stuck. This indicates a corrupted checkpoint or inconsistent state and should be treated as an error.
if b.sweepTx == nil {
return fmt.Errorf("sweep transaction missing")
}| resp, err := child.Ref().Ask( | ||
| ctx, &GetStateRequest{}, | ||
| ).Await(ctx).Unpack() |
There was a problem hiding this comment.
The registry actor performs a blocking Ask call to child actors. Since the registry runs on a single goroutine, if a child actor is unresponsive or deadlocked, the entire registry will hang, preventing status queries or new unroll requests for other VTXOs. Consider using a non-blocking approach or a shorter timeout with a retry mechanism.
![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: 29613fa181
ℹ️ 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".
| rpc Unroll (UnrollRequest) returns (UnrollResponse); | ||
|
|
||
| // GetUnrollStatus returns the current status of an unroll job for the |
There was a problem hiding this comment.
Declaring Unroll/GetUnrollStatus in the public service contract without server implementations makes both endpoints return Unimplemented at runtime (the daemon server only has the embedded UnimplementedDaemonServiceServer fallback, and there are no concrete RPCServer.Unroll or RPCServer.GetUnrollStatus methods in darepod). This advertises functionality clients can call but cannot use.
Useful? React with 👍 / 👎.
| if child, ok := r.active[req.Outpoint]; ok { | ||
| return fn.Ok[RegistryResp](&EnsureUnrollResp{ | ||
| ActorID: child.Ref().ID(), | ||
| Created: false, | ||
| }) | ||
| } |
There was a problem hiding this comment.
handleEnsure only checks r.active, so once a child is removed from active (for example after terminal notification) or while a snapshot is only in pending/store, a repeated EnsureUnrollRequest will spawn a new actor for the same outpoint instead of returning Created=false. That breaks outpoint-level deduplication and can retrigger recovery attempts while overwriting control-plane history for an already-tracked target.
Useful? React with 👍 / 👎.
88df42b to
269fb2b
Compare
29613fa to
b65ed0d
Compare
|
Claude finished @Roasbeef's task in 4m 14s —— View job PR #263 Review — unroll: durable per-target unroll subsystem (4/5)
Overall the architecture is clean and well thought out — the FSM-per-target actor, the thin registry over it, and the convergence of manual/automatic unroll paths on High — Correctness Bugs1. The three resp.OnchainWalletConfirmedSat += int64(walletBalance.Confirmed)
// ...
resp.OnchainWalletConfirmedSat += int64(confirmed)
// ...
resp.OnchainWalletConfirmedSat += int64(confirmed)2. case *ReissueInFlightTransactions:
for _, txid := range evt.Txids {
node, ok := b.proof.Node(txid)
if !ok {
continue // ← silent skip
}If a proof node isn't found during restart reissue, if !ok {
return fmt.Errorf("proof node %s missing on reissue", txid)
}3. case *ReissueSweepConfirmation:
if b.sweepTx == nil {
continue // ← actor stalls in AwaitingSweepConfirmation forever
}If the actor restores to if b.sweepTx == nil {
return fmt.Errorf(
"sweep tx missing in ReissueSweepConfirmation state",
)
}Medium — Correctness / Reliability4. After a child terminates and is removed from func (r *registryBehavior) handleEnsure(ctx context.Context,
req *EnsureUnrollRequest) fn.Result[RegistryResp] {
if child, ok := r.active[req.Outpoint]; ok {
return fn.Ok[RegistryResp](&EnsureUnrollResp{
ActorID: child.Ref().ID(), Created: false,
})
}
// Also deduplicate against pending (terminal or not).
if record, ok := r.pending[req.Outpoint]; ok {
return fn.Ok[RegistryResp](&EnsureUnrollResp{
ActorID: record.ActorID, Created: false,
})
}
// Check the durable store for previously persisted terminal jobs.
existing, err := r.cfg.Store.GetRecord(ctx, req.Outpoint)
if err == nil && existing != nil {
return fn.Ok[RegistryResp](&EnsureUnrollResp{
ActorID: existing.ActorID, Created: false,
})
}
// ...
}5.
The fix is to handle Low / Polish6.
7. The proto declares both RPCs, but no 8. The proto comment on 9. inFlight, ok := r.persisting[req.Outpoint]
if ok && sameRegistryRecord(inFlight, record) {
return fn.Ok[RegistryResp](&RegistryAckResp{}) // ← branch A
}
if ok {
return fn.Ok[RegistryResp](&RegistryAckResp{}) // ← branch B
}Branch A's condition is a strict subset of branch B. Both return the same value. The two can be collapsed to a single 10. ConfirmationPkScript: append(
[]byte(nil), node.Tx.TxOut[0].PkScript...,
),
11. The constant is conservative (actual tapscript CSV spend is ~120-150 vbytes), but there's no comment explaining its derivation. A brief note on how it was estimated (key path vs script path spend, witness stack composition) helps reviewers trust the constant and maintainers update it if the spend path changes. Confirmed / Agree with Prior Reviewers
|
ddc71a1 to
88cc739
Compare
Add ForceUnrollEvent so manual unroll requests route through the VTXO actor's FSM rather than bypassing it with direct DB writes. LiveState handles ForceUnrollEvent by transitioning to UnilateralExitState and emitting ExpiringNotification through the chain resolver seam, converging manual and automatic triggers. Add ForceUnrollRequest/Response to the VTXO manager admission types and TestLiveStateForceUnroll unit test.
Add Unroll RPC for triggering unilateral exit by outpoint and GetUnrollStatus RPC for querying job progress. Add UnrollJobStatus enum and request/response messages for both endpoints.
88cc739 to
e19f519
Compare
|
Claude finished @Roasbeef's task in 2m 47s —— View job PR #263 Review — unroll: durable per-target unroll subsystem (4/5)
The architecture is solid: FSM-per-target actor with TLV checkpointing, dedup registry, convergent manual/automatic exit paths, and safe-by-construction sweep tx reuse. The correctness issues flagged in previous reviews have largely been addressed in the current branch HEAD. One high-severity bug remains open, plus a few low/polish items. Status of Previous Review FindingsSeveral issues flagged by Gemini, Codex, and the earlier Claude review have been resolved in the current code:
High — Remaining Correctness Bug
All three // Line 274
resp.OnchainWalletConfirmedSat = int64(walletBalance.Confirmed)
// Line 285
resp.OnchainWalletConfirmedSat = int64(confirmed)
// Line 296
resp.OnchainWalletConfirmedSat = int64(confirmed)In any configuration with more than one active wallet backend, only the last non-erroring one's balance is reported. Should be Low / Polish1. resp.TotalConfirmedSat = resp.BoardingConfirmedSat +
resp.VtxoBalanceSatThe proto field name implies a total confirmed balance, but 2. The constant is intentionally conservative (actual tapscript CSV timeout-path spend is ~120-150 vbytes with a 2-item witness stack). A brief comment noting the spend-path composition (input: 1 × P2TR tapscript, witness: sig + script + control block; output: 1 × P2TR) would help maintainers validate the number if the spend path changes. 3. The guard 4. Acknowledged in the PR description as intentional (wiring lands in 5/5). Worth noting for the release gate: if 5/5 is delayed, any binary that ships this proto will advertise two RPCs that return Confirmed Good
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e19f519977
ℹ️ 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".
| func (UnimplementedDaemonServiceServer) Unroll(context.Context, *UnrollRequest) (*UnrollResponse, error) { | ||
| return nil, status.Errorf(codes.Unimplemented, "method Unroll not implemented") | ||
| } | ||
| func (UnimplementedDaemonServiceServer) GetUnrollStatus(context.Context, *GetUnrollStatusRequest) (*GetUnrollStatusResponse, error) { | ||
| return nil, status.Errorf(codes.Unimplemented, "method GetUnrollStatus not implemented") |
There was a problem hiding this comment.
Unroll/GetUnrollStatus are now part of the generated service surface, but the server implementation does not define these methods (the codebase only embeds UnimplementedDaemonServiceServer), so both RPCs currently return codes.Unimplemented at runtime. This makes the newly published API contract unusable for any client generated from this proto until concrete handlers are added.
Useful? React with 👍 / 👎.
| r.pending[req.Outpoint] = cloneRegistryRecord(record) | ||
| r.requestPersist(req.Outpoint, 0) | ||
|
|
||
| return fn.Ok[RegistryResp](&EnsureUnrollResp{ |
There was a problem hiding this comment.
handleEnsure returns success immediately after enqueueing asynchronous persistence, which creates a crash window where a child actor has started but no control-plane record is durable yet. On restart, RestoreNonTerminal only reads Store.ListNonTerminalRecords, so jobs created in that window are skipped and won’t be resumed automatically.
Useful? React with 👍 / 👎.
e19f519 to
df5558f
Compare
Add onchain_wallet_confirmed_sat field to GetBalanceResponse and populate it from the backing wallet (LND, lwwallet, or btcwallet) so clients can see confirmed on-chain funds including sweep proceeds.
Add unilateral_exit_jobs migration, sqlc queries, and persistence store for manager-facing unroll job control-plane rows. Provides UpsertJob, GetJob, ListNonTerminalJobs, and MarkJobTerminal.
df5558f to
3efaf20
Compare
Close the client-side drift that let Board and SendVTXO pay the legacy flat terms.MinOperatorFee regardless of the operator's runtime fee schedule. Under a non-zero schedule the legacy flat fee either silently over-paid (when the dynamic quote was cheaper, stashing the excess as operator profit) or got rejected server-side with ErrOperatorFeeTooLow (when the dynamic quote was higher, blocking refresh / boarding entirely). - rpc_fees.quoteOperatorFee is a new private helper that calls the operator's EstimateFee RPC and returns the TotalFeeSat the server will accept. Back-compat: on a zero schedule EstimateFee returns zero and the flow reduces to the pre-fee path. - rpc_server.Board now asks the wallet for the confirmed boarding balance via the existing GetBoardingBalanceRequest, quotes the dynamic fee for that amount, and passes the quote through BoardRequest.MinOperatorFee in place of the legacy static value. If the quote fails the handler logs and falls back to terms.MinOperatorFee so boarding stays possible in a degraded operator connection. - rpc_server.SendVTXO quotes the dynamic fee for the summed recipient amount (isBoarding=false), with the same log-and-fall-back failure behavior. The wallet handlers (handleBoard, handleSendVTXOs) already consume the fee via the request message; no wallet code change is needed. Part of issue #263.
Add a substring-based mapper (mapFeeError) that recognizes the four load-bearing server-side fee rejection sentinels (ErrVTXOBelowMinViable, ErrOperatorFeeTooLow, boarding-too-small, EstimateFee-no-calculator) and rewrites them as terse actionable CLI output instead of a raw gRPC status. The original server message is kept as parenthetical context so operators can still grep the server logs when needed. The board and send-inround subcommands now run their daemon RPC error through mapFeeError before falling back to the generic wrap. Substring matching beats status-code matching because the daemon sanitizes upstream gRPC statuses via proxyUpstreamError, erasing the original error code. A focused test suite in fee_errors_test.go pins the exact substrings we depend on, so a future server-side message change fails the CLI test rather than silently routing around the mapper. Part of issue #263.
Five rapid-driven invariants covering the client-side fee
reconciliation helper that every round's FeePaidMsg emission
depends on:
1. Non-negative: the helper clamps below zero, so every draw
must yield fee >= 0. A regression that returned a negative
value would surface as a silently-dropped FeePaidMsg (the
ledger actor rejects negatives), making the bug hard to
trace.
2. Fee = inputs - outputs when outputs <= inputs. The ledger
emission path depends on this identity to reconcile with
the server-booked operator_fee.
3. Monotone in boarding: adding a positive boarding input must
never decrease the fee. A regression that double-counted or
dropped a boarding input would violate this.
4. Monotone in owned output: adding a positive owned output
must never increase the fee (symmetric to 3).
5. Empty-intents identity: zero intents produces a zero fee.
The baseline case that every round's first iteration
traverses.
The random-intent generator drawIntentsAndOwned caps per-entry
amounts at 1B sat so the running sums can't overflow int64 for
realistic slice lengths.
Part of issue #263.
…nt match) Three focused unit tests that lock in the client↔server fee round-trip contract from issue #263 Phase C.4: - round/refresh_psbt_size_test.go: asserts PSBT cardinality (forfeit count, owned-output count, leave count) stays constant across fee-enabled vs fee-disabled schedules; only per-output VALUES move. A rapid property extends this across random (forfeit amount, fee amount) draws and asserts computeClientOperatorFee returns exactly `forfeit - owned`. - darepod/congestion_roundtrip_test.go: a bufconn-backed fakeArkService returns canned EstimateFee replies, and the client's quoteOperatorFee helper is driven through a baseline → congestion-bumped sequence. The test asserts the helper returns the TotalFeeSat the operator reported verbatim (no local caching). A second case pins the nil-serverConn guard so the helper surfaces a clean Unavailable error instead of panicking. - darepod/rpc_fees_e2e_test.go: verifies the daemon's EstimateFee RPC proxies every field (liquidity, on-chain, margin, total, effective rate, min viable, below-dust-flag) verbatim to the CLI caller, and asserts the CLI-facing EstimateFee number matches the internal quoteOperatorFee number for the same inputs — the invariant that keeps the CLI confirmation prompt aligned with the actual booking. Part of issue #263.
Add a substring-based mapper (mapFeeError) that recognizes the four load-bearing server-side fee rejection sentinels (ErrVTXOBelowMinViable, ErrOperatorFeeTooLow, boarding-too-small, EstimateFee-no-calculator) and rewrites them as terse actionable CLI output instead of a raw gRPC status. The original server message is kept as parenthetical context so operators can still grep the server logs when needed. The board and send-inround subcommands now run their daemon RPC error through mapFeeError before falling back to the generic wrap. Substring matching beats status-code matching because the daemon sanitizes upstream gRPC statuses via proxyUpstreamError, erasing the original error code. A focused test suite in fee_errors_test.go pins the exact substrings we depend on, so a future server-side message change fails the CLI test rather than silently routing around the mapper. Part of issue #263.
Five rapid-driven invariants covering the client-side fee
reconciliation helper that every round's FeePaidMsg emission
depends on:
1. Non-negative: the helper clamps below zero, so every draw
must yield fee >= 0. A regression that returned a negative
value would surface as a silently-dropped FeePaidMsg (the
ledger actor rejects negatives), making the bug hard to
trace.
2. Fee = inputs - outputs when outputs <= inputs. The ledger
emission path depends on this identity to reconcile with
the server-booked operator_fee.
3. Monotone in boarding: adding a positive boarding input must
never decrease the fee. A regression that double-counted or
dropped a boarding input would violate this.
4. Monotone in owned output: adding a positive owned output
must never increase the fee (symmetric to 3).
5. Empty-intents identity: zero intents produces a zero fee.
The baseline case that every round's first iteration
traverses.
The random-intent generator drawIntentsAndOwned caps per-entry
amounts at 1B sat so the running sums can't overflow int64 for
realistic slice lengths.
Part of issue #263.
…nt match) Three focused unit tests that lock in the client↔server fee round-trip contract from issue #263 Phase C.4: - round/refresh_psbt_size_test.go: asserts PSBT cardinality (forfeit count, owned-output count, leave count) stays constant across fee-enabled vs fee-disabled schedules; only per-output VALUES move. A rapid property extends this across random (forfeit amount, fee amount) draws and asserts computeClientOperatorFee returns exactly `forfeit - owned`. - darepod/congestion_roundtrip_test.go: a bufconn-backed fakeArkService returns canned EstimateFee replies, and the client's quoteOperatorFee helper is driven through a baseline → congestion-bumped sequence. The test asserts the helper returns the TotalFeeSat the operator reported verbatim (no local caching). A second case pins the nil-serverConn guard so the helper surfaces a clean Unavailable error instead of panicking. - darepod/rpc_fees_e2e_test.go: verifies the daemon's EstimateFee RPC proxies every field (liquidity, on-chain, margin, total, effective rate, min viable, below-dust-flag) verbatim to the CLI caller, and asserts the CLI-facing EstimateFee number matches the internal quoteOperatorFee number for the same inputs — the invariant that keeps the CLI confirmation prompt aligned with the actual booking. Part of issue #263.
Per-target durable actor managing the full unilateral-exit lifecycle: proof assembly, transaction materialization via txconfirm, CSV maturity wait, sweep construction and confirmation. Features: - UnrollRegistryActor: thin registry, dedup by outpoint, boot restore - VTXOUnrollActor: durable FSM per target, delegates to txconfirm - Sweep retry (up to 3 attempts before terminal failure) - Spend watch on target outpoint for early external-spend detection - Proof assembly from VTXO descriptors and OOR artifacts - Checkpoint persistence after every FSM transition Key types: UnrollRegistryActor, VTXOUnrollActor, LocalProofAssembler.
Add unroll/CLAUDE.md documenting the durable per-target unroll subsystem alongside the registry, FSM events, persist-before-broadcast contract, TLV-encoded durable messages, and safe TxOut indexing. Update vtxo, lib/actormsg, and db docs to reflect the ForceUnrollEvent handling in SpendingState / ForfeitingState, the handleForceUnroll Tell→Ask change and new ForceUnrollResponse.Reason field, and the unilateral_exit_jobs migration landed by this series.
3efaf20 to
cb6e1b3
Compare
Close the gap where every integration test ran against a zero fee schedule, leaving the fee-aware code paths in rounds, sweeps, OOR, refresh, and the classifier uncovered at integration level. The forcing function inverts today's default: the itest harness and systest E2EHarness now install harness.DefaultItestFeeSchedule() (5% annual rate, 100 sat margin, 30% dust-ratio cap, 10-block refresh floor) before arkd starts, and opt-out helpers (WithZeroFeeSchedule on itest, DisableFees on systest) preserve the pre-#263 behaviour for one focused regression test. Every existing itest that asserts a specific VTXO balance now runs under a non-zero schedule and must audit the expected post-fee amount accordingly (B.2/B.3 in issue #263). Additional scaffolding: - harness.RestartArkd mirrors RestartClientDaemon so the persistence regression test can stop/start arkd against the same data dir. - config.FeesConfig grows a MinRefreshDeltaBlocks field so the canonical schedule can set a sub-production refresh-fee floor; server_fees.scheduleFromConfig now copies the new field. Part of issue #263.
Close a round-trip gap in GetFeeSchedule / UpdateFeeSchedule: fees.Schedule carries a MinRefreshDeltaBlocks field (the d_min liquidity-fee floor) but the proto FeeScheduleParams omitted it, so admin callers could not read or write that field. Without it the restart-persistence regression in itest/fees_hotreload_test.go could not prove that the full schedule survives a daemon reboot. Regenerated via make rpc. The handler wiring in adminrpcserver.go is updated in a follow-up commit. Part of issue #263.
UpdateFeeSchedule previously mutated the in-memory Calculator only, silently reverting any runtime fee change on the next boot. The fee_schedule_history table and its InsertFeeScheduleHistory / ListFeeScheduleHistory sqlc queries were already shipped but unused; wire them up. - db.FeeScheduleStoreDB is a new small adapter over the existing sqlc queries. InsertFeeSchedule appends a row inside WriteTxOption(); LatestFeeSchedule reads the most recent row inside ReadTxOption() and returns (nil, false, nil) when the table is empty so callers can fall through to config. - server_fees.setupFeesSubsystem constructs the schedule store before the calculator, reads the latest persisted schedule, and falls back to scheduleFromConfig only when no row exists. - adminrpcserver.UpdateFeeSchedule calls InsertFeeSchedule after the in-memory UpdateSchedule succeeds, so the DB write never leaves the calculator and history out of sync. - GetFeeSchedule / UpdateFeeSchedule now round-trip MinRefreshDeltaBlocks (the proto field landed in the previous commit). Unit tests cover insert/list round-trip, latest-wins ordering, nil-input rejection, and pre-persist Validate() enforcement. Additional tests in server_fees_test.go cover the scheduleFromConfig conversion edge cases and the zero-config back-compat contract. Part of issue #263.
Two complementary helper sets that the per-file itest audits in Phase B.2 consume: - itest/fees_helpers_test.go: feeQuoteForBoarding, feeQuoteForRefresh, expectedNetAfterBoarding / expectedNetAfterRefresh all back onto a test-local fees.Calculator constructed from harness.DefaultItestFeeSchedule. operatorEstimateFee proxies the operator's direct-gRPC EstimateFee for assertions that need the server's exact quote at a specific moment (for example after utilization has moved). - harness/ledger_assert.go: LedgerSnapshot / TakeLedgerSnapshot / ExpectedDelta / AssertLedgerDelta walk the admin ListFeeEvents RPC to compute per-account signed balances and assert per-event-type counts between two snapshots. Computing client-side avoids adding a new GetAccountBalance admin surface; test ledgers stay small enough that the O(n) walk is cheap. Both helpers use testing.TB rather than *testing.T so systest can reuse them without build-tag divergence. Part of issue #263.
Three focused itest files covering the corners the existing refresh/boarding/sweep tests don't naturally host: - fees_hotreload_test.go: TestFeesHotReloadAppliesOnNextRound verifies UpdateFeeSchedule immediately reflects in a subsequent GetFeeSchedule and in a follow-up EstimateFee quote. TestFeesHotReloadPersistsAcrossRestart proves the new persistence plumbing survives a RestartArkd: every field of the target schedule (including MinRefreshDeltaBlocks) is reflected by the first GetFeeSchedule after the restart. - fees_admin_rpc_test.go: TestFeesAdminRPCGetScheduleRoundTrip locks in every proto field round-trips through the admin handler. TestFeesAdminRPCUpdateRejectsInvalidDustPolicy guards the policy parse path. TestFeesAdminRPCListFeeEventsPaginates asserts the page<=Total invariant and strictly-increasing entry_id invariants that downstream analytics rely on. - fees_disabled_regression_test.go: TestFeesDisabledGreenPath proves the harness opt-out (WithZeroFeeSchedule) still produces a zero schedule and zero EstimateFee totals, so operators who haven't enabled fees see identical pre-#263 behavior. Part of issue #263.
rapid-driven regression harness covering five invariants the fee
and ledger subsystems rely on:
1. Boarding never charges a liquidity fee. Every random
(schedule, amount, batch size, rate) triple must produce a
FeeBreakdown whose LiquidityFeeSat is zero.
2. EffectiveRate is non-decreasing in utilization. Drawing two
utilization points u1 <= u2 must yield EffectiveRate(u2) >=
EffectiveRate(u1) for any valid schedule.
3. Forfeit applies the MinRefreshDeltaBlocks floor. Any delta
below the floor must compute the same liquidity fee as the
floor itself.
4. Every Record* helper is double-entry with a positive amount,
distinct debit/credit accounts, and the caller-injected time.
5. Boarding + refresh legs (RecordBoardingDeposit +
RecordBoardingFee + RecordCapitalCommitted) sum to zero
across the chart of accounts for any random (deposit, fee)
pair.
Shared generators drawSchedule / drawFeeRate stay in this file
so other property tests can pick them up without export.
Part of issue #263.
The three hardcoded `int64(99_000)` VTXO-amount assertions in boarding_test.go were only correct under the pre-#263 flat-fee world (100_000 boarding minus the legacy 1_000 MinOperatorFee). Under the post-#263 fees-on default the boarding fee is computed dynamically by the operator's fees.Calculator over the full theoretical tree size, so the net VTXO value shifts. Replace the literal 99_000 with expectedNetAfterBoarding(t, 100_000, defaultBatchSizeForBoarding) so the assertion tracks harness.DefaultItestFeeSchedule automatically. A new constant defaultBatchSizeForBoarding=128 in fees_helpers_test.go mirrors config.DefaultRoundsConfig.MaxVTXOsPerTree so the test-local calculator computes the same per-input on-chain share the server uses. Other balance assertions in boarding_test.go are self-consistent (client view vs live VTXO amount) and do not need changes. Part of issue #263.
Three focused tests pin the partial-unique-index + `ON CONFLICT DO NOTHING` contract that keeps durable mailbox replay safe: - TestLedgerReplayIdempotentWithKey: inserting the same (idempotency_key, event_type, debit, credit) quadruple twice returns rowcount=1 on the first attempt and rowcount=0 on the second, with only the first row surviving. A regression that removed the partial unique index or changed the conflict target would fail this test immediately. - TestLedgerReplayAllowsDifferentEventTypesSameRoundID: the refresh path books two legs (forfeit + fee) under one round/idempotency_key; both must commit because the partial unique index discriminates on event_type. Without this, the fee leg would silently dedup behind the forfeit leg and refresh_fee_revenue would never accumulate. - TestLedgerReplayIdempotentWithoutKeyIsNotDeduped: nil-key entries are outside the partial unique index and always commit. Matches the schema contract and prevents an accidental collapse of the `WHERE idempotency_key IS NOT NULL` clause. Replaces the mailbox-replay itest proposed for Phase E.6 in the plan: the DB-layer unit test is strictly more precise because it isolates the invariant to the storage layer without needing to inject a duplicate-delivery side effect at the actor boundary. Part of issue #263.
Pin the darepo-client submodule at the tip of the parallel `issue-263-fees-client-coverage` branch so darepo reviewers see the exact client code the server-side assertions in this PR expect to work with. Client-side changes summary: - darepod.Server.quoteOperatorFee quotes the dynamic operator fee via EstimateFee; Board and SendVTXO now pay the schedule- derived fee instead of the legacy flat terms.MinOperatorFee. - darepoclicommands.mapFeeError translates server-side fee rejections (ErrVTXOBelowMinViable, ErrOperatorFeeTooLow, boarding-too-small, EstimateFee-no-calc) into concise CLI messages. - round/fees_invariants_test.go: rapid-driven properties over computeClientOperatorFee (non-negative, conservation identity, monotonicity, empty-intents identity). Bumping the submodule as a dedicated commit keeps the server diff clean to review without the client's code noise; git log --follow client/ in this PR surfaces a single change. Part of issue #263.
…reasury tests Broader itest coverage and the remaining assertion audits that the Phase A default flip requires: - itest/helpers_test.go: documents the post-#263 convention that every balance assertion under the default schedule computes expected net amounts via the fee-aware helpers. - itest/oor_test.go: replaces hardcoded int64(99_000) VTXO- amount assertions with expectedNetAfterBoarding. Other OOR amounts stay unchanged (OOR fees currently gated off). - itest/send_test.go: replaces operatorInfo.MinOperatorFee (legacy flat 1000 sat) in expectedChange / expectedTotal with feeQuoteForRefresh so the test tracks the post-#263 dynamic-fee quoting in darepod.Server.quoteOperatorFee. - itest/fees_hotreload_test.go: build fresh FeeScheduleParams rather than copying the proto by value (the embedded MessageState contains a sync.Mutex). - systest/boarding_e2e_test.go: insufficient-fee negative test reserves zero implicit fee (vtxoAmount=amount) so the rejection holds under both the dynamic schedule and the legacy flat fee. New itest/systest files: - itest/fees_validation_test.go: drives ErrVTXOBelowMinViable, ErrOperatorFeeTooLow, and warn-policy readback via EstimateFee. - itest/fees_congestion_test.go: compares baseline vs spread-active EffectiveAnnualRate and asserts the total fee rises when the spread activates. - itest/fees_classifier_test.go: verifies external_deposit / external_withdrawal event types are reachable via ListFeeEvents and debit/credit external_funding as expected. - itest/fees_treasury_rehydration_test.go: RestartArkd and assert every TreasuryTracker field survives the restart. - systest/fees_e2e_test.go: smoke test confirming the systest harness wires a non-zero calculator with the configured schedule shape. Part of issue #263.
Pin darepo-client at the tip of issue-263-fees-client-coverage now that the remaining client-side round-trip tests have landed on that branch: - round/refresh_psbt_size_test.go: PSBT shape stays constant across fee schedules. - darepod/congestion_roundtrip_test.go: bufconn-driven fake arkrpc server proves quoteOperatorFee returns the operator's TotalFeeSat verbatim across a baseline → congestion-bump sequence. - darepod/rpc_fees_e2e_test.go: every EstimateFee field proxies verbatim to the CLI; the CLI-visible total matches the wallet-actor-used total for the same inputs. Part of issue #263.
In this commit, we bump LatestMigrationVersion from 11 to 13 to match the actual set of migrations on master. Master added migrations 000012_utxo_attribution and 000013_round_attribution without updating this constant. First-boot works (DB starts at version 0, applies every migration it finds via Up()), but on daemon restart the downgrade-protection check at db/migrations.go:183 trips because the DB is at 13 while the constant says 11, refusing to open the DB with "database downgrade detected: db_version=13, latest_migration_version=11". The new RestartArkd harness helper added under issue #263 is the first thing in the test suite to exercise a restart, so it surfaced this latent master bug.
In this commit, we resolve a conflict between the fees-on default and the client's stale pre-flight fee check, and fix a latent systest harness gap where the dynamic fee validator never ran. The client's round FSM at client/round/transitions.go:383 has a legacy pre-flight check: if the implicit operator fee on a boarding is below env.OperatorTerms.MinOperatorFee, the client rejects its own submission before the server sees it. Under fees-off both values are zero and the check is dormant. Under the post-#263 fees-on default the dynamic EstimateFee quote sits at roughly 167 sats for a 100k boarding, well below the production default of 1000 sats, so every boarding itest gets stuck in ClientFailedState with "operator fee below minimum". The dynamic schedule is authoritative under fees-on, so we zero the legacy flat MinOperatorFee in both harnesses. Tests that need the legacy floor can still install it via an OperatorConfigMutator. For the systest harness, two additional fixes are required: 1. The rounds actor was constructed without a FeeCalculator, which meant validateOperatorFee fell through to the legacy flat path and ignored the dynamic schedule entirely. Any zero-fee boarding would slip through under fees-on. We wire FeeCalculator, TreasuryTracker, and LedgerRef on the rounds actor config so the systests exercise the same validation surface the production daemon does. 2. initFeesSubsystem() was called after the rounds actor config was constructed, so h.feeCalculator was still nil at wire time. We move it ahead of the rounds config so the field is populated. TestBoardingE2EInsufficientOperatorFee is updated to drop the now-obsolete "MinOperatorFee > 0" assertion; the dynamic validator in rounds.validateOperatorFee is the real gate and rejects the zero-fee submission with ErrOperatorFeeTooLow as expected.
3 participants
Summary
Part 4 of 5 in the stacked split of #235. The unilateral-exit
subsystem proper: durable per-target actor + registry, VTXO FSM
integration, RPC surface, DB store, plus a small
GetBalanceaddition so recovered funds are visible after sweep.
Commits (in order):
555cd6b—multi: add ForceUnrollEvent to VTXO lifecycle.Manual unroll requests route through the VTXO actor's FSM rather
than bypassing it with direct DB writes.
LiveStatehandlesForceUnrollEventby transitioning toUnilateralExitStateandemitting
ExpiringNotificationthrough the chain-resolver seam,converging manual and automatic triggers. Adds
ForceUnrollRequest/Responseto the VTXO manager admissiontypes and
TestLiveStateForceUnroll.6439f20—daemonrpc: add Unroll and GetUnrollStatus RPCs.2a541c4—darepod: surface onchain wallet balance in GetBalance RPC. Addsonchain_wallet_confirmed_satso clients cansee confirmed on-chain funds including sweep proceeds.
3c7dd66—db: add unilateral exit job store. Addsunilateral_exit_jobsmigration (now 000008 — 000007 was taken byutxo_audit_logon main), sqlc queries, and persistence store.Provides
UpsertJob,GetJob,ListNonTerminalJobs,MarkJobTerminal.fe4d49b—unroll: add durable per-target unroll actor and registry.UnrollRegistryActor(thin registry, dedup by outpoint,boot restore).
VTXOUnrollActor(durable FSM per target, delegatesto
txconfirm, proof assembly, CSV wait, sweep). Sweep retry (upto 3 attempts before terminal failure). Spend watch on target
outpoint for early external-spend detection. Checkpoint persistence
after every FSM transition.
Forward-port from the original branch
000007_unilateral_exit_storewas bumped to
000008to avoid collision with main's000007_utxo_audit_log.LatestMigrationVersionis now8.lib/scripts→lib/arkscript:unroll/sweep.gowasrewritten to use
arkscript.NewVTXOSpendInfoFromPolicy+SpendInfo.BuildSignDescriptor+arkscript.VTXOTimeoutSpendWitness(the legacy
scripts.NewVTXOSpendInfotook a prebuilt tapscript;the arkscript equivalent derives it from policy keys, which requires
the descriptor to expose
ClientKey.PubKey,OperatorKey, andRelativeExpiry).Descriptor.OwnerKey→Descriptor.ClientKey: main renamedthe field (70906ba). Callers in
sweep.goandunroll/actor_test.gowere updated accordingly.
daemon.pb.goconflicts were resolved byre-running
make rpcafter merging the.protofile; theregenerated output is the committed version.
All fixups were folded back into Elle's original commits so each
commit compiles standalone.
Stack
unroll-01-prepunroll-02-planlib/recovery+unrollplanunroll-03-txconfirmtxconfirmactorunroll-04-coreunroll/unroll-05-wireSupersedes #235.
Authorship
All five commits authored by @ellemouton.
Test plan
go test ./unroll/... ./db/... ./vtxo/...go vet ./...go build ./cmd/...