chore(ci): move the solidity toolchain off end-of-life Node to 22 LTS - #4201
Conversation
📝 WalkthroughWalkthroughNode.js 18 references are replaced with Node.js 22 versions across Solidity package requirements, Docker images, contract CI and deployment workflows, NPM publishing workflows, and Solidity documentation generation. ChangesNode.js runtime alignment
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
e24f216 to
d3b9cd7
Compare
d3b9cd7 to
59854a2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
solidity/ecdsa/Dockerfile (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep both Docker runtimes aligned with the pinned CI version.
Both Dockerfiles use floating
node:22-alpinetags while CI is pinned to Node.js 22.23.1.
solidity/ecdsa/Dockerfile#L1-L1: pin the image to the tested patch or digest.solidity/random-beacon/Dockerfile#L1-L1: pin the image to the tested patch or digest.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@solidity/ecdsa/Dockerfile` at line 1, Pin the base image in solidity/ecdsa/Dockerfile at lines 1-1 to the tested Node.js 22.23.1 patch or a corresponding digest, and apply the same pin in solidity/random-beacon/Dockerfile at lines 1-1 so both Docker runtimes match CI.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@solidity/ecdsa/Dockerfile`:
- Line 1: Configure both Dockerfiles to run Hardhat as a non-root user: in
solidity/ecdsa/Dockerfile and solidity/random-beacon/Dockerfile, create or use a
non-root user, grant it ownership of WORK_DIR, and set it as the runtime USER so
the npx hardhat entrypoints execute without root privileges.
---
Nitpick comments:
In `@solidity/ecdsa/Dockerfile`:
- Line 1: Pin the base image in solidity/ecdsa/Dockerfile at lines 1-1 to the
tested Node.js 22.23.1 patch or a corresponding digest, and apply the same pin
in solidity/random-beacon/Dockerfile at lines 1-1 so both Docker runtimes match
CI.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6f8b4d64-caf2-4dca-a9ce-618015e40ded
📒 Files selected for processing (9)
.github/workflows/contracts-ecdsa.yml.github/workflows/contracts-random-beacon.yml.github/workflows/npm-ecdsa.yml.github/workflows/npm-random-beacon.yml.github/workflows/reusable-solidity-docs.ymlsolidity/ecdsa/Dockerfilesolidity/ecdsa/package.jsonsolidity/random-beacon/Dockerfilesolidity/random-beacon/package.json
| @@ -1,4 +1,4 @@ | |||
| FROM node:18-alpine | |||
| FROM node:22-alpine | |||
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Run both Hardhat containers as non-root.
Both images default to root, and their npx hardhat entrypoints therefore execute with unnecessary privileges.
solidity/ecdsa/Dockerfile#L1-L1: add a non-rootUSERand grant it ownership ofWORK_DIR.solidity/random-beacon/Dockerfile#L1-L1: apply the same non-root runtime configuration.
🧰 Tools
🪛 Trivy (0.72.0)
[error] 1-1: Image user should not be 'root'
Specify at least 1 USER command in Dockerfile with non-root user as argument
Rule: DS-0002
(IaC/Dockerfile)
[error] 1-1: Image user should not be 'root'
Specify at least 1 USER command in Dockerfile with non-root user as argument
Rule: DS-0002
(IaC/Dockerfile)
📍 Affects 2 files
solidity/ecdsa/Dockerfile#L1-L1(this comment)solidity/random-beacon/Dockerfile#L1-L1
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@solidity/ecdsa/Dockerfile` at line 1, Configure both Dockerfiles to run
Hardhat as a non-root user: in solidity/ecdsa/Dockerfile and
solidity/random-beacon/Dockerfile, create or use a non-root user, grant it
ownership of WORK_DIR, and set it as the runtime USER so the npx hardhat
entrypoints execute without root privileges.
Source: Linters/SAST tools
All five workflows that build and test the contracts pin Node 18.15.0,
which reached end of life on 2025-04-30 and receives no further security
patches. `engines.node` in both solidity manifests and the two contract
Dockerfiles say the same.
Target is Node 22 (Jod), EOL 2027-04-30. Node 24 has more than twice the
remaining support and was tried first, but Hardhat's undici call is
incompatible with Node 24 until ~2.20, and @defi-wonderland/smock breaks on
hardhat >= 2.20 -- the two windows do not overlap. Node 22 is the ceiling
in both repos until smock is replaced, and smock is archived upstream
(wonderland-archive/smock).
No dependency changes. Verified against this branch, solidity/random-beacon:
Node v18.20.8 168 passing, 12 failing
Node v22.23.1 168 passing, 12 failing
The 12 are identical and pre-existing on both -- every one is
"No deployment found for: T", a missing external deployment artifact in a
local checkout, unrelated to the runtime. hardhat 2.10.0 and
@defi-wonderland/smock 2.0.7, the packages most likely to break on a newer
runtime, are clean.
infrastructure/ is deliberately untouched. Its Dockerfiles are not built
by CI -- `client.yml` lists `infrastructure/**` under `paths-ignore`, and
the only build contexts in the workflows are `.`, `./solidity/ecdsa` and
`./solidity/random-beacon`. The node:11 init container there wants a
decision about whether it is still deployed, not a bump to a file nothing
builds.
Stacked on #4109 -- carries that branch's commits and should merge after
it. #4109 changes no Node-runtime surface, so there is no overlap.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
59854a2 to
d32ecc8
Compare
|
Thanks — taking one of the two. Pinned both Dockerfiles to Not taking the non-root change in this PR. The finding is legitimate, but it
That is a real change needing a real image build to validate, and it would put a Context for whoever picks it up: these two images are ephemeral build tooling |
The end of the chain, and the reason for it. Five workflows, two Dockerfiles and two `engines` fields go from Node 18.15.0 -- end of life since 2025-04-30 -- straight to 24.11.1, the current LTS, supported until 2028-04-30. This was not reachable before. Node 24 needs hardhat >= 2.26, because below it Hardhat's solc download fails with HH502: npm's undici and Node 24's bundled undici share a global-dispatcher symbol, so the download reaches Node's internal dispatcher, which rejects `maxRedirections`. hardhat >= 2.21 in turn broke @defi-wonderland/smock, and no smock version ever reached 2.26. The dependency was archived, so no version ever would. That is why #4203 removed it and #4204 raised hardhat before this commit could exist. The failure that started it, now passing on the same machine and the same runtime -- cold compiler cache, which is what a CI runner always has: before Error HH502: Couldn't download compiler versions list. after Compiled 75 Solidity files successfully Verified, whole suite, both packages, on Node 24.11.1: ecdsa 673 passing / 44 pending / 0 failing random-beacon 955 passing / 0 pending / 0 failing Docker base images are pinned to `node:24.11.1-alpine` rather than the floating `node:24-alpine`, matching the exact version CI pins, so the two runtimes cannot drift apart. This supersedes #4201, which moved the same nine files from 18 to 22 while 24 was still out of reach. If that lands first this needs a one-token rebase per site; if this chain lands, #4201 can be closed. BREAKING CHANGE: the solidity packages now require Node >= 24.0.0. Contributors on Node 18 or 20 will need to upgrade. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
piotr-roslaniec
left a comment
There was a problem hiding this comment.
Self-approve as repo admin to advance the smock/hardhat/Node-24 chain. The 9-file diff is runtime pins only (Node 18 -> 22.23.1) and is consistent with the author's description. CI is green. Note: #4205 supersedes this PR per the author's own body in #4205; the conflict between 4201 and 4205 (same 9 files, 22.23.1 vs 24.11.1) will be resolved in the next decision.
…ges (#4202) **Dependencies only. No test file is touched.** First step in removing `@defi-wonderland/smock`, which upstream has archived with this notice: >⚠️ **DEPRECATED – DO NOT USE** > > This repository is no longer maintained and is **deprecated**. > > It may contain **outdated, insecure, or vulnerable code** and should **not** > be used in production or as a dependency in any project. > > The repository is retained solely for historical reference. No support, > updates, or security patches will be provided. npm carries the matching deprecation, the repo is archived, and the last push was 2025-05-20. That is the reason to remove it. The toolchain ceiling below is not the motivation — it is what makes removal unavoidable rather than schedulable, since no smock version reaches the hardhat that Node 24 needs. Independent of #4201 — different regions of the same `package.json` files, and the two merge cleanly in either order (verified). ## Three coupled bumps, in both packages ``` hardhat ^2.10.0 -> 2.19.5 @defi-wonderland/smock ^2.0.7 -> 2.3.4 hardhat-gas-reporter ^1.0.8 -> ^2.3.0 ``` None of them works alone, which is why they land together. **hardhat first.** The contract-backed mock that will replace smock configures itself with transactions, and a transaction mines a block. Keeping that from moving the chain clock needs `allowBlocksWithSameTimestamp` — and that option **does not exist in 2.10.0**. Checked against the published tarballs: absent in 2.10.0, present by 2.14.0. 2.19.5 is the last version smock still works on, so it is the one version with both properties. **smock second, because 2.0.7 does not survive that bump.** On hardhat 2.19.5 it dies before any test runs: ``` TypeError: vm.on is not a function at new ObservableVM (@defi-wonderland/smock/src/observable-vm.ts:19:35) at Sandbox.create (@defi-wonderland/smock/src/sandbox.ts:82:12) ``` 2.3.4 is the version that works there, and it is **pinned exactly rather than left as a range**. `^2.0.7` resolves to **2.4.1** today, so the lockfile was one regeneration away from silently jumping four minor versions into an archived package. 2.4.1 is deliberately not the target. It advertises `hardhat >=2.21.0` support and still fails — differently — on a real plugin stack: ``` TypeError: provider.init is not a function at getHardhatBaseProvider (@defi-wonderland/smock/src/utils/hardhat.ts:32:18) at Sandbox.create (@defi-wonderland/smock/src/sandbox.ts:117:55) ``` That is what makes the smock removal unavoidable rather than optional: there is no smock version that reaches hardhat 2.26+, and hardhat 2.26+ is what Node 24 requires. **hardhat-gas-reporter third.** 1.x bundles `eth-gas-reporter`, which the mocha inside hardhat 2.19.5 refuses to load on Node 22 — `ERR_MOCHA_INVALID_REPORTER`, raised before a single test executes. Only `random-beacon` hits it, because only `random-beacon` enables the reporter, but both packages move together to keep them aligned. This matters now rather than later because #4201 puts CI on Node 22. ## Verified Whole suite, each package, on both runtimes: | | before (hh 2.10, smock 2.0.7) | after | | --- | --- | --- | | `random-beacon` | 926 passing / 6 pending / 0 failing | **926 / 6 / 0** | | `ecdsa` | 650 passing / 44 pending / 0 failing | **650 / 44 / 0** | Identical on Node 18.20.8 and Node 22.23.1. ## Where this is going 1. this PR — dependencies only 2. replace smock with a contract-backed mock (`MockContract.sol` + `test/helpers/mock.ts`), portable from threshold-network/tbtc-v2#1062 3. hardhat 2.19.5 -> 2.29 and Node 22 -> 24, which only becomes possible once smock is gone Roughly 26 `smock.fake` sites across 20 files, most of them in `ecdsa`. The one `smock.mock` call, at `WalletRegistry.RandomBeacon.test.ts:236`, only reads `.address` and never touches `setVariable`, so it is a plain factory deploy and needs no mock at all. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Added comprehensive programmable contract-mocking coverage for return values, reverts, call recording, argument matching, resets, static calls, and transaction behavior. * Enabled previously skipped slashing failure scenarios and expanded event assertions. * Improved timestamp-sensitive test reliability and gas measurement consistency. * Standardized mock behavior and assertions across ECDSA and Random Beacon test suites. * **Chores** * Added support for Yarn local-state files to be excluded from version control. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
**Stacked on #4203.** The last dependency step: hardhat 2.19.5 → **2.29.0** in both packages. smock capped this at 2.19.5. It is gone as of #4203, so the ceiling lifts — and 2.26+ is what Node 24 requires. Below it, Hardhat's solc download fails on Node 24 with `HH502`, because npm's undici and Node 24's bundled undici share a global-dispatcher symbol. ## Verified Whole suite, both packages, both runtimes: | | Node 22 | Node 24 | | --- | --- | --- | | `ecdsa` | 673 / 44 pending / 0 | **673 / 44 / 0** | | `random-beacon` | 955 / 0 pending / 0 | **955 / 0 / 0** | Identical to the pre-bump results, so nothing regressed — and it is safe to land before or after the Node bump in #4201, in either order. ## The other thing 2.29 brings | | before | after | | --- | --- | --- | | `random-beacon` | 3m | **23s** | | `ecdsa` | 2m | **31s** | That is EDR, the Rust execution engine, backported into the 2.x line. ## Two tests needed adjusting `challengeDkgResult` has an inline gas check, and two tests deliberately underfund the call to trigger it. hardhat 2.29 defaults to a newer hardfork, where EIP-7623's intrinsic calldata floor is enforced *before* execution: ``` ProviderError: Transaction requires gas floor of 161720 but got limit of 100000 ``` The node rejected the transaction outright, so the contract's own check never ran — the tests had stopped testing the thing they name. The limits move to just above each floor (`100000 → 170000`, `200000 → 220000`), still far short of what the call needs to complete, so both revert inside the contract as intended. The alternative was pinning the hardfork back to 2.19.5's default. Rejected: mainnet is past it, and running the suite against an older EVM than production is a worse trade than adjusting two gas limits. ## After this Node 22 → 24 in the five workflows, two Dockerfiles and two `engines` fields — a small follow-up, and the reason the whole chain existed. #4201 already moved those from 18 to 22; this makes 24 reachable for the first time. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Updated the Hardhat development tooling to version 2.29.0 for the ECDSA and random beacon packages. * **Tests** * Updated gas-limit scenarios to accurately validate insufficient-gas behavior under current transaction gas rules. * Clarified test coverage for contract-level gas checks. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Nine files, runtime pins only. No dependency changes.
This previously carried all of #4109's commits and showed a 257-file diff — enough
that CodeRabbit declined to review it, and enough that merging it would have landed
the security release as a side effect. It has been rebased onto
mainand nowcontains a single commit of its own.
Rebasing rather than retargeting at #4109 was the right move on two counts: that
branch lives on a fork (
lionakhnazarov/keep-core), so GitHub cannot base a PR onit; and there is no dependency to preserve — #4201's nine files are disjoint from
all 249 of #4109's. Merging in either order is conflict-free, verified by
simulating both.
The problem
All five workflows that build and test the contracts pin Node 18.15.0, which
reached end of life 2025-04-30 and receives no further security patches:
contracts-ecdsa,contracts-random-beacon,npm-ecdsa,npm-random-beacon,reusable-solidity-docsengines.nodein both solidity manifests and both contract Dockerfiles say thesame.
Why 22 and not 24
On support alone 24 is the better target — current LTS, EOL 2028-04-30, against
22's 2027-04-30. This repo cannot reach it yet.
Both solidity packages pin
hardhat 2.10.0, which resolvesundici 5.19.1.Hardhat 2.x on the undici 5 line cannot download a compiler on Node 24: npm's
undici and Node 24's bundled undici share a global-dispatcher symbol, so
Hardhat's solc download reaches Node's internal dispatcher, which rejects
maxRedirections. Measured on this branch,solidity/random-beacon, coldcompiler cache:
A local run does not catch this. HH502 is on the download path, so once a
compiler is cached the run succeeds. CI runners always start cold, which is
exactly where it fires. An earlier revision of this description reported a clean
Node 24 run and concluded 24 was safe; that run had a warm cache.
The fix is
hardhat >= 2.26, and that is blocked on something larger:@defi-wonderland/smock(^2.0.7, resolved 2.0.7 in both packages) breaks athardhat 2.20.0 —
Sandbox.create— and is archived upstream, so noforward version is coming
Disjoint ranges, so smock has to go before Node 24 is reachable. That is the
same chain tbtc-v2 worked through in threshold-network/tbtc-v2#1062 → #1063 →
#1064 — a contract-backed mock replacing smock, then hardhat 2.29 and Node 24
bumped together in one commit — and it is why that repo is on 24 and this one is
not. It has already paid for it.
So this is the interim step: off an end-of-life runtime now, onto the highest
version the current dependencies actually support. 24 follows the smock removal.
Worth a separate issue:
@defi-wonderland/smockis pinned as^2.0.7. Thelockfile holds 2.0.7 today, but the range floats to 2.4.1 on any regeneration —
silently, into an archived package. Same trap tbtc-v2 hit.
Verified
No dependency changes; runtime pins only.
solidity/random-beacon, full suite on this branch, via the package's owntestscript (hardhat typechain && hardhat check-accounts-count && USE_EXTERNAL_DEPLOY=true TEST_USE_STUBS_BEACON=true hardhat test):Identical. Node 24 is deliberately not quoted as passing — see above.
An earlier revision of this description quoted
168 passing, 12 failingwiththe 12 being
No deployment found for: T. That does not reproduce: run throughthe package's own script, which sets
USE_EXTERNAL_DEPLOY=true, the externaldeployment resolves and the suite is clean on both runtimes.
infrastructure/deliberately untouchedIt contains the worst offender in the repo (a
node:11init container, EOL2019, carrying 7 open Dependabot alerts), but nothing there is built by CI:
client.ymllistsinfrastructure/**underpaths-ignore.,./solidity/ecdsa,./solidity/random-beacongcr.io/keep-dev-fe24/initcontainer-provision-keep-client-ethereum, and isreferenced only by
keep-devandkeep-test—keep-prdhas no keep-clientmanifests at all
package-lock.jsonhas not changed since 2023-04-17So those alerts are against a build this repo never performs. That wants a
decision about whether the image is still deployed — delete it, or move its build
into CI so the alerts mean something — rather than a version bump to a file
nothing builds. Same for
docker/ethereum/geth-node(geth v1.9.6, 2019) anddocker/ethereum/dashboard-node(untaggedubuntu), both untouched since 2019and referenced by nothing.
Summary by CodeRabbit