Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 36 additions & 29 deletions .github/actions/build-evm-base/action.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2,48 +2,61 @@ name: 'Build EVM'
description: 'Resolves and builds the requested EVM binary by name'
inputs:
type:
description: 'Type of EVM binary to build'
description: 'Type of EVM binary to build (key in .github/configs/evm.yaml)'
required: true
default: 'main'
repo_override:
description: 'Override the repo from evm.yaml (e.g. ethereum/go-ethereum)'
required: false
default: ''
ref_override:
description: 'Override the ref/branch/commit from evm.yaml'
required: false
default: ''
outputs:
impl:
description: "Implementation of EVM binary to build"
value: ${{ steps.config-evm-reader.outputs.impl }}
repo:
description: "Repository to use to build the EVM binary"
value: ${{ steps.config-evm-reader.outputs.repo }}
value: ${{ steps.resolved.outputs.repo }}
ref:
description: "Reference to branch, commit, or tag to use to build the EVM binary"
value: ${{ steps.config-evm-reader.outputs.ref }}
value: ${{ steps.resolved.outputs.ref }}
evm-bin:
description: "Binary name of the evm tool to use"
value: ${{ steps.config-evm-impl-config-reader.outputs.evm-bin }}
value: ${{ steps.config-evm-reader.outputs.evm-bin }}
x-dist:
description: "Binary name of the evm tool to use"
value: ${{ steps.config-evm-impl-config-reader.outputs.x-dist }}
description: "Number of parallel pytest-xdist workers to use"
value: ${{ steps.config-evm-reader.outputs.x-dist }}
runs:
using: "composite"
steps:
- name: Get the selected EVM version from the .github/configs/evm.yaml
- name: Get the selected EVM configuration from .github/configs/evm.yaml
id: config-evm-reader
shell: bash
run: |
awk "/^${{ inputs.type }}:/{flag=1; next} /^[[:alnum:]]/{flag=0} flag" ./.github/configs/evm.yaml \
| sed 's/ //g' | sed 's/:/=/g' >> "$GITHUB_OUTPUT"
- name: Get the EVM implementation configuration from .github/configs/evm-impl-config.yaml
id: config-evm-impl-config-reader
- name: Apply repo/ref overrides
id: resolved
shell: bash
env:
DEFAULT_REPO: ${{ steps.config-evm-reader.outputs.repo }}
DEFAULT_REF: ${{ steps.config-evm-reader.outputs.ref }}
REPO_OVERRIDE: ${{ inputs.repo_override }}
REF_OVERRIDE: ${{ inputs.ref_override }}
run: |
awk "/^${{ steps.config-evm-reader.outputs.impl }}:/{flag=1; next} /^[[:alnum:]]/{flag=0} flag" ./.github/configs/evm-impl.yaml \
| sed 's/ //g' | sed 's/:/=/g' >> "$GITHUB_OUTPUT"
echo "repo=${REPO_OVERRIDE:-$DEFAULT_REPO}" >> "$GITHUB_OUTPUT"
echo "ref=${REF_OVERRIDE:-$DEFAULT_REF}" >> "$GITHUB_OUTPUT"
- name: Print Variables for the selected EVM type
shell: bash
run: |
echo "Type: ${{ inputs.type }}"
echo "Implementation: ${{ steps.config-evm-reader.outputs.impl }}"
echo "Repository: ${{ steps.config-evm-reader.outputs.repo }}"
echo "Reference: ${{ steps.config-evm-reader.outputs.ref }}"
echo "EVM Binary: ${{ steps.config-evm-impl-config-reader.outputs.evm-bin }}"
echo "X-Dist parameter: ${{ steps.config-evm-impl-config-reader.outputs.x-dist }}"
echo "Repository: ${{ steps.resolved.outputs.repo }}"
echo "Reference: ${{ steps.resolved.outputs.ref }}"
echo "EVM Binary: ${{ steps.config-evm-reader.outputs.evm-bin }}"
echo "X-Dist parameter: ${{ steps.config-evm-reader.outputs.x-dist }}"
- name: Skip building for EELS
if: steps.config-evm-reader.outputs.impl == 'eels'
shell: bash
Expand All @@ -52,25 +65,19 @@ runs:
if: steps.config-evm-reader.outputs.impl == 'geth'
uses: ./.github/actions/build-evm-client/geth
with:
repo: ${{ steps.config-evm-reader.outputs.repo }}
ref: ${{ steps.config-evm-reader.outputs.ref }}
repo: ${{ steps.resolved.outputs.repo }}
ref: ${{ steps.resolved.outputs.ref }}
- name: Build the EVM using EVMONE action
if: steps.config-evm-reader.outputs.impl == 'evmone'
uses: ./.github/actions/build-evm-client/evmone
with:
repo: ${{ steps.config-evm-reader.outputs.repo }}
ref: ${{ steps.config-evm-reader.outputs.ref }}
# `targets` in the evm.yaml must be an inline array to not interfere with `config-evm-reader`'s parsing
repo: ${{ steps.resolved.outputs.repo }}
ref: ${{ steps.resolved.outputs.ref }}
# `targets` in evm.yaml must be an inline array to not interfere with `config-evm-reader`'s parsing
targets: ${{ join(fromJSON(steps.config-evm-reader.outputs.targets), ' ') }}
- name: Build the EVM using Besu action
if: steps.config-evm-reader.outputs.impl == 'besu'
uses: ./.github/actions/build-evm-client/besu
with:
repo: ${{ steps.config-evm-reader.outputs.repo }}
ref: ${{ steps.config-evm-reader.outputs.ref }}
- name: Build the EVM using EthJS action
if: steps.config-evm-reader.outputs.impl == 'ethjs'
uses: ./.github/actions/build-evm-client/ethjs
with:
repo: ${{ steps.config-evm-reader.outputs.repo }}
ref: ${{ steps.config-evm-reader.outputs.ref }}
repo: ${{ steps.resolved.outputs.repo }}
ref: ${{ steps.resolved.outputs.ref }}
37 changes: 0 additions & 37 deletions .github/actions/build-evm-client/ethjs/action.yaml

