add flag to bake non bootstrap validator stakes into genesis

[gregcusack]

Problem

There is currently no efficient way of to bake validator stakes into genesis without requiring the user to serialize and deserialize validator accounts into and out of a primordial-accounts-file file.

Summary of Changes

add --validator-accounts-file <path-to-file> cli arg that reads in validator accounts from a yaml file. These accounts and associated lamports are then baked into genesis.

validator-accounts-file format:

validator_accounts:
- balance_lamports: <balance-lamports-0>
  stake_lamports: <stake-lamports-0>
  identity_account: <identity-pubkey-0>
  vote_account: <vote-pubkey-0>
  stake_account: <stake-pubkey-0>
- balance_lamports: <balance-lamports-1>
  stake_lamports: <stake-lamports-1>
  identity_account: <identity-pubkey-1>
  vote_account: <vote-pubkey-1>
  stake_account: <stake-pubkey-1>
...
- balance_lamports: <balance-lamports-N>
  stake_lamports: <stake-lamports-N>
  identity_account: <identity-pubkey-N>
  vote_account: <vote-pubkey-N>
  stake_account: <stake-pubkey-N>

[CriesofCarrots]

I'm wondering if we want a more flexible interface here. I know you're reusing a lot of the --bootstrap-validator logic, and it is nice and clean. And I know it also matched the specific use-cases of /net and validator-lab. But the upshot is that this only enables two stake-levels of validators (bootstrap and internal).

What if internal_validator took something like this as input instead:

Arg::with_name("internal_validator")
    .number_of_values(5)
    .value_name("IDENTITY_PUBKEY VOTE_PUBKEY STAKE_PUBKEY IDENTITY_BALANCE_SOL STAKE_BALANCE_SOL")`

Oh, just saw the help text; it looks like you were originally headed that direction? https://github.com/anza-xyz/agave/pull/2704/files#diff-4ba9fb9faa2132f5a4b9dde7b311023011c423c34c21bebf6c8f6f638fae0bf3R321

[gregcusack]

I'm wondering if we want a more flexible interface here. I know you're reusing a lot of the --bootstrap-validator logic, and it is nice and clean. And I know it also matched the specific use-cases of /net and validator-lab. But the upshot is that this only enables two stake-levels of validators (bootstrap and internal).

What if internal_validator took something like this as input instead:

Arg::with_name("internal_validator")
    .number_of_values(5)
    .value_name("IDENTITY_PUBKEY VOTE_PUBKEY STAKE_PUBKEY IDENTITY_BALANCE_SOL STAKE_BALANCE_SOL")`

Oh, just saw the help text; it looks like you were originally headed that direction? https://github.com/anza-xyz/agave/pull/2704/files#diff-4ba9fb9faa2132f5a4b9dde7b311023011c423c34c21bebf6c8f6f638fae0bf3R321

omg i forgot to update the comment lol. and yesss I was. I was thinking it made it more confusing to have pubkeys and lamports in the same argument. But if we want this level of flexibility (sounds like we do), then it makes sense to add lamports to the end of that arg

[CriesofCarrots]

But if we want this level of flexibility (sounds like we do), then it makes sense to add lamports to the end of that arg

Alternately, we could read a yaml file that defines a list of validators with their keys and different funding levels. But I wasn't sure if this would be confusing alongside the primordial-accounts file, and/or harder to use than a long, multi-type CLI arg... What do you think?

[gregcusack]

But if we want this level of flexibility (sounds like we do), then it makes sense to add lamports to the end of that arg

Alternately, we could read a yaml file that defines a list of validators with their keys and different funding levels. But I wasn't sure if this would be confusing alongside the primordial-accounts file, and/or harder to use than a long, multi-type CLI arg... What do you think?

I do think that a yaml file is a little easier to manage. But, ya not sure it makes sense along side of primordial accounts file. I assume we do NOT want to change the functionality of the primordial-accounts file, right? Or would that be an option?

[CriesofCarrots]

I assume we do NOT want to change the functionality of the primordial-accounts file, right? Or would that be an option?

I'd say it's an option if we can do it in a backward compatible way; ie. still parse files written with the existing format. There are various ways I can think to do that. One way might be to replace Base64Account in the deserialization with an enum with Base64Account and some new validator-info type.

