fix(benchmark): split benchmark gas evenly: every tx can pay data floor - #3348
Draft
jochem-brouwer wants to merge 1 commit into
Draft
fix(benchmark): split benchmark gas evenly: every tx can pay data floor#3348jochem-brouwer wants to merge 1 commit into
jochem-brouwer wants to merge 1 commit into
Conversation
…ta floor
`split_transaction` gave each split the gas limit cap and the last one
whatever remained. Every split is a `model_copy()` of the same
transaction, so each carries identical calldata and owes identical floor
data gas -- but a remainder tail is unrelated to that floor and can land
below it, producing a transaction no client will accept.
Filling the compute benchmarks against Amsterdam hit exactly this. With
the EIP-7825 cap of 2**24, a 220M benchmark splits into 14 transactions
and the last receives 220,000,000 - 13 * 16,777,216 = 1,896,192 gas,
while the 36,864-byte worst-case BLS12_G2MSM k=128 calldata costs
2,374,296 at the floor. geth rejected the build:
insufficient gas for floor data gas cost: have 1896192, want 2374296
220M was the only value in 100..300M that failed, which is why this
surfaced as a single mystifying case rather than a pattern.
It is also new. The floor is 15,000 + 16 * 4 * 36,864 = 2,374,296 under
Amsterdam, where EIP-7976 charges 64 gas per calldata byte (zero and
non-zero alike) and EIP-2780 lowers the base to 15,000. Under Osaka the
same calldata's floor is 21,000 + 10 * 122,484 = 1,245,840, which fits in
1,896,192 -- so the test was correct until EIP-7976 raised the floor.
Both figures match `transaction_data_floor_cost_calculator` exactly.
Spreading the gas evenly across the same number of splits raises the
smallest share from "whatever remains" to `gas // num_splits`, at least
half the cap -- 15,000,000 at worst across the benchmark gas values,
against a 2,374,296 floor.
The total is deliberately preserved. Dropping the short tail instead
would trade a rejected transaction for a failed assertion, because
`BaseTest.validate_gas_used` requires the block to consume exactly
`gas_benchmark_value`.
Also makes the BLS12_G2MSM skip guard floor-aware; it compared only the
standard intrinsic cost, which is the smaller of the two on calldata this
large.
The existing split tests asserted the cap-cap-remainder shape itself, so
they now assert the invariants that matter: the split count, the cap
ceiling, exact total, incrementing nonces, and an even spread. The new
test fails on the old code at 220M with `assert 1896192 >= 2374296`.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## forks/amsterdam #3348 +/- ##
================================================
Coverage 93.53% 93.53%
================================================
Files 624 624
Lines 37070 37070
Branches 3394 3394
================================================
Hits 34675 34675
Misses 1645 1645
Partials 750 750
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
11 tasks
Contributor
|
I'm not sure we should modify the framework just to make a single benchmark test work better. Instead, maybe we should refactor the test itself? |
Member
Author
|
Yes, lets refactor the test. I'll keep this PR open in draft and will close once we do so. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note: written by LLM
MERGING THIS WILL CHANGE THE BENCHMARK/TEST OUTPUT SIGNIFICANTLY DUE TO UPDATED TX SPLITTER LOGIC
split_transactiongave each split the gas limit cap and the last one whatever remained. Every split is amodel_copy()of the same transaction, so each carries identical calldata and owes identical floor data gas -- but a remainder tail is unrelated to that floor and can land below it, producing a transaction no client will accept.Filling the compute benchmarks against Amsterdam hit exactly this. With the EIP-7825 cap of 2**24, a 220M benchmark splits into 14 transactions and the last receives 220,000,000 - 13 * 16,777,216 = 1,896,192 gas, while the 36,864-byte worst-case BLS12_G2MSM k=128 calldata costs 2,374,296 at the floor. geth rejected the build:
220M was the only value in 100..300M that failed, which is why this surfaced as a single mystifying case rather than a pattern.
It is also new. The floor is 15,000 + 16 * 4 * 36,864 = 2,374,296 under Amsterdam, where EIP-7976 charges 64 gas per calldata byte (zero and non-zero alike) and EIP-2780 lowers the base to 15,000. Under Osaka the same calldata's floor is 21,000 + 10 * 122,484 = 1,245,840, which fits in 1,896,192 -- so the test was correct until EIP-7976 raised the floor. Both figures match
transaction_data_floor_cost_calculatorexactly.Spreading the gas evenly across the same number of splits raises the smallest share from "whatever remains" to
gas // num_splits, at least half the cap -- 15,000,000 at worst across the benchmark gas values, against a 2,374,296 floor.The total is deliberately preserved. Dropping the short tail instead would trade a rejected transaction for a failed assertion, because
BaseTest.validate_gas_usedrequires the block to consume exactlygas_benchmark_value.Also makes the BLS12_G2MSM skip guard floor-aware; it compared only the standard intrinsic cost, which is the smaller of the two on calldata this large.
The existing split tests asserted the cap-cap-remainder shape itself, so they now assert the invariants that matter: the split count, the cap ceiling, exact total, incrementing nonces, and an even spread. The new test fails on the old code at 220M with
assert 1896192 >= 2374296.Description
Related Issues or PRs
N/A.
Checklist
just static<type>(<area>): <title>, where<type>and<area>come from an appropriateC-<type>, respectivelyA-<area>, label. The title should match the target squash commit message.Cute Animal Picture