fix(loadtest): update gas price logic in runner - #999
ayaanoncrypto wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Good find, and the regression test is better evidence than most PRs carry. The direction is right; a few things need doing before this can go in.
The bug is real — confirmed
canDecrease := blockNumber + blocksToWait <= header.Number.Uint64()blockNumber is the caller's current block, and header.Number is the latest block fetched two lines earlier in the same function. So this asks "has the chain advanced 5 blocks since the caller read the block number, milliseconds ago?" — essentially never true. canDecrease is permanently false, every decrease is suppressed, and maxFeePerGas ratchets upward for the life of the run without ever coming back down. That is not hysteresis, it is a one-way valve.
The test proves it. I applied runner_gas_test.go on top of unmodified main:
--- FAIL: TestRunnerGasPriceDecrease
runner_gas_test.go:76: Expected 110, got 210
Passes with the fix, and clean under -race (the HTTP round trip synchronises the baseFee / blockNum mutations, so there is no race despite the shared variables). That is a proper regression test.
The first deletion is correct, for a reason the PR does not give
The cache check removed from suggestMaxFeePerGas is dead code: the only caller performs the identical check immediately before calling it (runner.go:1509), so blockNumber <= *r.cachedBlockNumber cannot be true by the time the function runs. Removing it is right, and worth calling out somewhere, because a reviewer's first instinct is that you deleted a cache from a hot path in the load generator.
Blocking
1. Lint is failing, and it is the new test file. Reproduced locally:
runner_gas_test.go:28:33: Error return value of (*json.Decoder).Decode is not checked (errcheck)
runner_gas_test.go:32:15: Error return value of fmt.Fprintf is not checked (errcheck) [x3]
4 issues: errcheck 4
Four one-liners (_ = fmt.Fprintf(...), and handle or discard the Decode error). Every other check on the PR is green.
2. The second deletion is a behaviour change with no stated justification. The blocksToWait block was intended as hysteresis — only let the suggested fee fall every 5 blocks. It is implemented wrongly, but the intent is legitimate, and this deletes the intent rather than fixing it. The one-line fix that keeps it would be to compare against the cached block rather than the freshly fetched header:
canDecrease := blockNumber >= *r.cachedBlockNumber + blocksToWaitI lean towards your choice — maxFeePerGas already carries 2x base-fee headroom, so tracking downwards promptly is fine for a load generator — but I cannot tell from the diff whether dropping the hysteresis was a decision or collateral from fixing the comparison. Could you confirm which?
Nits
gp1, _ := r.getSuggestedGasPrices(ctx)followed bygp1.Int64()will nil-deref if it ever errors.if err != nil { t.Fatalf(...) }is cheap insurance in a test whose job is catching regressions.- The inline
eth_getBlockByNumberresponse is ~1.5KB on a single line. A struct literal or atestdatafixture would make it reviewable. - Consider a second assertion that a repeat call at the same block does not re-fetch
FeeHistory. Since this PR touches cache-adjacent code, pinning the part you are not changing is worth the three lines.
To approve
Lint green, and confirmation that the hysteresis removal is intentional. Happy to re-review quickly after that.
Description
Jira / Linear Tickets
Testing