Build genesis and add genesis cli flags#5
Conversation
gregcusack
force-pushed
the
build-genesis
branch
2 times, most recently
from
c14d31e to
65fe5b4
Compare
gregcusack
force-pushed
the
build-genesis
branch
from
65fe5b4 to
2b55941
Compare
gregcusack
force-pushed
the
build-genesis
branch
2 times, most recently
from
6812bdc to
f2c63b7
Compare
gregcusack
force-pushed
the
build-genesis
branch
from
f2c63b7 to
6217ab7
Compare
joncinque
left a comment
joncinque
left a comment
There was a problem hiding this comment.
Sorry for the late fly-by review, but a few questions / suggestions
| Arg::with_name("enable_warmup_epochs") | ||
| .long("enable-warmup-epochs") | ||
| .takes_value(true) | ||
| .possible_values(&["true", "false"]) | ||
| .default_value("true") | ||
| .help("Genesis config. enable warmup epoch. defaults to true"), |
There was a problem hiding this comment.
Nit: it's a bit of an anti-pattern to have a flag that also takes "true" or "false" -- if it's present, it's true!
There was a problem hiding this comment.
unclear why i wrote it this way. good catch
| Arg::with_name("slots_per_epoch") | ||
| .long("slots-per-epoch") | ||
| .takes_value(true) | ||
| .help("override the number of slots in an epoch"), |
There was a problem hiding this comment.
Since this flag can be omitted and doesn't have a default documented anywhere, can you say what it will be here?
There was a problem hiding this comment.
default is defined internally in genesis library and depends on cluster_type. But added a note here as well with those values.
| Arg::with_name("target_lamports_per_signature") | ||
| .long("target-lamports-per-signature") | ||
| .takes_value(true) | ||
| .help("Genesis config. target lamports per signature"), |
There was a problem hiding this comment.
Same here, can you specify the default?
| Arg::with_name("faucet_lamports") | ||
| .long("faucet-lamports") | ||
| .takes_value(true) | ||
| .help("Override the default 500000000000000000 lamports minted in genesis"), |
There was a problem hiding this comment.
Can you specify .default_value("500000000000000000")? then you'll be able to parse this more easily.
Even better, you can pass in &DEFAULT_FAUCET_LAMPORTS.to_string() to avoid the copying, but it does make this function a bit goofier
| Arg::with_name("max_genesis_archive_unpacked_size") | ||
| .long("max-genesis-archive-unpacked-size") | ||
| .takes_value(true) | ||
| .help("Genesis config. max_genesis_archive_unpacked_size"), |
There was a problem hiding this comment.
You can use the default value, similar to DEFAULT_FAUCET_LAMPORTS
| Arg::with_name("bootstrap_validator_sol") | ||
| .long("bootstrap-validator-sol") | ||
| .takes_value(true) | ||
| .help("Genesis config. bootstrap validator sol"), |
There was a problem hiding this comment.
Same here, you can have a default value with DEFAULT_BOOTSTRAP_NODE_SOL
| Arg::with_name("bootstrap_validator_stake_sol") | ||
| .long("bootstrap-validator-stake-sol") | ||
| .takes_value(true) | ||
| .help("Genesis config. bootstrap validator stake sol"), |
There was a problem hiding this comment.
And finally here, you can use DEFAULT_BOOTSTRAP_NODE_STAKE_SOL
There was a problem hiding this comment.
I will very likely forget to update this 😞 and it's currently out of date. Is there a way to move this functionality into solana-genesis or link to it from the solana repo?
Or maybe better, do the logic in some file here rather than relying on a bash script? You're pretty much just fetching files and outputting strings. That might be more portable.
Note: this isn't urgent
There was a problem hiding this comment.
ahh ok ya i can move this over to its own rust function or something. will add that.
| while let Some(token) = tokens_iter.next() { | ||
| args.push(token.to_string()); | ||
| // Find flag delimiters | ||
| if token.starts_with("--") { | ||
| for next_token in tokens_iter.by_ref() { | ||
| if next_token.starts_with("--") { | ||
| args.push(next_token.to_string()); | ||
| } else { | ||
| args.push(next_token.to_string()); | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Sorry if I'm being dense, but why is this logic needed? Shouldn't you be able to just take the Vec<String> after the whitespace has been removed?
There was a problem hiding this comment.
you are right. i had a different (ultimately wrong) understanding of how solana-genesis took in these commands. And when I figured out it was wrong, I didn't change this function. But even now realize this function just doesn't really do anything that isn't done by just taking the Vec.
3 participants
Summary of Changes
5th PR in a series of PRs that will build out the mongon testing framework for deploying validator clusters on Kubernetes
Previous PRs: