consensus: Wait timeout precommit before starting new round by milosevic · Pull Request #2493 · tendermint/tendermint

milosevic · 2018-09-26T12:16:50Z

Pull in ensureVote test function from #2132

Updated all relevant documentation in docs
Updated all code comments where relevant
Wrote tests
Updated CHANGELOG_PENDING.md

milosevic · 2018-09-26T12:22:07Z

Note that there is an open PR (#2132) that mentions this issue, but it's actually more related to #1745. I have pulled in ensureVote test function from #2132 (sorry for squashing it, needed to fix some stuff after merged #2132 here). #1745 will be addressed in the follow up PR so we can probably close #2132 for those two new PRs.

codecov-io · 2018-09-26T12:41:27Z

Codecov Report

Merging #2493 into develop will increase coverage by 0.01%.
The diff coverage is 83.33%.

@@             Coverage Diff             @@
##           develop    #2493      +/-   ##
===========================================
+ Coverage    61.29%   61.31%   +0.01%     
===========================================
  Files          202      202              
  Lines        16720    16658      -62     
===========================================
- Hits         10249    10214      -35     
+ Misses        5602     5582      -20     
+ Partials       869      862       -7

Impacted Files	Coverage Δ
p2p/peer.go	`62.73% <ø> (ø)`	⬆️
p2p/test_util.go	`60.15% <0%> (ø)`	⬆️
p2p/conn/connection.go	`79.71% <0%> (+0.28%)`	⬆️
config/config.go	`69.28% <100%> (ø)`	⬆️
consensus/state.go	`77.69% <100%> (+0.39%)`	⬆️
consensus/reactor.go	`73.9% <100%> (+0.77%)`	⬆️
libs/log/tmfmt_logger.go	`85.71% <0%> (-2.97%)`	⬇️
blockchain/pool.go	`65.73% <0%> (-0.7%)`	⬇️
libs/db/fsdb.go	`68.47% <0%> (+0.42%)`	⬆️
... and 3 more

Pull in ensureVote test function from #2132

xla

👍 🍡

xla · 2018-09-26T12:46:58Z

consensus/state_test.go

+	signAddVotes(cs1, types.VoteTypePrecommit, nil, types.PartSetHeader{}, vs2, vs3, vs4)
+
+	<-timeoutWaitCh
+	<-newRoundCh


It's probably a good idea to wrap these channel reads with timeouts to prevent the tests from hanging accidently and ease in debugging.

Good point. I will use the same pattern as in ensureX methods to read from round and timeout channels.

xla · 2018-09-26T13:27:36Z

consensus/state.go

+			if cs.config.SkipTimeoutCommit && precommits.HasAll() {
+				// if we have all the votes now,
+				// go straight to new round (skip timeout commit)
+				// cs.scheduleTimeout(time.Duration(0), cs.Height, 0, cstypes.RoundStepNewHeight)


Do we need to keep this commented code line around?

Probably not. @melekes @ebuchman?

Don't think so

xla · 2018-09-26T13:27:54Z

consensus/state.go

+				// cs.scheduleTimeout(time.Duration(0), cs.Height, 0, cstypes.RoundStepNewHeight)
+				cs.enterNewRound(cs.Height, 0)
 			}
+


Insignificant newline.

xla · 2018-09-26T13:29:36Z

consensus/common_test.go

+		}
+		vote := edv.Vote
+		if vote.Height != height {
+			panic(fmt.Sprintf("expected height %v, got %v", height, vote.Height))


Instead of panics this function should return errors, which makes it easier to report back in the tests.

melekes

👏

melekes

Good job! Would be good to test the type (edv, ok := v.(types.EventDataUnlock)) in functions like ensureNewUnlock, .. etc The same thing we do in ensureVote

melekes · 2018-10-01T09:02:04Z

consensus/common_test.go

+	errorMessage string) {
+	timer := time.NewTimer(timeout)
 	select {
 	case <-timer.C:


<-time.After(timeout)

melekes · 2018-10-01T09:04:45Z

consensus/common_test.go

+	voteType byte) {
 	timer := time.NewTimer(ensureTimeout)
 	select {
 	case <-timer.C:


<-time.After(ensureTimeout)

melekes · 2018-10-01T09:05:42Z

consensus/common_test.go

 		edv, ok := v.(types.EventDataVote)
 		if !ok {
-			panic(fmt.Sprintf("expected a *types.Vote, got %v. wrong subscription channel?",
+			t.Error(fmt.Sprintf("expected a *types.Vote, got %v. wrong subscription channel?",


t.Errorf(...)

ebuchman · 2018-10-03T13:52:47Z

consensus/state.go

-					// cs.scheduleTimeout(time.Duration(0), cs.Height, 0, cstypes.RoundStepNewHeight)
-					cs.enterNewRound(cs.Height, 0)
-				}
+		if ok && len(blockID.Hash) != 0 {


If len(blockID.Hash) == 0, don't we need to explicitly enterPrecommitWait?

We should always enterPrecommitWait upon receiving 2f+1 any precommits. We don't need to treat 2f+1 Precommit nil as a special case. We do it here:

tendermint/consensus/state.go

Lines 1657 to 1660 in bb8411b

} else if cs.Round <= vote.Round && precommits.HasTwoThirdsAny() {

cs.enterNewRound(height, vote.Round)

cs.enterPrecommit(height, vote.Round)

cs.enterPrecommitWait(height, vote.Round)

. Only in case we have 2f+1 precommits for some block we go straight to commit and we don't need to trigger timeout.

Ok right, it's taken care of by the else if. The first condition is only for +2/3 majority for a block hash. If there's a +2/3 majority for nil, it will be treated by the second condition. Thanks.

ebuchman · 2018-10-03T13:54:36Z

consensus/common_test.go

-	case <-stepCh:
-		panic("We should be stuck waiting, not moving to the next step")
+	case <-ch:
+		t.Error(errorMessage)


We probably want these to be panics for now until we build something more intelligent, otherwise we won't know which line in the actual test function caused the failure.

Or alternatively to the error check in the actual test and t.Fatalf there. I know some may be concerned about readability but it's a test after all and the execution should be clear.

For now, I would also say to leave panic. Those tests are mostly covering critical algorithm scenarios so it is good if they are readable. Tests will probably become shorter after consensus state refactor and then we can do it properly.

milosevic requested review from ebuchman, melekes and xla as code owners September 26, 2018 12:16

milosevic self-assigned this Sep 26, 2018

xla added this to the launch milestone Sep 26, 2018

xla changed the title ~~Wait timeout precommit before starting new round~~ consensus: Wait timeout precommit before starting new round Sep 26, 2018

xla added T:bug Type Bug (Confirmed) C:consensus Component: Consensus labels Sep 26, 2018

Disable transitioning to new round upon 2/3+ of Precommit nils

1d8f155

Pull in ensureVote test function from #2132

milosevic force-pushed the zarko/1690-wait-timeout-precommit-before-new-round branch from 6446be9 to 1d8f155 Compare September 26, 2018 12:42

xla approved these changes Sep 26, 2018

View reviewed changes

xla requested review from jaekwon and removed request for jaekwon September 26, 2018 13:30

melekes approved these changes Sep 27, 2018

View reviewed changes

milosevic force-pushed the zarko/1690-wait-timeout-precommit-before-new-round branch 4 times, most recently from 2e9e654 to 7a60808 Compare September 27, 2018 12:56

melekes approved these changes Oct 1, 2018

View reviewed changes

Add several ensureX test methods to wrap channel read with timeout

300330a

milosevic force-pushed the zarko/1690-wait-timeout-precommit-before-new-round branch from 7a60808 to 300330a Compare October 2, 2018 07:53

Merge with develop

bb8411b

milosevic force-pushed the zarko/1690-wait-timeout-precommit-before-new-round branch from 298f60b to bb8411b Compare October 2, 2018 08:26

melekes requested a review from jaekwon October 3, 2018 07:32

ebuchman reviewed Oct 3, 2018

View reviewed changes

Revert panic in tests

1e4df13

ebuchman merged commit 12675ec into develop Oct 4, 2018

ebuchman deleted the zarko/1690-wait-timeout-precommit-before-new-round branch October 4, 2018 13:37

This was referenced Oct 5, 2018

[WIP] Consensus liveness fix #2132

Closed

Do we break validBlock property if a process can leave round upon receiving 2f+1 Precommit nil messages? #1690

Closed

	} else if cs.Round <= vote.Round && precommits.HasTwoThirdsAny() {
	cs.enterNewRound(height, vote.Round)
	cs.enterPrecommit(height, vote.Round)
	cs.enterPrecommitWait(height, vote.Round)

Conversation

milosevic commented Sep 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

milosevic commented Sep 26, 2018

Uh oh!

codecov-io commented Sep 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

xla left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

melekes left a comment

Choose a reason for hiding this comment

Uh oh!

melekes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

milosevic Oct 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

milosevic commented Sep 26, 2018 •

edited

Loading

codecov-io commented Sep 26, 2018 •

edited

Loading

milosevic Oct 3, 2018 •

edited

Loading