rust runtime integration tests

[4meta5]

What does it do?

Adds integration tests for complete Moonbeam runtime.

This will be useful for testing #311 from the runtime context.

What important points reviewers should know?

Is there something left for follow-up PRs?

• verifying that block gas limit bug is in runtime and not the RPC rust runtime integration tests #339 (comment)
• test multiple block authors -> rewards are correctly distributed

What alternative implementations were considered?

Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?

What value does it bring to the blockchain users?

Checklist

• Does it require a purge of the network?
• You bumped the runtime version if there are breaking changes in the runtime ?
• Does it require changes in documentation/tutorials ?

[4meta5]

• test single block author -> rewards

[JoshOrndorff]

I see now, the runtime is imported from use moonbeam_runtime::Runtime. This is great 👍

[JoshOrndorff]

It might be useful to add a test similar to #337 at this level. That would help us know whether the block gas limit problem is in the runtime or RPC layer (I'm guessing runtime, but would be good to confirm).

[4meta5]

cc @JoshOrndorff I'm having trouble simulating the author-filter logic (necessary for block authoring):

• the set_author_inherent runtime method is private so can't call it from the integration test
• when I make the above method public (which shouldn't be done) and call it, it throws the parachain validation data not set error in author-filter so we need to set that validation data from the cumulus::parachain_system pallet but the set_validation_data method is also private in this pallet (and we can't make this method public)

I guess both problems reduce to figuring out how to call private runtime methods from this integration test context.

[JoshOrndorff]

Hmm, that's an interesting point. Here are a few thoughts on how we might proceed. I don't know which if any of them is right.

1. Should those methods just be public? Is the timestamp pallet's extrinsic public? If that's the solution, we can PR cumulus to make it public.

2. Maybe we shouldn't bother to actually call the extrinsics. What if the test is like:

• Alice stakes as collator.
• Bob nominates her.
• Simulate alice authoring a block by calling note_author directly (skipping the inherent entirely)
• Run chain to end of round
• Assert that both Alice and Bob received rewards.

Although, I admit option #2 is pretty focused on the staking solution and not so much on the authoring...

3. If we used a different filter (for example height filter) it wouldn't require the parachain inherent. Although that would have negative consequences on the live network. I guess that's more of a thought to share than a solution...

4. If it turns out authoring is too hard to interface with because of the inherents, another multi-pallet test would be:
   Alice sends funds to bob with balances::transfer
   Bob sends them to charlie through the evm
   Charlie stakes them.
   Assert charlie is staked.

Even if authoring can be made to work, it might not be the most rewarding place to start.

[JoshOrndorff]

https://github.com/paritytech/substrate/blob/565078a965698c425cd909db55abb3b381e27701/frame/timestamp/src/lib.rs#L192

[joelamouche]

Great progress towards security! Got a few comments

[joelamouche]

why do you need to set alice as author if she is the only collator? Maybe set up a test with more than one collator and somehow count how many time each collator was selected as author and test that the balance reward is correct in a dynamic way

[4meta5]

An author must be set every block or on_finalize in author-inherent panics.

Having multiple collators to test correct reward distribution is already done in parachain-staking unit tests. I think it's reasonable to add the same test here as well.

[JoshOrndorff]

@joelamouche This is a good chance to learn about Substrate inherents. When a collator authors a block, they have the opportunity to include inherent extrinsics. In moonbeam there are three of them (timestamp, validation_data, and author) and all three are required. It is behind-the-scenes from a user perspective, but every time a collator authors a moonbeam block, they insert these three inherents. Since Amar is mocking here, and there is no collator process to do any actual block authoring, we have to call these inherents manually. So setting the author to Alice here is mocking Alice authoring the block.

[4meta5]

Maybe set up a test with more than one collator and somehow count how many time each collator was selected as author and test that the balance reward is correct in a dynamic way

I'd rather do this at the pallet level (and have done it there) where I can mock the set_author command more easily.

I added Charlie, Dave, and Eve to the set of collators in this test and none of them were included in the set of valid block authors so it kept returning the following error:

---- reward_block_authors stdout ----
thread 'reward_block_authors' panicked at 'Expected Ok(_). Got Err(
    DispatchErrorWithPostInfo {
        post_info: PostDispatchInfo {
            actual_weight: None,
            pays_fee: Pays::Yes,
        },
        error: DispatchError::Module {
            index: 17,
            error: 1,
            message: Some(
                "CannotBeAuthor",
            ),
        },
    },
)', runtime/tests/integration_test.rs:337:17

Testing multiple block authors in a runtime integration test is worthwhile, but I'd prefer for it to be pursued in a follow up...

[4meta5]

This feels ready to merge. I'll do it once someone approves :)

[JoshOrndorff]

Looks great to me. Every time I read it, it inspires more ideas of tests to add. I think they should be added in followups though. This PR shows that the method works and adds 3 good tests already.