Skip to content

EIP Complexity Assessment — Checklist Revision 2 - #101

Open
marioevz wants to merge 3 commits into
mainfrom
revised-checklist
Open

EIP Complexity Assessment — Checklist Revision 2#101
marioevz wants to merge 3 commits into
mainfrom
revised-checklist

Conversation

@marioevz

@marioevz marioevz commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

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

  • Are the new rows scoreable at CFI time? Rows 2–5 look assessable a priori. Row 1 is the risky one — spotting that an EIP moves an observable execution detail is exactly the insight EIP-7928's original evaluation missed.
  • The +1 per 3 Cross-EIP increment and the 12/23 tier thresholds are judgement, not measurement.
  • The calibration fits 9 EIPs on one fork, and all proposed scores were assigned in hindsight. Caveats are listed in full in the calibration README.

@danceratopz danceratopz left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

  1. File measures come from the working tree, not the measured refloc, test_funcs and cases are 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.
  2. 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.
  3. --today defaults to the wall clock — makes the MATURE_WEEKS fit 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.
  4. The documented reproduce command overwrites the tracked dataset — following README §8 rewrites amsterdam-dataset.json in place, which silently changes every figure in proposed-anchors.md, since proposed-anchors.py can't be pointed elsewhere.

Worth fixing — real, but nothing moves

  1. 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.
  2. cases silently 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

  1. --collect without fill should hard-fail — currently zeroes every count behind an easy-to-miss stderr line.
  2. 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,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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 baseline branch below only runs when owners is 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 touched

Magnitude 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*")):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

--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:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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 know

and 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")):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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
  • --today is omitted here, so weeks is 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 in proposed-anchors.md, since proposed-anchors.py hardcodes a read of
    amsterdam-dataset.json from 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 danceratopz left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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!

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