Skip to content

fix: Use the same registry version in CatchUpPackageParam and verification - #11358

Open
eichhorl wants to merge 3 commits into
masterfrom
eichhorl/consistent-registry-version
Open

fix: Use the same registry version in CatchUpPackageParam and verification #11358
eichhorl wants to merge 3 commits into
masterfrom
eichhorl/consistent-registry-version

Conversation

@eichhorl

@eichhorl eichhorl commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

The orchestrator only fetches a CUP if it has a higher height and registry version than the local CUP. Here, the relevant registry version is the one of the DKG summary.

However, when validating the CUPs signature, we used the version of the validation context, which in principal could be smaller than the one of the DKG summary. This could lead to the CUP being verified with an old public key from the registry.

With this PR, we also use the registry version of the DKG summary for signature verification. This is fine, since under normal operation, both versions should always point to the same public key:

  1. The public key can only change via recovery and subnet splitting, during which both versions are equal.
  2. A recovery CUP may only be adopted if the subnet is stalled, it is therefore not possible to create a summary whose validation context points to the new key, while the DKG version still points to the old key.
  3. During a subnet split, the version of the validation context is intentionally frozen to one version below the one introducing the new key. The "scheduled split" summary (which bumps the context's version to the new key) is never part of a CUP. Instead, a new summary is derived for which both versions point to the new key (like in a recovery).

@eichhorl eichhorl added the CI_ALL_BAZEL_TARGETS Runs all bazel targets label Aug 27, 2026
@github-actions github-actions Bot added the fix label Aug 27, 2026
@eichhorl eichhorl changed the title fix: Use the same registry number in CertificateParams and verification fix: Use the same registry version in CertificateParams and verification Aug 27, 2026
@eichhorl eichhorl changed the title fix: Use the same registry version in CertificateParams and verification fix: Use the same registry version in CatchUpPackageParam and verification Aug 27, 2026
@eichhorl
eichhorl marked this pull request as ready for review August 27, 2026 17:03
@eichhorl
eichhorl requested a review from a team as a code owner August 27, 2026 17:03
@zeropath-ai

zeropath-ai Bot commented Aug 27, 2026

Copy link
Copy Markdown

No security or compliance issues detected. Reviewed everything up to e46aeeb.

Security Overview
Detected Code Changes
Change Type Relevant files
Enhancement ► rs/consensus/cup_utils/src/lib.rs
      Update tests to align registry CUP contents with registry version and add new registry version consistency checks
► rs/consensus/src/consensus/catchup_package_maker.rs
      Add post-split block registry version consistency assertions in tests
► rs/orchestrator/src/catch_up_package_provider.rs
      Use CUP registry_version() for verification context in provider calls and tests
► rs/orchestrator/src/catch_up_package_provider.rs
      Introduce test infrastructure: RegistryVersionRecordingCrypto and tests for verifier timing
► rs/orchestrator/src/catch_up_package_provider.rs
      Add test service to serve CUP content for verification tests
► (additional test-related changes within the above files)

@pierugo-dfinity pierugo-dfinity 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.

the validation context, which in principal could be smaller than the one of the DKG summary

You mean the opposite? So it's actually the other way around, we could technically validate the CUP with a "future" public key of the subnet.

The fix is anyways still correct: we should use the same registry version for validation than the one we used to sign it. The latter is the one from the high threshold transcript of the summary, and the payload builder ensures it is equal to the summary's registry version, which is what you are using here. Though it does not look like this implicit invariant is actually enforced by any tests today.

assert_eq!(
*crypto.verified_at.lock().unwrap(),
Some(summary_registry_version)
);

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.

Maybe also add an assert_ne!(verified_at, context_registry_version)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done, also added a test making sure that both versions are the same for registry cups

@eichhorl

Copy link
Copy Markdown
Contributor Author

the validation context, which in principal could be smaller than the one of the DKG summary

You mean the opposite? So it's actually the other way around, we could technically validate the CUP with a "future" public key of the subnet.

No, I mean that assuming the CUP is completely forged, then that forgery could also contain a validation context with a registry version that is smaller than the one of the DKG summary

Under normal operation I agree that the risk is the one you describe (although it should never happen with the argument that recovery/splitting set both versions to the same value)

@pierugo-dfinity

Copy link
Copy Markdown
Contributor

Ah yes, I understand your argument

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants