Never report a successful broadcast as a failed send

CHANGELOG.md

@@ -2,6 +2,8 @@
 
 ## Unreleased
 
+- fixed: Failed sends after a successful broadcast: `saveTx` no longer fails when the engine is not running, and an exhausted broadcast now distinguishes explicit server rejections (definitively failed, safe to retry) from transport failures where the transaction may have reached the network (`BroadcastAmbiguityError`).
+
 ## 3.11.0 (2026-07-13)
 
 - added: Support the `<code>-wif:` protohandler prefix (e.g. `bch-wif:`) in `parseUri` so CashStamps private keys can be swept.

src/common/utxobased/engine/ServerStates.ts

@@ -20,6 +20,11 @@ import Deferred from '../network/Deferred'
 import { WsTask, WsTaskGenerator } from '../network/Socket'
 import { SocketEmitter, SocketEvent } from '../network/SocketEmitter'
 import { pushUpdate, removeIdFromQueue } from '../network/socketQueue'
+import {
+  BroadcastAmbiguityError,
+  classifyBroadcastFailure,
+  isAlreadyKnownRejection
+} from './broadcastError'
 import { MAX_CONNECTIONS, NEW_CONNECTIONS } from './constants'
 import { UtxoInitOptions } from './types'
 
@@ -371,6 +376,39 @@ export function makeServerStates(config: ServerStateConfig): ServerStates {
         let resolved = false
         let bad = 0
 
+        // Collect every server's failure so the terminal rejection can be
+        // classified: all explicit rejections reject with the original error
+        // (a definitive failure, safe to retry); any transport failure in
+        // the set rejects as ambiguous, because one of those servers may
+        // have relayed the transaction before failing to answer.
+        const broadcastErrors: unknown[] = []
+        const rejectClassified = (error?: Error): void => {
+          // A server refusing because it already has the transaction is
+          // confirmation the transaction reached the network (from this
+          // attempt or an earlier one with the same signed bytes): success.
+          if (broadcastErrors.some(isAlreadyKnownRejection)) {
+            if (!resolved) {
+              resolved = true
+              log.warn(
+                `broadcastTx: server already has txid ${transaction.txid}; treating broadcast as a success`
+              )
+              resolve(transaction.txid)
+            }
+            return
+          }
+          const msg = error != null ? `With error ${error.message}` : ''
+          log.error(`broadcastTx fail: ${JSON.stringify(transaction)}\n${msg}`)
+          if (classifyBroadcastFailure(broadcastErrors) === 'ambiguous') {
+            reject(
+              new BroadcastAmbiguityError(
+                broadcastErrors.map(cause => String(cause))
+              )
+            )
+          } else {
+            reject(error)
+          }
+        }
+
         const wsUris = Object.keys(serverStatesCache).filter(
           uri => serverStatesCache[uri].blockbook != null
         )
@@ -393,12 +431,9 @@ export function makeServerStates(config: ServerStateConfig): ServerStates {
                 }
               })
               .catch((e?: Error) => {
+                broadcastErrors.push(e)
                 if (++bad === wsUris.length) {
-                  const msg = e != null ? `With error ${e.message}` : ''
-                  log.error(
-                    `broadcastTx fail: ${JSON.stringify(transaction)}\n${msg}`
-                  )
-                  reject(e)
+                  rejectClassified(e)
                 }
               })
           }
@@ -463,12 +498,9 @@ export function makeServerStates(config: ServerStateConfig): ServerStates {
                 }
               })
               .catch((e?: Error) => {
+                broadcastErrors.push(e)
                 if (++bad === nowNodeUris.length) {
-                  const msg = e != null ? `With error ${e.message}` : ''
-                  log.error(
-                    `broadcastTx fail: ${JSON.stringify(transaction)}\n${msg}`
-                  )
-                  reject(e)
+                  rejectClassified(e)
                 }
               })
           }

src/common/utxobased/engine/UtxoEngine.ts

@@ -432,7 +432,14 @@ export async function makeUtxoEngine(
         throw err
       })
       if (id !== transaction.txid) {
-        throw new Error('broadcast response txid does not match original')
+        // The transaction is on the network at this point, so a mismatched
+        // response txid must not be reported as a send failure. Track the
+        // txid the network actually accepted, or the wallet would watch a
+        // transaction that never confirms.
+        log.warn(
+          `broadcast response txid mismatch: expected ${transaction.txid} received ${id}`
+        )
+        return { ...transaction, txid: id }
       }
       return transaction
     },

src/common/utxobased/engine/UtxoEngineProcessor.ts

@@ -167,12 +167,17 @@ export function makeUtxoEngineProcessor(
     const expectedProcessCount =
       Object.keys(taskCache.addressSubscribeCache).length * processesPerAddress
 
+    // With no subscribed addresses there is no denominator to compute a
+    // progress ratio from. The cache is empty when the engine is not running
+    // (never started, or stopped and its task cache cleared) while saveTx
+    // still drives UTXO processing. Skip the progress update, without
+    // counting the call as progress, rather than fail the caller's data
+    // write.
+    if (expectedProcessCount === 0) return
+
     // Increment the processed count
     processedCount = processedCount + 1
 
-    // If we have no addresses, we should not have not yet began processing.
-    if (expectedProcessCount === 0) throw new Error('No addresses to process')
-
     const percent = processedCount / expectedProcessCount
     if (percent - processedPercent > CACHE_THROTTLE || percent === 1) {
       log(
@@ -371,6 +376,13 @@ export function makeUtxoEngineProcessor(
       serverStates.stop()
       clearTaskCache()
       clearPendingTimeouts()
+      // The progress counters describe the sync round that just ended.
+      // Clearing the cache without them would let leftover counts combine
+      // with a smaller refilled cache (saveTx-driven setLookAhead, or
+      // addGapLimitAddresses) into a bogus completed-sync emission and a
+      // seen-tx checkpoint advance on an engine that is not syncing.
+      processedCount = 0
+      processedPercent = 0
       running = false
     },
 
@@ -463,8 +475,9 @@ export function makeUtxoEngineProcessor(
     },
 
     async broadcastTx(transaction: EdgeTransaction): Promise<string> {
-      await serverStates.broadcastTx(transaction)
-      return transaction.txid
+      // Return the txid the network actually answered with, so the caller
+      // can detect a server accepting the transaction under a different id.
+      return await serverStates.broadcastTx(transaction)
     },
     refillServers(): void {
       serverStates.refillServers()

src/common/utxobased/engine/broadcastError.ts

@@ -0,0 +1,70 @@
+/**
+ * Classification of an all-servers-failed broadcast.
+ *
+ * An explicit rejection is a server answering the broadcast with a Blockbook
+ * error response: the server received the transaction and refused it, so the
+ * same signed bytes will be refused again and cannot be on the network from
+ * this attempt. Anything else (a request timeout, a dropped connection, an
+ * HTTP status error whose body was never read) leaves relay possible: a
+ * server can relay the transaction to the network and still fail to answer.
+ *
+ * The classification is pure error-shape inspection. It adds no network
+ * calls, so it cannot add latency to the send path.
+ */
+
+/**
+ * A broadcast that failed on every server where at least one failure was a
+ * transport error rather than an explicit rejection. The transaction cannot
+ * be assumed absent from the network, so a retry could produce a second real
+ * payment. Consumers branch on `name === 'BroadcastAmbiguityError'`; class
+ * identity does not survive the core bridge, names and properties do.
+ */
+export class BroadcastAmbiguityError extends Error {
+  readonly causes: string[]
+
+  constructor(causes: string[]) {
+    super('Broadcast failed, but the transaction may have reached the network')
+    this.name = 'BroadcastAmbiguityError'
+    this.causes = causes
+  }
+}
+
+export const isExplicitBroadcastRejection = (error: unknown): boolean =>
+  String(error instanceof Error ? error.message : error).includes(
+    'Blockbook Error: '
+  )
+
+/**
+ * A rejection that means the server ALREADY HAS the transaction
+ * ("transaction already in block chain", "txn-already-in-mempool",
+ * "txn-already-known"). This is a confirmation the transaction reached the
+ * network, from this attempt or an earlier one with the same signed bytes,
+ * so the broadcast must be treated as a success: presenting it as a
+ * failure invites the duplicate-payment retry this work exists to stop.
+ */
+export const isAlreadyKnownRejection = (error: unknown): boolean =>
+  isExplicitBroadcastRejection(error) &&
+  /alread/i.test(String(error instanceof Error ? error.message : error))
+
+/**
+ * A failure from a server that provably never accepted the payload, so it
+ * cannot have relayed the transaction: the Electrum stub refuses
+ * broadcastTx synchronously. Such failures say nothing about relay and are
+ * excluded from the ambiguity determination. (A not-yet-connected blockbook
+ * is NOT in this class: its queued request can still send once the
+ * connection completes, so its timeout stays ambiguous.)
+ */
+export const isNonRelayFailure = (error: unknown): boolean =>
+  String(error instanceof Error ? error.message : error).includes(
+    'not supported for Electrum connections'
+  )
+
+export const classifyBroadcastFailure = (
+  errors: unknown[]
+): 'rejected' | 'ambiguous' => {
+  if (errors.length === 0) return 'ambiguous'
+  const relayCapable = errors.filter(error => !isNonRelayFailure(error))
+  return relayCapable.every(isExplicitBroadcastRejection)
+    ? 'rejected'
+    : 'ambiguous'
+}

test/common/utxobased/engine/broadcastError.spec.ts

@@ -0,0 +1,109 @@
+import { assert } from 'chai'
+import { describe, it } from 'mocha'
+
+import {
+  BroadcastAmbiguityError,
+  classifyBroadcastFailure,
+  isAlreadyKnownRejection,
+  isExplicitBroadcastRejection,
+  isNonRelayFailure
+} from '../../../../src/common/utxobased/engine/broadcastError'
+
+describe('broadcast failure classification', function () {
+  it('classifies all-explicit-rejection sets as rejected', function () {
+    assert.equal(
+      classifyBroadcastFailure([
+        new Error('Blockbook Error: -26: dust'),
+        new Error('Blockbook Error: -25: missing inputs')
+      ]),
+      'rejected'
+    )
+  })
+
+  it('classifies any transport failure in the set as ambiguous', function () {
+    assert.equal(
+      classifyBroadcastFailure([
+        new Error('Blockbook Error: -26: dust'),
+        new Error('Timeout for request 42')
+      ]),
+      'ambiguous'
+    )
+    assert.equal(
+      classifyBroadcastFailure([new Error('Timeout for request 42')]),
+      'ambiguous'
+    )
+    assert.equal(
+      classifyBroadcastFailure([
+        new Error('Failed to broadcast transaction via Blockbook: HTTP 503')
+      ]),
+      'ambiguous'
+    )
+  })
+
+  it('treats missing or empty error information as ambiguous', function () {
+    assert.equal(classifyBroadcastFailure([]), 'ambiguous')
+    assert.equal(classifyBroadcastFailure([undefined]), 'ambiguous')
+  })
+
+  it('excludes non-relay failures from the ambiguity determination', function () {
+    const electrumStub = new Error(
+      'broadcastTx not supported for Electrum connections'
+    )
+    assert.isTrue(isNonRelayFailure(electrumStub))
+    // An Electrum stub alongside explicit rejections must not turn a
+    // definitively failed broadcast into an ambiguous one.
+    assert.equal(
+      classifyBroadcastFailure([
+        electrumStub,
+        new Error('Blockbook Error: -26: dust')
+      ]),
+      'rejected'
+    )
+    // All-stub sets never sent anything anywhere: definitively failed.
+    assert.equal(classifyBroadcastFailure([electrumStub]), 'rejected')
+    // A transport failure still dominates.
+    assert.equal(
+      classifyBroadcastFailure([
+        electrumStub,
+        new Error('Timeout for request 42')
+      ]),
+      'ambiguous'
+    )
+  })
+
+  it('recognizes explicit rejections by the Blockbook error marker', function () {
+    assert.isTrue(
+      isExplicitBroadcastRejection(new Error('Blockbook Error: -26: dust'))
+    )
+    assert.isFalse(isExplicitBroadcastRejection(new Error('socket closed')))
+    assert.isFalse(isExplicitBroadcastRejection(undefined))
+  })
+
+  it('recognizes already-known rejections as network confirmation', function () {
+    assert.isTrue(
+      isAlreadyKnownRejection(
+        new Error('Blockbook Error: -27: transaction already in block chain')
+      )
+    )
+    assert.isTrue(
+      isAlreadyKnownRejection(
+        new Error('Blockbook Error: txn-already-in-mempool')
+      )
+    )
+    assert.isTrue(
+      isAlreadyKnownRejection(new Error('Blockbook Error: txn-already-known'))
+    )
+    assert.isFalse(
+      isAlreadyKnownRejection(new Error('Blockbook Error: -26: dust'))
+    )
+    // "already" in a transport error is not a Blockbook rejection.
+    assert.isFalse(isAlreadyKnownRejection(new Error('socket already closed')))
+  })
+
+  it('keeps a bridge-stable name and carries its causes', function () {
+    const error = new BroadcastAmbiguityError(['Timeout for request 42'])
+    assert.equal(error.name, 'BroadcastAmbiguityError')
+    assert.deepEqual(error.causes, ['Timeout for request 42'])
+    assert.instanceOf(error, Error)
+  })
+})