Skip to content

fix(wallet-toolbox): close nosend orphan-output failure mode#122

Merged
sirdeggen merged 12 commits into
bsv-blockchain:mainfrom
BSVanon:fix/nosend-lifecycle-defense
May 19, 2026
Merged

fix(wallet-toolbox): close nosend orphan-output failure mode#122
sirdeggen merged 12 commits into
bsv-blockchain:mainfrom
BSVanon:fix/nosend-lifecycle-defense

Conversation

@BSVanon

@BSVanon BSVanon commented May 12, 2026

Copy link
Copy Markdown
Contributor

fix(wallet-toolbox): close nosend orphan-output failure mode

Summary

Closes a fund-loss-class failure mode where a nosend transaction that has been externally broadcast and confirmed on chain gets transitioned to proven_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-toolbox is 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:

  • The bug class is architecture-independent. State-machine inversion (wallet bookkeeping disagreeing with chain reality) is a property of the wallet's lifecycle state design, not the underlying broadcast/proof-fetch service abstractions. Even as services.getStatusForTxids is 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.
  • The Go equivalent has the same validateTxStatusForAbort allow-list gap. go-wallet-toolbox/pkg/storage/internal/actions/abort.go permits aborting NoSend without chain-check, same as TS. Out of scope for this PR; flagging it here in case maintainers want to track the parallel fix.
  • We're flexible on shape. If maintainers prefer to (a) merge this tactically and let it be re-shaped during the migration, (b) wait for the architecture pivot and design the migration-aware fix together, or (c) split this PR into atomic single-fix PRs for easier review, any of the three works. The default proposal below is (a).

The four fixes

  1. mergedInternalize lifecycle advance (storage/methods/internalizeAction.ts:327-341) — when internalizeAction is called against a tx already in nosend status, retire the nosend req (transition proven_tx_req.status out of nosend + add a history note) instead of silently no-op'ing on the lifecycle.
  2. TaskCheckNoSends.checkNow wire-up (monitor/Monitor.ts:434) — nudge TaskCheckNoSends on every new block header, parallel to the existing TaskCheckForProofs.checkNow nudge. The hook was designed but never wired up.
  3. abortAction chain-status check (storage/StorageProvider.ts:280-287) — refuse to invalidate a nosend req whose txid is mined or known (mempool-aware) per services.getStatusForTxids.
  4. specOpNoSendActions.postProcess pre-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:

  • Layer 1 (Fix 1): post-broadcast internalizeAction retires the nosend lifecycle immediately. Subsequent abortAction calls see the tx in unproven / completed status, which is in unAbortableStatus (StorageProvider.ts:273), and refuse.
  • Layer 2 (Fix 2): if Layer 1 was missed (caller never called internalizeAction, or it threw before reaching the merge path), TaskCheckNoSends now fires per-block and retires the nosend req within minutes of confirmation. Same unAbortableStatus protection then applies.
  • Layer 3 (Fix 3): if Layers 1+2 are bypassed (e.g., user-facing UI calls abortAction directly inside the same block as the broadcast, or against a tx that's still mempool-only), the new chain-status check inside abortAction itself refuses to invalidate any tx whose status is mined or known.
  • Layer 3 bulk-recovery integration (Fix 4): specOpNoSendActions pre-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 internalizeAction entirely, 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:

  1. Caller invokes wallet.createAction({noSend: true, ...}). Storage writes a new transactions row with status='nosend' and a new proven_tx_reqs row with status='nosend'. History: {what: 'status', status_was: 'unknown', status_now: 'nosend', when: T0}.

  2. 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.

  3. Caller invokes wallet.internalizeAction({tx: signedTx.toAtomicBEEF(), outputs: [...], description: '...'}) to inform the wallet. The storage-layer internalizeAction.ts:206 allows the merge path for tx already in nosend status. But mergedInternalize (lines 327-341) only touches labels + outputs — it never updates transactions.status, never updates proven_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.

  4. 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}) or listActions({labels:[specOpNoSendActions, 'abort']}) for bulk recovery. The storage-layer abortAction at line 273 allows nosend (it's only forbidden for completed | failed | sending | unproven). The function then unconditionally transitions:

    • transactions.status'failed' (line 280)
    • proven_tx_reqs.status'invalid' (line 285) — terminal state per ProvenTxReqTerminalStatus in sdk/types.ts

    Issue 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-sets tx.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.

  5. All outputs of the tx are now excluded from listOutputsKnex.ts:128's txStatusAllowed = ['completed', 'unproven', 'nosend', 'sending'] filter. specOpWalletBalance reports them as if they don't exist. The allocator can't pick them. User sees "balance gone."

  6. Theoretically TaskCheckNoSends would have transitioned the nosend req to completed before step 4 — the task explicitly handles status: ['nosend'] reqs and getProofs accepts nosend (TaskCheckForProofs.ts:118). But the task's checkNow static flag is never set by any external code path; the only mechanism is triggerMsecs = oneDay * 1, which combined with intermittent wallet uptime + the early-return on undefined lastNewHeader means 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 transactions and proven_tx_reqs. That's where users lose funds in their mental model, even though the keys + outpoints remain intact on chain.

Fix 1 — mergedInternalize retires the nosend lifecycle

File: packages/wallet/wallet-toolbox/src/storage/methods/internalizeAction.ts
Lines: 327-341 (current mergedInternalize method body)

Current behavior: when internalizeAction is invoked on a tx already in storage with status='nosend', the merge path adds labels + records output ownership but never updates transactions.status or proven_tx_reqs.status. The caller has no way to know the lifecycle wasn't advanced.

Proposed change (sketch — exact diff TBD per maintainer feedback):

async mergedInternalize () {
  const transactionId = this.etx!.transactionId

  await this.addLabels(transactionId)

  for (const payment of this.walletPayments) {
    if ((payment.eo != null) && !payment.ignore) await this.mergeWalletPaymentForOutput(transactionId, payment)
    else if (!payment.ignore) await this.storeNewWalletPaymentForOutput(transactionId, payment)
  }

  for (const basket of this.basketInsertions) {
    if (basket.eo != null) await this.mergeBasketInsertionForOutput(transactionId, basket)
    else await this.storeNewBasketInsertionForOutput(transactionId, basket)
  }

  // NEW: if we just merged into a nosend tx, the caller is asserting
  // this tx is now externally broadcast and the wallet should retire
  // the nosend lifecycle. Transition the proven_tx_req out of nosend
  // so Monitor's standard proof-fetching flow can take over, OR if a
  // BUMP is included in the BEEF, transition all the way to completed.
  if (this.etx!.status === 'nosend') {
    const txid = this.txid
    const bump = this.ab.findBump(txid)
    if (bump != null) {
      // BEEF includes mining proof — transition directly to completed.
      // Reuse the same `findOrInsertProvenTx` path that `newInternalize` uses.
      // [exact integration to be worked out per maintainer feedback]
    } else {
      // Mark req for re-validation; Monitor will fetch the proof.
      const req = await EntityProvenTxReq.fromStorageTxid(this.storage, txid, /* trx */)
      if (req != null && req.status === 'nosend') {
        req.addHistoryNote({ what: 'internalizeAction', userId: this.userId })
        req.status = 'unmined'
        await req.updateStorageDynamicProperties(this.storage, /* trx */)
      }
      await this.storage.updateTransaction(transactionId, { status: 'unproven' })
    }
  }
}

Open question for maintainers: there's a doc-string inconsistency at signer-layer internalizeAction.ts:19 which 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.checkNow wire-up

File: packages/wallet/wallet-toolbox/src/monitor/Monitor.ts
Line: 434 (inside processNewBlockHeader)

Current behavior: processNewBlockHeader sets TaskCheckForProofs.checkNow = true but not TaskCheckNoSends.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:

processNewBlockHeader (header: BlockHeader): void {
  const h = header
  this.lastNewHeader = h
  this.lastNewHeaderWhen = new Date()
  // Nudge the proof checker to try again.
  TaskCheckForProofs.checkNow = true
  // Nudge the nosend checker — externally-broadcast nosend txs may have
  // confirmed in this new block. The TaskCheckNoSends.checkNow hook was
  // designed for this signal but was never wired up; flipping it here
  // gives nosend reqs the same per-block proof-fetch opportunity as the
  // other lifecycle statuses TaskCheckForProofs handles.
  TaskCheckNoSends.checkNow = true
}

That's the entire fix. TaskCheckNoSends.runTask already handles the work correctly — queries proven_tx_reqs with status: ['nosend'], calls getProofs, transitions to completed on proof success. The daily triggerMsecs cadence stays as a fallback.

Empirical confirmation from the evidence DB: in 8 days of monitor_events (5,681 records), zero CheckNoSends events 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 for TaskCheckNoSends.checkNow returns exactly three sites — all inside TaskCheckNoSends.ts itself (the static-field declaration plus the read/clear inside trigger and runTask). No external setter exists.

Fix 3 — abortAction chain-status check

File: packages/wallet/wallet-toolbox/src/storage/StorageProvider.ts
Lines: 273-287 (inside abortAction)

Current behavior: when caller invokes abortAction({reference}) on a tx with status='nosend' AND tx.txid set, the function unconditionally:

  • transitions transactions.status'failed' (line 280)
  • transitions 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):

