fix(p2p): some clean up around qri peers connect and `upgradeToQriC…

…onnection` (#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

@@ -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))))

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)
  }
  }
  }()

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)
 }