Skip to content

chore: bump CI Node versions to 22 and 24#524

Open
lodekeeper wants to merge 2 commits intoChainSafe:masterfrom
lodekeeper:chore/update-node-versions-22-24
Open

chore: bump CI Node versions to 22 and 24#524
lodekeeper wants to merge 2 commits intoChainSafe:masterfrom
lodekeeper:chore/update-node-versions-22-24

Conversation

@lodekeeper
Copy link
Copy Markdown

Summary

  • Bump test matrix Node versions from [18, 20] to [22, 24] (Node 18 is EOL).
  • Bump benchmark workflow Node version from 20 to 24 to match the release workflow.

Closes #495

Test plan

  • CI green on test-node matrix for both Node 22 and Node 24
  • Benchmark workflow runs on Node 24

🤖 Generated with AI assistance

Node 18 is EOL; switch test matrix from [18, 20] to [22, 24] per @nflaig
in ChainSafe#495. Bump benchmark workflow to Node 24 to match the release workflow.

Closes ChainSafe#495

🤖 Generated with AI assistance
@lodekeeper lodekeeper requested a review from a team as a code owner April 21, 2026 12:26
nflaig
nflaig previously approved these changes Apr 21, 2026
@nflaig
Copy link
Copy Markdown
Member

nflaig commented Apr 21, 2026

I believe this might need some repo setting changes to be able to merge it cc @wemeetagain

@lodekeeper
Copy link
Copy Markdown
Author

Triaged before you get a chance to look at it — the blocker here isn't a repo setting. I checked:

  • branches/master/protection → 404 (no classic branch protection)
  • rulesets[] (no rulesets)
  • mergeable: true, mergeable_state: "blocked" is driven purely by the failing required status checks.

The actual blocker is two failing checks: Tests Node (22) and Tests Node (24). Both fail with the same symptom in packages/ssz spec-mainnet:

× mainnet/phase0/ssz_static/BeaconState/ssz_random case_0  16565ms
  → Test timed out in 15000ms.
× ... case_1 16023ms
× ... case_2, case_3, case_4 — all > 15000ms

Master's last Tests run (5ae98cf, Node 18/20) was green at 12:21 UTC, so it's specifically the big BeaconState/ssz_random cases crossing the 15s line on newer Node runners. Root cause looks like a latent bug in the test itself: test/spec/ssz_static.ts:53 calls vi.setConfig({testTimeout: 30 * 1000}) inside the it() callback, which doesn't take effect for the currently running test. Node 18/20 finished fast enough to stay under the default 15s; Node 22/24 pushed past it.

Fix is probably a one-liner — pass the timeout as the 3rd arg to it() or raise the default in vitest.base.unit.config.ts. Happy to push that here, or split it into its own PR first if you'd prefer to land the test fix independently before the Node bump. Let me know.

@nflaig
Copy link
Copy Markdown
Member

nflaig commented Apr 21, 2026

@lodekeeper please fix the tests as part of this PR

`vi.setConfig({testTimeout: 30 * 1000})` was called inside the `it()`
callback, which does not affect the currently-running test's timeout.
On Node 22/24 runners the mainnet `BeaconState/ssz_random` cases take
16-17s and exceed the default 15s, causing `Tests Node (22)` and
`Tests Node (24)` to fail. Move the timeout to the third argument of
`it()` so it actually applies.

🤖 Generated with AI assistance
@lodekeeper
Copy link
Copy Markdown
Author

Pushed 9342349 — moves the mainnet timeout to it()'s 3rd arg in packages/ssz/test/spec/ssz_static.ts. The vi.setConfig({testTimeout: 30_000}) call was inside the it() callback and therefore didn't apply to the currently-running test, which is why the default 15s was still in effect and the BeaconState/ssz_random cases (16–17s on Node 22/24 runners) were timing out. Also dropped the now-unused vi import. CI is running — I'll watch for Tests Node (22)/(24) to go green.

@lodekeeper
Copy link
Copy Markdown
Author

