Dont dial self (#633)

* don't dial self

* revert style changes

* filter out self on dial

* add helpers to `peerId` and `isSelf`

* don't fire up discovery eaguerly

* allow excluding multiple peers in sendWantHave

* revert style changes

* move self check to discovery.find

* readd eaguer dht lookup is required in some cases

* revert style changes

* misc

* drop peer first, before queueing a dht lookup

* moar style changes

* use isSelf

codex/blockexchange/engine/engine.nim

@@ -124,11 +124,11 @@ proc stop*(b: BlockExcEngine) {.async.} =
 proc sendWantHave(
   b: BlockExcEngine,
   address: BlockAddress,
-  selectedPeer: BlockExcPeerCtx,
+  excluded: seq[BlockExcPeerCtx],
   peers: seq[BlockExcPeerCtx]): Future[void] {.async.} =
   trace "Sending wantHave request to peers", address
   for p in peers:
-    if p != selectedPeer:
+    if p notin excluded:
       if address notin p.peerHave:
         trace " wantHave > ", peer = p.id
         await b.network.request.sendWantList(
@@ -172,14 +172,13 @@ proc monitorBlockHandle(b: BlockExcEngine, handle: Future[Block], address: Block
     # prioritization
 
     # drop unresponsive peer
-    b.discovery.queueFindBlocksReq(@[address.cidOrTreeCid])
     await b.network.switch.disconnect(peerId)
+    b.discovery.queueFindBlocksReq(@[address.cidOrTreeCid])
 
 proc requestBlock*(
   b: BlockExcEngine,
   address: BlockAddress,
-  timeout = DefaultBlockTimeout
+  timeout = DefaultBlockTimeout): Future[Block] {.async.} =
-): Future[Block] {.async.} =
   let blockFuture = b.pendingBlocks.getWantHandle(address, timeout)
 
   if b.pendingBlocks.isInFlight(address):
@@ -202,7 +201,7 @@ proc requestBlock*(
     b.pendingBlocks.setInFlight(address)
     await b.sendWantBlock(address, peer)
     codex_block_exchange_want_block_lists_sent.inc()
-    await b.sendWantHave(address, peer, toSeq(b.peers))
+    await b.sendWantHave(address, @[peer], toSeq(b.peers))
     codex_block_exchange_want_have_lists_sent.inc()
 
   return await blockFuture
@@ -210,8 +209,7 @@ proc requestBlock*(
 proc requestBlock*(
   b: BlockExcEngine,
   cid: Cid,
-  timeout = DefaultBlockTimeout
+  timeout = DefaultBlockTimeout): Future[Block] =
-): Future[Block] =
   b.requestBlock(BlockAddress.init(cid))
 
 proc blockPresenceHandler*(

codex/blockexchange/network/network.nim

@@ -75,6 +75,18 @@ type
     getConn: ConnProvider
     inflightSema: AsyncSemaphore
 
+proc peerId*(b: BlockExcNetwork): PeerId =
+  ## Return peer id
+  ##
+
+  return b.switch.peerInfo.peerId
+
+proc isSelf*(b: BlockExcNetwork, peer: PeerId): bool =
+  ## Check if peer is self
+  ##
+
+  return b.peerId == peer
+
 proc send*(b: BlockExcNetwork, id: PeerId, msg: pb.Message) {.async.} =
   ## Send message to peer
   ##
@@ -110,8 +122,7 @@ proc sendWantList*(
   cancel: bool = false,
   wantType: WantType = WantType.WantHave,
   full: bool = false,
-    sendDontHave: bool = false
+  sendDontHave: bool = false): Future[void] =
-): Future[void] =
   ## Send a want message to peer
   ##
 
@@ -131,8 +142,7 @@ proc sendWantList*(
 proc handleBlocksDelivery(
   b: BlockExcNetwork,
   peer: NetworkPeer,
-    blocksDelivery: seq[BlockDelivery]
+  blocksDelivery: seq[BlockDelivery]) {.async.} =
-) {.async.} =
   ## Handle incoming blocks
   ##
 
@@ -173,8 +183,7 @@ proc sendBlockPresence*(
 proc handleAccount(
   network: BlockExcNetwork,
   peer: NetworkPeer,
-    account: Account
+  account: Account) {.async.} =
-) {.async.} =
   ## Handle account info
   ##
 
@@ -184,8 +193,7 @@ proc handleAccount(
 proc sendAccount*(
   b: BlockExcNetwork,
   id: PeerId,
-    account: Account
+  account: Account): Future[void] =
-): Future[void] =
   ## Send account info to remote
   ##
 
@@ -194,8 +202,7 @@ proc sendAccount*(
 proc sendPayment*(
   b: BlockExcNetwork,
   id: PeerId,
-    payment: SignedState
+  payment: SignedState): Future[void] =
-): Future[void] =
   ## Send payment to remote
   ##
 
@@ -204,8 +211,7 @@ proc sendPayment*(
 proc handlePayment(
   network: BlockExcNetwork,
   peer: NetworkPeer,
-    payment: SignedState
+  payment: SignedState) {.async.} =
-) {.async.} =
   ## Handle payment
   ##
 
@@ -215,8 +221,7 @@ proc handlePayment(
 proc rpcHandler(
   b: BlockExcNetwork,
   peer: NetworkPeer,
-    msg: Message
+  msg: Message) {.async.} =
-) {.async.} =
   ## handle rpc messages
   ##
   try:
@@ -273,6 +278,13 @@ proc setupPeer*(b: BlockExcNetwork, peer: PeerId) =
   discard b.getOrCreatePeer(peer)
 
 proc dialPeer*(b: BlockExcNetwork, peer: PeerRecord) {.async.} =
+  ## Dial a peer
+  ##
+
+  if b.isSelf(peer.peerId):
+    trace "Skipping dialing self", peer = peer.peerId
+    return
+
   await b.switch.connect(peer.peerId, peer.addresses.mapIt(it.address))
 
 proc dropPeer*(b: BlockExcNetwork, peer: PeerId) =
@@ -306,8 +318,7 @@ proc new*(
   T: type BlockExcNetwork,
   switch: Switch,
   connProvider: ConnProvider = nil,
-    maxInflight = MaxInflight
+  maxInflight = MaxInflight): BlockExcNetwork =
-): BlockExcNetwork =
   ## Create a new BlockExcNetwork instance
   ##
 

codex/blockexchange/network/networkpeer.nim

@@ -83,8 +83,7 @@ func new*(
   T: type NetworkPeer,
   peer: PeerId,
   connProvider: ConnProvider,
-  rpcHandler: RPCHandler
+  rpcHandler: RPCHandler): NetworkPeer =
-): NetworkPeer =
 
   doAssert(not isNil(connProvider),
     "should supply connection provider")

codex/discovery.nim

@@ -8,6 +8,7 @@
 ## those terms.
 
 import std/algorithm
+import std/sequtils
 
 import pkg/chronos
 import pkg/chronicles
@@ -55,8 +56,7 @@ proc toNodeId*(host: ca.Address): NodeId =
 
 proc findPeer*(
   d: Discovery,
-    peerId: PeerId
+  peerId: PeerId): Future[?PeerRecord] {.async.} =
-): Future[?PeerRecord] {.async.} =
   trace "protocol.resolve..."
   ## Find peer using the given Discovery object
   ##
