Never report a successful broadcast as a failed send - #455
Conversation
UtxoEngineProcessor's updateProgressRatio threw 'No addresses to process' whenever the engine had zero subscribed addresses, which is the state of a wallet whose blockbook sockets are all down. saveTx reaches this code via processUtxos after the transaction is already saved and its inputs marked spent, so the throw turned an already-successful send into a reported failure. Skip the progress update instead; there is no denominator to compute a ratio from without subscribed addresses.
ServerStates.broadcastTx submits the signed transaction to every connected blockbook (or every NOWNode HTTP fallback) and rejects only when all of them fail, but a server can relay the transaction to the network and still return an error or fail to respond. Before rejecting, query the network for the txid and treat a known transaction as a successful broadcast. Also stop throwing on a mismatched broadcast-response txid in UtxoEngine.broadcastTx: the transaction is already on the network at that point, so log a warning instead of reporting a send failure.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
| const { nowNodesApiKey } = initOptions | ||
| if (nowNodesApiKey == null) return false | ||
| const nowNodeUris = serverConfigs | ||
| .filter(config => config.type === 'blockbook-nownode') |
There was a problem hiding this comment.
Why this query is hard-coded to NOWNode: the checker must work in the exact state the incident happened in (zero connected blockbooks), and the only transport the engine is configured to reach there is the NOWNode REST API. The sendtx fallback ~90 lines below already restricts itself to type === 'blockbook-nownode' + nowNodesApiKey (with a commented-out future hook for user-configured HTTP servers), so the query mirrors that exactly: whatever server could have relayed the transaction is the server we ask about it. Connected blockbooks are tried first, so NOWNode is only the last resort for the disconnected state.
Failure direction is safe: with no NOWNode config or key, isTxidKnown returns false and the broadcast rejects with the original error, which is the pre-fix behavior. The hard-coding can cost a missed rescue, never a false success.
Known drift risk: the sendtx fallback and this query each derive the NOWNode uri list + key independently. If the fallback ever broadens to other HTTP servers, this query must broaden with it; a shared resolveHttpFallbackServers() helper would collapse the two sites and is a clean follow-up.
peachbits
left a comment
peachbits
left a comment
There was a problem hiding this comment.
The first commit fixes a real crash, but the return lands in a spot that lets setLookAhead run in a state it previously never reached — see the UtxoEngineProcessor.ts comment. The second commit's verification step is defensible in principle (a node can relay a tx and still error), but as written it is serial, untimed, and unconditional, so it turns a fast send failure into a long one on the money path.
| const isTxidKnown = async (txid: string): Promise<boolean> => { | ||
| // Ask connected blockbook instances first: | ||
| for (const uri of Object.keys(serverStatesCache)) { | ||
| const { blockbook } = serverStatesCache[uri] | ||
| if (blockbook == null || !blockbook.isConnected) continue | ||
| const known = await blockbook | ||
| .fetchTransaction(txid) | ||
| .then(() => true) | ||
| .catch(() => false) | ||
| if (known) return true | ||
| } |
There was a problem hiding this comment.
Unbounded latency on the send path. This loop awaits each blockbook serially, and Socket.ts uses a 30s per-request timeout (constants.ts MAX_CONNECTIONS = 2), so this alone can add ~60s. The NOWNodes loop below then adds N more io.fetchCors calls with no timeout at all.
Before this change a total broadcast failure rejected immediately. Now the user sits on a spinner for a minute or more — potentially forever if a fetchCors never settles — before being told the send failed.
Suggest racing these concurrently and wrapping the whole isTxidKnown call in an overall deadline (~5-10s), falling back to fail() on timeout.
| // Fall back to the NOWNode HTTP API when no blockbook is connected: | ||
| const { nowNodesApiKey } = initOptions | ||
| if (nowNodesApiKey == null) return false | ||
| const nowNodeUris = serverConfigs | ||
| .filter(config => config.type === 'blockbook-nownode') | ||
| .map(config => config.uris) | ||
| .flat(1) |
There was a problem hiding this comment.
The comment says "when no blockbook is connected", but there is no such condition — this block runs on every exhausted broadcast, including the case where blockbooks are connected and simply returned false above.
Either gate it on !isAnyBlockbookConnected to match the intent (and match the broadcast fallback at L472), or fix the comment. As-is it silently widens both the latency and the api-key exposure below.
| for (const uri of nowNodeUris) { | ||
| const known = await io | ||
| .fetchCors(`${uri}/api/v2/tx/${txid}`, { | ||
| headers: { | ||
| 'api-key': nowNodesApiKey | ||
| } | ||
| }) | ||
| .then(async response => { | ||
| if (!response.ok) return false | ||
| const json = await response.json() | ||
| return asMaybe(asTxQueryResponse)(json)?.txid === txid | ||
| }) | ||
| .catch(() => false) | ||
| if (known) return true | ||
| } |
There was a problem hiding this comment.
The NOWNodes api-key goes to hosts that aren't NOWNodes. ServerConfig['type'] is a single-member union (types.ts:160), so .filter(config => config.type === 'blockbook-nownode') never excludes anything. And those URI lists are mixed — bitcoin.ts:54 has https://btc-wusa1.edge.app, https://btc-eu1.edge.app and https://btcbook.nownodes.io, all under the same type. Same in bitcoincash.ts, etc.
This is pre-existing in the broadcast fallback at L502-512, so not introduced here — but this PR replicates it on a new path that (per the comment above) fires far more often. Worth splitting the config type so the key is only attached to actual nownodes.io hosts, rather than duplicating the pattern.
| isTxidKnown(transaction.txid) | ||
| .then(known => { | ||
| if (!known) return fail() | ||
| if (!resolved) { | ||
| resolved = true | ||
| log.warn( | ||
| `broadcastTx errored, but txid ${transaction.txid} is known to the network; treating broadcast as a success` | ||
| ) | ||
| resolve(transaction.txid) | ||
| } | ||
| }) | ||
| .catch(fail) |
There was a problem hiding this comment.
No propagation delay means this will usually return false when it matters. isTxidKnown fires the instant the last broadcast rejects. In the exact race being defended against — the node relayed the tx but the response was lost — the tx has had ~0ms to be indexed into the queried server's mempool, so fetchTransaction / /api/v2/tx/ will very likely 404 and we fail() anyway.
A short delay plus one retry before giving up would make the check actually fire in the target scenario. Without it, the added latency buys little.
Minor: an early if (resolved) return at the top would also skip the whole query in the (currently unreachable, but cheap to guard) case where another server already resolved.
| // With no subscribed addresses there is no denominator to compute a | ||
| // progress ratio from. This is a legitimate state when processing is | ||
| // driven by saveTx on a disconnected engine (no blockbook sockets, so | ||
| // nothing is subscribed), so skip the progress update rather than fail | ||
| // the caller's data write. | ||
| if (expectedProcessCount === 0) return |
There was a problem hiding this comment.
Returning instead of throwing is right, but two things here:
1. The comment's premise is wrong. This is not "a running engine whose sockets are all down." initializeAddressSubscriptions() (L300) populates addressSubscribeCache from the DataLayer with no network involvement, and entries are only ever marked processing, never removed (L936-966). The cache is empty in exactly two states: the engine was never started, or it was stopped (clearTaskCache at L136). A started-but-disconnected engine has a full cache. Worth correcting here and in saveTx.spec.ts:112-113, since it changes how someone reasons about this branch.
2. setLookAhead now runs in a state it never reached before, and the counter is already dirty. Previously the throw on L177 short-circuited processDataLayerUtxos before setLookAhead(common) at L1583. Now it runs. setLookAhead itself is network-free and safe, but it repopulates addressSubscribeCache with the handful of addresses it newly derives — so the denominator goes from 0 to something very small.
Combined with processedCount being incremented above the guard on L171, a single saveTx with two scriptPubkeys (input + change — the common case) does:
- call 1:
expectedProcessCount = 0,processedCount → 1, return;setLookAheadderives 1 new address → cache size 1 - call 2:
expectedProcessCount = 2,processedCount → 2→percent === 1
That emits ADDRESSES_CHECKED(1) and calls updateSeenTxCheckpoint() on a stopped engine, advancing the seen-tx checkpoint to maxSeenTxBlockHeight without having synced. Moving the increment below the guard fixes it — a call with no denominator shouldn't count as progress.
| // The transaction is on the network at this point, so a mismatched | ||
| // response txid must not be reported as a send failure. | ||
| log.warn( | ||
| `broadcast response txid mismatch: expected ${transaction.txid} received ${id}` | ||
| ) |
There was a problem hiding this comment.
Agreed that throwing here was wrong post-broadcast, but returning transaction unchanged means the wallet persists transaction.txid while the network accepted id. The wallet then tracks a txid that doesn't exist on-chain: it never confirms, the UTXOs stay marked spent, and the funds look stuck — with no error surfaced anywhere the user can see.
Since formats includes non-segwit (bip44, bip32), a genuine txid change isn't purely hypothetical. Consider returning { ...transaction, txid: id } so the wallet tracks what the network actually has, or at minimum promote this above log.warn.
| const factory = edgeCorePlugins[tests.pluginId] | ||
| if (typeof factory !== 'function') | ||
| throw new Error(`Missing plugin factory for ${tests.pluginId}`) | ||
| const plugin: EdgeCurrencyPlugin = factory(pluginOpts) as any |
There was a problem hiding this comment.
const plugin: EdgeCurrencyPlugin = factory(pluginOpts) as any — the annotation and the as any cancel each other out. factory returns EdgeCorePlugin; narrowing with a type guard (or as EdgeCurrencyPlugin) keeps the assertion visible instead of disabling checking entirely.
2 participants
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
none
Description
Technical design doc
Fixes the engine half of Edge bug "Send - multiple transactions after network error": one intended Bitcoin send became five real payments because every failure the user saw happened AFTER a successful broadcast.
Asana: https://app.asana.com/1/9976422036640/project/1213843652804305/task/1217135300337949
Two defects fixed:
saveTxfailed on a disconnected engine.updateProgressRatiothrewNo addresses to processwhenever zero addresses were subscribed (exactly the state of a wallet whose blockbook sockets are down), andsaveTxreaches it viaprocessUtxosafter the transaction is already saved and its inputs marked spent. The throw is progress bookkeeping with no denominator, not a data error, so it is now a no-op instead of failing the caller's data write. Regression test:saveTxon a never-connected engine resolves (test/common/utxobased/engine/saveTx.spec.ts, red before the fix with the exact incident stack).Broadcast failure was ambiguous.
ServerStates.broadcastTxmulticasts to every connected blockbook (or every NOWNode HTTP fallback) and rejects only when all fail, but a server can relay the transaction and still return an error or time out. On the all-failed path the engine now queries the network for the txid (connected blockbooks first, NOWNodeGET /api/v2/tx/in fallback mode) and treats a known transaction as a successful broadcast; an unknown transaction still rejects with the original error, so genuinely failed broadcasts keep failing and stay retryable. The post-broadcast txid-mismatch throw inUtxoEngine.broadcastTx(dead code today, failure-after-success if ever revived) is now a logged warning.The GUI half (send scene must not present post-broadcast errors as failed sends) is the EdgeApp/edge-react-gui companion PR on branch
jon/send-post-broadcast-failure.Note
High Risk
Changes send/broadcast success vs failure semantics and local save paths after broadcast; incorrect network verification could mask real failures or accept wrong outcomes, though genuinely unknown txs still fail.
Overview
Fixes duplicate real payments when the UI showed send failures after the network had already accepted the transaction.
saveTxon a disconnected engine no longer fails after the tx is persisted:updateProgressRatioskips progress updates when no addresses are subscribed (emptyaddressSubscribeCache), instead of throwingNo addresses to process.Broadcast failure is verified before rejecting: when every Blockbook WebSocket or NOWNode HTTP broadcast errors,
ServerStates.broadcastTxqueries whether the txid is known (connected blockbooks first, then NOWNodeGET /api/v2/tx/). A known tx is treated as success; only an unknown tx still rejects with the original error.Post-broadcast txid mismatch in
UtxoEngine.broadcastTxis logged as a warning instead of throwing, so a relayed tx is not reported as a failed send.Adds regression test
saveTx.spec.tsfor a never-started engine.Reviewed by Cursor Bugbot for commit 0b5ce06. Bugbot is set up for automated code reviews on this repo. Configure here.