Skip to content

fix(evm): don't report a broadcast transaction as failed (double-payment risk) - #30846

Open
pragmaxim wants to merge 4 commits into
developfrom
fix/evm-push-tx-timeout
Open

fix(evm): don't report a broadcast transaction as failed (double-payment risk)#30846
pragmaxim wants to merge 4 commits into
developfrom
fix/evm-push-tx-timeout

Conversation

@pragmaxim

@pragmaxim pragmaxim commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Description

A transaction push gets its own 60-second deadline so a slow private/MEV relay can no longer outlast the client. Previously the 20-second default deadline rejected the push and closed the socket while blockbook may still have broadcast the transaction, producing a misleading "send failed" toast that can end in a double payment (a retry signs the next nonce while both transactions mine).

While a push may now take tens of seconds the UI no longer misleads:

  • the send review modal shows the in-progress state (Send disabled, cancel disabled) on every network, and
  • pushTransaction no longer locks the device for the whole broadcast.

Two pending-transaction assumptions were also fixed so a relay-backed pending tx no longer aborts an in-flight replace-by-fee or breaks the whole addTransaction account batch.

Counterpart backend change: trezor/blockbook#1638.

Why

A transaction push is the one blockbook request whose loss cannot be shrugged off, and today we can lose it in a way that costs the user money.

websocket-client's DEFAULT_TIMEOUT is 20 s and applies to sendTransaction like any other request. Blockbook's own budget for that call is larger: an EVM coin routed through a private/MEV relay allows rpc_timeout (25 s on ETH) per relay URL for the broadcast alone, plus a post-send eth_getTransactionByHash. When our deadline wins, onMessageTimeout rejects the push and closes the socket, so:

  1. pushSendFormTransactionThunk takes the failure branch — a sign-tx-error toast, no fake pending tx, no privatePending record;
  2. blockbook has in fact broadcast the transaction;
  3. once it surfaces, the account's confirmedNonce/pending set advance past it, so a user who believes the send failed and retries gets the transaction signed with the next nonce;
  4. both transactions mine and the recipient is paid twice.

Step 3 is not a bug — resolveEthereumNonce advancing past known pending nonces is exactly the anti-nonce-reuse behaviour we want. The defect is the misleading failure in step 1.

What

fix(blockchain-link): give the push its own 60 s deadline.
sendMessage is used directly because only it takes a per-request timeout; send's overloaded signature has no room for one. The response type is unchanged, so this works against any blockbook version — including one that has not been patched to answer faster.

60 s rather than something larger is deliberate: the keep-alive ping fires after 50 s of silence carrying the default 20 s deadline, and any expiry rejects every in-flight request and closes the socket. Past ~70 s the push would be killed by the ping rather than by its own deadline, and a genuinely dead socket is still detected on the ping instead of being held open.

fix(suite): stop a slow push from locking the device and looking idle.
Raising the deadline is only safe if the seconds it may now take are not seconds of frozen UI and blocked device access. Two pre-existing problems made them exactly that:

  • The review modal set isSending only for solana | stellar | tron. On every other network — all EVM coins and all BTC-likes — the Send button stayed enabled and un-spinnered and the cancel "X" stayed live, on a modal that looks idle. Cancelling closes the modal but does not abort the push, which reproduces the same "told it failed while it was broadcast" outcome the deadline change exists to prevent. isSending is reset when a push fails and the modal closes either way, so setting it unconditionally introduces no stuck state.
  • pushTransaction was missing from the connect-init blacklist, so the TrezorConnect.call patch wrapped it in lockDevice(true) + synchronize() for its whole duration, queueing every other connect method behind a broadcast. PushTransaction sets useDevice: false, so it belongs on that list regardless; at a 60 s deadline it matters.

fix(suite): two pending-tx assumptions that break on a relay-backed pending tx.
Both are latent today and become reachable once blockbook exposes every relay-accepted send as pending (see below), including sends the relay never surfaces:

  • replaceByFeeErrorMiddleware treated blockHeight !== undefined as "the predecessor is mined". Blockbook always sends blockHeight and it is 0 for a mempool tx, so a still-pending predecessor matched: the RBF flow showed "the previous transaction was already mined" and cancelled a replacement that was in flight.
  • getEthereumRbfParams dereferenced vout[0].addresses![0] and tokens[0]. A pending contract creation has no recipient (blockbook serialises addresses as null), and a transfer call whose token transfer blockbook could not decode leaves tokens empty — either one threw inside the addTransaction action creator, which fails the whole account's transaction batch, not just that row. Neither transaction can offer a bump/cancel target, so both now return no rbfParams.

Relation to blockbook

Counterpart backend work: trezor/blockbook#1638. It attacks the same failure from the other side — the broadcast to every relay URL runs concurrently instead of sequentially (worst case one rpc_timeout instead of N), the post-send fetch-back no longer sits inside the wallet's wait, and an accepted send is cached from its own signed bytes so it is never exposed nowhere and its nonce is never handed out again.

The two changes are independent and each is worth having alone: this PR protects users running against an unpatched blockbook, and blockbook#1638 protects wallets that have not shipped this PR.

Notes for QA

Focus on sending on a blockbook-served network, especially EVM (ETH + ERC-20) and BTC:

  • After tapping Send, the review modal should show the in-progress state (disabled Send button / spinner, disabled cancel "X") for the whole broadcast — including when the backend is slow. Cancel must no longer be possible while the push is in flight.
  • The device must not stay locked during a broadcast: unrelated UI/device actions on another connected account stay responsive.
  • RBF on a still-pending EVM transaction should no longer show "previous transaction was already mined" and cancel the replacement.
  • A pending contract-creation or an undecodable transfer tx must not break the account's transaction list.
  • Scope: the 60 s deadline applies to every coin served by the blockbook worker (BTC-likes, all EVM, TRX, plus trading/staking/earn/WalletConnect/RBF flows). Solana, Stellar, XRP, Cardano and the custom evm-rpc backend are untouched.

Verification

  • Reviewed by reading, in both repos: the transport chain (sendMessagecreateDeferredManager.create(timeout)onTimeoutonMessageTimeout), the Push export and the deep import path, isPending's re-export from @suite-common/wallet-utils, TokenTransfer.to being non-optional, and the isSending state owner (reset on push failure, modal closed either way).
  • Not run locally: tsc, jest and eslint — the environment these commits were authored in has no node_modules installed, so CI is the first typecheck. The blast radius is small (5 files, no new dependency, no protocol change) but please treat green CI as a precondition rather than a formality.
  • Scope note: the 60 s deadline applies to every coin served by the blockbook worker (BTC-likes, all EVM, TRX, and the trading/staking/earn/WalletConnect/RBF flows that push through it). Solana, Stellar, XRP, Cardano and the custom evm-rpc backend use other workers and are untouched.

Deliberately not included

onMessageTimeout rejects all in-flight requests and closes the socket, so an unrelated slow request can still kill a push that is in flight. Rejecting only the expired promise needs a liveness re-check to replace the teardown, and the obvious ping-based version recurses when the ping itself expires. That is a shared-transport change affecting every backend and deserves its own PR with real test runs.

🤖 Generated with Claude Code

🌐 Preview deployments

🌐 Suite Web preview: https://dev.suite.sldev.cz/suite-web/fix/evm-push-tx-timeout/web/

🔍 Currents Test Results

🔍 Suite web test results: View in Currents

🔍 Suite desktop test results: View in Currents

🔍 Suite native android test results: View in Currents

🔒 Quarantined E2E Tests

Trezor Suite (desktop) — 2 test(s)
Test Type
Quarantine test: "Recovery - dry run,Recovery after partial recovery" 🙋 manual
Quarantine test: "Recovery - dry run,Recovery with device reconnection" 🙋 manual

Updated: 2026-08-05T07:31:30.413Z • 2 test(s) total

Trezor Suite (web) — 3 test(s)
Test Type
Quarantine test: "Recovery - dry run,Recovery with device reconnection" 🙋 manual
Quarantine test: "TrezorConnect webextension -> Suite Web,second call after popup was closed by user should work" 🙋 manual
Quarantine test: "Recovery T2T1 - dry run,Recovery after partial recovery" 🙋 manual

Updated: 2026-08-05T07:32:24.836Z • 3 test(s) total

🤖 LLM Test Recommendations

Summary: This change set spans transaction review UI, RBF error handling, Blockbook backend connectivity, TrezorConnect initialization, and transaction utilities. The highest risk regressions are in the transaction review modal and Blockbook WebSocket worker. Run the targeted send and backend tests first; the Connect and pending-transaction tests provide secondary coverage for less directly exercised files.

Changed files (5)
  • packages/blockchain-link/src/workers/blockbook/websocket.ts
  • packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewModalBottomContent.tsx
  • packages/suite/src/middlewares/wallet/replaceByFeeErrorMiddleware.ts
  • suite-common/connect-init/src/blacklist.ts
  • suite-common/wallet-utils/src/transactionUtils.ts

Recommended tests (7)

🔴 High priority (5)
  • suite/e2e/tests/wallet/send-form-regtest.test.ts — Directly exercises the transaction review modal bottom content, BTC send options that may trigger replaceByFeeErrorMiddleware, and uses a regtest Blockbook backend that relies on the blockchain-link websocket worker. Also depends on transactionUtils for output/transaction handling.
  • suite/e2e/tests/wallet/send-eth.test.ts — Confirms the transaction review modal bottom content renders and behaves correctly for Ethereum sends with custom fees, directly exercising TransactionReviewModalBottomContent.
  • suite/e2e/tests/wallet/send-sol.test.ts — Confirms the transaction review modal bottom content renders and behaves correctly for Solana sends, directly exercising TransactionReviewModalBottomContent.
  • suite/e2e/tests/wallet/coins-custom-backend.test.ts — Tests connecting to custom Blockbook backends and account loading, directly exercising the blockchain-link websocket worker that handles Blockbook connections. (Inferred coverage for the uncovered websocket file.)
  • suite/e2e/tests/wallet/blockbook-discovery.test.ts — Tests account discovery through custom Blockbook backends, directly exercising the blockchain-link websocket worker. (Inferred coverage for the uncovered websocket file.)
🟡 Medium priority (2)
  • suite/e2e/tests/trezor-connect/getAddress.test.ts — Tests the TrezorConnect initialization and address export flow, which may be affected by changes to the connect-init blacklist configuration.
  • suite/e2e/tests/wallet/pending-transactions.test.ts — Tests pending/confirmed transaction state transitions and transaction list grouping, which relies on transactionUtils and transaction-related middleware including RBF handling.

Updated: 2026-08-05T07:31:24.027Z

pragmaxim and others added 3 commits August 5, 2026 05:06
A push is the one blockbook request whose loss cannot be shrugged off. The
default 20s message deadline (websocket-client DEFAULT_TIMEOUT) is shorter than
blockbook's own budget for `sendTransaction`: EVM coins routed through a
private/MEV relay allow rpc_timeout (25s on ETH) per relay URL for the broadcast
alone, plus its post-send fetch-back. When the deadline wins, onMessageTimeout
rejects the push and closes the socket, so the user is told the send failed while
blockbook has in fact broadcast it - and a re-send is then signed with the *next*
nonce (confirmedNonce advanced past the pending tx once it surfaces), paying the
recipient twice.

Give the push 60s of its own instead, so the answer arrives while we are still
listening even against a blockbook that has not been patched to answer faster.
60s is the largest deadline that is really the push's own: the keep-alive ping
fires after 50s of silence with the default 20s deadline, and any expiry rejects
every in-flight request and closes the socket, so past ~70s the ping would kill
the push rather than its own deadline - and a genuinely dead socket is still
detected on the ping instead of being held open.

sendMessage is used directly because only it accepts a per-request timeout; the
overloaded `send` signature has no room for one. The response type is unchanged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Raising the push deadline is only safe if the seconds it may now take are not
seconds of frozen UI and blocked device access. Two things made them exactly
that, both of which predate the deadline change and are worsened by it.

The review modal set isSending only for solana, stellar and tron. On every other
network - all EVM coins and all BTC-likes - the Send button stayed enabled and
un-spinnered and the cancel "X" stayed live while the push was in flight, on a
modal that looks idle. Cancelling closes the modal but does not abort the push
(pushSendFormTransactionThunk is already past the point of no return), which
produces precisely the "user told it failed while blockbook broadcast it" outcome
the longer deadline exists to prevent. isSending is reset when a push fails and
the modal closes either way, so setting it unconditionally has no stuck state.

pushTransaction was not in the connect-init blacklist, so the TrezorConnect.call
patch wrapped it in lockDevice(true) + synchronize() for its whole duration -
queueing every other connect method behind a broadcast and greying out unrelated
UI. PushTransaction sets useDevice: false, so it belongs on that list regardless;
at a 60s deadline it matters.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ending tx

Both are latent today and become reachable when blockbook exposes every
relay-accepted send as pending (trezor/blockbook, "never lose a relay-accepted
send"), including sends the relay never surfaces.

replaceByFeeErrorMiddleware treated `blockHeight !== undefined` as "the
predecessor is mined". Blockbook always sends blockHeight and it is 0 for a
mempool tx, so a still-PENDING predecessor matched: the RBF flow showed "the
previous transaction was already mined" and cancelled a replacement that was in
flight. Only a genuinely mined predecessor may abort it.

getEthereumRbfParams dereferenced `vout[0].addresses![0]` and `tokens[0]`. A
pending contract creation has no recipient and blockbook serialises `addresses`
as null for it, and a `transfer` call whose token transfer blockbook could not
decode leaves `tokens` empty - either one threw inside the addTransaction action
creator, which fails the whole account's transaction batch, not just that row.
Neither tx can offer a bump or cancel target, so both cases return no rbfParams.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-project This label is used to specify that PR doesn't need to be added to a project

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant