From 51d546d509177d6aebda2bbee2e2d546ad41e78e Mon Sep 17 00:00:00 2001 From: highlander Date: Fri, 14 Aug 2026 21:39:32 -0600 Subject: [PATCH] fix(evm): fail over on Chrome network errors instead of claiming the vault is down MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A dApp eth_sendTransaction failed with "KeepKey Vault is not running" while the vault was running. Two message-text classifiers combined to turn a dead Ethereum RPC into a false claim about the vault: isTransientRpcError("Failed to fetch") -> false => failover aborts isVaultUnreachableError("Failed to fetch") -> true => "Vault not running" Chrome throws byte-identical text for an unreachable RPC and a closed vault, so no regex can separate them. - isTransientRpcError now covers browser connection-level wording (failed to fetch / load failed / err_ / aborted). It was written against Firefox/Node wording, so the same dead RPC failed over on Firefox and hard-threw on Chrome. The loop now tries the remaining URLs. - Collapse the two copies of the classifier (ethereumHandler + rpcFailover) into one export; they had already drifted. - Log the URL before both definitive throws. That branch was silent while the transient branch logged, so the failing RPC never appeared in the console — the single biggest reason this was misdiagnosed as a vault problem. - formatUserError probes localhost:1646 before blaming the vault, instead of inferring vault state from an arbitrary error string in a catch-all. Tests: browser error strings in rpcFailover.test.ts; utils.test.ts asserts an RPC-origin "Failed to fetch" does NOT produce VAULT_REQUIRED_MESSAGE when the vault answers. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/background/chains/ethereumHandler.ts | 50 ++----------------- .../src/background/chains/rpcFailover.test.ts | 18 +++++++ .../src/background/chains/rpcFailover.ts | 46 ++++++++++++++++- chrome-extension/src/background/index.ts | 2 +- chrome-extension/src/background/methods.ts | 2 +- chrome-extension/src/background/utils.test.ts | 42 +++++++++++----- chrome-extension/src/background/utils.ts | 31 +++++++++--- 7 files changed, 124 insertions(+), 67 deletions(-) diff --git a/chrome-extension/src/background/chains/ethereumHandler.ts b/chrome-extension/src/background/chains/ethereumHandler.ts index 87eefdc..0807b53 100644 --- a/chrome-extension/src/background/chains/ethereumHandler.ts +++ b/chrome-extension/src/background/chains/ethereumHandler.ts @@ -20,6 +20,7 @@ import * as wallet from '../wallet'; import { buildFeeWarning, getFeeFloor, getPriorityFeeFloor, type FeeChoice, type FeeWarning } from './feeFloors'; import { openSidePanel, setApprovalBadge } from '../popup'; import { getChainInfo, makeStaticProvider } from './registry'; +import { isTransientRpcError } from './rpcFailover'; import { getLastResortRpcs } from './lastResortRpcs'; const TAG = ' | ethereumHandler | '; @@ -1610,50 +1611,6 @@ async function getCandidateRpcs(): Promise<{ return { availableRpcs, networkId, chainIdRaw: currentProvider.chainId ?? '' }; } -/** - * Heuristic: is this RPC error worth retrying against a different URL? - * Used by withRpcFailover (read calls). Broadcast has its own - * classifier because it has additional tx-level definitive cases - * (insufficient funds, nonce too low, etc.). - * - * Includes "method-rejection" patterns because narrow-purpose RPCs in - * Pioneer's catalog (Flashbots' rpc.flashbots.net is the canonical - * example — only supports eth_sendRawTransaction / eth_chainId / - * eth_blockNumber, rejects everything else with HTTP 403 + JSON-RPC - * code -32601 "rpc method is not whitelisted") would otherwise be - * sticky: their pre-flight `getBlockNumber()` test passes, so they get - * picked first on every read, and every read fails 403. Treating the - * rejection as transient lets the loop blacklist them for 60s and try - * the next URL. - */ -const isTransientRpcError = (errMsg: string): boolean => { - const m = errMsg.toLowerCase(); - return ( - m.includes('rate limit') || - m.includes('throttle') || - m.includes('429') || - m.includes('timeout') || - m.includes('econnreset') || - m.includes('etimedout') || - m.includes('network') || - m.includes('server_error') || - m.includes('exceeded maximum retry') || - /\b5\d{2}\b/.test(m) || // 5xx HTTP code - // Method-rejection: this URL doesn't support this method. Try next. - m.includes('rpc method is not whitelisted') || - m.includes('method not found') || - m.includes('method not supported') || - m.includes('method does not exist') || - m.includes('-32601') || - // Narrow to ethers' transport-level wrapper text. A bare `.includes('403')` - // would misfire on revert reasons or hex payloads that happen to - // contain "403", causing a successfully-rejected eth_call to be - // replayed across every URL and pointlessly cool them all. - m.includes('server response 403') || - m.includes('http 403') - ); -}; - /** * Run a read-style RPC call across the failover candidate list. Used * for preflight calls (nonce, gas estimate, fee data) where any working @@ -1683,7 +1640,10 @@ async function withRpcFailover( const errMsg = String(e?.message || e); if (!isTransientRpcError(errMsg)) { // Definitive (revert, invalid params, etc.) — won't help to - // try another RPC. Surface to caller. + // try another RPC. Surface to caller. Log the URL first: this + // branch used to throw silently while the transient branch below + // logged, so an RPC failure left no trace and got misdiagnosed. + console.error(tag, `RPC ${url} definitive failure, aborting failover:`, errMsg); throw e; } console.warn(tag, `RPC ${url} transient failure, trying next:`, errMsg); diff --git a/chrome-extension/src/background/chains/rpcFailover.test.ts b/chrome-extension/src/background/chains/rpcFailover.test.ts index 47d0766..fc2d411 100644 --- a/chrome-extension/src/background/chains/rpcFailover.test.ts +++ b/chrome-extension/src/background/chains/rpcFailover.test.ts @@ -41,6 +41,24 @@ describe('isTransientRpcError', () => { expect(isTransientRpcError('503 Service Unavailable')).toBe(true); }); + // Regression: written against Firefox/Node wording only, so a dead RPC + // hard-threw on Chrome instead of failing over — and the resulting error + // was mislabeled "KeepKey Vault is not running". + it('classifies browser connection-level failures as transient', () => { + expect(isTransientRpcError('Failed to fetch')).toBe(true); + expect(isTransientRpcError('TypeError: Failed to fetch')).toBe(true); + expect(isTransientRpcError('Load failed')).toBe(true); + expect(isTransientRpcError('net::ERR_NAME_NOT_RESOLVED')).toBe(true); + expect(isTransientRpcError('signal is aborted without reason')).toBe(true); + expect(isTransientRpcError('NetworkError when attempting to fetch resource.')).toBe(true); + }); + + it('classifies method-rejection (Flashbots-style narrow RPCs) as transient', () => { + expect(isTransientRpcError('rpc method is not whitelisted')).toBe(true); + expect(isTransientRpcError('server response 403 Forbidden')).toBe(true); + expect(isTransientRpcError('the method does not exist/is not available')).toBe(true); + }); + it('treats definitive RPC errors (revert / invalid params) as NOT transient', () => { expect(isTransientRpcError('execution reverted')).toBe(false); expect(isTransientRpcError('invalid params')).toBe(false); diff --git a/chrome-extension/src/background/chains/rpcFailover.ts b/chrome-extension/src/background/chains/rpcFailover.ts index ff7bb72..7f532cb 100644 --- a/chrome-extension/src/background/chains/rpcFailover.ts +++ b/chrome-extension/src/background/chains/rpcFailover.ts @@ -30,6 +30,28 @@ const FAILED_RPC_COOLDOWN_MS = 60_000; // blocking the other. const failedRpcs = new Map(); +/** + * Heuristic: is this RPC error worth retrying against a different URL? + * Shared by both failover loops (this module's by-networkId reads and + * ethereumHandler's active-provider path) so the two cannot drift. + * Broadcast keeps its own classifier — it has tx-level definitive cases + * (insufficient funds, nonce too low) that don't apply to reads. + * + * The `failed to fetch` / `load failed` / `err_` group matters more than it + * looks: those are what Chrome and Safari throw for a connection-level + * failure (TLS handshake, DNS, refused). The original list was written + * against Firefox/Node wording ("NetworkError..."), so the same dead RPC + * failed over on Firefox and hard-threw on Chrome — and that hard throw + * reached a catch-all that mislabeled it "KeepKey Vault is not running". + * + * The method-rejection patterns cover narrow-purpose RPCs in Pioneer's + * catalog (Flashbots' rpc.flashbots.net is the canonical example — supports + * only eth_sendRawTransaction / eth_chainId / eth_blockNumber, rejects the + * rest with HTTP 403 + JSON-RPC -32601). Without them such URLs are sticky: + * their pre-flight getBlockNumber() passes so they get picked first, then + * every read fails. Treating the rejection as transient blacklists them for + * 60s and moves on. + */ export const isTransientRpcError = (errMsg: string): boolean => { const m = errMsg.toLowerCase(); return ( @@ -42,7 +64,24 @@ export const isTransientRpcError = (errMsg: string): boolean => { m.includes('network') || m.includes('server_error') || m.includes('exceeded maximum retry') || - /\b5\d{2}\b/.test(m) // 5xx + /\b5\d{2}\b/.test(m) || // 5xx + // Connection-level failure, browser wording. + m.includes('failed to fetch') || // Chrome / Edge + m.includes('load failed') || // Safari + m.includes('fetch failed') || // Node / undici + m.includes('err_') || // Chrome net errors: ERR_NAME_NOT_RESOLVED, ERR_CONNECTION_REFUSED, ... + m.includes('aborted') || // per-attempt AbortSignal.timeout fired + // Method-rejection: this URL doesn't support this method. Try next. + m.includes('rpc method is not whitelisted') || + m.includes('method not found') || + m.includes('method not supported') || + m.includes('method does not exist') || + m.includes('-32601') || + // Narrow to ethers' transport-level wrapper text. A bare `.includes('403')` + // would misfire on revert reasons or hex payloads that happen to contain + // "403", replaying a successfully-rejected eth_call across every URL. + m.includes('server response 403') || + m.includes('http 403') ); }; @@ -115,7 +154,10 @@ export async function withRpcFailoverByNetworkId( } catch (e: any) { const errMsg = String(e?.message || e); if (!isTransientRpcError(errMsg)) { - // Definitive — won't help to try another RPC. + // Definitive — won't help to try another RPC. Log the URL: this + // branch used to throw silently, which made RPC failures look like + // they came from somewhere else entirely. + console.error(`[rpcFailover] ${networkId} ${url} definitive failure, aborting failover:`, errMsg); throw e; } console.warn(`[rpcFailover] ${networkId} ${url} transient failure, trying next:`, errMsg); diff --git a/chrome-extension/src/background/index.ts b/chrome-extension/src/background/index.ts index a81f100..9ef3660 100644 --- a/chrome-extension/src/background/index.ts +++ b/chrome-extension/src/background/index.ts @@ -1237,7 +1237,7 @@ chrome.runtime.onMessage.addListener((message: any, sender: any, sendResponse: a `[HANDOFF] BEX → content script (${chain}/${method}) ERROR\n params=${JSON.stringify(params)}\n error=`, error, ); - sendResponse({ error: formatUserError(error) }); + sendResponse({ error: await formatUserError(error) }); } } else { sendResponse({ error: 'Invalid request: missing method' }); diff --git a/chrome-extension/src/background/methods.ts b/chrome-extension/src/background/methods.ts index 3b28adf..9572804 100644 --- a/chrome-extension/src/background/methods.ts +++ b/chrome-extension/src/background/methods.ts @@ -309,7 +309,7 @@ const routeWalletRequest = async ( } // Translate "No device connected" SdkError into user-facing message - errorMessage = formatUserError({ message: errorMessage }); + errorMessage = await formatUserError({ message: errorMessage }); //push error to the popup // Forward `kind` so the side panel can render category-specific UI diff --git a/chrome-extension/src/background/utils.test.ts b/chrome-extension/src/background/utils.test.ts index 2f4252c..aef273f 100644 --- a/chrome-extension/src/background/utils.test.ts +++ b/chrome-extension/src/background/utils.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect } from 'vitest'; +import { describe, it, expect, vi, afterEach } from 'vitest'; import { createProviderRpcError, createTimeoutError, @@ -61,27 +61,45 @@ describe('isVaultUnreachableError', () => { }); describe('formatUserError', () => { - it('translates a vault-unreachable network error into the launch instruction', () => { - expect(formatUserError(new Error('TypeError: Failed to fetch'))).toBe(VAULT_REQUIRED_MESSAGE); + // formatUserError probes localhost:1646 before blaming the vault, so every + // case here has to say whether the vault is up. `vaultUp(false)` = closed. + const vaultUp = (up: boolean) => + vi.stubGlobal( + 'fetch', + vi.fn(() => (up ? Promise.resolve(new Response('ok')) : Promise.reject(new TypeError('Failed to fetch')))), + ); + + afterEach(() => vi.unstubAllGlobals()); + + it('translates a vault-unreachable network error into the launch instruction', async () => { + vaultUp(false); + await expect(formatUserError(new Error('TypeError: Failed to fetch'))).resolves.toBe(VAULT_REQUIRED_MESSAGE); + }); + + // The regression this guards: Chrome throws the identical string for a dead + // Ethereum RPC, and users were told to launch a vault that was already up. + it('does NOT blame the vault when the vault answers', async () => { + vaultUp(true); + await expect(formatUserError(new Error('TypeError: Failed to fetch'))).resolves.toBe('TypeError: Failed to fetch'); }); - it('translates the vault "No device connected" error into a friendly message', () => { + it('translates the vault "No device connected" error into a friendly message', async () => { const e = new Error('SdkError: No device connected'); - expect(formatUserError(e)).toBe('Please connect your KeepKey device and try again.'); + await expect(formatUserError(e)).resolves.toBe('Please connect your KeepKey device and try again.'); }); - it('passes other error messages through unchanged', () => { - expect(formatUserError(new Error('replacement transaction underpriced'))).toBe( + it('passes other error messages through unchanged', async () => { + await expect(formatUserError(new Error('replacement transaction underpriced'))).resolves.toBe( 'replacement transaction underpriced', ); }); - it('handles non-Error values by stringifying them', () => { - expect(formatUserError('plain string failure')).toBe('plain string failure'); + it('handles non-Error values by stringifying them', async () => { + await expect(formatUserError('plain string failure')).resolves.toBe('plain string failure'); }); - it('does not throw on null/undefined input', () => { - expect(() => formatUserError(null)).not.toThrow(); - expect(() => formatUserError(undefined)).not.toThrow(); + it('does not throw on null/undefined input', async () => { + await expect(formatUserError(null)).resolves.toBeDefined(); + await expect(formatUserError(undefined)).resolves.toBeDefined(); }); }); diff --git a/chrome-extension/src/background/utils.ts b/chrome-extension/src/background/utils.ts index 79235d0..02b7c42 100644 --- a/chrome-extension/src/background/utils.ts +++ b/chrome-extension/src/background/utils.ts @@ -40,24 +40,43 @@ export const VAULT_REQUIRED_MESSAGE = export const createVaultRequiredError = (): ProviderRpcError => createProviderRpcError(4900, VAULT_REQUIRED_MESSAGE); /** - * True when an error came from the vault REST server being down. The signing - * path fetches localhost:1646; when the vault is closed that rejects with a - * network error whose message varies by browser/runtime ("Failed to fetch", - * "Load failed", "NetworkError", "ECONNREFUSED"). + * True when an error *could* have come from the vault REST server being down. + * The signing path fetches localhost:1646; when the vault is closed that + * rejects with a network error whose message varies by browser/runtime + * ("Failed to fetch", "Load failed", "NetworkError", "ECONNREFUSED"). + * + * Deliberately NOT sufficient on its own. Chrome throws the exact same + * "Failed to fetch" for a dead Ethereum RPC, so this test alone told users to + * launch a vault that was already running. `formatUserError` confirms with a + * live probe before claiming the vault is down — see `isVaultReachable`. */ export function isVaultUnreachableError(msg: string): boolean { return /failed to fetch|load failed|networkerror|econnrefused|fetch failed|err_connection_refused/i.test(msg); } +/** + * Ask the vault directly instead of guessing from error text. Cheap + * (localhost, only runs on an already-failed request) and authoritative, + * unlike the 5s-stale KEEPKEY_STATE poll. Same endpoint `checkKeepKey()` uses. + */ +export async function isVaultReachable(): Promise { + try { + await fetch('http://localhost:1646/docs', { signal: AbortSignal.timeout(1500) }); + return true; + } catch { + return false; + } +} + /** * Translate low-level errors into user-facing messages: * - vault unreachable (localhost:1646 down) → "Vault not running" instruction * - vault SdkError ("No device connected") → connect-device instruction * All other errors pass through unchanged. */ -export function formatUserError(err: unknown): string { +export async function formatUserError(err: unknown): Promise { const msg = (err as Error)?.message ?? String(err); - if (isVaultUnreachableError(msg)) { + if (isVaultUnreachableError(msg) && !(await isVaultReachable())) { return VAULT_REQUIRED_MESSAGE; } if (msg.includes('No device connected')) {