chore: update system_version to 3#1900
Conversation
Signed-off-by: Tomasz Bartos <tomasz.bartos@shielded.io>
This comment has been minimized.
This comment has been minimized.
|
I think even with |
Signed-off-by: Tomasz Bartos <tomasz.bartos@shielded.io>
Signed-off-by: Tomasz Bartos <tomasz.bartos@shielded.io>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
There was a problem hiding this comment.
PR Review Summary
PR: #1900 - chore: update system_version to 3
Plan: work package README
Reviewers: Post-Implementation Review · Validate · Strategic Review
Reports: Prior Feedback Triage · Code Review · Test Suite Review
Date: 2026-07-21
Executive Summary
The system_version 1 to 3 bump is correct and minimal: it closes the off-chain-Runtime-API-against-unmigrated-storage window that issue #1901 targets, at the correct minimum value, with a faithful one-byte metadata regeneration. No code defects on the authored surface; the prior blocking doubt is refuted on the mechanism (see Prior Feedback Triage). Remaining items are advisory.
Overall Rating: Comment Only
Prior Feedback Triage
Disposition of every prior comment on the PR, determined before independent analysis.
| # | Prior Comment | Author | Disposition | Reasoning |
|---|---|---|---|---|
| 1 | Metadata Check, npm audit, and sbom-scan failed on commit 07f256e | datadog-official[bot] | Superseded | Metadata Check passes on the current head 9ddb43d after the later fixup commits; the remaining red checks are PR-independent dependency-vulnerability scanners (see VF-1) |
| 2 | "same behavior but one block later" — the bump only defers the window | LGLO | Refuted | At system_version 3 the new code lands in :pending_code with a two-block delay, so :code stays on the old runtime through the set_code block and off-chain Runtime API calls read old code over old storage (consistent). The deferral pairs the first new-code block with the migration boundary; it does not reopen a state executed with new code over unmigrated storage. Full derivation against pinned polkadot-sdk (tag polkadot-stable2606) in the Code Review report |
| 3 | Codex usage limit notice | chatgpt-codex-connector[bot] | Refuted | Automated notice that the Codex reviewer did not run; carries no technical signal |
Code Review Findings
Details: Code Review report.
| # | Finding | Source | Severity | Disposition |
|---|---|---|---|---|
| CR-1 | Forward-coupling: the next upgrade shipping new pallet code under system_version 3 must bump spec_version, or the spec_version-gated on-runtime-upgrade migrations silently skip (the extrinsics-root trie also flips V0 to V1 at this bump) | runtime/src/lib.rs:282 | Low | Noted — record on the PR or issue #1901 |
No defect-class findings. The authored surface is the system_version 1 to 3 literal in both runtimes (runtime/src/lib.rs:282, partner-chains/demo/runtime/src/lib.rs:195), the two one-byte metadata regenerations, and the change fragment. 3 is the correct minimum value — 2 stays in the immediate-write branch and would be a silent no-op.
Test Review Findings
None. No authored test accompanies the change; the pending-code mechanism is covered by the canonical upstream frame_system test, so a project-level test carries low marginal value. See the Test Suite Review report.
Documentation Review
| # | Gap | Source | Severity | Disposition |
|---|---|---|---|---|
DR-1 |
PR body has no "Closes #1901" reference, so merging will not auto-close the tracked issue (the change fragment does reference it) | PR #1900 | Low | Suggested |
Validation Findings
| # | Check | Source | Severity | Disposition |
|---|---|---|---|---|
VF-1 |
npm audit and sbom-scan are red — PR-independent dependency-vulnerability scanners over surfaces this change does not touch (no JS, npm, or toolkit deps change); the scanners over the touched surfaces are green, and Metadata Check passes | PR checks | Warning | Noted — not attributable to this PR |
Branch Hygiene
| # | Item | Source | Severity | Disposition |
|---|---|---|---|---|
BH-1 |
Branch is behind main | 9ddb43d8 | Warning | Rebase before merge (re-runs Metadata Check against current main; squash merge is supported) |
Action Items
Should Address (Recommended):
- Record the spec_version forward-coupling note on the PR or issue #1901 (CR-1)
- Rebase onto current main before merge (BH-1)
Could Address (Suggested):
- Add a "Closes #1901" reference to the PR body so merge auto-closes the tracked issue (DR-1)
Posted by an automated review agent on behalf of @m2ux. The recommendation reflects an independent re-verification at head 9ddb43d8; the maintainers retain full discretion over disposition.
Overview
Bump
RuntimeVersion::system_versionfrom1to3in the Midnight runtime (and partner-chains demo runtime).This opts the runtime into the FRAME pending-code upgrade path from RFC-123 / paritytech/polkadot-sdk#6029, which fixes paritytech/polkadot-sdk#64: after a runtime upgrade (
set_code), off-chain Runtime API calls must not run the new Wasm against unmigrated storage.Without this bump, nodes keep the old upgrade behavior even on a modern polkadot-sdk.
🗹 TODO before merging
📌 Submission Checklist
git commit -s) for the DCO🧪 Testing Evidence
Please describe any additional testing aside from CI:
🔱 Fork Strategy
Links