fix: tolerate wallet connection loss - #1080
Conversation
WalkthroughThis PR adds resilience to wallet interactions across three layers: the gRPC client now automatically retries transient failures with exponential backoff; confirmation polling treats wallet errors as transient and retries on each tick; and sweep scheduling fails safely instead of dereferencing nil responses, preventing crashes during temporary wallet unavailability. ChangesSweep scheduling resilience and wallet retry handling
Sequence DiagramsequenceDiagram
participant Scheduler as scheduleSweepBatchOutput / createBatchSweepTask
participant ConfirmUtil as waitForConfirmation
participant WalletClient as Wallet gRPC Client
participant WalletServer as Wallet Server
Scheduler->>ConfirmUtil: Poll for confirmation (retry loop, uses s.ctx)
ConfirmUtil->>WalletClient: IsTransactionConfirmed RPC
WalletClient->>WalletServer: RPC attempt (unary retry interceptor)
alt Transient failure
WalletServer-->>WalletClient: Retryable error (e.g., Unavailable)
WalletClient->>WalletServer: Retries with backoff
WalletServer-->>WalletClient: Retryable error
WalletClient-->>ConfirmUtil: error (per-call may disable retry)
ConfirmUtil->>ConfirmUtil: Log transient warning, wait next tick
ConfirmUtil->>WalletClient: Retry on next tick
else Confirmation succeeds
WalletServer-->>WalletClient: Confirmed + blockTimestamp
WalletClient-->>ConfirmUtil: Confirmed + blockTimestamp
ConfirmUtil-->>Scheduler: blockTimestamp
Scheduler->>Scheduler: Schedule batch sweep
else Confirmation loop canceled or unrecoverable
ConfirmUtil-->>Scheduler: Error (wallet unavailable)
Scheduler->>Scheduler: Log and return early (skip scheduling)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 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)
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 |
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Arkana Code Review — #1080
Overall direction is correct and important — the old time.Now() fallback on block-height schedulers was a real bug that could shorten the safety window. Retrying transient wallet errors instead of bailing is the right call. But there are issues that need fixing before merge.
🔴 MUST FIX
1. waitForConfirmation now retries forever with context.Background() — no escape hatch
internal/core/application/service.go:3550 — scheduleSweepBatchOutput passes context.Background(). With the old code, a wallet error would break the loop and return. With this PR, a wallet error logs a warning and continues, meaning the goroutine polls indefinitely if the wallet stays down or the txid is somehow invalid.
This is a goroutine leak. If the wallet goes down for hours, you accumulate one stuck goroutine per round that completes during the outage. Since scheduleSweepBatchOutput is called via go s.scheduleSweepBatchOutput(round) at service.go:349, these are fire-and-forget goroutines with no cancellation.
Fix: Either:
- Add a max retry count or timeout context in
scheduleSweepBatchOutput(e.g.,context.WithTimeout(ctx, 1*time.Hour)), then let thereturnpath handle it as before — bail and rely on next-startup recovery. - Or add a retry ceiling inside
waitForConfirmationitself (e.g., N consecutive errors → return error).
2. Same bug exists in sweeper.go:475-482 — not fixed by this PR
internal/core/application/sweeper.go:475-482 has the exact same pattern this PR fixes in service.go:3552-3556:
blockTimestamp, err := waitForConfirmation(context.Background(), rootInput, s.wallet)
if err != nil {
blockTimestamp = &ports.BlockTimestamp{Time: time.Now().Unix()}
}This is the same dangerous fallback on block-height schedulers. If you're fixing the pattern in scheduleSweepBatchOutput, you should fix it here too, otherwise the same bug class remains.
Similarly, fraud.go:46-61 has a variant of this pattern that falls back to GetCurrentBlockTime — which is better but still problematic if the wallet is truly unreachable.
3. grpc_retry dependency — does it build?
wallet_client.go imports github.com/grpc-ecosystem/go-grpc-middleware/retry, but the PR doesn't modify go.mod or go.sum. While go-grpc-middleware v1.4.0 is in go.mod, the /retry sub-package may or may not be included in the module's dependency graph already. If CI passes, this is fine — but please confirm. If it doesn't build, go.sum needs updating.
🟡 SHOULD FIX
4. Withdraw retry opt-out: grpc_retry.WithMax(0) — verify semantics
wallet_client.go:458 — The comment says "opt out of the interceptor" by passing grpc_retry.WithMax(0). Double-check that WithMax(0) means "zero retries" and not "unlimited retries" in go-grpc-middleware/retry. The library source uses maxRetry uint — 0 should mean no retries, but this is safety-critical (double-spend vector). Please add a unit test or at minimum a code comment citing the library behavior.
5. waitForConfirmation silently swallows non-transient errors
utils.go:544-548 — After this PR, ALL errors from IsTransactionConfirmed are treated as transient and retried. But some errors are permanent (e.g., codes.InvalidArgument if the txid is malformed, codes.NotFound if the tx doesn't exist). These will now retry forever instead of surfacing immediately.
Consider distinguishing transient vs. permanent errors, or at minimum log at Error level after N consecutive failures.
6. No tests for the new retry/bail-out behavior
The PR description checkbox "Add a safety net so a crash in a background task can't take down the server" is unchecked. There are no new tests for:
waitForConfirmationretrying on transient error then succeedingscheduleSweepBatchOutputreturning early whenwaitForConfirmationfailsWithdrawnot retrying
These are protocol-critical code paths — they deserve test coverage.
✅ LOOKS GOOD
- The
returninstead of fallback inscheduleSweepBatchOutputis the correct fix — relying on next-startup recovery is far safer than scheduling at the wrong height. - Disabling retry on
Withdrawis the right call — idempotency isn't guaranteed. - Using
grpc_retryat the transport level rather than hand-rolling retry loops is clean. - Log level escalation from
WarntoErroron the bail-out path is appropriate.
⚠️ PROTOCOL-CRITICAL FLAG
This PR touches sweep scheduling and round lifecycle — both protocol-critical paths. The sweep schedule determines when users lose the ability to unilaterally exit. A bug here directly impacts fund safety. Requesting human review even though the direction is correct.
cc @louisinger — you authored the original fallback in 5d4ecade. The same pattern in sweeper.go:475-482 should be addressed.
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Arkana Code Review — #1080
Overall direction is correct and important — the old time.Now() fallback on block-height schedulers was a real bug that could shorten the safety window. Retrying transient wallet errors instead of bailing is the right call. But there are issues that need fixing before merge.
🔴 MUST FIX
1. waitForConfirmation now retries forever with context.Background() — no escape hatch
internal/core/application/service.go:3550 — scheduleSweepBatchOutput passes context.Background(). With the old code, a wallet error would break the loop and return. With this PR, a wallet error logs a warning and continues, meaning the goroutine polls indefinitely if the wallet stays down or the txid is somehow invalid.
This is a goroutine leak. If the wallet goes down for hours, you accumulate one stuck goroutine per round that completes during the outage. Since scheduleSweepBatchOutput is called via go s.scheduleSweepBatchOutput(round) at service.go:349, these are fire-and-forget goroutines with no cancellation.
Fix: Either:
- Add a max retry count or timeout context in
scheduleSweepBatchOutput(e.g.,context.WithTimeout(ctx, 1*time.Hour)), then let thereturnpath handle it as before — bail and rely on next-startup recovery. - Or add a retry ceiling inside
waitForConfirmationitself (e.g., N consecutive errors → return error).
2. Same bug exists in sweeper.go:475-482 — not fixed by this PR
internal/core/application/sweeper.go:475-482 has the exact same pattern this PR fixes in service.go:3552-3556:
blockTimestamp, err := waitForConfirmation(context.Background(), rootInput, s.wallet)
if err != nil {
blockTimestamp = &ports.BlockTimestamp{Time: time.Now().Unix()}
}This is the same dangerous fallback on block-height schedulers. If you're fixing the pattern in scheduleSweepBatchOutput, you should fix it here too, otherwise the same bug class remains.
Similarly, fraud.go:46-61 has a variant of this pattern that falls back to GetCurrentBlockTime — which is better but still problematic if the wallet is truly unreachable.
3. grpc_retry dependency — does it build?
wallet_client.go imports github.com/grpc-ecosystem/go-grpc-middleware/retry, but the PR doesn't modify go.mod or go.sum. While go-grpc-middleware v1.4.0 is in go.mod, the /retry sub-package may or may not be included in the module's dependency graph already. If CI passes, this is fine — but please confirm. If it doesn't build, go.sum needs updating.
🟡 SHOULD FIX
4. Withdraw retry opt-out: grpc_retry.WithMax(0) — verify semantics
wallet_client.go:458 — The comment says "opt out of the interceptor" by passing grpc_retry.WithMax(0). Double-check that WithMax(0) means "zero retries" and not "unlimited retries" in go-grpc-middleware/retry. The library source uses maxRetry uint — 0 should mean no retries, but this is safety-critical (double-spend vector). Please add a unit test or at minimum a code comment citing the library behavior.
5. waitForConfirmation silently swallows non-transient errors
utils.go:544-548 — After this PR, ALL errors from IsTransactionConfirmed are treated as transient and retried. But some errors are permanent (e.g., codes.InvalidArgument if the txid is malformed, codes.NotFound if the tx doesn't exist). These will now retry forever instead of surfacing immediately.
Consider distinguishing transient vs. permanent errors, or at minimum log at Error level after N consecutive failures.
6. No tests for the new retry/bail-out behavior
The PR description checkbox "Add a safety net so a crash in a background task can't take down the server" is unchecked. There are no new tests for:
waitForConfirmationretrying on transient error then succeedingscheduleSweepBatchOutputreturning early whenwaitForConfirmationfailsWithdrawnot retrying
These are protocol-critical code paths — they deserve test coverage.
✅ LOOKS GOOD
- The
returninstead of fallback inscheduleSweepBatchOutputis the correct fix — relying on next-startup recovery is far safer than scheduling at the wrong height. - Disabling retry on
Withdrawis the right call — idempotency isn't guaranteed. - Using
grpc_retryat the transport level rather than hand-rolling retry loops is clean. - Log level escalation from
WarntoErroron the bail-out path is appropriate.
⚠️ PROTOCOL-CRITICAL FLAG
This PR touches sweep scheduling and round lifecycle — both protocol-critical paths. The sweep schedule determines when users lose the ability to unilaterally exit. A bug here directly impacts fund safety. Requesting human review even though the direction is correct.
cc @louisinger — you authored the original fallback in 5d4ecade. The same pattern in sweeper.go:475-482 should be addressed.
![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 `@internal/core/application/utils.go`:
- Around line 544-549: The loop in waitForConfirmation currently swallows errors
from IsTransactionConfirmed and retries forever, which can strand background
work; modify waitForConfirmation to respect a caller context
deadline/cancellation or enforce a capped transient-error budget: accept a
context.Context (or use the existing one) and return when ctx.Done() triggers,
and/or track a retry counter with backoff and a maxAttempts after which return
the last error; update references in waitForConfirmation and the
IsTransactionConfirmed error handling (txid, retry logic) so the function
returns a bounded error instead of looping indefinitely.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 05bd81db-b527-4f31-a0b9-b8e9b398c19b
📒 Files selected for processing (3)
internal/core/application/service.gointernal/core/application/utils.gointernal/infrastructure/wallet/wallet_client.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/core/application/sweeper.go (1)
465-477:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the subtree argument for expiry/confirmation anchor.
scheduleForSubTreecomputesvtxoTreeExpiryandrootInputfrom outervtxoTreeinstead of thetreeparameter. That can schedule subtree sweeps from the wrong confirmation point and wrong expiry basis.💡 Proposed fix
scheduleForSubTree := func(txid string, tree *tree.TxTree) { - vtxoTreeExpiry, err := s.getVtxoTreeExpiry(vtxoTree) + vtxoTreeExpiry, err := s.getVtxoTreeExpiry(tree) if err != nil { log.WithError(err). Errorf("failed to get vtxo tree expiry for batch %s", commitmentTxid) return } @@ - rootInput := vtxoTree.Root.UnsignedTx.TxIn[0].PreviousOutPoint.Hash.String() + rootInput := tree.Root.UnsignedTx.TxIn[0].PreviousOutPoint.Hash.String() blockTimestamp, err := waitForConfirmation(s.ctx, rootInput, s.wallet)🤖 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 `@internal/core/application/sweeper.go` around lines 465 - 477, The scheduleForSubTree closure incorrectly uses the outer vtxoTree variable instead of its tree parameter, causing wrong expiry and confirmation anchors; change the two usages so s.getVtxoTreeExpiry(...) is called with the local tree (i.e., s.getVtxoTreeExpiry(tree)) and derive rootInput from tree.Root.UnsignedTx.TxIn[0].PreviousOutPoint.Hash.String() (leave the existing error handling and commitmentTxid context intact).
🤖 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.
Outside diff comments:
In `@internal/core/application/sweeper.go`:
- Around line 465-477: The scheduleForSubTree closure incorrectly uses the outer
vtxoTree variable instead of its tree parameter, causing wrong expiry and
confirmation anchors; change the two usages so s.getVtxoTreeExpiry(...) is
called with the local tree (i.e., s.getVtxoTreeExpiry(tree)) and derive
rootInput from tree.Root.UnsignedTx.TxIn[0].PreviousOutPoint.Hash.String()
(leave the existing error handling and commitmentTxid context intact).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 854e4aa0-8e38-4887-a52f-12e5da991c82
📒 Files selected for processing (5)
internal/core/application/service.gointernal/core/application/sweeper.gointernal/core/application/utils.gointernal/infrastructure/wallet/wallet_client.gointernal/infrastructure/wallet/wallet_client_test.go
|
hi @altafan , please review |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
PROTOCOL-CRITICAL: human review required.
Flagging for human protocol review before merge — sweep scheduling and forfeit-adjacent paths are money-at-risk code. The fix itself looks correct and addresses a real bug: the previous blockTimestamp = &ports.BlockTimestamp{Time: time.Now().Unix()} fallback left Height == 0, so in a BlockHeight scheduler regime the sweep was scheduled at vtxoTreeExpiry.Value (≈40) instead of currentHeight + vtxoTreeExpiry.Value — potentially firing an on-chain sweep well before users' unilateral-exit window, exactly as the PR body describes. Reverting the #1034 fallback and bailing to next-boot is the right call.
Findings / questions:
-
grpc_retrywithDeadlineExceededon non-Withdraw side-effect RPCs —internal/infrastructure/wallet/wallet_client.go:34-39.Withdrawis correctly opted out withWithMax(0), but the interceptor still auto-retries every other call onDeadlineExceeded, including ones with server-side side effects:DeriveAddresses/DeriveConnectorAddress(may burn nbxplorer address indices),LockConnectorUtxos,RescanUtxos,LoadSignerKey, andBroadcastTransaction. Broadcast and (probably) LockConnectorUtxos are naturally idempotent; the derivation calls resolve toNbxplorer.GetNewUnusedAddress(..., i)(pkg/arkd-wallet/core/application/wallet/service.go:947-957) with an explicit index so they should be safe, but this is worth confirming against nbxplorer semantics. If any of these are non-idempotent, please either opt them out withWithMax(0)or dropDeadlineExceededfrom the retryable set (keepUnavailable/ResourceExhausted, which imply the RPC never reached the server). -
waitForConfirmationnow retries every non-ctx error forever —internal/core/application/utils.go:544-553. That's the intended behavior for wallet flakes, but it also masks permanent errors (invalid txid, wrong network, wallet returning a structural error). At regtest tick=1s the loop is tight; at mainnet tick=1m operators will get one warn/min forever. Consider: (a) surfacing a distinct error class after N consecutive failures to alert operators, or (b) at least tightening the log message so it's clearly "will retry forever until wallet recovers or shutdown", not just "transient". -
fraud.go:46-61still has the old fallback pattern — falls back tos.wallet.GetCurrentBlockTime(ctx)whenwaitForConfirmationerrors. With the new semanticswaitForConfirmationonly returnsctx.Err(), so that fallback is now effectively dead code (if ctx is canceled,GetCurrentBlockTime(ctx)will also fail). Not a regression, but the inconsistency invites future reintroduction of the same bug you just fixed — worth cleaning up in this PR while it's in view. -
"picked up on next startup" claim — verified:
sweeper.start()→GetSweepableRounds()→createBatchSweepTask()atsweeper.go:64,100will re-run for any completed-but-unswept round. So the bail-out inservice.go:3552andsweeper.go:476is genuinely recoverable. No test proves this though — a unit test covering "wallet down during round completion → wallet up on restart → sweep scheduled correctly" would harden the guarantee. -
Log wording —
"wallet unavailable; cannot schedule sweep..."atservice.go:3554/sweeper.go:478will also fire on service shutdown (ctx canceled). Consider distinguishingctx.Err() != nil(shutdown, expected) from an actual wallet failure to keep alerting signal clean. -
Test —
TestWithdrawDoesNotRetryatwallet_client_test.go:33-52is solid; the control case with a raw client proving 5 retries do occur under the same interceptor is the right way to guard against silent removal of theWithMax(0)opt-out. Good. -
Sweeper
s.ctxfield — set once instart()atsweeper.go:62, read from goroutines. Pre-existing pattern, but note thatcreateBatchSweepTaskclosures scheduled byscheduleBatchSweepcapturesand reads.ctxat fire time; safe today becausestart()runs before the scheduler dispatches, but no synchronization guards this ordering.
Nothing in this PR requires a change for correctness of the fix — items 1, 2, 3, 5 are the highest-value follow-ups. Requesting changes only to enforce the protocol-review gate on sweeper.go / service.go.
|
Changes were requested 2+ days ago. @Dunsin-cyber need any help addressing the feedback? |
|
Changes were requested 8+ days ago. @Dunsin-cyber need any help addressing the feedback? |
|
Changes were requested 5 days ago. @Dunsin-cyber need any help addressing the feedback? |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Changes were requested 94 days ago on this wallet connection-loss fix. @Dunsin-cyber need any help addressing the feedback?
|
Changes were requested 4-5+ days ago. @Dunsin-cyber need any help addressing the feedback? |
|
Changes were requested on this PR (tolerate wallet connection loss). @Dunsin-cyber need any help addressing the feedback? |
2 participants
What's wrong
The code assumed calls to the wallet (and nbxplorer behind it) would never fail. So a brief connection drop could break a running flow or, worse, save wrong data to disk.
The worst case is in
scheduleSweepBatchOutput. When it couldn't reach the wallet to ask "what block are we on?", it just made up the answer (block height 0). That scheduled the sweep at around block 40 instead of the realcurrentHeight + 40— so the sweep fires way too early and the safety window users rely on to pull their money out basically disappears.Restarting didn't help, because the wrong schedule was already saved. The same path could also hit a nil value and crash the process (#1031).
What this changes
Instead of pretending a failed call worked, we retry — and if the wallet still can't be reached, we stop and try again later instead of saving bad data.
waitForConfirmationkeeps trying instead of giving up on the first errorscheduleSweepBatchOutputstops instead of making up a block height (fixes Panic in scheduleSweepBatchOutput when nbxplorer loses DB connection #1031); the round is picked back up on the next startupWithdrawdoes NOT auto-retry, so we never send money twiceSummary by CodeRabbit
Bug Fixes
Tests