testnet: Add flag for time between blocks #134
Conversation
| @@ -215,6 +220,8 @@ func InitTestnet( | |||
| nodeConfig.Instrumentation.Prometheus = true | |||
| // Set the number of simultaneous connections to unlimited | |||
| nodeConfig.Instrumentation.MaxOpenConnections = 0 | |||
| // Time between blocks | |||
| nodeConfig.Consensus.TimeoutCommit = time.Second * time.Duration(timeBetweenBlocks) | |||
There was a problem hiding this comment.
This doesn't look to be in line with what setting is for when I read https://github.com/tendermint/tendermint/blob/v0.35.9/config/config.go#L1130-L1133
The setting is to allow some extra votes to arrive. You need +2/3 votes for a quorum, but waiting a little longer can gather more votes, which is nice. If you say you want blocks every 5 seconds, then you can't spend the entire 5 seconds waiting for new votes before starting your next round, to collect your votes.
Say it takes 2 seconds for a quorum to form on a proposed block, and you want to wait an extra 1 second before you start making the next block for more votes to arrive. Then you start making the new blocks, but you only have 2 seconds left before your target of 5 seconds is reached. If you waited 5 seconds, then it would be 7 second per block.
I also don't understand why you would want to tweak this setting all of a sudden.
There was a problem hiding this comment.
Yep, that's what I initially understood, but the Tendermint docs say that this configuration should be specified
cmd/babylond/cmd/testnet.go
Outdated
| @@ -127,6 +130,7 @@ Example: | |||
| cmd.Flags().String(server.FlagMinGasPrices, fmt.Sprintf("0.000006%s", sdk.DefaultBondDenom), "Minimum gas prices to accept for transactions; All fees in a tx must meet this minimum (e.g. 0.01photino,0.001stake)") | |||
| cmd.Flags().String(flags.FlagKeyringBackend, flags.DefaultKeyringBackend, "Select keyring's backend (os|file|test)") | |||
| cmd.Flags().String(flags.FlagKeyAlgorithm, string(hd.Secp256k1Type), "Key signing algorithm to generate keys for") | |||
| cmd.Flags().Uint64(flagTimeBetweenBlocks, 5, "Time between blocks in seconds.") | |||
There was a problem hiding this comment.
Maybe what you need to tweak is https://github.com/tendermint/tendermint/blob/v0.35.9/config/config.go#L1138-L1140
There was a problem hiding this comment.
Although I don't understand the default values at https://github.com/tendermint/tendermint/blob/v0.35.9/config/config.go#L1161-L1162
There was a problem hiding this comment.
There was a problem hiding this comment.
Oh I see it does recommend setting timeout_commit. How odd.
There was a problem hiding this comment.
Plus, if you set create_empty_blocks_interval to something other than the default (0), Tendermint will be creating empty blocks even in the absence of transactions every create_empty_blocks_interval. For instance, with create_empty_blocks = false and create_empty_blocks_interval = "30s", Tendermint will only create blocks if there are transactions, or after waiting 30 seconds without receiving any transactions.
That sounds good, though?
vitsalis
force-pushed
the
block-timeout
branch
from
c56cd88
to
429506a
Compare
|
Thanks for the suggestions @aakoshh ! Updated the code based on those. |
|
Blocks are still not getting produced every |
vitsalis
force-pushed
the
block-timeout
branch
from
429506a
to
7be13c5
Compare
vitsalis
requested review from
SebastianElvis,
gitferry and
aakoshh
and removed request for
aakoshh
|
@SebastianElvis @gitferry Reworked on this and added changes that allow us to specify the time between blocks. Tested with the local deployment and we can change the time between blocks. |
|
Seems the build fails due to integration tests. I rerun the job a couple of times, and this seems to be timing related. For example:
@SebastianElvis given that you reviewed many of the e2e tests and @KonradStaniec is missing right now, any thoughts? side note: This is not a requirement for the alpha testnet as this only affects the |
|
Debugged for a couple of times and the error persists 😅 Nevertheless, I found this in Tendermint's source code // NOTE: when modifying, make sure to update time_iota_ms genesis parameter
TimeoutCommit time.Duration `mapstructure:"timeout_commit"`where |
|
I see. time_iota_ms seems to define the minimum time between blocks, so I guess it should be unrelated. |
|
Finally fixed this 😆 Reduced the default time between blocks to avoid inconsistencies in integration tests. @KonradStaniec @SebastianElvis another look please? ( |
Co-authored-by: KonradStaniec <konrad.staniec@gmail.com>
No description provided.