From af8793c01a168f0f15d3147673c572c0f8bf7863 Mon Sep 17 00:00:00 2001 From: Zarko Milosevic Date: Mon, 18 Feb 2019 08:29:41 +0100 Subject: [PATCH] cs: reset triggered timeout precommit (#3310) * Reset TriggeredTimeoutPrecommit as part of updateToState * Add failing test and fix * fix DATA RACE in TestResetTimeoutPrecommitUponNewHeight ``` WARNING: DATA RACE Read at 0x00c001691d28 by goroutine 691: github.com/tendermint/tendermint/consensus.decideProposal() /go/src/github.com/tendermint/tendermint/consensus/common_test.go:133 +0x121 github.com/tendermint/tendermint/consensus.TestResetTimeoutPrecommitUponNewHeight() /go/src/github.com/tendermint/tendermint/consensus/state_test.go:1389 +0x958 testing.tRunner() /usr/local/go/src/testing/testing.go:827 +0x162 Previous write at 0x00c001691d28 by goroutine 931: github.com/tendermint/tendermint/consensus.(*ConsensusState).updateToState() /go/src/github.com/tendermint/tendermint/consensus/state.go:562 +0x5b2 github.com/tendermint/tendermint/consensus.(*ConsensusState).finalizeCommit() /go/src/github.com/tendermint/tendermint/consensus/state.go:1340 +0x141e github.com/tendermint/tendermint/consensus.(*ConsensusState).tryFinalizeCommit() /go/src/github.com/tendermint/tendermint/consensus/state.go:1255 +0x66e github.com/tendermint/tendermint/consensus.(*ConsensusState).enterCommit.func1() /go/src/github.com/tendermint/tendermint/consensus/state.go:1201 +0x135 github.com/tendermint/tendermint/consensus.(*ConsensusState).enterCommit() /go/src/github.com/tendermint/tendermint/consensus/state.go:1232 +0x94b github.com/tendermint/tendermint/consensus.(*ConsensusState).addVote() /go/src/github.com/tendermint/tendermint/consensus/state.go:1657 +0x132e github.com/tendermint/tendermint/consensus.(*ConsensusState).tryAddVote() /go/src/github.com/tendermint/tendermint/consensus/state.go:1503 +0x8f github.com/tendermint/tendermint/consensus.(*ConsensusState).handleMsg() /go/src/github.com/tendermint/tendermint/consensus/state.go:694 +0xa1e github.com/tendermint/tendermint/consensus.(*ConsensusState).receiveRoutine() /go/src/github.com/tendermint/tendermint/consensus/state.go:642 +0x948 github.com/tendermint/tendermint/consensus.(*ConsensusState).receiveRoutine() /go/src/github.com/tendermint/tendermint/consensus/state.go:642 +0x948 github.com/tendermint/tendermint/consensus.(*ConsensusState).receiveRoutine() /go/src/github.com/tendermint/tendermint/consensus/state.go:642 +0x948 github.com/tendermint/tendermint/consensus.(*ConsensusState).receiveRoutine() /go/src/github.com/tendermint/tendermint/consensus/state.go:655 +0x7dd github.com/tendermint/tendermint/consensus.(*ConsensusState).receiveRoutine() /go/src/github.com/tendermint/tendermint/consensus/state.go:642 +0x948 github.com/tendermint/tendermint/consensus.(*ConsensusState).receiveRoutine() /go/src/github.com/tendermint/tendermint/consensus/state.go:655 +0x7dd Goroutine 691 (running) created at: testing.(*T).Run() /usr/local/go/src/testing/testing.go:878 +0x659 testing.runTests.func1() /usr/local/go/src/testing/testing.go:1119 +0xa8 testing.tRunner() /usr/local/go/src/testing/testing.go:827 +0x162 testing.runTests() testing.(*M).Run() /usr/local/go/src/testing/testing.go:1034 +0x2ee main.main() _testmain.go:216 +0x332 ``` * fix another DATA RACE by locking consensus ``` WARNING: DATA RACE Read at 0x00c009b835a8 by goroutine 871: github.com/tendermint/tendermint/consensus.(*ConsensusState).createProposalBlock() /go/src/github.com/tendermint/tendermint/consensus/state.go:955 +0x7c github.com/tendermint/tendermint/consensus.decideProposal() /go/src/github.com/tendermint/tendermint/consensus/common_test.go:127 +0x53 github.com/tendermint/tendermint/consensus.TestResetTimeoutPrecommitUponNewHeight() /go/src/github.com/tendermint/tendermint/consensus/state_test.go:1389 +0x958 testing.tRunner() /usr/local/go/src/testing/testing.go:827 +0x162 Previous write at 0x00c009b835a8 by goroutine 931: github.com/tendermint/tendermint/consensus.(*ConsensusState).updateHeight() /go/src/github.com/tendermint/tendermint/consensus/state.go:446 +0xb7 github.com/tendermint/tendermint/consensus.(*ConsensusState).updateToState() /go/src/github.com/tendermint/tendermint/consensus/state.go:542 +0x22f github.com/tendermint/tendermint/consensus.(*ConsensusState).finalizeCommit() /go/src/github.com/tendermint/tendermint/consensus/state.go:1340 +0x141e github.com/tendermint/tendermint/consensus.(*ConsensusState).tryFinalizeCommit() /go/src/github.com/tendermint/tendermint/consensus/state.go:1255 +0x66e github.com/tendermint/tendermint/consensus.(*ConsensusState).enterCommit.func1() /go/src/github.com/tendermint/tendermint/consensus/state.go:1201 +0x135 github.com/tendermint/tendermint/consensus.(*ConsensusState).enterCommit() /go/src/github.com/tendermint/tendermint/consensus/state.go:1232 +0x94b github.com/tendermint/tendermint/consensus.(*ConsensusState).addVote() /go/src/github.com/tendermint/tendermint/consensus/state.go:1657 +0x132e github.com/tendermint/tendermint/consensus.(*ConsensusState).tryAddVote() /go/src/github.com/tendermint/tendermint/consensus/state.go:1503 +0x8f github.com/tendermint/tendermint/consensus.(*ConsensusState).handleMsg() /go/src/github.com/tendermint/tendermint/consensus/state.go:694 +0xa1e github.com/tendermint/tendermint/consensus.(*ConsensusState).receiveRoutine() /go/src/github.com/tendermint/tendermint/consensus/state.go:642 +0x948 github.com/tendermint/tendermint/consensus.(*ConsensusState).receiveRoutine() /go/src/github.com/tendermint/tendermint/consensus/state.go:642 +0x948 github.com/tendermint/tendermint/consensus.(*ConsensusState).receiveRoutine() /go/src/github.com/tendermint/tendermint/consensus/state.go:642 +0x948 github.com/tendermint/tendermint/consensus.(*ConsensusState).receiveRoutine() /go/src/github.com/tendermint/tendermint/consensus/state.go:655 +0x7dd github.com/tendermint/tendermint/consensus.(*ConsensusState).receiveRoutine() /go/src/github.com/tendermint/tendermint/consensus/state.go:642 +0x948 github.com/tendermint/tendermint/consensus.(*ConsensusState).receiveRoutine() /go/src/github.com/tendermint/tendermint/consensus/state.go:655 +0x7dd ``` * Fix failing test * Delete profile.out * fix data races --- CHANGELOG_PENDING.md | 1 + consensus/common_test.go | 10 +++++-- consensus/state.go | 1 + consensus/state_test.go | 65 ++++++++++++++++++++++++++++++++++++++-- 4 files changed, 72 insertions(+), 5 deletions(-) diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index 540a5d7d9..9ce6f496f 100644 --- a/CHANGELOG_PENDING.md +++ b/CHANGELOG_PENDING.md @@ -24,6 +24,7 @@ Special thanks to external contributors on this release: * [consensus] \#3297 Flush WAL on stop to prevent data corruption during graceful shutdown +- [consensus] \#3310 Reset TriggeredTimeoutPrecommit before starting next height - [rpc] \#3251 Fix /net_info#peers#remote_ip format. New format spec: * dotted decimal ("192.0.2.1"), if ip is an IPv4 or IP4-mapped IPv6 address * IPv6 ("2001:db8::1"), if ip is a valid IPv6 address diff --git a/consensus/common_test.go b/consensus/common_test.go index 7e24d3ffa..ec24704ac 100644 --- a/consensus/common_test.go +++ b/consensus/common_test.go @@ -124,15 +124,21 @@ func startTestRound(cs *ConsensusState, height int64, round int) { // Create proposal block from cs1 but sign it with vs func decideProposal(cs1 *ConsensusState, vs *validatorStub, height int64, round int) (proposal *types.Proposal, block *types.Block) { + cs1.mtx.Lock() block, blockParts := cs1.createProposalBlock() + cs1.mtx.Unlock() if block == nil { // on error panic("error creating proposal block") } // Make proposal - polRound, propBlockID := cs1.ValidRound, types.BlockID{block.Hash(), blockParts.Header()} + cs1.mtx.RLock() + validRound := cs1.ValidRound + chainID := cs1.state.ChainID + cs1.mtx.RUnlock() + polRound, propBlockID := validRound, types.BlockID{block.Hash(), blockParts.Header()} proposal = types.NewProposal(height, round, polRound, propBlockID) - if err := vs.SignProposal(cs1.state.ChainID, proposal); err != nil { + if err := vs.SignProposal(chainID, proposal); err != nil { panic(err) } return diff --git a/consensus/state.go b/consensus/state.go index 940318c0b..08e1da267 100644 --- a/consensus/state.go +++ b/consensus/state.go @@ -566,6 +566,7 @@ func (cs *ConsensusState) updateToState(state sm.State) { cs.CommitRound = -1 cs.LastCommit = lastPrecommits cs.LastValidators = state.LastValidators + cs.TriggeredTimeoutPrecommit = false cs.state = state diff --git a/consensus/state_test.go b/consensus/state_test.go index 153f51e16..e90035737 100644 --- a/consensus/state_test.go +++ b/consensus/state_test.go @@ -1288,8 +1288,8 @@ func (n *fakeTxNotifier) Notify() { } func TestStartNextHeightCorrectly(t *testing.T) { + config.Consensus.SkipTimeoutCommit = false cs1, vss := randConsensusState(4) - cs1.config.SkipTimeoutCommit = false cs1.txNotifier = &fakeTxNotifier{ch: make(chan struct{})} vs2, vs3, vs4 := vss[1], vss[2], vss[3] @@ -1326,13 +1326,14 @@ func TestStartNextHeightCorrectly(t *testing.T) { // add precommits signAddVotes(cs1, types.PrecommitType, nil, types.PartSetHeader{}, vs2) signAddVotes(cs1, types.PrecommitType, theBlockHash, theBlockParts, vs3) + time.Sleep(5 * time.Millisecond) signAddVotes(cs1, types.PrecommitType, theBlockHash, theBlockParts, vs4) - ensureNewBlockHeader(newBlockHeader, height, theBlockHash) - rs = cs1.GetRoundState() assert.True(t, rs.TriggeredTimeoutPrecommit) + ensureNewBlockHeader(newBlockHeader, height, theBlockHash) + cs1.txNotifier.(*fakeTxNotifier).Notify() ensureNewTimeout(timeoutProposeCh, height+1, round, cs1.config.TimeoutPropose.Nanoseconds()) @@ -1340,6 +1341,64 @@ func TestStartNextHeightCorrectly(t *testing.T) { assert.False(t, rs.TriggeredTimeoutPrecommit, "triggeredTimeoutPrecommit should be false at the beginning of each round") } +func TestResetTimeoutPrecommitUponNewHeight(t *testing.T) { + config.Consensus.SkipTimeoutCommit = false + cs1, vss := randConsensusState(4) + + vs2, vs3, vs4 := vss[1], vss[2], vss[3] + height, round := cs1.Height, cs1.Round + + partSize := types.BlockPartSizeBytes + + proposalCh := subscribe(cs1.eventBus, types.EventQueryCompleteProposal) + + newRoundCh := subscribe(cs1.eventBus, types.EventQueryNewRound) + newBlockHeader := subscribe(cs1.eventBus, types.EventQueryNewBlockHeader) + addr := cs1.privValidator.GetPubKey().Address() + voteCh := subscribeToVoter(cs1, addr) + + // start round and wait for propose and prevote + startTestRound(cs1, height, round) + ensureNewRound(newRoundCh, height, round) + + ensureNewProposal(proposalCh, height, round) + rs := cs1.GetRoundState() + theBlockHash := rs.ProposalBlock.Hash() + theBlockParts := rs.ProposalBlockParts.Header() + + ensurePrevote(voteCh, height, round) + validatePrevote(t, cs1, round, vss[0], theBlockHash) + + signAddVotes(cs1, types.PrevoteType, theBlockHash, theBlockParts, vs2, vs3, vs4) + + ensurePrecommit(voteCh, height, round) + validatePrecommit(t, cs1, round, round, vss[0], theBlockHash, theBlockHash) + + rs = cs1.GetRoundState() + + // add precommits + signAddVotes(cs1, types.PrecommitType, nil, types.PartSetHeader{}, vs2) + signAddVotes(cs1, types.PrecommitType, theBlockHash, theBlockParts, vs3) + time.Sleep(5 * time.Millisecond) + signAddVotes(cs1, types.PrecommitType, theBlockHash, theBlockParts, vs4) + + rs = cs1.GetRoundState() + assert.True(t, rs.TriggeredTimeoutPrecommit) + + ensureNewBlockHeader(newBlockHeader, height, theBlockHash) + + prop, propBlock := decideProposal(cs1, vs2, height+1, 0) + propBlockParts := propBlock.MakePartSet(partSize) + + if err := cs1.SetProposalAndBlock(prop, propBlock, propBlockParts, "some peer"); err != nil { + t.Fatal(err) + } + ensureNewProposal(proposalCh, height+1, 0) + + rs = cs1.GetRoundState() + assert.False(t, rs.TriggeredTimeoutPrecommit, "triggeredTimeoutPrecommit should be false at the beginning of each height") +} + //------------------------------------------------------------------------------------------ // SlashingSuite // TODO: Slashing