const unAbortableStatus: TransactionStatus[] = ['completed', 'failed', 'sending', 'unproven']
if ((tx == null) || !tx.isOutgoing || unAbortableStatus.findIndex(s => s === tx.status) > -1) {
  throw new WERR_INVALID_PARAMETER(
    'reference',
    'an inprocess, outgoing action that has not been signed and shared to the network.'
  )
}

// NEW: if the tx has been signed and assigned a txid, verify the
// network doesn't already know about it before invalidating. Aborting
// a tx that's already on chain (or in mempool about to mine) orphans
// its outputs in the wallet's view, including auto-fund change.
//
// `getStatusForTxids` (WalletServices.interfaces.ts:341-353) returns
// `StatusForTxidResult[]` with status ∈ {'mined' | 'known' | 'unknown'}:
//   - 'mined'   : depth > 0 (confirmed in a block)
//   - 'known'   : depth === 0 (in mempool, processor-aware)
//   - 'unknown' : depth === undefined (never processed or purged)
//
// Protect BOTH 'mined' AND 'known'. A tx that's been broadcast and
// is propagating in mempool returns 'known' — it's about to mine,
// and invalidating it would cause the same orphan-output failure as
// invalidating a mined tx.
if (tx.txid && tx.status === 'nosend') {
  const services = this.getServices()
  let chainStatus: 'mined' | 'known' | 'unknown' | undefined
  try {
    const r = await services.getStatusForTxids([tx.txid])
    // GetStatusForTxidsResult has TWO failure modes:
    //   (a) the call throws → caught below
    //   (b) the call returns with top-level `status === 'error'` and
    //       (typically) empty `results[]` → graceful failure
    // We treat BOTH as service-unreachable and conservatively refuse
    // the abort. Reading `r.results.find(...)` directly without the
    // (b) check would silently fall through with chainStatus undefined,
    // which would proceed-with-abort and re-introduce the bug.
    if (r.status !== 'success') {
      chainStatus = 'known'
    } else {
      chainStatus = r.results.find(x => x.txid === tx.txid)?.status
    }
  } catch {
    // (a) Indexer unavailable. Conservative refuse, same as (b).
    chainStatus = 'known'
  }
  if (chainStatus === 'mined' || chainStatus === 'known') {
    const req = await EntityProvenTxReq.fromStorageTxid(this, tx.txid, trx)
    if (req != null) {
      req.addHistoryNote({ what: 'abortAction-skipped-onchain', reference: args.reference, chainStatus })
      await req.updateStorageDynamicProperties(this, trx)
    }
    throw new WERR_INVALID_PARAMETER(
      'reference',
      `transaction is already on chain (status='${chainStatus}'); call internalizeAction instead of abortAction.`,
    )
  }
}

await this.updateTransactionStatus('failed', tx.transactionId, userId, reference, trx)
if (tx.txid) {
  const req = await EntityProvenTxReq.fromStorageTxid(this, tx.txid, trx)
  if (req != null) {
    req.addHistoryNote({ what: 'abortAction', reference: args.reference })
    req.status = 'invalid'
    await req.updateStorageDynamicProperties(this, trx)
  }
}

Four explicit choices in this sketch, called out so they're easy to push back on:

  1. Status vocabulary: 'mined' | 'known' | 'unknown', matching WalletServices.interfaces.ts:352.
  2. Protection set: '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).
  3. Service-unreachable on THROW: treat as 'known' (conservative refuse), not as 'unknown' (proceed with abort).
  4. Service-unreachable on graceful-error return: GetStatusForTxidsResult has two failure modes — the call can throw, OR it can return with top-level status === 'error' and empty results[]. The explicit if (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 abortAction throw inside StorageProvider would cause specOpNoSendActions.postProcess to 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 unproven and queuing for proof fetch) is more user-friendly but more invasive. Either is implementable.

Fix 4 — specOpNoSendActions.postProcess pre-filters chain-known rows

File: packages/wallet/wallet-toolbox/src/storage/methods/ListActionsSpecOp.ts
Lines: 50-70 (specOpNoSendActions.postProcess)

Current behavior: when listActions({labels: [specOpNoSendActions, 'abort'], ...}) is called, the spec-op's postProcess iterates every matched nosend row and calls storage.abortAction({reference: tx.reference!}) per row, then unconditionally force-sets tx.status = 'failed' in the returned page (lines 62-67):

for (const tx of txs) {
  if (tx.status === 'nosend') {
    await s.abortAction(auth, { reference: tx.reference! })
    tx.status = 'failed'   // <-- unconditional, even though abortAction may have changed reality differently
  }
}

Two problems given Fix 3's throw behavior:

  1. If abortAction throws on a chain-known nosend row, the await propagates 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.
  2. Even if Fix 3 is auto-recovering instead of throwing, the postProcess still force-sets tx.status = 'failed' for the rows it skipped — lying to the caller about what happened.

Proposed change:

[specOpNoSendActions]: {
  name: 'noSendActions',
  labelsToIntercept: ['abort'],
  setStatusFilter: () => ['nosend'],
  postProcess: async (
    s: StorageProvider,
    auth: AuthId,
    vargs: Validation.ValidListActionsArgs,
    specOpLabels: string[],
    txs: Array<Partial<TableTransaction>>
  ): Promise<void> => {
    if (!specOpLabels.includes('abort')) return

    // Pre-filter: which rows have txids the network already knows about?
    // Don't call abortAction for those — that would orphan outputs of
    // chain-confirmed (or about-to-mine) txs. Without this pre-filter,
    // the per-row abortAction call in the loop below would either throw
    // (per the chain-check fix in StorageProvider.abortAction) and bail
    // the bulk call halfway, or silently no-op while we still force-set
    // tx.status = 'failed' below.
    const candidates = txs.filter(tx => tx.status === 'nosend' && !!tx.txid)
    const candidateTxids = candidates.map(tx => tx.txid as string)
    const protectedTxids = new Set<string>()
    if (candidateTxids.length > 0) {
      const services = s.getServices()
      let serviceFailed = false
      try {
        const r = await services.getStatusForTxids(candidateTxids)
        if (r.status !== 'success') {
          // Graceful service-level failure (e.g., upstream provider
          // returned an error but the call did not throw). Treat as
          // unreachable per the conservative-refuse policy.
          serviceFailed = true
        } else {
          for (const result of r.results) {
            if (result.status === 'mined' || result.status === 'known') {
              protectedTxids.add(result.txid)
            }
          }
        }
      } catch {
        serviceFailed = true
      }
      if (serviceFailed) {
        // Indexer unreachable. Conservative: protect everything. Better
        // to leave nosend rows alone than risk orphaning outputs of txs
        // that may be on chain. The caller can retry when indexers are
        // available.
        for (const txid of candidateTxids) protectedTxids.add(txid)
      }
    }

    for (const tx of txs) {
      if (tx.status !== 'nosend') continue
      if (tx.txid && protectedTxids.has(tx.txid)) {
        // Skip: tx is on chain or in mempool. Leave the returned row's
        // status as 'nosend' so the caller sees clearly that this row
        // was NOT aborted. Monitor's TaskCheckNoSends (per Fix 2) will
        // retire the nosend lifecycle on the next block tick.
        continue
      }
      await s.abortAction(auth, { reference: tx.reference! })
      tx.status = 'failed'
    }
  }
}

Result: callers iterating a page of nosend rows now see accurate per-row outcomes — nosend for skipped rows (Monitor will retire) and failed for 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 NULL rows in the evidence wallet DB shows 8 of 37 are confirmed on chain via WhatsOnChain, with every single one following the identical unknown → nosend → abortAction → invalid trajectory in proven_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:

tx (short) flow abort time recoverable BSV
fd8853b7…dcdca5eb Inscribe icon (1Sat-Ord) 2026-05-05T02:02Z 631,971 change
525c938f…daa45d8c Deploy + mint BSV-21 token 2026-05-05T02:02Z 448 change
7fd1a788…b7fe4a8a BRC-29 self-pay output 2026-05-08T22:23Z 540 change + 1M output
fd41177b…4a38742 BRC-29 self-pay output 2026-05-08T22:42Z 391 change + 1M output
d8b28734…6576b77 MNEE buy vault 2026-05-09T19:39Z 1,625 change + 1,500 vault
b77fb25e…f9dbf81ec MNEE refund 2026-05-09T23:04Z 4,058 change
068fabc5…20ce0b346 AMM pool state 2026-05-10T01:58Z 8,510 change
096a9252…49a22bfed AMM pool swap 2026-05-11T15:49Z 1,490 change + 498,510 wallet change

The pattern is the same across all 8: caller used createAction({noSend:true}), broadcast externally, called internalizeAction post-broadcast (which silently no-op'd on lifecycle — Issue #1), TaskCheckNoSends never 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:

  • Pre-surgery SQLite (~2.9 MB) at the moment of the largest incident — full storage state with 1 inverted tx, 498,510 sats orphaned. SHA256 329e5e7a2d0e955a05515b9d6fc7ba3aa3ccb215c290e53950ac914412d8e1a5.
  • Pre-surgery SQLite with the remaining 7 inversions still in place (for reproduction independent of the first), SHA256 9624002b5fe864b5e037169a792eae2ab9b78e5c866c07a5ae094f94763b1a6d.
  • Read-only state dump of relevant rows from the affected wallet.
  • Line-anchored bug analysis + trigger sequence + fix shape per finding.

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/ and monitor/tasks/__tests/ continue to pass unchanged. New tests required as part of this PR:

  • Storage: abortAction refuses to invalidate a nosend tx that getStatusForTxids reports as 'mined' (mock the service to return {status:'success', results:[{txid, status:'mined', depth:6}]}).
  • Storage: abortAction refuses to invalidate a nosend tx that getStatusForTxids reports as 'known' (results:[{txid, status:'known', depth:0}]).
  • Storage: abortAction proceeds normally for a nosend tx that getStatusForTxids reports as 'unknown' (results:[{txid, status:'unknown', depth:undefined}]).
  • Storage: abortAction conservatively refuses (treats as 'known') when getStatusForTxids THROWS.
  • Storage: abortAction conservatively refuses (treats as 'known') when getStatusForTxids returns {status:'error', results:[]} gracefully (graceful service-level failure).
  • Storage: mergedInternalize against a nosend tx transitions both transactions.status and proven_tx_reqs.status correctly, with and without a BUMP in the BEEF.
  • Storage: specOpNoSendActions.postProcess skips per-row when chain says 'mined' or 'known', processes per-row when 'unknown', leaves skipped rows with status='nosend' in the returned page (not blanket-failed).
  • Storage: specOpNoSendActions.postProcess conservatively protects ALL rows when getStatusForTxids throws AND when it returns {status:'error', results:[]} (both service-failure modes).
  • Storage: specOpNoSendActions.postProcess doesn't bail mid-page when a chain-known row is encountered — subsequent off-chain rows are still processed.
  • Monitor: processNewBlockHeader sets both TaskCheckForProofs.checkNow and TaskCheckNoSends.checkNow.

Residual edge case

One narrow case is not closed by this bundle: if services.getStatusForTxids returns status='success' AND results[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:

  • Fix 2's per-block TaskCheckNoSends nudge gives a self-heal opportunity: once the indexer ingests the block, the next TaskCheckNoSends run will transition the (now failed/invalid) req via the existing unfail recovery path if a caller invokes it.
  • Callers can be conservative on their side (chain-verify across multiple indexers before calling abortAction).

Maintainer-side hardening that would close this case (out of scope for this PR but worth flagging):

  • Multi-source chain-status check: query 2 or more indexer providers and treat the inconclusive case (any of them returning 'mined'/'known') as protective.
  • Confidence interval on 'unknown' based on tx age: if the tx was created via createAction({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:

  • Modify the triggerMsecs cadence 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.
  • Add a new public wallet API for chain-status reconciliation. That's a larger design conversation; this PR is the minimum surface to close the failure mode.
  • Address TaskReviewUtxos policy (the open follow-up from PR fix(wallet-toolbox): evict confirmed-stale inputs on broadcast failure #113's review).
  • Address internalizeAction allowing failed status as a merge target (would close another theoretical failure class, but no concrete evidence in this dataset). Separate followup if the maintainers want it.
  • Change the getStatusForTxids API or status taxonomy. The 'mined' | 'known' | 'unknown' vocabulary is left as-is.
  • Touch go-wallet-toolbox. Same validateTxStatusForAbort gap 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 (#113 is in storage/methods/attemptToPostReqsToNetwork.ts; this PR is in storage/StorageProvider.ts, storage/methods/internalizeAction.ts, storage/methods/ListActionsSpecOp.ts, and monitor/Monitor.ts).

BSVanon added 2 commits May 12, 2026 12:07
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).
@BSVanon BSVanon requested a review from sirdeggen as a code owner May 12, 2026 17:07
… 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.
@tonesnotes

Copy link
Copy Markdown
Collaborator

Comments on the four fixes:

  1. internalizeAction: Fix 1 looks good. Internalizing a transaction with matching proven_tx_req in noSend state is an ambiguous situation. This is an attempt to take ownwership of an output where control of the underlying transaction was externalized. For simplicity, it makes sense to resolve the ambiguity in favor of treating the internalizeAction call as explicity authorization to broadcast. A clear reason for this choice is to keep the behavior of the internalizeAction call consistent both when the transaction originator is using the same storage and when they are not. On the open doc-string question: Expand the "must be unproven or completed" to correctly describe what gets implemented.
  2. TaskCheckNoSends.checkNow has an unresolved cost issue as the set of noSend transactions can grow large and there is a cost to constantly checking transactions that may be sitting in escrow or may be un-aborted tests. What must be added is an aging system where the re-try interval grows with the age of the transaction. Ultimately, optimizing checking for delayed externally broadcast transactions must come with performance limitations differentiating it from "normal" wallet-in-charge broadcasting. It is not enough to just enable checking all noSend proven_tx_reqs each time a new block appears.
  3. abortAction: Yes, this should never abort a valid transaction that has been shared with the network, but it should remain possible to abort transactions even when offline and network confirmation is impossible. This hole can never be 100% closed, but it should refuse to abort when confirmation is possible. Don't throw when abort fails due to valid tx confirmation. Use the method return value to indicate successful abort vs refused abort.
  4. specOpNoSendActions: Same as 3, never abort a valid transaction that has been shared with the network.

@tonesnotes

Copy link
Copy Markdown
Collaborator

On TaskCheckNoSend optimization:

A primary use case for noSend is batched transactions. The central purpose it to avoid duplicative network traffic when creating chains of large transactions. By NOT sending incrementally larger BEEFs until one complete BEEF with multiple new transactions can be sent following the sendWith createAction call.

How so ever TaskCheckNoSend is optimized, this use case must remain efficient.

BSVanon and others added 6 commits May 13, 2026 16:25
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>
@BSVanon

BSVanon commented May 13, 2026

Copy link
Copy Markdown
Contributor Author

Comments on the four fixes:

  1. internalizeAction: Fix 1 looks good. Internalizing a transaction with matching proven_tx_req in noSend state is an ambiguous situation. This is an attempt to take ownwership of an output where control of the underlying transaction was externalized. For simplicity, it makes sense to resolve the ambiguity in favor of treating the internalizeAction call as explicity authorization to broadcast. A clear reason for this choice is to keep the behavior of the internalizeAction call consistent both when the transaction originator is using the same storage and when they are not. On the open doc-string question: Expand the "must be unproven or completed" to correctly describe what gets implemented.
  2. TaskCheckNoSends.checkNow has an unresolved cost issue as the set of noSend transactions can grow large and there is a cost to constantly checking transactions that may be sitting in escrow or may be un-aborted tests. What must be added is an aging system where the re-try interval grows with the age of the transaction. Ultimately, optimizing checking for delayed externally broadcast transactions must come with performance limitations differentiating it from "normal" wallet-in-charge broadcasting. It is not enough to just enable checking all noSend proven_tx_reqs each time a new block appears.
  3. abortAction: Yes, this should never abort a valid transaction that has been shared with the network, but it should remain possible to abort transactions even when offline and network confirmation is impossible. This hole can never be 100% closed, but it should refuse to abort when confirmation is possible. Don't throw when abort fails due to valid tx confirmation. Use the method return value to indicate successful abort vs refused abort.
  4. specOpNoSendActions: Same as 3, never abort a valid transaction that has been shared with the network.

Thank you Tone — really good feedback. Worked through all four.

1. internalizeAction (Fix 1, docstring) — expanded both signer and storage docstrings to the per-status table you described, including the rationale that the call is explicit authorization to advance the lifecycle so behavior is consistent whether the originator shares the same storage as the internalizer or not. Commit: 7e676b9 → bc5f067.

2. TaskCheckNoSends.checkNow aging system — built a 5-tier age-based schedule on the checkNow path (skip <5 min / every-block 5 min – 1 hr / ~hourly 1 hr – 24 hr / ~daily 24 hr – 7 d / ~weekly 7 d+), with per-row staggering keyed by provenTxReqId so same-tier rows are spread across the modulo cycle instead of firing on the same block. The scheduled daily cadence (no checkNow) remains unfiltered as a safety net so externally-broadcast nosend txs are still eventually recognized regardless of age. Commits: 90db153 (initial freshness gate) → bc5f067 (full 5-tier schedule) → d3804cc (per-row staggering after an internal review caught synchronized same-tier bursts in v2).

3. abortAction return-value pivot — switched chain-confirmed refusal from throw WERR_INVALID_PARAMETER to return { aborted: false }, and lifted the conservative-refuse-on-service-unreachable. Both getStatusForTxids failure paths (catch + r.status !== 'success') now set an internal serviceUnreachable flag and proceed with the abort, writing a new abortAction-offline-fallback history note on the proven_tx_req for forensic audit. Refusal is reserved for positive on-chain confirmation, exactly as you described. Required widening AbortActionResult.aborted in the SDK from the literal true to boolean. Commits: 3c2ecbf (SDK widening) + 19895c2 (wallet-toolbox pivot).

4. specOpNoSendActions — same pivot in the bulk path. The pre-filter still does the batched getStatusForTxids for efficiency, but only protects rows on positive mined/known confirmation; service failures fall through to per-row abortAction calls (which each apply their own offline-fallback policy). The loop body now checks result.aborted per row, so race-window rows that became chain-known between pre-filter and the per-row call leave their status as nosend rather than being blanket-set to failed. Commit: 19895c2.

Build-graph follow-up — the SDK widening required switching @bsv/wallet-toolbox, @bsv/auth-express-middleware, and @bsv/payment-express-middleware from "@bsv/sdk": "^2.1.0" to "@bsv/sdk": "workspace:*" so the workspace SDK source is authoritative. Otherwise StorageServer.ts fails type identity at the points where this.wallet is passed to createAuthMiddleware/createPaymentMiddleware, because the published middleware packages bake in the older literal aborted: true WalletInterface. The pattern matches the wiring already used by conformance/runner/ts. If you'd rather coordinate this differently (e.g. release-coordinated SDK bump + middleware republish), happy to rework — let me know. Commit: a2f8244.

Test coverage: 28/28 pass on the targeted lane (taskCheckNoSendsFreshnessGate 12 + nosendLifecycleDefense 14 + processNewBlockHeader 2) under Node 24.15.0, including new cases for the offline-fallback audit notes and the mid-page race window.

@BSVanon

BSVanon commented May 13, 2026

Copy link
Copy Markdown
Contributor Author

On TaskCheckNoSend optimization:

A primary use case for noSend is batched transactions. The central purpose it to avoid duplicative network traffic when creating chains of large transactions. By NOT sending incrementally larger BEEFs until one complete BEEF with multiple new transactions can be sent following the sendWith createAction call.

How so ever TaskCheckNoSend is optimized, this use case must remain efficient.

Directly addressed by the tier0FreshSkipMsecs (<5 min) tier in commits bc5f067 + d3804cc: rows fresher than 5 minutes are entirely excluded from checkNow-triggered chain checks. Chained createAction({ noSend: true, sendWith: [...] }) builds therefore never trigger external getMerklePath lookups during the build phase. Once the terminator createAction broadcasts the assembled BEEF, the constituent rows age past the 5-min threshold and enter the regular schedule (per-row staggered so even a wallet with N batched-tx tails doesn't see synchronized bursts).

The class docstring at TaskCheckNoSends.ts calls out this batched-tx protection explicitly both in the aging-schedule paragraph and as the rationale on tier0FreshSkipMsecs. The scheduled daily cadence (no checkNow) is unchanged and still scans every nosend row regardless of age, so the safety net for externally-broadcast unmined nosend txs is preserved.

@tonesnotes tonesnotes requested review from tonesnotes and removed request for tonesnotes May 13, 2026 23:08
@tonesnotes

Copy link
Copy Markdown
Collaborator

Updates are consistent with issues raised in my earlier comments.
I approve the substance of these changes.

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
@sirdeggen

Copy link
Copy Markdown
Contributor

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.

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 release/v* and it will release all updated packages in topological order. It's pretty nifty.

sirdeggen and others added 2 commits May 18, 2026 20:55
…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>
@sonarqubecloud

Copy link
Copy Markdown

@sirdeggen sirdeggen merged commit c821b10 into bsv-blockchain:main May 19, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants