fix(fill): warn and discard a partial opcode trace instead of reporting it - #3505
Open
jochem-brouwer wants to merge 1 commit into
Open
fix(fill): warn and discard a partial opcode trace instead of reporting it#3505jochem-brouwer wants to merge 1 commit into
jochem-brouwer wants to merge 1 commit into
Conversation
…ng it
`debug_traceBlockByHash` returns one entry per transaction. When a single
transaction's trace fails, its entry carries an error instead of a
`result`, and the aggregation loop skipped it silently -- returning a
well-formed tally that is short by whole transactions, with no exception
and no log line. The caller could not tell 17 traced transactions from
18.
That surfaced while filling the compute benchmarks.
test_identity_uncachable[size_32, 300M] failed with
Target opcode IDENTITY count mismatch: expected ~1693322 (+-5.0%),
got 1598624
The deficit is 94,698, which is exactly the calls one full-cap
transaction makes: (16777216 - 15512 - 17) // 177. The block itself was
fine -- all 18 transactions present, 299.998M of 300M gas used -- and the
same case passes on re-run (verified four times). Nothing was wrong with
the benchmark; one transaction's trace was lost and the shortfall was
reported as if it were a real count.
It read as a benchmark bug for exactly as long as it took to notice the
count was one whole transaction short. The test's own model is correct:
across the ten gas values that passed, its predicted transaction counts
match the fixtures exactly and its predicted gas matches to within the
128-143 gas per transaction that WhileGas necessarily strands.
So the aggregators now report how many entries contributed, and a tally
that does not cover every transaction is discarded with a warning naming
the shortfall. It is not fatal: a missing trace should not throw away a
block that filled correctly, and the count can be recovered by re-running.
The fixture is still written, it just carries no opcode count.
Struct-log aggregation had the same hole and gets the same treatment; an
entry with no `structLogs` key is a failed trace, not an empty one.
jochem-brouwer
added
backport forks/amsterdam
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## forks/amsterdam #3505 +/- ##
================================================
Coverage 93.90% 93.90%
================================================
Files 624 624
Lines 36905 36905
Branches 3326 3326
================================================
Hits 34655 34655
Misses 1569 1569
Partials 681 681
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:
|
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
1 participant
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.
debug_traceBlockByHashreturns one entry per transaction. When a single transaction's trace fails, its entry carries an error instead of aresult, and the aggregation loop skipped it silently -- returning a well-formed tally that is short by whole transactions, with no exception and no log line. The caller could not tell 17 traced transactions from 18.That surfaced while filling the compute benchmarks. test_identity_uncachable[size_32, 300M] failed with
The deficit is 94,698, which is exactly the calls one full-cap transaction makes: (16777216 - 15512 - 17) // 177. The block itself was fine -- all 18 transactions present, 299.998M of 300M gas used -- and the same case passes on re-run (verified four times). Nothing was wrong with the benchmark; one transaction's trace was lost and the shortfall was reported as if it were a real count.
It read as a benchmark bug for exactly as long as it took to notice the count was one whole transaction short. The test's own model is correct: across the ten gas values that passed, its predicted transaction counts match the fixtures exactly and its predicted gas matches to within the 128-143 gas per transaction that WhileGas necessarily strands.
So the aggregators now report how many entries contributed, and a tally that does not cover every transaction is discarded with a warning naming the shortfall. It is not fatal: a missing trace should not throw away a block that filled correctly, and the count can be recovered by re-running. The fixture is still written, it just carries no opcode count.
Struct-log aggregation had the same hole and gets the same treatment; an entry with no
structLogskey is a failed trace, not an empty one.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