Repository navigation
fix(wallet): estimate gas with value and confirm failed sends on chain - #42
Conversation
…ncel - Do not retry a transaction the node rejected; ask the chain once. - On "nonce too low", move the next nonce up to the pending nonce. - Stop the lookup when the context is cancelled, after the first request. - Match "already known" without case, to include Nethermind. - Replace funder WithRetryOption with WithWalletOptions. - Shorten comments. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Make the logger after the flags are parsed, so --log-verbosity applies. - Pass the logger to Fund and Stake. - Add --retryAttempts and --retryDelay flags. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
e94a808 to
289218c
Compare
aloknerurkar
left a comment
aloknerurkar
left a comment
There was a problem hiding this comment.
Nice work. Signing once and re-sending the same transaction, the nonce that never moves back, and the on-chain lookup before giving up together remove the double-funding risk the old "replacement transaction underpriced" loop had. The gas fixes are clearly right: on main, gas *= 1 + (30/100) was gas *= 1, and the estimate had no value. Tests cover the cases well. Build, vet and go test -race -count=3 ./... pass locally.
One issue I'd like to discuss before merge, plus a couple of smaller points inline.
A rejected send leaves a nonce gap that blocks later transfers. The nonce is reserved before the send and never handed back. When the node rejects the transaction outright, nothing is ever broadcast on that nonce. The next transfer from the same wallet gets nonce+1, which the chain holds as a future transaction until the gap is filled, so it isn't mined. I checked it with the scripted client: "insufficient funds…" on the first transfer, then the second one sends with nonce 8 while the node's pending nonce stays 7. The same happens with a generic send error (RPC timeout, connection reset) where the node never got the transaction.
Main had the same gap, so this isn't a regression. But the PR now classifies rejectedMsgs as final, so at that point the wallet knows the nonce wasn't used. In a funder run the wallets are funded concurrently, so one "insufficient funds" failure (funding wallet running low) can leave the rest of the run's transactions accepted by the node but stuck in its future queue, while the run reports them as sent.
Re-fetching PendingNonceAt isn't a safe fix, because with other sends in flight it would hand out a nonce that's already used. Options I can see:
- On a final rejection, give the nonce back only if it's still the latest one reserved (
nonce == nonceNext-1). This covers the sequential case cheaply. - Otherwise, fill the hole with a 0-value self-transfer on that nonce, or at least log the gap at error level so the operator knows later transfers won't mine until it's filled.
Happy to hear if the funder is only ever run with enough balance and this was a conscious trade-off.
| s.resyncNonce(ctx, fromAddr) | ||
| } | ||
|
|
||
| if rejected || attempt == attempts { |
There was a problem hiding this comment.
This is where a final rejection returns while its nonce stays reserved (see the review body). For the sequential case, something like if rejected && !nonceTooLow { s.releaseNonce(nonce) }, where release moves nonceNext back only when nonce == nonceNext-1, would avoid stalling the next transfer.
| // The nonce is already used on chain. | ||
| nonceTooLowMsgs = []string{"nonce too low"} | ||
| // The node rejected the transaction itself, so re-sending it cannot help. | ||
| rejectedMsgs = []string{"nonce too low", "insufficient funds", "intrinsic gas too low", "exceeds block gas limit"} |
There was a problem hiding this comment.
Matching node errors by substring is the pragmatic choice here. One consequence worth knowing: "replacement transaction underpriced", which main used as its retry trigger, is now treated as "maybe kept" and polled for the full lookup window on each attempt. With the CLI default of one attempt that's 15s per failed transfer. That's fine, and WithLookup's doc already mentions the Nethermind case.
| fundCmd.PersistentFlags().StringVar(&cfg.WalletKey, "walletKey", "", "wallet key") | ||
| fundCmd.PersistentFlags().Float64Var(&cfg.MinAmounts.NativeCoin, "minNative", 0, "specifies min amount of chain native coins (DAI) nodes should have") | ||
| fundCmd.PersistentFlags().Float64Var(&cfg.MinAmounts.SwarmToken, "minSwarm", 0, "specifies min amount of swarm tokens (BZZ) nodes should have") | ||
| fundCmd.PersistentFlags().IntVar(&retryAttempts, "retryAttempts", 1, "number of attempts to send a transaction whose send failed") |
There was a problem hiding this comment.
nit: --retryAttempts defaults to 1, so the CLI never retries unless asked. That's the safe default, but the PR summary reads as if retries are on. Maybe say so in the README entry.
Summary
This PR fixes the gas estimate and makes a failed send safe.
Gas estimate
Failed send
Options and CLI
WithRetry(attempts and delay) andWithLookup(timeout and interval).WithWalletOptions. It sends options to the wallet that the funder makes.--retryAttemptsand--retryDelay.--log-verbositynow applies. Before, the CLI made the logger before it read the flags.API change
wallet.BackendClienthas a new method,TransactionByHash.ethclient.Clientalready has it.Tests
go test -race ./...passes.