Deprecate para_id Field from Chain Specification

[Stephenlawrence00]

• Resolving Deprecate para_id field from chain-specification #7384 and related Issues:
• Introduce new runtime api GetParachainId #75
• Get rid off parachain-info #2116

[cla-bot-2021]

User @Stephenlawrence00, please sign the CLA here.

[iulianbarbu]

Hi @Stephenlawrence00 , just wondering if you're still interested in getting this ready for review. I glanced over the changes and thought you might find useful this info: #75 (comment).

If you don't have time to still look into it, let us know. We're keen in assisting/owning the delivery of this in the next period.

Let us know if you have questions!

[Stephenlawrence00]

Thanks @iulianbarbu

Still interested in getting this done.

[seemantaggarwal]

@Stephenlawrence00 if you have something to parallelise or I can pick up, let me know

[iulianbarbu]

Hey @Stephenlawrence00 , how is going? I was wondering if you got a chance to continue the work. Feel free to reach out in case implementation details are not clear.

For context, we want to have this functionality merged by mid of May so that it ends up in June release. FYI, the reviews and addressing them can take a few weeks, so IMO ideally we should kickstart the work this week/next week the latest. Does that sound doable on your end?

[Stephenlawrence00]

Sounds good @iulianbarbu

[iulianbarbu]

I think this should be mostly it. Now I am wondering what's a good way to sunset parachain-info. @bkchr what are your thoughts?

LE: upsy, there is an issue for that: #2116. Based on it, I think we can start removing parachain info from our runtimes. What we need to double check though is if adding the para id in the runtimes' parameters is enough for downstream usage which might rely on the Get<ParaId> trait impl for Pallet<T> where T: Config.

LELE: once the above is done, we can start doing the actualy para_id field removal from chain specs, according to #7384

[skunert]

Getting rid of ParachainInfo pallet is not needed for now. The pallet should stay for the reasons I outlined here: #2116 (comment).

The runtime API should return the para id that is present in ParachainInfo pallet. Then the node side should be modified in this PR to not use the para_id from the chainspec anymore if the runtime API is present. One example of this is here:

polkadot-sdk/cumulus/polkadot-omni-node/lib/src/command.rs

Line 241 in 6ce6110

