Fix issue where invalid / not validated would get gossiped (#1247)

Also requires us to split header data propagation from block body
and receipts propagation as the now fixed bug would allow for more
data to be gossiped even when data does not get validated (which
requires the headers).

fluffy/data/history_data_parser.nim

@@ -149,6 +149,45 @@ func readBlockHeader*(blockData: BlockData): Result[BlockHeader, string] =
   except RlpError as e:
     return err("Invalid header, number " & $blockData.number & ": " & e.msg)
 
+func readHeaderData*(
+    hash: string, blockData: BlockData, verify = false):
+    Result[(ContentKey, seq[byte]), string] =
+  var blockHash: BlockHash
+  try:
+    blockHash.data = hexToByteArray[sizeof(BlockHash)](hash)
+  except ValueError as e:
+    return err("Invalid hex for blockhash, number " &
+      $blockData.number & ": " & e.msg)
+
+  let contentKeyType =
+    BlockKey(blockHash: blockHash)
+
+  try:
+    # If wanted the hash for the corresponding header can be verified
+    if verify:
+      if keccakHash(blockData.header.hexToSeqByte()) != blockHash:
+        return err("Data is not matching hash, number " & $blockData.number)
+
+    let contentKey = ContentKey(
+      contentType: blockHeader,
+      blockHeaderKey: contentKeyType)
+
+    let res = (contentKey, blockData.header.hexToSeqByte())
+    return ok(res)
+
+  except ValueError as e:
+    return err("Invalid hex data, number " & $blockData.number & ": " & e.msg)
+
+iterator headers*(
+    blockData: BlockDataTable, verify = false): (ContentKey, seq[byte]) =
+  for k,v in blockData:
+    let res = readHeaderData(k, v, verify)
+
+    if res.isOk():
+      yield res.get()
+    else:
+      error "Failed reading header from block data", error = res.error
+
 proc getGenesisHeader*(id: NetworkId = MainNet): BlockHeader =
   let params =
     try:

fluffy/data/history_data_seeding.nim

@@ -127,7 +127,13 @@ proc historyPropagate*(
   let blockData = readJsonType(dataFile, BlockDataTable)
   if blockData.isOk():
     for b in blocks(blockData.get(), verify):
-      for value in b:
+      for i, value in b:
+        if i == 0:
+          # Note: Skipping propagation of headers here as they should be offered
+          # separately to be certain that bodies and receipts can be verified.
+          # TODO: Rename this chain of calls to be more clear about this and
+          # adjust the interator call.
+          continue
         # Only sending non empty data, e.g. empty receipts are not send
         # TODO: Could do a similar thing for a combination of empty
         # txs and empty uncles, as then the serialization is always the same.
@@ -172,3 +178,36 @@ proc historyPropagateBlock*(
     return ok()
   else:
     return err(blockDataTable.error)
+
+proc historyPropagateHeaders*(
+    p: PortalProtocol, dataFile: string, verify = false):
+    Future[Result[void, string]] {.async.} =
+  # TODO: Should perhaps be integrated with `historyPropagate` call.
+  const concurrentGossips = 20
+
+  var gossipQueue =
+    newAsyncQueue[(ContentKeysList, seq[byte])](concurrentGossips)
+  var gossipWorkers: seq[Future[void]]
+
+  proc gossipWorker(p: PortalProtocol) {.async.} =
+    while true:
+      let (keys, content) = await gossipQueue.popFirst()
+
+      await p.neighborhoodGossip(keys, @[content])
+
+  for i in 0 ..< concurrentGossips:
+    gossipWorkers.add(gossipWorker(p))
+
+  let blockData = readJsonType(dataFile, BlockDataTable)
+  if blockData.isOk():
+    for header in headers(blockData.get(), verify):
+      info "Seeding header content into the network", contentKey = header[0]
+      let contentId = history_content.toContentId(header[0])
+      p.storeContent(contentId, header[1])
+
+      await gossipQueue.addLast(
+        (ContentKeysList(@[encode(header[0])]), header[1]))
+
+    return ok()
+  else:
+    return err(blockData.error)

fluffy/network/history/history_network.nim

@@ -753,32 +753,44 @@ proc new*(
     contentQueue: cq
   )
 
+proc validateContent(
+    n: HistoryNetwork,
+    contentKeys: ContentKeysList,
+    contentItems: seq[seq[byte]]): Future[bool] {.async.} =
+  # content passed here can have less items then contentKeys, but not more.
+  for i, contentItem in contentItems:
+    let contentKey = contentKeys[i]
+    if await n.validateContent(contentItem, contentKey):
+      let contentIdOpt = n.portalProtocol.toContentId(contentKey)
+      if contentIdOpt.isNone():
+        error "Received offered content with invalid content key", contentKey
+        return false
+
+      let contentId = contentIdOpt.get()
+
+      n.portalProtocol.storeContent(contentId, contentItem)
+
+      info "Received offered content validated successfully", contentKey
+
+    else:
+      error "Received offered content failed validation", contentKey
+      return false
+
+  return true
+
 proc processContentLoop(n: HistoryNetwork) {.async.} =
   try:
     while true:
       let (contentKeys, contentItems) =
         await n.contentQueue.popFirst()
 
-      # content passed here can have less items then contentKeys, but not more.
+      # When there is one invalid content item, all other content items are
-      for i, contentItem in contentItems:
+      # dropped and not gossiped around.
-        let contentKey = contentKeys[i]
+      # TODO: Differentiate between failures due to invalid data and failures
-        if await n.validateContent(contentItem, contentKey):
+      # due to missing network data for validation.
-          let contentIdOpt = n.portalProtocol.toContentId(contentKey)
+      if await n.validateContent(contentKeys, contentItems):
-          if contentIdOpt.isNone():
+        asyncSpawn n.portalProtocol.neighborhoodGossip(contentKeys, contentItems)
-            continue
 
-          let contentId = contentIdOpt.get()
-
-          n.portalProtocol.storeContent(contentId, contentItem)
-
-          info "Received offered content validated successfully", contentKey
-        else:
-          error "Received offered content failed validation", contentKey
-          # On one invalid piece of content we drop all and don't forward any of it
-          # TODO: Could also filter it out and still gossip the rest.
-          continue
-
-      asyncSpawn n.portalProtocol.neighborhoodGossip(contentKeys, contentItems)
   except CancelledError:
     trace "processContentLoop canceled"
 

fluffy/rpc/rpc_calls/rpc_portal_debug_calls.nim

@@ -2,6 +2,7 @@
 proc portal_history_store(contentKey: string, content: string): bool
 proc portal_history_storeContent(dataFile: string): bool
 proc portal_history_propagate(dataFile: string): bool
+proc portal_history_propagateHeaders(dataFile: string): bool
 proc portal_history_propagateBlock(dataFile: string, blockHash: string): bool
 proc portal_history_propagateAccumulatorData(
     dataFile: string): bool
@@ -12,7 +13,7 @@ proc portal_history_storeContentInNodeRange(
 proc portal_history_offerContentInNodeRange(
     dbPath: string, nodeId: NodeId, max: uint32, starting: uint32): int
 proc portal_history_depthContentPropagate(
-  dbPath: string, max: uint32): bool
+    dbPath: string, max: uint32): bool
 proc portal_history_breadthContentPropagate(
-  dbPath: string): bool
+    dbPath: string): bool
 

fluffy/rpc/rpc_portal_debug_api.nim

@@ -48,6 +48,14 @@ proc installPortalDebugApiHandlers*(
     else:
       raise newException(ValueError, $res.error)
 
+  rpcServer.rpc("portal_" & network & "_propagateHeaders") do(
+      dataFile: string) -> bool:
+    let res = await p.historyPropagateHeaders(dataFile)
+    if res.isOk():
+      return true
+    else:
+      raise newException(ValueError, $res.error)
+
   rpcServer.rpc("portal_" & network & "_propagateBlock") do(
       dataFile: string, blockHash: string) -> bool:
     let res = await p.historyPropagateBlock(dataFile, blockHash)

fluffy/scripts/test_portal_testnet.nim

@@ -239,6 +239,14 @@ procSuite "Portal testnet tests":
     # await sleepAsync(60.seconds)
 
     const dataFile = "./fluffy/tests/blocks/mainnet_blocks_1000001_1000010.json"
+
+    check (await clients[0].portal_history_propagateHeaders(dataFile))
+    await clients[0].close()
+
+    # Short sleep between propagation of block headers and propagation of block
+    # bodies and receipts as the latter two require the first for validation.
+    await sleepAsync(5.seconds)
+
     # This will fill the first node its db with blocks from the data file. Next,
     # this node wil offer all these blocks their headers one by one.
     check (await clients[0].portal_history_propagate(dataFile))
@@ -251,11 +259,9 @@ procSuite "Portal testnet tests":
       # Note: Once there is the Canonical Indices Network, we don't need to
       # access this file anymore here for the block hashes.
       for hash in blockData.get().blockHashes():
-
         # Note: More flexible approach instead of generic retries could be to
         # add a json-rpc debug proc that returns whether the offer queue is empty or
         # not. And then poll every node until all nodes have an empty queue.
-
         let content = await retryUntil(
           proc (): Future[Option[BlockObject]] {.async.} =
             try:
@@ -397,68 +403,72 @@ procSuite "Portal testnet tests":
       removeDir(dbFile)
 
   asyncTest "Portal History - Propagate content from seed db in depth first fashion":
-    let clients = await connectToRpcServers(config)
+    # Skipping this test as it is flawed considering block headers should be
+    # offered before bodies and receipts.
+    # TODO: Split this up and activate test
+    skip()
+    # let clients = await connectToRpcServers(config)
 
-    var nodeInfos: seq[NodeInfo]
+    # var nodeInfos: seq[NodeInfo]
-    for client in clients:
+    # for client in clients:
-      let nodeInfo = await client.portal_history_nodeInfo()
+    #   let nodeInfo = await client.portal_history_nodeInfo()
-      await client.close()
+    #   await client.close()
-      nodeInfos.add(nodeInfo)
+    #   nodeInfos.add(nodeInfo)
 
-    # different set of data for each test as tests are statefull so previously propagated
+    # # different set of data for each test as tests are statefull so previously propagated
-    # block are already in the network
+    # # block are already in the network
-    const dataPath = "./fluffy/tests/blocks/mainnet_blocks_1000040_1000050.json"
+    # const dataPath = "./fluffy/tests/blocks/mainnet_blocks_1000040_1000050.json"
 
-    # path for temporary db, separate dir is used as sqlite usually also creates
+    # # path for temporary db, separate dir is used as sqlite usually also creates
-    # wal files, and we do not want for those to linger in filesystem
+    # # wal files, and we do not want for those to linger in filesystem
-    const tempDbPath = "./fluffy/tests/blocks/tempDir/mainnet_blocks_1000040_100050.sqlite3"
+    # const tempDbPath = "./fluffy/tests/blocks/tempDir/mainnet_blocks_1000040_100050.sqlite3"
 
-    let (dbFile, dbName) = getDbBasePathAndName(tempDbPath).unsafeGet()
+    # let (dbFile, dbName) = getDbBasePathAndName(tempDbPath).unsafeGet()
 
-    let blockData = readJsonType(dataPath, BlockDataTable)
+    # let blockData = readJsonType(dataPath, BlockDataTable)
-    check blockData.isOk()
+    # check blockData.isOk()
-    let bd = blockData.get()
+    # let bd = blockData.get()
 
-    createDir(dbFile)
+    # createDir(dbFile)
-    let db = SeedDb.new(path = dbFile, name = dbName)
+    # let db = SeedDb.new(path = dbFile, name = dbName)
 
-    try:
+    # try:
-      # populate temp database from json file
+    #   # populate temp database from json file
-      for t in blocksContent(bd, false):
+    #   for t in blocksContent(bd, false):
-        db.put(t[0], t[1], t[2])
+    #     db.put(t[0], t[1], t[2])
 
-      check (await clients[0].portal_history_depthContentPropagate(tempDbPath, 64))
+    #   check (await clients[0].portal_history_depthContentPropagate(tempDbPath, 64))
-      await clients[0].close()
+    #   await clients[0].close()
 
-      for i, client in clients:
+    #   for i, client in clients:
-        # Note: Once there is the Canonical Indices Network, we don't need to
+    #     # Note: Once there is the Canonical Indices Network, we don't need to
-        # access this file anymore here for the block hashes.
+    #     # access this file anymore here for the block hashes.
-        for hash in bd.blockHashes():
+    #     for hash in bd.blockHashes():
-          let content = await retryUntil(
+    #       let content = await retryUntil(
-            proc (): Future[Option[BlockObject]] {.async.} =
+    #         proc (): Future[Option[BlockObject]] {.async.} =
-              try:
+    #           try:
-                let res = await client.eth_getBlockByHash(hash.ethHashStr(), false)
+    #             let res = await client.eth_getBlockByHash(hash.ethHashStr(), false)
-                await client.close()
+    #             await client.close()
-                return res
+    #             return res
-              except CatchableError as exc:
+    #           except CatchableError as exc:
-                await client.close()
+    #             await client.close()
-                raise exc
+    #             raise exc
-            ,
+    #         ,
-            proc (mc: Option[BlockObject]): bool = return mc.isSome(),
+    #         proc (mc: Option[BlockObject]): bool = return mc.isSome(),
-            "Did not receive expected Block with hash " & hash.data.toHex(),
+    #         "Did not receive expected Block with hash " & hash.data.toHex(),
-            i
+    #         i
-          )
+    #       )
-          check content.isSome()
+    #       check content.isSome()
 
-          let blockObj = content.get()
+    #       let blockObj = content.get()
-          check blockObj.hash.get() == hash
+    #       check blockObj.hash.get() == hash
 
-          for tx in blockObj.transactions:
+    #       for tx in blockObj.transactions:
-            var txObj: TransactionObject
+    #         var txObj: TransactionObject
-            tx.fromJson("tx", txObj)
+    #         tx.fromJson("tx", txObj)
-            check txObj.blockHash.get() == hash
+    #         check txObj.blockHash.get() == hash
 
-        await client.close()
+    #     await client.close()
-    finally:
+    # finally:
-      db.close()
+    #   db.close()
-      removeDir(dbFile)
+    #   removeDir(dbFile)