Skip to content

DO NOT MERGE PR 10857 needs to be fully in production first feat(registry): Enable blank replica_version_id for engines. - #10888

Open
daniel-wong-dfinity-org-twin wants to merge 1 commit into
masterfrom
enable-standard-engine-replica-version-daniel-wong
Open

DO NOT MERGE PR 10857 needs to be fully in production first feat(registry): Enable blank replica_version_id for engines.#10888
daniel-wong-dfinity-org-twin wants to merge 1 commit into
masterfrom
enable-standard-engine-replica-version-daniel-wong

Conversation

@daniel-wong-dfinity-org-twin

@daniel-wong-dfinity-org-twin daniel-wong-dfinity-org-twin commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

This is "just" a flag flip, but it suddenly makes a whole bunch of code active.

👈 Previous PR

@daniel-wong-dfinity-org-twin
daniel-wong-dfinity-org-twin requested a review from a team as a code owner July 23, 2026 16:17
github-actions[bot]

This comment was marked as resolved.

@zeropath-ai

zeropath-ai Bot commented Jul 23, 2026

Copy link
Copy Markdown

No security or compliance issues detected. Reviewed everything up to 486a25f.

Security Overview
Detected Code Changes
Change Type Relevant files
Bug Fix ► rs/registry/canister/src/flags.rs
    Modify default for IS_BLANK_REPLICA_VERSION_ID_FOR_CLOUD_ENGINES_ENABLED from false to true
Enhancement ► rs/registry/canister/unreleased_changelog.md
    Add note about Cloud Engines allowed to have blank replica_version_id in SubnetRecord

@daniel-wong-dfinity-org-twin daniel-wong-dfinity-org-twin changed the title Enable blank replica_version_id for engines. DO NOT MERGE we need PR 10857 to be fully in production first feat(registry): Enable blank replica_version_id for engines. Jul 23, 2026
@daniel-wong-dfinity-org-twin daniel-wong-dfinity-org-twin changed the title DO NOT MERGE we need PR 10857 to be fully in production first feat(registry): Enable blank replica_version_id for engines. DO NOT MERGE PR 10857 needs to be fully in production first feat(registry): Enable blank replica_version_id for engines. Jul 23, 2026
@daniel-wong-dfinity-org-twin
daniel-wong-dfinity-org-twin dismissed github-actions[bot]’s stale review July 23, 2026 16:21
  1. Done.
  2. No, this relaxes data rules.
  3. Ditto. Using the newly relaxed rule can be done later.
  4. Seems low risk.
@daniel-wong-dfinity-org-twin
daniel-wong-dfinity-org-twin requested review from Copilot and removed request for a team July 23, 2026 16:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR flips the registry canister feature flag that allows Cloud Engine subnets to have a blank replica_version_id, relying on StandardEngineReplicaVersionRecord to determine the effective replica version when blank.

Changes:

  • Enable the IS_BLANK_REPLICA_VERSION_ID_FOR_CLOUD_ENGINES_ENABLED flag by default in the registry canister.
  • Document the behavior change in the registry canister unreleased changelog.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
rs/registry/canister/src/flags.rs Flips the Cloud Engines blank replica_version_id feature flag default to enabled.
rs/registry/canister/unreleased_changelog.md Adds a changelog entry describing the newly enabled behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@daniel-wong-dfinity-org-twin
daniel-wong-dfinity-org-twin force-pushed the ic-admin-propose-to-update-standard-engine-replica-version-daniel-wong branch from 64b37fd to a514985 Compare July 24, 2026 15:08
Base automatically changed from ic-admin-propose-to-update-standard-engine-replica-version-daniel-wong to master July 24, 2026 15:51
@daniel-wong-dfinity-org-twin
daniel-wong-dfinity-org-twin force-pushed the enable-standard-engine-replica-version-daniel-wong branch from 6ac6120 to 486a25f Compare July 27, 2026 15:55
static IS_CHUNKIFYING_LARGE_VALUES_ENABLED: Cell<bool> = const { Cell::new(true) };
static IS_NODE_SWAPPING_ENABLED: Cell<bool> = const { Cell::new(true) };
static IS_BLANK_REPLICA_VERSION_ID_FOR_CLOUD_ENGINES_ENABLED: Cell<bool> = const { Cell::new(false) };
static IS_BLANK_REPLICA_VERSION_ID_FOR_CLOUD_ENGINES_ENABLED: Cell<bool> = const { Cell::new(true) };

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Flipping this makes the doc comment on check_replica_version_invariants false. rs/registry/canister/src/invariants/replica_version.rs:34-38 still reads:

Exception: a CloudEngine can have a blank replica_version_id in its SubnetRecord if there is a StandardEngineReplicaVersionRecord. As of July 22, 2026, this feature is disabled via a flag (but the plan is to enable it in the not too distant future).

The first sentence is still accurate; the second is falsified by this PR. Could you update it here, so we don't leave a "this is disabled" comment sitting next to code that is now enabled? Suggested replacement:

/// Exception: a CloudEngine is allowed to have a blank replica_version_id in
/// its SubnetRecord, provided a StandardEngineReplicaVersionRecord exists. In
/// that case, that record determines the Cloud Engine's replica version.

(Commenting here rather than inline, since replica_version.rs isn't part of this diff.)

@bjoernek

Copy link
Copy Markdown
Contributor

Question rather than a finding: what's the migration path for existing Cloud Engines?

I went through every writer of SubnetRecord.replica_version_id in the registry canister:

  • do_deploy_guestos_to_all_subnet_nodes.rs:40 calls check_replica_version_is_elected, which rejects "".
  • do_split_subnet.rs:95 copies the source subnet's value verbatim.
  • do_create_subnet.rs:565 passes the payload straight through, with no election check.

So as far as I can tell this flip only reaches newly created engines, via create_engine (rs/engine_controller/canister/canister.rs:137), which forwards replica_version_id unvalidated. Existing Cloud Engines can't be moved to a blank replica_version_id at all.

Is that intentional for now, with a follow-up planned, or am I missing a path? Either way it'd be worth a line in the PR description: it shrinks the blast radius of this flip considerably, and it's exactly the context whoever approves the eventual NNS proposal will want.

@bjoernek

Copy link
Copy Markdown
Contributor

Minor, non-blocking, and possibly already considered: 10840 taught SubnetRegistry::get_replica_version to resolve a blank replica_version_id via the StandardEngineReplicaVersionRecord, but its sibling get_replica_version_record (rs/registry/helpers/src/subnet.rs:512-528) still does make_replica_version_key(record.replica_version_id) on the raw field, so it returns None for a blank Cloud Engine.

Only a test calls it today, so nothing is broken. But the two methods now disagree, and the name doesn't hint that one is blank-aware and the other isn't. Was leaving it as-is deliberate? If so, a one-line comment there would save the next person the grep.

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.

4 participants