Heads up — the push landed action_required on Tests, Benchmark, and Labeler. The workflow runs need a maintainer to approve them before Node 22/24 will actually re-run against the fix. (This is probably the repo setting you were pointing at earlier — likely Settings → Actions → Require approval for all outside collaborators.) Could you (or @wemeetagain) kick off approval so the checks can run? I'll monitor once they start.

}
// Mainnet deals with big full states; timeout must be the 3rd arg to `it`
// because `vi.setConfig` inside the test callback doesn't affect it.
const timeout = preset === "mainnet" ? 30 * 1000 : undefined;
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.

@lodekeeper why do we need these changes in packages/ssz/test/spec/ssz_static.ts and how does this relate to using different node versions?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Link is CI — without this, Tests Node (22) and Tests Node (24) fail. BeaconState/ssz_random cases 0–4 on the mainnet preset take ~16–17s; before this patch they were running with the default 15s timeout and hitting Test timed out in 15000ms. on the newer Node runners.

The 30s bump was already intended here — the existing code called vi.setConfig({testTimeout: 30 * 1000}) at line 56. But that call is inside the it() callback, and vitest applies setConfig only to subsequent tests — not the one currently running. So the default 15s was still in effect for the test we were trying to extend. Node 18/20 runners happened to finish the big BeaconState cases just under 15s, which is why it was invisible on master; Node 22/24 cross the line and unmask the bug.

Moving the 30_000 to the 3rd arg of it(testId, fn, timeout) makes vitest honor it for the actual test, which is what the original comment wanted. Didn't want to leave the latent bug in place and let the Node bump fail spuriously — happy to split it to a separate PR if you'd rather land them independently.

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.

so the 18/20 node versions are faster than 22/24?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Not as a general statement — just for this exact CI workload (vitest + coverage + test:spec-static-mainnet) the old 18/20 matrix stayed under the default 15s, while 22/24 crossed it and exposed the latent timeout bug.

So I would phrase it as: the Node-matrix change made these cases slow enough on GitHub runners to start timing out; not that 18/20 are universally faster. Could be a mix of Node/V8/coverage overhead or runner-image differences. The important bit is that these mainnet BeaconState static cases legitimately need >15s here, and the existing vi.setConfig(...) wasn’t actually applying to the current test.

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.

hmm that doesn't make much sense, why would node 20/22 be slower?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fair pushback — I reached for runner noise too quickly. Grabbed actual per-case timings from CI logs; this is a real regression, not variance.

Same test, same code (mainnet/phase0/ssz_static/BeaconState/ssz_random), five cases each:

Run Node case_0 case_1 case_2 case_3 case_4
master 0feec86 (run 24725934536) 18 7611 ms 7110 ms 7321 ms 7412 ms 7213 ms
master 0feec86 (run 24725934536) 20 7148 ms 6845 ms 7066 ms 7131 ms 7254 ms
#524 b761812 (run 24722270228) 22 16565 ms 16023 ms 16045 ms 16042 ms 16091 ms
#524 b761812 (run 24722270228) 24 15827 ms 15615 ms 15496 ms 15441 ms 15498 ms

So ~7 s on 18/20 and ~15.5–16.5 s on 22/24 on the exact same workload. ~2.2× slowdown — not noise, and clearly an inflection at Node 22. Root cause isn't obvious from the CI logs alone; best guesses are V8 changes around large typed-array / SSZ tree walks or something in the @chainsafe/as-sha256 hot path interacting with 22+. Worth a separate investigation issue if we care about matching the 18/20 runtime, but I don't think we need to block this PR on it.

Orthogonal point: the vi.setConfig({testTimeout: 30_000}) inside the it() callback was already a latent bug regardless of the Node slowdown — it never applied to the test it was attached to, so the whole 30 s bump was a no-op. The 22/24 matrix just happens to be what exposed it. Moving the timeout to it()'s 3rd arg makes the original 30 s intent actually take effect.

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.

Remove the support for Node 18

2 participants