Only Sync With the Peer With the Highest Observed Slot #2280

rauljordan · 2019-04-17T14:15:59Z

This is part of #1586

Description

Write why you are making the changes in this pull request

Currently, our chain head querier naively accepts any response it gets, even though it might be from a peer that is really far behind or forked. Instead, we should query all peers and use the highest one as our reference to sync with the network.

Where it requests the latest heads from all peers and only exits after a timeout. It then goes with the peer that has the highest canonical slot.

rauljordan · 2019-04-17T14:40:21Z

beacon-chain/sync/initial-sync/service.go

@@ -268,24 +274,6 @@ func (s *InitialSync) checkInMemoryBlocks() {
 	}
 }

-// listenForNewBlocks listens for block announcements beyond the canonical head slot that may


No longer relevant as this is handled by regular sync

we still should have it, otherwise the node will always have to play catch up whenever it starts regular sync

prestonvanloon · 2019-04-17T20:48:58Z

Build and lint errors

stale · 2019-04-24T20:49:50Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

terencechain · 2019-04-25T03:33:16Z

Build and test failing

prestonvanloon · 2019-04-25T16:15:17Z

Test failures

codecov · 2019-04-25T17:19:42Z

Codecov Report

Merging #2280 into master will decrease coverage by 1.4%.
The diff coverage is 86.36%.

@@            Coverage Diff             @@
##           master    #2280      +/-   ##
==========================================
- Coverage    66.6%   65.19%   -1.41%     
==========================================
  Files         119      119              
  Lines        9642     9533     -109     
==========================================
- Hits         6422     6215     -207     
- Misses       2503     2627     +124     
+ Partials      717      691      -26

prestonvanloon · 2019-04-25T17:19:26Z

beacon-chain/sync/initial-sync/service.go

@@ -17,6 +17,8 @@ import (
 	"sync"
 	"time"

+	peer "github.com/libp2p/go-libp2p-peer"
+


prestonvanloon · 2019-04-25T17:19:45Z

beacon-chain/sync/querier.go

@@ -5,6 +5,8 @@ import (
 	"math/big"
 	"time"

+	peer "github.com/libp2p/go-libp2p-peer"
+


prestonvanloon · 2019-04-25T17:20:22Z

beacon-chain/sync/regular_sync.go

@@ -8,6 +8,8 @@ import (
 	"runtime/debug"
 	"sync"

+	peer "github.com/libp2p/go-libp2p-peer"
+


prestonvanloon · 2019-04-25T17:21:18Z

shared/p2p/service.go

@@ -184,6 +184,11 @@ func (s *Server) Status() error {
 	return nil
 }

+// Peers returns a slice of currently connected peer id's to be used throughout the runtime.
+func (s *Server) Peers() peer.IDSlice {
+	return s.host.Peerstore().Peers()


Use s.host.Network().Peers()

rauljordan · 2019-04-25T19:16:28Z

PTAL @prestonvanloon

nisdas · 2019-04-25T22:50:46Z

beacon-chain/sync/initial-sync/service.go

@@ -268,24 +274,6 @@ func (s *InitialSync) checkInMemoryBlocks() {
 	}
 }

-// listenForNewBlocks listens for block announcements beyond the canonical head slot that may


we still should have it, otherwise the node will always have to play catch up whenever it starts regular sync

nisdas · 2019-04-25T22:51:34Z

beacon-chain/sync/initial-sync/sync_state.go

@@ -96,11 +97,11 @@ func (s *InitialSync) processState(msg p2p.Message) {
 }

 // requestStateFromPeer requests for the canonical state, finalized state, and justified state from a peer.
-func (s *InitialSync) requestStateFromPeer(ctx context.Context, lastFinalizedRoot [32]byte) error {
+func (s *InitialSync) requestStateFromPeer(ctx context.Context, lastFinalizedRoot [32]byte, peerID peer.ID) error {


you should just use s.bestPeer here, since we are only going to sync with one peer

prestonvanloon

Minor comments. Overall LGTM.

This is a great interim solution until we rethink the sync logic with the weak subjectivity logic.

Thanks!

prestonvanloon · 2019-04-26T01:41:46Z

beacon-chain/sync/initial-sync/sync_blocks.go

+	if msg.Peer != s.bestPeer {
+		// Only process batch block responses that come from the best peer
+		// we originally synced with.
+		log.Debugf("Received batch blocks from a different peer: %s", msg.Peer.Pretty())


Use WithField

prestonvanloon · 2019-04-26T01:42:28Z

beacon-chain/sync/querier.go

-		case msg := <-q.responseBuf:
-			response := msg.Data.(*pb.ChainHeadResponse)
+		case <-timeout:
+			queryLog.Infof("Peer with highest canonical head: %v", q.bestPeer.Pretty())


These should conform to WithFields logging standard, please

prestonvanloon · 2019-04-26T01:43:10Z

beacon-chain/sync/querier.go

-// RequestLatestHead broadcasts out a request for all
-// the latest chain heads from the node's peers.
+// RequestLatestHead sends a request for all
+// the latest chain head slot and state root to a peer.


Do you want to change this comment back? It’s still broadcasting

prestonvanloon · 2019-04-26T14:27:58Z

You have new lint issues

beacon-chain/sync/initial-sync/metrics.go:22:2: U1000: var `recBlockAnnounce` is unused (unused)
	recBlockAnnounce = promauto.NewCounter(prometheus.CounterOpts{
	^
beacon-chain/sync/initial-sync/sync_blocks.go:17:23: U1000: func `(*InitialSync).processBlockAnnounce` is unused (unused)
func (s *InitialSync) processBlockAnnounce(msg p2p.Message) {
                      ^
beacon-chain/sync/regular_sync_test.go:51:2: U1000: field `bFeed` is unused (unused)
	bFeed           *event.Feed
	^

Lint issues

…eers

rauljordan added 2 commits April 17, 2019 08:54

only accept the highest finalized slot from peers

7e1dd70

use best peer for state requests

ebe6977

rauljordan commented Apr 17, 2019

View reviewed changes

stale bot added the Stale There hasn't been any activity here in some time... label Apr 24, 2019

rauljordan removed the Stale There hasn't been any activity here in some time... label Apr 25, 2019

rauljordan added 3 commits April 24, 2019 22:33

Merge branch 'master' into req-peers

6bc8e96

peer id

f6b84ce

rem old tests

6920423

rauljordan added 5 commits April 25, 2019 11:57

Merge branch 'master' into req-peers

84521e4

req peers tests pass

47c499a

warn level support for peer not having the same protocol

2007b69

pretty

d49b2af

best peer

9a9809d

prestonvanloon requested changes Apr 25, 2019

View reviewed changes

rauljordan added 5 commits April 25, 2019 12:24

comments

ae4e008

Merge branch 'master' into req-peers

55059da

req peers done

eebfce7

tests passing

9677a56

fully functional

513b509

rauljordan self-assigned this Apr 25, 2019

rauljordan added Priority: Critical Highest, immediate priority item Ready For Review labels Apr 25, 2019

rauljordan added this to the Sapphire milestone Apr 25, 2019

rauljordan requested review from nisdas and prestonvanloon April 25, 2019 17:58

rauljordan requested a review from terencechain April 25, 2019 17:58

terencechain previously approved these changes Apr 25, 2019

View reviewed changes

nisdas reviewed Apr 25, 2019

View reviewed changes

enforce receiving from the best peer

d3ef315

rauljordan dismissed terencechain’s stale review via d3ef315 April 25, 2019 23:06

Merge branch 'master' into req-peers

01f69e3

prestonvanloon previously approved these changes Apr 26, 2019

View reviewed changes

comments

11ce6ad

rauljordan dismissed prestonvanloon’s stale review via 11ce6ad April 26, 2019 04:09

rauljordan added 2 commits April 25, 2019 23:09

Merge branch 'master' into req-peers

574ca10

Merge branch 'master' into req-peers

e5ebb19

prestonvanloon previously approved these changes Apr 26, 2019

View reviewed changes

rauljordan added 2 commits April 26, 2019 09:32

lint fixes

75f32a9

Merge branch 'req-peers' of github.com:prysmaticlabs/prysm into req-p…

70ccd16

…eers

prestonvanloon approved these changes Apr 26, 2019

View reviewed changes

rauljordan merged commit 81c8b13 into master Apr 26, 2019

rauljordan deleted the req-peers branch April 26, 2019 15:18

rauljordan mentioned this pull request May 5, 2019

Fatal error when failing to retrieve chain head on a forked node restart #2288

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only Sync With the Peer With the Highest Observed Slot #2280

Only Sync With the Peer With the Highest Observed Slot #2280

rauljordan commented Apr 17, 2019 •

edited

Loading

rauljordan Apr 17, 2019

nisdas Apr 25, 2019

prestonvanloon commented Apr 17, 2019

stale bot commented Apr 24, 2019

terencechain commented Apr 25, 2019

prestonvanloon commented Apr 25, 2019

codecov bot commented Apr 25, 2019 •

edited

Loading

prestonvanloon Apr 25, 2019

prestonvanloon Apr 25, 2019

prestonvanloon Apr 25, 2019

prestonvanloon Apr 25, 2019

rauljordan commented Apr 25, 2019

nisdas Apr 25, 2019

nisdas Apr 25, 2019

prestonvanloon left a comment

prestonvanloon Apr 26, 2019

prestonvanloon Apr 26, 2019

prestonvanloon Apr 26, 2019

prestonvanloon commented Apr 26, 2019

Only Sync With the Peer With the Highest Observed Slot #2280

Only Sync With the Peer With the Highest Observed Slot #2280

Conversation

rauljordan commented Apr 17, 2019 • edited Loading

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prestonvanloon commented Apr 17, 2019

stale bot commented Apr 24, 2019

terencechain commented Apr 25, 2019

prestonvanloon commented Apr 25, 2019

codecov bot commented Apr 25, 2019 • edited Loading

Codecov Report

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

rauljordan commented Apr 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prestonvanloon left a 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

prestonvanloon commented Apr 26, 2019

rauljordan commented Apr 17, 2019 •

edited

Loading

codecov bot commented Apr 25, 2019 •

edited

Loading