From 65909a13d59896acbaa4f773f94ff84e4354b106 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Wed, 10 Jun 2020 11:13:38 +0400 Subject: [PATCH] consensus: stricter on LastCommitRound check (#4970) LastCommitRound should always be >= 0 for heights > 1. In State.updateToState, last precommit is computed only when round greater than -1 and has votes. But "LastCommit" is always updated regardless of the condition. If there's no last precommit, "LastCommit" is set to (*types.VoteSet)(nil). That's why "LastCommit" can be -1 for heights > 1. To fix it, only update State.RoundState.LastCommit when there is last precommit. Fixes #2737 Co-authored-by: Cuong Manh Le --- CHANGELOG_PENDING.md | 1 + consensus/reactor.go | 13 +++++++++---- consensus/reactor_test.go | 12 ++++++------ consensus/state.go | 33 ++++++++++++++++++++++++--------- consensus/types/round_state.go | 2 +- 5 files changed, 41 insertions(+), 20 deletions(-) diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index de693aa06..c1b3faa28 100644 --- a/CHANGELOG_PENDING.md +++ b/CHANGELOG_PENDING.md @@ -94,4 +94,5 @@ Friendly reminder, we have a [bug bounty program](https://hackerone.com/tendermi ### BUG FIXES: - [consensus] [\#4895](https://github.com/tendermint/tendermint/pull/4895) Cache the address of the validator to reduce querying a remote KMS (@joe-bowman) +- [consensus] \#4970 Stricter on `LastCommitRound` check (@cuonglm) - [blockchain/v2] Correctly set block store base in status responses (@erikgrinaker) diff --git a/consensus/reactor.go b/consensus/reactor.go index 9faea561f..b427dcec6 100644 --- a/consensus/reactor.go +++ b/consensus/reactor.go @@ -101,9 +101,14 @@ func (conR *Reactor) OnStop() { // It resets the state, turns off fast_sync, and starts the consensus state-machine func (conR *Reactor) SwitchToConsensus(state sm.State, skipWAL bool) { conR.Logger.Info("SwitchToConsensus") - conR.conS.reconstructLastCommit(state) - // NOTE: The line below causes broadcastNewRoundStepRoutine() to - // broadcast a NewRoundStepMessage. + + // We have no votes, so reconstruct LastCommit from SeenCommit. + if state.LastBlockHeight > 0 { + conR.conS.reconstructLastCommit(state) + } + + // NOTE: The line below causes broadcastNewRoundStepRoutine() to broadcast a + // NewRoundStepMessage. conR.conS.updateToState(state) conR.mtx.Lock() @@ -1438,7 +1443,7 @@ func (m *NewRoundStepMessage) ValidateBasic() error { // NOTE: SecondsSinceStartTime may be negative if (m.Height == 1 && m.LastCommitRound != -1) || - (m.Height > 1 && m.LastCommitRound < -1) { // TODO: #2737 LastCommitRound should always be >= 0 for heights > 1 + (m.Height > 1 && m.LastCommitRound < 0) { return errors.New("invalid LastCommitRound (for 1st block: -1, for others: >= 0)") } return nil diff --git a/consensus/reactor_test.go b/consensus/reactor_test.go index c57a6184c..adfdf4d8e 100644 --- a/consensus/reactor_test.go +++ b/consensus/reactor_test.go @@ -699,12 +699,12 @@ func TestNewRoundStepMessageValidateBasic(t *testing.T) { testName string messageStep cstypes.RoundStepType }{ - {false, 0, 0, 0, "Valid Message", 0x01}, - {true, -1, 0, 0, "Invalid Message", 0x01}, - {true, 0, 0, -1, "Invalid Message", 0x01}, - {true, 0, 0, 1, "Invalid Message", 0x00}, - {true, 0, 0, 1, "Invalid Message", 0x00}, - {true, 0, -2, 2, "Invalid Message", 0x01}, + {false, 0, 0, 0, "Valid Message", cstypes.RoundStepNewHeight}, + {true, -1, 0, 0, "Negative round", cstypes.RoundStepNewHeight}, + {true, 0, 0, -1, "Negative height", cstypes.RoundStepNewHeight}, + {true, 0, 0, 0, "Invalid Step", cstypes.RoundStepCommit + 1}, + {true, 0, 0, 1, "H == 1 but LCR != -1 ", cstypes.RoundStepNewHeight}, + {true, 0, -1, 2, "H > 1 but LCR < 0", cstypes.RoundStepNewHeight}, } for _, tc := range testCases { diff --git a/consensus/state.go b/consensus/state.go index 924ffbefa..7e8ce5d40 100644 --- a/consensus/state.go +++ b/consensus/state.go @@ -175,11 +175,16 @@ func NewState( cs.doPrevote = cs.defaultDoPrevote cs.setProposal = cs.defaultSetProposal + // We have no votes, so reconstruct LastCommit from SeenCommit. + if state.LastBlockHeight > 0 { + cs.reconstructLastCommit(state) + } + cs.updateToState(state) // Don't call scheduleRound0 yet. // We do that upon Start(). - cs.reconstructLastCommit(state) + cs.BaseService = *service.NewBaseService(nil, "State", cs) for _, option := range options { option(cs) @@ -517,18 +522,17 @@ func (cs *State) sendInternalMessage(mi msgInfo) { // Reconstruct LastCommit from SeenCommit, which we saved along with the block, // (which happens even before saving the state) func (cs *State) reconstructLastCommit(state sm.State) { - if state.LastBlockHeight == 0 { - return - } seenCommit := cs.blockStore.LoadSeenCommit(state.LastBlockHeight) if seenCommit == nil { panic(fmt.Sprintf("Failed to reconstruct LastCommit: seen commit for height %v not found", state.LastBlockHeight)) } + lastPrecommits := types.CommitToVoteSet(state.ChainID, seenCommit, state.LastValidators) if !lastPrecommits.HasTwoThirdsMajority() { panic("Failed to reconstruct LastCommit: Does not have +2/3 maj") } + cs.LastCommit = lastPrecommits } @@ -564,12 +568,24 @@ func (cs *State) updateToState(state sm.State) { // Reset fields based on state. validators := state.Validators - lastPrecommits := (*types.VoteSet)(nil) - if cs.CommitRound > -1 && cs.Votes != nil { + + switch { + case state.LastBlockHeight == 0: // Very first commit should be empty. + cs.LastCommit = (*types.VoteSet)(nil) + case cs.CommitRound > -1 && cs.Votes != nil: // Otherwise, use cs.Votes if !cs.Votes.Precommits(cs.CommitRound).HasTwoThirdsMajority() { - panic("updateToState(state) called but last Precommit round didn't have +2/3") + panic(fmt.Sprintf("Wanted to form a Commit, but Precommits (H/R: %d/%d) didn't have 2/3+: %v", + state.LastBlockHeight, + cs.CommitRound, + cs.Votes.Precommits(cs.CommitRound))) } - lastPrecommits = cs.Votes.Precommits(cs.CommitRound) + cs.LastCommit = cs.Votes.Precommits(cs.CommitRound) + case cs.LastCommit == nil: + // NOTE: when Tendermint starts, it has no votes. reconstructLastCommit + // must be called to reconstruct LastCommit from SeenCommit. + panic(fmt.Sprintf("LastCommit cannot be empty in heights > 1 (H:%d)", + state.LastBlockHeight+1, + )) } // Next desired block height @@ -601,7 +617,6 @@ func (cs *State) updateToState(state sm.State) { cs.ValidBlockParts = nil cs.Votes = cstypes.NewHeightVoteSet(state.ChainID, height, validators) cs.CommitRound = -1 - cs.LastCommit = lastPrecommits cs.LastValidators = state.LastValidators cs.TriggeredTimeoutPrecommit = false diff --git a/consensus/types/round_state.go b/consensus/types/round_state.go index 5d2bc9ca5..6fd674f73 100644 --- a/consensus/types/round_state.go +++ b/consensus/types/round_state.go @@ -84,7 +84,7 @@ type RoundState struct { ValidRound int32 `json:"valid_round"` ValidBlock *types.Block `json:"valid_block"` // Last known block of POL mentioned above. - // Last known block parts of POL metnioned above. + // Last known block parts of POL mentioned above. ValidBlockParts *types.PartSet `json:"valid_block_parts"` Votes *HeightVoteSet `json:"votes"` CommitRound int32 `json:"commit_round"` //