Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Completely remove cumulus (dev-)dependencies #44

Closed
wants to merge 21 commits into from

Conversation

JoshOrndorff
Copy link
Contributor

This PR is a followup to #43. It removes all dependencies (even dev-dependencies) on crates from cumulus.

Tests now use the parachain's own block number rather than the mocked relay number. This means that the InitVestingBlock was 1 rather than 2 as it was before. This caused most of the block indices to be off-by-one in tests that tested for specific vested amounts.

@JoshOrndorff JoshOrndorff requested a review from girazoki August 23, 2021 21:21
@notlesh
Copy link
Contributor

notlesh commented Aug 23, 2021

This PR title looked alarming when it came across my inbox 😆

@JoshOrndorff JoshOrndorff changed the title Joshy totally kill cumulus Completely remove cumulus (dev-)dependencies Aug 23, 2021
@girazoki
Copy link
Contributor

Although tests work fine in this case, benchmarking is run against the moonbase runtime and this fetches the relay block height from cumulus-pallet-parachain-system. So even though the pallet will compile, the benchmarking will not work when we run it against our moonbase runtime (e.g., the claim function needs the relay chain height to advance to be able to claim). And the way to do that is to inject valid inherents (thus the reason why create_inherent_data was there before).

@girazoki
Copy link
Contributor

girazoki commented Aug 31, 2021

So even if we do that, this associated type has only the BlockNumberProvider trait which allows only to get the current relay block number. So since we are not setting this number anywhere in the benchmarking code (nor the associated type provides a function to do so), when we benchmark against the moonbase runtime we will always read a 0 there and the benchmarking will not be properly done for functions like claim where we need that number to be moving up.

I see a couple of solutions:

  1. Read this value on_initialize, and store it in our own storage. Our own storage is something we can hack in the benchmarking code. But of course this is much less efficient.

  2. Add a trait bound like ProvideInherent in addition to BlockNumberProvider. In this case we could write the benchmarking code to add the inherents. The problem is we probably wont remove the dependency on cumulus but rather make it optional and add it for benchmarking only. The reason is that we still need to push valid inherents in the context of a moonbase runtime, and they need to have this https://github.com/paritytech/cumulus/blob/968c91e357e184ed117f3b39fd51fce23a6945c4/pallets/parachain-system/src/lib.rs#L566 specific format

@4meta5
Copy link

4meta5 commented Aug 31, 2021

Read this value on_initialize, and store it in our own storage. Our own storage is something we can hack in the benchmarking code. But of course this is much less efficient.

Sounds reasonable to me, how much less efficient is it?

@JoshOrndorff
Copy link
Contributor Author

The key idea here is that a pallet may be used in multiple different runtimes, and the costs to use the pallet may differ between those runtimes.

I think rather than hacking at the pallet level to get accurate benchmarks for the one runtime that we care most about (at the expense of anyone else who uses this pallet) we should instead run the benchmarks against each runtime that actually uses the pallet.

@notlesh weren't you having similar ideas about pallets that Moonbeam uses from Substrate?

So my main point is that this problem is not specific to crowdloan rewards, so it should not have a crowdloan-rewards-specific solution.

@girazoki
Copy link
Contributor

I like that idea, but in that case we need to write runtime-dependent benchmark code. The benchmarking code that exists here aims to be generic, something that we might want to forget about

@girazoki
Copy link
Contributor

Superseded by #61 due to merge conflicts

@girazoki girazoki closed this Aug 25, 2022
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