cs/replay: execCommitBlock should not read from state.lastValidators by james-ray · Pull Request #3067 · tendermint/tendermint

james-ray · 2018-12-24T08:38:41Z

Updated all relevant documentation in docs
Updated all code comments where relevant
Wrote tests
Updated CHANGELOG_PENDING.md

codecov-io · 2018-12-24T10:47:55Z

Codecov Report

Merging #3067 into develop will decrease coverage by 0.05%.
The diff coverage is 74.07%.

@@             Coverage Diff             @@
##           develop    #3067      +/-   ##
===========================================
- Coverage    64.44%   64.39%   -0.06%     
===========================================
  Files          213      213              
  Lines        17467    17507      +40     
===========================================
+ Hits         11257    11274      +17     
- Misses        5293     5308      +15     
- Partials       917      925       +8

Impacted Files	Coverage Δ
consensus/wal_generator.go	`82.17% <100%> (+0.17%)`	⬆️
consensus/replay.go	`71.02% <50%> (-0.06%)`	⬇️
state/execution.go	`73.3% <78.94%> (-0.71%)`	⬇️
privval/socket_listeners.go	`87.03% <0%> (-2.62%)`	⬇️
p2p/pex/pex_reactor.go	`80.05% <0%> (-2.35%)`	⬇️
libs/clist/clist.go	`66.66% <0%> (-1.52%)`	⬇️
consensus/reactor.go	`72.01% <0%> (-0.95%)`	⬇️
libs/db/mem_batch.go	`92.59% <0%> (-0.75%)`	⬇️
libs/db/types.go	`100% <0%> (ø)`	⬆️
... and 7 more

melekes · 2019-01-24T08:54:03Z

Would be great if you could write a test case (or multiple test cases) covering the bug from #3054 and earlier changes #3006 you've made.

james-ray · 2019-01-25T07:04:25Z

OK, I'll handle the test cases.

ebuchman

Thanks @james-ray. Sorry for the delays. I think the core changes should be fine. Still a bit confused with the tests, and they can probably be much improved, but it doesn't need to block this.

Though it seems in the meantime the branch got out of sync. Would you mind merging in latest develop and addressing the conflicts? Then we can merge.

We can follow up on fixing the tests in #3600 and on the SaveState stuff in #1029

ebuchman · 2019-04-26T23:42:25Z

consensus/mempool_test.go

-	cs := newConsensusState(state, privVals[0], NewCounterApplication())
+	blockDB := dbm.NewMemDB()
+	cs := newConsensusStateWithConfigAndBlockStore(config, state, privVals[0], NewCounterApplication(), blockDB)
+	sm.SaveState(blockDB, state)


Why did this need to change at all ?

ebuchman · 2019-04-26T23:42:41Z

consensus/mempool_test.go

-	cs := newConsensusState(state, privVals[0], app)
+	blockDB := dbm.NewMemDB()
+	cs := newConsensusStateWithConfigAndBlockStore(config, state, privVals[0], app, blockDB)
+	sm.SaveState(blockDB, state)


Why did this need to change?

ebuchman · 2019-04-27T01:08:16Z

consensus/replay.go

 		h.logger.Info("Applying block", "height", i)
 		block := h.store.LoadBlock(i)
-		appHash, err = sm.ExecCommitBlock(proxyApp.Consensus(), block, h.logger, state.LastValidators, h.stateDB)
+		appHash, err = sm.ExecCommitBlock(proxyApp.Consensus(), block, h.logger, h.stateDB)


I see. We already know what the validators are supposed to be in the Tendermint state, we just need to fetch that for the BeginBlock info.

ebuchman · 2019-04-27T01:10:58Z

consensus/common_test.go

 	// Make ConsensusState
-	stateDB := dbm.NewMemDB()
+	stateDB := blockDB
+	sm.SaveState(stateDB, state) //for save height 1's validators info


not sure I follow. why can't we use stateDB := dbm.NewMemDB() here? Isn't the BlockExecutor supposed to use the stateDB?

ebuchman · 2019-04-27T01:12:02Z

consensus/replay_test.go

+func TestSimulateValidatorsChange(t *testing.T) {
+	nPeers := 7
+	nVals := 4
+	css, genDoc, config, cleanup := randConsensusNetWithPeers(nVals, nPeers, "replay_test", newMockTickerFunc(true), newPersistentKVStoreWithPath)


No, not hard code, but it should be much easier to build a valid blockchain with the functions. No need to go through the entire consensus process like this!

# Conflicts: # CHANGELOG_PENDING.md # consensus/replay.go # consensus/replay_test.go

consensus/reactor.go

consensus/replay_test.go

james-ray · 2019-04-28T15:03:07Z

I see now there are 2 suggestions,

Simplify that simulate chain test case to calling some MakeBlock() with some precommits and validatorChange Txs. No need to go through consensus to create a mock chain.
A lot of places need to call saveState() in order to save height 1's validator set. About how to fetch the stateDB, reviewer suggests

Maybe we should have a newStateDB(state) function in this package that we could use in all these tests instead?

I think they are both good suggestions. Would you mind approve this first? I could work on them at another pr.

ebuchman · 2019-04-28T22:30:09Z

libs/pubsub/pubsub.go

 					select {
 					case subscription.out <- Message{msg, tags}:
-					default:
+					case <-time.After(2 * time.Second): //give some time for the subscriber to handle


Why was this necessary?

I don't think we want this, especially not with a hard coded time. We just refactored this module to not block here.

ebuchman · 2019-04-28T22:32:48Z

Thank you for updating. I opened those two issues so we can track them without blocking this PR. This PR is basically good to merge, except for the new change to the pubsub. Is it possible to revert that? cc @melekes

Also, would be great if you could clarify why the changes were necessary in the mempool_test.go

james-ray · 2019-04-29T01:46:07Z

As for mempool_test, do you mean these code below?

blockDB := dbm.NewMemDB()
cs := newConsensusStateWithConfigAndBlockStore(config, state, privVals[0], NewCounterApplication(), blockDB)
sm.SaveState(blockDB, state)

These code serve a similar purpose, to call the SaveState, otherwise I couldn't get stateDB. We could adopt your suggestion, add a function newStateDB(state) to do that, then stateDB would not have to be the same with blockDB.

Why I modify pubsub is because TestStateLockPOLRelock in state_test has some probability to fail, you can try it. I debug and found

state.remove(clientID, qStr, ErrOutOfCapacity)

this code is the reason. It will delete the subscriber of vote event, the ensurePrecommit or ensurePrevote will timeout and fail. I think you cannot ensure the consumer is bound to handle a message prior to the next publish message, so I add a time interval to give it more chance. Or we could enlarge the capacity of the channel. Or We could set the capacity to 0 to make it block there. Anyway just because the consumer doesn't handle previous message in time we do the removal of it is not reasonable.
I can revert that, but the CI test may fail. Otherwise I wouldn't have modified it.

As in the pubsub_test, I see 3 publish and 2 receive, I think the 3rd publish won't cause the state.remove(clientID, qStr, ErrOutOfCapacity) code above, so I add one more publish.
Why the previous code can succeed because 2 sequential publishes cause the subscriber probably doesn't have the chance to handle the 1st one. However you cannot ensure that.

james-ray · 2019-04-29T08:20:38Z

I found an interesting phenomenon:

func TestStateFullRound1(t *testing.T) {
cs, vss := randConsensusState(1)
height, round := cs.Height, cs.Round

// NOTE: buffer capacity of 0 ensures we can validate prevote and last commit
// before consensus can move to the next height (and cause a race condition)
cs.eventBus.Stop()
eventBus := types.NewEventBusWithBufferCapacity(0)
eventBus.SetLogger(log.TestingLogger().With("module", "events"))
cs.SetEventBus(eventBus)
eventBus.Start()

voteCh := subscribeUnBuffered(cs.eventBus, types.EventQueryVote)
propCh := subscribe(cs.eventBus, types.EventQueryCompleteProposal)
newRoundCh := subscribe(cs.eventBus, types.EventQueryNewRound)

// Maybe it would be better to call explicitly startRoutines(4)
startTestRound(cs, height, round)

ensureNewRound(newRoundCh, height, round)

ensureNewProposal(propCh, height, round)
propBlockHash := cs.GetRoundState().ProposalBlock.Hash()

ensurePrevote(voteCh, height, round) // wait for prevote
validatePrevote(t, cs, round, vss[0], propBlockHash)

ensurePrecommit(voteCh, height, round) // wait for precommit

// we're going to roll right into new height
ensureNewRound(newRoundCh, height+1, 0)

validateLastPrecommit(t, cs, vss[0], propBlockHash)

}

This case, if we set voteCh := subscribeUnBuffered(..) , then there is a small probability that the test case will hang there because when the publish is executed, the ensurePrevote hasn't been executed, the publish will be forever blocked. I don't know why, the loop function in pubsub is executed in a go routine, it should not suppose to block the main routine.

But if we set voteCh := subscribe(...), then there is also a small probability that the last code validateLastPrecommit(t, cs, vss[0], propBlockHash) could fail, because it may already go through several heights, once a time I ran it, it went to height 4, so this validation failed.

james-ray · 2019-04-29T08:26:35Z

--- FAIL: TestBadBlockStopsPeer (19.83s)
testing.go:809: race detected during execution of test

Also 2 times the CI error on this. No hint of where or which variable the race is.

melekes · 2019-05-02T05:05:28Z

consensus/common_test.go


 	"github.com/go-kit/kit/log/term"

+	"path"


Suggested change

"path"

"filepath"

path does not work on Windows

melekes · 2019-05-02T05:30:46Z

consensus/byzantine_test.go

 		}

 		reactors[i] = conRI
+		sm.SaveState(css[i].blockExec.DB(), css[i].state) //for save height 1's validators info


Suggested change

sm.SaveState(css[i].blockExec.DB(), css[i].state) //for save height 1's validators info

sm.SaveState(css[i].blockExec.DB(), css[i].state) // save height=1 validators info

melekes · 2019-05-02T05:31:20Z

consensus/common_test.go


 func subscribeToVoter(cs *ConsensusState, addr []byte) <-chan tmpubsub.Message {
-	votesSub, err := cs.eventBus.Subscribe(context.Background(), testSubscriber, types.EventQueryVote)
+	votesSub, err := cs.eventBus.SubscribeUnbuffered(context.Background(), testSubscriber, types.EventQueryVote)


maybe we should leave a comment why SubscribeUnbuffered must be used here?

melekes · 2019-05-02T05:32:30Z

consensus/common_test.go


 	// Make ConsensusState
-	stateDB := dbm.NewMemDB()
+	stateDB := blockDB


this is super confusing. please add a comment

can't we add yet another argument to newConsensusStateWithConfigAndBlockStore function - stateDB dbm.DB

melekes · 2019-05-02T05:32:57Z

consensus/common_test.go

 	// Make ConsensusState
-	stateDB := dbm.NewMemDB()
+	stateDB := blockDB
+	sm.SaveState(stateDB, state) //for save height 1's validators info


Suggested change

sm.SaveState(stateDB, state) //for save height 1's validators info

sm.SaveState(stateDB, state) // save height=1 validators info

melekes · 2019-05-02T05:41:26Z

consensus/wal_generator.go

 	}
-	stateDB := db.NewMemDB()
 	blockStoreDB := db.NewMemDB()
+	stateDB := blockStoreDB


this is super confusing. please add a comment at the very least

melekes · 2019-05-02T05:41:45Z

libs/pubsub/pubsub_test.go

 		assertReceive(t, "Quicksilver", subscription.Out())
 		assertCancelled(t, subscription, pubsub.ErrOutOfCapacity)
-	case <-time.After(100 * time.Millisecond):
+	case <-time.After(3 * time.Second):


why increase?

melekes · 2019-05-02T05:41:53Z

libs/pubsub/pubsub_test.go

 	case <-published:
 		t.Fatal("Expected Publish(Darkhawk) to block")
-	case <-time.After(100 * time.Millisecond):
+	case <-time.After(3 * time.Second):


same question

melekes · 2019-05-02T05:43:39Z

consensus/replay_test.go

 		logger := log.NewNopLogger()
-		stateDB := dbm.NewMemDB()
+		blockDB := dbm.NewMemDB()
+		stateDB := blockDB


this is super confusing. please add a comment at the very least

melekes · 2019-05-02T05:45:20Z

consensus/replay_test.go


 //------------------------------------------------------------------------------------------
-// Handshake Tests
+type testSim struct {


what Sim stands for?

…endermint#3067) * execCommitBlock should not read from state.lastValidators * fix height 1 * fix blockchain/reactor_test * fix consensus/mempool_test * fix consensus/reactor_test * fix consensus/replay_test * add CHANGELOG * fix consensus/reactor_test * fix consensus/replay_test * add a test for replay validators change * fix mem_pool test * fix byzantine test * remove a redundant code * reduce validator change blocks to 6 * fix * return peer0 config * seperate testName * seperate testName 1 * seperate testName 2 * seperate app db path * seperate app db path 1 * add a lock before startNet * move the lock to reactor_test * simulate just once * try to find problem * handshake only saveState when app version changed * update gometalinter to 3.0.0 (tendermint#3233) in the attempt to fix https://circleci.com/gh/tendermint/tendermint/43165 also code is simplified by running gofmt -s . remove unused vars enable linters we're currently passing remove deprecated linters (cherry picked from commit d470945) * gofmt code * goimport code * change the bool name to testValidatorsChange * adjust receive kvstore.ProtocolVersion * adjust receive kvstore.ProtocolVersion 1 * adjust receive kvstore.ProtocolVersion 3 * fix merge execution.go * fix merge develop * fix merge develop 1 * fix run cleanupFunc * adjust code according to reviewers' opinion * modify the func name match the convention * simplify simulate a chain containing some validator change txs 1 * test CI error * Merge remote-tracking branch 'upstream/develop' into fixReplay 1 * fix pubsub_test * subscribeUnbuffered vote channel

execCommitBlock should not read from state.lastValidators

bc0bea3

james-ray requested review from ebuchman and melekes as code owners December 24, 2018 08:38

james-ray added 5 commits December 24, 2018 17:02

fix height 1

2df6f59

fix blockchain/reactor_test

4d68e3e

fix consensus/mempool_test

4e862df

fix consensus/reactor_test

1552758

fix consensus/replay_test

8f5c650

james-ray added 3 commits December 24, 2018 18:51

add CHANGELOG

593e162

fix consensus/reactor_test

5c114c5

fix consensus/replay_test

29f283a

james-ray changed the title ~~execCommitBlock should not read from state.lastValidators~~ In replay, execCommitBlock should not read from state.lastValidators Dec 25, 2018

Merge branch 'develop' into fixReplay

c60e43a

melekes changed the title ~~In replay, execCommitBlock should not read from state.lastValidators~~ [WIP] In replay, execCommitBlock should not read from state.lastValidators Jan 29, 2019

james-ray added 13 commits January 30, 2019 10:01

Merge branch 'develop' into fixReplay

4a2c9c7

add a test for replay validators change

0343102

Merge remote-tracking branch 'origin/fixReplay' into fixReplay

d7cd051

fix mem_pool test

ba81bff

fix byzantine test

7ad9bea

remove a redundant code

472251d

reduce validator change blocks to 6

bd97c04

fix

fcb919c

return peer0 config

b6bff17

seperate testName

f52ff78

seperate testName 1

f4a0427

seperate testName 2

c93110e

seperate app db path

855d406

This was referenced Apr 27, 2019

Cleanup replay tests from #3067 #3600

Closed

state: Use BlockExecutor everywhere #1029

Closed

ebuchman approved these changes Apr 27, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/develop' into fixReplay

59338b5

# Conflicts: # CHANGELOG_PENDING.md # consensus/replay.go # consensus/replay_test.go

james-ray requested a review from xla as a code owner April 28, 2019 02:58

golangcibot reviewed Apr 28, 2019

View reviewed changes

consensus/reactor.go Outdated Show resolved Hide resolved

Merge remote-tracking branch 'upstream/develop' into fixReplay 1

5a00948

golangcibot reviewed Apr 28, 2019

View reviewed changes

consensus/replay_test.go Outdated Show resolved Hide resolved

consensus/replay_test.go Outdated Show resolved Hide resolved

consensus/replay_test.go Outdated Show resolved Hide resolved

fix pubsub_test

8246aff

james-ray force-pushed the fixReplay branch from 7d70463 to 8246aff Compare April 28, 2019 09:26

ebuchman reviewed Apr 28, 2019

View reviewed changes

james-ray force-pushed the fixReplay branch from 7dacec1 to 7b43ff9 Compare April 29, 2019 07:45

subscribeUnbuffered vote channel

30c24d5

james-ray force-pushed the fixReplay branch from 7b43ff9 to 30c24d5 Compare April 29, 2019 08:07

Merge branch 'develop' into fixReplay

2d6a35f

ebuchman approved these changes May 1, 2019

View reviewed changes

ebuchman merged commit 2c26d95 into tendermint:develop May 1, 2019

melekes reviewed May 2, 2019

View reviewed changes

melekes mentioned this pull request May 7, 2019

v0.31.6 release preview #3637

Closed

36 tasks

ebuchman mentioned this pull request May 10, 2019

panic: precommit length (X) doesn't match valset length (Y) at height Z #3054

Closed

This was referenced May 30, 2019

temporary #3684

Closed

cs/replay: address execCommitBlock PR comments #3685

Closed

	sm.SaveState(css[i].blockExec.DB(), css[i].state) //for save height 1's validators info
	sm.SaveState(css[i].blockExec.DB(), css[i].state) // save height=1 validators info

	sm.SaveState(stateDB, state) //for save height 1's validators info
	sm.SaveState(stateDB, state) // save height=1 validators info

Conversation

james-ray commented Dec 24, 2018 • edited by melekes Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Dec 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

melekes commented Jan 24, 2019

Uh oh!

james-ray commented Jan 25, 2019

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

james-ray commented Apr 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebuchman commented Apr 28, 2019

Uh oh!

james-ray commented Apr 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

james-ray commented Apr 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

james-ray commented Apr 29, 2019

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

james-ray commented Dec 24, 2018 •

edited by melekes

Loading

codecov-io commented Dec 24, 2018 •

edited

Loading

james-ray commented Apr 28, 2019 •

edited

Loading

james-ray commented Apr 29, 2019 •

edited

Loading

james-ray commented Apr 29, 2019 •

edited

Loading