Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[docs]: New configuration reference #397
base: main
Are you sure you want to change the base?
[docs]: New configuration reference #397
Changes from 36 commits
15d9ea4
6b17267
d13775b
793959a
122f05f
c472637
6bd761d
15f8e10
6d39164
6a9e6dd
1d091a7
29736bd
c88c48e
17fc897
e123ac0
f6bb470
e2ecaa7
6786935
ce9bf11
5828496
134a275
de585ca
4bafdab
cd0cede
1fe2a67
cbe8885
13fe5ce
5e2265a
eff21b2
18b614b
32008ad
2adbbf9
cfabb36
6d88337
c9e4fcf
07fd463
60edf4e
a0bc81f
b1c7ef8
12a026d
2db62aa
08532e0
bb938ef
35a0974
5b3dd60
0efcf1c
ea286e6
9dd1a49
76b850c
38dd466
6688e80
d5d1156
e9aa302
fc9bd81
2750023
df7415b
bde027d
b86a04c
5e5582a
b9177e7
0d949c6
f74845e
99698dd
7d9c926
128e675
2be984d
4c78fe2
8841a0b
3d26c72
c704b6f
a96fa91
b21c68e
cfe5ae4
dfd5498
ca2f813
eb231f3
aae25ba
9fe82b7
1e3b453
39cfe97
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use in case you pick my suggestion below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any requirements for the key?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should add some links to the source of those keys:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kagami crypto
might generate a usable key pairThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that we should mention keys generation and Kagami in the configuration reference. There are many other places in the docs which mention keys, and explaining the same things again and again might be ridiculous and inconsistent. I would like to have a single "Keys 101" section which we could reference from everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I haven't heard about that part before, so any explanation would be helpful. I wonder who can help?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder why the explanation of a config parameter goes after its specifications (type, values, default value, etc.). Why not change this order?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no objections, and you probably understand better how it would be more comprehensible. Should I change the order everywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it seconds or as in the example below? What will actually be accepted by the
gossip_period
parameter?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gossip_period
accepts a String or a Number, and interprets it as a Duration. Duration is explained at the link. A question: would it be clearer to say "Type: Duration (String or Number)", or "Type: Duration, String or Number" or current "Type: String or Number, Duration"?For Duration types, I specify their "Default" values as a human readable string like "30 seconds" or "5 minutes".
In the examples, I specify any valid value. Here is a question: maybe, we should change example to show the default value for each config parameter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My last comment doesn't have a part of the message for some reason. I was actually interested in the terminology.
In the "Default" you have "10 seconds", while in the example you have "5 secs".
So, the question is: does a peer have to write out "seconds" or "secs", or just a plain number in the "gossip_period" field? Juding by your reply, it seems like peers can input minutes as well.
For instance, if I want the interval to be 3 minutes and 22 seconds, should I input "3 mins, 22 secs" or "202 secs", or any other variation?
I'd like to make it clear for the readers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, and it is why I put the link
[Duration](glossary#type-duration)
. That section explains all the possible inputs for durations.Should it be pointed out more explicitly in some way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What exactly does the warning say? What happens after the warning is printed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's come up with a line!
I want to remind that this configuration reference describes the configuration that doesn't exist yet. We are at the design stage, and we need to find the best ways to present configuration system to the users. It means that we might change the naming/structure/phrasing of anything if we decide it will be more convenient to users.
As for this topic, actually Iroha should fail to start with
--submit-genesis
arg, but withoutgenesis.private_key
config parameter. However, if there is no--submit-genesis
arg, butgenesis.private_key
is provided, what Iroha should do? Just print a warning and continue execution, or fail? @Mingela, your input might be valuable.@yamkovoy, for more context about what we are doing in this PR, please refer to the RFC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the private key is needed only when a flag is specified it feels like the values should be combined somehow, i.e.
--submit-genesis --private-key <key>
. I might have encountered flag dependencies where one couldn't be used without other but cannot find an example. So following my suggestion just--private-key <key>
must prompt an error and usage manual (likeusage: iroha [--submit-genesis --private-key <key>] .. etc
).Another idea came to my mind, what about removing
--submit-genesis
at all and treat presence of--private-key <key>
implicitly as it's intended to submit the genesis block?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private-key
was used for simplicity there, we should define a simpler name forgenesis.private_key
in that case, maybegenesis-key
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two parameters:
genesis.public_key
: used by all peersgenesis.private_key
: used only by the peer which submits the genesisNot sure how
genesis-key
(orgenesis.key
) might be used in this context.The downside of
iroha --submit-genesis --private-key <key>
: it is not good to provide sensitive information through CLI. Although it might be workarounded with ENV:--private-key $IROHA_PRIV_KEY
.Anyway, the solution of omitting
--submit-genesis
seems fine. The main downside of it as I see is that it might not be that obvious which peer submits the genesis, and the user must know the details of how configuration works.Still, if we want to choose the path of the highest reliability for the end user, I think the current approach is the best. You clearly identify your intention with
--submit-genesis
flag. Iroha will clearly fail if you forgot to providegenesis.private_key
(orGENESIS_PRIVATE_KEY
var (naming is up to discussion)). In this case, the question becomes: should Iroha fail or warn if the key is provided, but the intention was not given?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either is fine. Seems all options around the
--submit-genesis
have tradeoffs. I don't like the ability to have unused parameters specified, though a warning would be reasonable in that case. Let's hear opinions of others, @mversic @BAStos525There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We definitely should not provide
genesis.private.key
as shell parameter and it has to be hided somehow, e.g. env var. Currently, we have this block in peerconfig.json
file, and we pass these keys in plain text. Should it be hided somehow? Moreover, we don't separate this config file between peers, so it's the same for all peers. We manage genesis submitting only by--submit-genesis
forpeer-0
selection by script.So, there are two ways:
config.json
? Should it be hided and private key should be specified only for genesis peer-0 (different configs for peers then). And if it's necessary to use--submit-genesis
flag forpeer-0
if keys are already specified in config?--submit-genesis
flag and thisgenesis.private.key
field in config and apply genesis private key through ENV var or any kind of secret.The second item sounds a kinda better since it may be easier to hide a sensitive data.
As for should iroha if the keys are provided, but the intention was not given, I think it depends on the selected approach how to provide the private key. If through config and flag, so it means to have a more deployment routine to separate peers configs e.t.c. to avoid iroha fail. So, here warning sounds more reasonable. In the second option we could try to fail it in order do not spread sensitive data across peers where and if it's not necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had a discussion with the team and dove deeper into the topic of genesis. We agreed that the whole genesis approach as of now is somewhat weird and needs thoughtful revision. It will be done later (idk when, hopefully soon). As for configuration, I will preserve the current behavior without significant changes.
That is:
genesis.public_key
should be provided for all peers. Through config file or ENV var.genesis.private_key
should be provided for the peer with--submit-genesis
CLI flag.genesis.private_key
might be set through config file or ENV var.genesis.private_key
is provided, but--submit-genesis
is not, then Iroha prints a warning, but continues execution.--submig-genesis
is provided, butgenesis.private_key
is not, then Iroha fails with FATAL error.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--submit-genesis
is not a command, but a CLI (command-line interface) flag. It might be set or not.