[BridgeHub] Add and run Basic unit tests

[EmmanuellNorbertTulbure]

For Rococo - Wococo

• add integrity tests to the bridge-hub-rococo runtime (for both sides R->W and W->R)

     fn ensure_bridge_integrity() {
		assert_complete_bridge_types!(

• ...

[serban300]

Attaching the integrity test for Rialto -> Millau as an example:

parity-bridges-common/bin/rialto/runtime/src/millau_messages.rs

Line 206 in ec6bafa

fn ensure_bridge_integrity() {

[serban300]

Started adding integrity tests for Rococo -> Wococo and Wococo -> Rococo. There are 2 issues so far:

1. Checking the type of the InboundPayload. The tests expects a FromBridgedChainMessagePayload, but we use XcmAsPlainPayload for BridgeHubRococo and BridgeHubWococo

@bkontur @svyatonik do we plan to use FromBridgedChainMessagePayload here in the future ?

2. Checking the BlockWeights. For BridgeHubRococo and BridgeHubWococo we expect to find the weights defined in polkadot-core which use the constants defined also in polkadot-core. But instead the runtime uses the same formula, but with other constants defined in cumulus/parachains/common and other BlockExecutionWeight.

[svyatonik]

1 Checking the type of the InboundPayload. The tests expects a FromBridgedChainMessagePayload, but we use XcmAsPlainPayload for BridgeHubRococo and BridgeHubWococo

My guess that we'll be using XcmAsPlainPayload. @bkontur wanted to backport his changed to the parity-bridge-common, so in the end the FromBridgedChainMessagePayload will use the same payload.

2. Checking the BlockWeights. For BridgeHubRococo and BridgeHubWococo we expect to find the weights defined in polkadot-core which use the constants defined also in polkadot-core. But instead the runtime uses the same formula, but with other constants defined in cumulus/parachains/common and other BlockExecutionWeight.

I've tried to find where we are using those BlockWeights and it seems that they are no longer required. So IMO we need to remove this check - added a comment to the #1638. It needs to be double checked before removal

[serban300]

Thanks for the answer !

My guess that we'll be using XcmAsPlainPayload. @bkontur wanted to backport his changed to the parity-bridge-common, so in the end the FromBridgedChainMessagePayload will use the same payload.

@bkontur can I help with backporting this change ? Is there an issue for it ?

I've tried to find where we are using those BlockWeights and it seems that they are no longer required. So IMO we need to remove this check - added #1638 (comment) to the #1638. It needs to be double checked before removal

Ok, I can double check this and remove the BlockWeights and the assert.

[serban300]

I've tried to find where we are using those BlockWeights and it seems that they are no longer required. So IMO we need to remove this check - added #1638 (comment) to the #1638. It needs to be double checked before removal

@svyatonik I checked this and we use the BlockWeights in computing the max_extrinsic_weight(), which is then used in messages relaying here. I guess we still need this if I understand correctly.

[svyatonik]

I've tried to find where we are using those BlockWeights and it seems that they are no longer required. So IMO we need to remove this check - added #1638 (comment) to the #1638. It needs to be double checked before removal

@svyatonik I checked this and we use the BlockWeights in computing the max_extrinsic_weight(), which is then used in messages relaying here. I guess we still need this if I understand correctly.

Yeah, you're right, thank you! Then we need to change weights of our parachains Rococo/Wococo/Polkadot/Kusama bridge hubs to what Cumulus offers. Then it should be fine. Sorry for misleading you :/

[bkontur]

@serban300
I am waiting for one PR to be merged to the polkadot gav-xcm-v3 and after that I would like to update (polkadot/substrate) deps for parity-bridges-common + cumulus, so after that I would like to backport that

and to the weights, those block_weights in cumulus they are just copy/past from other cumulus runtimes,
what is another issue, related to this weights, we need to add all bridges pallets to the automatic benchmarking here: https://github.com/paritytech/cumulus/blob/master/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/lib.rs#L463-L470
to be able to regenerate weights in cumulus with bench.bot, you could take this issue, if you want: #391

e.g.:
/cmd queue -c bench-bot $ xcm statemint assets pallet_xcm_benchmarks::fungible

/cmd queue -c bench-bot $ pallet bridge-hub-rococo bridge-hubs pallet_bridges_granda
/cmd queue -c bench-bot $ pallet bridge-hub-rococo bridge-hubs pallet_bridges_parachain
/cmd queue -c bench-bot $ pallet bridge-hub-rococo bridge-hubs pallet_bridges_messages
/cmd queue -c bench-bot $ pallet bridge-hub-rococo bridge-hubs pallet_bridges_rewards

[svyatonik]

@serban300 I am waiting for one PR to be merged to the polkadot gav-xcm-v3 and after that I would like to update (polkadot/substrate) deps for parity-bridges-common + cumulus, so after that I would like to backport that

and to the weights, those block_weights in cumulus they are just copy/past from other cumulus runtimes, what is another issue, related to this weights, we need to add all bridges pallets to the automatic benchmarking here: https://github.com/paritytech/cumulus/blob/master/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/lib.rs#L463-L470 to be able to regenerate weights in cumulus with bench.bot, you could take this issue, if you want: #391

e.g.:
/cmd queue -c bench-bot $ xcm statemint assets pallet_xcm_benchmarks::fungible

/cmd queue -c bench-bot $ pallet bridge-hub-rococo bridge-hubs pallet_bridges_granda
/cmd queue -c bench-bot $ pallet bridge-hub-rococo bridge-hubs pallet_bridges_parachain
/cmd queue -c bench-bot $ pallet bridge-hub-rococo bridge-hubs pallet_bridges_messages
/cmd queue -c bench-bot $ pallet bridge-hub-rococo bridge-hubs pallet_bridges_rewards

That's a bit different weights - they're weights of calls, not block limits. And we yet need to find a way to deal with these. This should probably be a part of release process (manual) - i.e. we'll be checking that weights in our repo are close to what bridge hubs are using. Let's probably not touch it now

[serban300]

@bkontur thanks for the answer ! Ok, I'll merge the integrity tests after you finish backporting the change. In the meanwhile I'll just fix the weights for Rococo/Wococo bridge hubs: #1709

[bkontur]

@serban300
backport is ready, #1813

but I was thinking, if should better change that integrity test, to be more flexible:

parity-bridges-common/bin/runtime-common/src/integrity.rs

Lines 90 to 92 in 8649d12

	assert_type_eq_all!(<$r as MessagesConfig<$i>>::OutboundPayload, FromThisChainMessagePayload);
	assert_type_eq_all!(<$r as MessagesConfig<$i>>::InboundPayload, FromBridgedChainMessagePayload<CallOf<ThisChain<$bridge>>>);

for example FromThisChainMessagePayload / FromBridgedChainMessagePayload<CallOf<ThisChain<$bridge>>> could be passed as macro parameter, it will be more flexible and generic, rialto/millau stuff could still use FromBridgedChainMessagePayload and BridgeHubs XcmAsPlainPayload or whatever, wdyt?

[serban300]

Thanks! I'm taking a more detailed look on the tests to see what would be the best solution

[serban300]

Resolved by paritytech/cumulus#1975