-
Notifications
You must be signed in to change notification settings - Fork 29
fix: prevent 'nonce too low' on cannon build -- broadcast (strict-ordering RPCs) #1876
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: dev
Are you sure you want to change the base?
Changes from 7 commits
afac605
73e9c32
964b974
19441a4
f7e4f3d
7d7fed0
0ff332c
c9bc3fd
8b15b1a
7e803b7
80c26b3
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 |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| --- | ||
| "@usecannon/cli": patch | ||
| "@usecannon/builder": patch | ||
| --- | ||
|
|
||
| fix: prevent "nonce too low" during `cannon build -- broadcast` on strict-ordering RPCs | ||
|
|
||
| Live private-key signers now use a viem `nonceManager`, so nonces are assigned from a | ||
| serialized local counter that never regresses below an already-used nonce instead of a fresh | ||
| `eth_getTransactionCount('pending')` read on every transaction. Every broadcast is also wrapped | ||
| in a nonce-aware retry that re-reads the nonce and resubmits on a transient nonce error. Fixes | ||
| repeated build aborts on eventually-consistent / load-balanced RPCs (e.g. MegaETH testnet). |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ import { PackageReference } from './package-reference'; | |
| import { OnChainRegistry, FallbackRegistry, InMemoryRegistry } from './registry'; | ||
| import { CannonStorage } from './runtime'; | ||
| import { getContractFromPath } from './util'; | ||
| import type { CannonSigner } from './types'; | ||
|
|
||
| export function getDefaultStorage() { | ||
| const registryChainIds = DEFAULT_REGISTRY_CONFIG.map((registry) => registry.chainId); | ||
|
|
@@ -77,3 +78,42 @@ export async function getBlockRetried(provider: viem.PublicClient, blockHash: vi | |
| return provider.getBlock({ blockHash }).catch(retry); | ||
| }); | ||
| } | ||
|
|
||
| const NONCE_ERROR_RE = | ||
| /nonce too low|nonce too high|nonce provided.*is (?:too )?(?:low|high|higher|lower)|invalid (?:transaction )?nonce|replacement transaction underpriced|(?:transaction|tx).*already known|already known.*(?:transaction|tx)/i; | ||
|
|
||
| // Detects whether an error (or any error in its `cause` chain) is a nonce-related RPC error. | ||
| // viem wraps RPC errors, so the underlying message can live a few levels down in `.cause`. | ||
| export function isNonceError(err: unknown): boolean { | ||
| 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; | ||
|
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. 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?
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. Agreed — done in c9bc3fd. 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. |
||
| e = e.cause; | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| // Defense-in-depth around a broadcast: runs the prepare+send closure and, on a nonce-class | ||
| // error, resets the signer's viem nonce manager (so the next prepare re-reads the chain) and | ||
| // retries with exponential backoff. The closure is responsible for (re-)preparing the | ||
| // transaction each attempt so a fresh nonce is assigned. Non-nonce errors propagate immediately, | ||
| // so the happy path is unchanged. | ||
| export async function sendTransactionWithNonceRetry( | ||
| signer: CannonSigner, | ||
| chainId: number, | ||
| prepareAndSend: () => Promise<viem.Hash> | ||
| ): Promise<viem.Hash> { | ||
| 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; | ||
|
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. why do we need to do so much type casting here? Seems like an indication that something may be wrong with this strategy.
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. It was an indication of exactly that — the wrapper was reaching into account internals as |
||
| try { | ||
| account?.nonceManager?.reset?.({ address: account.address, chainId }); | ||
|
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. 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
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. Fixed in c9bc3fd — the reset is now an explicit typed path: the account is narrowed to a viem local account and |
||
| } catch { | ||
| // best-effort: the nonce manager re-reads the chain on the next prepare regardless | ||
| } | ||
| return retry(err); | ||
| }) | ||
| )) as viem.Hash; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ import * as viem from 'viem'; | |
| import { z } from 'zod'; | ||
| import { ARACHNID_DEFAULT_DEPLOY_ADDR, ensureArachnidCreate2Exists, makeArachnidCreate2Txn } from '../create2'; | ||
| import { mergeTemplateAccesses } from '../access-recorder'; | ||
| import { sendTransactionWithNonceRetry } from '../helpers'; | ||
| import { ChainBuilderRuntime } from '../runtime'; | ||
| import { diamondSchema } from '../schemas'; | ||
| import { ContractArtifact, ContractMap, PackageState } from '../types'; | ||
|
|
@@ -170,7 +171,7 @@ const diamondStep = { | |
| ]); | ||
|
|
||
| // finally, do the cut operation | ||
| const updateFacets = []; | ||
| const updateFacets: 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. we don't want to expand the valid types to
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. 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 |
||
|
|
||
| for (const facetName of deployedContracts) { | ||
| updateFacets.push({ | ||
|
|
@@ -194,18 +195,19 @@ const diamondStep = { | |
|
|
||
| // todo: what to do about the owner of the proxy changing unexpectedly? | ||
| try { | ||
| const preparedTxn = await runtime.provider.prepareTransactionRequest({ | ||
| account: ownerSigner.wallet.account || ownerSigner.address, | ||
| to: proxyAddress, | ||
| data: viem.encodeFunctionData({ | ||
| abi: (await import('../abis/diamond/DiamondWipeAndPaveFacet.json')).abi, | ||
| functionName: 'diamondWipeAndPave', | ||
| args: [updateFacets, config.diamondArgs.init, config.diamondArgs.initCalldata], | ||
| }), | ||
| ...config.overrides, | ||
| } as any); | ||
|
|
||
| const txn = await ownerSigner.wallet.sendTransaction(preparedTxn as any); | ||
| const txn = await sendTransactionWithNonceRetry(ownerSigner, runtime.chainId, async () => { | ||
| const preparedTxn = await runtime.provider.prepareTransactionRequest({ | ||
| account: ownerSigner.wallet.account || ownerSigner.address, | ||
| to: proxyAddress, | ||
| data: viem.encodeFunctionData({ | ||
| abi: (await import('../abis/diamond/DiamondWipeAndPaveFacet.json')).abi, | ||
| functionName: 'diamondWipeAndPave', | ||
| args: [updateFacets, config.diamondArgs.init, config.diamondArgs.initCalldata], | ||
| }), | ||
| ...config.overrides, | ||
| } as any); | ||
| return ownerSigner.wallet.sendTransaction(preparedTxn as any); | ||
| }); | ||
|
|
||
| const receipt = await runtime.provider.waitForTransactionReceipt({ hash: txn }); | ||
| debug('got receipt', receipt); | ||
|
|
@@ -298,8 +300,8 @@ async function firstTimeDeploy( | |
| const bytecode = await runtime.provider.getCode({ address: addr }); | ||
|
|
||
| if (!bytecode) { | ||
| const hash = await signer.wallet.sendTransaction( | ||
| _.assign({ account: signer.wallet.account || signer.address }, create2Txn as any) | ||
| const hash = await sendTransactionWithNonceRetry(signer, runtime.chainId, async () => | ||
| signer.wallet.sendTransaction(_.assign({ account: signer.wallet.account || signer.address }, create2Txn as any)) | ||
| ); | ||
| const receipt = await runtime.provider.waitForTransactionReceipt({ hash }); | ||
| const block = await runtime.provider.getBlock({ blockHash: receipt.blockHash }); | ||
|
|
||
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.
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.
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.
Deleted in 7e803b7.