enhance(spec-tool): create blob parameter-only hardforks on the fly

[SamWilsn]

🗒️ Description

This pull request hooks up the new fork tool to the Hardfork class, uses that integration to generate new forks when the blob parameters are changed by the tests.

🔗 Related Issues or PRs

closes #1453

Cute Animal Picture

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.87%. Comparing base (2a6f9ee) to head (52a4ebe).
⚠️ Report is 12 commits behind head on forks/osaka.

Additional details and impacted files

@@               Coverage Diff               @@
##           forks/osaka    #1789      +/-   ##
===============================================
- Coverage        87.31%   83.87%   -3.45%     
===============================================
  Files              541      402     -139     
  Lines            32832    25101    -7731     
  Branches          3015     2285     -730     
===============================================
- Hits             28668    21053    -7615     
- Misses            3557     3609      +52     
+ Partials           607      439     -168     

Flag	Coverage Δ
unittests	83.87% <ø> (-3.45%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[spencer-tb]

Thanks for adding this. Just a couple small comments!

I filled all our BPO tests with this and they pass on Geth :D

Would you be happy adding this feature.yaml change, is so once merged we should create a quick patch release on top of v5.4.0? edited: added here #1845

[spencer-tb]

LGTM!

FYI I am filling with:

uv run fill --from=Osaka --until=BPO4 --clean -n 4 -k BPO -v

[SamWilsn]

@spencer-tb should I add --until BPO4 to our tox.ini?

[spencer-tb]

@spencer-tb should I add --until BPO4 to our tox.ini?

Yes please! Totally forgot 😓

[SamWilsn]

Done :3

[spencer-tb]

cc-ing from Discord:

I think this is the cause for the CI fails.

pytest-xdist -> fill command -> Hardfork.clone()
-> libcst_tool() -> spawns process pool -> PicklingError

The AI tells me to add this here.

  exit_code = libcst_tool(
      "<libcst_tool>",
      [args[0], "-j", "1"] + args[1:],
  )

[marioevz]

Thanks, just one comment that I think should skip having to generate temporary forks if the BPO values are unchanged.

[marioevz]

We currently pass the blob parameters unconditionally from the tests, i.e. as long as the fork supports blobs we pass the parameters. And I think we are generating an unnecessary exact copy of the fork in that case.

We could skip this for forks that are defined in the transition tool and only pass the blob parameters if the BPO fork is undefined.

I.e. for Osaka, BPO1 and BPO2, which are defined src/ethereum/forks (BPO1 and BPO2 not yet but eventually), we pass directly the name and skip the BPO parameters. But for example, if we want to test BPO3, which is still undefined in the specs, then in that case we do pass the parameters along with the Osaka fork name.

We have to guarantee that the BPO parameter values match in the specs and the tests but I think that's a completely valid requirement.

[SamWilsn]

So something like pre-populating the fork cache with the forks that exist, or possibly just checking if the temporary fork's values are the same as the template fork's?

[marioevz]

Yes exactly, that would be even better, if the template already matches the values, just return the template.

[marioevz]

Found the issue on why we are currently failing to fill tests for BPO1:

We are calling ForkCache.get with blob_schedule_max set but max_blob_gas_per_block set to None, so the temporary fork updates the vm.gas.BLOB_SCHEDULE_MAX but fork.MAX_BLOB_GAS_PER_BLOCK remains intact.

So we might need to calculate max_blob_gas_per_block on the fly if blob_schedule_max is passed (which depends on vm.gas.GAS_PER_BLOB which can also be modified so it might be not as straightforward).

[spencer-tb]

Found the issue on why we are currently failing to fill tests for BPO1:

Are we failing to fill? It should be running in CI with the tox change I think

[SamWilsn]

This PR changes MAX_BLOB_GAS_PER_BLOCK to be calculated dynamically:

https://github.com/ethereum/execution-specs/pull/1789/files#diff-819115894be2a859a2ee56a9c942931f9fe0ac1820cc6b8c8d8686079fc988e8R101

[marioevz]

LGTM, thanks for this needed change :D

Only downside is that the startup time for fill went up since we do the copy of the template regardless of BPO values:

===================================================== 48 passed, 10 warnings in 116.29s (0:01:56) ======================================================

from:

=========================================================== 48 passed, 10 warnings in 5.05s ============================================================

Added a P-High label to #1864 based on this.