Skip to content
This repository was archived by the owner on Jun 12, 2026. It is now read-only.

StorageIdb: align filter/update semantics with Knex canon - #151

Merged
sirdeggen merged 2 commits into
bsv-blockchain:masterfrom
b-open-io:fix/storage-provider-parity
Apr 27, 2026
Merged

StorageIdb: align filter/update semantics with Knex canon#151
sirdeggen merged 2 commits into
bsv-blockchain:masterfrom
b-open-io:fix/storage-provider-parity

Conversation

@shruggr

@shruggr shruggr commented Apr 22, 2026

Copy link
Copy Markdown
Contributor

Context

Surfaced during a sync-failure investigation in yours-wallet. A user's IndexedDB held multiple accounts; Account B's output_tags_map rows leaked into Account A's outbound sync chunk and then exploded on the receiving storage (an mss1 wallet server in the 1sat-sdk monorepo) with "Result must be unique." The direct cause was a dead-code user-scope filter in StorageIdb.filterOutputTagMaps; auditing that surfaced five related divergences from the StorageKnex canon, fixed in this PR.

Full divergence audit (what matched, what drifted, severity) lives in 1sat-sdk/docs/plans/2026-04-21-storage-provider-divergences.md — happy to copy here on request.

Changes (all in src/storage/StorageIdb.ts)

1. filterOutputTagMaps user-scope filter — CRIT

The guard was if (userId !== undefined && r.txid) but TableOutputTagMap has no txid column — the check was always false and the scope filter never ran. getOutputTagMapsForUser returned rows across all users in the DB. Drop the bogus r.txid and run the sub-count against output_tags unconditionally when userId is provided. Matches Knex's WHERE EXISTS (SELECT * FROM output_tags WHERE ...) behavior.

2. filterOutputs / filterTransactions empty-array txStatus / status poisoning — HIGH

filterOutputs applied the tx-status sub-count on args.txStatus !== undefined; filterTransactions:1855 used if (args.status && !args.status.includes(...)). Since [] is truthy, an empty array caused every row to be rejected on Idb while Knex treats empty array as a no-op. Added .length > 0 guards matching Knex/StorageBunSqlite.

3. Undefined-partial silent over-match — HIGH

Knex throws Undefined binding(s) detected when a partial filter has undefined values. StorageIdb silently skipped undefined keys via per-key truthiness guards, producing unintended over-matches — especially painful in sync's EntityOutputTagMap.mergeFind path where an unmapped idMap entry quietly propagates undefined down to findOutputTagMaps({partial: {outputId: undefined, outputTagId: undefined}}) and then verifyOneOrNone throws a misleading "Result must be unique." New private assertNoUndefinedInPartial called at the top of every filter* method reproduces Knex's loud failure.

4. Truthiness guards on integer-ID / string partial fields — MED

Sweep-replaced if (args.partial.X && r.X !== args.partial.X)if (args.partial.X !== undefined && r.X !== args.partial.X) across all filter methods (~106 replacements covering IDs, strings, date fields). Fixes silent filter skips for lockTime === 0, version === 0, empty-string description/derivationPrefix/customInstructions, etc. Fields that were already !== undefined (isDeleted, satoshis, booleans) were left alone.

5. updateIdb return count — MED

Was unconditionally return 1 even for batch updates across an array of ids. Now tracks and returns the real updated count. (updateIdbKey is single-record by composite key; its return 1 is correct and unchanged.)

6. filterProvenTxReqs user-scope r.txid guard — LOW

Same shape as finding 1. TableProvenTxReq.txid is NOT NULL in the Knex canon schema, so && r.txid is almost always truthy in practice — but IDB has no schema enforcement, and a row with empty/undefined txid would silently bypass user scoping. Knex's WHERE EXISTS (... JOIN ON txid) would exclude such rows; Idb was including them. Drop the guard.

What this does not touch

  • purgeDataIdb parity vs methods/purgeData.ts — not audited in depth, flagged for follow-up.

Rebase + packaging

