-
Notifications
You must be signed in to change notification settings - Fork 175
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
feat: Add prepare-genesis command #172
Conversation
NB the "osmosis one" link points at this PR, not Osmosis. |
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.
Looks good, just minor comments.
cmd.Flags().Uint32(flagMaxActiveValidators, 10, "Maximum number of validators.") | ||
// btccheckpoint flags | ||
cmd.Flags().Uint64(flagBtcConfirmationDepth, 6, "Confirmation depth for Bitcoin headers.") | ||
cmd.Flags().Uint64(flagBtcFinalizationTimeout, 20, "Finalization timeout for Bitcoin headers.") |
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.
Can you remind me what happens after the timeout?
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.
This is the w
finalization 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.
I know. I just always found the naming confusing, that one is called Depth and the other is called a Timeout. It sounds like "if finalization hasn't happened by this time then ???". IIUC reporters have w
time to report malfeasance, and if nothing happens then we consider things final. But why is that considered a "finalization timeout" and not a "finalization depth"? Like a "request timeout" is the time after which the request fails unless it succeeds earlier.
So I just wanted to know what happens after the timeout exactly. But maybe it's just a confusing name and that's all.
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.
Indeed. Maybe @KonradStaniec can shed some light behind the naming convention
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.
So for me it was also no really clear (as the name was changed by @SebastianElvis ) and I needed to consult Fisher about it.
From my talk with @fishermanymc, as I understand from point of view of babylon node this is indeed just finalisation parameter, but from point of view of monitor it is finalisation timeout i.e if babylon node do not receive any submissions in flagBtcFinalizationTimeout
time from epoch end, it means something fishy is going on (like there is no honest reporters) and monitor can sounds an alarm.
So maybe it would be better to keep it as FinalisationDepth
and expose separate query to get it as finalisation timeout ? As it seems this naming confused everyone who looked at it.
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.
Ah so there is more to it than what I thought. It's not just a checkpoint is final unless vigilantes report a conflict by the timeout, but there's an alarm unless vigilantes report at least one valid submission by the timeout in the first place. The name "submission report timeout" could capture it, but then it would lose the connection to finalization.
@@ -137,6 +137,7 @@ func initRootCmd(rootCmd *cobra.Command, encodingConfig params.EncodingConfig) { | |||
genutilcli.MigrateGenesisCmd(), | |||
genutilcli.GenTxCmd(app.ModuleBasics, encodingConfig.TxConfig, banktypes.GenesisBalancesIterator{}, app.DefaultNodeHome), | |||
genutilcli.ValidateGenesisCmd(app.ModuleBasics), | |||
PrepareGenesisCmd(app.DefaultNodeHome, app.ModuleBasics), |
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.
Above there is already genutilcli.InitCmd
which also creates genesis (and a bit more : Initialize private validator, p2p, genesis, and application configuration files
). Maybe we should have only one of those commands to avoid confusion for users ?
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.
Indeed, the init
command initializes everything. There are some issues with it though:
- It does not take into account our own genesis and only uses the default ones. If we want to parameterize genesis (especially useful for testnet), this would require us to wrap it.
- We will have to add all the accounts that receive funds during genesis into the codebase. In the case of an airdrop, this could lead to a file which is several 100K lines long.
Judging from this guide, I understand the process of running a validator as:
- Init the validator files through the
babylond init
command. - Download the genesis file and replace the one created by the
init
command. - Modify the configuration file if there's anything you want to change. Also, through an online source identify what is the consensus for the BTC related values in the config.
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.
Also this guide for running a full osmosis nodes also mentions downloading a genesis file through an external source.
This command is inspired by the osmosis one. Its aim is to be used to bootstrap a genesis file with provided parameters. When we launch our testnet/mainnet we will use this command to generate a genesis file according to our specifications and distribute it to validators/people joining the chain.