consensus: Wait for proposal or timeout before prevote by milosevic · Pull Request #2540 · tendermint/tendermint

milosevic · 2018-10-04T12:59:26Z

Fix Receiving 2f+1 Prevotes before Proposal might affect termination #1745.
Improve tests.

codecov-io · 2018-10-09T11:38:37Z

Codecov Report

Merging #2540 into develop will decrease coverage by 0.14%.
The diff coverage is 89.65%.

@@             Coverage Diff             @@
##           develop    #2540      +/-   ##
===========================================
- Coverage    61.38%   61.24%   -0.15%     
===========================================
  Files          203      203              
  Lines        16776    16816      +40     
===========================================
+ Hits         10298    10299       +1     
- Misses        5609     5642      +33     
- Partials       869      875       +6

Impacted Files	Coverage Δ
state/metrics.go	`25% <ø> (ø)`	⬆️
p2p/metrics.go	`17.94% <ø> (ø)`	⬆️
node/node.go	`66.05% <0%> (ø)`	⬆️
config/config.go	`69.28% <100%> (ø)`	⬆️
state/execution.go	`75.45% <100%> (ø)`	⬆️
consensus/state.go	`75.64% <92%> (-1.35%)`	⬇️
consensus/reactor.go	`71.48% <0%> (-1.04%)`	⬇️
libs/db/util.go	`71.42% <0%> (+0.69%)`	⬆️
libs/db/remotedb/remotedb.go	`41.52% <0%> (+4.68%)`	⬆️

xla

Just some formatting nits.

xla · 2018-10-09T13:03:08Z

consensus/common_test.go

-func ensureNoNewStep(stepCh <-chan interface{}) {
+func ensureNoNewEventOnChannel(ch <-chan interface{}) {
+	ensureNoNewEvent(ch, ensureTimeout, "We should be stuck waiting, "+
+		"not receiving new event on the channel")


This breaking is a bit clunky, it would be better to use this pattern:

ensureNoNewEvent( ch, ensureTimeout, "We should be stuck waiting, not receiving new event on the channel", )

xla · 2018-10-09T13:04:06Z

CHANGELOG_PENDING.md

 - [node] \#2434 Make node respond to signal interrupts while sleeping for genesis time
 - [consensus] [\#1690](https://github.com/tendermint/tendermint/issues/1690) wait for
 timeoutPrecommit before starting next round
+- [consensus] [\#1745] (https://github.com/tendermint/tendermint/issues/1745) wait for 


The space between the ] and ( will not render the link correctly.

xla · 2018-10-09T13:04:29Z

consensus/common_test.go

+func ensureNoNewRoundStep(stepCh <-chan interface{}) {
 	ensureNoNewEvent(stepCh, ensureTimeout, "We should be stuck waiting, "+
-		"not moving to the next step")
+		"not receiving NewRoundStep event")


Same as above.

xla · 2018-10-09T13:04:40Z

consensus/common_test.go

+
+func ensureNoNewUnlock(unlockCh <-chan interface{}) {
+	ensureNoNewEvent(unlockCh, ensureTimeout, "We should be stuck waiting, "+
+		"not receiving Unlock event")


Same as above.

xla · 2018-10-09T13:04:52Z

consensus/common_test.go

 	timeoutDuration := time.Duration(timeout*5) * time.Nanosecond
 	ensureNoNewEvent(stepCh, timeoutDuration, "We should be stuck waiting, "+
-		"not moving to the next step")
+		"not receiving NewTimeout event")


Same as above.

xla · 2018-10-09T13:11:24Z

consensus/common_test.go


-func ensureNewEvent(ch <-chan interface{}, timeout time.Duration, errorMessage string) {
+func ensureNewEvent(ch <-chan interface{}, height int64, round int,
+	timeout time.Duration, errorMessage string) {


Should be broken up by param per line with trailing comma and newline behind the last one.

xla · 2018-10-09T13:11:52Z

consensus/common_test.go

+		if !ok {
+			panic(fmt.Sprintf("expected a *types.EventDataRoundState, "+
+				"got %v. wrong subscription channel?",
+				reflect.TypeOf(rs)))


Same as above.

ebuchman

Initial review - let's chat about it

ebuchman · 2018-10-11T14:19:22Z

consensus/common_test.go

-var config *cfg.Config              // NOTE: must be reset for each _test.go file
-var ensureTimeout = time.Second * 1 // must be in seconds because CreateEmptyBlocksInterval is
+var config *cfg.Config                     // NOTE: must be reset for each _test.go file
+var ensureTimeout = time.Millisecond * 100 // must be in seconds because CreateEmptyBlocksInterval is


Oh, what about the comment claiming this must be in seconds?

I guess it was because of this line

tendermint/consensus/mempool_test.go

Line 41 in 69ecda1

config.Consensus.CreateEmptyBlocksInterval = ensureTimeout

as it used to be

tendermint/consensus/mempool_test.go

Line 41 in 0c9c329

config.Consensus.CreateEmptyBlocksInterval = int(ensureTimeout.Seconds())

. Probably comment can be removed.

ebuchman · 2018-10-11T14:20:13Z

consensus/state.go

 		cs.enterPrecommit(ti.Height, ti.Round)
 	case cstypes.RoundStepPrecommitWait:
 		cs.eventBus.PublishEventTimeoutWait(cs.RoundStateEvent())
+		cs.enterPrecommit(ti.Height, ti.Round)


Weird that we can get here without having done enterPrecommit already

It's possible as you might receive 2/3+ Precommit any so you trigger timeout. In the meantime you don't receive Polka and timeoutPrevote hasn't expired, so you precommit nil before going to the next round.

ebuchman · 2018-10-11T14:21:05Z

consensus/state.go

 func (cs *ConsensusState) defaultDoPrevote(height int64, round int) {
 	logger := cs.Logger.With("height", height, "round", round)
+
+	//TODO: Remove this so it is aligned with spec!


Let's open an issue for it? This isn't a bug, afawk, right? Just an optimization that would complicate the spec/proofs?

There is already an issue open: #2529. It's a bug; we need to propose validBlock, not lockedBlock. I will remove TODO as there is issue open.

ebuchman · 2018-10-11T14:21:39Z

consensus/state.go

 			// NOTE: our proposal block may be nil or not what received a polka..
 			// TODO: we may want to still update the ValidBlock and obtain it via gossipping
-			if !blockID.IsZero() &&
+			if len(blockID.Hash) != 0 &&


Why this change?

Need to confirm these always match - a test was failing.

ebuchman · 2018-10-11T15:01:49Z

consensus/state.go

+
 		blockID, ok := precommits.TwoThirdsMajority()
-		if ok && len(blockID.Hash) != 0 {
-			// Executed as TwoThirdsMajority could be from a higher round


Isn't this comment still true?

We could maybe move comment before enterNewRound, L1656.

ebuchman · 2018-10-11T15:02:36Z

consensus/state.go

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


Why remove?

It's too early to Precommit at this point, as it would mean Precommiting nil. We wait either prevote based condition to be true or timeoutPrecommit to expire.

Right. I think we need to update the enter condition on enterPrecommit then which currently says:

// Enter: any +2/3 precommits for next round.

melekes

Great work 🍑

melekes · 2018-10-12T08:13:31Z

consensus/common_test.go

+			panic(fmt.Sprintf("expected height %v, got %v", height, blockHeader.Header.Height))
+		}
+		if !bytes.Equal(blockHeader.Header.Hash(), blockHash) {
+			panic(fmt.Sprintf("expected header %v, got %v", blockHash, blockHeader.Header.Hash()))


Shouldn't this be expected header's hash %X, got %X?

melekes · 2018-10-12T08:14:16Z

consensus/common_test.go

+	case <-time.After(ensureTimeout):
+		panic("Timeout expired while waiting for new activity on the channel")
+	case <-ch:
+		break


no need for break

melekes · 2018-10-12T08:21:54Z

consensus/state.go

-	if cs.Height != height || round < cs.Round || (cs.Round == round && cstypes.RoundStepPrecommitWait <= cs.Step) {
-		logger.Debug(fmt.Sprintf("enterPrecommitWait(%v/%v): Invalid args. Current step: %v/%v/%v", height, round, cs.Height, cs.Round, cs.Step))
+	if cs.Height != height || round < cs.Round || (cs.Round == round && cs.triggeredTimeoutPrecommit) {
+		logger.Debug(fmt.Sprintf("enterPrecommitWait(%v/%v): Invalid args. Current step: %v/%v/%v", height, round, cs.Height, cs.Round, cs.triggeredTimeoutPrecommit))


comment needs to be updated to reflect it's no longer about "step". maybe %v/%v (timeoutPrecommit: %v)

ebuchman

Looks good, thanks @milosevic . And thanks for the great update to the tests!

ebuchman · 2018-10-12T16:53:59Z

consensus/state.go

+		logger.Debug(
+			fmt.Sprintf(
+				"enterPrecommitWait(%v/%v): Invalid args. "+
+					"Current state is Height/Round: %v/%v/, triggeredTimeoutPrecommit:%v",


Maybe we should keep the Step in this output, for completeness/debugging.

ebuchman · 2018-10-12T17:05:42Z

consensus/state.go

+		// If +2/3 prevotes for *anything* for future round:
+		if cs.Round < vote.Round && prevotes.HasTwoThirdsAny() {
+			// Round-skip if there is any 2/3+ of votes ahead of us
+			cs.enterNewRound(height, vote.Round)


So the reason we don't enterPrevote or enterPrecommit here anymore is because we have to see the entire propose step through before prevoting, right?

Either we will:

receive the whole proposal, and then prevote block

receive +2/3-prevotes for nil, and then prevote nil

timeout propose, and then prevote nil

Whereas before, we just prevoted right away on seeing +2/3-prevote-any

In the spec, the second condition (receiving +2/3-prevotes for nil) is conditioned by step=prevote (which means a process always wait for the other two conditions to be true) and we precommit nil at that point. It is probably fine also to prevote nil upon receiving +2/3-prevotes for nil even if step != prevote, but we need to check the proofs for this.

ebuchman · 2018-10-12T17:07:28Z

consensus/state.go

+		if cs.Round < vote.Round && prevotes.HasTwoThirdsAny() {
+			// Round-skip if there is any 2/3+ of votes ahead of us
+			cs.enterNewRound(height, vote.Round)
+		} else if cs.Round == vote.Round && cstypes.RoundStepPrevote <= cs.Step { // current round


And the reason we add cstypes.RoundStepPrevote <= cs.Step and drop the enterPrevote from here is because

if cs.Step < RoundStepPrevote, we still have to wait out the full propose step, so +2/3-prevote-any can't trigger anything. Even if we receive +2/3-prevote-block, we still have to wait until addProposalBlock gets the last part and calls enterPrevote, or we finally timeoutPropose

if cs.Step >= RoundStepPrevote`, we've already prevoted

So now addVote will only call enterPrevote if it just received the last vote in a POL it was waiting for to complete a proposal

ebuchman · 2018-10-12T18:43:01Z

consensus/state.go

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


Right. I think we need to update the enter condition on enterPrecommit then which currently says:

// Enter: any +2/3 precommits for next round.

ebuchman · 2018-10-12T18:47:19Z

consensus/state_test.go

 	// wait for precommit
-	ensureNewVote(voteCh)
-
+	ensureVote(voteCh, height, round, types.VoteTypePrecommit)


maybe these should just be ensurePrevote and ensurePrecommit and we can drop the last arg

ebuchman

A few more notes based on discussion from the issue #1745

ebuchman · 2018-10-12T20:03:29Z

consensus/state.go

 			if prevotes.HasTwoThirdsMajority() {
 				cs.enterPrecommit(height, vote.Round)
-			} else {
-				cs.enterPrevote(height, vote.Round) // if the vote is ahead of us


@jaekwon was suggesting we call enterPropose here instead of enterPrevote. But since we already guarantee the round matches and the cs.Step >= PrevoteStep, we shouldn't need to.

ebuchman · 2018-10-12T20:11:00Z

consensus/state.go

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


Jae was suggesting we also call enterPropose and enterPrevoteWait here, instead of the enterPrecommit. Note enterNewRound calls enterPropose, so it shouldn't be needed. And we want to wait the whole proposal step before doing any prevoting, so I don't think we need the enterPrevoteWait here either (it will happen latter if it needs to).

But then calling enterPrecommitWait right away means we now only have TimeoutPrecommit to receive the proposal block. Is that right?

In this case, we could be in any step, and may even be jumping to a later round, but then we only give ourselves TimeoutPrecommit to receive the proposal block for that round.

But then calling enterPrecommitWait right away means we now only have TimeoutPrecommit to receive the proposal block. Is that right?
In this case, we could be in any step, and may even be jumping to a later round, but then we only give ourselves TimeoutPrecommit to receive the proposal block for that round.

That's why in Lemma 5 we need to assume that timeoutPropose(r) > 2Delta + timeoutPrecommit(r-1), i.e., timeoutPropose need to be big enough so we will wait for a process that jump to a higher round to receive Proposal. So we don't wait timeoutPrecommit to receive the proposal block. We wait timeoutPrecommit to set validBlock in the current round. And we wait timeoutPropose to receive the proposal, but there is dependency on the duration of timeoutPropose on timeoutPrecommit. Does this answer your question?

Addressed

zsystm · 2023-07-03T12:15:08Z

consensus/state.go

+					cs.enterNewRound(cs.Height, 0)
+				}
+			} else {
+				cs.enterPrecommitWait(height, vote.Round)


@milosevic @ebuchman
Hey guys. Late to follow up this code, but I have a question for line 1668.
Thanks in advance.

Why should we enter precommit wait? we already checked majority for nil so we don't have to wait because consensus already achieved for nil.
I think 1668 should be enterNewRound because it don't need to wait more precommit votes.

cometbft/cometbft#1431

milosevic requested review from ebuchman, melekes, xla and zramsay as code owners October 4, 2018 12:59

milosevic changed the base branch from master to develop October 4, 2018 12:59

milosevic force-pushed the zarko/1745-wait-for-proposal-or-timeout-before-prevote branch from 751cb37 to 1a5bbc4 Compare October 4, 2018 13:26

milosevic self-assigned this Oct 4, 2018

milosevic force-pushed the zarko/1745-wait-for-proposal-or-timeout-before-prevote branch from 1a5bbc4 to e2cd6b3 Compare October 4, 2018 13:34

ebuchman mentioned this pull request Oct 5, 2018

[WIP] Consensus liveness fix #2132

Closed

4 tasks

xla changed the title ~~WIP: Wait for proposal or timeout before prevote~~ [WIP] Wait for proposal or timeout before prevote Oct 8, 2018

xla changed the title ~~[WIP] Wait for proposal or timeout before prevote~~ [WIP] consensus: Wait for proposal or timeout before prevote Oct 8, 2018

milosevic force-pushed the zarko/1745-wait-for-proposal-or-timeout-before-prevote branch from e2cd6b3 to ba336b5 Compare October 9, 2018 11:30

milosevic force-pushed the zarko/1745-wait-for-proposal-or-timeout-before-prevote branch from ba336b5 to 2dfd595 Compare October 9, 2018 12:30

milosevic changed the title ~~[WIP] consensus: Wait for proposal or timeout before prevote~~ Wait for proposal or timeout before prevote Oct 9, 2018

Fix termination issues and improve tests

8284693

milosevic force-pushed the zarko/1745-wait-for-proposal-or-timeout-before-prevote branch from 2dfd595 to 8284693 Compare October 9, 2018 12:51

xla changed the title ~~Wait for proposal or timeout before prevote~~ consensus: Wait for proposal or timeout before prevote Oct 9, 2018

xla removed the request for review from zramsay October 9, 2018 12:59

xla previously requested changes Oct 9, 2018

View reviewed changes

milosevic force-pushed the zarko/1745-wait-for-proposal-or-timeout-before-prevote branch from 82235ba to 23999ca Compare October 11, 2018 13:45

ebuchman reviewed Oct 11, 2018

View reviewed changes

This was referenced Oct 11, 2018

consensus: reconcile conditions in handleTimeout and enterXxx #2613

Closed

Round synchronisation could be very slow #1496

Closed

melekes reviewed Oct 12, 2018

View reviewed changes

Improve formatting and tests based on reviewer feedback

6ec7076

milosevic force-pushed the zarko/1745-wait-for-proposal-or-timeout-before-prevote branch from 23999ca to 6ec7076 Compare October 12, 2018 11:55

Merge develop

8094436

milosevic force-pushed the zarko/1745-wait-for-proposal-or-timeout-before-prevote branch from 957c806 to 8094436 Compare October 12, 2018 12:54

jackzampolin mentioned this pull request Oct 12, 2018

0.26.0 Release #2624

Closed

7 tasks

ebuchman approved these changes Oct 12, 2018

View reviewed changes

ebuchman reviewed Oct 12, 2018

View reviewed changes

ebuchman merged commit 2363d88 into develop Oct 12, 2018

ebuchman deleted the zarko/1745-wait-for-proposal-or-timeout-before-prevote branch October 12, 2018 20:13

james-ray mentioned this pull request Oct 13, 2018

Fix add proposal block part #2591

Closed

4 tasks

zmanian mentioned this pull request Dec 12, 2018

Liveness failure on community testnet genki-3000 #3003

Closed

zsystm reviewed Jul 3, 2023

View reviewed changes

cason mentioned this pull request Oct 4, 2023

Upon receiving +2/3 nil precommits, the consensus state-machine does not proceed to the step Propose(H,R+1) as specified in the documentation, instead it waits for a timeout cometbft/cometbft#1431

Open

Conversation

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

Uh oh!

Uh oh!

codecov-io commented Oct 9, 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!

ebuchman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

milosevic Oct 11, 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!

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!

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!

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!

ebuchman 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!

milosevic commented Oct 4, 2018 •

edited

Loading

codecov-io commented Oct 9, 2018 •

edited

Loading

milosevic Oct 11, 2018 •

edited

Loading

ebuchman Oct 12, 2018 •

edited

Loading