feat: read gas limits, fee caps and contract addresses from the chain - #73
Conversation
The scenario declared 22460 gas for a mint. Measured against the deployed binding, a mint to a receiver holding none of the token needs 69319, and one to a receiver that already holds some needs 51757. Every mint the scenario sent landed in a block with a failed status, having burned the whole limit, and trackReceipts defaults to false so the run reported each one as sent. 22460 is ERC20Noop's constant, copied. PLT-1091 covers the two scenarios that still carry it. The limit is now 75000, and the test pins it against the measurement rather than against itself. Broke the constant back to 22460 and to 200000 on purpose; the test caught both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A DeFi profile had no contract to drive. This adds a constant-product pair with the storage and gas shape of a UniswapV2 swap: both reserves, the caller's balance in each token, and an event. The contract never reverts on bookkeeping, which is the choice StorageRWv1 already makes. The balances wrap rather than check, because nothing reads them back and a load generator that fails on its own accounting stops measuring the chain. A short caller is not credited: crediting exactly what is then debited returns the slot to zero, and a zero to non-zero storage write costs four times one that changes a slot already holding a value. Under the default mix, which draws one direction, that write would land on every swap rather than the first. The reserves sit between a floor and a ceiling. Without the ceiling the input side grows without bound and the output halves every 100000 swaps, so a long run prices nothing like its start. The ceiling is also what keeps one oversized call from ending the pair: a swap of 1e49 leaves the input reserve at 1e49, and the contract has no owner and no reset. Measured, the next ordinary swap instead resets that side to the floor and pays out in full. The gas limit is 85000, read from eth_estimateGas rather than from a receipt. GasUsed is the post-refund charge and a transaction carries the pre-refund peak; sizing from a receipt put an earlier draft 20% under what its own swap needed. An account's first swap needs 79988 and every later one needs 45177, so a run in steady state declares about 44% more gas than it spends. PLT-1093 carries the prewarm change that would close that. PLT-1092 carries the chain-parameter exposure, which is the whole package rather than this constant. Every guard here was broken on purpose before it was believed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A deployment has no way to tell a run that is starting from one that is stuck. The process serves /metrics and nothing else, so a probe set has nothing to gate on and a pod counts as available the moment its container starts. /healthz answers as soon as the server binds and never reads the startup sequence. /readyz refuses until the dispatcher is running. Keeping those separate is the whole point. Funding, deployment and prewarm take minutes against a cold chain. A liveness probe that reported the run dead for that window would restart the pod before it sent a transaction, then restart the next attempt at the same place, and the cause would read as a crash loop rather than a slow start. While /readyz refuses it names the phase, so a ten-minute startup shows the step it is on. Measured against the binary: healthz held 200 through a 21 second prewarm while readyz reported "prewarming accounts", then both answered once the dispatcher started. The flag and the phase are stored as one value rather than as two atomics. Two would leave a window where a reader sees the run serving while the body still names the step it left, so the status and the body would disagree about the same instant. Five mutations, five caught, including that one: split into two atomics, a reader observed a serving status carrying "funding accounts". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every contract scenario declared a gas limit as a constant. Those constants assume the EVM default of 20,000 for a storage write that takes a slot from zero to a value. Sei sets that as a governance parameter and its live networks charge 72,000, so every one of them is short on Sei by a factor. Measured against arctic-1: an AMM swap needs about 185,000 where the constant said 85,000, an ERC20 transfer 175,097 against 72,156, an ERC721 mint 174,782 against 75,000. A short limit does not fail visibly. The transaction reaches a block, burns the whole limit, and a run without receipt tracking reports it as sent. ERC20Noop was short by eight gas with no Sei parameter involved at all, which is the argument against hand-picked constants in one line. A scenario now declares GasEstimateCalls, one per operation it issues, and the preparation step asks the chain what each costs after the contracts are bound. ContractScenarioBase does not implement it, so a scenario added without one does not compile — the same gate that already forces Operation(). The priced call is the expensive shape. Cost is bimodal per account: the first transaction from an address writes slots holding zero. Pricing from a freshly generated address makes those slots cold by construction, so the measurement bounds what a run sends rather than describing its cheap case. The call carries no fee cap, because a call carrying one makes the node check the caller's balance and this caller has none; verified against arctic-1, where the same estimate succeeds without fee fields and fails with them. Calldata is recomposed rather than measured. GasModel keeps the execution term apart from the calldata term, so StorageRW reuses one measurement across every pad it draws. The recomposition calls the chain's own IntrinsicGas and FloorDataGas, so it is exact rather than fitted, and it covers the EIP-7623 floor that Sei's ante does not check. That deletes storageRWBaseGas, abiWord and calldataFloorGasPerByte along with the per-scenario constants. Pricing fails the run rather than falling back. A fallback is a cold branch that runs exactly when the estimate could not be trusted, and its failure is the invisible kind. Margin defaults to 1.20 and is a profile setting. Sei fills a block against two budgets, one charged at the declared limit and one at what the transaction spends, and the declared one binds only past four times the spend. Below that, margin costs no block space. A profile of native transfers alone prices nothing and issues no extra call. Five mutations, five caught: an operation left unpriced, two operations priced against one method, the limit stopping coming from the model, a scenario declaring no calls at all, and the decomposition failing to round-trip. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PR SummaryMedium Risk Overview Fee caps come from Gas limits move to a startup Registry: arctic-1 is committed under Settings validation rejects multipliers/margins below 1; extensive tests cover base-fee headroom, estimator wire shape, hung-RPC budget errors, and fail-closed behavior when no cap is resolved. Reviewed by Cursor Bugbot for commit 47202fa. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Replacing hard-coded gas constants with a measured GasModel is the right call and the recomposition/round-trip tests are solid, but two PR-introduced defects block it: the Disperse probe omits msg.value so eth_estimateGas reverts and startup fails, and GasLimitFor rebuilds every scenario's probe calldata per transaction — generating a fresh secp256k1 key on the send path for the ERC20/ERC721 scenarios.
Findings: 2 blocking | 6 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
- 2 blocking issue(s) flagged inline on specific lines.
Non-blocking
- [suggestion] Test coverage for the new mechanism is uneven:
requireGasMatchesModelis only wired intoAMM_test.goandStorageRW_test.go, and the deletedERC721_test.gowas not replaced with an equivalent.TestEveryDrawableOperationIsPricedproves an operation is priced, but nothing proves the send path applies the measured limit — which is exactly how the Disperse gap (priced, then ignored) survives the suite. A table test over every contract scenario that prices with a knownGasModel, generates a tx, and assertstx.Gas() == model.Limit(tx.Data())would close both that gap and the ERC20/ERC721 regression risk. - [suggestion]
ContractScenarioBase.GasLimitForData(generator/scenarios/base.go:238) is added but never called —StorageRWusesMaxGasLimitForDataand everyone else usesGasLimitFor. It is the method the per-operation scenarios should be using (see the inline comment onGasLimitFor); as it stands it is dead code. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
generator/scenarios/Disperse.go:82—CreateContractTransactioncallsDisperseEtherFixedwithout settingauth.Value, but the contract requiresmsg.value == fixedEtherAmount * recipients.length(100 wei, given thebigOneconstructor args inDeployContract). Every disperse transaction therefore reverts on chain, and withtrackReceiptsoff the run reports it as sent — the same invisible failure this PR exists to eliminate.
The repo carried three hard-coded fee caps: 20 gwei for a contract call, 100 for a deployment and for funding, 200 for a native transfer. The live base fee is 50 gwei on pacific-1 and atlantic-2, so the first of the three was rejecting every transaction it priced and the other two were guesses that happened to clear. A cap under the base fee fails at the fee ante, before the EVM runs and after the nonce is consumed. So it produces a receipt with a failed status rather than no receipt at all, and a run without receipt tracking reports it as sent. Same failure mode as the gas limits, same cause: a number written down once cannot be right on a chain that reprices. Startup now asks the chain what gas costs and scales it, before anything is signed. Every path reads that one value, so no two of them can drift apart again. Measured: the derived cap clears the base fee on all three networks, where the constant cleared it on one. The multiple is 5 by default and is a profile setting. The cap is a ceiling, not a price: a transaction pays the base fee and the cap only says how high it will follow one, so a generous multiple costs nothing per transaction. It costs balance, because the chain locks the cap times the gas limit while a transaction is in flight, and that is the only reason not to make it larger. It has to be generous because the base fee moves. Sei raises it by up to about 1.9% per block while blocks are full, which is the state a load run exists to produce. At five times the reported price the cap outlives about ninety such blocks; at the reported price itself it outlives five. Scaling stays in the integer domain. A wei price passes what a float64 holds exactly, and a cap that moved for that reason would look like a chain disagreeing with itself. Five mutations, five caught. The first version of the drift guard survived the mutation it named, because it asserted the arithmetic directly and never drove the resolver that uses it. It now runs startup against a chain and reads the cap back off the config. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…requires Three review findings, all correct. The send path rebuilt the priced call on every transaction. GasLimitFor derived its answer from the call's calldata, and building that call mints a fresh address, so a secp256k1 keypair was generated per transaction on the path this change exists to keep free of work. The limit is now resolved once, while the chain is being asked, and the send path reads a number. Measured on the AMM scenario, allocations per generated transaction fell from 50 to 39; the scenarios that mint an address in their priced call were paying far more. The PR body claimed the send path issues no estimate. It did not, but it did do per-transaction keygen and ABI packing, which is the same claim broken a different way. A test now counts how often the priced call is built and fails if that number moves with the number of transactions. Disperse could not be priced at all. disperseEtherFixed opens with require(msg.value == fixedEtherAmount * recipients.length), and a priced call carried no value, so the estimate reverted and any profile naming disperse refused to start. GasEstimateCall now carries a value, and the send path sets it too — which it never did, so every disperse reverted on entry and burned its limit while reporting as sent. A contract bound from a registry entry could hold a different fixedEtherAmount; reading it back off the contract needs GasEstimateCalls to be able to report a failure, and the comment says so. Each quote now has its own timeout inside the step's collective budget, so one endpoint that accepts a request and never answers cannot spend the ceiling for every scenario behind it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
All three findings confirmed and fixed. The fix landed in #74, one commit up the stack ( The send path rebuilt the priced call on every transaction. Correct, and worse than the comment says: My PR body claimed "the send path issues no estimate". Literally true and materially wrong — it did per-transaction keygen and ABI packing instead. There is now a test that counts how often the priced call is built and fails if that number moves with the number of transactions. It catches the regression. Disperse could not be priced. Correct. The send path never set On reading One shared budget for every quote. Fair. Each quote now has its own 10s timeout inside the step's collective budget, so one endpoint that accepts a request and never answers cannot spend the ceiling for every scenario behind it. |
arctic-1 was excluded because it can be re-genesised. It can, but we control that and intend not to, and the exclusion cost more than the risk. Supplying the file from a deployment instead lets anyone who can edit that source point a run at a contract of their choosing, and the code-hash check cannot catch it: the check proves the address holds the code the file recorded, not that the code is ours. Committing removes that exposure and puts every address change through review. Two corrections to what the section claimed about a re-genesis. It does not break a run. Resolve returns "deploy" when the chain key does not match, with no error, so a run whose committed entries have gone stale deploys its own contracts — which is what every run did before this directory held a file. Recovery is a pull request here at whatever pace suits. What does not degrade quietly is a recorded address whose code has changed: Verify fails and the run stops at startup. It also omitted the requirement that makes committing work at all. The registry keys on genesisHash, so a profile that omits it matches nothing, and a run that omits it on a chain the registry describes already fails at startup with that explanation. Any profile reading a committed entry has to carry it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three contracts deployed to arctic-1 from a local run, so a deployment binds them instead of deploying its own on every restart. defi-amm 0x225af59603bb554686adfbb2869af4cec12488a1 tokenops-erc20 0xe66344c8ed6dbde610725cd7e3359b1fe4d7ff26 tokenops-erc721 0x815299db5f8e3c6c42356655429cf2701e502bea The run recorded 3 deployed and 0 copied unverified, so nothing here is carried through from a file this run did not check. Verified independently of the run that wrote it. Every address holds code, the recorded hash is keccak256 of the code the chain serves, and that code is byte-identical to what the pinned compiler produces locally. So these are our contracts, not merely addresses that hold something. Verified end to end: a binary carrying this file bound all three and sent no deployment, and the transactions it then sent moved the contracts' state. arctic-1's AMM reserve is no longer at its floor and its ERC721 token 1 has an owner, so the transactions executed rather than reverting. The tripwire test asserting an empty compiled-in registry said a chain file needs its own test naming it. It has one. Three mutations, three caught: a changed address, a changed genesis hash, a dropped contract. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review found that StorageRW's doc claimed the margin absorbs the gap between its priced calls and read's expensive shape. Settings.Validate accepts a GasMargin of 1, and at 1 nothing absorbs it: the first read of a written slot would land in a block having burned its whole limit, which is the failure this sizing exists to remove. read costs most against a slot that already holds a value, and every call the scenario prices reads an untouched one, so no measurement reaches that shape. What the write and rmw models miss is a cold slot read, which EIP-2929 prices at 2,100 and which is not one of the costs Sei moves. The scenario adds twice that as a constant, so correctness no longer depends on how an operator sets a knob. Two mutations, two caught: the headroom removed, and the headroom shrunk below a cold read. Also moved gasProbeSlot out of CreateContractTransaction's doc comment. Inserting it there left the function undocumented and turned a warning about that function's PRNG draw order into commentary on a package-level big.Int. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Both StorageRW findings confirmed and fixed, in #76 at the top of the stack ( The Fixed with a constant rather than by requiring Guarded at the margin that gives it no help: The comment placement is also mine. I inserted Worth noting the first fix I attempted for this reported |
#71 and #72 both merged squashed, so their content reached main under new commits and this branch's history no longer shares it. Git therefore saw the scenario files as added on both sides. Resolved toward this branch for AMM.go, AMM_test.go and ERC721.go, which carry the same contracts with their hard-coded gas constants replaced by the measured path — main holds the earlier form. Removed ERC721_test.go, which pinned a constant this branch deletes and could not compile against it. Took main's deferred NotReady in main.go. That fix landed in #72 after this branch was cut, and it covers every exit rather than the signal path alone.
The conflict resolution took main's deferred call but git had already auto-merged this branch's inline one from a region that did not conflict, so the signal path called NotReady twice. Harmless, and the opposite of what #72 did: it replaced the inline call precisely because it covered only that path.
|
All five findings are fixed, none of them in this PR — they landed upstack in #74 and #76, and I have verified each against the top of the stack. Merging this one and #74 back to back so the two blockers do not sit on [blocker] Disperse probe omits [blocker] Disperse never reads
Verified at the stack head: One note on this branch specifically: #71 and #72 both merged squashed, so the merge back to |
Dismissing because both blockers are fixed, but upstack in #74 rather than in this diff — so a re-review of this PR alone would raise them again, correctly.
Disperse probe omitting msg.value: GasEstimateCall carries a Value now, plumbed into the CallMsg. Disperse.go:93.
gasCallData rebuilding the priced call per transaction: the limit is resolved once at pricing time and the send path reads a map. Allocations per generated transaction on AMM fell 50 to 39, and a test now counts how often the priced call is built and fails if that number tracks the transaction count. base.go GasLimitFor.
#74 is retargeted to main and merges immediately after this one, so neither blocker sits on main. Merging them apart rather than together was a judgement call taken with the repo owner; the alternative was pulling the fixes down into this PR.
|
@seidroid please re-review. Both blockers you raised are correct and both are fixed — but in #74, one PR up the stack, not in this diff. I dismissed the changes-requested review with that reasoning rather than leaving it standing, because a re-review of this PR in isolation would raise them again and be right to. Context for the re-read, so the same two are not the whole result: Disperse probe omitting
Your other three findings are also fixed: Disperse now reads What would be most useful from a re-read is anything in the gas design itself that the first pass did not reach — the fail-closed posture in |
|
Claude finished @bdchatham's task in 6m 21s —— View job Re-review of the gas design
I took the five earlier findings as fixed upstack and did not re-derive them; they are all still present in this diff, so anything reviewing #73 alone will raise them again. Everything below is new. Note on verification: 1. [blocker at
|
| scenario | probe writes | run's worst | holds |
|---|---|---|---|
ERC20 / ERC20Conflict |
_balances[fresh sender] 0→1 (the < branch takes it), _balances[fresh recipient] 0→1 |
same two cold sets on an account's first transfer | ✓ |
AMM |
balIn[fresh] 0→wrapped, balOut[fresh] 0→out; reserves nonzero from the constructor in both |
same two cold sets on an account's first swap | ✓ |
ERC721 |
_balanceOf[fresh] 0→1, _ownerOf[1<<255] 0→addr |
same pair | ✓ |
ERC20Noop |
nothing | nothing | ✓ (its exposure is finding 1) |
StorageRW write/rmw |
store[1<<200] 0→v |
same | ✓ |
StorageRW read |
— | — | the gap you fixed upstack |
Two things worth pinning while they are still true. Pricing ERC721 at 1<<255 rather than at a low id is what makes the guarantee survive a recorded contract that a previous run already minted into — the doc at ERC721.go:61 says this and it is the sharpest comment in the diff. And gasProbeSlot = 1<<200 is outside every configurable keyspace only because RecordCount is a uint64 and pickSlot draws SampleIndex(rng, cfg.RecordCount). That is load-bearing and unwritten; a RecordCount widened later collides in silence, and the probe stops being cold.
5. Smaller things
MaxGasLimitForDatareports "no measured gas limits" when everyExecis 0.foundis set only insideif model.Exec > widest.Exec(generator/scenarios/base.go:258), so a map of models that are all zero looks empty. Unreachable today —gasEstimatorrejectsrequired <= intrinsic, the mock uses 200,000 — but the flag is keyed on the comparison rather than on presence.if !found || model.Exec > widest.Execsays what is meant.- The block-limit error names the wrong number.
blockGasLimitreturnsmin(header.GasLimit, gasEstimateCap)(generator/gas.go:125), and the failure reads "past the chain's %d per block" (generator/gas.go:111). On a chain whose header reports more than 10,000,000, the number printed is the estimate cap, and an operator goes looking for a block-size problem that does not exist. Two values deserve two messages. GasLimitForcollapses two failures into one.(0, false)means both "never priced" and "recomposition failed" (generator/scenarios/base.go:223-233), and every caller reports it as "no measured gas limit". The second is a bug in the model, not a missing measurement, and it now reads as the first.GasLimitForDatais still dead code (generator/scenarios/base.go:238) — no caller. Finding 1 is the caller it was written for.mockGasLimitsdrops the caller's context forcontext.Background()(generator/gas.go:146). Nothing blocks on that path today, so it costs nothing until something does.
Verdict
The core of this is right and the reasoning behind it is unusually well recorded — the fail-closed posture, the ordering that puts pricing after recordDeployments so a pricing failure cannot discard a paid-for deployment, the cold-by-construction probe, and the compile-time gate on GasEstimateCalls are all sound, and I checked the cold-shape claim against each contract rather than taking it. The round-trip test does pin what it says it pins.
Finding 1 is the one I would not ship without: at gasMargin: 1 it is the same silent burn this PR exists to close, reached by a different route, and the PR's own test helper is already shaped to catch it. Findings 2 and 3 are about the code and its stated contract disagreeing, which matters more than usual here because the comments are doing real work.
· branch brandon2/gas-estimate-calls
Re-review found three things the first pass did not reach. All three are real and the first changes behaviour. A priced call has to bound the call the run sends, and for the four scenarios carrying an address it did not. GasLimitFor resolves against the probe's bytes, and calldata costs 16 gas for a non-zero byte against 4 for a zero one, so a probe address holding zero bytes prices a cheaper word than the address a transaction actually carries. Measured: 21,440 intrinsic against 21,560, a shortfall of 120. About one address in thirteen holds a zero byte, and the probe is minted once and held for the whole run, so it is a per-run coin flip rather than a per-transaction one. On the runs where it lands, nearly every transaction is short at a margin of 1, which Validate accepts. Both probe values are now non-zero in every byte, so the priced call is the more expensive one on calldata as well as on storage. The address is still one this run mints and never uses again, which is what makes its slots cold. The tell was in this package's own test helper. requireGasMatchesModel asserts the limit equals what the model derives from the transaction's own calldata, which is exactly the invariant at issue, and it was wired into the two scenarios that satisfy it. ERC721's test had been deleted rather than converted. There is now a test over every contract scenario asserting the priced call's intrinsic cost is at least the sent call's. The margin scaled the calldata intrinsic as well as execution, which both doc comments said it did not. It errs high, so it was not a correctness bug, but it declares gas no byte can consume and it lands hardest on the largest draws a size distribution produces: about thirty thousand at a 32 KiB pad. The margin now scales execution alone. Reusing one Exec across calldata sizes holds only while the varying bytes are ones the contract never reads. StorageRWv1 takes its pad as bytes calldata and touches it nowhere, so nothing copies it into memory. A method taking bytes memory would pay memory expansion growing with the square of the length, none of it in an Exec measured at an empty pad. Said so on GasModel, along with why the floor comparison in Limit is not redundant. Three mutations, three caught. Two survived a first attempt, because the guards were weak rather than the fixes: the token-id case needed draws past 255, where an id first needs a second non-zero byte, and the margin case needed a margin above 1, where the two forms stop agreeing. A fixture of mine was also wrong — at a 32 KiB pad the EIP-7623 floor dominates, so the assertion was about the floor rather than the margin. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@seidroid all three are real and fixed in 1. The priced call was not an upper bound on the sent call. Confirmed: Took your fix. Your observation about 2. The margin scaled the calldata intrinsic. Confirmed, and it matches your figure: Now 3. The precondition on Two of my three mutations survived a first attempt, and both because the guards were weak rather than the fixes. The token-id case needed draws past 255, where an id first needs a second non-zero byte — your example of 257 was the thing my 200-transaction loop never reached. The margin case needed a margin above 1, where the two formulas stop agreeing. One fixture of mine was also simply wrong: at a 32 KiB pad the EIP-7623 floor dominates, so Three mutations, three caught. Worth another look given this round changed behaviour rather than comments. |
|
Claude finished @bdchatham's task in 5m 2s —— View job Re-review of
|
…hind it Re-review confirmed the three behaviour fixes and raised five smaller things. The one worth taking before merge was the shape of the new test. It compared calldata costs and threw the transaction's own limit away. Two different failures land there and only one is about calldata: a probe that prices a cheaper call than the run makes produces a short limit, and so does a scenario that prices correctly and then never reads the measurement back. The second is what Disperse did, and it has now been found twice by review rather than once by this suite. Asserting the limit catches both. It also fits a scenario whose calldata varies, which the calldata form did not. StorageRW recomposes against the bytes it is about to send, so its probe has no obligation to bound them, and it passed the old assertion only because the fixture left the size distribution unset. A default pad added later would have failed a scenario that was correct. The assertion caught Disperse immediately, wanting 258,072 against the 200,000 it declared. Rather than skip it, the Disperse fix comes down from #74: the priced call carries the value the contract requires, and the send path reads the measurement and sets that value too. That defect is in this diff, so it belongs in this commit. Also from the re-review: deleted GasLimitForData, which nothing called once the probe-maximal route was taken; keyed MaxGasLimitForData's presence check on the map rather than on a comparison, which reported a zero execution term as missing; named what the block-fit guard does not cover, since it evaluates the probe's bytes and a varying-calldata scenario can pass it and still exceed a block; gave mockGasLimits the caller's context; and stopped minting a secp256k1 key to produce twenty bytes that are only ever an ABI argument, which Disperse asked for a hundred of per call. Two mutations, two caught: a scenario that prices and never reads it back, and a probe address left holding zero bytes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@seidroid review Everything from both earlier passes is addressed as of From the first pass — all five fixed. Disperse's probe carries the From the second pass — all three behaviour fixes plus four of the five smaller items. Probe calldata is maximal by construction, so The one you flagged as pre-merge is done, and it found something. Asserting the limit rather than the calldata cost behind it caught Disperse immediately — 258,072 needed against 200,000 declared, the figure you predicted. Rather than skip that scenario I pulled the Disperse fix down from #74, since the defect is in this diff. This PR is now individually correct rather than relying on the next one. Your equivalence argument is load-bearing and I checked it independently: the EIP-7623 floor is a fixed 2.5x of the intrinsic's variable part at every byte composition I tried, so the intrinsic bound does imply the limit bound. I still moved the assertion to the limit, because the two failures that land there are not both about calldata. Verification on my side: |
|
Claude finished @bdchatham's task in 5m 53s —— View job Re-review of
|
The Value field added last round broke the scenario it was added for. The justification for an unfunded caller covers the fee check and not the value check: the state transition tests that the caller can cover msg.value whatever the fee fields say, so a value-carrying call from a fresh address fails with insufficient funds before it runs. Pricing then fails closed and any profile naming disperse refuses to start — the mirror image of the defect the field was added to fix. Measured against arctic-1, injecting Disperse's runtime with fixedEtherAmount at 1 and sending the hundred recipients the scenario sends: no balance override insufficient funds balance override 3,573,887 gas A call carrying a value now goes through the raw endpoint with a state override funding its caller by exactly what the call sends. The override touches that balance and nothing else, so the contract's slots stay cold and the quote stays the expensive shape. A call sending no value goes through ethclient as before, so an endpoint serving no state overrides still prices every scenario that needs none. Nothing exercised this path. Every other test stubs the estimator, and the mock chain discarded the request it was handed, so the one place the shape of this call could have been asserted asserted nothing. It records the request now, and a test reads it back: no fee field on any priced call, calldata present, and a value-carrying call funded by an override for at least what it sends. Two mutations, two caught: the value sent with no override, and the value dropped from the call. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four branches reviewed apart cost more than the separate boundaries were worth. The Disperse fix and the Value field landed in one, the read headroom in another, and the newest estimator fixes in a third, so no branch was individually correct and every review pass had to reason about where a fix actually lived. A reviewer checked the branch rather than the claim and found three fixes I had said were present and were not. All of it is one change: stop declaring numbers the chain owns, and ask it instead. Gas limits, the fee cap, and the contract addresses that make a run reproducible across restarts. Four files conflicted, all where both sides had grown a version of the same fix. Resolved toward the union in each: the estimator keeps the new helper that funds a value-carrying call and the per-quote timeout that bounds it; the mock chain keeps both the recorded estimate requests and the gas price; the test file keeps every test from both sides. The merge caught one thing by itself. The newest test built a config with no fee cap, and the fail-closed path this merge brought in rejected it — the two halves meeting the way they should.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ee754ed. Configure here.
bdchatham
changed the title
|
@seidroid review This PR is now the whole change. #74, #75 and #76 are collapsed into it and closed — reviewing them apart was what let a fix sit in one branch while the defect it fixed sat in another, which you caught last round when three things I said were present were not. What is here that you have not read:
Smaller items from your last round also taken: Not taken, with reasons: the Worth reading as a new surface rather than a diff against your last read — it is four PRs wide now, and each of your three rounds has found something real. |
|
Claude finished @bdchatham's task in 6m 51s —— View job Full-surface review of
|
main treats a context sentinel in the error chain as a clean exit. The per-quote timeout added last round was a bare context, so it expired while the collective budget was still healthy, nothing above stripped the sentinel, and a run that never priced a call exited reporting success. Bound the quote through WithinBudget, which strips it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Confirmed and fixed in The reasoning holds exactly as written. The fix routes the per-quote bound through Guarded by Worth naming plainly: I introduced this while implementing your previous round's suggestion to bound each quote. The suggestion was right; my first implementation of it reintroduced the defect the surrounding code exists to prevent. |
|
@seidroid review |
There was a problem hiding this comment.
All five previously-flagged findings are genuinely fixed — the Disperse probe now carries msg.value through a balance state override and reads its limit back, priced calldata is resolved once at pricing time instead of per send, and StorageRW's unpriceable read peak is covered by an explicit constant with a test at margin 1. What remains is non-blocking: two sibling transfer scenarios still carry hard-coded fee caps, the gas tip stayed a constant while the cap became chain-derived, and the StorageRW send path now packs its calldata twice.
Findings: 0 blocking | 4 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] Committing
registry/chains/arctic-1.jsonchanges startup behaviour for existing arctic-1 profiles:planOne(generator/prepare.go:250) errors whenreg.HasChainID(chainID)is true and the profile names nogenesisHash, which was previously unreachable becausechains/shipped empty.profiles/arctic-1.jsonhaschainId: 713715and nogenesisHash, and survives only because it drives EVMTransfer alone and so creates no bindings — adding any contract scenario to it turns it into a startup failure. Adding the genesis hash to that profile would keep it consistent with the entry the binary now ships, and the behaviour change is worth a line in the PR's Known section for operators running their own arctic-1 profiles. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
EVMTransferNoop declared 20 gwei and EVMTransferFast 200 gwei as literals. Both are wrong on a chain whose base fee has passed them, and 20 gwei is under what pacific-1 and atlantic-2 charge today, so that scenario was rejected at the ante rather than failing loudly. Both now read the cap the run resolved from the chain, which is what the comment in utils already claimed of every path. The 2 gwei tip becomes one exported constant instead of three literals. EVMTransferFast keeps its zero tip, which is deliberate; only its comment said otherwise. profiles/arctic-1.json gains the genesisHash the registry keys on. The profile drives EVMTransfer alone, so it started without one; adding any contract scenario to it would have turned that into a startup failure. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Addressed three of the four non-blocking suggestions. Detail on each: Hard-coded fee caps ( Tip constant (
StorageRW double-pack ( Guard added: One honest gap: that test names its three scenarios explicitly, so a new raw scenario is not covered automatically. The contract path is covered separately by |
|
@seidroid review |
There was a problem hiding this comment.
The final commit migrates EVMTransferNoop and EVMTransferFast off their hard-coded caps, so the fee cap is now genuinely single-sourced from the chain across every send path, deployment and the funder. Nothing blocking remains; two earlier non-blocking findings (the tip/cap relationship in resolveGasFeeCap, and StorageRW packing its calldata twice per transaction) are still present unchanged.
Findings: 0 blocking | 2 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- None at the file/PR level.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
| if suggested.Sign() <= 0 { | ||
| return fmt.Errorf("the chain reported a gas price of %s, so no cap can be derived from it", suggested) | ||
| } | ||
| cap := scaleWei(suggested, g.config.GetGasFeeCapMultiplier()) |
There was a problem hiding this comment.
[suggestion] Still open from the previous round: the cap is derived from the chain but the tip stayed a constant. utils.GasTipCapWei is 2 gwei (generator/utils/utils.go:37) and EVMTransfer.go:68 / EVMTransferNoop.go both declare it, while the funder declares 1 gwei. Nothing ties either to the value computed here, so if eth_gasPrice reports below ~0.4 gwei — a local dev node, or a chain configured with a lower min fee — suggested * 5 lands under the tip and every transaction is structurally invalid with max priority fee per gas higher than max fee per gas. That is a whole-run failure derived from a perfectly valid chain reading, and the Sign() <= 0 guard above does not catch it because the price is positive. The old constants (20/100/200 gwei) were all far above the tip and could not hit this. A cap = max(cap, gasTipCapWei) clamp here, or deriving the tip from the same read, closes it.
| if err2 != nil { | ||
| return nil, fmt.Errorf("storagerw: pack %q: %w", op, err2) | ||
| } | ||
| limit, err := s.MaxGasLimitForData(data) |
There was a problem hiding this comment.
[suggestion] Still open from the previous round. Caching the parsed ABI on the scenario removed the per-transaction JSON decode, which is a real improvement, but the double pack itself is unchanged: data is packed above only to size the limit and is then discarded, and s.contract.Rmw/Read/Write below packs the identical arguments again. MaxGasLimitForData then runs core.IntrinsicGas and core.FloorDataGas, both O(len(data)), over that throwaway copy. With a size distribution reaching multi-KiB pads that is a full extra allocation and copy of the calldata per generated transaction, in the hot loop of a load generator — the same class of send-path work this PR argues against in the gasLimits rationale in base.go. Everything the limit depends on is known here without materialising a second copy: the selector, the slot word's byte composition, the length word, and the padded pad length (the pad is all zero bytes from pickPad). Deriving the zero/non-zero counts from those keeps the recomposition exact and drops the second pack.
1 participant
Replaces the numbers this tool hard-codes with numbers it asks the chain for.
Sei charges 72,000 for a storage write that takes a slot from zero, against the
EVM default of 20,000. Every gas constant in the repo was calibrated against the
default, so every contract scenario was short on Sei — and a short limit does not
fail visibly. The transaction reaches a block, burns the whole limit, and a run
with
trackReceiptsoff reports it as sent.AMM.swapAToBERC20.transferERC721.mintWhat it does
A scenario declares
GasEstimateCalls(); the run prices them once at startup,after the contracts are bound, and the send path reads a number.
ContractScenarioBasedoes not implement the method, so a scenario added withoutone does not compile.
The fee cap comes from
eth_gasPriceat startup. Contract addresses for arctic-1ship in
registry/chains/, so a run binds them instead of redeploying on everyrestart.
Pricing failure stops the run. There is no fallback constant, because a fallback
constant is what this removes.
Also here
/healthzand/readyz, and readiness now drops on every exit rather thanonly on a signal
ammscenario, and ERC721's gas constant fixed before it was deletedVerification
Run against arctic-1: contracts bound from the registry with no deployment sent,
gas quoted per operation, transactions executed — the AMM reserve moved off its
floor and ERC721 token 1 has an owner.
Every fix broken on purpose first. Four review rounds found five, three, two and
one defect; all are fixed here. Twice a fix introduced its own mirror image: the
value-carrying estimate now funds its caller through a state override, and the
per-quote timeout runs through
WithinBudgetso a hung endpoint reads as afailure rather than a clean shutdown.
gofmt,go vet,golangci-lintclean; 15 packages passing.Known
registry/chains/arctic-1.jsonmakesgenesisHashrequired for anyarctic-1 profile that drives a contract scenario;
profiles/arctic-1.jsonnowcarries it, but a profile kept outside this repo needs it added
packs its calldata twice per send; both are flagged non-blocking on the PR
until it passes the previous high-water mark (PLT-1107)
undetected
--duration Nexits 1 at the end of a bounded runCollapsed from #74, #75 and #76, which are closed. Deploys via
sei-protocol/platform#1587.