Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Allow to benchmark HRMP messages from the undying collator #6261

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open

Allow to benchmark HRMP messages from the undying collator #6261

wants to merge 23 commits into from

Conversation

vstakhov
Copy link
Contributor

@vstakhov vstakhov commented Nov 9, 2022 •
edited
Loading

Work description

We want to test HRMP/DMP/UMP (and XCM in perspective) during our zombienet automated tests. Hence, it might be a good idea to have an ability to generate and process such messages within our test collators.

Tasks list

  • Pass RuntimeApi to the collator function
  • Support HRMP channels configuration
  • Move HRMP messages into collator state
  • Support messages within wasm_validation
  • Add Zombienet scenario sample with HRMP channels configured
  • Think about useful payload
  • Probably add UMP/DMP messages processing as well

@github-actions github-actions bot added the A3-in_progress Pull request is in progress. No review needed at this stage. label Nov 9, 2022
@vstakhov
Copy link
Contributor Author

@sandreim What do you think about the final point - useful payload? Now the test just adds messages that consist of only zeroes filled to the specific size. Do we want something more sophisticated?

@vstakhov vstakhov force-pushed the vstakhov-undying-hrmp branch from 79405dc to 6109424 Compare November 21, 2022 13:14
@vstakhov vstakhov marked this pull request as ready for review November 21, 2022 21:43
@vstakhov vstakhov changed the title [DNM] Allow to benchmark HRMP messages from the undying collator Allow to benchmark HRMP messages from the undying collator Nov 21, 2022
@github-actions github-actions bot added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Nov 21, 2022
@vstakhov vstakhov added B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. labels Nov 21, 2022
@vstakhov vstakhov requested a review from sandreim November 21, 2022 21:45
@vstakhov vstakhov added the T4-parachains_engineering This PR/Issue is related to Parachains performance, stability, maintenance. label Nov 21, 2022
sandreim
sandreim reviewed Nov 25, 2022
View reviewed changes
Copy link
Contributor

@sandreim sandreim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good at high level. What I think is missing is a way to measure the amount of messages sent/received in the test and assert against that. We could add some prometheus metrics for this.

parachain/test-parachains/undying/collator/src/cli.rs Outdated Show resolved Hide resolved
parachain/test-parachains/undying/src/lib.rs
pub struct HrmpChannelConfiguration {
/// Where to send HRMP messages
pub destination_para_id: u32,
/// Message size
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagine we can have a better interface here to configure the utilization of the channels. For example, can we also add number of messages ?

HrmpChannel::{max_capacity, max_message_size, max_total_size} already provide the maximum values, maybe we can use them

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was there and there was such a parameter. However, there is a rule that there could be only one message per a combination of sender-recipient in a single candidate: https://github.com/paritytech/polkadot/blob/master/runtime/parachains/src/hrmp.rs#L941
So this parameter would be useless I suppose.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This make sense and it would be nice to document this and show it to the user.

zombienet_tests/functional/0001-parachains-pvf.toml Outdated
@@ -117,7 +117,7 @@ genesis_state_generator = "undying-collator export-genesis-state --pov-size=1000
name = "collator07"
image = "{{COL_IMAGE}}"
command = "undying-collator"
args = ["-lparachain=debug", "--pov-size=100000", "--pvf-complexity=300", "--parachain-id=2006"]
args = ["-lparachain=debug", "--pov-size=100000", "--pvf-complexity=300", "--parachain-id=2006", "--hrmp-params=2007:4096", "-lruntime::undying=debug"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would create a separate test for this where we match on some channel utilization metrics to be sure everything works as expected.

To make this truly deterministic we would need to be able to configure the test parachain to send a predefined amount of messages then stop. We can then expect this to happen in a number of relay chain blocks - a minimum tput.

parachain/test-parachains/undying/collator/src/lib.rs Show resolved Hide resolved
parachain/test-parachains/undying/collator/src/cli.rs

/// Configuration of the HRMP channels
#[clap(long)]
pub hrmp_params: Vec<CliHrmpChannelConfiguration>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think sending messages should be optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is optional, e.g. no messages are sent when a vector is empty.

zombienet_tests/functional/0001-parachains-pvf.toml
[[hrmp_channels]]
sender = 2006
recipient = 2007
max_capacity = 2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why only 2 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In and out.

@sandreim
Copy link
Contributor

@sandreim What do you think about the final point - useful payload? Now the test just adds messages that consist of only zeroes filled to the specific size. Do we want something more sophisticated?

I think it's enough if we fill it with zeroes. If we were to add something more sophisticated, we would have to build a real runtime and send/receive real messages.

@vstakhov vstakhov requested a review from sandreim November 30, 2022 17:12
sandreim
sandreim reviewed Dec 6, 2022
View reviewed changes
Copy link
Contributor

@sandreim sandreim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall, just a few things I spotted. We should also have the Zombienet test check the metrics. @pepoviola can you help us with support for querying metrics of collators ?

parachain/test-parachains/undying/collator/src/cli.rs Show resolved Hide resolved
parachain/test-parachains/undying/src/lib.rs
/// Where to send HRMP messages
pub destination_para_id: u32,
/// Message size
pub message_size: u32,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This interface seems to allow to configure the rate (even if it will be probabilistic) and actual bandwidth, but as state above I think we need to make clear how many messages one para is allowed to send, and the fact that it is actually a blob.

parachain/test-parachains/undying/src/lib.rs
pub struct HrmpChannelConfiguration {
/// Where to send HRMP messages
pub destination_para_id: u32,
/// Message size
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This make sense and it would be nice to document this and show it to the user.

parachain/test-parachains/undying/src/lib.rs
))
}

// Returns coin probability based on hash (assuming that the hash value is indistinguishable from random)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure about this. I would use a RNG and allow user to specify seed for reproducibility reasons.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, since crypto hashes outputs are indistinguishable from random (by definition), I assume it is fine to use those as PRG that is totally reproducable when needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation has been added.

@vstakhov vstakhov force-pushed the vstakhov-undying-hrmp branch from 51a173a to 1a1b696 Compare March 28, 2023 09:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sandreim sandreim sandreim left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. T4-parachains_engineering This PR/Issue is related to Parachains performance, stability, maintenance.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vstakhov @sandreim