ADD: detect regular payments in scanByTxid

[chaitika]

No description provided.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[theanmolsharma]

I'd like to change the approach before this lands.

The core issue

The regular branch uses a wallet resync as the detection mechanism. scanTxidForRegularPayment runs fetchBalance() → fetchTransactions() → fetchUtxo() and then answers "did this tx pay me?" with "does my freshly-synced history happen to contain this txid, and is its net value positive?" (hd-bip352-wallet.ts:609-610).

That's indirect in ways that bite:

• It inherits every blind spot of address discovery. If the receiving address is past the gap limit, or the Electrum history sync errored, or the server's per-address history lags a mempool tx, super.getTransactions().find(...) misses and we report not-found. The user pasted a txid — we should ask the server about that txid.
• tx.value > 0 is a net figure, ours-out minus ours-in. A tx that pays us 0.5 BTC while also spending 1 BTC of our own coins nets negative and is reported not-found, even though it contains outputs we own.
• It costs three multi-address network sweeps to answer a yes/no, and it runs unconditionally — including when the SP branch already returned found: true, so the user waits on a full resync whose only payoff is the rare tx paying both an SP output and a derived address of the same wallet.
• utxosFound is a bare integer summed across two branches (:570) that computed it by different rules. Nothing can catch a double-count, and it can't express what we actually care about.

That last one matters most for what this feature is supposed to do: a tx can contain multiple SP outputs, multiple payments to our regular addresses, or both at once. We should detect and report all of them individually.

What I'd do instead

Detect from the transaction itself, output by output; sync only to ingest what detection found.

1. Return the outputs, not a count

type OwnedOutput = {
  vout: number;
  value: number;                              // sats
  kind: 'silent-payment' | 'regular';
  address?: string;                           // regular only
  isChange?: boolean;                         // regular, from the internal chain
};

type ScanByTxidResult = {
  found: boolean;
  outputs: OwnedOutput[];
  totalValue: number;
  blockHeight: number;
  tipHeight: number;
};

utxosFound becomes outputs.length. Merging two branches then means deduping on vout, which is verifiable, instead of adding two integers, which isn't.

2. SP branch — close to right already

processBatch already returns every matching SP output in the tx, so multi-output SP is handled. Two cleanups:

• Replace the hand-rolled addUTXO loop at :584-593 with the existing commitUTXOs() (:313) — it already handles added-count, the lastScannedBlock bump, and the callbacks.
• Distinguish detected (all SP outputs in this tx) from newly added (what addUTXO accepted). We want to show the user all of them on a re-check, but persist/notify should key off newly-added.

3. Regular branch — direct tx lookup

Electrum.multiGetTransactionByTxid (modules/Electrum.ts:875) fetches one tx by txid, verbose, and already normalizes Core 22's scriptPubKey.address back to .addresses (:988). It only Realm-caches at ≥7 confirmations, so recent and mempool txs are always fresh.

private async detectRegularOutputs(txid: string): Promise<{ outputs: OwnedOutput[]; confirmations: number } | null> {
  const result = await Electrum.multiGetTransactionByTxid([txid], true);
  const tx = result[txid];
  if (!tx || typeof tx === 'string') return null;

  const outputs: OwnedOutput[] = [];
  for (const vout of tx.vout) {
    const address = vout.scriptPubKey?.addresses?.[0];
    if (!address) continue;
    const owned = this.findAddressIndex(address);
    if (!owned) continue;
    outputs.push({
      vout: vout.n,
      value: new BigNumber(vout.value).multipliedBy(100000000).toNumber(),  // verbose value is BTC
      kind: 'regular',
      address,
      isChange: owned.internal,
    });
  }
  return { outputs, confirmations: tx.confirmations ?? 0 };
}

One blockchain.transaction.get, every owned output enumerated, no dependency on the wallet already knowing the tx, and no tx.value heuristic. isChange also lets the UI decide how to present a self-send rather than the wallet silently suppressing it.

findAddressIndex doesn't exist yet — we only have the boolean weOwnAddress (abstract-hd-electrum-wallet.ts:875). I'd add an index-returning variant next to it over the same next_free_*_index + gap_limit range, returning { internal: boolean; index: number } | null. It's needed for step 4 anyway.

4. Ingest only when detection matched

This inverts the cost model — detection tells us whether to sync at all.

• SP outputs → commitUTXOs(), as they can't come from Electrum.
• Regular outputs → they need to reach _utxo to be spendable and getTransactions() to show in history. PaymentFound.tsx:32 does wallet.getTransactions().find(...) and renders nothing if the tx is absent, so history ingestion does matter. Before syncing, advance next_free_address_index past any matched external index (and the change index likewise) so discovery actually covers the address we just matched, then run the existing fetchBalance() → fetchTransactions() → fetchUtxo(). Please keep the ordering comment at :604 — it's correct and non-obvious.

Electrum.multiGetUtxoByAddress (modules/Electrum.ts:772) is the cheaper targeted alternative: it returns {height, address, txid, vout, value} per address, values already in sats, already filtered to unspent — which the current code doesn't distinguish at all, since it counts owned outputs whether or not they're still spendable and calls them UTXOs. It no-ops under disableBatching (EPS has no listunspent), so it needs the full-sync fallback regardless. Fine to ship the gated full sync first and add this if latency shows.

5. One height convention for both branches

Today the SP branch reports true confirmations while the regular branch reports one fewer: Electrum.ts:581 defines confirmations as estimateCurrentBlockheight() - txHeight, and :615-621 inverts PaymentFound.tsx:36 exactly, so the UI ends up displaying that raw value. Same depth, two different numbers, and a tx mined into the current tip block shows as unconfirmed — which shifts the CONFIRMATIONS_THRESHOLD = 6 transition by a block. The regular path's tipHeight is also estimateCurrentBlockheight(), a 9.93-min-per-block extrapolation rather than a real tip.

Since we're in here: return confirmations directly instead of round-tripping through blockHeight/tipHeight and back out in PaymentFound. The verbose Electrum tx carries real server-side confirmations and the indexer carries a real height and tip — convert once, in one place.

Smaller things

• :557 logs the regular-branch failure under the [SP] prefix.
• TrackPayment.tsx:87 shows the same NoPaymentFound screen for a total network outage as for a stranger's txid. Pre-existing, but now that both branches swallow their errors it'd be worth surfacing "both branches failed" distinctly.
• scanTxidForRegularPayment doesn't scan by txid — it full-syncs and then does a local .find(). Naming should follow whatever it ends up doing.
• PR body is empty; given the two branches carry different height semantics, a couple of lines would help the next reader.

Tests

The merge-matrix tests mostly survive with outputs replacing utxosFound — nice coverage of the hit/miss combinations and both throw paths. The cases worth adding are the ones the current design can't express:

• a tx with two owned regular outputs
• a tx with two SP outputs
• a tx with one of each (and assert the dedupe/merge)
• an owned output on a tx whose net value is negative
• a payment to an address one past next_free_address_index

Each is a single fixture against detectRegularOutputs, with no prototype stubbing of getTransactions/weOwnAddress — which is what currently makes the regular-branch tests mostly assert their own arithmetic.

Happy to pair on this if it's easier than a written back-and-forth.