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

staking-miner: remove need of a file to pass the seed #3680

Merged
merged 5 commits into from
Aug 27, 2021

Conversation

chevdor
Copy link
Contributor

@chevdor chevdor commented Aug 20, 2021

Currently the seed needs to be passed as a PathBuf pointing to a file containing the seed.

This PR allows to:

  • renames --account-seed into --seed
  • pass the seed directly as a string (--seed 0x1234...) ⚠️
  • pass the seed as string via ENV ( SEED=0x123... staking-miner dry-run) or via .env ℹ️
  • pass the node URI as ENV ( URI=wss://... SEED=0x123... staking-miner dry-run) ℹ️

⚠️ Not recommended for safety reasons
ℹ️ Make sure to always prepend your commands mentioning the SEED with a leading space

Those changes make it potentially safer and easier to package the staking miner and use it as a container (prepared in #3682)

@chevdor chevdor added A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). B1-releasenotes C1-low PR touches the given topic and has a low impact on builders. labels Aug 20, 2021
@chevdor chevdor marked this pull request as ready for review August 20, 2021 19:29
@chevdor chevdor added the A0-please_review Pull request needs code review. label Aug 20, 2021
utils/staking-miner/src/main.rs Show resolved Hide resolved
utils/staking-miner/src/main.rs Show resolved Hide resolved
utils/staking-miner/src/signer.rs Outdated Show resolved Hide resolved
@kianenigma
Copy link
Contributor

isn't it a bad practice to pass the seed like that to the command, since it will be saved in all sorts of history files etc.? This is why I initially pref red passing a file in.

Otherwise looks good to me.

@chevdor
Copy link
Contributor Author

chevdor commented Aug 23, 2021

isn't it a bad practice to pass the seed like that to the command, since it will be saved in all sorts of history files etc.? This is why I initially pref red passing a file in.

Totally!
The best recommandation is to pass an ENV, thus this PR. It may be useful however for testers and devs to have a simple way to pass the seed for now. If we want to really harden this, we could totally remove both short and long, effectively forcing the user to pass the seed as a SEED ENV var.

Note that the security still remains in the hand of the user, who can still call the staking miner with (⚠️ do NOT do the following at home, this is bad too!!):

SEED=0x1234... staking-miner ...

Effectively leaving the SEED in the history exactly like it happens calling staking-miner --seed 0x1234...

NOTE: There a little trick if you do not want to leave traces in your history though. The history can be totally disabled but that removes the convenience... or use:

 staking-miner --seed 0x1234... ...
^

instead of:

staking-miner --seed 0x1234... ...

(Notice the barely visible leading space).

I also added .env to the list of ignored files in the repo in this PR. That helps also regarding security as one can then create for dev purposes a .env file with the content:

SEED=0x1234

This file can be loaded manually or automatically. Making a call as staking-miner --uri ws://... dry-run possible, without having to pass the ENV at all.

Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

Okay, sounds good to me. Only note is that this will need to be redeployed after the first runtime release that contains this, since the flag has changed. But not a blocker, since a proper upgrade mechanism for staking miner does not exist and is purely manual for now.

@kianenigma kianenigma merged commit 948e0f1 into paritytech:master Aug 27, 2021
@chevdor chevdor deleted the wk-staking-miner branch August 27, 2021 10:25
ordian added a commit that referenced this pull request Aug 27, 2021
* master:
  staking-miner: remove need of a file to pass the seed (#3680)
  Companion for 9619 (Private Events) (#3712)
  Fix Try-Runtime  (#3725)
  XCM v2: Scripting, Query responses, Exception handling and Error reporting (#3629)
  Bump async-trait from 0.1.50 to 0.1.51 (#3721)
  allow some overhead in MERKLE_NODE_MAX_SIZE (#3724)
  Bump itertools from 0.10.0 to 0.10.1 (#3719)
  Bump tokio from 1.10.0 to 1.10.1 (#3717)
  Bump trybuild from 1.0.43 to 1.0.45 (#3713)
  Don't err on deactivated leaf during valiation. (#3708)
  Bump libc from 0.2.99 to 0.2.100 (#3703)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants