Introduce EventValidBlock for informing peers about wanted block #2652

milosevic · 2018-10-17T17:50:52Z

Fix #2583. Note that tests that check consensus reactor part will be addressed in the follow up PR.

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

codecov-io · 2018-10-19T09:02:33Z

Codecov Report

Merging #2652 into develop will increase coverage by 0.19%.
The diff coverage is 88.13%.

@@            Coverage Diff             @@
##           develop   #2652      +/-   ##
==========================================
+ Coverage    61.61%   61.8%   +0.19%     
==========================================
  Files          207     207              
  Lines        16942   16952      +10     
==========================================
+ Hits         10439   10478      +39     
+ Misses        5640    5615      -25     
+ Partials       863     859       -4

Impacted Files	Coverage Δ
consensus/state.go	`80.02% <100%> (+3.84%)`	⬆️
p2p/metrics.go	`17.94% <100%> (ø)`	⬆️
consensus/reactor.go	`72.34% <80%> (+0.34%)`	⬆️
blockchain/pool.go	`65.73% <0%> (-0.7%)`	⬇️
p2p/pex/pex_reactor.go	`74% <0%> (-0.34%)`	⬇️
p2p/conn/connection.go	`79.43% <0%> (-0.29%)`	⬇️

…-enable-valid-block-property

melekes · 2018-10-21T08:55:12Z

consensus/reactor.go

@@ -392,13 +397,18 @@ func (conR *ConsensusReactor) broadcastProposalHeartbeatMessage(hb *types.Heartb
 }

 func (conR *ConsensusReactor) broadcastNewRoundStepMessages(rs *cstypes.RoundState) {


broadcastNewRoundStepMessage

melekes · 2018-10-24T16:43:09Z

consensus/state.go

+						"Valid block we don't know about. Set ProposalBlock=nil",
+						"proposal", cs.ProposalBlock.Hash(), "blockId", blockID.Hash)
+					// We're getting the wrong block.
+					cs.ProposalBlock = nil


what about cs.ValidRound and cs.ValidBlockParts? Shouldn't we reset these too?

We update cs.ValidRound, cs.ValidBlockParts and cs.ValidBlock only when we have the corresponding block. Therefore, at this point we can only "tell" gossip layer that we need some block, and then update ValidX variables when block arrives (if it arrives on time).

ebuchman

Great work, but still have a few questions and some minor nits.

ebuchman · 2018-10-25T01:48:35Z

consensus/reactor.go

