-
Notifications
You must be signed in to change notification settings - Fork 462
feat(ci): overhaul fixture releases #2888
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: forks/amsterdam
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Can we rename |
| 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For And, same for |
||
| 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 | ||
| 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 | ||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||||||||||||||||
| mainnet: | ||||||||||||||||||||||||||||||||||||
| evm-type: eels | ||||||||||||||||||||||||||||||||||||
| fill-params: --until=BPO2 --generate-all-formats | ||||||||||||||||||||||||||||||||||||
| consensus: | ||||||||||||||||||||||||||||||||||||
| evm-type: consensus | ||||||||||||||||||||||||||||||||||||
| fill-params: --until=BPO4 | ||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we keep |
||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| 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 | ||||||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Oh thanks, that fixes something I pointed out in ethereum/go-ethereum#34972 (comment) :)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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!
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we still need
Suggested change
|
||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| bal: | ||||||||||||||||||||||||||||||||||||
| devnet: | ||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||||||||||||||||
| evm-type: eels | ||||||||||||||||||||||||||||||||||||
| fill-params: --fork=Amsterdam | ||||||||||||||||||||||||||||||||||||
| feature_only: true | ||||||||||||||||||||||||||||||||||||
| fill-params: --until=Amsterdam | ||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment.
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
fillflag here is initially surprizing, a booleansupports-xdistinstead ofx-distwould show clearer intent. But probably not worth the overhead.