diff --git a/CHANGELOG.md b/CHANGELOG.md index a32d680f..fb1b5685 100644 --- a/CHANGELOG.md +++ b/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 `-wif:` protohandler prefix (e.g. `bch-wif:`) in `parseUri` so CashStamps private keys can be swept. diff --git a/src/common/utxobased/engine/ServerStates.ts b/src/common/utxobased/engine/ServerStates.ts index 18d289ab..e676eab6 100644 --- a/src/common/utxobased/engine/ServerStates.ts +++ b/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) } }) } diff --git a/src/common/utxobased/engine/UtxoEngine.ts b/src/common/utxobased/engine/UtxoEngine.ts index cc4ff6bd..de87c8bb 100644 --- a/src/common/utxobased/engine/UtxoEngine.ts +++ b/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 }, diff --git a/src/common/utxobased/engine/UtxoEngineProcessor.ts b/src/common/utxobased/engine/UtxoEngineProcessor.ts index 6f6b16fa..0c9524b8 100644 --- a/src/common/utxobased/engine/UtxoEngineProcessor.ts +++ b/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 { - 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() diff --git a/src/common/utxobased/engine/broadcastError.ts b/src/common/utxobased/engine/broadcastError.ts new file mode 100644 index 00000000..95475eef --- /dev/null +++ b/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' +} diff --git a/test/common/utxobased/engine/broadcastError.spec.ts b/test/common/utxobased/engine/broadcastError.spec.ts new file mode 100644 index 00000000..46d4c76d --- /dev/null +++ b/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) + }) +}) diff --git a/test/common/utxobased/engine/saveTx.spec.ts b/test/common/utxobased/engine/saveTx.spec.ts new file mode 100644 index 00000000..e9f00d97 --- /dev/null +++ b/test/common/utxobased/engine/saveTx.spec.ts @@ -0,0 +1,230 @@ +import { assert } from 'chai' +import { makeMemoryDisklet, makeNodeDisklet } from 'disklet' +import { + EdgeCorePluginOptions, + EdgeCurrencyEngine, + EdgeCurrencyEngineCallbacks, + EdgeCurrencyEngineOptions, + EdgeCurrencyPlugin, + EdgeCurrencyTools, + EdgeTransaction, + JsonObject, + makeFakeIo +} from 'edge-core-js' +import { describe, it } from 'mocha' + +import edgeCorePlugins from '../../../../src/index' +import { noOp, testLog } from '../../../util/testLog' +import { makeFakeNativeIo } from '../../../utils' +import { fixtures } from './engine.fixtures/index' + +const [tests] = fixtures + +/** + * A transaction paying to an address that the dummy-data wallet owns + * (scriptPubkey a9142244... with a bip49 path), spending a UTXO that the + * dummy-data set holds (19e59364...:0). This makes saveTx's + * getOwnUtxosFromTx return both a spent input and a new output, which drives + * processUtxos -> processDataLayerUtxos -> updateProgressRatio. + */ +const OWN_SCRIPT_PUBKEY = 'a9142244ce86d664e85801f7eb2a56dd35afd268212587' +const SPENT_TXID = + '19e59364daf34d97ed6584e9e978f3e2375adea9a4561a83d2066d92a010ba13' +const NEW_TXID = + 'f00dbabef00dbabef00dbabef00dbabef00dbabef00dbabef00dbabef00dbabe' +const SECOND_TXID = + 'deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef' + +describe('saveTx on a disconnected engine', function () { + it('resolves with zero subscribed addresses', async function () { + this.timeout(10000) + + const fakeIo = makeFakeIo() + const fixtureDisklet = makeNodeDisklet(tests.dummyDataPath) + const fakeIoDisklet = makeMemoryDisklet() + const nativeIo = makeFakeNativeIo() + + // Preload the wallet's data layer with the dummy dataset so the wallet + // owns addresses and UTXOs: + const migrate = async (dir: string): Promise => { + const files = await fixtureDisklet.list(dir) + await Promise.all( + Object.entries(files).map(async ([path, type]) => { + if (type === 'folder') await migrate(path) + if (type === 'file') + await fixtureDisklet + .getText(path) + .then(async data => await fakeIoDisklet.setText(path, data)) + }) + ) + } + await migrate('tables') + + const pluginOpts: EdgeCorePluginOptions = { + initOptions: {}, + io: { + ...fakeIo, + random: () => Uint8Array.from(tests.key) + }, + log: testLog, + infoPayload: {}, + nativeIo, + pluginDisklet: fakeIoDisklet + } + const factory = edgeCorePlugins[tests.pluginId] + if (typeof factory !== 'function') + throw new Error(`Missing plugin factory for ${tests.pluginId}`) + const plugin = factory(pluginOpts) as EdgeCurrencyPlugin + + const tools: EdgeCurrencyTools = await plugin.makeCurrencyTools() + const privateKeys = await tools.createPrivateKey(tests.WALLET_TYPE) + Object.assign(privateKeys, { coinType: 0, format: tests.WALLET_FORMAT }) + const publicKeys = await tools.derivePublicKey({ + type: tests.WALLET_TYPE, + keys: privateKeys, + id: '!' + }) + const keys: JsonObject = { ...privateKeys, ...publicKeys } + + // Track progress-side emissions: an engine that never ran a sync must + // never report itself fully synced, and must never advance the seen-tx + // checkpoint, no matter how many saveTx calls process UTXOs. + const addressesCheckedRatios: number[] = [] + const seenTxCheckpoints: string[] = [] + const callbacks: EdgeCurrencyEngineCallbacks = { + onAddressChanged: noOp, + onAddressesChecked: (ratio: number) => { + addressesCheckedRatios.push(ratio) + }, + onBalanceChanged: noOp, + onBlockHeightChanged: noOp, + onNewTokens: noOp, + onSeenTxCheckpoint: (checkpoint: string) => { + seenTxCheckpoints.push(checkpoint) + }, + onStakingStatusChanged: noOp, + onTokenBalanceChanged: noOp, + onTransactions: noOp, + onTransactionsChanged: noOp, + onTxidsChanged: noOp, + onUnactivatedTokenIdsChanged: noOp, + onWcNewContractCall: noOp + } + const engineOpts: EdgeCurrencyEngineOptions = { + callbacks, + log: testLog, + walletLocalDisklet: fakeIoDisklet, + walletLocalEncryptedDisklet: fakeIoDisklet, + customTokens: {}, + enabledTokenIds: [], + userSettings: {} + } + + // The engine is never started, so nothing populates the address + // subscribe cache (startEngine's initializeAddressSubscriptions and + // setLookAhead are what fill it, network-free). A stopped engine is in + // the same state: stop() clears the task cache. saveTx must still + // resolve there. + const engine: EdgeCurrencyEngine = await plugin.makeCurrencyEngine( + { type: tests.WALLET_TYPE, keys, id: '!' }, + engineOpts + ) + + const scriptPubkeyBuffer = Buffer.from(OWN_SCRIPT_PUBKEY, 'hex') + const edgeTx: EdgeTransaction = { + blockHeight: 0, + currencyCode: 'TESTBTC', + date: 1723000000, + isSend: true, + memos: [], + nativeAmount: '-50000', + networkFee: '1000', + networkFees: [], + otherParams: { + psbt: { + base64: '', + inputs: [ + { + hash: Buffer.from(SPENT_TXID, 'hex').reverse(), + index: 0, + value: 16250000, + scriptPubkey: scriptPubkeyBuffer, + sequence: 0xffffffff + } + ], + outputs: [ + { + value: 16200000, + scriptPubkey: scriptPubkeyBuffer + } + ] + } + }, + ourReceiveAddresses: [], + signedTx: '0100000000', + tokenId: null, + txid: NEW_TXID, + walletId: '!' + } + + // The regression under test: updateProgressRatio used to throw + // 'No addresses to process' here, failing saveTx AFTER the transaction + // had already been saved and its inputs marked spent. + await engine.saveTx(edgeTx) + + const txs = await engine.getTransactions({ tokenId: null }) + assert.isTrue( + txs.some(tx => tx.txid === NEW_TXID), + 'saved transaction should be listed' + ) + + // A second saveTx spends the first transaction's output. The first + // call's setLookAhead derived lookahead addresses into the subscribe + // cache, so this call has a nonzero progress denominator; the first + // call's denominator-less pass must not have counted as progress. + const edgeTx2: EdgeTransaction = { + ...edgeTx, + otherParams: { + psbt: { + base64: '', + inputs: [ + { + hash: Buffer.from(NEW_TXID, 'hex').reverse(), + index: 0, + value: 16200000, + scriptPubkey: scriptPubkeyBuffer, + sequence: 0xffffffff + } + ], + outputs: [ + { + value: 16150000, + scriptPubkey: scriptPubkeyBuffer + } + ] + } + }, + txid: SECOND_TXID + } + await engine.saveTx(edgeTx2) + + const txsAfterSecond = await engine.getTransactions({ tokenId: null }) + assert.isTrue( + txsAfterSecond.some(tx => tx.txid === SECOND_TXID), + 'second saved transaction should be listed' + ) + + // Progress bookkeeping must not fabricate a completed sync out of + // saveTx-driven processing: an engine that never synced must not emit a + // fully-synced ratio and must not advance the seen-tx checkpoint. + assert.notInclude( + addressesCheckedRatios, + 1, + 'saveTx must not produce a fully-synced progress emission' + ) + assert.isEmpty( + seenTxCheckpoints, + 'saveTx must not advance the seen-tx checkpoint' + ) + }) +})