From b7bb076d403a21c605c0d307874a5c90ed3b9cbe Mon Sep 17 00:00:00 2001
From: Kasey <kasey@qri.io>
Date: Mon, 10 Aug 2020 09:52:23 -0400
Subject: [PATCH] fix(p2p): some clean up around `qri peers connect` and
 `upgradeToQriConnection` (#1489)

* refactor(p2p): don't double report upgrade error

We already log.Debug that there was an upgrade error inside `upgradeToQriConnection`, there is no need to also `log.Error` this message as well. We expect that many of the connections that we will be making will not "speak" qri. This shouldn't be an error.

* fix(`peers connect`): we need to explicitly upgrade a connection when we connect

When we connect directly to a peer using `qri peers connect`, since the command expects the peer's profile as a return value, we need to explicitly upgrade the connection so we know that the profile exists in the Profile store.

When we refactor, we really should be listening on a channel that is monitoring the profile exchange process & only trying to access the peer profile then. This is a temporary solution.
---
 p2p/bootstrap.go | 11 ++++++++++-
 p2p/node.go      |  7 ++-----
 p2p/peers.go     | 18 +++++++++++-------
 3 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/p2p/bootstrap.go b/p2p/bootstrap.go
index 5400eca1b..8d09bb25d 100644
--- a/p2p/bootstrap.go
+++ b/p2p/bootstrap.go
@@ -21,7 +21,13 @@ func (n *QriNode) Bootstrap(boostrapAddrs []string, boostrapPeers chan peer.Addr
 	}
 
 	pinfos := toPeerInfos(peers)
-	for _, p := range randomSubsetOfPeers(pinfos, 4) {
+	// TODO (ramfox): this randomSubsetOfPeers func is currently always
+	// returning the same 4 peers. Right now, I think it's okay to attempt to
+	// connect to all 7 of the bootstrap peers
+	// when we have more bootstraps in the future, then we can add back
+	// only dialing to a random subset
+	// for _, p := range randomSubsetOfPeers(pinfos, 4) {
+	for _, p := range pinfos {
 		go func(p peer.AddrInfo) {
 			log.Debugf("boostrapping to: %s", p.ID.Pretty())
 			if err := n.host.Connect(context.Background(), p); err == nil {
@@ -88,6 +94,9 @@ func toPeerInfos(addrs []ma.Multiaddr) []peer.AddrInfo {
 	return peers
 }
 
+// TODO (ramfox): this is always returning the same bootstrap peers
+// since the length of the list of peers that is given are always
+// the same
 // randomSubsetOfPeers samples up to max from a slice of PeerInfos
 func randomSubsetOfPeers(in []peer.AddrInfo, max int) []peer.AddrInfo {
 	n := int(math.Min(float64(max), float64(len(in))))
diff --git a/p2p/node.go b/p2p/node.go
index 501f729ba..ad78825fe 100644
--- a/p2p/node.go
+++ b/p2p/node.go
@@ -406,12 +406,9 @@ func (n *QriNode) libp2pSubscribe() error {
 			switch e := e.(type) {
 			case libp2pevent.EvtPeerIdentificationCompleted:
 				log.Debugf("libp2p identified peer: %s\n", e.Peer)
-				err := n.upgradeToQriConnection(e.Peer)
-				if err != nil {
-					log.Errorf("error upgrading to %s to Qri Connection: %s", e.Peer, err)
-				}
+				n.upgradeToQriConnection(e.Peer)
 			case libp2pevent.EvtPeerIdentificationFailed:
-				log.Errorf("libp2p failed to identify peer peer %s: %s", e.Peer, e.Reason)
+				log.Debugf("libp2p failed to identify peer peer %s: %s", e.Peer, e.Reason)
 			}
 		}
 	}()
diff --git a/p2p/peers.go b/p2p/peers.go
index 8696ce839..8b7013258 100644
--- a/p2p/peers.go
+++ b/p2p/peers.go
@@ -184,13 +184,17 @@ func (n *QriNode) ConnectToPeer(ctx context.Context, p PeerConnectionParams) (*p
 		return nil, fmt.Errorf("host connect %s failure: %s", pinfo.ID.Pretty(), err)
 	}
 
-	// // do an explicit connection upgrade. We're assmun
-	// if err := n.upgradeToQriConnection(pinfo.ID); err != nil {
-	// 	if err == ErrQriProtocolNotSupported {
-	// 		return nil, fmt.Errorf("upgrading p2p connection to a qri connection: %w", err)
-	// 	}
-	// 	return nil, err
-	// }
+	// do an explicit connection upgrade
+	// TODO (ramfox): when we refactor this, we should really
+	// be handing back a channel that will notify us when
+	// the profile exchange is complete, so we know it's safe
+	// to request the profile
+	if err := n.upgradeToQriConnection(pinfo.ID); err != nil {
+		if err == ErrQriProtocolNotSupported {
+			return nil, fmt.Errorf("upgrading p2p connection to a qri connection: %w", err)
+		}
+		return nil, err
+	}
 
 	return n.Repo.Profiles().PeerProfile(pinfo.ID)
 }