Remove tight coupling to parachain system

[JoshOrndorff]

This PR makes the pallet generic over notions of VestingBlockNumber and VestingBlockProvider. This allows us to remove the tight coupling to pallet parachain system and make our dependency graph much smaller.

This also adds the feature that any runtime designer using this pallet can choose whether to follow the relay chain's block number or the local chain's block number. It is no longer hard-coded.

This solves #26 and #34, and builds on #42 which will need to be merged first. In a follow-up (#44), I refactor the testing mock to eliminate cumulus dependencies entirely.

[4meta5]

CI is failing because benchmarking needs to be updated to prevent it from depending on some of these cumulus-dev-dependencies. This is because benchmarking requires no-std and it also has the following code in it

fn create_inherent_data<T: Config>(block_number: u32) -> InherentData {
	//let (relay_parent_storage_root, relay_chain_state) = create_fake_valid_proof();
	let vfp = PersistedValidationData {
		relay_parent_number: block_number as RelayChainBlockNumber,
		relay_parent_storage_root: MOCK_PROOF_HASH.into(),
		max_pov_size: 1000u32,
		parent_head: HeadData(vec![1, 1, 1]),
	};
	let inherent_data = {
		let mut inherent_data = InherentData::default();
		let system_inherent_data = ParachainInherentData {
			validation_data: vfp.clone(),
			relay_chain_state: StorageProof::new(vec![MOCK_PROOF.to_vec()]),
			downward_messages: Default::default(),
			horizontal_messages: Default::default(),
		};
		inherent_data
			.put_data(
				cumulus_primitives_parachain_inherent::INHERENT_IDENTIFIER,
				&system_inherent_data,
			)
			.expect("failed to put VFP inherent");
		inherent_data
	};
	inherent_data
}

A comment in the code says this is called with block_number = 1u32 to "to create the first block inherent, to initialize the initRelayBlock". I'm looking into if there is a way to just set the required storage without doing it exactly like how it is done in the pallet's on_finalize. I'll wait until Gorka gets back, maybe he has an idea. It may not be possible #44 (comment)

[girazoki]

The plan is to completely remove cumulus in #44. For now, and since benchmarking uses cumulus still, we should leave them as optional dependencies that are injected when compiling with runtime-benchmarks feature.

[girazoki]

This should go as optional depencencies (and enable them in runtime-benchmarks feature) so that benchmarking uses them.

[JoshOrndorff]

I've done this, but we still have the problem that we can't initialize pallet parachain system in the benchmarks. The problem is that even though our mock runtime does have parachain system installed, the benchmarks are generic over a runtime T and it is not guaranteed that every T will have that pallet installed. Previously this was guaranteed by the trait bound Config: cumulus_pallet_parachain_system::Config.

So I think we need paritytech/substrate#9668 even for this PR.

[girazoki]

I have been looking into this, and since we removed the cumulus_pallet_parachain_system::Config, we would have to write the benchmark code in a similar fashion to https://github.com/paritytech/substrate/tree/master/frame/session/benchmarking. I might give it a try in the next few days but since we are gonna end up removing the dependencies, I dont know if its worth having this intermediate state.

[JoshOrndorff]

dont know if its worth having this intermediate state.

Yeah, I totally agree. I was thinking this intermediate state would be an easy place to rest during the refactor, but now I'm thinking it isn't worth it. Shall we just take this PR and #44 together?

[girazoki]

Yes, let's do that

[JoshOrndorff]

@girazoki Over the weekend, I had an idea to make this work. What do you think.

[girazoki]

Still does not seem to work, not entirely sure why. Maybe its because we dont have the Config implemented in the mock runtime for the benchmarking pallet now? in this case our only solution is to create a subforlder and implement a specific mock runtime for the benchmarking pallet, which I am not sure its worth the effort since we will change it back. I dont think it would cost me more than 30 minutes but...what do you think?

[JoshOrndorff]

I don't think it's worth a second mock. I still feel same as before that it isn't very urgent to get this in. I just wanted to try that idea (partly as a Rust exercise). I'm not sure why it isn't working 🤔

[girazoki]

Not sure either. You are right we shouldnt need a new runtime. I can re-take a look at some point and try to figure out why

[girazoki]

Nice, the CI passes now! let me verify it works against moonbase as well and we are good to go

[JoshOrndorff]

Hooray. Still no hurry, so you can test at your leisure. I'm stoked that this worked though!

[girazoki]

Suggested change

	cumulus-primitives-core = { git = "https://github.com/purestake/cumulus", branch = "nimbus-polkadot-v0.9.9", optional = true }
	cumulus-primitives-core = { git = "https://github.com/purestake/cumulus", branch = "nimbus-polkadot-v0.9.9", optional = true, default-features=false }

[girazoki]

Even if they are optional, as benchmarking is compiled without std, we should add default-features = false in all of them

[JoshOrndorff]

Good catch. CI should also catch this for us.

[girazoki]

Just verified it works with moonbase too. So take a look at my last suggestions and we can merge it