fix: prevent 'nonce too low' on cannon build -- broadcast (strict-ordering RPCs) - #1876
fix: prevent 'nonce too low' on cannon build -- broadcast (strict-ordering RPCs)#1876alxwlw wants to merge 11 commits into
Conversation
Live signers built from --private-key had no nonce manager, so every
broadcast relied on a fresh eth_getTransactionCount('pending') read.
On strict-ordering / eventually-consistent RPCs that read can lag an
already-submitted transaction, yielding a stale low nonce and
'nonce too low'. Attaching viem's nonceManager serializes assignment
and prevents the nonce from regressing.
Refs usecannon#1875
Wraps every transaction broadcast (invoke/deploy/router/diamond/create2) in a nonce-aware retry so a transient 'nonce too low'/'nonce too high' from a flaky strict-ordering RPC re-reads the nonce and resubmits instead of aborting the whole build. Refs usecannon#1875
Apply the nonce-aware retry to the two remaining live broadcasts in onchain-store.ts (deployStoreIfNeeded / storeWrite), which run via writeUpgradeFromInfo on every 'cannon build -- broadcast' and were the last unprotected send sites. Also narrow the 'already known' branch of the nonce error matcher to a transaction context so unrelated errors no longer trigger a spurious retry, and cover the retries-exhausted path. Refs usecannon#1875
Note: extra broadcast site beyond the step actionsWhile reviewing this branch I found that the nonce-retry wrapper needed to cover one more live broadcast that isn't a step action: the on-chain "upgrade-from" info writes.
Addressed in 0ff332c — both sends now go through
The same commit also tightens the |
dbeal-eth
left a comment
There was a problem hiding this comment.
hey! thanks again for the contribution, and sorry for my delay in getting back to you.
There are some things to look at, mostly with regards to type protection and watching the edge cases around null types.
|
|
||
| // finally, do the cut operation | ||
| const updateFacets = []; | ||
| const updateFacets: any[] = []; |
There was a problem hiding this comment.
we don't want to expand the valid types to any here. can we identify why this would seem to be needed and narrow the type down/preventt this change entirely?
There was a problem hiding this comment.
The widening came from moving the send into the retry closure — the array is read inside a nested function, where TS can't apply evolving-array inference. Replaced with an explicit FacetCut type and getFacetSelectors now returns viem.Hex[] via a type-guard filter; addFacets in firstTimeDeploy is typed the same way. Done in c9bc3fd.
| let e: any = err; | ||
| for (let i = 0; e && i < 5; i++) { | ||
| const msg = e.details ?? e.shortMessage ?? e.message ?? ''; | ||
| if (NONCE_ERROR_RE.test(String(msg))) return true; |
There was a problem hiding this comment.
this strategy seems very brittle (either having false positives or negatives). can we instead just always retry and assume the nonce is broken? I understand its challenging because there isn't a global or reliable error that RPCs return on this type of error, but we need to have a strategy here that gives maximum predictability to the user. Perhaps we offer a command line flag instead?
There was a problem hiding this comment.
Agreed — done in c9bc3fd. isNonceError and the regex are gone; the wrapper (now sendTransactionWithRetry) retries every failed broadcast and always assumes the nonce may be broken: before each retry it resets the signer's nonce manager (when present) so the closure re-prepares with a fresh pending nonce. Deterministic failures (e.g. reverts) fail identically on every attempt and surface the original error after the last one, so the only cost there is the bounded backoff (3 retries, ~1.75s total).
The one exception is an explicit user rejection (EIP-1193 code 4001, matched via viem's typed error classes, not strings) — retrying that would re-prompt a wallet user who already declined to sign.
Since unconditional retry behaves predictably without new config surface I didn't add a flag, but happy to add an opt-out if you'd prefer.
| if (!isNonceError(err)) throw err; | ||
| const account: any = (signer.wallet as any).account; | ||
| try { | ||
| account?.nonceManager?.reset?.({ address: account.address, chainId }); |
There was a problem hiding this comment.
if the account doesn't have its nonce manager set (or it doesn't have a reset function) then this will silently have no effect
There was a problem hiding this comment.
Fixed in c9bc3fd — the reset is now an explicit typed path: the account is narrowed to a viem local account and reset is only called when a manager is present; both outcomes are debug-logged, so nothing silently no-ops. When there is no manager (json-rpc backed signers, e.g. Frame) there is genuinely nothing to reset client-side — the node assigns their nonces on every send — and the retry still applies.
| return (await promiseRetry({ retries: 3, minTimeout: 250, factor: 2 }, (retry) => | ||
| prepareAndSend().catch((err: unknown) => { | ||
| if (!isNonceError(err)) throw err; | ||
| const account: any = (signer.wallet as any).account; |
There was a problem hiding this comment.
why do we need to do so much type casting here? Seems like an indication that something may be wrong with this strategy.
There was a problem hiding this comment.
It was an indication of exactly that — the wrapper was reaching into account internals as any. With the account properly narrowed (account.type === 'local'), viem fully types nonceManager and reset, so all casts are gone in c9bc3fd. Dropping the chainId parameter (the reset now resolves it the way viem does at consume time: wallet chain, falling back to eth_chainId) also removed a bogus ?? 0 fallback in onchain-store.
| // nonce. This prevents "nonce too low" on strict-ordering / eventually-consistent RPCs where a | ||
| // fresh eth_getTransactionCount('pending') read can lag an already-submitted transaction. | ||
| export function createPrivateKeySigner(privateKey: viem.Hex, chainId: number, transport: viem.Transport): CannonSigner { | ||
| const account = privateKeyToAccount(privateKey, { nonceManager }); |
There was a problem hiding this comment.
it seems we are using the default nonce manager here. Is there any reason why we shouldn't use createNonceManager() instead to make sure that the nonces are being managed by the same RPC that is the provided transport?
There was a problem hiding this comment.
Good call. To be precise, neither form owns an RPC: the manager's jsonRpc source issues eth_getTransactionCount through the client passed at consume time, which is the wallet client built on this transport — so even the default manager read through the right RPC. But the module-level singleton is shared process-wide, so 8b15b1a switches to an explicit per-signer createNonceManager({ source: jsonRpc() }) with a comment documenting that it reads through the same transport the transactions are sent on.
| signers.push( | ||
| createPrivateKeySigner( | ||
| normalizePrivateKey(keyPrompt.value as viem.Hex), | ||
| chainId!, |
There was a problem hiding this comment.
do we know that chainId is set here?
There was a problem hiding this comment.
Yes — within resolveProviderAndSigners the chainId parameter is a required number (it's only optional in the outer ProviderParams), so the assertions were redundant; both dropped in 8b15b1a.
| ### Deploying against strict-ordering RPCs | ||
|
|
||
| Some RPC providers enforce strict nonce ordering or serve `eth_getTransactionCount` from eventually-consistent / load-balanced replicas. On such endpoints a freshly-read pending nonce can briefly lag an already-submitted transaction. Cannon assigns nonces to live signers through a [viem nonce manager](https://viem.sh/docs/accounts/local/createNonceManager) and retries nonce-class broadcast errors automatically. | ||
|
|
||
| If a deploy still aborts partway through, it is safe to re-run it — Cannon resumes where it stopped: | ||
|
|
||
| ```bash | ||
| cannon build ... --upgrade-from <name:version> | ||
| ``` | ||
|
|
||
| Re-run until the build reports **0 skipped steps**. | ||
|
|
There was a problem hiding this comment.
this section is probably unneeded, as people expect cannon to be properly managing nonces regardless, and there isn't really anything actionable here by informing users that this feature is in-use.
There was a problem hiding this comment.
we have never actually ended up getting around to finishing using changeset in our release process, so you can go ahead and delete this file from the changes.
…errors Per review: nonce-class RPC errors have no reliable shape across providers, so string-matching them is brittle. Drop isNonceError and the regex entirely and make sendTransactionWithRetry (renamed from sendTransactionWithNonceRetry) retry every failed broadcast, assuming the nonce may be broken: before each retry the signer's viem nonce manager (when present) is reset so the closure re-prepares with a fresh pending nonce. Deterministic failures surface unchanged after the final attempt; explicit user rejections (EIP-1193 code 4001, matched via the typed viem error classes) are never retried so wallet users are not re-prompted after declining to sign. The wrapper no longer takes a chainId: the reset resolves it the way viem does on consume (wallet chain, falling back to eth_chainId), which removes the `?? 0` fallback in onchain-store. All casts around the nonce manager are gone — access is narrowed through the typed local account, and a missing manager is an explicit, logged no-reset path (json-rpc backed signers get their nonces from the node on every send). Also type the diamond facet-cut lists (FacetCut[]) instead of widening to any[]. Refs usecannon#1875 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Per review: replace the shared default viem nonceManager with an
explicit createNonceManager({ source: jsonRpc() }) per live signer. The
jsonRpc source has no RPC of its own — it issues eth_getTransactionCount
through the wallet client built on the provided transport, so nonces are
always managed by the same RPC the transactions are sent over. Also drop
the redundant non-null assertions on chainId (the parameter is already a
required number inside resolveProviderAndSigners).
Refs usecannon#1875
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Per review: the release process does not use changesets, and users expect cannon to manage nonces correctly without being told about it — the docs section is unneeded. Refs usecannon#1875 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the as-any signer fakes in the sendTransactionWithRetry tests with real viem objects: a privateKeyToAccount local account carrying a spied createNonceManager, real wallet clients (with and without a configured chain) over a custom transport that serves eth_chainId. The tests are now fully typed with zero casts and exercise the actual LocalAccount/NonceManager shapes the helper narrows on, so the new code in this PR introduces no `any` at all. Refs usecannon#1875 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
Fixes #1875.
cannon build ... -- broadcastaborts withnonce too low: next nonce N+1, tx nonce N(viemInvalidTransactionNonce) on strict-ordering / eventually-consistent RPCs (observed on MegaETH testnet, chainId 6343).Root cause (corrected)
The build is strictly sequential — there is no parallel step execution. The real cause is that live signers built from
--private-key(packages/cli/src/util/provider.ts) had no viem nonce manager, so every broadcast relied on a fresheth_getTransactionCount(addr, 'pending')read viaprepareTransactionRequest. When that read is served by a lagging replica it can return a stale, too-low nonce after the previous transaction already consumed it →nonce too low. This also explains why repeated--upgrade-fromretries gradually converge.Fix
createNonceManager({ source: jsonRpc() })) to all live private-key signers (newcreatePrivateKeySignerhelper). Nonce assignment is serialized per(address, chainId)and never regresses below an already-used nonce; the manager'sjsonRpcsource readseth_getTransactionCountthrough the same transport the transactions are sent on.invoke/deploy/router/diamond/create2and theonchain-storeupgrade-info writes (writeUpgradeFromInfo) — insendTransactionWithRetry: any failed broadcast is retried with bounded backoff, always assuming the nonce may be broken (per review — no error-string matching). Before each retry the signer's nonce manager (when present) is reset so the closure re-prepares with a fresh pending nonce. Explicit user rejections (EIP-1193 code 4001, matched via viem's typed error classes) are never retried.Tests
provider.test.ts: live signer gets a nonce manager + correct address.helpers.test.ts: retry-on-any-failure with nonce manager reset, retry for signers without a manager,eth_chainIdfallback when the wallet has no configured chain, user rejection not retried, last error surfaced after retries are exhausted.🤖 Generated with Claude Code