Forward-port: update state to prevote `nil` when proposal block does not match locked block by sergio-mena · Pull Request #1203 · cometbft/cometbft

sergio-mena · 2023-08-03T21:05:09Z

Contributes to #1171

Mechanical forward-port. For a thorough explanation of the contents (and context) of this PR, please check the original PR (tendermint/tendermint#6986) and original issue (tendermint/tendermint#6850).

N.B. The PR is currently open on a temporary base branch to only display the changes proper to the forward-port. When #1175 is merged, the base branch is to be changed to main (@BrendanChou)

PR checklist

Tests written/updated
Changelog entry added in .changelog (we use unclog to manage our changelog)
Updated relevant documentation (docs/ or spec/) and code comments

lasarojc

LGTM

consensus/state.go

hvanz · 2023-08-07T09:54:35Z

consensus/state.go

+			cs.signAddVote(cmtproto.PrevoteType, cs.ProposalBlock.Hash(), cs.ProposalBlockParts.Header())
+			return
+		}
+	}


The "else" branch of the condition in line 23 of the pseudo code is missing. If cs.Proposal.POLRound is still -1 at this point, the line below to fetch blockID will not behave as expected.

hvanz · 2023-08-07T10:11:29Z

consensus/state.go

+		missed the proposal in round 'v_r'.
+	*/
+	blockID, ok := cs.Votes.Prevotes(cs.Proposal.POLRound).TwoThirdsMajority()
+	if ok && cs.ProposalBlock.HashesTo(blockID.Hash) && cs.Proposal.POLRound >= 0 && cs.Proposal.POLRound < cs.Round {


Shouldn't we also check that !blockID.IsNil()?

Indeed. We also need to change TestVoteSet_2_3Majority. Bear with me

...although HashesTo will do the right thing if passed a nil block.

Nevertheless, adding your suggestion because it increases readability.

hvanz · 2023-08-07T13:53:01Z

consensus/state.go

+		}
+	}
+
+	logger.Debug("prevote step: ProposalBlock is valid but was not our locked block or" +


What should happen at this point when !ok or blockID.IsNil()? According to the paper, the rule at line 28 should not apply, so here we should probably not vote anything (and log some error message).

My understanding of function defaultDoPrevote is that it will be called when we are ready to send our prevote:

either because we detected a majority (outside defaultDoPrevote),

or because we timed out

hence, we will prevote nil, rather than keep waiting.

Although I'm not 100% sure of this, I should probably examine all call sites.

I have the same understanding.
If the timeout hit before having the full proposal, prevote nil at the beginning.
If the timeout hit before having a majority, prevote nil here.

Btw, in state.go, update the comment // Prevote for ~~LockedBlock if we're locked, or ~~ProposalBlock if valid.

Btw, in state.go, update the comment // Prevote for ~LockedBlock if we're locked, or ~ProposalBlock if valid.

But we're doing it already, aren't we?

This part of the comment is no longer true Prevote for LockedBlock if we're locked; this was the first if that you removed.

Pls go ahead with the merge

…atch locked block. (#6986) * add failing test * tweak comments in failing test * failing test comment * initial attempt at removing prevote locked block logic * comment out broken function * undo reset on prevotes * fixing TestProposeValidBlock test * update test for completed POL update * comment updates * further unlock testing * update comments * Update internal/consensus/state.go * spacing nit * comment cleanup * nil check in addVote * update unlock description * update precommit on relock comment * add ensure new timeout back * rename IsZero to IsNil and replace uses of block len check with helper * add testing.T to new assertions * begin removing unlock condition * fix TestStateProposerSelection2 to precommit for nil correctly * remove erroneous sleep * update TestStatePOL comment * update relock test to be more clear * add _ into test names * rename slashing * udpate no relock function to be cleaner * do not relock on old proposal test cleanup * con state name update * remove all references to unlock * update test comments to include new * add relock test * add ensureRelock to common_test * remove all event unlock * remove unlock checks * no lint add space * lint ++ * add test for nil prevote on different proposal * fix prevote nil condition * fix defaultDoPrevote * state_test.go fixes to accomodate prevoting for nil * add failing test for POL from previous round case * update prevote logic to prevote POL from previous round * state.go comment fixes * update validatePrevotes to correctly look for nil * update new test name and comment * update POLFromPreviousRound test * fixes post merge * fix spacing * make the linter happy * change prevote log message * update prevote nil debug line * update enterPrevote comment * lint * Update internal/consensus/state.go Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> * Update internal/consensus/state.go Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> * add english description of alg rules * Update internal/consensus/state.go Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> * comment fixes from review * fix comment * fix comment Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>

hvanz

LGTM

consensus/state.go

Co-authored-by: Hernán Vanzetto <15466498+hvanz@users.noreply.github.com>

…metbft#2218) This bug was possibly introduced by cometbft#1203. This PR refactored the consensus operation, propose -> prevote transition (`defaultDoPrevote` method), to match the pseudo-code. The refactoring assumed that when `cs.ProposalBlock` is unset (nil), so should be `cs.Proposal` (meaning that the node didn't receive the round proposal). In this case, the node must prevote nil. The added test unit describes a scenario where `cs.Proposal` is nil (not received), but the block ` cs.ProposalBlock` is received because the node sees a Polka for a block, then receives the full associated block. The node must prevote nil in this scenario, in line with cometbft#1203. But the lack of the test on `cs.Proposal` leads to a bug. This bug does not affect previous releases except for `v1.0`. --- - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments - [ ] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

…not match locked block (cometbft#1203) * consensus: update state to prevote nil when proposal block does not match locked block. (#6986) * add failing test * tweak comments in failing test * failing test comment * initial attempt at removing prevote locked block logic * comment out broken function * undo reset on prevotes * fixing TestProposeValidBlock test * update test for completed POL update * comment updates * further unlock testing * update comments * Update internal/consensus/state.go * spacing nit * comment cleanup * nil check in addVote * update unlock description * update precommit on relock comment * add ensure new timeout back * rename IsZero to IsNil and replace uses of block len check with helper * add testing.T to new assertions * begin removing unlock condition * fix TestStateProposerSelection2 to precommit for nil correctly * remove erroneous sleep * update TestStatePOL comment * update relock test to be more clear * add _ into test names * rename slashing * udpate no relock function to be cleaner * do not relock on old proposal test cleanup * con state name update * remove all references to unlock * update test comments to include new * add relock test * add ensureRelock to common_test * remove all event unlock * remove unlock checks * no lint add space * lint ++ * add test for nil prevote on different proposal * fix prevote nil condition * fix defaultDoPrevote * state_test.go fixes to accomodate prevoting for nil * add failing test for POL from previous round case * update prevote logic to prevote POL from previous round * state.go comment fixes * update validatePrevotes to correctly look for nil * update new test name and comment * update POLFromPreviousRound test * fixes post merge * fix spacing * make the linter happy * change prevote log message * update prevote nil debug line * update enterPrevote comment * lint * Update internal/consensus/state.go Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> * Update internal/consensus/state.go Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> * add english description of alg rules * Update internal/consensus/state.go Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> * comment fixes from review * fix comment * fix comment Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> * Fix UTs * Addressed comments * Add changelog * Update consensus/state.go Co-authored-by: Hernán Vanzetto <15466498+hvanz@users.noreply.github.com> --------- Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> Co-authored-by: Hernán Vanzetto <15466498+hvanz@users.noreply.github.com>