cs: check for SkipTimeoutCommit or wait timeout in handleTxsAvailable#3928
cs: check for SkipTimeoutCommit or wait timeout in handleTxsAvailable#3928
Conversation
Previously, if create_empty_blocks was set to false, TM sometimes could create 2 consecutive blocks without actually waiting for timeoutCommit (1 sec. by default). ``` I[2019-08-29|07:32:43.874] Executed block module=state height=47 validTxs=10 invalidTxs=0 I[2019-08-29|07:32:43.876] Committed state module=state height=47 txs=10 appHash=F88C010000000000 I[2019-08-29|07:32:44.885] Executed block module=state height=48 validTxs=2 invalidTxs=0 I[2019-08-29|07:32:44.887] Committed state module=state height=48 txs=2 appHash=FC8C010000000000 I[2019-08-29|07:32:44.908] Executed block module=state height=49 validTxs=8 invalidTxs=0 I[2019-08-29|07:32:44.909] Committed state module=state height=49 txs=8 appHash=8C8D010000000000 I[2019-08-29|07:32:45.886] Executed block module=state height=50 validTxs=2 invalidTxs=0 I[2019-08-29|07:32:45.895] Committed state module=state height=50 txs=2 appHash=908D010000000000 I[2019-08-29|07:32:45.908] Executed block module=state height=51 validTxs=8 invalidTxs=0 I[2019-08-29|07:32:45.909] Committed state module=state height=51 txs=8 appHash=A08D010000000000 ``` This commit fixes that by adding a check to handleTxsAvailable. Fixes #3908
or SkipTimeoutCommit=true && we DON'T have all the votes
|
|
|
Thumb up! Thanks |
by checking if we have LastCommit or not
Codecov Report
@@ Coverage Diff @@
## master #3928 +/- ##
=========================================
+ Coverage 66.91% 67.01% +0.1%
=========================================
Files 219 219
Lines 18478 18486 +8
=========================================
+ Hits 12364 12389 +25
+ Misses 5192 5177 -15
+ Partials 922 920 -2
|
consensus/state.go
Outdated
| now := tmtime.Now() | ||
| if cs.StartTime.After(now) { | ||
| timeoutCommit := cs.StartTime.Sub(now) | ||
| cs.scheduleTimeout(timeoutCommit, cs.Height, 0, cstypes.RoundStepNewHeight) |
consensus/state.go
Outdated
| } | ||
|
|
||
| now := tmtime.Now() | ||
| if cs.StartTime.After(now) { |
There was a problem hiding this comment.
Can we more concretely assert that we're in the timeout commit phase here using the state/step rather than the time?
| // we only need to do this for round 0 | ||
| cs.enterNewRound(cs.Height, 0) | ||
| cs.enterPropose(cs.Height, 0) | ||
|
|
There was a problem hiding this comment.
First thing we should check is if we're in the timeout commit period.
If so, and this stuff is true, go then go to the next round.
If so, and this stuff is not true, then schedule the round timeout.
Otherwise, go to the round.
|
|
||
| switch cs.Step { | ||
| case cstypes.RoundStepNewHeight: // timeoutCommit phase | ||
| if cs.needProofBlock(cs.Height) { |
There was a problem hiding this comment.
don't like that we're loading block meta here, but I guess it's fine for now
There was a problem hiding this comment.
Agree - why did we need to check if needProofBlock here? And what happened to checking SkipTimeoutCommit?
There was a problem hiding this comment.
needProofBlock to avoid race between this func and enterNewRound https://github.com/tendermint/tendermint/blob/master/consensus/state.go#L835
SkipTimeoutCommit I've realized that we don't need it here since
a) if we're in timeoutCommit phase, it means we haven't received all the votes (https://github.com/tendermint/tendermint/blob/master/consensus/state.go#L1574) and it's ok to wait
b) if we're in propose phase, we don't need to check SkipTimeoutCommit and can enterPropose as we do right now
| } | ||
|
|
||
| // +1ms to ensure RoundStepNewRound timeout always happens after RoundStepNewHeight | ||
| timeoutCommit := cs.StartTime.Sub(tmtime.Now()) + 1*time.Millisecond |
There was a problem hiding this comment.
we should use time.Now() here and when calculating StartTime to ensure monotonic time! tmtime strips monotonic component
Previously, if create_empty_blocks was set to false, TM sometimes could
create 2 consecutive blocks without actually waiting for timeoutCommit
(1 sec. by default).
This commit fixes that by adding a check to handleTxsAvailable.
Fixes #3908
Updated all relevant documentation in docs