Rebased onto origin/master (2.1.21). Package name + version bumped to @bopen-io/wallet-toolbox@2.1.21-parity-fix.0 (and the mirrored @bopen-io/wallet-toolbox-mobile@2.1.21-parity-fix.0) for immediate publication via the @bopen-io npm org. If this PR merges, a separate commit would revert the rename for @bsv publication.

Validation

  • npm run build — clean on 2.1.21 + my changes.
  • npm test -- --testPathPattern="OutputTagMapTests|TxLabelMapTests|OutputTagTests|TxLabelTests|TransactionTests|OutputTests" — 61/61 passing.
  • One unrelated test failure in ChaintracksStorageIdb.test.ts (WoC file-hash mismatch, network-dependent, fails on master too).

Test plan for merge

  • Unit test for getOutputTagMapsForUser with a multi-user IDB returns only the requested user's maps.
  • Unit test that findOutputTagMaps({partial: {outputId: undefined}}) throws WERR_INVALID_PARAMETER.
  • Unit test that findTransactions({status: []}) returns rows instead of nothing.
  • Unit test that findTransactions({partial: {lockTime: 0}}) filters correctly.
  • Unit test that updateIdb returns actual affected-row count on a batch update.
  • Integration test: build a sync chunk from a multi-account IDB; confirm only the scoped user's tag maps are included.

@shruggr
shruggr force-pushed the fix/storage-provider-parity branch from 12518d0 to 90ea80f Compare April 22, 2026 02:28
Surfaced during a multi-account sync-failure investigation. A user's
IndexedDB held multiple accounts; Account B's output_tags_map rows
leaked into Account A's outbound sync chunk because filterOutputTagMaps
had a dead-code user-scope filter (guarded on a non-existent txid
column). Auditing that surfaced five related divergences from the
StorageKnex canon, all fixed in this commit.

All changes in src/storage/StorageIdb.ts (plus one small caller change
in src/storage/methods/listOutputsIdb.ts).

1. filterOutputTagMaps user-scope filter — CRIT
   The guard was `if (userId !== undefined && r.txid)` but
   TableOutputTagMap has no txid column — the check was always false
   and the scope filter never ran. getOutputTagMapsForUser returned
   rows across all users. Drop the bogus r.txid and run the sub-count
   against output_tags unconditionally when userId is provided.
   Matches Knex's WHERE EXISTS behavior.

2. filterOutputs / filterTransactions empty-array poisoning — HIGH
   filterOutputs applied the tx-status sub-count on
   `args.txStatus !== undefined`; filterTransactions used
   `if (args.status && !args.status.includes(...))`. Since [] is
   truthy, an empty array caused every row to be rejected on Idb while
   Knex treats empty array as a no-op. Added .length > 0 guards
   matching Knex/StorageBunSqlite.

3. HIGH-7: IDB listOutputs include 'sending' tx status
4. HIGH-8: IDB tag/label resolution tolerates orphan map rows
5. HIGH-11: IDB updateIdb silent-skip missing ids
6. MED-14: IDB allocateChangeInput hydrates chosen output's script
7. MED-16: IDB listOutputs supports negative offset (tail pagination)
8. MED-18: IDB findTransactions(noRawTx) keeps inputBEEF
9. MED-19: IDB insertCertificate strips non-schema logger field
10. LOW-28: IDB getRawTxOfKnownValidTransaction uses 'unfail'-inclusive
    status set on slice path

Rebased onto 2.1.22. Previous branch history had a long sequence of
apply-then-revert commits that have been squashed into this single
commit against the current main.
@shruggr
shruggr force-pushed the fix/storage-provider-parity branch from 02da237 to 5afac5d Compare April 23, 2026 19:34
@shruggr
shruggr marked this pull request as ready for review April 27, 2026 14:17
Sonar flagged: unused verifyOne import, and non-null assertions on
offset/length already narrowed via the sliceRequested const.
@sonarqubecloud

Copy link
Copy Markdown

@sirdeggen
sirdeggen merged commit 3225488 into bsv-blockchain:master Apr 27, 2026
5 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants