Skip to content

fix(loadtest): update gas price logic in runner - #999

Open
ayaanoncrypto wants to merge 2 commits into
0xPolygon:mainfrom
ayaanoncrypto:contrib/issue-555
Open

ayaanoncrypto wants to merge 2 commits into
0xPolygon:mainfrom
ayaanoncrypto:contrib/issue-555

Conversation

@ayaanoncrypto

Copy link
Copy Markdown

Description

Jira / Linear Tickets

Testing

  • Test A
  • Test B

@minhd-vu minhd-vu changed the title Fix gas price update logic in loadtest runner fix(loadtest): update gas price update logic runner Sep 21, 2026

@minhd-vu minhd-vu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 + blocksToWait

I 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 by gp1.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_getBlockByNumber response is ~1.5KB on a single line. A struct literal or a testdata fixture 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.

@minhd-vu minhd-vu changed the title fix(loadtest): update gas price update logic runner fix(loadtest): update gas price logic in runner Sep 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants