feat: support retrieval hints for efficient message retrieval from store nodes #18
Conversation
shash256
requested review from
Ivansete-status,
gabrielmer and
jm-clius
and removed request for
gabrielmer
jm-clius
left a comment
jm-clius
left a comment
There was a problem hiding this comment.
Thanks! A few minor comments, with a question about retrieval hints on checkDependencies being the only blocker to approving.
|
|
||
| proc toCausalHistory*(messageIds: seq[SdsMessageID]): seq[HistoryEntry] = | ||
| ## Converts a sequence of message IDs to HistoryEntry sequence | ||
| result = newSeq[HistoryEntry](messageIds.len) |
There was a problem hiding this comment.
Ah, we don't use result returns: https://status-im.github.io/nim-style-guide/language.result.html
| if not rm.onRetrievalHint.isNil() and dep.retrievalHint.len == 0: | ||
| let hint = rm.onRetrievalHint(dep.messageId) | ||
| missingDeps.add(newHistoryEntry(dep.messageId, hint)) |
There was a problem hiding this comment.
I'm not sure I get the point of this logic:
- We have received a message and are checking dependencies
- A dependency is missing (from library history cache) and has no retrieval hint.
- We somehow believe the app incorporating this library will have a retrieval hint for this dependency even if we didn't receive it from the remote? I guess this is very unlikely to happen if this is an actual missing dependency - for example, if the Waku message ID is the retrieval hint, this is only visible to an app if it has received the message or crafted it.
In other words, doesn't it make more sense to say a retrieval hint must always come from the remote party. If it's not there we simply do not have a retrieval hint?
| if not rm.onRetrievalHint.isNil(): | ||
| for dep in deps: | ||
| if dep.retrievalHint.len == 0: | ||
| let hint = rm.onRetrievalHint(dep.messageId) | ||
| missingDeps.add(newHistoryEntry(dep.messageId, hint)) | ||
| else: | ||
| missingDeps.add(dep) |
There was a problem hiding this comment.
Again, I might be missing something obvious, but I'm not sure how an app could fill in the retrieval hint for received missing dependencies?
|
@adklempner I would appreciate your eye on this - not necessarily reviewing Nim code, but a sanity check on the logic of the @Ivansete-status feel free to nominate anyone from the nwaku team to help check Nim usage here (the detail of how SDS retrieval hints work is less important). |
Ivansete-status
left a comment
Ivansete-status
left a comment
There was a problem hiding this comment.
LGTM re Nim PoV! Thanks!
Only need to avoid the use of result
| ) | ||
|
|
||
| if not isNil(hint) and hintLen > 0: | ||
| result = newSeq[byte](hintLen) |
There was a problem hiding this comment.
We should always avoid the use of the implicit result
| msg.causalHistory = causalHistory | ||
| # Handle both old and new causal history formats | ||
| var historyBuffers: seq[seq[byte]] | ||
| if pb.getRepeatedField(3, historyBuffers).isOk: |
There was a problem hiding this comment.
Super nitpick one. Verbs with parenthesis; nouns without
| if pb.getRepeatedField(3, historyBuffers).isOk: | |
| if pb.getRepeatedField(3, historyBuffers).isOk(): |
Implements the SDS spec update: vacp2p/rfc-index#130 and closes the issue #11.
Summary: