Skip to content

Fix wallet storage self-healing for stuck change#188

Draft
ty-everett wants to merge 1 commit into
bsv-blockchain:mainfrom
ty-everett:codex/wallet-storage-self-heal
Draft

Fix wallet storage self-healing for stuck change#188
ty-everett wants to merge 1 commit into
bsv-blockchain:mainfrom
ty-everett:codex/wallet-storage-self-heal

Conversation

@ty-everett

Copy link
Copy Markdown
Contributor

Summary

This PR addresses a customer-observed wallet storage state that can brick createAction in browser-local storage and leaves web apps unable to repair the wallet through BRC-100 calls.

The problematic state is a contradictory change row where an output remains spendable = true while also retaining spentBy = <failed transaction id>. In that state, change allocation can keep selecting an output that is still logically locked by a failed or otherwise unrecoverable transaction. If the holder transaction references a txid whose raw bytes/proof are no longer available, the double-spend review path can throw WERR_INVALID_PARAMETER from getBeefForTransaction instead of returning a reviewable double-spend result. From the app side, admin-only basket access and hidden/unabortable action states mean there is no reliable BRC-100 repair path.

Changes

  • Exclude outputs with spentBy set from wallet balance and change allocation eligibility in both IndexedDB and Knex storage.
  • Complete the IndexedDB reviewStatus repair path so it now:
    • marks transactions failed when their matching proven tx request is invalid;
    • marks transactions completed when a matching proven tx exists;
    • releases outputs held by terminally failed transactions when no active/non-terminal proven tx request still blocks restoration.
  • Run reviewStatus before the specOpInvalidChange release sweep, giving apps an existing non-admin repair trigger that can clear this stale failed-transaction lock class before chain-validating change outputs.
  • Make the double-spend reporter degrade gracefully when the competing transaction BEEF cannot be retrieved: it now returns a txid-only competing BEEF instead of surfacing an opaque WERR_INVALID_PARAMETER to the caller.
  • Add focused IndexedDB regression coverage for the stuck state:
    • failed transaction output release;
    • change allocation skipping spendable rows that still have spentBy set;
    • spendable-balance totals excluding rows still held by another transaction.

Why these changes are needed

The observed failure was not simply a bad UTXO. It was local storage drift that made the wallet believe an output was available for funding while the same row still pointed at a failed transaction as its spender. That is enough to trap repeated createAction attempts in the funding/input validation path, and the current app-facing repair tools cannot target the hidden holder transaction or admin-only default basket directly.

The defensive eligibility checks prevent the allocator from making the stuck state worse. The IndexedDB status review brings browser-local storage closer to the existing Knex repair behavior. Running review from the invalid-change release specOp gives applications a practical repair entry point without broadening basket permissions. The txid-only BEEF fallback preserves the intended doubleSpend review semantics even when the competing transaction is no longer locally reconstructable.

Validation

  • corepack pnpm install --frozen-lockfile
  • corepack pnpm --filter @bsv/wallet-toolbox exec jest --runTestsByPath src/storage/__test/StorageIdb.test.ts --runInBand
  • corepack pnpm --filter @bsv/auth-express-middleware run build
  • corepack pnpm --filter @bsv/payment-express-middleware run build
  • corepack pnpm --filter @bsv/wallet-toolbox run build
  • corepack pnpm --filter @bsv/wallet-toolbox exec ts-standard src/storage/StorageIdb.ts src/storage/StorageKnex.ts src/storage/StorageProvider.ts src/storage/__test/StorageIdb.test.ts src/storage/methods/reviewStatusIdb.ts

Note: full @bsv/wallet-toolbox lint currently fails on pre-existing no-unnecessary-type-assertion findings in src/__tests/WalletPermissionsManager.pmodules.test.ts; those files are unrelated to this patch.

@sonarqubecloud

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
1 Security Hotspot

See analysis details on SonarQube Cloud

@tonesnotes tonesnotes left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of this looks wrong. More belt and suspenders plus enhancements to idb where it lagged knex and additional recovery robustness.

In general, spendable should reflect the truth of whether an output has been spent or not. spentBy is a lame, incomplete hint that allows tracking spend flows in only a narrow class of situations and is also hard to maintain over some recovery flows. Adding the checks for spentBy also being null elevates the required validity threshold of the property which may now trigger recovery issues where spentBy is somehow not null but the output is spendable. As in, there was an attempt to spend this output by another transaction, but somehow that failed in an unusual way such that the spentBy didn't get reset to null.

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.

2 participants