Broadcast to every server at once, and stop leaking the NOWNodes key - #458
Conversation
`ServerConfig` had a single type, `blockbook-nownode`, so when Edge's own Blockbook hosts were added in Dec 2024 (9962d7e) they were filed under it alongside the real NOWNodes entries. Every HTTP broadcast therefore sent the NOWNodes key to Edge's servers as well. They are not the same kind of server: GET https://btc-wusa1.edge.app/api/v2 -> 200, no key needed GET https://btcbook.nownodes.io/api/v2 -> 401 Unknown API_key The type becomes `blockbook | blockbook-nownode`. The ten edge.app HTTP entries are now plain `blockbook`, the nownodes.io entries are unchanged, and the key is attached per target rather than to the whole batch. This also fixes the no-key case. A missing `nowNodesApiKey` previously rejected the entire HTTP path with "Missing connection key for fallback servers", even though the public servers need no key. NOWNodes targets are now skipped with a warning and the public servers still carry the broadcast; the reject is reserved for having no usable server at all. Adds test/common/utxobased/engine/ServerStates.spec.ts covering the header routing, both no-key cases, and the existing HTTP broadcast behavior it has to preserve.
fa5f837 to
f809273
Compare
j0ntz
left a comment
There was a problem hiding this comment.
Reviewed alongside EdgeApp/edge-react-gui#6190; the findings are inline.
Nit: the commit subject Broadcast to every server at once instead of gating HTTP on the sockets is 71 characters, over Edge's 50-character limit. Broadcast to every server at once is 33 and the body already carries the rest. (Send the NOWNodes API key only to NOWNodes servers is exactly 50 and fine.)
j0ntz
left a comment
There was a problem hiding this comment.
Correcting the verdict on my earlier review: those findings are blocking, not advisory.
The broadcast hang is the one that gates this. An all-fail broadcast with any entry in serverStatesCache settles neither way, and paired with EdgeApp/edge-react-gui#6190 the send scene has no error to show and no finally to unlock the slider.
promisifyWsMessage wraps every request in a Deferred that is only ever resolved from inside its own generator, on the happy path. The socket reports every failure by throwing into that generator: - an error response in Socket.onMessage - the 30 second request expiry in Socket.onTimer - the socket closing in Socket.onSocketClose The generator had no try/catch, so the throw was logged by the socket and the deferred was left pending forever. Every caller of promisifyWsMessage (fetchInfo, fetchAddress, fetchAddressUtxos, fetchTransaction, ping, broadcastTx) therefore hung on a failed request instead of seeing the error. The generator now catches the throw and rejects its deferred. Found in review of the broadcast fan-out that follows: once every socket attempt is counted, an all-fail broadcast with a cached socket could never settle without this.
f809273 to
0d32978
Compare
|
Subject reworded to |
0d32978 to
32c5992
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 32c5992. Configure here.
ce3c96a to
b1d77cf
Compare
`broadcastTx` used the HTTP servers only when no WebSocket reported itself connected. A socket that looks connected but never answers `sendTransaction` therefore failed the entire broadcast, after the 30 second request timeout, without a single HTTP attempt being made. That is the shape of the incident this task exists for (Asana 1217135300337949): the send failed in the UI while the transaction was already on the network. The gate is worse than it looks, because the NOWNodes WebSocket URI is docked 400 points at score load (ServerScores.serverScoresLoad penalises any URI carrying key params) and so rarely wins one of the two connection slots. HTTP is the realistic route to those servers, and it was reachable only when every socket was visibly down. broadcastTx now fires in two waves. The first goes out immediately to every cached blockbook (sockets still connecting included, since a queued request transmits as soon as the socket opens) and to every `blockbook` HTTP server, which are Edge's own. The second wave, the `blockbook-nownode` HTTP servers, fires after NOWNODES_BROADCAST_DELAY_MS or as soon as every first-wave attempt has failed, whichever comes first. NOWNodes is a third party, so it only sees a transaction when Edge's own infrastructure has not already carried it. The first success resolves; the promise rejects only once every attempt has failed, logging each failure against its server. Every attempt is bounded by BROADCAST_ATTEMPT_TIMEOUT_MS (30 seconds, the same figure the socket layer uses to expire a request). Because each attempt counts toward "all attempts failed", each one has to settle; without the bound an HTTP server that accepted the connection and never answered could hold the broadcast open even after every socket had failed. The fetch itself is left to finish; only the attempt is settled as a failure. ServerStateConfig.broadcastTimeoutMs overrides the bound for tests. Extends the ServerStates spec with a real WebSocket server: one that accepts `sendTransaction` and never answers (NOWNodes is held back for the delay, then the broadcast resolves over HTTP well inside the socket timeout), one that refuses it alongside failing and hung HTTP servers (rejects instead of hanging), plus the two-wave timing in both directions and the api-key header reaching only NOWNodes.
b1d77cf to
d04bef3
Compare

CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
none
Description
Engine half of the "send failed in the UI but the money moved" incident. Asana: https://app.asana.com/1/9976422036640/project/1213880789473005/task/1217135300337949
GUI companion (independent, neither blocks the other): EdgeApp/edge-react-gui#6190
Three commits, review fixups squashed in. The first commit stands alone and can be reviewed, or landed, without the rest.
1.
Send the NOWNodes API key only to NOWNodes serversServerConfighad a single type,blockbook-nownode, so when Edge's own Blockbook hosts were added in Dec 2024 (9962d7e) they were filed under it alongside the real NOWNodes entries. Every HTTP broadcast therefore sent the NOWNodes key to Edge's servers too.They are not the same kind of server:
The type becomes
blockbook | blockbook-nownode. The tenedge.appHTTP entries are now plainblockbook, thenownodes.ioentries are unchanged, and the key is attached per target instead of to the whole batch.This also fixes the no-key case. A missing
nowNodesApiKeypreviously rejected the entire HTTP path with "Missing connection key for fallback servers", even though the public servers need no key. NOWNodes targets are now skipped with a warning and the public servers still carry the broadcast.2.
Reject socket requests when the socket failsFound in review.
promisifyWsMessagewraps each request in aDeferredthat was only ever resolved from inside its own generator. The socket reports every failure (error response, 30s expiry, socket close) by throwing into that generator, which had no try/catch, so the deferred stayed pending forever and the caller hung. That applied tofetchInfo,fetchAddress,fetchAddressUtxos,fetchTransactionandping, not onlybroadcastTx. The generator now rejects its deferred on throw.This has to precede the fan-out: once every socket attempt is counted, an all-fail broadcast with a cached socket could never settle without it.
3.
Broadcast to every server at onceThe HTTP servers were used only when no WebSocket reported itself connected. A socket that looks connected but never answers
sendTransactiontherefore failed the whole broadcast, after the 30 second request timeout, without a single HTTP attempt being made. That is the shape of the incident behind this task.The gate is worse than it looks: the NOWNodes WebSocket URI is docked 400 points at score load (
ServerScores.serverScoresLoadpenalises any URI carrying key params) and so rarely wins one of the two connection slots. HTTP is the realistic route to those servers, and it was reachable only when every socket was visibly down.broadcastTxnow fires in two waves. The first goes out immediately to every cached blockbook (sockets still connecting included, since a queued request transmits as soon as the socket opens) and to everyblockbookHTTP server, which are Edge's own. The second wave, theblockbook-nownodeHTTP servers, fires afterNOWNODES_BROADCAST_DELAY_MS(2s), or as soon as every first-wave attempt has failed, whichever comes first. The first success resolves; it rejects only once every attempt has failed, logging each failure against its server. Every attempt is bounded byBROADCAST_ATTEMPT_TIMEOUT_MS(30s, mirroring the socket request expiry), so a server that accepts the connection and never answers cannot hold the broadcast open; the socket layer already expired its own requests, HTTP had no bound until Bugbot pointed it out.The two waves are the answer to the review thread on unconditional HTTP. NOWNodes is a third party, so it only sees a transaction when Edge's own infrastructure has not already carried it. In the incident every socket was down and the HTTP fallback that saved the broadcast included Edge's own servers, so the first wave would very likely have succeeded on its own; the delay costs anything only in the double-failure case, and two seconds against the old thirty-second hang is nothing.
attemptscounts both waves up front, so an all-fail first wave cannot reject before the second has run.Testing
test/common/utxobased/engine/ServerStates.spec.ts(new) andBlockbook.spec.ts:api-keyheader on the NOWNodes request and not on the public onefetchInforejects when the server answers with an error (commit 2)sendTransactionand never answers: NOWNodes is held back for the delay, then the broadcast resolves through it well inside the 30s socket timeoutsendTransactionwith no HTTP server rejects instead of hangingFull suite: 1261 passing at commit 1, 1262 at commit 2, 1270 at HEAD (re-run after the squash; tree identical to the reviewed head).
tscandeslintclean.Note for reviewers
This deliberately does not include the broadcast-failure classification from #455. Per the direction agreed for this task, the app now hard-fails an ambiguous broadcast and says so, rather than trying to infer whether the transaction landed. The
saveTx"No addresses to process" fix from #455 is still valid and is not in this branch.Note
High Risk
Changes core transaction broadcast paths, third-party exposure timing, and API key handling—directly affects whether sends succeed or fail and whether txs leak to NOWNodes early.
Overview
Fixes UTXO sends that could fail in the UI while the transaction still propagated, by changing how
broadcastTxreaches Blockbook.broadcastTxnow fans out in two waves. The first wave immediately hits every cached Blockbook WebSocket (including sockets still connecting) and every EdgeblockbookHTTP host. NOWNodesblockbook-nownodeHTTP targets run in a second wave after 2 seconds, or immediately once the first wave has all failed—whichever comes first. The first success wins; the call rejects only when every attempt fails or hits a 30s per-attempt timeout (HTTP previously could hang indefinitely). This replaces the old rule that skipped HTTP whenever any socket looked connected, which let one silent socket block fallback entirely.Server typing and credentials:
ServerConfigis split intoblockbook(no headers) vsblockbook-nownode(api-keyonly on NOWNodes). Edge*.edge.appHTTP entries across coin configs are reclassified asblockbook, so the NOWNodes key is no longer sent to Edge. Without a key, NOWNodes servers are skipped with a warning while public Edge HTTP still broadcasts.Socket hang fix:
Blockbook.promisifyWsMessagenow rejects when the socket throws (errors, timeout, close), so broadcast and other WS calls settle instead of pending forever—required for counting all broadcast attempts.New
ServerStates.broadcastTxtests cover wave timing, header placement, timeouts, and socket+HTTP combinations;Blockbook.specadds error-response rejection.Reviewed by Cursor Bugbot for commit b1d77cf. Bugbot is set up for automated code reviews on this repo. Configure here.