Skip to content

fix(wallet-toolbox): findings from running the toolbox as a production IndexedDB wallet #1136

Open

Description

@GenericCPU

Area

Wallet

Affected package or service and version

@bsv/wallet-toolbox / @bsv/wallet-toolbox-client. Used in production on 2.4.4 through 2.13.2; every finding below is re-checked against main at 8ac5939 (2.15.0).

Defect

We ran @bsv/wallet-toolbox-client as the storage and signing core of HandCash Desktop (Electron) and HandCash Mobile (Android WebView) for several months, on IndexedDB storage. Until 2.13.2 we kept HandCash patches applied to the published bundles. We have since moved those fixes into reviewed TypeScript, in a wallet engine derived from the 2.13.2 source with the Open BSV license retained (packages/wallet-engine, tag v1.3.571). This issue collects what we found so the fixes can come back here. The engine is linked only as a reference implementation.

Four findings are narrow defects with a fix and a regression test in their own pull requests. The rest change behaviour or a storage contract, so they are proposals for discussion first.

Defects with a pull request

1. A stored explicit input is not reserved when the batch BEEF already holds its source. ActionBatchWorkspace.missingExplicitOutpoints skips any explicit input whose source transaction is already in sharedBeef, which happens whenever the caller passes it in inputBEEF. The planner then builds the input from the BEEF, and commitActionBatch rejects the whole batch with input <txid>.<vout> was not reserved by this action batch. Any app that spends one of its own wallet outputs inside an open batch and supplies the BEEF hits it. We found this one reviewing the batch code while porting it, not in production: HandCash runs legacy batch mode (see finding 3), which bypasses the workspace. The regression test reproduces it on main. Fix: keep skipping BEEF-carried inputs, except those the wallet stores (found with a read-only findOutputs scoped to the user), which are reserved. External inputs are never sent for reservation, so they keep today's cost and cannot hit the batch capacity limit. Reference: missingExplicitOutpoints. Pull request: #1138.

2. A funding output chosen from a noScript scan has no locking script. StorageIdb.allocateChangeInput and availableManagedChange read candidates with noScript: true, which clears every script. validateOutputScript reloads only a script offloaded into the raw transaction (scriptOffset/scriptLength set and the raw transaction stored); a script stored inline on the row stays cleared. allocateChangeInput and makeFundingResult then hand the signer a funding input without a locking script, and the send fails. In our wallets the rows that hit this were change outputs restored from a history backup or synced between devices, which carry their script but no txid, scriptOffset or scriptLength; on IndexedDB they stopped sends until we patched it. An offloaded script whose raw transaction storage no longer holds fails the same way. Fix: an internal helper that falls back to rereading the row, used on both funding paths; no public API change. Reference: allocateChangeInput. Pull request: #1139.

3. Setup.createWallet and SetupClient.createWallet drop actionBatchMode. WalletArgs.actionBatchMode defaults to auto, where a signed noSend action is staged in memory and reaches storage only through a later sendWith. The setup helpers never pass the option on, so every wallet built through them runs in auto. A caller that broadcasts its own noSend transactions never calls sendWith. If the page reloads or the batch is aborted, the signed action, its change and the spent marks on its inputs are lost, and the caller still holds a transaction that spends those inputs. The Wallet class supports legacy, but there is no way to select it through the helpers. We had to swap the controller's constructor at runtime. Fix: forward an optional actionBatchMode; the default stays auto. Setup.createWallet now builds through SetupClient.createWallet, so the option is forwarded once. Pull request: #1140.

4. createStorageIdb's databaseName does not name the IndexedDB database. StorageIdb always opens wallet-toolbox-${chain}net. SetupClient.createStorageIdb documents databaseName as "Name for this storage" but passes it only to migrate as the storage name. Every wallet in an origin therefore shares one database, whatever name each identity or account is given. The IndexedDB test helpers have the same problem: createIdbLegacyWalletCopy(databaseName) calls dropAllData() on the shared database. Honouring databaseName directly would be a regression. Existing wallets live in the shared database, and opening a new name for them shows an empty wallet; that is exactly what happened to us when our engine started using the storage name (migrate). Fix: an optional, explicit dbName on StorageIdbOptions and SetupWalletIdbArgs (blank names refused), with the default unchanged and the docs saying databaseName is not the IndexedDB name. Pull request: #1141.

Proposals for discussion

5. Funding reads every spendable change output on each batch. beginActionBatch and extendActionBatch call findAvailableManagedChangeInputs and findTransactionStatusesByIds for the whole change basket every time. Their cost grows with the wallet, not with the payment, and it dominated planning on our larger wallets. We read the newest change in pages of 24, stop once the target is covered by at least four outputs, and fall back to the full scan only when five pages are not enough (fundingChangeCandidates / newestSpendableChange). This changes which coins are selected, so it needs agreement and benchmarks before a pull request.

6. Reservation loads proof data for coins the payment does not spend. makeFundingResult calls getBeefForTransaction for every reserved output, one at a time, so signing waits on ancestry for coins the transaction never uses. We load scripts at reservation and fetch BEEF only for the inputs the planned transaction spends, in one getBeefForTransactions call (makeFundingResult, ensurePlannedInputBeef). getBeefForTransactions exists only on local StorageProviders, so a remote (StorageClient) wallet would need a new storage method. That is why this is a proposal and not a pull request.

7. TaskMonitorCallHistory serializes the full services call history. Each run stringifies the entire history before logEvent truncates the stored copy to 8 KiB, and the first run happens when the monitor starts. Before the stored copy was truncated, this task's write froze navigation taps in our app right after unlock. We now persist a placeholder for it (Monitor). We suggest bounding the history before serializing it, or leaving the task out of the default client task set.

8. Signing copies the source graph once per input. Transaction.sign snapshots every parent transaction for every input before asking the template to sign. Wallet-managed inputs already carry their amount and locking script, so we sign them against the live transaction (signManagedInputs). The cost is in @bsv/sdk, so it belongs in a separate SDK issue; it is listed here because the toolbox signing path pays it.

Minimal reproduction

# Each defect has a regression test in its pull request. On main without the fix:
pnpm --filter @bsv/wallet-toolbox exec jest --watchman=false test/Wallet/action/actionBatch.test.ts -t "stored explicit input"
#   WERR_INVALID_OPERATION: input <txid>.0 was not reserved by this action batch
pnpm --filter @bsv/wallet-toolbox exec jest --watchman=false src/storage/__test/allocateChangeInputScript.test.ts
#   IndexedDB: allocateChangeInput returns the chosen output with lockingScript undefined
pnpm --filter @bsv/wallet-toolbox exec jest --watchman=false src/__tests/SetupClient.actionBatchMode.test.ts
#   Expected "legacy", received "auto"
pnpm --filter @bsv/wallet-toolbox exec jest --watchman=false src/storage/__test/StorageIdb.dbName.test.ts
#   createStorageIdb opens wallet-toolbox-testnet whatever name is given

Expected behavior

  1. An explicit input that exists in storage is reserved by the batch whether or not its source is already in the batch BEEF, and the batch commits.
  2. A funding output selected from storage reaches the signer with its locking script.
  3. Options documented on WalletArgs can be set through the setup helpers that build the Wallet.
  4. A documented storage name either names the IndexedDB database or is documented as not doing so, with an explicit way to name it.

Actual behavior and evidence

  1. commitActionBatch throws input <outpoint> was not reserved by this action batch.
  2. lockingScript is undefined on the selected funding output whenever its script is stored inline.
  3. wallet.actionBatch.mode is auto regardless of the argument.
  4. storage.dbName is wallet-toolbox-${chain}net regardless of databaseName.

Environment

Electron (Chromium IndexedDB) on macOS, Windows and Linux; Android System WebView through Capacitor. Long-lived wallets with large change baskets. Reproduced in the repository's own Jest suites on Node 24.22 against SQLite and fake-indexeddb.

Safety checks

  • I searched for an existing issue or pull request describing this defect.
  • This report contains no secret, private key, token, production data, or uncoordinated vulnerability detail.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions