-
Notifications
You must be signed in to change notification settings - Fork 18
Never report a successful broadcast as a failed send #455
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| import { asMaybe, asObject, asString } from 'cleaners' | ||
| import { EdgeIo, EdgeLog, EdgeTransaction } from 'edge-core-js/types' | ||
| import { parse } from 'uri-js' | ||
|
|
||
|
|
@@ -367,10 +368,76 @@ export function makeServerStates(config: ServerStateConfig): ServerStates { | |
|
|
||
| const instance: ServerStates = { | ||
| async broadcastTx(transaction: EdgeTransaction): Promise<string> { | ||
| // Query the network for the transaction to determine whether a failed | ||
| // broadcast actually reached the network anyway. A server can relay | ||
| // the transaction and still return an error or fail to respond, so an | ||
| // error from every server does not prove the transaction wasn't sent. | ||
| 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 | ||
| } | ||
|
|
||
| // 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') | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 Failure direction is safe: with no NOWNode config or key, 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 |
||
| .map(config => config.uris) | ||
| .flat(1) | ||
|
Comment on lines
+387
to
+393
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 Either gate it on |
||
| 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 | ||
| } | ||
|
Comment on lines
+394
to
+408
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The NOWNodes api-key goes to hosts that aren't NOWNodes. 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 |
||
| return false | ||
| } | ||
|
|
||
| return await new Promise((resolve, reject) => { | ||
| let resolved = false | ||
| let bad = 0 | ||
|
|
||
| // Reject with the given error only when the transaction is verifiably | ||
| // absent from the network; a transaction that reached the network | ||
| // despite the error is a successful broadcast. | ||
| const rejectUnlessTxKnown = (error?: Error): void => { | ||
| const fail = (): void => { | ||
| const msg = error != null ? `With error ${error.message}` : '' | ||
| log.error( | ||
| `broadcastTx fail: ${JSON.stringify(transaction)}\n${msg}` | ||
| ) | ||
| reject(error) | ||
| } | ||
| 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) | ||
|
Comment on lines
+427
to
+438
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No propagation delay means this will usually return 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 |
||
| } | ||
|
|
||
| const wsUris = Object.keys(serverStatesCache).filter( | ||
| uri => serverStatesCache[uri].blockbook != null | ||
| ) | ||
|
|
@@ -394,11 +461,7 @@ export function makeServerStates(config: ServerStateConfig): ServerStates { | |
| }) | ||
| .catch((e?: Error) => { | ||
| if (++bad === wsUris.length) { | ||
| const msg = e != null ? `With error ${e.message}` : '' | ||
| log.error( | ||
| `broadcastTx fail: ${JSON.stringify(transaction)}\n${msg}` | ||
| ) | ||
| reject(e) | ||
| rejectUnlessTxKnown(e) | ||
| } | ||
| }) | ||
| } | ||
|
|
@@ -464,11 +527,7 @@ export function makeServerStates(config: ServerStateConfig): ServerStates { | |
| }) | ||
| .catch((e?: Error) => { | ||
| if (++bad === nowNodeUris.length) { | ||
| const msg = e != null ? `With error ${e.message}` : '' | ||
| log.error( | ||
| `broadcastTx fail: ${JSON.stringify(transaction)}\n${msg}` | ||
| ) | ||
| reject(e) | ||
| rejectUnlessTxKnown(e) | ||
| } | ||
| }) | ||
| } | ||
|
|
@@ -670,3 +729,11 @@ export function makeServerStates(config: ServerStateConfig): ServerStates { | |
|
|
||
| return instance | ||
| } | ||
|
|
||
| /** | ||
| * Minimal shape of a Blockbook REST `/api/v2/tx/<txid>` response, used only | ||
| * to confirm that a transaction is known to the network. | ||
| */ | ||
| const asTxQueryResponse = asObject({ | ||
| txid: asString | ||
| }) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -432,7 +432,11 @@ 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. | ||
| log.warn( | ||
| `broadcast response txid mismatch: expected ${transaction.txid} received ${id}` | ||
| ) | ||
|
Comment on lines
+435
to
+439
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed that throwing here was wrong post-broadcast, but returning Since |
||
| } | ||
| return transaction | ||
| }, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -170,8 +170,12 @@ export function makeUtxoEngineProcessor( | |
| // 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') | ||
| // 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 | ||
|
Comment on lines
+173
to
+178
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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." 2. Combined with
That emits |
||
|
|
||
| const percent = processedCount / expectedProcessCount | ||
| if (percent - processedPercent > CACHE_THROTTLE || percent === 1) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,168 @@ | ||
| 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' | ||
|
|
||
| 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<void> => { | ||
| 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: EdgeCurrencyPlugin = factory(pluginOpts) as any | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| 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 } | ||
|
|
||
| const callbacks: EdgeCurrencyEngineCallbacks = { | ||
| onAddressChanged: noOp, | ||
| onAddressesChecked: noOp, | ||
| onBalanceChanged: noOp, | ||
| onBlockHeightChanged: noOp, | ||
| onNewTokens: noOp, | ||
| onSeenTxCheckpoint: noOp, | ||
| 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 no blockbook connects and no address | ||
| // is ever subscribed. This is the same state as a running engine whose | ||
| // sockets are all down: taskCache.addressSubscribeCache is empty. | ||
| 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' | ||
| ) | ||
| }) | ||
| }) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unbounded latency on the send path. This loop
awaits each blockbook serially, andSocket.tsuses a 30s per-request timeout (constants.tsMAX_CONNECTIONS = 2), so this alone can add ~60s. The NOWNodes loop below then adds N moreio.fetchCorscalls 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
fetchCorsnever settles — before being told the send failed.Suggest racing these concurrently and wrapping the whole
isTxidKnowncall in an overall deadline (~5-10s), falling back tofail()on timeout.