Skip to content

Conversation

Sajjon
Copy link
Contributor

@Sajjon Sajjon commented Aug 7, 2025

Solves #9400

No logic change, only moves types from polkadot/primitives/src/vstaging into polkadot/primitives/src/v9 (renamed from v8 to v9).

Sajjon and others added 23 commits August 5, 2025 13:36
@Sajjon Sajjon requested a review from a team as a code owner August 7, 2025 09:57
@Sajjon Sajjon requested a review from alindima August 7, 2025 09:57
Copy link
Contributor

@sistemd sistemd left a comment

Choose a reason for hiding this comment

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

Looks good. Merge conflicts are going to be annoying 😄

@Sajjon
Copy link
Contributor Author

Sajjon commented Aug 7, 2025

Forgot to rename https://github.com/paritytech/polkadot-sdk/blob/master/polkadot/runtime/parachains/src/runtime_api_impl/v11.rs to v13.rs and move relevant APIs from polkadot/runtime/parachains/src/runtime_api_impl/vstaging.rs. I'm fixing now!

@@ -763,7 +772,7 @@ pub type UncheckedSignedAvailabilityBitfields = Vec<UncheckedSignedAvailabilityB

/// A backed (or backable, depending on context) candidate.
#[derive(Encode, Decode, Clone, PartialEq, Eq, RuntimeDebug, TypeInfo)]
pub struct BackedCandidate<H = Hash> {
pub struct LegacyBackedCandidate<H = Hash> {
Copy link
Contributor

Choose a reason for hiding this comment

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

CandidateDescriptorV2 is binary compatible with V1, why do we need these legacy structures ? Am I missing something ?

Copy link
Contributor Author

@Sajjon Sajjon Aug 11, 2025

Choose a reason for hiding this comment

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

Great! I remove as many types as possible. It was just a strategy to do zero logic change and resolve name conflicts

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also remove old CandidateDescriptor and other types using it ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Old CandidateDescriptor is used by old CommittedCandidateReceipt which in turn is referenced 210 times in 57 files.

Do you want me to try to go through these 210 places and see if I can upgrade to CommittedCandidateReceiptV2 (which uses CandidateDescriptorV2)?

Copy link
Contributor

Choose a reason for hiding this comment

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

@sandreim you mean totally replace CandidateDescriptor with CandidateDescriptorV2? They're not necessarily binary compatible (although I see an effort has been put into making that happen). structs not attributed with #[repr(C)] do not have guaranteed memory layout. So we'll have to either make sure the current version of Rust lays them out in a compatible way, or migrate storages. Let me know wdyt.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see what you mean. We're insured to be binary compatible by the SCALE codec, and as long as we don't mix them up unencoded in-memory, we're good. I'll try to sort that out in a single commit, so we can easily revert it if any problems arise

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, here we go: 8c5a93a

I didn't have a strong feeling whether I was doing the right thing or not when I was removing V1 entirely from the collation generation subsys, but you won't allow me to merge anything that will cause everything to explode, right? You won't? Right?..

@Sajjon Sajjon requested a review from sandreim August 12, 2025 06:05
@@ -763,7 +772,7 @@ pub type UncheckedSignedAvailabilityBitfields = Vec<UncheckedSignedAvailabilityB

/// A backed (or backable, depending on context) candidate.
#[derive(Encode, Decode, Clone, PartialEq, Eq, RuntimeDebug, TypeInfo)]
pub struct BackedCandidate<H = Hash> {
pub struct LegacyBackedCandidate<H = Hash> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also remove old CandidateDescriptor and other types using it ?

@s0me0ne-unkn0wn s0me0ne-unkn0wn added T17-primitives Changes to primitives that are not covered by any other label. and removed T8-polkadot This PR/Issue is related to/affects the Polkadot network. T9-cumulus This PR/Issue is related to cumulus. labels Aug 28, 2025
@paritytech-workflow-stopper
Copy link

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/17308831278
Failed job name: cargo-clippy

@paritytech-workflow-stopper
Copy link

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/17308831211
Failed job name: test-linux-stable-runtime-benchmarks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T17-primitives Changes to primitives that are not covered by any other label.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants