Mark peer as good fix by milosevic · Pull Request #2388 · tendermint/tendermint

milosevic · 2018-09-13T12:23:20Z

Refs #2048

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

milosevic · 2018-09-13T12:33:03Z

consensus/state.go

 	case *BlockPartMessage:
 		// if the proposal is complete, we'll enterPrevote or tryFinalizeCommit
 		_, err = cs.addProposalBlockPart(msg, peerID)
+		// TODO: At this point if added is true we have added new and useful block part so we can mark it in peer statistics.


To be able to update statistics at this point we need access to PeerState. One way to achieve this would be to pass Peer instead of p2p.ID in msgInfo. We still have issue how to call MarkGood method of AddrBook from ConsensusState. One option would be having a channel that is read by Switch in which we can write MarkGood messages. Channel can be obtained either through Peer or directly from ConsensusState. Any other idea?

milosevic · 2018-09-13T12:33:25Z

consensus/state.go

 // Attempt to add the vote. if its a duplicate signature, dupeout the validator
 func (cs *ConsensusState) tryAddVote(vote *types.Vote, peerID p2p.ID) error {
 	_, err := cs.addVote(vote, peerID)
+	// TODO: at this point we should update statistics if addVote return value added = true. Maybe tryAddVote can return


Similar as above.

consensus/reactor.go

codecov-io · 2018-09-19T12:57:50Z

Codecov Report

Merging #2388 into develop will decrease coverage by 0.42%.
The diff coverage is 62.26%.

@@             Coverage Diff             @@
##           develop    #2388      +/-   ##
===========================================
- Coverage    61.72%   61.29%   -0.43%     
===========================================
  Files          198      198              
  Lines        16292    16403     +111     
===========================================
- Hits         10057    10055       -2     
- Misses        5396     5515     +119     
+ Partials       839      833       -6

Impacted Files	Coverage Δ
consensus/metrics.go	`18.42% <ø> (ø)`	⬆️
cmd/tendermint/commands/init.go	`0% <0%> (ø)`	⬆️
cmd/tendermint/commands/testnet.go	`19.31% <0%> (ø)`	⬆️
consensus/state.go	`76.44% <62.5%> (-1.54%)`	⬇️
consensus/reactor.go	`72.45% <75%> (-0.57%)`	⬇️
libs/common/int.go	`17.85% <0%> (-82.15%)`	⬇️
libs/common/string.go	`61.53% <0%> (-15.13%)`	⬇️
consensus/replay.go	`54.85% <0%> (-7.77%)`	⬇️
state/store.go	`64.96% <0%> (-5.11%)`	⬇️
... and 18 more

xla

Really like the approach taken for the refactoring, Some inline comments that be good to see addressed.

xla · 2018-09-19T13:03:30Z

consensus/state.go

+// msgs sent to reactor to compute statistics on peer activity
+type peerInfo struct {
+	MsgType MsgType `json:"msg_type"`
+	PeerID  p2p.ID  `json:"peer_key"`


Is this struct every serialised? If not we should omit struct tags as it indicates that.

xla · 2018-09-19T13:06:30Z

consensus/reactor.go

+		case msg := <-conR.conS.statsMsgQueue:
+			// Get peer
+			peer := conR.Switch.Peers().Get(msg.PeerID)
+			if peer != nil {


What if the peer can't be found?

Should we reset peer stats on disconnect?

Good point. It could happen that at the point peerInfo arrives the peer is removed; I guess we can't do much in that case.

I don't think we should reset in on disconnect as this is more consensus level stats. It does not care if underlying connection is temporarily broken or not; this could be important metric for peer quality but it's different concern.

We should reverse the condition so we minimize the amount of code in the conditional. ie

if peer == nil { log.Debug("Attempt to update stats for non-existent peer", "peer", peerID) continue }

xla · 2018-09-19T13:07:35Z

consensus/reactor.go

+						conR.Switch.MarkPeerAsGood(peer)
+					}
+				case BlockPart:
+					if numParts := ps.RecordBlockPart(); numParts%blocksToContributeToBecomeGoodPeer == 0 {


Is parts equivalent to blocks? The constant indicates we expect a certain amount of blocks.

Good point. I was also confused with this. My understanding is that a peer can send several parts for the same block and we will count all parts. @ebuchman Could you provide input here?

Great point - it's about parts! At least in the current design, if the same peer sends us many unique parts for the same block, we count all of them. Could be good to fix the var name

xla · 2018-09-19T13:08:06Z

consensus/reactor.go

+				ps := peer.Get(types.PeerStateKey).(*PeerState)
+				switch msg.MsgType {
+				case Vote:
+					if numVotes := ps.RecordVote(); numVotes%votesToContributeToBecomeGoodPeer == 0 {


Why modulo and not check if the number of votes is greater than the constant we check against?

Because we want to mark peer as good every 10000 blocks/votes.

And also because we have a sense of overall peer contribution. This info could potentially be used break ties between two peers in case we need to choose. If we reset every 10k we lose this information.

xla · 2018-09-19T13:09:01Z

consensus/reactor.go

-// It returns the total number of votes (1 per block). This essentially means
-// the number of blocks for which peer has been sending us votes.
-func (ps *PeerState) RecordVote(vote *types.Vote) int {
+// RecordVote increments internal votes related statistics for this peer. It returns the total number of added votes.


Can we break long lines by the 80 char limit, please.

Do we have guideline regarding line limit? 80 is maybe too small in my view. I though that it's 90 in my editor but it was actually 120. I normally prefer 90 but we are not consistent in the code regarding this. Would be happy to have strict guideline on this.

xla · 2018-09-19T13:10:48Z

consensus/state_test.go

 	"testing"
 	"time"

+	"github.com/stretchr/testify/require"


Imports should be separated in three different groups: stdlib, external packages, internal packages.

xla · 2018-09-19T13:11:27Z

consensus/state_test.go

+	select {
+	case <-cs.statsMsgQueue:
+		t.Errorf("Should not output stats message after receiving the known block part!")
+	case <-time.After(1 * time.Second):


Can we lower this timeout? We already suffer from high timeouts in our test suite and we should be adamant to keep the test timings as tight as possible.

xla · 2018-09-19T13:11:37Z

consensus/state_test.go

+	select {
+	case <-cs.statsMsgQueue:
+		t.Errorf("Should not output stats message after receiving the known vote or vote from bigger height")
+	case <-time.After(1 * time.Second):


Same timeout concern.

xla · 2018-09-19T13:12:56Z

consensus/state.go

 // Attempt to add the vote. if its a duplicate signature, dupeout the validator
-func (cs *ConsensusState) tryAddVote(vote *types.Vote, peerID p2p.ID) error {
-	_, err := cs.addVote(vote, peerID)
+func (cs *ConsensusState) tryAddVote(vote *types.Vote, peerID p2p.ID) (added bool, err error) {


Can we avoid named return values if possible? It doesn't seem to be needed here.

Sure. Is there a best practice about using versus not named return values?

xla · 2018-09-19T13:17:44Z

consensus/state.go

+type MsgType int
+
+const (
+	Vote MsgType = iota


A defensive mechanism which helps increae the usability of "enums" in go is to either start of with iota+1 or have the first value be the an unkown. Reasoning being that the default value for int which is the underlying type is 0, which could lead to accidental use as Vote. Also those consts should be prefixed as such for readibilty and to reduce the likelihood of collision with other types, my proposal would be:

type MsgType int const ( MsgTypeUnknown MsgType = iota MsgTypeVote MsgTypeBlockPart )

This would create symmetry with the solution to the String method on the same type you provided.

Thanks a lot for this comment.

melekes · 2018-09-20T07:48:03Z

consensus/reactor.go

+	for {
+		// Manage disconnects from self or peer.
+		if !conR.IsRunning() {
+			conR.Logger.Info("Stopping peerStatsRoutine")


Think this needs to be Debug or removed completely.

Why's that ? Other "Stopping" logs are Info. This should only happen once, during shutdown

What purpose does this line serve? If that is to inform developer (validators shouldn't care about it) about a successful exit, then I'd prefer to have leaktest to make sure go-routine exits instead of relying on human eye post-failure.

melekes · 2018-09-20T07:53:37Z

consensus/reactor.go

+		case msg := <-conR.conS.statsMsgQueue:
+			// Get peer
+			peer := conR.Switch.Peers().Get(msg.PeerID)
+			if peer != nil {


Should we reset peer stats on disconnect?

melekes · 2018-09-20T07:54:01Z

consensus/reactor.go

+				ps := peer.Get(types.PeerStateKey).(*PeerState)
+				switch msg.MsgType {
+				case Vote:
+					if numVotes := ps.RecordVote(); numVotes%votesToContributeToBecomeGoodPeer == 0 {


Because we want to mark peer as good every 10000 blocks/votes.

ebuchman

Nice work! LGTM but a few minor nits to address. Especially I think the new MsgType/peerInfo stuff is redundant with the existing msgInfo

ebuchman · 2018-09-20T18:41:58Z

consensus/reactor.go

+		case msg := <-conR.conS.statsMsgQueue:
+			// Get peer
+			peer := conR.Switch.Peers().Get(msg.PeerID)
+			if peer != nil {


We should reverse the condition so we minimize the amount of code in the conditional. ie

if peer == nil { log.Debug("Attempt to update stats for non-existent peer", "peer", peerID) continue }

ebuchman · 2018-09-20T18:48:28Z

consensus/reactor.go


+func (conR *ConsensusReactor) peerStatsRoutine() {
+	for {
+		// Manage disconnects from self or peer.


comment is incorrect for this routine - it applies to the per-peer routines but this routine is global to all peers, so this is about shutting down the whole system.

ebuchman · 2018-09-20T18:49:03Z

consensus/reactor.go

+	for {
+		// Manage disconnects from self or peer.
+		if !conR.IsRunning() {
+			conR.Logger.Info("Stopping peerStatsRoutine")


Why's that ? Other "Stopping" logs are Info. This should only happen once, during shutdown

ebuchman · 2018-09-20T18:55:30Z

consensus/reactor.go

+					}
+				}
+			}
+		case <-conR.conS.Quit():


do we also need <-conR.Quit ? I don't think there's a way for the reactor to exit without the state exiting, but just noting.

Was thinking the same. Probably not needed, but added <-conR.Quit to be more defensive.

ebuchman · 2018-09-20T19:03:33Z

consensus/reactor.go

+// RecordVote increments internal votes related statistics for this peer.
+// It returns the total number of added votes.
+func (ps *PeerState) RecordVote() int {
 	ps.mtx.Lock()


I was going to suggest we could remove these locks since we only access the Stats synchronously, but that's not true because they're included in an RPC output. Cool.

ebuchman · 2018-09-20T19:12:53Z

consensus/state.go

 	return fmt.Sprintf("%v ; %d/%d %v", ti.Duration, ti.Height, ti.Round, ti.Step)
 }

+type MsgType int


Maybe we can just use the existing msgInfo instead of introducing these new types?

Good point. I will remove peerInfo for msgInfo.

ebuchman · 2018-09-20T19:15:03Z

consensus/state.go

 	timeoutTicker    TimeoutTicker

+	// information about about added votes and block parts are written on this channel
+	// so statistics can be computed by switch


by reactor not by switch

ebuchman · 2018-09-20T19:20:09Z

consensus/state.go

-		_, err = cs.addProposalBlockPart(msg, peerID)
+		added, err := cs.addProposalBlockPart(msg, peerID)
+		if added {
+			cs.statsMsgQueue <- peerInfo{MsgTypeBlockPart, peerID}


note its not safe to do this on the internalMsgQueue because reading off it happens synchronously with execution here, so we could have deadlocks (hence https://github.com/tendermint/tendermint/blob/develop/consensus/state.go#L437). but since this channel has its own independent routine that doesnt involve logic over here, this won't cause deadlock.

Very good point.

ebuchman · 2018-09-20T19:23:50Z

consensus/reactor_test.go

 }

-// Test we record block parts from other peers
-func TestReactorRecordsBlockParts(t *testing.T) {


Is there a way to bring back the test in a simpler version like for the Votes ?

It's very tricky in general to write tests for reactor side as I haven't found a simple way to add a peer so we can check if posting a message by a peer on statsMsgQueue trigger stats update. I will extend this test to include similar check for block part stats, but in general don't like this test.

ebuchman · 2018-09-20T19:24:50Z

consensus/reactor.go

 func (conR *ConsensusReactor) OnStart() error {
 	conR.Logger.Info("ConsensusReactor ", "fastSync", conR.FastSync())

+	// start routine that computes peer statistics


add for evaluating peer quality

milosevic requested review from ebuchman, melekes, xla and zramsay as code owners September 13, 2018 12:23

milosevic changed the base branch from master to develop September 13, 2018 12:23

milosevic commented Sep 13, 2018

View reviewed changes

melekes reviewed Sep 13, 2018

View reviewed changes

consensus/reactor.go Outdated Show resolved Hide resolved

melekes mentioned this pull request Sep 13, 2018

consensus: peer can be marked good after a single message #2048

Closed

milosevic force-pushed the zarko/2048-mark-peer-as-good-fix branch from 072ffcf to 845c367 Compare September 19, 2018 12:42

milosevic changed the title ~~WIP: Zarko/2048 mark peer as good fix~~ Zarko/2048 mark peer as good fix Sep 19, 2018

milosevic changed the title ~~Zarko/2048 mark peer as good fix~~ Mark peer as good fix Sep 19, 2018

milosevic force-pushed the zarko/2048-mark-peer-as-good-fix branch from 845c367 to 98e1be5 Compare September 19, 2018 12:47

xla suggested changes Sep 19, 2018

View reviewed changes

melekes reviewed Sep 20, 2018

View reviewed changes

ebuchman approved these changes Sep 20, 2018

View reviewed changes

xla added C:consensus Component: Consensus T:enhancement Type: Enhancement T:bug Type Bug (Confirmed) T:security Type: Security (specify priority) C:p2p Component: P2P pkg and removed T:enhancement Type: Enhancement labels Sep 21, 2018

xla added this to the launch milestone Sep 21, 2018

Add stats related channel between consensus state and reactor

36b6087

milosevic force-pushed the zarko/2048-mark-peer-as-good-fix branch from 9bdaabb to 36b6087 Compare September 21, 2018 11:12

xla assigned milosevic Sep 21, 2018

ebuchman approved these changes Sep 21, 2018

View reviewed changes

Merge branch 'develop' into zarko/2048-mark-peer-as-good-fix

162f299

ebuchman merged commit f99e401 into develop Sep 21, 2018

ebuchman deleted the zarko/2048-mark-peer-as-good-fix branch September 21, 2018 18:37

ebuchman mentioned this pull request Sep 28, 2018

[WIP] General Merkle Proof #2298

Merged

4 tasks

Conversation

milosevic commented Sep 13, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

xla 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!

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!

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!

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

milosevic commented Sep 13, 2018 •

edited

Loading

codecov-io commented Sep 19, 2018 •

edited

Loading