Tests: eth exec specs - #4064
Conversation
PR SummaryLow Risk Overview The suite pins upstream revisions, applies A new Reviewed by Cursor Bugbot for commit 7bd4bca. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f497215. Configure here.
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Adds a post-merge, informational Ethereum execution-specs / legacy transaction-RLP harness (workflow, pinned upstream checkouts, a Sei compatibility patch, a pytest selection plugin, and reporting scripts). No production code is touched and I found no blockers; the findings are about CI signal, a destructive cache reset, and a few maintainability gaps in the selection/skip policy.
Findings: 0 blocking | 10 non-blocking | 7 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] No tests cover the new Python helpers.
eest_plugin.partition()/shard_for_nodeid()andplan_chains.build_matrix()decide which vectors run at all, and because every job iscontinue-on-error, a bug that silently deselects a whole shard looks identical to a green run. A small pytest for partition disjointness/completeness and for the emitted matrix would be cheap insurance. - [suggestion]
patches/sei-compat.patchmutates upstream test bodies, not just the runner (removing@pytest.mark.pre_alloc_mutablefrom twoported_statictests, raisingmax_fee_per_gasintest_set_code_txs.py, addingpytest.skips intest_eip150_selfdestruct.py). Most of those hunks carry no rationale, so on the next revision bump there is no way to tell "required to run on a live chain" from "hides a Sei bug." A short comment per test-mutating hunk (or a note in the README) would make the rebase reviewable. - [suggestion] The push trigger still includes the author-marked
# TEMPbranchtests/exec-specs(.github/workflows/execution-specs.yml:8); remember to drop it before/right after merge so main does not carry a stray trigger. - 7 suggestion(s)/nit(s) flagged inline on specific lines.
|
|
||
| # This checkout is a derived cache. Restore upstream before applying the | ||
| # versioned Sei compatibility patch. | ||
| git -C "${EEST_DIR}" reset --quiet --hard HEAD |
There was a problem hiding this comment.
[suggestion] reset --hard + clean --force -d (plus checkout --force above) silently destroy tracked and untracked work in EEST_DIR. That is safe for the default .cache/execution-specs, but EEST_DIR is user-overridable and pointing it at a real execution-specs checkout is exactly what someone iterating on sei-compat.patch would do. The sibling script already models the safe behavior — install_legacy_tests.sh refuses when git status --short is non-empty. Either refuse a dirty checkout here too, or restrict the destructive restore to the case where EEST_DIR is the default cache path.
| SkipRule( | ||
| id="eip7623-admission", | ||
| reason="Known Sei EIP-7623 transaction-admission issue.", | ||
| matches=_eip7623_admission, |
There was a problem hiding this comment.
[suggestion] These rules skip vectors because of Sei defects ("Known Sei EIP-7623 transaction-admission issue", "Known Sei EIP-6780 repeated-SELFDESTRUCT issue"), which is different in kind from the surrounding rules that skip vectors unrepresentable on a persistent remote chain. Without a tracking issue on each SkipRule.id — and with the README listing no known-deviation table — these become permanent silent exclusions that nobody revisits once the bug is fixed. Adding the issue link to each reason (and a short deviations section in the README) keeps them auditable.
| EEST_MAX_FEE_PER_BLOB_GAS="${EEST_MAX_FEE_PER_BLOB_GAS:-1}" | ||
| EEST_EOA_FUND_AMOUNT_DEFAULT="${EEST_EOA_FUND_AMOUNT_DEFAULT:-100000000000000000}" | ||
| EEST_SKIP_CLEANUP="${EEST_SKIP_CLEANUP:-1}" | ||
| EEST_TOLERATE_MALFORMED_PENDING_TX="${EEST_TOLERATE_MALFORMED_PENDING_TX:-1}" |
There was a problem hiding this comment.
[suggestion] EEST_TOLERATE_MALFORMED_PENDING_TX defaults to 1, so every run silently accepts eth_getTransactionByHash responses that fail schema validation as long as blockHash is null. The narrowing to pending responses is reasonable, but defaulting it on means a future regression in Sei's pending-transaction response shape is tolerated by the very suite meant to catch spec deviations, with no record of what the current deviation is. Worth documenting the specific malformed field(s) alongside the flag.
|
|
||
| execution-specs: | ||
| name: Execution Specs (${{ matrix.name }}) | ||
| needs: [ plan, prepare-cluster ] |
There was a problem hiding this comment.
[suggestion] Because plan and prepare-cluster are both continue-on-error: true, a failure in either still lets this job start: all nine matrix entries claim ubuntu-large, reclaim disk, relocate the Docker data root, and then die at Download seid (or the matrix fails to evaluate at all when needs.plan.outputs.matrix is empty and fromJSON gets ''). Adding if: needs.prepare-cluster.result == 'success' && needs.plan.result == 'success' avoids burning nine large runners on a guaranteed failure.
| - name: Write run summary | ||
| run: | | ||
| python3 integration_test/exec_specs/scripts/summarize_reports.py \ | ||
| integration_test/exec_specs/reports/downloaded || true |
There was a problem hiding this comment.
[suggestion] summarize_reports.py deliberately returns 1 both when a report contains failures and when no reports were found, and || true (on top of continue-on-error: true on the job) throws both away. Since every job in this workflow is continue-on-error, the net effect is that "the harness broke and ran zero vectors" is indistinguishable from a healthy run. Keeping at least the no-reports case visible — drop || true here, or emit an explicit marker line into $GITHUB_STEP_SUMMARY — preserves that signal without making the workflow gate merges.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4064 +/- ##
==========================================
- Coverage 61.30% 60.23% -1.08%
==========================================
Files 2161 2055 -106
Lines 189066 176721 -12345
==========================================
- Hits 115909 106448 -9461
+ Misses 62290 60430 -1860
+ Partials 10867 9843 -1024
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
masih
left a comment
masih
left a comment
There was a problem hiding this comment.
Exciting to see these landing 🙌
I need your help to understand the rationale behind continue on error and some of the patch work since as is they make the CI look green regardless or can shadow root causes.
| + # rejection tests. Only valid transactions should cause the | ||
| + # remote pre-allocation layer to increase explicit funding. | ||
| + if tx.error is not None: | ||
| + continue |
There was a problem hiding this comment.
Hm this would avoid funding every tx with error? This means we then mask expected errors with "insufficient funds".
I am curious, why?
An alternative is to fund invalid transactions as normal except for INSUFFICIENT_ACCOUNT_FUNDS?
There was a problem hiding this comment.
You are correct, updated the tests.
| name: Ethereum Transaction and RLP Tests | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 20 | ||
| continue-on-error: true |
There was a problem hiding this comment.
This means CI is effectively always green? ditto in other places.
What was the rationale for this ?
There was a problem hiding this comment.
Yes, this was an experiment from before and latest runs were all passing so I didnt notice. Getting too AI complacent I am afraid. Updated.
| with: | ||
| pattern: exec-specs-*-${{ github.run_id }} | ||
| path: integration_test/exec_specs/reports/downloaded | ||
| - name: Write run summary |
There was a problem hiding this comment.
We should check expliclty the expected report name per chain, otherwise a partial failure gets swallowed up in a normal looking report?
|
|
||
| concurrency: | ||
| group: execution-specs-${{ github.ref }} | ||
| cancel-in-progress: false |
There was a problem hiding this comment.
Should this one remain false? I think it means if newer commits are pushed to branch, it doesn't cancel the older currently running ones. Setting to true can save runner time and cost.
There was a problem hiding this comment.
This runs only when PR's are merged to main. I guess it would be good to have this running per merged PR so that we would know which PR is the issue.
| @@ -0,0 +1,27 @@ | |||
| /** | |||
There was a problem hiding this comment.
I was wondering do we need to use the mix of typescript, bash and python for these tests, or is it better if everything could be written in go?
| )" | ||
| while read -r version; do | ||
| [[ -n "$version" ]] || continue | ||
| gh api --method DELETE "${base}/${package}/versions/${version}" || true |
There was a problem hiding this comment.
I would: remove || true; retry, attempt both packages, then fail if either operation failed.
3 participants
Summary
ethereum/testsTransactionTests and RLPTests through Sei's go-ethereum fork.Testing performed