consensus: proposal#POLRound checking code might be incorrect

[melekes]

tendermint/consensus/state.go

Lines 1428 to 1432 in bbf15b3

	// Verify POLRound, which must be -1 or between 0 and proposal.Round exclusive.
	if proposal.POLRound != -1 &&
	(proposal.POLRound < 0 || proposal.Round <= proposal.POLRound) {
	return ErrInvalidProposalPOLRound
	}

Refs #2392 (comment)
In log-node2.txt I found

Okt 01 15:50:24 ip-172-31-9-150.us-east-2.compute.internal thorchaind[20170]: E[10-01|15:50:24.490] Error with msg                               module=consensus height=366511 round=0 type=*consensus.ProposalMessage peer= err="Error invalid proposal POL round" msg="[Proposal Proposal{366511/0 1:707EC619D399 (0,:0:000000000000) /EA1BAEEF4B7D.../ @ 2018-10-01T15:50:24.485Z}]"

Can't POLRound be 0? If not, what could've led the honest node to create such a proposal?

In any case, I'd prefer the above code to be rewritten:

	if !(proposal.POLRound == -1 ||
		(proposal.POLRound > 0 && proposal.POLRound < proposal.Round)) {
		return ErrInvalidProposalPOLRound
	}

[milosevic]

This condition is really hard to get. Initially, lockedRound and validRound are set to -1 as we start with round 0. If some process locks a block in round 0, then 0 is valid proposal.POLRound in rounds > 0. I would suggest small modifications to your proposal:

	if !(proposal.POLRound < -1 ||
		(proposal.POLRound >= 0 && proposal.POLRound >= proposal.Round)) {
		return ErrInvalidProposalPOLRound
	}

[melekes]

Note here

Okt 01 15:50:24 ip-172-31-9-150.us-east-2.compute.internal thorchaind[20170]: E[10-01|15:50:24.490] Error with msg                               module=consensus height=366511 round=0 type=*consensus.ProposalMessage peer= err="Error invalid proposal POL round" msg="[Proposal Proposal{366511/0 1:707EC619D399 (0,:0:000000000000) /EA1BAEEF4B7D.../ @ 2018-10-01T15:50:24.485Z}]"

proposal.Round (0) == proposal.POLRound (0)

[ebuchman]

	if !(proposal.POLRound < -1 ||
		(proposal.POLRound >= 0 && proposal.POLRound >= proposal.Round)) {
		return ErrInvalidProposalPOLRound
	}

This doesn't look right - wouldn't this return an error if POLRound == -1 ?

I think perhaps what's happening is that the proposer is receiving +2/3 prevotes for nil for the round before they even get a chance to make a proposal. So then POLInfo returns a 0 for POLRound since it's seen a +2/3 prevotes

[milosevic]

@ebuchman You are right, it returns error for POLRound == -1; the original condition is correct. Regarding your other comment, it will not be a concern after we modify Proposal to use ValidRound instead of computing POLInfo (#2646).
We could potentially change condition slightly to make it more explicit:

	if (proposal.POLRound < 0 && proposal.POLRound != -1)  || proposal.POLRound >= proposal.Round {
		return ErrInvalidProposalPOLRound
	}

[ebuchman]

Let's close this for #2646