@@ -964,7 +963,8 @@ func (ps *PeerState) SetHasProposal(proposal *types.Proposal) {
 	if ps.PRS.Height != proposal.Height || ps.PRS.Round != proposal.Round {
 		return
 	}
-	if ps.PRS.Proposal {
+	// ps.PRS.ProposalBlockParts is set due to NewValidBlockMessage
+	if ps.PRS.Proposal || ps.PRS.ProposalBlockParts != nil {


Is this really necessary?

I believe the situation this captures is when they don't have a proposal, but they sent us NewValidBlockMessage. For this to run, then either they'd have to send us a proposal (which I think would be Byzantine) or we'd have to have tried to send it to them (should that ever happen?).

I think it does make sense to have the check here, just hard to follow the circumstance where !PRS.Proposal && PRS.ProposalBlockParts != nil

The key part here is ps.PRS.ProposalBlockParts != nil. In this case we should't update PRS.ProposalBlockPartsHeader as it is already set (either by previous Proposal message or NewValidBlockMessage). But we might still want to set PRS.Proposal to true in case it is not already set. So maybe we should split this into 2 conditions. Regarding your last point, negation is !PRS.Proposal && PRS.ProposalBlockParts == nil and this corresponds to the case that we are receiving Proposal for the first time and we haven't received NewValidBlockMessage yet.

ebuchman · 2018-10-25T01:50:40Z

consensus/reactor.go

 	ps.mtx.Lock()
 	defer ps.mtx.Unlock()

-	if ps.PRS.Height != msg.Height {
+	if ps.PRS.Height != msg.Height && ps.PRS.Round != msg.Round {


What if there was a commit in Round 2, but this peer never received the block from Round 2, and is already in Round 3. Then it sees the commit from round 2, and sends us NewValidBlockMessage with Round==2?

Do we need special logic for when the NewValidBlockMessage is specifically for a commit, since the round may be different then?

Hard to write a test for this right now :(

Excellent point, I missed this. We probably need to treat commit in a special way. Maybe we can just add flag to NewValidBlockMessage to say if it is commit or not. And yes, we don't have tests for this, but it will come soon :)

ebuchman · 2018-10-25T01:53:37Z

consensus/state.go

@@ -1420,11 +1415,6 @@ func (cs *ConsensusState) defaultSetProposal(proposal *types.Proposal) error {
 		return nil
 	}

-	// We don't care about the proposal if we're already in cstypes.RoundStepCommit.


This line was pointed out as the reason #2567 wasn't a bug (ie. see #2567 (comment)).

Now, instead of checking the commit step, we check if the ProposalBlockParts is already set, so we don't overwrite it. Is that right?

I was aware of #2567 and was convinced that we still have the same guarantee with the new condition. I will make one more check.

ebuchman · 2018-10-25T01:58:01Z

consensus/state.go

@@ -1616,16 +1611,26 @@ func (cs *ConsensusState) addVote(vote *types.Vote, peerID p2p.ID) (added bool,

 			// Update Valid* if we can.
 			// NOTE: our proposal block may be nil or not what received a polka..
-			// TODO: we may want to still update the ValidBlock and obtain it via gossipping


Why don't we consider vote.Round < cs.Round anymore?

cs.ProposalBlock corresponds to the cs.Round. The spec also only cares about updating validBlock for the current round.

ebuchman · 2018-10-25T02:06:47Z

consensus/state_test.go

+	ensurePrecommit(voteCh, height, round)
+	// we should have precommitted
+	validatePrecommit(t, cs1, round, -1, vss[0], nil, nil)
+


We should add checks here to show that ValidBlock and ValidBlockParts are currently empty

ebuchman · 2018-10-25T02:08:15Z

consensus/state_test.go

+		t.Fatal(err)
+	}
+
+	time.Sleep(20 * time.Millisecond)


Oh this isn't good. Can't we wait on the CompleteProposal event here instead?

Good point. The only point at which we emit CompleteProposal is in enterPrevote (

tendermint/consensus/state.go

Line 999 in 6643c5d

cs.eventBus.PublishEventCompleteProposal(cs.RoundStateEvent())

), so it does not work out of the box. But I will modify this to emit CompleteProposal upon receiving CompleteProposal in addProposalBlockPart, and then it's fine. We should probably add more tests to check when we are emitting events as they are crucial for overall functioning of the system.

ebuchman · 2018-10-25T02:10:13Z

consensus/state_test.go

+	startTestRound(cs1, height, round)
+	ensureNewRound(newRoundCh, height, round)
+
+	// vs2, vs3 and vs4 send prevote for propBlock


send precommit

ebuchman · 2018-10-25T02:11:30Z

consensus/state_test.go

+	assert.True(t, rs.Step == cstypes.RoundStepCommit)
+	assert.True(t, rs.ProposalBlock == nil)
+	assert.True(t, rs.ProposalBlockParts.Header().Equals(propBlockParts.Header()))
+}


Awesome.

Can we add another test for the situation where we've already gone to the next round, but then see the commit for the previous round, ie. what was described in #2567 ?

Also, out of curiosity, what happens if we propose a block and then see +2/3 precommit for a different block in that round? That's supposed to imply +2/3 are Byzantine, right?

In the current version of the code this can happen as we are prevoting for the locked value, so it can be normal case. I prefer more having prevote to reflect reaction to proposal (or absence of proposal) like done in spec; in that case seeing +2/3 precommit for a different block means we have +2/3 Byzantine as correct can only precommit for a proposed value in case proposer is correct, or 2) private key of current proposer is stolen :)

consensus/reactor.go

ebuchman · 2018-10-25T02:15:54Z

If we release v0.26.0 before merging this, we should consider trying to do this in a backwards compatible manner - ie. keep the CommitStepMessage, and only send the NewValidBlock message to peers on the right P2PVersion. We'll have to make the peer's P2PVersion available somehow to the reactor.

- Add test for the case of +2/3 Precommit from the previous round

milosevic requested review from ebuchman, melekes and xla as code owners October 17, 2018 17:50

milosevic force-pushed the zarko/2583-enable-valid-block-property branch from 66b0938 to a06929b Compare October 19, 2018 08:56

milosevic force-pushed the zarko/2583-enable-valid-block-property branch 5 times, most recently from 3111d10 to a4cde65 Compare October 19, 2018 09:37

milosevic requested a review from zramsay as a code owner October 19, 2018 09:37

milosevic force-pushed the zarko/2583-enable-valid-block-property branch from a4cde65 to 72637c5 Compare October 19, 2018 09:50

Introduce EventValidBlock for informing peer about wanted block

8ec9a55

milosevic force-pushed the zarko/2583-enable-valid-block-property branch from 72637c5 to 8ec9a55 Compare October 19, 2018 09:54

milosevic changed the title ~~[WIP] Introduce EventValidBlock for informing peers about wanted block~~ Introduce EventValidBlock for informing peers about wanted block Oct 19, 2018

Zarko Milosevic added 2 commits October 19, 2018 18:30

Merge remote-tracking branch 'remotes/origin/develop' into zarko/2583…

4fe1b89

…-enable-valid-block-property

Merge with develop

339fdf2

melekes approved these changes Oct 24, 2018

View reviewed changes

melekes reviewed Oct 24, 2018

View reviewed changes

ebuchman reviewed Oct 25, 2018

View reviewed changes

Zarko Milosevic added 2 commits October 29, 2018 17:44

Add isCommit flag to NewValidBlock message

fa68a91

- Add test for the case of +2/3 Precommit from the previous round

Merge with develop

108a9a8

milosevic mentioned this pull request Oct 31, 2018

Simplify proposal msg #2735

Merged

4 tasks

ebuchman approved these changes Oct 31, 2018

View reviewed changes

ebuchman merged commit 7a03344 into develop Oct 31, 2018

ebuchman deleted the zarko/2583-enable-valid-block-property branch June 24, 2019 14:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce EventValidBlock for informing peers about wanted block #2652

Introduce EventValidBlock for informing peers about wanted block #2652

milosevic commented Oct 17, 2018 •

edited

codecov-io commented Oct 19, 2018 •

edited

melekes Oct 21, 2018

melekes Oct 24, 2018

milosevic Oct 25, 2018 •

edited

ebuchman left a comment

ebuchman Oct 25, 2018

milosevic Oct 25, 2018 •

edited

ebuchman Oct 25, 2018

milosevic Oct 25, 2018 •

edited

ebuchman Oct 25, 2018

milosevic Oct 25, 2018

ebuchman Oct 25, 2018

milosevic Oct 25, 2018

ebuchman Oct 25, 2018

ebuchman Oct 25, 2018

milosevic Oct 25, 2018 •

edited

ebuchman Oct 25, 2018

ebuchman Oct 25, 2018

milosevic Oct 25, 2018

ebuchman commented Oct 25, 2018 •

edited

		@@ -392,13 +397,18 @@ func (conR ConsensusReactor) broadcastProposalHeartbeatMessage(hb types.Heartb
		}

		func (conR ConsensusReactor) broadcastNewRoundStepMessages(rs cstypes.RoundState) {

Introduce EventValidBlock for informing peers about wanted block #2652

Introduce EventValidBlock for informing peers about wanted block #2652

Conversation

milosevic commented Oct 17, 2018 • edited

codecov-io commented Oct 19, 2018 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

milosevic Oct 25, 2018 • edited

Choose a reason for hiding this comment

ebuchman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

milosevic Oct 25, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

milosevic Oct 25, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

milosevic Oct 25, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebuchman commented Oct 25, 2018 • edited

milosevic commented Oct 17, 2018 •

edited

codecov-io commented Oct 19, 2018 •

edited

milosevic Oct 25, 2018 •

edited

milosevic Oct 25, 2018 •

edited

milosevic Oct 25, 2018 •

edited

milosevic Oct 25, 2018 •

edited

ebuchman commented Oct 25, 2018 •

edited