This file was deleted.

13 changes: 12 additions & 1 deletion .github/actions/build-fixtures/action.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,15 @@ inputs:
split_label:
description: "Label for this fork-range split. Empty for unsplit builds."
default: ""
evm:
description: "Override the evm impl. Defaults to the feature's evm-type."
default: ""
evm_repo:
description: "Override the t8n tool repo (e.g. ethereum/go-ethereum)"
default: ""
evm_ref:
description: "Override the t8n tool branch / tag / commit"
default: ""
runs:
using: "composite"
steps:
Expand All @@ -32,7 +41,9 @@ runs:
- uses: ./.github/actions/build-evm-base
id: evm-builder
with:
type: ${{ steps.properties.outputs.evm-type }}
type: ${{ inputs.evm != '' && inputs.evm || steps.properties.outputs.evm-type }}
repo_override: ${{ inputs.evm_repo }}
ref_override: ${{ inputs.evm_ref }}
- name: Install pigz for parallel tarball compression
if: inputs.split_label == ''
shell: bash
Expand Down
15 changes: 0 additions & 15 deletions .github/configs/evm-impl.yaml

This file was deleted.

32 changes: 29 additions & 3 deletions .github/configs/evm.yaml
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The fact that we configure a fill flag here is initially surprizing, a boolean supports-xdist instead of x-dist would show clearer intent. But probably not worth the overhead.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: Can we rename x-dist to xdist? I've never seen it hyphenated anywhere else )

Original file line number Diff line number Diff line change
@@ -1,13 +1,39 @@
# Client aliases
benchmark:
impl: geth
repo: ethereum/go-ethereum
ref: master
evm-bin: evm
x-dist: auto
consensus:
impl: eels
repo: null
ref: null
evm-bin: null
x-dist: auto

Comment on lines +1 to +14
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For consensus this seems like an unnecessary redirct? I'd skip this obfuscation and just use eels, respectively geth in .github/configs/feature.yaml.

And, same for benchmark, assuming we only have one type of benchmark feature (as is the case in this PR - it removes benchmark_fast).

