Conversation
| timeoutCommit := 2 * time.Second | ||
| customCMTConfig := initCometBFTConfig(timeoutCommit) | ||
|
|
||
| err = os.Setenv("TACCHAIND_CONSENSUS_TIMEOUT_COMMIT", cast.ToString(timeoutCommit)) |
There was a problem hiding this comment.
nevermind! This is actually needed.
I'm adding it back.
Setting it only in the "initComet" function works only for first time setups, for already existing validators config is read from .tacchaind/config/config.toml and we must set the env var to override it.
reference:
https://github.com/cosmos/cosmos-sdk/blob/e265bb9b42308dc70743b6200a70db9aafb70527/server/util.go#L252 <- here the default config gets saved to config.toml if it's not existing - it's also funny because comet default is 1s but cosmos-sdk forces default to 5s
https://github.com/cosmos/cosmos-sdk/blob/e265bb9b42308dc70743b6200a70db9aafb70527/server/util.go#L277 <- here all the config passed to intercept gets overwritten with the ones found in node home
There was a problem hiding this comment.
makes sense, actually that's how i thought it works because its how it does for app.toml, in the code we set default 0utac, but can be overriden in init scripts
wonder if that is the case is it any different from what we currently do (setting the value in config.toml in our guides)? if the validators initialize their config files with default values (5s) then is setting it in code going to matter?
also wondering how TACCHAIND_CONSENSUS_TIMEOUT_COMMIT env is going to be picked up by the code?
There was a problem hiding this comment.
The env var is read by viper here https://github.com/cosmos/cosmos-sdk/blob/e265bb9b42308dc70743b6200a70db9aafb70527/server/util.go#L277 and has more priority over the config.toml file.
Additional settings of viper env vars are here https://github.com/cosmos/cosmos-sdk/blob/e265bb9b42308dc70743b6200a70db9aafb70527/server/util.go#L152
Validator can still override it by specifying the env var in their os, but nobody does that.
I use this same approach on juno network and we didn't had any issues so far.
A cleanest way could be to fork that function out of cosmos-sdk and replace the setting entirely, but overkill in my opinion.
There was a problem hiding this comment.
agreed it's overkill
can you please quickly explain for my understanding how viper will pick this specific env var? how does it understand what it means if we don't specify it anywhere?
meanwhile i found out that osmosis are possibly doing something similar, now trying to understand their approach (https://github.com/osmosis-labs/osmosis/blob/main/cmd/osmosisd/cmd/root.go#L519-L528)
There was a problem hiding this comment.
nevermind, looked at the code refs u shared, so basically it will auto map it to timeout_commit param in config.toml?
| // Enforce faster block times | ||
| // 2 seconds + 1 second tendermint = 3 second blocks | ||
| timeoutCommit := 2 * time.Second |
There was a problem hiding this comment.
if the following calculation is how cometbft works can we adjust for 2s block time?
There was a problem hiding this comment.
This setting only adjust the additional time validators wait before moving to next block after +2/3 of vp has committed.
Final block time depends on various factors, including other timeouts and network health.
This is the current configuration of tac:
timeout_propose = "3s"
timeout_prevote = "1s"
timeout_precommit = "1s"
timeout_commit = "2s"
if we sum all the timeouts we should expect 7s block time, but that's certanly not the case.
These timeouts can be "skipped" under certain conditions, in particular:
- Propose timeout is always skipped if the proposer is not offline and network is healthy, usually lasts something like 100ms
- Prevote & Precommit are skipped if majority of validators are agreeing and not voting "nil"
- Commit it's the only one always waited.
| Phase | Moves early if condition met? | Blocking? |
|---|---|---|
| Propose | ✅ Proposal received early | ❌ |
| Prevote | ✅ Moves to precommit on 2/3 prevotes "block" | ❌ |
| Precommit | ✅ Moves to commit on 2/3 precommits "block" | ❌ |
| Commit | ❌ Always waits timeout_commit | ✅ |
references:
https://github.com/cometbft/cometbft/blob/v0.38.17/docs/introduction/README.md#consensus-overview
https://github.com/cometbft/cometbft/blob/v0.38.17/docs/core/configuration.md#consensus-timeouts-explained
so yes, in theory if we reduce timeout commit even more, we should save 1 more second. Reducing timeout commit even more only penalize low-hardware validators, or can have some implication if for any reason we have to process a huge block
There was a problem hiding this comment.
I changed it to 1s
|
|
||
| // Enforce faster block times | ||
| // 2 seconds + 1 second tendermint = 3 second blocks | ||
| timeoutCommit := 2 * time.Second |
There was a problem hiding this comment.
can we add this as const in https://github.com/TacBuild/tacchain/blob/main/app/config.go please
remove os env var change timeout commit to 1s
|
hello @dimiandre thanks for your contribution! looks good to me i will try to push some guide/init.sh changes in your branch to adjust for this and will merge |
Thanks you! I'm doing some more testing meanwhile |
code cleanup better comments
|
didn't have permissions to push to your branch so opened PR targeting it (kintsugi-tech#1) @dimiandre |
chore: adjust scripts and guides
2 participants
This enforce 1s timeout commit regardless of validator configuration
non consensus breaking