Add semi-automated e2e major version upgrade test - #4019
Conversation
Add CI job and consistently tests major version upgrade against the latest head of latest release branch. This will be off the critical path of landing PRs; experimental only at this stage.
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4019 +/- ##
==========================================
- Coverage 61.29% 60.28% -1.01%
==========================================
Files 2158 2059 -99
Lines 188204 176609 -11595
==========================================
- Hits 115352 106467 -8885
+ Misses 62201 60431 -1770
+ Partials 10651 9711 -940
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
PR SummaryLow Risk Overview The new workflow ( When main does not already define a newer upgrade name than the release branch, the script synthesizes the next minor tag on the main worktree and runs Reviewed by Cursor Bugbot for commit 2ec2130. Bugbot is set up for automated code reviews on this repo. Configure here. |
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 2ec2130. Configure here.
| go-version: '1.25.6' | ||
|
|
||
| - name: Run coordinated release upgrade | ||
| run: bash .github/scripts/release-upgrade-test.sh |
There was a problem hiding this comment.
Missing Docker Hub login before image build
Medium Severity
The job builds the localnode image (which pulls golang:1.25.6-bookworm and ubuntu:24.04 from Docker Hub) without logging in first. GitHub-hosted runners share NAT IPs and often hit anonymous Docker Hub rate limits, so make build-docker-node can fail before the upgrade test runs. The integration-test workflow logs in to Docker Hub before the same image build.
Reviewed by Cursor Bugbot for commit 2ec2130. Configure here.
There was a problem hiding this comment.
Adds a nightly/dispatchable e2e test that boots a 4-validator localnet on the latest release binary, drives a real gov upgrade proposal, and swaps each validator to a main-built binary as it halts. The flow faithfully mirrors the existing integration_test/upgrade_module scripts and container conventions; no correctness blockers found, but a few CI-robustness issues (a height-skew assertion that is likely to flake, a fixed supervision deadline that ignores the configurable upgrade lead, and unhardened Docker Hub pulls) are worth addressing.
Findings: 0 blocking | 5 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] The workflow builds the localnode image with a plain
make build-docker-node(docker build), with no Docker Hub login, no buildx/GHCR layer cache, and no.github/scripts/docker-registry-retry.sh.integration-test.ymldoes all three for the same image. As written, every nightly run re-pullsgolang:1.25.6-bookwormandubuntu:24.04anonymously and re-runs the apt/foundry/nodesource layers, so the job is exposed to Docker Hub rate limits and pays several minutes of avoidable build time. - [suggestion]
build_binarymounts a linked git worktree, whose.gitis a file pointing at a host path ($REPO_ROOT/.git/worktrees/...) that is not mounted into the container. Every$(shell git ...)in the Makefile therefore fails inside the build container, soVERSION,TAG_VERSIONandCOMMITresolve to empty and both binaries are built with blank version ldflags. This differs frombuild-seid-in-localnode, where.gitis a real directory inside the mount. Not fatal (nothing inbuild/build-linuxneeds those values), butseid versionon the nodes and any version string in the collected logs will be empty, which weakens the diagnostics this test is meant to produce. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
| minimum="$current" | ||
| fi | ||
| done | ||
| ((maximum - minimum <= 3)) || |
There was a problem hiding this comment.
[suggestion] This synchronization check is likely to flake. minimum/maximum come from four sequential height calls, each of which is a docker exec plus a full seid status CLI invocation (~0.3-0.5 s apiece in practice). Localnet block time is ~300-400 ms (see the comment in integration_test/upgrade_module/scripts/proposal_target_height.sh), so the sampling window alone advances the chain by roughly 3-5 blocks between the first and last read — and the drift is systematic, since later nodes are always sampled later. A perfectly healthy cluster can therefore report max - min > 3 and die.
The check also can't distinguish sampling skew from real divergence. Consider either re-reading sei-node-0 after the loop and comparing the spread against that node's own advance over the same window, or dropping the check entirely — wait_for_node_height "$node" "$required_height" at line 523 already establishes that every validator reached a common height, which is the property this is trying to assert.
|
|
||
| upgrade_nodes_as_they_halt() { | ||
| log "Supervising each validator through the upgrade at height $TARGET_HEIGHT" | ||
| local deadline=$((SECONDS + 360)) |
There was a problem hiding this comment.
[suggestion] The 360 s deadline is fixed while UPGRADE_LEAD_SECONDS is configurable up to 300 s (validate_inputs). proposal_target_height.sh computes the target height as roughly "now + lead", and the proposal/vote/tally phase consumes only a fixed slice of that (voting period, not lead). With upgrade_lead_seconds: 300 the loop can enter with ~250 s of pure waiting still to go before any node even reaches the halt height, leaving ~110 s for four halts, installs, restarts and readiness — likely a spurious only N of 4 validators halted and upgraded within 360 seconds. Deriving it from the input (e.g. SECONDS + UPGRADE_LEAD_SECONDS + 360) makes the whole documented input range usable. The same literal 360 is repeated in the two die messages at lines 504 and 506.
| readonly MAIN_WORKTREE="$RUN_ROOT/main" | ||
| readonly RELEASE_WORKTREE="$RUN_ROOT/release" | ||
| readonly BUILD_ROOT="$RUN_ROOT/bin" | ||
| readonly ARTIFACT_ROOT="$REPO_ROOT/artifacts/release-upgrade" |
There was a problem hiding this comment.
[suggestion] ARTIFACT_ROOT writes node logs, container logs and revisions.txt into the repository working tree, and artifacts/ is not in .gitignore (only nested **/artifacts/ paths and specific subdirectories are). On CI this is harmless, but anyone running the script locally ends up with a tree full of untracked chain logs that can be swept into a commit. Either add /artifacts/ to .gitignore or place the artifact root under $RUN_ROOT and point the workflow's upload-artifact path at it.


Add an end-to-end upgrade test that starts a four-validator network on the latest release branch, schedules an on-chain upgrade, swaps each validator to the latest main binary, and verifies block production before and after.
Unlike existing tests that simulate versions with UPGRADE_VERSION_LIST, this validates compatibility using two real binaries and persistent chain state.