[WIP] Add core e2e reliability constructs and functions #1
Conversation
split to common and utils, logging, error handling, thread safety
shash256
changed the title
There was a problem hiding this comment.
Thanks! It's good to see e2e reliability taking shape. I have added some comments below as a start. However, I think it will also be good to split this PR into (significantly) smaller increments in order to get thorough review also from nwaku developers on the Nim usage. For example, my proposed order of PRs would be:
- Consider comments and open PR for only the bloom filter modules.
- Consider code reorganisation re my top-level comment under "common.nim" and open incremental PRs only for one or two types at a time - e.g. a PR only for the type and util functions related to
MessageandReliabilityManager.
| import private/probabilities | ||
|
|
||
| # Import MurmurHash3 code and compile at the same time as Nim code | ||
| {.compile: "murmur3.c".} |
There was a problem hiding this comment.
Perhaps I'm missing something important, but afaics this murmur3 import should be unnecessary as Nim's standard hashing already use murmur hashes by default for strings. See the implementation here: https://github.com/nim-lang/Nim/blob/devel/lib/pure/hashes.nim#L527-L542
| mBits*: int | ||
| intArray: seq[int] | ||
| nBitsPerElem*: int | ||
| useMurmurHash*: bool |
There was a problem hiding this comment.
For simplicity's sake I wouldn't add an option for different hashes here. However, as mentioned earlier, it seems to me that the Nim "standard" hashing are murmur hashes in any case? https://github.com/nim-lang/Nim/blob/devel/lib/pure/hashes.nim#L527-L542
| except: | ||
| return err[ReliabilityManager](reOutOfMemory) | ||
|
|
||
| proc wrapOutgoingMessage*(rm: ReliabilityManager, message: string): Result[Message] = |
There was a problem hiding this comment.
Presumably what would be more useful to the application is if the API takes a raw byte array (seq[byte]) and similarly returns a raw byte array.
| MessageID* = string | ||
|
|
||
| Message* = object | ||
| senderId*: string |
There was a problem hiding this comment.
I know that we included this in the forum post, but while we have no use for the senderId we can exclude it from the implementation.
| try: | ||
| let msg = Message( | ||
| senderId: "TODO_SENDER_ID", | ||
| messageId: generateUniqueID(), |
There was a problem hiding this comment.
I think for the application to be able to use message IDs to request missing messages from store nodes, this would have to be an id that is provided by the app to the API library (in other words, it cannot simply be generated internally).
There was a problem hiding this comment.
In terms of code organisation, we avoid using general "catch-all" modules like utils, commons, types, etc. where it can be avoided (there are of course exceptions where these type of modules make good sense). We'd rather organise related types and functions together in descriptive modules. For example, in this case I would create a module for message.nim, reliability_manager.nim, etc. and add the type and util functions related to that type in that module. Care must be taken to not cause circular dependencies with the import statements, but I think that should be possible in this case.
| Result*[T] = object | ||
| case isOk*: bool | ||
| of true: | ||
| value*: T | ||
| of false: | ||
| error*: ReliabilityError |
There was a problem hiding this comment.
Use nim-results: https://github.com/arnetheduck/nim-results
This is the first draft which adds core e2e reliability features