fix: one transaction approval at a time, and honest copy for a nonce collision (closes #271) #284

clawbot · 2026-08-14T06:12:57+02:00

clawbot commented

2026-08-14 06:12:57 +02:00

Closes #271.

The decision, and why

Of the three options the issue named, this takes the third: refuse a second transaction approval while one is pending.

Re-populate at Confirm reintroduces exactly the gap #216 closed. The nonce on the approval screen would no longer be the nonce that gets signed, so the artifact could not be verified against the displayed object without either lying about what was displayed or dropping the nonce from the comparison.
Allocate around in-flight approvals makes the wallet's own bookkeeping the authority on a nonce the network has not accepted. An approval the user abandons — the window left open, the worker terminated — then leaves a hole that every later transaction queues behind until something reconciles it.
Refuse keeps the displayed object the verified object, holds no state the network can contradict, and fails the second request while the page is still waiting and nothing has been shown. For a wallet that signs from one address at a time, one unanswered transaction is the natural limit.

What changed

src/background/index.js

A single transaction-approval slot, taken synchronously in the eth_sendTransaction branch before its first await — two requests delivered in the same tick cannot both pass it — and released when the requesting page has its answer (broadcast, rejected, or window closed). The second request is refused with EIP-1193 -32002 (resource unavailable, the standard code for "already pending") and never reaches prepareApprovalTx(): no second nonce, no second window. Signature approvals are not gated, consuming no nonce.
The body of the branch moved into handleSendTransaction() so the reservation and its finally read as one thing; the body itself is unchanged.
Nonces this worker has broadcast are recorded per address and checked before the artifact is handed to the node. A node's pending count can lag a transaction it has itself just accepted, and a request populated inside that window would otherwise be signed and sent at a nonce this wallet has already used. The record dies with the worker, which is right: after a restart the node's count is the only answer available.
Each sendResponse now reports outcome.stage rather than the stage it passed in, because a broadcast failure the node blamed on the nonce is reclassified.

src/shared/approvalVerify.js

isNonceCollision() and a new TX_STAGE_NONCE. Classified from ethers' NONCE_EXPIRED / REPLACEMENT_UNDERPRICED codes and from the node's own words (nonce too low, nonce has already been used, invalid nonce, OldNonce, replacement transaction underpriced, replacement fee too low), including the nested info.error.message shape ethers hands up when it could not classify the error itself.
already known is deliberately not a collision: a node that says it knows the transaction has it, so it did reach the network and the existing ambiguous wording is the correct one for it.
The copy. A nonce collision produces "The transaction was not sent, because its nonce had already been used by another transaction." to the page, and in the popup that plus "The transaction did not reach the network. Please send it again from the site." The node's own fragment (nonce too low) is replaced rather than passed through: it is not a sentence, and it says less than the wallet knows.

Verification is untouched. The approval still carries the transaction the screen displayed, and the artifact is still compared against that object field for field.

The new tests, failing first

Written against the unfixed tree and run there before any source change. make test on next + tests only: 7 failed, 682 passed.

● one transaction approval at a time › a second eth_sendTransaction while one is
  pending is refused before it takes a nonce

    expect(received).toEqual(expected) // deep equality

    Expected: {"error": {"code": -32002, "message": StringMatching /already waiting to be approved/}}
    Received: null

null is the whole defect: the second request had not answered the page, because it had raised its own approval window at the same nonce the first one is holding.

● a nonce collision is reported as a transaction that did not go out › a nonce
  this wallet already broadcast is refused without asking the node again

    expect(jest.fn()).toHaveBeenCalledTimes(expected)

    Expected number of calls: 1
    Received number of calls: 2

Two broadcasts, the second at a nonce this wallet had already used — the terminal failure the issue describes.

● a nonce collision is reported as a transaction that did not go out › a
  broadcast the node refused for the nonce is not reported as possibly sent

      Object {
    -   "error": StringMatching /nonce had already been used/,
    +   "error": "nonce too low",
        "retryable": false,
    -   "stage": "nonce",
    +   "stage": "broadcast",
      }

● signing failure and retry › a nonce collision says the transaction did not
  reach the network

    Expected pattern: /did not reach the network/
    Received string:  "nonce too low. This request can no longer be signed. Please start it again from the site."

Also red beforehand: a nonce the node refused is classified however it was worded, already known is not a nonce collision, and the existing a failed broadcast is terminal, whatever the node said (extended to pin outcome.stage).

One existing assertion changed rather than being added to: that terminal-broadcast test looped over four node messages asserting the wallet passes each through verbatim, and replacement transaction underpriced is now one of the reclassified ones. It is still asserted terminal, in the new nonce test, with the new message.

Verification

make check — green: 28 suites, 689 tests, script/verify-build 18 cases, prettier --check clean. make fmt run; the branch is rebased on next at 9dcd875.

Not covered, and knowingly so: the popup's own Send screen populates at send time and is outside the approval record, so it is not gated by this slot. A nonce it takes under an open dApp approval still collides — and that collision now reports accurately through both paths above, which is the part of it this issue asked for. An approval the user never answers holds the slot until the window is closed or the worker restarts; that is the same interval in which its nonce is allocated and unspent, so it is the intended behaviour rather than a leak.

Closes [#271](https://git.eeqj.de/sneak/AutistMask/issues/271). ## The decision, and why Of the three options the issue named, this takes the third: **refuse a second transaction approval while one is pending**. - **Re-populate at Confirm** reintroduces exactly the gap [#216](https://git.eeqj.de/sneak/AutistMask/issues/216) closed. The nonce on the approval screen would no longer be the nonce that gets signed, so the artifact could not be verified against the displayed object without either lying about what was displayed or dropping the nonce from the comparison. - **Allocate around in-flight approvals** makes the wallet's own bookkeeping the authority on a nonce the network has not accepted. An approval the user abandons — the window left open, the worker terminated — then leaves a hole that every later transaction queues behind until something reconciles it. - **Refuse** keeps the displayed object the verified object, holds no state the network can contradict, and fails the second request while the page is still waiting and nothing has been shown. For a wallet that signs from one address at a time, one unanswered transaction is the natural limit. ## What changed `src/background/index.js` - A single transaction-approval slot, taken synchronously in the `eth_sendTransaction` branch **before its first `await`** — two requests delivered in the same tick cannot both pass it — and released when the requesting page has its answer (broadcast, rejected, or window closed). The second request is refused with EIP-1193 `-32002` (`resource unavailable`, the standard code for "already pending") and never reaches `prepareApprovalTx()`: no second nonce, no second window. Signature approvals are not gated, consuming no nonce. - The body of the branch moved into `handleSendTransaction()` so the reservation and its `finally` read as one thing; the body itself is unchanged. - Nonces this worker has broadcast are recorded per address and checked **before** the artifact is handed to the node. A node's pending count can lag a transaction it has itself just accepted, and a request populated inside that window would otherwise be signed and sent at a nonce this wallet has already used. The record dies with the worker, which is right: after a restart the node's count is the only answer available. - Each `sendResponse` now reports `outcome.stage` rather than the stage it passed in, because a broadcast failure the node blamed on the nonce is reclassified. `src/shared/approvalVerify.js` - `isNonceCollision()` and a new `TX_STAGE_NONCE`. Classified from ethers' `NONCE_EXPIRED` / `REPLACEMENT_UNDERPRICED` codes and from the node's own words (`nonce too low`, `nonce has already been used`, `invalid nonce`, `OldNonce`, `replacement transaction underpriced`, `replacement fee too low`), including the nested `info.error.message` shape ethers hands up when it could not classify the error itself. - `already known` is deliberately **not** a collision: a node that says it knows the transaction has it, so it did reach the network and the existing ambiguous wording is the correct one for it. - The copy. A nonce collision produces "The transaction was not sent, because its nonce had already been used by another transaction." to the page, and in the popup that plus "The transaction did not reach the network. Please send it again from the site." The node's own fragment (`nonce too low`) is replaced rather than passed through: it is not a sentence, and it says less than the wallet knows. Verification is untouched. The approval still carries the transaction the screen displayed, and the artifact is still compared against that object field for field. ## The new tests, failing first Written against the unfixed tree and run there before any source change. `make test` on `next` + tests only: **7 failed, 682 passed**. ``` ● one transaction approval at a time › a second eth_sendTransaction while one is pending is refused before it takes a nonce expect(received).toEqual(expected) // deep equality Expected: {"error": {"code": -32002, "message": StringMatching /already waiting to be approved/}} Received: null ``` `null` is the whole defect: the second request had not answered the page, because it had raised its own approval window at the same nonce the first one is holding. ``` ● a nonce collision is reported as a transaction that did not go out › a nonce this wallet already broadcast is refused without asking the node again expect(jest.fn()).toHaveBeenCalledTimes(expected) Expected number of calls: 1 Received number of calls: 2 ``` Two broadcasts, the second at a nonce this wallet had already used — the terminal failure the issue describes. ``` ● a nonce collision is reported as a transaction that did not go out › a broadcast the node refused for the nonce is not reported as possibly sent Object { - "error": StringMatching /nonce had already been used/, + "error": "nonce too low", "retryable": false, - "stage": "nonce", + "stage": "broadcast", } ``` ``` ● signing failure and retry › a nonce collision says the transaction did not reach the network Expected pattern: /did not reach the network/ Received string: "nonce too low. This request can no longer be signed. Please start it again from the site." ``` Also red beforehand: `a nonce the node refused is classified however it was worded`, `already known is not a nonce collision`, and the existing `a failed broadcast is terminal, whatever the node said` (extended to pin `outcome.stage`). One existing assertion changed rather than being added to: that terminal-broadcast test looped over four node messages asserting the wallet passes each through verbatim, and `replacement transaction underpriced` is now one of the reclassified ones. It is still asserted terminal, in the new nonce test, with the new message. ## Verification `make check` — green: 28 suites, 689 tests, `script/verify-build` 18 cases, `prettier --check` clean. `make fmt` run; the branch is rebased on `next` at `9dcd875`. Not covered, and knowingly so: the popup's own Send screen populates at send time and is outside the approval record, so it is not gated by this slot. A nonce it takes under an open dApp approval still collides — and that collision now reports accurately through both paths above, which is the part of it this issue asked for. An approval the user never answers holds the slot until the window is closed or the worker restarts; that is the same interval in which its nonce is allocated and unspent, so it is the intended behaviour rather than a leak.

clawbot added 1 commit 2026-08-14 06:12:57 +02:00

fix: one transaction approval at a time, and honest copy for a nonce collision (closes #271 )

check / check (push) Successful in 40s

Details

73db8eee43

Populating the transaction in the background before the approval window
opens is what makes the displayed object the verified object. It also
fixes the nonce before the user has answered anything, so two
eth_sendTransaction calls populated concurrently took the same nonce
from a node that had seen neither of them broadcast, and the second
could never be sent: its approved nonce is spent, and the only way to
give it a fresh one is to populate it again after the user has read the
old one off the screen.

A second transaction approval is now refused while one is unanswered,
with EIP-1193 code -32002. The refusal happens before anything is
populated — no second nonce is allocated, no window opens — and the slot
is released when the requesting page has its answer. Signature approvals
are not gated; a signature consumes no nonce.

A collision that does happen is now reported for what it is. A broadcast
the node refused for the nonce, and an approval carrying a nonce this
worker has already broadcast (caught before the node is asked at all),
both report that the transaction did not reach the network and to send
it again, instead of the standing broadcast wording that warns it may
have sent. "already known" keeps that ambiguous wording deliberately: a
node that says it has the transaction has it.

Nothing about verification is weakened. The approval still carries the
transaction the screen displayed, and the artifact is still compared
against that object field for field.

clawbot added the needs-review label 2026-08-14 06:13:09 +02:00

clawbot self-assigned this 2026-08-14 06:13:10 +02:00

clawbot commented

2026-08-14 06:21:12 +02:00

FAIL — needs-rework. Three defects, all in src/background/index.js, all reproduced against head 73db8ee with a scratch harness test (deleted, nothing committed).

1. src/background/index.js:117-124 and :1130-1151 — the broadcast-nonce record is keyed by address only, so a nonce spent on one chain blocks that nonce on every other chain. broadcastNoncesFor(address) has no chain component, while the wallet switches networks (wallet_switchEthereumChain at :502-510, currentNetwork().chainId at :1101) and nonce spaces are per chain. Reproduced: broadcast at nonce 7 on chain 1, switch network, approve a transaction the node populated at nonce 7 on chain 11155111 → broadcastTransaction was never called for it (call count stayed at 1) and both the page and the popup were told "The transaction was not sent, because its nonce had already been used by another transaction." That is false — no transaction of this user's has used that nonce on the new chain — and it is unrecoverable: the copy says "Please send it again from the site", the resend repopulates the same nonce, and it is refused identically for the life of the worker. Low nonces overlap across chains routinely, so this blocks ordinary multi-chain use with an inaccurate message, in the exact direction the issue was filed to stop. Acceptable: key the record by chain id plus address, record under the chain the broadcast actually went out on, and check only the chain that is current.

2. :674-681 — the slot is released only when the approval promise resolves, and there are paths on which it never resolves; the wallet then refuses every eth_sendTransaction for the life of the worker. finally { releaseTxApprovalSlot(); } waits on requestTxApproval(). An approval whose attempt is claimed, whose window is then closed (:934-950, where settleApproval() correctly declines while attemptInFlight), and whose attempt then fails retryably (:1116 releaseApproval(approval) — reached by any non-mismatch throw: loadState(), getActiveAddress(), an unparseable artifact) is left in pendingApprovals with no window and no resolver. Reproduced: after that sequence the first request's result is still null and the next eth_sendTransaction returns -32002 with zero windows opened, permanently. windowsApi.create handing back no window at :278-282 strands it the same way. Before this PR that sequence stranded one request; with the slot it denies the wallet's main function until the worker restarts. Acceptable: bind the slot to the approval id and free it whenever that approval leaves pendingApprovals or loses its window — including the window-closed path that currently declines to settle — rather than only on promise resolution.

3. :663-673 — the slot is taken before the authorization check, so any web page can make the user's single legitimate transaction fail with "already pending". reserveTxApprovalSlot() is the first statement of the branch, ahead of await getState() / getActiveAddress() and the 4100 Unauthorized return inside handleSendTransaction(). Reproduced: an unconnected origin's eth_sendTransaction (itself rejected 4100) held the slot while the connected dApp's own single request was refused with -32002 and no window opened. Any page can loop this and deny sending wallet-wide. TX_APPROVAL_PENDING_MESSAGE is also untrue in that window: nothing is "already waiting to be approved" and there is nothing for the user to answer. Acceptable: the take is atomic because nothing awaits between test and set, not because of where it sits in the function — move it after the authorization and from checks, immediately before prepareApprovalTx().

Verified and passing: make check green here (28 suites, 689 tests, script/verify-build 18 cases, prettier clean); the seven new/changed tests are load-bearing — reverting src/ to 9dcd875 with the tests kept gives 7 failed / 682 passed, matching the PR body; replacement transaction underpriced is correctly a collision (the node refused that artifact) and already known correctly is not; verification is untouched and the nonce check sits after verifySignedTx(), so #216 is not weakened; merges cleanly onto current next (0be20d7), no conflict with #282 today; commit title, TODO.md in the same commit, terminology and formatting all fine; no attribution trailers.

Disclosures: the head commit's CI check is still "Waiting to run", so CI green is unconfirmed — the local make check above is the evidence. Neither e2e suite was run (docker browser suites, not part of check). The disclosed popup-Send gap is waived against the issue's definition of done: the Send screen is not an eth_sendTransaction request, and finding 1 is the part of it that matters.

FAIL — `needs-rework`. Three defects, all in `src/background/index.js`, all reproduced against head `73db8ee` with a scratch harness test (deleted, nothing committed). **1. `src/background/index.js:117-124` and `:1130-1151` — the broadcast-nonce record is keyed by address only, so a nonce spent on one chain blocks that nonce on every other chain.** `broadcastNoncesFor(address)` has no chain component, while the wallet switches networks (`wallet_switchEthereumChain` at `:502-510`, `currentNetwork().chainId` at `:1101`) and nonce spaces are per chain. Reproduced: broadcast at nonce 7 on chain 1, switch network, approve a transaction the node populated at nonce 7 on chain 11155111 → `broadcastTransaction` was never called for it (call count stayed at 1) and both the page and the popup were told "The transaction was not sent, because its nonce had already been used by another transaction." That is false — no transaction of this user's has used that nonce on the new chain — and it is unrecoverable: the copy says "Please send it again from the site", the resend repopulates the same nonce, and it is refused identically for the life of the worker. Low nonces overlap across chains routinely, so this blocks ordinary multi-chain use with an inaccurate message, in the exact direction the issue was filed to stop. Acceptable: key the record by chain id plus address, record under the chain the broadcast actually went out on, and check only the chain that is current. **2. `:674-681` — the slot is released only when the approval promise resolves, and there are paths on which it never resolves; the wallet then refuses every `eth_sendTransaction` for the life of the worker.** `finally { releaseTxApprovalSlot(); }` waits on `requestTxApproval()`. An approval whose attempt is claimed, whose window is then closed (`:934-950`, where `settleApproval()` correctly declines while `attemptInFlight`), and whose attempt then fails retryably (`:1116` `releaseApproval(approval)` — reached by any non-mismatch throw: `loadState()`, `getActiveAddress()`, an unparseable artifact) is left in `pendingApprovals` with no window and no resolver. Reproduced: after that sequence the first request's result is still `null` and the next `eth_sendTransaction` returns `-32002` with zero windows opened, permanently. `windowsApi.create` handing back no window at `:278-282` strands it the same way. Before this PR that sequence stranded one request; with the slot it denies the wallet's main function until the worker restarts. Acceptable: bind the slot to the approval id and free it whenever that approval leaves `pendingApprovals` or loses its window — including the window-closed path that currently declines to settle — rather than only on promise resolution. **3. `:663-673` — the slot is taken before the authorization check, so any web page can make the user's single legitimate transaction fail with "already pending".** `reserveTxApprovalSlot()` is the first statement of the branch, ahead of `await getState()` / `getActiveAddress()` and the `4100 Unauthorized` return inside `handleSendTransaction()`. Reproduced: an unconnected origin's `eth_sendTransaction` (itself rejected `4100`) held the slot while the connected dApp's own single request was refused with `-32002` and no window opened. Any page can loop this and deny sending wallet-wide. `TX_APPROVAL_PENDING_MESSAGE` is also untrue in that window: nothing is "already waiting to be approved" and there is nothing for the user to answer. Acceptable: the take is atomic because nothing awaits between test and set, not because of where it sits in the function — move it after the authorization and `from` checks, immediately before `prepareApprovalTx()`. Verified and passing: `make check` green here (28 suites, 689 tests, `script/verify-build` 18 cases, prettier clean); the seven new/changed tests are load-bearing — reverting `src/` to `9dcd875` with the tests kept gives 7 failed / 682 passed, matching the PR body; `replacement transaction underpriced` is correctly a collision (the node refused that artifact) and `already known` correctly is not; verification is untouched and the nonce check sits after `verifySignedTx()`, so [#216](https://git.eeqj.de/sneak/AutistMask/issues/216) is not weakened; merges cleanly onto current `next` (`0be20d7`), no conflict with [#282](https://git.eeqj.de/sneak/AutistMask/pulls/282) today; commit title, `TODO.md` in the same commit, terminology and formatting all fine; no attribution trailers. Disclosures: the head commit's CI check is still "Waiting to run", so CI green is unconfirmed — the local `make check` above is the evidence. Neither e2e suite was run (docker browser suites, not part of `check`). The disclosed popup-Send gap is waived against the issue's definition of done: the Send screen is not an `eth_sendTransaction` request, and finding 1 is the part of it that matters.

clawbot added needs-rework and removed needs-review labels 2026-08-14 06:21:41 +02:00

check / check (push) Successful in 40s

Details

This pull request can be merged automatically.

This branch is out-of-date with the base branch

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin issue-271-concurrent-nonce:issue-271-concurrent-nonce

git checkout issue-271-concurrent-nonce

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/AutistMask#284