Fix 5560: add support for a new `staking-miner info` command #5577

chevdor · 2022-05-20T16:28:31Z

Summary

This PR fixes #5560. It adds a new info command that can be used to identity the runtime version that is supported by this version of the staking-miner. It also refactors away the clap options to a separate file.

Warning breaking change (cli)

⚠️ This PR slightly alters the cli interface:
--seed-or-path moves from being a global flag to being a flag only for the 2 commands requiring it.

So the previous:

staking-miner --seed-or-path <foo> monitor|dry-run

becomes now:

staking-miner monitor|dry-run --seed-or-path <foo>

and the seed is not required for the other commands.

help

staking-miner 0.9.22
Parity Technologies <admin@parity.io>

USAGE:
    staking-miner [OPTIONS] <SUBCOMMAND>

OPTIONS:
    -h, --help         Print help information
    -u, --uri <URI>    The `ws` node to connect to [env: URI=] [default: wss://rpc.polkadot.io:443]
    -V, --version      Print version information

SUBCOMMANDS:
    dry-run               Just compute a solution now, and don't submit it
    emergency-solution    Provide a solution that can be submitted to the chain as an emergency
                              response
    help                  Print this message or the help of the given subcommand(s)
    info                  Return information about the current version
    monitor               Monitor for the phase being signed, then compute

New `info` command:

Return information about the current version

USAGE:
    staking-miner info [OPTIONS]

OPTIONS:
    -h, --help         Print help information
    -j, --json         Serialize the output as json
    -u, --uri <URI>    The `ws` node to connect to [env: URI=] [default: wss://rpc.polkadot.io:443]

Output (human, default)

- linked:
   impl_name           :  parity-polkadot
   spec_name           :         polkadot
   spec_version        :             9250
   transaction_version :               13
   impl_version        :                0
   authoringVersion    :                0
   state_version       :                0
- remote :
   impl_name           :  parity-polkadot
   spec_name           :         polkadot
   spec_version        :             9220
   transaction_version :               12
   impl_version        :                0
   authoringVersion    :                0
   state_version       :                0
Compatibles: NO

Output (`--json`)

{
  "linked": {
    "specName": "polkadot",
    "implName": "parity-polkadot",
    "authoringVersion": 0,
    "specVersion": 9250,
    "implVersion": 0,
    "apis": [...],
    "transactionVersion": 13,
    "stateVersion": 0
  },
  "remote": {
    "specName": "polkadot",
    "implName": "parity-polkadot",
    "authoringVersion": 0,
    "specVersion": 9220,
    "implVersion": 0,
    "apis": [...],
    "transactionVersion": 12,
    "stateVersion": 0
  },
  "compatible": false
}

cc @kianenigma @PierreBesson @ggwpez

chevdor · 2022-06-05T17:15:44Z

Rebased on master

ggwpez

Only some small things 👍
@kianenigma may want to take a look as well.

utils/staking-miner/src/info.rs

utils/staking-miner/src/main.rs

utils/staking-miner/src/opts.rs

chevdor · 2022-06-06T14:46:16Z

@PierreBesson does the current impl. work for you ?

PierreBesson · 2022-06-06T15:38:44Z

Yes it looks good to me.

niklasad1 · 2022-06-09T09:27:59Z

utils/staking-miner/src/main.rs

+			Command::Info(_info_opts) => {
+				let runtime_version: RuntimeVersion = rpc.runtime_version(None).await.expect("runtime_version infallible; qed.");
+				let info = Info::new(&runtime_version);
+				let info = serde_json::to_string_pretty(&info).expect("Failed serializing version info");


implement display on Info directly would be better no need to serialize it as JSON.

EDIT: ok, I see could be useful with JSON.

utils/staking-miner/src/info.rs

niklasad1 · 2022-06-09T09:36:01Z

utils/staking-miner/src/info.rs

+use sp_version::RuntimeVersion;
+
+#[derive(Debug, serde::Serialize)]
+pub(crate) struct Info {


Info is a bit vague, I would rather call it RuntimeVersionor something like that.

Even if it's doesn't have all the data that the RuntimeVersion from substrate.

Indeed, I wanted to it vague and extensible but I don't see an immediate need for that. I will make it more specific.

So I left the subcommand as info since it returns several information and no longer one version but internally the struct are now named more specifically.

utils/staking-miner/src/dry_run.rs

utils/staking-miner/src/main.rs

niklasad1

Cool, overall the code looks quite clean but I have some comments:

I propose to change --info to --runtime-version because it's too vague IMO.

In addition the entire point is to fetch the runtime version from a "specific hardcoded runtime" such as polkadot, kusama and westend which doesn't work when you just use the RuntimeVersion from the RPC node which may be different from what's used in the miner.

kianenigma

We need to get the linked version, not the one over RPC. Depending on your usecase, we might also need both? I am not sure. But the one over RPC can be fetched directly by contacting that node. No need to do that with staking miner. See here:

polkadot/utils/staking-miner/src/main.rs

Line 558 in ef57112

pub(crate) async fn check_versions<T: frame_system::Config + EPM::Config>(

chevdor · 2022-06-21T20:10:51Z

@PierreBesson Note that you may need to silence the logger if you want to use the json output from a script:

RUST_LOG=none c run -- info --json

Not doing so will output the following "json":

---> 2022-06-21T20:09:49.085595Z ERROR staking-miner: VERSION MISMATCH: any transaction will fail with bad-proof    
{
  "builtin": {
    ...
  },
  "remote": {
    ...
  }
}

fix #5560

chevdor · 2022-06-22T07:54:07Z

The initial impl. was rather naive as spec_name and spec_version solely are not enough to compare 2 runtimes. I have reworked this PR and that should be now not only correct this time ( cc @niklasad1 and @kianenigma ) but also more convenient ( cc @PierreBesson ).

So the changes are mainly:

better naming in the code, I left the command's name as info though
the info command now returns both runtime versions: linked and remote
the info command support regular text output by default but also --json for devops and automation
the output now contains the information whether the 2 runtimes are compatibles

utils/staking-miner/src/runtime_versions.rs

niklasad1

LGTM,

Fetching the remote version and comparing to linked version seems like a niche feature as it depends on the remote node but I reckon that it would be super useful for the devops deployment.

Would be good with some feedback from @PierreBesson regarding the output format 🙏

chevdor · 2022-06-22T08:10:02Z

Fetching the remote version and comparing to linked version seems like a niche feature as it depends on the remote node but I reckon that it would be super useful for the devops deployment.

Totally niche indeed, as this PR is. It is mainly to help the devops team run the right version of the staking-miner and check that it is (or not) still THE right version.

Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>

kianenigma · 2022-06-25T09:31:14Z

utils/staking-miner/src/runtime_versions.rs

+}
+
+#[derive(Debug, serde::Serialize)]
+pub(crate) struct RuntimeVersions<'a> {


@niklasad1 would be good if we backport this clean approach to the new version :) nicely done @chevdor 💯

kianenigma

💯

kianenigma · 2022-06-25T09:31:49Z

Just needs to make the CI green.

* master: zombienet: try to fix parachains upgrade test (#5724) Update dependencies (companion for substrate#11722) (#5731) Update metric name and doc (#5716) Bump reqwest from 0.11.10 to 0.11.11 (#5732) add release-engineering to CI files' reviewers (#5733) Bump parity-scale-codec from 3.1.2 to 3.1.5 (#5720) Add checklist item (#5715) Fix 5560: add support for a new `staking-miner info` command (#5577) Bump `wasmtime` to 0.38.0 and `zstd` to 0.11.2 (companion for substrate#11720) (#5707) pvf: ensure enough stack space (#5712) Bump generic-array from 0.12.3 to 0.12.4 in /bridges/fuzz/storage-proof (#5648) pvf: unignore `terminates_on_timeout` test (#5722) Bump proc-macro2 from 1.0.39 to 1.0.40 (#5719) pass $COMPANION_OVERRIDES to check_dependent_project (#5708) Bump thread_local from 1.1.0 to 1.1.4 in /bridges/fuzz/storage-proof (#5687) Bump quote from 1.0.18 to 1.0.19 (#5700) Rococo: add new pallet-beefy-mmr API (companion for substrate#11406) (#5516) Update metric before bailing out (#5706) Add publish docker staking-miner (#5710)

* Refactoring opts out * Implement info command fix #5560 * remove useless change * Remove unnecessary brackets * Fix and add tests * Promote the uri flag to global * Ignore lint identity ops * Reverse adding #[allow(identity_op)] * Add cli test for the info command * Add licende headers and fix some grumbles * Add retrieval of the linked version and make the json output optional * Fix tests * Keep it generic and renamed builtin into linked * Rebase master * Add runtimes compatibility information * Silence erroneous warning about unsafe * Fix spellcheck * Update utils/staking-miner/src/runtime_versions.rs Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>

chevdor added A0-please_review Pull request needs code review. B1-releasenotes C1-low PR touches the given topic and has a low impact on builders. labels May 20, 2022

chevdor requested review from kianenigma and ggwpez May 20, 2022 16:28

github-actions bot added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels May 20, 2022

chevdor requested a review from PierreBesson May 20, 2022 16:34

chevdor marked this pull request as ready for review May 20, 2022 16:52

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 May 20, 2022

chevdor force-pushed the wk-fix-5560 branch 2 times, most recently from c0a0173 to eb54f0e Compare June 5, 2022 17:15

ggwpez approved these changes Jun 5, 2022

View reviewed changes

chevdor force-pushed the wk-fix-5560 branch from 713f1ba to d842294 Compare June 6, 2022 14:43

niklasad1 reviewed Jun 9, 2022

View reviewed changes

utils/staking-miner/src/info.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Jun 9, 2022

View reviewed changes

utils/staking-miner/src/dry_run.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Jun 9, 2022

View reviewed changes

utils/staking-miner/src/main.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Jun 9, 2022

View reviewed changes

kianenigma suggested changes Jun 11, 2022

View reviewed changes

chevdor added 4 commits June 22, 2022 00:38

Refactoring opts out

713eb8c

Implement info command

69ff865

fix #5560

fmt

acfe014

remove useless change

b5ccadd

chevdor added 6 commits June 22, 2022 00:40

Add licende headers and fix some grumbles

bda111a

Add retrieval of the linked version and make the json output optional

1f998ef

Cleanup

6200a7b

Fix tests

d1ed2d3

Keep it generic and renamed builtin into linked

a3783ae

Rebase master

4a38ea6

chevdor force-pushed the wk-fix-5560 branch from 66ad5da to 4a38ea6 Compare June 21, 2022 22:51

Add runtimes compatibility information

94b16e8

chevdor requested review from kianenigma and niklasad1 June 22, 2022 07:54

niklasad1 reviewed Jun 22, 2022

View reviewed changes

utils/staking-miner/src/runtime_versions.rs Outdated Show resolved Hide resolved

niklasad1 approved these changes Jun 22, 2022

View reviewed changes

chevdor and others added 4 commits June 22, 2022 13:00

Silence erroneous warning about unsafe

9f421eb

Cleanup

bcd461a

Fix spellcheck

d8d1423

Update utils/staking-miner/src/runtime_versions.rs

e59cfa2

Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>

kianenigma reviewed Jun 25, 2022

View reviewed changes

kianenigma approved these changes Jun 25, 2022

View reviewed changes

kianenigma added the D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit. label Jun 25, 2022

chevdor added 2 commits June 25, 2022 13:44

Fix test

ed4f9b6

fmt

d7d1540

chevdor merged commit 714c687 into master Jun 25, 2022

chevdor deleted the wk-fix-5560 branch June 25, 2022 14:46

bakhtin mentioned this pull request Jun 27, 2022

Update staking-miner positional args paritytech/helm-charts#102

Merged

librelois mentioned this pull request Jul 19, 2022

Update substrate/polkadot/cumulus from v0.9.23 to v0.9.26 moonbeam-foundation/moonbeam#1691

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix 5560: add support for a new `staking-miner info` command #5577

Fix 5560: add support for a new `staking-miner info` command #5577

chevdor commented May 20, 2022 •

edited

Loading

chevdor commented Jun 5, 2022

ggwpez left a comment

chevdor commented Jun 6, 2022

PierreBesson commented Jun 6, 2022

niklasad1 Jun 9, 2022 •

edited

Loading

niklasad1 Jun 9, 2022

chevdor Jun 21, 2022

chevdor Jun 21, 2022

niklasad1 left a comment •

edited

Loading

kianenigma left a comment

chevdor commented Jun 21, 2022 •

edited

Loading

chevdor commented Jun 22, 2022

niklasad1 left a comment

chevdor commented Jun 22, 2022

kianenigma Jun 25, 2022

kianenigma left a comment

kianenigma commented Jun 25, 2022

Fix 5560: add support for a new staking-miner info command #5577

Fix 5560: add support for a new staking-miner info command #5577

Conversation

chevdor commented May 20, 2022 • edited Loading

Summary

Warning breaking change (cli)

help

New info command:

Output (human, default)

Output (--json)

chevdor commented Jun 5, 2022

ggwpez left a comment

Choose a reason for hiding this comment

chevdor commented Jun 6, 2022

PierreBesson commented Jun 6, 2022

niklasad1 Jun 9, 2022 • edited Loading

Choose a reason for hiding this comment

niklasad1 Jun 9, 2022

Choose a reason for hiding this comment

chevdor Jun 21, 2022

Choose a reason for hiding this comment

chevdor Jun 21, 2022

Choose a reason for hiding this comment

niklasad1 left a comment • edited Loading

Choose a reason for hiding this comment

kianenigma left a comment

Choose a reason for hiding this comment

chevdor commented Jun 21, 2022 • edited Loading

chevdor commented Jun 22, 2022

niklasad1 left a comment

Choose a reason for hiding this comment

chevdor commented Jun 22, 2022

kianenigma Jun 25, 2022

Choose a reason for hiding this comment

kianenigma left a comment

Choose a reason for hiding this comment

kianenigma commented Jun 25, 2022

Fix 5560: add support for a new `staking-miner info` command #5577

Fix 5560: add support for a new `staking-miner info` command #5577

chevdor commented May 20, 2022 •

edited

Loading

New `info` command:

Output (`--json`)

niklasad1 Jun 9, 2022 •

edited

Loading

niklasad1 left a comment •

edited

Loading

chevdor commented Jun 21, 2022 •

edited

Loading