@@ -71,8 +71,7 @@ proc findPeer*(
 
 method find*(
   d: Discovery,
-    cid: Cid
+  cid: Cid): Future[seq[SignedPeerRecord]] {.async, base.} =
-): Future[seq[SignedPeerRecord]] {.async, base.} =
   ## Find block providers
   ##
 
@@ -81,7 +80,7 @@ method find*(
     (await d.protocol.getProviders(cid.toNodeId())).mapFailure, error:
     trace "Error finding providers for block", cid, error = error.msg
 
-  return providers
+  return providers.filterIt( not (it.data.peerId == d.peerId) )
 
 method provide*(d: Discovery, cid: Cid) {.async, base.} =
   ## Provide a bock Cid
@@ -99,8 +98,7 @@ method provide*(d: Discovery, cid: Cid) {.async, base.} =
 
 method find*(
   d: Discovery,
-    host: ca.Address
+  host: ca.Address): Future[seq[SignedPeerRecord]] {.async, base.} =
-): Future[seq[SignedPeerRecord]] {.async, base.} =
   ## Find host providers
   ##
 
@@ -132,8 +130,7 @@ method provide*(d: Discovery, host: ca.Address) {.async, base.} =
 
 method removeProvider*(
   d: Discovery,
-    peerId: PeerId
+  peerId: PeerId): Future[void] {.base.} =
-): Future[void] {.base.} =
   ## Remove provider from providers table
   ##
 

codex/stores/networkstore.nim

@@ -16,16 +16,16 @@ import std/sugar
 import pkg/chronicles
 import pkg/chronos
 import pkg/libp2p
+import pkg/questionable/results
 
-import ../blocktype
 import ../utils/asyncheapqueue
 import ../utils/asynciter
 import ../clock
 
+import ../blocktype
 import ./blockstore
 import ../blockexchange
 import ../merkletree
-import ../blocktype
 
 export blockstore, blockexchange, asyncheapqueue
 
@@ -69,12 +69,11 @@ method getBlock*(self: NetworkStore, treeCid: Cid, index: Natural): Future[?!Blo
 method putBlock*(
   self: NetworkStore,
   blk: Block,
-    ttl = Duration.none
+  ttl = Duration.none): Future[?!void] {.async.} =
-): Future[?!void] {.async.} =
   ## Store block locally and notify the network
   ##
 
-  trace "Puting block into network store", cid = blk.cid
+  trace "Putting block into network store", cid = blk.cid
 
   let res = await self.localStore.putBlock(blk, ttl)
   if res.isErr:
@@ -88,15 +87,13 @@ method putBlockCidAndProof*(
   treeCid: Cid,
   index: Natural,
   blockCid: Cid,
-  proof: MerkleProof
+  proof: MerkleProof): Future[?!void] =
-): Future[?!void] =
   self.localStore.putBlockCidAndProof(treeCid, index, blockCid, proof)
 
 method ensureExpiry*(
   self: NetworkStore,
   cid: Cid,
-    expiry: SecondsSince1970
+  expiry: SecondsSince1970): Future[?!void] {.async.} =
-): Future[?!void] {.async.} =
   ## Ensure that block's assosicated expiry is at least given timestamp
   ## If the current expiry is lower then it is updated to the given one, otherwise it is left intact
   ##
@@ -115,8 +112,7 @@ method ensureExpiry*(
   self: NetworkStore,
   treeCid: Cid,
   index: Natural,
-    expiry: SecondsSince1970
+  expiry: SecondsSince1970): Future[?!void] {.async.} =
-): Future[?!void] {.async.} =
   ## Ensure that block's associated expiry is at least given timestamp
   ## If the current expiry is lower then it is updated to the given one, otherwise it is left intact
   ##

tests/integration/testIntegration.nim

@@ -229,7 +229,6 @@ twonodessuite "Integration tests", debug1 = false, debug2 = false:
     check responsePast.status == "400 Bad Request"
     check responsePast.body == "Expiry needs to be in future"
 
-
   test "expired request partially pays out for stored time":
     let marketplace = Marketplace.new(Marketplace.address, provider.getSigner())
     let tokenAddress = await marketplace.token()