EIP Complexity Assessment — Checklist Revision 2 - #101
Conversation
danceratopz
left a comment
There was a problem hiding this comment.
Really love this data driven approach! Clever and great idea.
Reviewed with Claude: It re-ran the calibration end to end and drafted the inline comments below, which I've read and edited.
The dataset does re-create. Re-running calibrate.py against origin/forks/amsterdam gives TEU = 0.39·S^1.62 vs the README's 0.38·S^1.62, r = 0.922 vs 0.920, LOO 20% vs 21%, fork closure 326/330 vs 320/328, mature n = 9 either way, and the per-anchor table matches. proposed-anchors.py reproduces proposed-anchors.md verbatim, including the +11 gap on EIP-7928. Stdlib-only, ~3 minutes. No concerns about the conclusions — the comments below are about the audit trail, not the result.
Important — these change what the dataset contains
- File measures come from the working tree, not the measured ref —
loc,test_funcsandcasesare read off whatever is checked out, so an untracked variable feeds README §5 and half of §6's per-anchor table; TEU itself is unaffected. - Nothing pins the measured commit — §7 plans a re-run after Amsterdam ships, and there'll be no way to separate real change from branch drift. Suggests pinning the rev rather than adding
--until, which filters on a mutable committer date. --todaydefaults to the wall clock — makes theMATURE_WEEKSfit sample depend on when the script runs; EIP-7997 crosses the line around 2026-09-06 and would join silently. Falls out of (2) if the date is derived from the pinned tip.- The documented reproduce command overwrites the tracked dataset — following README §8 rewrites
amsterdam-dataset.jsonin place, which silently changes every figure inproposed-anchors.md, sinceproposed-anchors.pycan't be pointed elsewhere.
Worth fixing — real, but nothing moves
- Title matching mints owner EIPs that have no row — 22 work units are dropped from both the per-EIP rows and the baseline, and EIP-7928 is diluted by 2 PRs. The fit barely moves (~2% on 7928), but the mechanism is unbounded. Three-character fix.
casessilently degrades to 0 — the realistic trigger is the first run on a new fork, where no cached dataset exists and the filled-cases row gets fabricated rather than omitted — the same row §5 cites to reject filled cases.
Low — hygiene
--collectwithoutfillshould hard-fail — currently zeroes every count behind an easy-to-miss stderr line.- No revision filter on assessments — correct today (all 28 are revision 1), but the first revision-2 assessment silently pools into a revision-1 fit, which is exactly what this PR warns against.
One practical note
(1), (2), (3) and (5) all change what the dataset contains — fixing the script doesn't fix the committed JSON. So they want to land as one commit and one regeneration, with the resulting SHA stamped into the file; regenerating before pinning just produces another unverifiable dataset.
(4), (6), (7) and (8) change no current number and can follow separately.
| out = subprocess.run( | ||
| ["git", "-C", str(repo), "log", branch, f"--since={WELD_DATE}", "--numstat", | ||
| "--no-merges", "--format=__C__%H|%ad|%an|%s", "--date=short"], | ||
| capture_output=True, text=True, check=True, |
There was a problem hiding this comment.
Reproducibility: pin the rev, don't bound it by date.
The log window is open-ended on the right, and the dataset records generated_on but not the commit it measured — so there's no way to check a re-run against the committed numbers.
Re-running today against origin/forks/amsterdam picks up 12 commits that landed after 2026-08-05 (917 → 929 units), which moves EIP-2780 from 10 → 11 PRs and EIP-7778 from 10 → 11 PRs.
I'd suggest not adding --until. It filters on committer date, which rebases and force-pushes mutate, and it can't exclude a commit that gets rebased into the window later. Pinning the rev makes the window exact and --until unnecessary:
ap.add_argument("--rev", default=None,
help="commit or ref to measure; default: upstream/forks/<fork>")
...
rev = subprocess.run(
["git", "-C", str(repo), "rev-parse", args.rev or f"upstream/forks/{args.fork}"],
capture_output=True, text=True, check=True).stdout.strip()then git_log(repo, rev) and stamp data["rev"] = rev into the JSON. Reproduction becomes --rev <sha from the dataset>, byte-identical.
| ap.add_argument("--gh-repo", default="ethereum/execution-specs") | ||
| ap.add_argument("--collect", action="store_true", | ||
| help="recount filled test cases via the execution-specs venv") | ||
| ap.add_argument("--today", default=None, help="YYYY-MM-DD, for reproducible week counts") |
There was a problem hiding this comment.
--today should default to the tip's commit date, not the wall clock.
Answering the "--until: today?" question here since this is where it bites. date.today() makes weeks — and therefore the MATURE_WEEKS cutoff that selects the fit sample — depend on when you run the script rather than on the history you measured. Once the rev is pinned, deriving the date from it is both more reproducible and more correct: weeks then means "observation window for this EIP", not "weeks until whenever I happened to re-run this".
tip_date = subprocess.run(
["git", "-C", str(repo), "log", "-1", "--format=%ad", "--date=short", rev],
capture_output=True, text=True, check=True).stdout.strip()
today = date.fromisoformat(args.today) if args.today else date.fromisoformat(tip_date)This already matches how the committed dataset was made — generated_on: 2026-08-05 equals the tip date at the time. It's not currently load-bearing (with today = 2026-08-10 the
mature set is unchanged at n=9), but EIP-7997 crosses 10 weeks around 2026-09-06 and would silently join the fit sample on the next re-run.
| kind, eip = classify(path, fork) | ||
| if kind == "own": | ||
| touched.add(eip) | ||
| return titled or touched |
There was a problem hiding this comment.
Title matching mints owner EIPs that have no row, and their work is silently dropped.
EIP_TEXT_RE matches any 4-digit EIP in a subject, and titled wins unconditionally. Across the branch that produces 32 distinct owner EIPs, 17 of which have no tests/amsterdam/eipNNNN_ directory:
EIP-7934: 5 units chore(tests): cache calibration functions in EIP-7934 tests (#2332)
EIP-7702: 3 units fix(tests): drop gas_price from EIP-7702 txs in BAL 7702 tests (#2862)
EIP-7825: 3 units chore(tests): EIP-7825 test fix to avoid 3 failures on EIP-7976 ci run
EIP-6110: 2 units feat(tests): cover noncanonical deposit ABI offsets in eip6110 (#3240)
Two effects, both present in the committed dataset:
- 22 units disappear entirely. They're owned only by phantoms, so they get no row — and the
baselinebranch below only runs whenownersis empty, so they don't land there either. Fork accounting doesn't close over them. - 5 units dilute a real EIP, because
n = len(owners)counts phantoms in the denominator. EIP-7928 loses 2.0 PRs of credit outright, e.g.feat(tests): EIP-7928 tests for EIP 2935 (#2113)splits 1/2 with a 2935 that has no row.
Fix is to intersect against the known suites before letting a title win:
def owners_of(unit: list[dict], fork: str, suites: set[str]) -> set[str]:
...
return (titled & suites) or touchedMagnitude is small — 2 PRs against 7928's 95 is ~2%, and the fit barely moves — but the mechanism has no bound, so it's worth closing.
| cases = collect_cases(repo, fork) if collect else dict(cached_cases) | ||
|
|
||
| rows = [] | ||
| for suite in sorted((repo / "tests" / fork).glob("eip*")): |
There was a problem hiding this comment.
I think this one is def worth addressing.
These measures come from the working tree, not from --branch.
loc, test_funcs, py_files (and cases, via collect_cases) are read off disk, while
churn and PR metrics come from the ref. If the checkout isn't the measured ref, one dataset silently blends two repo states. On my clone (wirex-prototype checked out):
| suite | worktree | origin/forks/amsterdam |
|---|---|---|
| eip7928 | 16 497 | 16 480 |
| eip2780 | 5 992 | 6 255 |
Neither matches the committed 5625 for 2780, so both drift and checkout mismatch are in play.
Cheapest correct fix is to read blobs from the pinned rev instead of the filesystem:
names = subprocess.run(
["git", "-C", str(repo), "ls-tree", "-r", "--name-only", rev, f"tests/{fork}/"],
capture_output=True, text=True, check=True).stdout.splitlines()then git show {rev}:{path} (or one git cat-file --batch pass) for content. If you'd rather keep it filesystem-based, at minimum hard-fail when git rev-parse HEAD != rev — the silent version is the dangerous one.
| README §5: vector counts track parametrization style, not work.""" | ||
| fill = repo / ".venv/bin/fill" | ||
| if not fill.exists(): | ||
| print(f" no {fill}; skipping case collection", file=sys.stderr) |
There was a problem hiding this comment.
--collect with no fill should be a hard error.
Answering "just fail if fill not available?" — yes, for this path. --collect is an explicit
request to recount, and it also suppresses the cache (see main()), so a missing fill
currently yields cases = 0 for every EIP behind a stderr line that's easy to miss:
if not fill.exists():
sys.exit(f"--collect requires {fill}; run `uv sync` in the specs repo or drop --collect")| today = date.fromisoformat(args.today) if args.today else date.today() | ||
|
|
||
| cached: dict[str, int] = {} | ||
| if out.exists() and not args.collect: |
There was a problem hiding this comment.
The cache reads cases from the file it's about to overwrite, and degrades to 0 when absent.
This is the half of (3) that a hard-fail in collect_cases doesn't cover. Delete
amsterdam-dataset.json and re-run without --collect: cached is {}, every cases becomes 0, and report() prints a filled cases row built from all-zero data rather than erroring — pearson returns nan and power_fit returns a meaningless exponent. That's the row README §5 uses to argue filled cases are misleading (r = 0.674, LOO 72%), so a silently fabricated version of it is the worst failure mode here.
Suggest distinguishing unknown from zero:
"cases": cases.get(eip), # None when we genuinely don't knowand skipping the row in MEASURES when any value is None, so you get an honest omission instead of a plausible-looking number. Worth stamping provenance too (data["cases_source"] = "collected" | "inherited"), since the value otherwise has no traceable origin.
| """Return {eip: total_score} and {eip: {anchor: score}} from the markdown.""" | ||
| scores: dict[str, int] = {} | ||
| anchors: dict[str, dict[str, int]] = {} | ||
| for path in sorted(ASSESSMENTS.glob("EIP-*.md")): |
There was a problem hiding this comment.
No revision filter: revision-1 and revision-2 assessments will pool into one fit.
The PR is explicit that "scores are not comparable across revisions", and the new template carries Checklist revision: **2** on line 3, but nothing here reads it. All 28 current assessments parse and are all revision 1, so this is fine today; the first revision-2 assessment silently enters the same regression. Parsing the revision line and either filtering to one revision or keying the fit by revision would make that a hard error rather than a quiet contamination.
(Confirmed the rest of the parsing survives the template change — ### Checklist,
#### Special Considerations and the **Total Score** row all still match, and column 2 is the score in every assessment.)
|
|
||
| ```sh | ||
| # from a clone of ethereum/execution-specs with `upstream` pointing at the repo | ||
| python3 calibrate.py --specs-repo ~/path/to/execution-specs --fork amsterdam |
There was a problem hiding this comment.
This command doesn't reproduce the committed dataset.
Three deviations were needed to get a clean run:
python3 calibrate.py --specs-repo ~/path/to/execution-specs \
--branch origin/forks/amsterdam --today 2026-08-05 --out /tmp/amsterdam-dataset.json--todayis omitted here, soweeksis computed against the wall clock (see the--rev
suggestion — this goes away if the date is derived from the pinned tip);- the default branch is
upstream/forks/<fork>, which is documented, but worth noting the
remote name is a local convention; - without
--out, the command overwrites the committed dataset in place — which also silently
changes every figure inproposed-anchors.md, sinceproposed-anchors.pyhardcodes a read of
amsterdam-dataset.jsonfrom its own directory with no way to point it elsewhere.
For the record, the re-run does substantively reproduce: TEU = 0.39·S^1.62 (vs 0.38),
r = 0.922 (vs 0.920), LOO 20% (vs 21%), fork closure 326/330 (vs 320/328), mature n = 9. The
per-anchor table matches too. The model is robust; it's the audit trail that's missing.
danceratopz
left a comment
There was a problem hiding this comment.
This looks pretty good to me. You can probably merge as-is, but it might be worth getting your Claude to read the review above and applying some of the feedback, more to help with the next time we do this. But up to you, it doesn't need to be perfect!
Five new anchors, one removed, Cross-EIP interactions uncapped, and the checklist versioned. 24 anchors become 28; nominal range 0–72 becomes 0–84.
Driven by the Amsterdam calibration, which compared each Amsterdam EIP's score against the testing work it actually produced in
ethereum/execution-specs. Every mature EIP landed within ±3 of the score its measured work implies — except EIP-7928, short by 11 points with no rows left to score on. The five new rows are where that work should have been recorded.New anchors
1. State-access ordering within opcode execution
Changes where inside an opcode's execution state is accessed, or where gas is charged relative to that access. Because a state access is recorded in the block-level access list only if execution had enough gas to reach it, this ordering is consensus-critical: moving it changes the BAL at every gas boundary of every affected opcode.
2. State gas accounting changes
New state gas accounting rules. State gas is the cost of writing state, as opposed to accessing or executing it:
StateGasCosts,COST_PER_STATE_BYTE, the block-level state gas budget, and the spill path into execution gas.3. New invariant on pre-existing tests
Tests that are not about this EIP must nonetheless assert something this EIP produces. Their logic does not change; they gain a new thing to check. E.g. EIP-7928 added block-level access lists, which could add an assertion to every previous test (not the case but possible).
4. New test-framework primitives
Requires new abstractions in the test framework itself — expectation types, modifiers, helpers — beyond writing test functions with what already exists. E.g. EIP-7928 added
BlockAccessListExpectation, which had to be designed and then applied to tests.5. Unspecified behavior requiring cross-client consensus
The EIP text does not determine the answer for cases a test can construct. Clients must agree on a previously unspecified detail before tests can be baselined. The cost here is coordination and re-baselining, not test writing.
Cross-EIP interactions — uncapped
Now the only anchor without a ceiling. EIP-7928's rationale names 12 interacting EIPs, EIP-8038 names 6, every other Amsterdam EIP names 0 — and all scored the same 3. +1 for every 3 additional interacting EIPs beyond the first 3 (each of which requires its own coordinated test cases).
Removed: Engine API encoding changes
Described a wire-format change at the Engine API layer (JSON → RLP/SSZ), but those migrations are coordinated outside the EIP process.
Versioning
The template carries the revision version, now at number 2, and Revision Notes is split into per-revision subsections.
Scores are not comparable across revisions — re-score rather than compare. The 28 existing assessments stay on revision 1 and are not re-scored here.
Worth pushing back on
+1 per 3Cross-EIP increment and the 12/23 tier thresholds are judgement, not measurement.