fix(wallet-toolbox): close nosend orphan-output failure mode#122
Conversation
A nosend transaction (created via createAction({noSend:true})) can be
externally broadcast by the caller and confirmed on chain before any
internalizeAction or Monitor.TaskCheckNoSends cycle has retired its
nosend status. Two paths could then destroy the wallet's bookkeeping
for the chain-confirmed tx, hiding every output it produced (including
the wallet's own auto-fund change) from the listOutputsKnex
txStatusAllowed filter. A third gap meant a correctly-issued
post-broadcast internalizeAction call silently no-op'd on the
lifecycle, leaving the tx vulnerable to a later abortAction.
This commit lands four fixes in defense-in-depth:
1. storage/methods/internalizeAction.ts (mergedInternalize): retires
the nosend lifecycle when merging into an existing nosend tx.
Transitions transactions.status to 'completed' (if the BEEF
includes a BUMP, via the same findOrInsertProvenTx path
newInternalize uses) or 'unproven' otherwise, and moves the
proven_tx_req out of 'nosend' to 'completed' or 'unmined'
respectively so Monitor's standard proof-fetching flow takes over.
Adds a history note for forensics. Updates the doc-string in both
signer- and storage-layer internalizeAction.ts to reflect that
nosend is a documented merge-target status.
2. monitor/Monitor.ts (processNewBlockHeader): nudges
TaskCheckNoSends.checkNow on every new block header, parallel to
the existing TaskCheckForProofs.checkNow nudge. The
TaskCheckNoSends.checkNow flag is documented in the task source
as designed for this signal but was never wired — a grep across
the source tree returned zero external setters. Empirically: in
8 days of monitor_events from a production wallet (5,681 records),
zero CheckNoSends events fired while other tasks fired normally.
3. storage/StorageProvider.ts (abortAction): refuses to invalidate a
signed nosend tx whose txid the network already knows about.
Calls services.getStatusForTxids and refuses on 'mined' or
'known' (mempool-aware) status. Service-unreachable handling is
conservative-refuse on both the throw path AND the graceful
r.status='error' return path; the latter explicitly checked
before r.results.find(...) to avoid a silent fall-through that
would re-introduce the bug. Records an abortAction-skipped-onchain
history note on the proven_tx_req for forensics. Throws
WERR_INVALID_PARAMETER with a message directing the caller to
internalizeAction.
4. storage/methods/ListActionsSpecOp.ts (specOpNoSendActions.postProcess):
pre-filters chain-known rows before the per-row bulk-abort loop.
Without this filter, Fix 3's throw would propagate out of the
await and bail the entire bulk call halfway, leaving genuinely
off-chain nosend rows in the same page un-aborted. The pre-filter
does one batched getStatusForTxids call for the whole page,
builds a protectedTxids set, and in the loop skips protected
rows (leaving their tx.status as 'nosend' so the caller sees the
skip) while processing off-chain rows normally. Conservative-
refuse on both service-failure modes protects ALL candidate
txids — better to leave nosend rows alone than risk orphaning
outputs of txs that may be on chain.
Layered defense: Fix 1 retires the lifecycle immediately on
post-broadcast internalizeAction; Fix 2 gives Monitor a per-block
self-heal cadence for cases where Fix 1 was missed; Fix 3 protects
the abortAction surface directly for callers that reach it anyway;
Fix 4 keeps bulk-recovery affordances working for genuinely off-chain
rows. Any of the three primary fixes alone closes the most common
failure path; all four together close the failure path the
recurrence audit demonstrates (8 incidents in 6 days in one
production wallet).
Residual edge case (documented in PR description, not closed here):
if getStatusForTxids returns success+per-txid-unknown for a tx that
is actually on chain (indexer ingest lag / transient cache miss),
Fix 3 treats the chain-check as authoritative and proceeds. Fix 2's
per-block nudge self-heals once the indexer ingests the block;
caller-side multi-source verification can close it pre-call.
Two test files covering the four fixes from the preceding commit:
test/monitor/processNewBlockHeader.test.ts
- TaskCheckNoSends.checkNow transitions to true when a new block
header arrives (alongside the existing TaskCheckForProofs.checkNow
nudge).
- Multiple consecutive new-header notifications keep
TaskCheckNoSends.checkNow asserted (handling the case where
runTask clears the flag mid-cycle).
test/storage/nosendLifecycleDefense.test.ts
- Fix 3 (abortAction chain-status check):
- Refuses on chain status 'mined' (confirmed in a block).
- Refuses on chain status 'known' (mempool, mempool-aware
protection during the propagation window).
- Proceeds normally on chain status 'unknown'.
- Conservatively refuses when getStatusForTxids throws.
- Conservatively refuses on graceful r.status='error' return.
- Records abortAction-skipped-onchain history note on protected
requests.
- Fix 4 (specOpNoSendActions.postProcess pre-filter):
- Skips per-row when chain says 'mined'; returned row's
tx.status stays 'nosend' (not blanket-set to 'failed').
- Processes per-row when chain says 'unknown' (off-chain abort
fires normally).
- Mixed page: aborts off-chain rows, skips chain-known rows,
NO bail-out mid-page when chain-known row is encountered.
- Conservatively protects ALL candidate rows when
getStatusForTxids throws.
- Conservatively protects ALL candidate rows on graceful
r.status='error' return.
- Fix 1 (mergedInternalize lifecycle advance):
- BUMP-absent path: nosend→unproven on transactions,
nosend→unmined on proven_tx_req; history note recorded.
- End-to-end regression: after lifecycle advance, abortAction
refuses the (formerly nosend) tx via the existing
unAbortableStatus gate (Layer 1 of the defense).
- Note: full BUMP-present path coverage relies on the existing
internalizeAction.a.test.ts integration suite (which exercises
the proof-creation path via real AtomicBEEF). Adding standalone
BUMP coverage here would duplicate fixture work.
Service mocking pattern follows test/storage/markStaleInputsAsSpent.test.ts
(the test added with PR bsv-blockchain#113). Tests build cleanly; full jest
execution is via CI (Node 24.x matches the workflow).
… before throwing The chain-status refusal in StorageProvider.abortAction was writing the forensic 'abortAction-skipped-onchain' history note AND throwing WERR_INVALID_PARAMETER inside the same storage transaction. The throw rolled the transaction back, discarding the note — the protection still refused the abort, but the audit trail a future operator would grep for these refusals never persisted. Restructure the refusal branch to return a typed sentinel from the transaction callback instead of throwing inside it. The transaction commits the history note, then the surrounding await-site inspects the sentinel and throws the original WERR_INVALID_PARAMETER with the same message — so external callers see identical behavior while the forensic note is durably written. Verified: npx jest test/storage/nosendLifecycleDefense.test.ts test/monitor/processNewBlockHeader.test.ts --runInBand under Node 24.x — 15/15 pass, including the previously-failing 'records an abortAction-skipped-onchain history note on the proven_tx_req' case.
|
Comments on the four fixes:
|
|
On TaskCheckNoSend optimization: A primary use case for How so ever TaskCheckNoSend is optimized, this use case must remain efficient. |
Addresses tonesnotes's review feedback on PR bsv-blockchain#122: the WT-4 checkNow wire-up made TaskCheckNoSends fire on every new block header, but the task does no filtering on the rows it scans, so an active batched-tx workflow (chained createAction({noSend:true, sendWith:[...]}) builds broadcast all-at-once by the terminator) gets every participating row chain-checked via getMerklePath on each block trigger. Those rows are deliberately not yet on chain; the calls return "not found" and waste network round-trips at ~144×/day on mainnet vs the prior daily-cadence behavior. A simple `wasBroadcast = true` filter does not work — `nosend` covers both batched-pending and externally-broadcast cases (per the task's existing docstring), with `wasBroadcast` false for both. Fix: when TaskCheckNoSends.runTask is invoked on the checkNow path, skip rows whose `created_at` is more recent than `checkNowFreshnessSkipMsecs` (default 5 minutes). The scheduled daily cadence is unfiltered, so externally-broadcast unmined nosend txs are still recognized eventually regardless of age, and the next block-triggered run after a row ages past the threshold catches it. Filter is applied on rows returned from `findProvenTxReqs` — no storage-API change required. Regression coverage in test/monitor/taskCheckNoSendsFreshnessGate.test.ts: - checkNow + fresh row → SKIPPED (no getMerklePath call) - checkNow + old row → CHECKED - checkNow + mixed ages → only old txids checked - daily cadence + fresh row → CHECKED (filter only applies to checkNow) - checkNow + row exactly at boundary → CHECKED (filter uses <= cutoff) Targeted suite verified: npx jest test/monitor/taskCheckNoSendsFreshnessGate.test.ts test/storage/nosendLifecycleDefense.test.ts test/monitor/processNewBlockHeader.test.ts --runInBand under Node 24.15.0 — 20/20 pass (5 new + 15 previously-existing).
…ion docstrings Two of tonesnotes's PR bsv-blockchain#122 review asks, bundled: 1. Expand the freshness gate in 90db153 into a full tiered aging schedule on the checkNow path (Tone Comment 2: "an aging system where the re-try interval grows with the age of the transaction"). Schedule: age < 5 min → SKIP (in-flight batch protection) 5 min ≤ age < 1 hr → every checkNow trigger 1 hr ≤ age < 24 hr → ~hourly (block_height % 6 === 0) 24 hr ≤ age < 7 days → ~daily (block_height % 144 === 0) age ≥ 7 days → ~weekly (block_height % 1008 === 0) Block-height modulo gives a deterministic, stateless way to spread checks for older rows; no per-row "last-checked" persistence is required. The scheduled daily cadence (no checkNow) is unaffected — still scans every row, serving as the fallback that guarantees externally-broadcast nosend txs are eventually recognized even if the aging schedule defers them. Decision logic is a static `shouldCheckOnCheckNow(createdAt, nowMs, blockHeight)` helper for testability. Tier thresholds are static readonly constants so they're tunable in one place. 2. Expand the internalizeAction docstring (Tone Comment 1: "Expand the 'must be unproven or completed' to correctly describe what gets implemented"). Both the storage-layer and signer-layer internalizeAction docs now describe the per-status merge behavior explicitly: 'unproven' / 'completed' → merge outputs, status unchanged 'nosend' → merge AND advance lifecycle (treats the call as authorization to broadcast; keeps semantics consistent across same-storage vs different-storage originators per Tone's framing) any other → error Regression tests in test/monitor/taskCheckNoSendsFreshnessGate.test.ts expanded from 5 to 11 cases covering every tier boundary plus a mixed-tier scenario on a single block. Targeted suite under Node 24.15.0: 26/26 pass (11 new + 15 previously-existing across taskCheckNoSendsFreshnessGate + nosendLifecycleDefense + processNewBlockHeader).
Codex review on bc5f067 caught that the aging schedule reduces overall frequency but synchronizes same-tier rows on the same block. For a wallet with N tier-T rows and tier interval K, the previous logic fired all N rows on every K-th block (a burst) rather than ~N/K rows per block (a spread). The docstring already promised spreading; the implementation didn't deliver it. Fix: include `provenTxReqId` in the modulo decision so each row has its own offset within the cycle: (currentBlockHeight + provenTxReqId) % tierInterval === 0 Different rows in the same tier have different residues (auto-increment IDs are dense), so they fire on different blocks within the tier's cycle. Total fire rate per row is unchanged. No new persistence required — the row's existing provenTxReqId provides the offset. Test coverage: existing tier 2/3/4 cases reworked to derive block-height from the seeded row's `provenTxReqId` (via new `eligibleBlockHeight` / `ineligibleBlockHeight` helpers). One new test case explicitly verifies two same-tier rows fire on different blocks — seeds two tier-2 rows with consecutive IDs, picks a block where one is eligible and the other isn't, asserts only the eligible one hits `getMerklePath`. Targeted Jest suite under Node 24.15.0: 27/27 pass (12 in the TaskCheckNoSends file: 11 from bc5f067 + 1 new staggering case; 15 in nosendLifecycleDefense; 2 in processNewBlockHeader).
Per BRC-100 review feedback on PR bsv-blockchain#122 from @tonesnotes (github.com/bsv-blockchain/pull/122 comment 4444566147 item 3): the wallet must be able to refuse an abort when the underlying transaction is already on chain, and that refusal must come via the method return value rather than a thrown error. This widens AbortActionResult.aborted from the literal `true` to `boolean` so wallets can return `{ aborted: false }` when chain-status confirmation positively identifies the transaction as mined or known to mempool. The recommended caller follow-up on refusal is `internalizeAction`, which treats the call as explicit authorization to advance the nosend lifecycle. Service-unreachable conditions still return `aborted: true` — refusal is reserved for positive on-chain confirmation. The companion wallet-toolbox change documents the offline-fallback audit-trail mechanism. Two existing internal callsites already declared `Promise<{ aborted: true }>` return types (WalletClient.abortAction, window.CWI.abortAction); both widened to `Promise<{ aborted: boolean }>` for type consistency. Refs: bsv-blockchain#122 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ed nosend and offline-fallback Per @tonesnotes review on PR bsv-blockchain#122 (comment 4444566147 items 3 + 4): abortAction must signal refusal via the return value rather than a thrown error, and abort must remain possible when network confirmation is impossible. This pivots both the single-row `StorageProvider.abortAction` path and the bulk `specOpNoSendActions.postProcess` path. StorageProvider.abortAction (single-row): - Positive on-chain confirmation (mined / known) now returns { aborted: false } instead of throwing WERR_INVALID_PARAMETER. The forensic 'abortAction-skipped-onchain' history note is preserved. - Service-unreachable (getStatusForTxids throws or returns r.status !== 'success') is no longer treated as conservative-refuse. The wallet proceeds with the abort and writes a new 'abortAction-offline-fallback' history note for audit. Refusal is reserved for positive on-chain confirmation. specOpNoSendActions.postProcess (bulk): - Pre-filter still does the batched getStatusForTxids call but now only protects rows with positive mined/known status; service failure in the batched call leaves protectedTxids empty and falls through to per-row abortAction calls (which apply their own offline-fallback policy). - Loop body now checks per-row `result.aborted` and only stamps tx.status = 'failed' when the inner call returned true. Race-window rows that became chain-known between pre-filter and per-row call return aborted:false and have their tx.status left as 'nosend'. Tests (test/storage/nosendLifecycleDefense.test.ts): - Four existing Fix 3 throw-assertions pivoted to return-value assertions (`expect(result.aborted).toBe(false)`). - Two existing Fix 4 service-unreachable "protects ALL" assertions pivoted to "proceeds with per-row aborts". - New Fix 3 case: PROCEEDS WITH ABORT on getStatusForTxids throw with abortAction-offline-fallback audit-note check. - New Fix 3 case: same for graceful r.status='error' service failure. - New Fix 4 case: mid-page race window — pre-filter sees 'unknown', per-row call sees 'mined', tx stays as 'nosend' and abortAction-skipped-onchain note is written. - seedNoSendTx helper now accepts an optional shared user so multi-row bulk tests can validate actual same-page processing. Test counts: 28/28 pass under Node 24.15.0 on the targeted PR bsv-blockchain#122 lane (taskCheckNoSendsFreshnessGate 12 + nosendLifecycleDefense 14 + processNewBlockHeader 2). Refs: bsv-blockchain#122 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Required for the AbortActionResult widening in 3c2ecbf to typecheck end-to-end. Three packages declared @bsv/sdk via the registry specifier "^2.1.0" and therefore resolved to a published SDK whose AbortActionResult still carries the literal `{ aborted: true }`. That broke type identity in wallet-toolbox's StorageServer.ts where this.wallet (which returns { aborted: boolean } after the widening) is passed to createAuthMiddleware and createPaymentMiddleware (whose WalletInterface bakes in the older literal). Codex review 53e5782a226d623b flagged this MEDIUM. Switch wallet-toolbox + auth-express-middleware + payment-express-middleware from "@bsv/sdk": "^2.1.0" to "@bsv/sdk": "workspace:*", matching the pattern already used by conformance/runner/ts. The lockfile re-resolves all three to link:.../packages/sdk so the local SDK source widening propagates through the monorepo's build graph without manual symlink surgery. Verification: cd packages/wallet/wallet-toolbox && pnpm build is now clean (was failing with TS2322 cross-SDK type-identity errors at StorageServer.ts lines 126/132). Targeted Jest lane still passes 28/28 under Node 24.15.0 (taskCheckNoSendsFreshnessGate 12 + nosendLifecycleDefense 14 + processNewBlockHeader 2). SDK package builds clean. Refs: bsv-blockchain#122 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Thank you Tone — really good feedback. Worked through all four. 1. 2. 3. 4. Build-graph follow-up — the SDK widening required switching Test coverage: 28/28 pass on the targeted lane ( |
Directly addressed by the The class docstring at |
|
Updates are consistent with issues raised in my earlier comments. Can't speak to the build-graph choices as I'm not current on the release process. I am not currently listed as a reviewer, not sure if that was intentional but will defer at this time to @sirdeggen for merge approval. |
…defense # Conflicts: # packages/wallet/wallet-toolbox/CHANGELOG.md # packages/wallet/wallet-toolbox/package.json # pnpm-lock.yaml
Not intentional, no. It's more a case of the auto stuff will pick up CODEOWNERS which I defaulted to me unless otherwise specified. May I add you as codeowner for wallet-toolbox? Still getting used to the new repo structure. Happy to discuss. Roughly speaking you can tag main branch with |
…consumers Two CI failures on PR bsv-blockchain#122 came from the same root cause as a2f8244: some packages in wallet-toolbox's build graph were still pulling @bsv/sdk via the registry specifier "^2.1.0", producing a second SDK copy in node_modules and breaking type identity at StorageServer.ts:126/132 (Wallet ≠ WalletInterface across the two SDK copies). Workspace links extended: - wallet-toolbox/package.json: middleware deps now workspace:* too (the published 2.0.5/2.0.2 middlewares carry their own @bsv/sdk@2.1.0 transitive, which is what was still polluting the wallet-toolbox/node_modules resolution). - wallet-toolbox/client + mobile/package.json: SDK now workspace:* so tsc --build's project references for tsconfig.client.json / tsconfig.mobile.json no longer drag in a second SDK. SonarCloud Quality Gate (16.1% duplication on new code, required ≤3%): - test/utils/TestUtilsWalletStorage.ts: new createFreshSQLiteStorage helper consolidates the dropAllData + migrate + makeAvailable boilerplate that nosendLifecycleDefense.test.ts and markStaleInputsAsSpent.test.ts both inlined (Sonar's largest cross- file dup block: 84 lines on one side, 65 on the other). - test/storage/nosendLifecycleDefense.test.ts: extracted expectStatuses, getTxStatus, getReq, runBulkAbort, countHistoryNotes, and the ABORT_LABELS constant to collapse the repeated assertion + listActions + history-note query patterns Sonar flagged as internal duplication (5 internal dup blocks, 117 total dup lines). - src/storage/methods/internalizeAction.ts: extracted findOrInsertProvenTxFromBump (the 26-line bump→provenTx block duplicated between mergedInternalize and newInternalize at lines 384-410 vs 448-474). Verification: cd packages/wallet/wallet-toolbox && pnpm build is clean. Targeted Jest lane still passes 37/37 under Node 24.15.0 (taskCheckNoSendsFreshnessGate 12 + nosendLifecycleDefense 14 + markStaleInputsAsSpent 9 + processNewBlockHeader 2). Refs: bsv-blockchain#122 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
CI test step for payment-express-middleware failed with the same cross-SDK type identity error pattern as before: its dev/test build pulled @bsv/auth-express-middleware@2.0.5 from the npm registry, which in turn transitively dragged in @bsv/sdk@2.1.0 via .pnpm/@bsv+auth-express-middleware@2.0.5/node_modules/@bsv/sdk. That second SDK copy collided with the workspace SDK at src/__tests/testExpressServer.ts:42 (`wallet: mockWallet` vs the AuthMiddlewareOptions.wallet WalletInterface declared via the registry SDK). Flip the devDependency to workspace:* so payment-express-middleware test compilation resolves auth-express-middleware (and therefore @bsv/sdk) through the workspace, eliminating the type-identity gap. Verification: cd packages/middleware/payment-express-middleware && npm run build is clean; npx jest --runInBand passes 1/1 (the existing integration suite). Refs: bsv-blockchain#122 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|



fix(wallet-toolbox): close nosend orphan-output failure mode
Summary
Closes a fund-loss-class failure mode where a
nosendtransaction that has been externally broadcast and confirmed on chain gets transitioned toproven_tx_reqs.status='invalid'+transactions.status='failed'by wallet-toolbox's own state machine, orphaning every output the tx produced (including auto-fund change outputs the wallet itself emitted).Four independent issues combine to make this failure mode reachable. Each fix below is necessary but not sufficient alone; all four together close the demonstrated failure path (and any failure ordering involving accurate chain-status answers — see "Residual edge cases" near the end of this PR for the one case the four-fix bundle does not cover).
In one production wallet this failure mode fired 8 times in 6 days, orphaning 1,146,053 sats of BSV change across 8 transactions — recoverable only via direct SQLite UPDATE on the wallet's storage. The largest single incident orphaned 498,510 sats from 8 derivation addresses; the user thought their funds were lost until chain-verification confirmed the keys + outpoints were intact and the bug was in wallet bookkeeping, not anywhere else. Full recurrence audit below.
Architectural context
We're aware that wallet-toolbox is in mid-transition to the Teranode / Arcade / Merkle-service ecosystem, that
go-wallet-toolboxis in flight as the canonical reference, and that maintainer attention is reasonably split between tactical fixes to the TypeScript stack and the larger architectural redesign. This PR is positioned as a tactical fix on the current TypeScript wallet-toolbox:services.getStatusForTxidsis reshaped to source from Merkle-service in the new architecture, the fix shape — "don't invalidate a tx the chain already knows about" — will port. The new tests will port too.validateTxStatusForAbortallow-list gap.go-wallet-toolbox/pkg/storage/internal/actions/abort.gopermits abortingNoSendwithout chain-check, same as TS. Out of scope for this PR; flagging it here in case maintainers want to track the parallel fix.The four fixes
mergedInternalizelifecycle advance (storage/methods/internalizeAction.ts:327-341) — wheninternalizeActionis called against a tx already innosendstatus, retire the nosend req (transitionproven_tx_req.statusout ofnosend+ add a history note) instead of silently no-op'ing on the lifecycle.TaskCheckNoSends.checkNowwire-up (monitor/Monitor.ts:434) — nudgeTaskCheckNoSendson every new block header, parallel to the existingTaskCheckForProofs.checkNownudge. The hook was designed but never wired up.abortActionchain-status check (storage/StorageProvider.ts:280-287) — refuse to invalidate anosendreq whose txid isminedorknown(mempool-aware) perservices.getStatusForTxids.specOpNoSendActions.postProcesspre-filter (storage/methods/ListActionsSpecOp.ts:50-70) — chain-check rows before bulk-abort so Fix 3's throw never fires from the bulk-recovery path, and so returned rows reflect actual outcomes per-row (not blanket-failed).Each fix is line-anchored, mechanically simple, and independently testable. Combined, they form a layered defense:
internalizeActionretires the nosend lifecycle immediately. Subsequent abortAction calls see the tx inunproven/completedstatus, which is inunAbortableStatus(StorageProvider.ts:273), and refuse.internalizeAction, or it threw before reaching the merge path),TaskCheckNoSendsnow fires per-block and retires the nosend req within minutes of confirmation. SameunAbortableStatusprotection then applies.abortActiondirectly inside the same block as the broadcast, or against a tx that's still mempool-only), the new chain-status check insideabortActionitself refuses to invalidate any tx whose status isminedorknown.specOpNoSendActionspre-filters chain-known rows so Layer 3's throw never fires mid-page from bulk recovery, and so returned row statuses reflect per-row outcomes accurately.Any of Fixes 1, 2, or 3 alone closes the most common failure path. All four together close the failure path the recurrence audit demonstrates — including edge cases (broadcast in same block as abort click, intermittent wallet uptime causing missed Monitor cycles, callers that omit
internalizeActionentirely, and bulk-abort recovery affordances that should still work for genuinely off-chain rows). One residual edge case is documented near the end of this PR.Background — the failure mode walkthrough
The incident sequence reproducible from the evidence DB:
Caller invokes
wallet.createAction({noSend: true, ...}). Storage writes a newtransactionsrow withstatus='nosend'and a newproven_tx_reqsrow withstatus='nosend'. History:{what: 'status', status_was: 'unknown', status_now: 'nosend', when: T0}.Caller signs the tx (via
signAction) and broadcasts externally — e.g., via a multi-source broadcast ladder, ARC, or any non-wallet broadcaster. The tx confirms on chain at block H within minutes.Caller invokes
wallet.internalizeAction({tx: signedTx.toAtomicBEEF(), outputs: [...], description: '...'})to inform the wallet. The storage-layerinternalizeAction.ts:206allows the merge path for tx already innosendstatus. ButmergedInternalize(lines 327-341) only touches labels + outputs — it never updatestransactions.status, never updatesproven_tx_reqs.status, never adds a history note. Issue chore(deps): bump pnpm/action-setup from 3 to 6 #1: silent no-op on lifecycle.Hours later (or minutes — any time after step 3), the user clicks an "abort pending action" affordance in any wallet UI, OR an app calls
wallet.abortAction({reference: txid})orlistActions({labels:[specOpNoSendActions, 'abort']})for bulk recovery. The storage-layerabortActionat line 273 allowsnosend(it's only forbidden forcompleted | failed | sending | unproven). The function then unconditionally transitions:transactions.status→'failed'(line 280)proven_tx_reqs.status→'invalid'(line 285) — terminal state perProvenTxReqTerminalStatusinsdk/types.tsIssue chore(deps): bump actions/setup-node from 4 to 6 #2: no chain-status check before invalidating. When called via the bulk-recovery
specOpNoSendActions.postProcess(ListActionsSpecOp.ts:50-70), the spec-op iterates rows and unconditionally force-setstx.status = 'failed'in the returned page regardless of what the per-row abort actually did. Issue Implementation tooling visibility #4: bulk-recovery spec-op forces blanket failure status on returned rows.All outputs of the tx are now excluded from
listOutputsKnex.ts:128'stxStatusAllowed = ['completed', 'unproven', 'nosend', 'sending']filter.specOpWalletBalancereports them as if they don't exist. The allocator can't pick them. User sees "balance gone."Theoretically
TaskCheckNoSendswould have transitioned the nosend req tocompletedbefore step 4 — the task explicitly handlesstatus: ['nosend']reqs andgetProofsacceptsnosend(TaskCheckForProofs.ts:118). But the task'scheckNowstatic flag is never set by any external code path; the only mechanism istriggerMsecs = oneDay * 1, which combined with intermittent wallet uptime + the early-return on undefinedlastNewHeadermeans the task effectively never runs. Issue chore(deps): bump actions/checkout from 4 to 6 #3: orphaned hook.The user's bookkeeping has now diverged from chain reality, and there is no path back to a healthy state through any public wallet-toolbox API — only direct SQLite UPDATE on
transactionsandproven_tx_reqs. That's where users lose funds in their mental model, even though the keys + outpoints remain intact on chain.Fix 1 —
mergedInternalizeretires the nosend lifecycleFile:
packages/wallet/wallet-toolbox/src/storage/methods/internalizeAction.tsLines:
327-341(currentmergedInternalizemethod body)Current behavior: when
internalizeActionis invoked on a tx already in storage withstatus='nosend', the merge path adds labels + records output ownership but never updatestransactions.statusorproven_tx_reqs.status. The caller has no way to know the lifecycle wasn't advanced.Proposed change (sketch — exact diff TBD per maintainer feedback):
Open question for maintainers: there's a doc-string inconsistency at signer-layer
internalizeAction.ts:19which says "must be unproven or completed" — contradicting the storage code's permissive line 206 which allows nosend. Should the signer's doc-string be updated to reflect that nosend is also a valid merge target (since this PR is explicitly making nosend retirement reliable)? Or is the doc-string the canonical intent and merge should be restricted to non-nosend statuses, requiring callers to use a different API for nosend retirement? Either shape is implementable; the user-friendlier one is to allow merge and advance lifecycle (what this PR does).Fix 2 —
TaskCheckNoSends.checkNowwire-upFile:
packages/wallet/wallet-toolbox/src/monitor/Monitor.tsLine:
434(insideprocessNewBlockHeader)Current behavior:
processNewBlockHeadersetsTaskCheckForProofs.checkNow = truebut notTaskCheckNoSends.checkNow. The TaskCheckNoSends.ts source comment at line 22-25 documents the intended design ("An external service such as the chaintracks new block header listener can set this true") but no code anywhere in the repo actually does it.Proposed change:
That's the entire fix.
TaskCheckNoSends.runTaskalready handles the work correctly — queriesproven_tx_reqswithstatus: ['nosend'], callsgetProofs, transitions tocompletedon proof success. The dailytriggerMsecscadence stays as a fallback.Empirical confirmation from the evidence DB: in 8 days of
monitor_events(5,681 records), zeroCheckNoSendsevents fired. Other tasks fired normally: Clock (2649), ReviewProvenTxs (2215), NewHeader (534), MonitorCallHistory (236), FailAbandoned (13), SendWaiting (12), CheckForProofs (6), ReviewDoubleSpends (4), ReviewStatus (3). The 6 CheckForProofs events demonstrate the per-block nudge mechanism works correctly when wired; the 0 CheckNoSends events demonstrate it's not wired for the parallel task. A grep across the source tree forTaskCheckNoSends.checkNowreturns exactly three sites — all insideTaskCheckNoSends.tsitself (the static-field declaration plus the read/clear insidetriggerandrunTask). No external setter exists.Fix 3 —
abortActionchain-status checkFile:
packages/wallet/wallet-toolbox/src/storage/StorageProvider.tsLines:
273-287(insideabortAction)Current behavior: when caller invokes
abortAction({reference})on a tx withstatus='nosend'ANDtx.txidset, the function unconditionally:transactions.status→'failed'(line 280)proven_tx_reqs.status→'invalid'(line 285)No check that the txid might be known to the network already (in which case the abort is destructive: it inverts the wallet's bookkeeping relative to chain reality and orphans every output the tx produced).
Proposed change (sketch):
Four explicit choices in this sketch, called out so they're easy to push back on:
'mined' | 'known' | 'unknown', matchingWalletServices.interfaces.ts:352.'mined' OR 'known'. A tx that's been broadcast and is in mempool returns'known'(depth === 0); guarding only'mined'would still allow the bug to fire during the propagation window (typically several minutes between broadcast and first confirmation).'known'(conservative refuse), not as'unknown'(proceed with abort).GetStatusForTxidsResulthas two failure modes — the call can throw, OR it can return with top-levelstatus === 'error'and emptyresults[]. The explicitif (r.status !== 'success')branch closes the silent fall-through that would otherwise proceed-with-abort.Companion fix to bulk-abort path (see Fix 4 below) — by itself, this
abortActionthrow insideStorageProviderwould causespecOpNoSendActions.postProcessto bail mid-page on the first chain-known nosend row, leaving genuinely off-chain nosend rows in the same page un-aborted. Fix 4 pre-filters the page so the throw never fires from bulk-abort.Open question for maintainers: throw vs. auto-recover? Draft prefers the throw shape (more conservative, surfaces divergence to caller, gives the bulk-abort spec-op pre-filter something to detect). Auto-recovery (silently transitioning to
unprovenand queuing for proof fetch) is more user-friendly but more invasive. Either is implementable.Fix 4 —
specOpNoSendActions.postProcesspre-filters chain-known rowsFile:
packages/wallet/wallet-toolbox/src/storage/methods/ListActionsSpecOp.tsLines:
50-70(specOpNoSendActions.postProcess)Current behavior: when
listActions({labels: [specOpNoSendActions, 'abort'], ...})is called, the spec-op'spostProcessiterates every matched nosend row and callsstorage.abortAction({reference: tx.reference!})per row, then unconditionally force-setstx.status = 'failed'in the returned page (lines 62-67):Two problems given Fix 3's throw behavior:
abortActionthrows on a chain-known nosend row, theawaitpropagates the throw out of the for-loop, aborting the entire bulk call. Subsequent rows in the same page (including genuinely off-chain nosend rows that SHOULD be aborted) never get processed.tx.status = 'failed'for the rows it skipped — lying to the caller about what happened.Proposed change:
Result: callers iterating a page of nosend rows now see accurate per-row outcomes —
nosendfor skipped rows (Monitor will retire) andfailedfor genuinely off-chain rows the abort processed. Bulk recovery affordances (wallet UI, scripts) keep working for the rows that should be processed, instead of bailing on the first chain-known row.Recurrence audit — evidence this is a systemic, not isolated, bug class
A 37-row sweep of
transactions.status='failed' AND txid IS NOT NULLrows in the evidence wallet DB shows 8 of 37 are confirmed on chain via WhatsOnChain, with every single one following the identicalunknown → nosend → abortAction → invalidtrajectory inproven_tx_reqs.history. The 8 incidents span 2026-05-05 through 2026-05-11 — six days of repeated firings in one wallet, all closeable retroactively by this 4-fix bundle. The 29 remaining "failed" rows are genuinely off-chain (the wallet was correct in marking those failed).The 8 confirmed-on-chain orphaned txs:
fd8853b7…dcdca5eb525c938f…daa45d8c7fd1a788…b7fe4a8afd41177b…4a38742d8b28734…6576b77b77fb25e…f9dbf81ec068fabc5…20ce0b346096a9252…49a22bfedThe pattern is the same across all 8: caller used
createAction({noSend:true}), broadcast externally, calledinternalizeActionpost-broadcast (which silently no-op'd on lifecycle — Issue #1),TaskCheckNoSendsnever fired (Issue #3), and eventually some abort path fired which invalidated the chain-confirmed tx (Issue #2 / #4).Evidence pack
Available on request to maintainers. Contents:
329e5e7a2d0e955a05515b9d6fc7ba3aa3ccb215c290e53950ac914412d8e1a5.9624002b5fe864b5e037169a792eae2ab9b78e5c866c07a5ae094f94763b1a6d.Wallet identity in the evidence: pubkey
02d29584e1f8fadcb4ef525e922debcde346fed99c6ac3cde235f276ae2d2ec85d. All chain references in the audit table are independently verifiable on mainnet.Tests
Existing wallet-toolbox unit tests in
storage/methods/__test/andmonitor/tasks/__tests/continue to pass unchanged. New tests required as part of this PR:abortActionrefuses to invalidate a nosend tx thatgetStatusForTxidsreports as'mined'(mock the service to return{status:'success', results:[{txid, status:'mined', depth:6}]}).abortActionrefuses to invalidate a nosend tx thatgetStatusForTxidsreports as'known'(results:[{txid, status:'known', depth:0}]).abortActionproceeds normally for a nosend tx thatgetStatusForTxidsreports as'unknown'(results:[{txid, status:'unknown', depth:undefined}]).abortActionconservatively refuses (treats as'known') whengetStatusForTxidsTHROWS.abortActionconservatively refuses (treats as'known') whengetStatusForTxidsreturns{status:'error', results:[]}gracefully (graceful service-level failure).mergedInternalizeagainst a nosend tx transitions bothtransactions.statusandproven_tx_reqs.statuscorrectly, with and without a BUMP in the BEEF.specOpNoSendActions.postProcessskips per-row when chain says'mined'or'known', processes per-row when'unknown', leaves skipped rows withstatus='nosend'in the returned page (not blanket-failed).specOpNoSendActions.postProcessconservatively protects ALL rows whengetStatusForTxidsthrows AND when it returns{status:'error', results:[]}(both service-failure modes).specOpNoSendActions.postProcessdoesn't bail mid-page when a chain-known row is encountered — subsequent off-chain rows are still processed.processNewBlockHeadersets bothTaskCheckForProofs.checkNowandTaskCheckNoSends.checkNow.Residual edge case
One narrow case is not closed by this bundle: if
services.getStatusForTxidsreturnsstatus='success'ANDresults[i].status='unknown'for a txid that is in fact on chain (e.g., the indexer hasn't yet ingested the new block, or it's a transient cache miss for a tx that's still propagating), Fix 3 treats the chain-check as authoritative and proceeds with the abort. That destroys bookkeeping for the on-chain tx anyway.Mitigations available without changing this PR's scope:
TaskCheckNoSendsnudge gives a self-heal opportunity: once the indexer ingests the block, the nextTaskCheckNoSendsrun will transition the (nowfailed/invalid) req via the existingunfailrecovery path if a caller invokes it.abortAction).Maintainer-side hardening that would close this case (out of scope for this PR but worth flagging):
'mined'/'known') as protective.'unknown'based on tx age: if the tx was created viacreateAction({noSend:true})within the last N seconds and the chain says'unknown', treat as'known'(it's likely still propagating).Both belong to a larger "chain-status confidence" design conversation, out of scope for this PR.
Scope and limitations
This PR specifically does NOT:
triggerMsecscadence for any task. The per-block nudge added in Fix 2 makes the daily fallback effectively unused in normal operation, but the schedule is left intact as a safety net.TaskReviewUtxospolicy (the open follow-up from PR fix(wallet-toolbox): evict confirmed-stale inputs on broadcast failure #113's review).internalizeActionallowingfailedstatus as a merge target (would close another theoretical failure class, but no concrete evidence in this dataset). Separate followup if the maintainers want it.getStatusForTxidsAPI or status taxonomy. The'mined' | 'known' | 'unknown'vocabulary is left as-is.go-wallet-toolbox. SamevalidateTxStatusForAbortgap exists there; flagged for separate consideration.Coordination with PR #113
PR #113 (
fix/auto-evict-stale-utxos-on-doublespend) merged on 2026-05-11, so this PR can be opened directly without coordination wait. The two PRs are cleanly separable in concern:No file overlap between the two PRs (
#113is instorage/methods/attemptToPostReqsToNetwork.ts; this PR is instorage/StorageProvider.ts,storage/methods/internalizeAction.ts,storage/methods/ListActionsSpecOp.ts, andmonitor/Monitor.ts).