Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1615 +/- ##
===========================================
+ Coverage 60.62% 63.67% +3.04%
===========================================
Files 115 103 -12
Lines 9971 9574 -397
===========================================
+ Hits 6045 6096 +51
+ Misses 3356 2912 -444
+ Partials 570 566 -4
|
| test: | ||
| @echo "--> Running go test" | ||
| @go test $(PACKAGES) | ||
| @GOCACHE=off go test $(PACKAGES) |
There was a problem hiding this comment.
haven't seen this before) do we really need it off?
There was a problem hiding this comment.
Yes it helps catch nondeterministic test failures.
| cfg.PeerGossipSleepDuration = 5 | ||
| cfg.PeerQueryMaj23SleepDuration = 250 | ||
|
|
||
| // 1 second so we actually test the iota function. |
| if b.TotalTxs != s.LastBlockTotalTx+newTxs { | ||
| return fmt.Errorf("Wrong Block.Header.TotalTxs. Expected %v, got %v", s.LastBlockTotalTx+newTxs, b.TotalTxs) | ||
| } | ||
| if !b.Time.After(s.LastBlockTime) { |
There was a problem hiding this comment.
shouldn't we include blockTimeIota here too?
There was a problem hiding this comment.
yes we should. Need to put iota in consensusparams so we can deal with updating params.
| } | ||
| } | ||
|
|
||
| if genDoc.GenesisTime.IsZero() { |
There was a problem hiding this comment.
I remember we needed this for an older version of Tendermint consensus that required synchrony. We shouldn't need this anymore.
| part2 = partSet.GetPart(1) | ||
| seenCommit1 = &types.Commit{Precommits: []*types.Vote{{Height: 10, | ||
| Timestamp: time.Now().UTC()}}} | ||
| Timestamp: time.Now().Round(0).UTC()}}} |
There was a problem hiding this comment.
maybe it would be better to define a function for this and forbid use of time.Now so no one accidentally forgets ? Something like types.Now()
There was a problem hiding this comment.
yeah that would work too.
| // MakeBlock builds a block with the given txs and commit from the current state. | ||
| func (s State) MakeBlock(height int64, txs []types.Tx, commit *types.Commit) (*types.Block, *types.PartSet) { | ||
| // build base block | ||
| func (s State) MakeBlock(height int64, txs []types.Tx, commit *types.Commit, blockTimeIota int) (*types.Block, *types.PartSet) { |
There was a problem hiding this comment.
This used to be a gross function with many many args. I had hoped that MakeBlock could more directly reflect the block structure - header, txs, commit. By moving it to be a method on the State, we achieved that. Ideally, we wouldn't add more args, and everything else would come in through the state. We did add Evidence to the block, but we just use the block.AddEvidence method for it. So ideally, we wouldn't have to pass blockTimeIota here.
Actually, I don't think blockTimeIota belong in ConsensusConfig - it is more like ConsensusParams! ConsensusConfig is not required to validate blocks, and nodes can tweak them subjectively. Not so for the ConsensusParams. Unlike the other timeout parameters, blockTimeIota is actually used in validating a block in the state package, so it can't be tweaked.
So I think we need to move blockTimeIota into ConsensusParams. That should also eliminate the need to pass it in to this method, since we have State.ConsensusParams.
As for the other blockTime params, my understanding is they do belong in ConsensusConfig.
There was a problem hiding this comment.
ConsensusConfig.Timeout*, MaxBlockSize* are maybe others are on the same level of configuration as blockTimeIota. It doesn't make sense to separate them, they're intertwined. We can make them all consensus params, that would be OK.
There was a problem hiding this comment.
But it would suck if people's apps messed up the params to the point that it breaks consensus. So maybe we should preserve a way to override these configs if they are present as overrides in consensus config.
| Header: &Header{ | ||
| Height: height, | ||
| Time: time.Now(), | ||
| Time: time.Now().Round(0), // Strip monotonic. |
There was a problem hiding this comment.
we use .UTC everywhere else ? Should just have types.Now()
There was a problem hiding this comment.
It should always be Round(0).UTC().
| Height int64 `json:"height"` | ||
| Time time.Time `json:"time"` | ||
| NumTxs int64 `json:"num_txs"` | ||
| Nonce string `json:"nonce"` |
There was a problem hiding this comment.
With the new monotonic time increment, it becomes nearly impossible to differentiate between different blocks by different proposers at different rounds... but we need to differentiate in our tests, or was it for debugging I forget. Either way it's cheap and it ensures uniqueness of proposals regardless of what info we decide to keep in the block (e.g. proposer in the header) so lets keep this in.
|
This is security critical for the Cosmos Hub launch. We should probably prioritize getting it merged and into the a testnet. |
|
From discussion the other week I think we're going to drop this approach and stick with the median of the timestamps in the votes. We still need to actually implement that tho .. |
|
So I think we should probably close this. But we should standardize our use of Time like is being done here (though using a function like We also need to implement the BFT we agreed on instead of this, as specified in the spec, ie. using the median of timestamps from the precommits. |
|
replaced by #2013 |
Solution for BFT time: #1146