Rework ContentId, ContentKey, GetContent to be more clear (#845)

* Small clarification in naming and note where GetContent is actually needed

* Rework contentId and contentKey

fluffy/network/portalcli.nim

@@ -13,7 +13,8 @@ import
   eth/[keys, net/nat],
   eth/p2p/discoveryv5/[enr, node],
   eth/p2p/discoveryv5/protocol as discv5_protocol,
-  ./wire/messages, ./wire/portal_protocol
+  ./wire/messages, ./wire/portal_protocol,
+  ./state/state_content
 
 type
   PortalCmd* = enum
@@ -151,10 +152,11 @@ proc discover(d: discv5_protocol.Protocol) {.async.} =
     info "Lookup finished", nodes = discovered.len
     await sleepAsync(30.seconds)
 
-# TODO for now just return some random id
-proc testHandler(contentKey: ByteList): ContentResult =
-  let id =  sha256.digest("test")
-  ContentResult(kind: ContentMissing, contentId: id)
+proc testHandler(contentKey: state_content.ByteList): ContentResult =
+  # Note: We don't incorperate storage in this tool so we always return
+  # missing content. For now we are using the state network derivation but it
+  # could be selective based on the network the tool is used for.
+  ContentResult(kind: ContentMissing, contentId: toContentId(contentKey))
 
 proc run(config: DiscoveryConf) =
   let

fluffy/network/state/state_content.nim

@@ -41,7 +41,7 @@ type
     # triePath: ByteList
     nodeHash*: NodeHash
 
-  ContentId* = MDigest[32 * 8]
+  ContentId* = Uint256
 
   KeccakHash* = MDigest[32 * 8] # could also import from either eth common types or trie defs
 
@@ -62,30 +62,25 @@ func fromSszBytes*(T: type ContentType, data: openArray[byte]):
 
   contentType
 
-func encodeKey*(contentKey: ContentKey): seq[byte] =
-  SSZ.encode(contentKey)
+func encode*(contentKey: ContentKey): ByteList =
+  List.init(SSZ.encode(contentKey), 2048)
 
 # TODO consider if more powerfull error handling is necessary here.
-func decodeKey*(contentKey: ByteList): Option[ContentKey] = 
+func decode*(contentKey: ByteList): Option[ContentKey] =
   try:
     some(SSZ.decode(contentKey.asSeq(), ContentKey))
   except SszError:
     return none[ContentKey]()
 
-func encodeKeyAsList*(contentKey: ContentKey): ByteList =
-  List.init(encodeKey(contentKey), 2048)
-
 func toContentId*(contentKey: ByteList): ContentId =
   # TODO: Hash function to be defined, sha256 used now, might be confusing
   # with keccak256 that is used for the actual nodes:
   # https://github.com/ethereum/stateless-ethereum-specs/blob/master/state-network.md#content
-  sha2.sha_256.digest(contentKey.asSeq())
+  let idHash = sha2.sha_256.digest(contentKey.asSeq())
+  readUintBE[256](idHash.data)
 
 func toContentId*(contentKey: ContentKey): ContentId =
-  toContentId(encodeKeyAsList(contentKey))
-
-func contentIdAsUint256*(id: ContentId): Uint256 =
-  readUintBE[256](id.data)
+  toContentId(encode(contentKey))
 
 type
   ContentStorage* = object
@@ -96,7 +91,7 @@ type
     # storage, via json rpc client requesting data from a full eth1 client.
     trie*: HexaryTrie
 
-proc getContent*(storage: ContentStorage, key: ContentKey): Option[seq[byte]] =
+proc get*(storage: ContentStorage, key: ContentKey): Option[seq[byte]] =
   if storage.trie.db == nil: # TODO: for now...
     return none(seq[byte])
   let val = storage.trie.db.get(key.nodeHash.data)
@@ -105,8 +100,8 @@ proc getContent*(storage: ContentStorage, key: ContentKey): Option[seq[byte]] =
   else:
     none(seq[byte])
 
-proc getContent*(storage: ContentStorage, contentKey: ByteList): Option[seq[byte]] =
-  decodeKey(contentKey).flatMap((key: ContentKey) => getContent(storage, key))
+proc get*(storage: ContentStorage, contentKey: ByteList): Option[seq[byte]] =
+  decode(contentKey).flatMap((key: ContentKey) => get(storage, key))
 
 proc newEmptyInMemoryStorage*(): ContentStorage =
   let trie = initHexaryTrie(newMemoryDb())

fluffy/network/state/state_network.nim

@@ -16,7 +16,7 @@ type StateNetwork* = ref object
 
 proc getHandler(storage: ContentStorage): ContentHandler =
     return (proc (contentKey: state_content.ByteList): ContentResult =
-      let maybeContent = storage.getContent(contentKey)
+      let maybeContent = storage.get(contentKey)
       if (maybeContent.isSome()):
         ContentResult(kind: ContentFound, content: maybeContent.unsafeGet())
       else:
@@ -28,12 +28,13 @@ proc getHandler(storage: ContentStorage): ContentHandler =
 # 3. Put item into storage (if in radius) after succesful lookup
 proc getContent*(p: StateNetwork, key: ContentKey):
     Future[Option[seq[byte]]] {.async.} =
-  let keyAsBytes = encodeKeyAsList(key)
-  let id = contentIdAsUint256(toContentId(keyAsBytes))
-  let result = await p.portalProtocol.contentLookup(keyAsBytes, id)
-  # for now returning bytes, ultimatly it would be nice to return proper domain
+  let
+    keyEncoded = encode(key)
+    id = toContentId(keyEncoded)
+    content = await p.portalProtocol.contentLookup(keyEncoded, id)
+  # for now returning bytes, ultimately it would be nice to return proper domain
   # types from here
-  return result.map(x => x.asSeq())
+  return content.map(x => x.asSeq())
 
 proc new*(T: type StateNetwork, baseProtocol: protocol.Protocol,
     storage: ContentStorage , dataRadius = UInt256.high(),

fluffy/network/wire/portal_protocol.nim

@@ -39,7 +39,7 @@ type
     of ContentFound:
       content*: seq[byte]
     of ContentMissing:
-      contentId*: MDigest[32 * 8]
+      contentId*: Uint256
     of ContentKeyValidationFailure:
       error*: string
 
@@ -139,7 +139,7 @@ proc handleFindContent(p: PortalProtocol, fc: FindContentMessage): seq[byte] =
       contentId = contentHandlingResult.contentId
       # TODO: Should we first do a simple check on ContentId versus Radius?
       closestNodes = p.routingTable.neighbours(
-        NodeId(readUintBE[256](contentId.data)), seenOnly = true)
+        NodeId(contentId), seenOnly = true)
       payload = ByteList(@[]) # Empty payload when enrs are send
       enrs =
         closestNodes.map(proc(x: Node): ByteList = ByteList(x.record.raw))

fluffy/tests/test_portal_wire_protocol.nim

@@ -23,7 +23,14 @@ type Default2NodeTest = ref object
   proto2: PortalProtocol
 
 proc testHandler(contentKey: ByteList): ContentResult =
-  let id =  sha256.digest("test")
+  let
+    idHash = sha256.digest("test")
+    id = readUintBE[256](idHash.data)
+  # TODO: Ideally we can return here a more valid content id. But that depends
+  # on the content key to content id derivation, which is different for the
+  # different content networks. And we want these tests to be independent from
+  # that. Could do something specifically for these tests, when there is a test
+  # case that would actually test this.
   ContentResult(kind: ContentMissing, contentId: id)
 
 proc defaultTestCase(rng: ref BrHmacDrbgContext): Default2NodeTest =
@@ -50,7 +57,7 @@ procSuite "Portal Wire Protocol Tests":
   asyncTest "Ping/Pong":
     let test = defaultTestCase(rng)
 
-    let pong = await test.proto1.ping(test.proto2.baseProtocol.localNode)
+    let pong = await test.proto1.ping(test.proto2.localNode)
 
     check:
       pong.isOk()
@@ -63,7 +70,7 @@ procSuite "Portal Wire Protocol Tests":
     let test = defaultTestCase(rng)
   
     block: # Find itself
-      let nodes = await test.proto1.findNode(test.proto2.baseProtocol.localNode,
+      let nodes = await test.proto1.findNode(test.proto2.localNode,
         List[uint16, 256](@[0'u16]))
 
       check:
@@ -73,7 +80,7 @@ procSuite "Portal Wire Protocol Tests":
 
     block: # Find nothing: this should result in nothing as we haven't started
       # the seeding of the portal protocol routing table yet.
-      let nodes = await test.proto1.findNode(test.proto2.baseProtocol.localNode,
+      let nodes = await test.proto1.findNode(test.proto2.localNode,
         List[uint16, 256](@[]))
 
       check:
@@ -93,7 +100,7 @@ procSuite "Portal Wire Protocol Tests":
       test.proto2.start()
 
       let distance = logDist(test.node1.localNode.id, test.node2.localNode.id)
-      let nodes = await test.proto1.findNode(test.proto2.baseProtocol.localNode,
+      let nodes = await test.proto1.findNode(test.proto2.localNode,
         List[uint16, 256](@[distance]))
 
       check:
@@ -118,7 +125,7 @@ procSuite "Portal Wire Protocol Tests":
 
     # content does not exist so this should provide us with the closest nodes
     # to the content, which is the only node in the routing table.
-    let foundContent = await test.proto1.findContent(test.proto2.baseProtocol.localNode,
+    let foundContent = await test.proto1.findContent(test.proto2.localNode,
       contentKey)
 
     check:
@@ -145,15 +152,18 @@ procSuite "Portal Wire Protocol Tests":
   asyncTest "Correctly mark node as seen after request":
     let test = defaultTestCase(rng)
 
-    let initialNeighbours = test.proto1.neighbours(test.proto1.baseProtocol.localNode.id, seenOnly = false)
+    let initialNeighbours = test.proto1.neighbours(test.proto1.localNode.id,
+      seenOnly = false)
 
     check:
       len(initialNeighbours) == 0
 
     discard test.proto1.addNode(test.proto2.baseProtocol.localNode)
 
-    let allNeighboursAfterAdd = test.proto1.neighbours(test.proto1.baseProtocol.localNode.id, seenOnly = false)
-    let seenNeighboursAfterAdd = test.proto1.neighbours(test.proto1.baseProtocol.localNode.id, seenOnly = true)
+    let allNeighboursAfterAdd = test.proto1.neighbours(
+      test.proto1.localNode.id, seenOnly = false)
+    let seenNeighboursAfterAdd = test.proto1.neighbours(
+      test.proto1.localNode.id, seenOnly = true)
 
     check:
       len(allNeighboursAfterAdd) == 1
@@ -161,8 +171,10 @@ procSuite "Portal Wire Protocol Tests":
 
     let pong = await test.proto1.ping(test.proto2.baseProtocol.localNode)
 
-    let allNeighboursAfterPing = test.proto1.neighbours(test.proto1.baseProtocol.localNode.id, seenOnly = false)
-    let seenNeighboursAfterPing = test.proto1.neighbours(test.proto1.baseProtocol.localNode.id, seenOnly = true)
+    let allNeighboursAfterPing = test.proto1.neighbours(
+      test.proto1.localNode.id, seenOnly = false)
+    let seenNeighboursAfterPing = test.proto1.neighbours(
+      test.proto1.localNode.id, seenOnly = true)
 
     check:
       pong.isOk()

fluffy/tests/test_state_network.nim

@@ -64,6 +64,8 @@ procSuite "State Content Network":
           contentType: state_content.ContentType.Account,
           nodeHash: nodeHash)
 
+      # Note: GetContent and thus the lookup here is not really needed, as we
+      # only have to request data to one node.
       let foundContent = await proto2.getContent(contentKey)
 
       check:
@@ -76,6 +78,8 @@ procSuite "State Content Network":
     await node2.closeWait()
 
   asyncTest "Find content in the network via content lookup":
+    # TODO: Improve this test so it actually need to go through several
+    # findNode request, to properly test the lookup call.
     let
       trie = genesisToTrie("fluffy" / "tests" / "custom_genesis" / "chainid7.json")
       node1 = initDiscoveryNode(