Add stuck-transaction monitor for wallet transactions#4174
Conversation
A broadcast wallet transaction (deposit sweep, redemption, moving funds) that stays unconfirmed in the mempool locks the wallet's main UTXO and blocks all subsequent wallet transactions until it confirms. Today this is silent: the broadcast path waits only for mempool acceptance and never watches for confirmation. Add a Node-scoped transaction monitor that tracks broadcast wallet transactions and, if one stays unconfirmed for longer than a threshold number of Bitcoin blocks, emits a metric (stuck_wallet_transactions_total) and a warn log identifying the wallet and txid so operators can intervene (e.g. mempool acceleration or CPFP). Detection and alerting only; automated fee-bumping is out of scope (Part B of threshold-network#4171). Refs threshold-network#4173 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds an in-memory transaction monitor, connects wallet transaction executors and node coordination to it, makes confirmation lookups context-aware, and exposes metrics for stuck and unmonitored wallet transactions. Tests cover alerting, eviction, lookup budgets, ordering, and capacity limits. ChangesWallet transaction monitoring
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant WalletAction
participant walletTransactionExecutor
participant transactionMonitor
participant BitcoinChain
WalletAction->>walletTransactionExecutor: broadcast transaction
walletTransactionExecutor->>transactionMonitor: track transaction
transactionMonitor->>BitcoinChain: poll confirmations with bounded context
BitcoinChain-->>transactionMonitor: return confirmation count
transactionMonitor-->>transactionMonitor: alert, evict, or remove transaction
Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
- Track by wall-clock time instead of block height. This removes the synchronous GetLatestBlockHeight call from the broadcast path and, more importantly, the silent-skip on its failure - a transaction is now always tracked once broadcast, including during chain instability when it is most likely to get stuck. - Give up on transactions that stay unconfirmed past a maximum tracking age (e.g. dropped from the mempool) so never-confirming transactions cannot fill the tracking table and starve monitoring of new ones. - Emit the metric outside the mutex rather than holding the lock during the recorder call. - Document the single-goroutine invariant of check(), the per-operator nature of the metric/log, and the in-memory (cross-restart) limitation. - Rework the unit test accordingly and add coverage for the give-up eviction and the capacity bound. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
|
||
| broadcastTxLogger.Infof("transaction is known on Bitcoin chain") | ||
|
|
||
| if wte.transactionMonitor != nil { |
There was a problem hiding this comment.
track() here only runs after GetTransactionConfirmations (line 447) succeeds. If that call keeps failing, line 454 continues and this transaction is never registered — even though BroadcastTransaction at line 420 may have already succeeded. Two paths need to register the transaction: right after a successful broadcast (don't wait on the confirmation lookup for that case), and also here when a later lookup finds the transaction known even if this operator's own broadcast call errored. track() is already safe to call more than once on the same hash, so both call sites can register it without special-casing.
|
|
||
| // Give up on transactions that have been unconfirmed for too long (e.g. | ||
| // dropped from the mempool) so they cannot fill the tracking table. | ||
| if outstanding > transactionMonitorMaxTrackingAge { |
There was a problem hiding this comment.
The 24-hour give-up check here runs and continues before the 6-hour stuck-alert check at line 185, in the same pass. If a transaction's first check() happens after 24 hours already passed, it gets evicted with only a "giving up" log — the stuck alert and the stuck_wallet_transactions_total metric never fire for it. At the defaults, each Electrum operation has a 30-second attempt timeout and a two-minute retry window, but GetTransactionConfirmations can perform several such operations. Reaching eviction before the first alert still requires substantial accumulated delay across the serialized checks, or a long process-level pause, so this is a defensive robustness fix rather than a normal-operation blocker. Still worth fixing: swap the order so the alert check runs before eviction.
| tm.mu.Unlock() | ||
|
|
||
| for txHash, t := range snapshot { | ||
| confirmations, err := tm.btcChain.GetTransactionConfirmations(txHash) |
There was a problem hiding this comment.
Although Electrum bounds each individual operation by default, checks here are serialized, so one slow call delays every transaction behind it in the same pass. Strongest fix is a monitor-specific end-to-end timeout budget for the whole check. Bounded concurrency would help too, though the Electrum client's shared connection mutex means the underlying wire requests stay serialized either way.
| // transaction tracks it independently, so the metric and log are emitted per | ||
| // operator and should be de-duplicated by transaction hash downstream. | ||
| // | ||
| // The tracked set is in-memory only. A transaction that is already stuck when |
There was a problem hiding this comment.
Already called out in the comment, so not a new finding — flagging because restarts (deploys, upgrades, crashes) aren't rare, so this is a monitoring gap on a fairly regular cadence, not just an edge case. Recommend filing a follow-up issue to persist the tracked set and rehydrate it on startup before this ships to mainnet.
| return | ||
| } | ||
|
|
||
| if len(tm.tracked) >= transactionMonitorMaxTracked { |
There was a problem hiding this comment.
When the tracking table is full, this just logs a warning and returns — no metric, and the caller in wallet.go has no way to know registration failed. A full table means a real broadcast transaction goes unmonitored, so suggest adding a dedicated metric here, not just a log line.
| // defaultStuckTransactionThreshold is how long a broadcast wallet | ||
| // transaction may remain unconfirmed before it is considered stuck and an | ||
| // alert is raised (~6 hours). | ||
| defaultStuckTransactionThreshold = 6 * time.Hour |
There was a problem hiding this comment.
The PR description's "Open for review" section mentions defaultStuckTransactionThresholdBlocks = 36, but the constant actually implemented here is defaultStuckTransactionThreshold = 6 * time.Hour — a wall-clock duration, not a block count, and a different name. Worth updating the description to match, or confirming wall-clock is the intended final approach (tracking by block height would need an extra RPC per check). On the config question you raised: since none of these four constants can be tuned without a code change today, recommend moving them into node config with the current values as defaults.
| t.Fatalf("expected no alert for a fresh transaction; got counter [%v]", got) | ||
| } | ||
|
|
||
| // Past the threshold: flagged as stuck exactly once across repeated checks. |
There was a problem hiding this comment.
One more small description/test mismatch: the body says this covers "no alert at the threshold," but this test checks a fresh transaction and then jumps to threshold + 1 minute — the exact boundary itself isn't tested. Please add a case at exactly defaultStuckTransactionThreshold, or update that line in the description.
…st track path - Register a broadcast transaction with the monitor immediately after a successful BroadcastTransaction, not only after the confirmation lookup succeeds, so a failing lookup no longer leaves a broadcast transaction unmonitored (lrsaturnino review). - Fire the stuck alert before the give-up eviction so a transaction first observed after the maximum tracking age still alerts exactly once instead of being silently evicted. - Emit an unmonitored-transactions metric (not just a log) when the tracking table is full, so a real broadcast transaction going unmonitored is visible. - Bound each check pass with a time budget so one slow chain call cannot stall monitoring of the remaining transactions. - Extend tests to cover the alert-before-eviction path, the table-full metric, and the threshold boundary. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
884ff3e to
d3b8b5b
Compare
Note that each GetTransactionConfirmations call is separately bounded by the Electrum client's own operation timeouts, so the per-pass budget plus per-call bound together keep one slow call from stalling a check pass. Correct the boundary-test comment to state it exercises just-below the threshold and explain why exactly-at is not tested deterministically. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![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 `@pkg/tbtc/transaction_monitor.go`:
- Around line 168-190: Update the transaction-monitor check pass after building
the snapshot in the relevant monitoring function to iterate transactions
deterministically by ascending broadcastAt, rather than directly over the
randomized map. Import and use sort as needed, ensuring older transactions are
checked first and deferred newer transactions do not starve older ones across
passes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ebc33ba8-2553-4618-ad4f-c06ca2560eac
📒 Files selected for processing (13)
pkg/clientinfo/performance.gopkg/tbtc/deposit_sweep.gopkg/tbtc/deposit_sweep_test.gopkg/tbtc/moved_funds_sweep.gopkg/tbtc/moved_funds_sweep_test.gopkg/tbtc/moving_funds.gopkg/tbtc/moving_funds_test.gopkg/tbtc/node.gopkg/tbtc/redemption.gopkg/tbtc/redemption_test.gopkg/tbtc/transaction_monitor.gopkg/tbtc/transaction_monitor_test.gopkg/tbtc/wallet.go
Go map iteration is randomized, so when a check pass hits its time budget and defers the rest, an unlucky old transaction could be skipped pass after pass and miss its stuck threshold entirely. Iterate a snapshot ordered by broadcast time (oldest first) via snapshotByAge, so the transactions closest to alerting are always checked first and only the newest are ever deferred (per CodeRabbit review on threshold-network#4174). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
3834410
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/tbtc/transaction_monitor_test.go (1)
226-242: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard the second
check()call with a timeout, like the first.This block asserts the core "later pass skips the in-flight lookup instead of stalling" behavior, but
monitor.check(context.Background())at line 235 is called synchronously with no timeout. If this exact behavior regresses, the call blocks indefinitely and the test fails via a global Go test timeout instead of a clear, fast assertion — losing the diagnostic value this test is meant to provide. The firstcheckWithBudgetcall a few lines above (lines 203-220) already uses acheckDone/select+time.Afterpattern for the same reason; mirror it here.♻️ Proposed fix
confirmedTxHash := confirmedTx.Hash() monitor.track(confirmedTxHash, [20]byte{}) - monitor.check(context.Background()) + secondCheckDone := make(chan struct{}) + go func() { + monitor.check(context.Background()) + close(secondCheckDone) + }() + select { + case <-secondCheckDone: + case <-time.After(2 * time.Second): + t.Fatal("second check pass stalled on the in-flight lookup") + } if got := chain.getLookupCount(); got != 1 { t.Fatalf("expected one in-flight lookup; got [%d]", got) }🤖 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 `@pkg/tbtc/transaction_monitor_test.go` around lines 226 - 242, Wrap the second monitor check in the same timeout-based completion pattern used by the earlier checkWithBudget block: run monitor.check in a goroutine, wait on its completion channel, and fail with a short time.After timeout if it stalls. Preserve the existing lookup-count and tracking assertions after the check completes.
🤖 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.
Nitpick comments:
In `@pkg/tbtc/transaction_monitor_test.go`:
- Around line 226-242: Wrap the second monitor check in the same timeout-based
completion pattern used by the earlier checkWithBudget block: run monitor.check
in a goroutine, wait on its completion channel, and fail with a short time.After
timeout if it stalls. Preserve the existing lookup-count and tracking assertions
after the check completes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ea421f54-43f6-44aa-a4f8-6f1e077c83cc
📒 Files selected for processing (2)
pkg/tbtc/transaction_monitor.gopkg/tbtc/transaction_monitor_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/tbtc/transaction_monitor.go
Add a context parameter to bitcoin.Chain.GetTransactionConfirmations so callers can bound and cancel the lookup. The electrum implementation threads the caller context (combined with the connection lifetime context) into its retry loop; other callers pass context.Background(), which the combining preserves as their prior behavior. This lets the transaction monitor drop its per-lookup goroutine and the inFlightConfirmationLookups dedup map. The confirmation lookup is now a synchronous call bounded by the check budget, so a slow or hung backend can no longer strand a goroutine or a map entry beyond a check pass.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/bitcoin/electrum/electrum.go (1)
1290-1301: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftMake mutex acquisition cancellation-aware.
requestWithRetryderivesrequestCtxonly afterreconnectIfShutdown()andc.clientMutex.Lock(). Because the mutex is held across the network request, an unrelated slow request can keep a monitor lookup blocked even after itsparentCtxexpires. Avoid non-cancellable lock waits in this path, or use a context-aware semaphore/connection design.🤖 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 `@pkg/bitcoin/electrum/electrum.go` around lines 1290 - 1301, Update requestWithRetry so mutex acquisition is cancellation-aware: derive/use the request context before waiting for c.clientMutex and replace the blocking Lock path with a context-aware acquisition mechanism. Ensure an expired parentCtx returns promptly without waiting behind another network request, while preserving the existing mutex protection around the request and retry flow.
🤖 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 `@pkg/bitcoin/electrum/electrum.go`:
- Around line 143-152: Update GetTransactionConfirmations to use a context-aware
internal latest-block-height helper with reqCtx instead of
c.GetLatestBlockHeight(), while preserving the existing public method behavior.
Implement the helper using the supplied context and add a regression test that
blocks the tip lookup and verifies cancellation within the monitor check budget.
In `@pkg/tbtc/transaction_monitor.go`:
- Around line 216-221: Update the transaction-processing loop around
GetTransactionConfirmations so a lookup that ends with checkCtx expiration does
not remain first on every pass. Defer or rotate that timed-out transaction, or
continue processing subsequent entries as unconfirmed so newer transactions
still receive confirmation removal, stuck alerts, and maximum-age eviction; add
coverage for a blocked oldest transaction and a newer eligible transaction.
---
Outside diff comments:
In `@pkg/bitcoin/electrum/electrum.go`:
- Around line 1290-1301: Update requestWithRetry so mutex acquisition is
cancellation-aware: derive/use the request context before waiting for
c.clientMutex and replace the blocking Lock path with a context-aware
acquisition mechanism. Ensure an expired parentCtx returns promptly without
waiting behind another network request, while preserving the existing mutex
protection around the request and retry flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a015f496-4bcd-4f21-aa3c-b868d78811b1
📒 Files selected for processing (17)
pkg/bitcoin/chain.gopkg/bitcoin/chain_test.gopkg/bitcoin/electrum/electrum.gopkg/bitcoin/electrum/electrum_integration_test.gopkg/bitcoin/spv_proof.gopkg/clientinfo/rpc_health_test.gopkg/maintainer/btcdiff/bitcoin_chain_test.gopkg/maintainer/spv/bitcoin_chain_test.gopkg/maintainer/spv/spv.gopkg/tbtc/bitcoin_chain_test.gopkg/tbtc/deposit_sweep.gopkg/tbtc/transaction_monitor.gopkg/tbtc/transaction_monitor_test.gopkg/tbtc/wallet.gopkg/tbtc/wallet_test.gopkg/tbtcpg/bitcoin_chain_test.gopkg/tbtcpg/deposit_sweep.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/tbtc/wallet.go
- pkg/tbtc/deposit_sweep.go
- pkg/tbtc/transaction_monitor_test.go
| // Bound the network calls below by both the caller's context and the | ||
| // connection lifetime context, so a cancelled caller aborts the lookup while | ||
| // connection shutdown still cancels it as before. | ||
| reqCtx, cancel := context.WithCancel(ctx) | ||
| defer cancel() | ||
| stopOnParentDone := context.AfterFunc(c.parentCtx, cancel) | ||
| defer stopOnParentDone() | ||
|
|
||
| rawTransaction, err := requestWithRetry( | ||
| reqCtx, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Propagate the caller context through the latest-block lookup.
GetTransactionConfirmations bounds the raw transaction and history calls with reqCtx, but its later c.GetLatestBlockHeight() call uses c.parentCtx. A slow tip request can therefore continue past the monitor’s check budget and stall the monitoring pass. Add a context-aware internal latest-height helper and call it with reqCtx; add a regression test covering a blocked tip lookup.
🤖 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 `@pkg/bitcoin/electrum/electrum.go` around lines 143 - 152, Update
GetTransactionConfirmations to use a context-aware internal latest-block-height
helper with reqCtx instead of c.GetLatestBlockHeight(), while preserving the
existing public method behavior. Implement the helper using the supplied context
and add a regression test that blocks the tip lookup and verifies cancellation
within the monitor check budget.
| // The chain call is bounded by checkCtx, so a slow or hung backend cannot | ||
| // keep this run loop blocked past the check budget: when the budget | ||
| // expires the call is cancelled and returns, and the checkCtx.Err() guard | ||
| // below defers the remaining transactions to the next pass. | ||
| confirmations, err := | ||
| tm.btcChain.GetTransactionConfirmations(checkCtx, txHash) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Prevent a timed-out oldest lookup from starving the rest of the table.
Because each pass starts with the oldest transaction, a lookup that blocks until checkCtx expires is retried first on every pass; the function returns before reaching newer transactions. Their confirmation removal, stuck alerts, and maximum-age eviction can then be delayed indefinitely. Rotate/defer the timed-out entry or process remaining entries as unconfirmed for local alert/eviction handling, and add a test with a blocked old transaction plus a newer eligible one.
🤖 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 `@pkg/tbtc/transaction_monitor.go` around lines 216 - 221, Update the
transaction-processing loop around GetTransactionConfirmations so a lookup that
ends with checkCtx expiration does not remain first on every pass. Defer or
rotate that timed-out transaction, or continue processing subsequent entries as
unconfirmed so newer transactions still receive confirmation removal, stuck
alerts, and maximum-age eviction; add coverage for a blocked oldest transaction
and a newer eligible transaction.
Run the age-based stuck-alert and give-up eviction for the current transaction before deferring the rest when a check pass hits its time budget. Previously the pass returned immediately after a cancelled confirmation lookup, so a backend that consistently hung on the oldest tracked transaction would keep it (the one closest to the stuck threshold) from ever alerting or being evicted, and newer transactions were never reached either. The remaining transactions are still deferred rather than alerted on without a lookup, so they cannot false-alert on age alone. Also thread broadcastCtx into the post-broadcast confirmation check so a hung backend cannot block the broadcast goroutine past its timeout, and make the oldest-first snapshot sort stable.
3 participants
Implements #4173. A broadcast wallet transaction that stays unconfirmed in the mempool locks the wallet's main UTXO and blocks all subsequent wallet transactions until it confirms. Today this is silent — the broadcast path waits only for mempool acceptance, never for confirmation.
Change
A Node-scoped
transactionMonitor(pkg/tbtc/transaction_monitor.go):walletTransactionExecutorregisters every broadcast transaction with the monitor — immediately after a successfulBroadcastTransaction, and again once it's known on-chain (idempotent) — so all four wallet-action types are covered even if a confirmation lookup fails.runCoordinationLayer) pollsGetTransactionConfirmationsfor each tracked tx on a wall-clock cadence. Confirmed → dropped. Unconfirmed past the stuck threshold → emitstuck_wallet_transactions_total+ a Warn log naming the wallet and txid (once per tx, and before any give-up eviction). Never-confirming transactions are evicted after a maximum tracking age; a full tracking table surfacesunmonitored_wallet_transactions_total. Each check pass is time-budgeted so one slow chain call cannot stall the rest.keep-client only exposes the metric + log; alerting/paging is left to the operator's existing Prometheus/Grafana. Additive to the coarse
gated_sweep_stall_secondssignal — it surfaces the specific stuck txid so an operator can act in minutes.Threshold is wall-clock
Tracking uses a wall-clock duration (
defaultStuckTransactionThreshold = 6h), not a block count — no extra RPC per check. (An earlier revision of this PR used a block count; this supersedes it.)Context-aware confirmation lookups
bitcoin.Chain.GetTransactionConfirmationsnow takes acontext.Contextso callers can bound and cancel the lookup. The Electrum implementation threads the caller context (combined with the connection-lifetime context) into its retry loop; the other callers passcontext.Background(), which the combining preserves as their prior behavior.This lets the monitor's check pass call the lookup synchronously, bounded by the per-pass time budget, instead of spawning a per-lookup goroutine guarded by an in-flight dedup map. When the budget expires the call is cancelled and the pass defers the remaining transactions to the next pass, so a slow or hung backend can no longer strand a goroutine or an in-flight map entry beyond a check pass.
Note: Electrum's
GetTransactionConfirmationsalso makes an internalGetLatestBlockHeightcall — a separatebitcoin.Chainmethod that is not context-threaded. That trailing lookup still self-bounds via Electrum's own request-retry timeout (~2 min) rather than cancelling with the caller context, so a pass can overrun its budget by up to that before being caught on the next iteration. Not a hang or a leak.Tests
Cover: no alert below the threshold and at the boundary; exactly-once alert past it; untracking on confirmation; alert-before-eviction for a never-confirming transaction; the table-full metric; and a budgeted check pass cancelling a blocked confirmation lookup rather than stalling.
go build+go test ./pkg/tbtc/... ./pkg/clientinfo/...pass.Out of scope / follow-ups
Summary by CodeRabbit
New Features
Tests