But I'm also not totally opposed to a 2nd yaml for validator defs. I think we could make it distinct enough and clear enough to work... This crate probably needs a readme to explain stuff like the primodial-accounts file syntax anyway.

[gregcusack]

I assume we do NOT want to change the functionality of the primordial-accounts file, right? Or would that be an option?

I'd say it's an option if we can do it in a backward compatible way; ie. still parse files written with the existing format. There are various ways I can think to do that. One way might be to replace Base64Account in the deserialization with an enum with Base64Account and some new validator-info type.

But I'm also not totally opposed to a 2nd yaml for validator defs. I think we could make it distinct enough and clear enough to work... This crate probably needs a readme to explain stuff like the primodial-accounts file syntax anyway.

i think the backwards compatibility makes the most sense since it's somewhat similar logic (Adding accounts into genesis). I'll take a stab at it.

[bw-solana]

@yihau - pardon my ignorance. How do we do this on invalidator today? Should we adopt what @gregcusack is doing? What does it buy us?

[yihau]

I hacked for solana-genesis to handle different stakes for validators. for now, all the stakes are the same, so it's quite easy to manage. and yeah, we do have some primordial-accounts-file in the genesis 🙈 I think this will be really useful if we need to assign different stakes again in the future

[gregcusack]

ya as @yihau mentioned, the main motivations are:

1. Bake validator stakes into genesis with different stake and account distributions
2. Remove the overhead of forcing the user to serialize and deserialize validator stake and vote account state if they want varying stake distributions

[gregcusack]

I'd say it's an option if we can do it in a backward compatible way; ie. still parse files written with the existing format. There are various ways I can think to do that. One way might be to replace Base64Account in the deserialization with an enum with Base64Account and some new validator-info type.

But I'm also not totally opposed to a 2nd yaml for validator defs. I think we could make it distinct enough and clear enough to work... This crate probably needs a readme to explain stuff like the primodial-accounts file syntax anyway.

I ended up going with option 2 here. I felt it would be more clear to separate these yaml files. added a README as discussed that briefly outlines the different cli flags for baking accounts into genesis

[CriesofCarrots]

Awesome, api looks great to me. Just a couple suggestions for naming things and docs.

[CriesofCarrots]

Suggested change

	## There are a few ways to bake validator accounts and staked into genesis:
	## There are a few ways to bake validator accounts and delegated stake into genesis:

[CriesofCarrots]

Suggested change

	The validator accounts file is an alternative to the primordial accounts file. The main goal with the validator accounts file is to:
	- Bake validator stakes into genesis with different stake and acount distributions
	- Remove the overhead of forcing the user to serialize and deserialize validator stake and vote account state if they want varying stake distributions - as required by (2)
	The main goal with the validator accounts file is to:
	- Bake validator stakes into genesis with different stake and account distributions
	- Remove the overhead of forcing the user to serialize and deserialize validator stake and vote account state, as required by a primordial accounts file.

[CriesofCarrots]

Suggested change

	The `data` portion of the yaml file holds BASE64 encoded data about the account. `data` can include both vote and stake account information.
	The `data` portion of the yaml file holds BASE64 encoded data about the account, which can be vote or stake account information.

[CriesofCarrots]

I think this struct needs a new name (and comment) now, right? There isn't any base64-encoded data.

[CriesofCarrots]

Suggested change

	/// A validator account where the data is encoded as a Base64 string.
	/// Includes the vote account and stake account.
	#[derive(Serialize, Deserialize, Debug)]
	pub struct Base64ValidatorAccount {
	/// Info needed to create a staked validator account, including relevant balances and vote- and stake-account addresses
	#[derive(Serialize, Deserialize, Debug)]
	pub struct StakedValidatorInfo {

Not totally set on this struct name, if you can think of something better. I originally had Data in there, but kind of confusing wrt account data.

[CriesofCarrots]

Suggested change

	.help("The location of identity, vote, and stake pubkeys and balances for validator accounts to bake into genesis")
	.help("The location of a file containing a list of identity, vote, and stake pubkeys and balances for validator accounts to bake into genesis")

[CriesofCarrots]

With the caveat that I haven't actually tried running this, LGTM!

[gregcusack]

With the caveat that I haven't actually tried running this, LGTM!

i have tested on distributed cluster + unit tests!