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

Warn validators with slow hardware #12017

Closed
ggwpez opened this issue Aug 12, 2022 · 10 comments · Fixed by #12620
Closed

Warn validators with slow hardware #12017

ggwpez opened this issue Aug 12, 2022 · 10 comments · Fixed by #12620
Assignees
Labels
I7-refactor Code needs refactoring. J0-enhancement An additional feature request. U3-nice_to_have Issue is worth doing eventually. Z1-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder Z6-mentor An easy task where a mentor is available. Please indicate in the issue who the mentor could be.

Comments

@ggwpez
Copy link
Member

ggwpez commented Aug 12, 2022

Upon starting a node we currently print some brief hardware metrics.
This could be extended to include a warning in case the node has bad performance and was started with --validator.

The logic for comparing results and having a baseline is currently in utils/frame/benchmarking-cli, some of this would probably move to sc-sysinfo. sc-sysinfo would profit from this since it can use Throughput types (and other) instead of just u64 in some places.

@ggwpez ggwpez added J0-enhancement An additional feature request. U3-nice_to_have Issue is worth doing eventually. Z6-mentor An easy task where a mentor is available. Please indicate in the issue who the mentor could be. Z1-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder labels Aug 12, 2022
@ggwpez ggwpez added the I7-refactor Code needs refactoring. label Aug 12, 2022
@dvdplm
Copy link
Contributor

dvdplm commented Aug 17, 2022

@ordian Maybe this can be helpful in view of the disputes slashing roll-out as well?

@bkchr
Copy link
Member

bkchr commented Aug 17, 2022

We already got the host performance check in Polkadot for this: https://github.com/paritytech/polkadot/blob/master/cli/src/host_perf_check.rs

@ggwpez
Copy link
Member Author

ggwpez commented Aug 17, 2022

We already got the host performance check in Polkadot for this …

Do you mean that we should not add it to Polkadot then, but still to Substrate? Or not at all?
I keep seeing people in the Kusama 1KV channel actually using the benchmark machine command, so they seem to like it.

@bkchr
Copy link
Member

bkchr commented Aug 17, 2022

We can add it to Substrate, if it is done in a generic way. Aka chain implementors like Polkadot provide their own expected numbers that are treated as "good".

My comment was more an answer to @dvdplm.

But if we have your stuff, we can deprecate the host perf check.

@Szegoo
Copy link
Contributor

Szegoo commented Sep 27, 2022

@ggwpez I am gonna pick this up :)

@Szegoo
Copy link
Contributor

Szegoo commented Oct 15, 2022

@ggwpez You mentioned in #12368 that we could move Metric to sc-sysinfo also, I will open a small PR for that if you didn't change your mind.

@ggwpez
Copy link
Member Author

ggwpez commented Oct 15, 2022

Please try to do it in one. I dont want to increase the review overhead for such a rather simple issue @Szegoo

@Szegoo
Copy link
Contributor

Szegoo commented Oct 15, 2022

@ggwpez Ok, I will close this issue and move Metric in the same PR.

@Szegoo
Copy link
Contributor

Szegoo commented Nov 5, 2022

@ggwpez Should we deprecate the host perf check in polkadot then and have it in substrate implemented in a generic way?

@ggwpez
Copy link
Member Author

ggwpez commented Nov 5, 2022

I have to look into the PVF check command again and chat with who created it. For now it is still fine to have both IMO.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
I7-refactor Code needs refactoring. J0-enhancement An additional feature request. U3-nice_to_have Issue is worth doing eventually. Z1-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder Z6-mentor An easy task where a mentor is available. Please indicate in the issue who the mentor could be.
Projects
Development

Successfully merging a pull request may close this issue.

4 participants