eels:
impl: eels
repo: null
ref: null
static:
evm-bin: null
x-dist: auto
evmone:
impl: evmone
repo: ethereum/evmone
ref: master
targets: ["evmone-t8n"]
benchmark:
evm-bin: evmone-t8n
x-dist: auto
geth:
impl: geth
repo: ethereum/go-ethereum
ref: master
ref: master
evm-bin: evm
x-dist: auto
besu:
impl: besu
repo: hyperledger/besu
ref: main
evm-bin: evmtool
x-dist: 0
19 changes: 6 additions & 13 deletions .github/configs/feature.yaml
Original file line number Diff line number Diff line change
@@ -1,19 +1,12 @@
# Unless filling for special features, all features should fill for previous forks (starting from Frontier) too
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
# Unless filling for special features, all features should fill for previous forks (starting from Frontier) too
# Release feature definitions consumed by the release_fixtures workflow.
#
# Each top-level key is either:
# 1. A literal feature name used verbatim in the release tag, e.g.
# `consensus` -> `tests-consensus@vX.Y.Z`, `benchmark` -> `tests-benchmark@vX.Y.Z`.
# 2. The shared `devnet` entry, which is resolved by `-devnet` suffix:
# any workflow input feature ending in `-devnet` (e.g. `bal-devnet`,
# `glamsterdam-devnet`) maps here while keeping its `<feat>-devnet`
# name in the tag (`tests-<feat>-devnet@vX.Y.Z`). The devnet index
# is encoded in the version (X), not in the feature name -- so
# `glamsterdam-devnet-5` (branch) releases as feature
# `glamsterdam-devnet` at version `v5.0.0`, yielding the tag
# `tests-glamsterdam-devnet@v5.0.0`. This means `feature.yaml` does
# not need to be updated for new devnets.
#
# Unless filling for special features, all features should fill for previous forks (starting from Frontier) too

mainnet:
evm-type: eels
fill-params: --until=BPO2 --generate-all-formats
consensus:
evm-type: consensus
fill-params: --until=BPO4
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we keep --generate-all-formats please? :)


benchmark:
evm-type: benchmark
fill-params: --fork=Osaka --generate-all-formats --gas-benchmark-values 1,5,10,30,60,100,150 ./tests/benchmark/compute --maxprocesses=30 --dist=worksteal
feature_only: true

benchmark_fast:
evm-type: benchmark
fill-params: --fork=Osaka --generate-all-formats --gas-benchmark-values 100 ./tests/benchmark/compute
feature_only: true
fill-params: --fork=Amsterdam --gas-benchmark-values 1,10,30,60,100,150 ./tests/benchmark
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.

We're using geth to fill the tests, but as we discussed, I don't think it has t8n tool support yet.

I did a partial review focusing mostly on benchmarking, and I don't see any other issues from my side.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Geth should have t8n support after this was merged, curious if there any issues

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.

Geth should have t8n support after this was merged, curious if there any issues

Oh thanks, that fixes something I pointed out in ethereum/go-ethereum#34972 (comment) :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we append 200M here to reflect the minimum glammie target value from the interop? Or other values @LouisTsai-Csie

Can also be a follow-up!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we still need --generate-all-formats here. The changes to packages/testing/src/execution_testing/specs/benchmark.py only remove blockchain_test_engine fixture formats for benchmark tests.

Suggested change
fill-params: --fork=Amsterdam --gas-benchmark-values 1,10,30,60,100,150 ./tests/benchmark
fill-params: --fork=Amsterdam --generate-all-formats --gas-benchmark-values 1,10,30,60,100,150 ./tests/benchmark


bal:
devnet:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
devnet:
# Shared entry for all `<feat>-devnet` releases; matched by `-devnet` suffix.
devnet:

evm-type: eels
fill-params: --fork=Amsterdam
feature_only: true
fill-params: --until=Amsterdam
5 changes: 1 addition & 4 deletions .github/configs/fork-ranges.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,7 @@
until: Prague
- label: osaka
from: Osaka
until: Osaka
- label: bpo
from: BPO1
until: BPO2
until: BPO5
- label: amsterdam
from: Amsterdam
until: Amsterdam
15 changes: 12 additions & 3 deletions .github/scripts/generate_build_matrix.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,9 @@
"Osaka",
"BPO1",
"BPO2",
"BPO3",
"BPO4",
"BPO5",
"Amsterdam",
]

Expand Down Expand Up @@ -142,14 +145,20 @@ def main() -> None:
fork_ranges = load_config(FORK_RANGES_CONFIG) or []
name = sys.argv[1]

if name not in config or not isinstance(config[name], dict):
# `<feat>-devnet` releases (e.g. bal-devnet) share the `devnet` entry,
# while keeping their friendly name in the matrix and artifact outputs.
lookup = (
"devnet" if name.endswith("-devnet") and "devnet" in config else name
)

if lookup not in config or not isinstance(config[lookup], dict):
print(
f"Error: feature '{name}' not found in {FEATURE_CONFIG}.",
f"Error: feature '{lookup}' not found in {FEATURE_CONFIG}.",
file=sys.stderr,
)
sys.exit(1)

build, labels = build_matrix(config[name], name, fork_ranges)
build, labels = build_matrix(config[lookup], name, fork_ranges)

print(f"build_matrix={json.dumps(build)}")
print(f"feature_name={name}")
Expand Down
10 changes: 8 additions & 2 deletions .github/scripts/get_release_props.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,14 @@ def get_release_props(release: str) -> None:
with open(RELEASE_PROPS_FILE) as f:
data = yaml.safe_load(f)
if release not in data:
print(f"Error: Release {release} not found in {RELEASE_PROPS_FILE}.")
sys.exit(1)
# `<feat>-devnet` releases (e.g. bal-devnet) share the `devnet` entry.
if release.endswith("-devnet") and "devnet" in data:
release = "devnet"
else:
print(
f"Error: Release {release} not found in {RELEASE_PROPS_FILE}."
)
sys.exit(1)
print("\n".join(f"{key}={value}" for key, value in data[release].items()))


Expand Down
18 changes: 9 additions & 9 deletions .github/scripts/tests/test_release_scripts.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,12 @@ class TestGenerateBuildMatrix:

def test_split_feature_produces_entries_per_range(self):
"""Verify a split feature expands into one entry per range."""
result = run_script(BUILD_MATRIX_SCRIPT, "mainnet")
result = run_script(BUILD_MATRIX_SCRIPT, "consensus")
assert result.returncode == 0
out = parse_matrix_output(result.stdout)
matrix = json.loads(out["build_matrix"])
assert len(matrix) > 1
assert out["feature_name"] == "mainnet"
assert out["feature_name"] == "consensus"
assert out["combine_labels"] != ""
labels = [e["label"] for e in matrix]
assert all(lbl != "" for lbl in labels)
Expand All @@ -68,15 +68,15 @@ def test_unsplit_feature_produces_single_entry(self):
assert matrix[0]["from_fork"] == ""
assert matrix[0]["until_fork"] == ""

def test_feature_only_can_be_requested_explicitly(self):
"""Verify feature_only entries work when named directly."""
result = run_script(BUILD_MATRIX_SCRIPT, "bal")
def test_devnet_name_resolves_to_shared_feature(self):
"""Verify a <feat>-devnet name resolves to the devnet feature."""
result = run_script(BUILD_MATRIX_SCRIPT, "bal-devnet")
assert result.returncode == 0
out = parse_matrix_output(result.stdout)
matrix = json.loads(out["build_matrix"])
assert len(matrix) == 1
assert matrix[0]["feature"] == "bal"
assert out["combine_labels"] == ""
assert out["feature_name"] == "bal-devnet"
# Entries keep the friendly name, not the shared "devnet" key.
assert all(e["feature"] == "bal-devnet" for e in matrix)

def test_unknown_feature_fails(self):
"""Verify error exit for unknown feature name."""
Expand All @@ -92,7 +92,7 @@ def test_no_args_fails(self):

def test_output_is_valid_github_actions_format(self):
"""Verify output lines are key=value for GITHUB_OUTPUT."""
result = run_script(BUILD_MATRIX_SCRIPT, "mainnet")
result = run_script(BUILD_MATRIX_SCRIPT, "consensus")
assert result.returncode == 0
lines = result.stdout.strip().splitlines()
assert len(lines) == 3
Expand Down
15 changes: 0 additions & 15 deletions .github/workflows/benchmark.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -83,18 +83,3 @@ jobs:

# TODO: Add execute remote tests with --gas-benchmark-values
# TODO: Add execute remote tests with --fixed-opcode-count

build-artifact:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is quite slow and we should remove it if no-one is using the artifacts, but this has been a nice benchmark release sanity check. Also for hive startup and genesis file production.

Perhaps we can come up with a faster replacement (lower gas limit and not artifact production)?

name: Build Benchmark Fixture Artifact
needs: [sanity-checks] # TODO: Add execute remote jobs when implemented
if: github.event_name == 'push'
runs-on: [self-hosted-ghr, size-gigachungus-x64]
timeout-minutes: 720
steps:
- uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
with:
submodules: true

- uses: ./.github/actions/build-fixtures
with:
release_name: benchmark_fast
Loading
Loading