let para_id = ParaId::from(

There we read the para_id from the chain_spec, which should not be the case anymore.

[iulianbarbu]

Hey @Stephenlawrence00 ! Looks like we still need return the 'para_id' of the ParachainInfo, instead of hardcoding it into the runtime to return it. It means we need to go back to this state mostly: 46004e3. Sorry for making you digress with my suggestion for hardcoding, but I missed the important point @skunert made here: #2116 (comment).

Separately, in addition to @skunert pointer to how to not use anymore the para_id from the chain spec like here:

polkadot-sdk/cumulus/polkadot-omni-node/lib/src/command.rs

Line 241 in 6ce6110

let para_id = ParaId::from(

, the runtime api call that should be made can be done somewhere around here:

polkadot-sdk/cumulus/polkadot-omni-node/lib/src/common/spec.rs

Line 273 in 6ce6110

let client = params.client.clone();

, by using the let para_id = client.runtime_api().id();. To be able to call GetParachainInfo::id() though you'd need to extend the NodeRuntimeApi here:

polkadot-sdk/cumulus/polkadot-omni-node/lib/src/common/mod.rs

Line 64 in 6ce6110

pub trait NodeRuntimeApi<Block: BlockT>:

.

Once we have this, I think we should be good with deprecating this:

polkadot-sdk/cumulus/polkadot-omni-node/lib/src/common/chain_spec.rs

Line 47 in 6ce6110

pub para_id: u32,

. E.g. just add a #[deprecated] attribute, a comment that clarifies this is deprecated, and a link to a new issue which tracks its removal (for removal timeline, we can mention it will be removed in the next stable release, so by 25/09). Any objections on this deprecation notice @skunert ?

[skunert]

(for removal timeline, we can mention it will be removed in the next stable release, so by 25/09). Any objections on this deprecation notice @skunert ?

Nope, makes sense to me.

However, we should come up with a better name for the runtime api function than id(). Maybe just parachain_id() or para_id().

[skunert]

@Stephenlawrence00 Just checking in if you planning to continue with this. If not, we will internally find someone to work on this soon (end of week).

[Stephenlawrence00]

Will have the changes in @skunert

[Stephenlawrence00]

Would I need to add a test case in system chain runtimes?

for instance

#[test]
fn parachain_id_is_retrieved_from_runtime_api() {
    ExtBuilder::<Runtime>::default()
        .build()
        .execute_with(|| {
            // Get para_id from ParachainInfo pallet storage
            let expected_para_id = parachain_info::Pallet::<Runtime>::parachain_id();
            
            // Get para_id via runtime API
            let actual_para_id = Runtime::parachain_id();
            
            assert_eq!(
                actual_para_id, expected_para_id,
                "Runtime API para_id must match ParachainInfo storage"
            );
        });
}

Don't think this tests much really cc: @skunert @iulianbarbu

[skunert]

I agree, such test does not seem very useful

[Stephenlawrence00]

I agree, such test does not seem very useful

What tests would you consider, regarding this pull request? @skunert

[skunert]

Hey,

thanks for driving this forward!
Please take a look at these missing occurences that require adjustment:

• https://github.com/paritytech/polkadot-sdk/blob/master/templates/parachain/node/src/command.rs#L234-L234
• https://github.com/paritytech/polkadot-sdk/blob/master/cumulus/test/service/src/main.rs#L82-L82

There is the chain-spec-builder, which we use to build chain-specs. When someone provides the --para-id arguments there, we should print that this argument is deprecated after stable2506 and will not be used by parachain nodes anymore. We can not just remove the arg, since there should be a proper migration phase.

As for testing, we have some tests that test the manual-seal functionality of omni-node:

polkadot-sdk/docs/sdk/src/guides/your_first_node.rs

Line 316 in db5ee7d

fn omni_node_dev_mode_works() {

There it would be super easy to read the chain-spec, modify the para_id to something completely random and then write the chain spec to a temporary directory. Without the changes in your PR the test would fail, because there would be a mismatch between runtime and node. However this only tests this specific mode of operation.

In addition, we can have better test-coverage by removing the para_id field from chain-specs exported from the test-parachain node. Remove the Extension here and just pass Default::default().

polkadot-sdk/cumulus/test/service/src/chain_spec.rs

Line 92 in db5ee7d

Extensions { para_id: id.unwrap_or(cumulus_test_runtime::PARACHAIN_ID.into()).into() },

As a result, all chain-specs exported from test-parachain will not have the para_id field anymore and all our zombienet-tests will work with the runtime para_id. This is already a good chunk of testing. In the long run, we should remove the extension from the other chain-specs too, but IMO this should be done in the next release, so that the changes you introduce have some time to saturate.

[iulianbarbu]

Hey @Stephenlawrence00 , how is going? Was wondering how the testing goes. Once we'll get past that we'll have leeway to focus on the last bits, related to para-id deprecation notices. Would be great to capture everything in the cutoff for next release, which has been pushed back with one more week roughly. Let us know how you feel about the left work and your availability.

[Stephenlawrence00]

@iulianbarbu On it

[iulianbarbu]

Hey @Stephenlawrence00 ! Hope you're well.

We decided to take the work you started here and continue it with dedicated capacity internally. The continuation will be based on your work so that your effort is not lost. We will do this because we want to ship this sometime sooner, to align with the next release. You can push any in-flight work here and I'll try to consider it for a new PR I'll open based on the existing state of the branch.

Hope this is understandable, and please don't hesistate to reach out if you want to contribute to other issues - hoppefully will mark better those that allow for a wider implementation/testing timespan.