Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Add core e2e reliability constructs and functions #1

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

shash256
Copy link
Collaborator

This is the first draft which adds core e2e reliability features

@shash256 shash256 self-assigned this Oct 21, 2024
@shash256 shash256 changed the title [WIP] Add core reliability constructs and functions [WIP] Add core e2e reliability constructs and functions Oct 21, 2024
@shash256 shash256 linked an issue Oct 21, 2024 that may be closed by this pull request
7 tasks
@shash256 shash256 requested a review from jm-clius October 21, 2024 11:28
Copy link

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Consider comments and open PR for only the bloom filter modules.
  2. 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 Message and ReliabilityManager.

import private/probabilities

# Import MurmurHash3 code and compile at the same time as Nim code
{.compile: "murmur3.c".}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per benchmarks in the bloom filter PR, nim's standard hashes are not performing better (perhaps due to a nim re-implementation while this is C). The murmur3.c is the same file through which was referenced in nim hashes - https://github.com/nim-lang/Nim/blob/devel/lib/pure/hashes.nim#L307

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks for clarifying! Note, we are not trying to implement the most optimal hash implementation. We have a very simple use case for what would be comparatively small bloom filters. Our main focus is simply on getting the minimum set of bloom filter functionality that will allow us to build the SDS protocol - if it turns out to be too slow, we can certainly use these learnings to optimise. For now, adding a lot of new code and libraries without a clear need would add to the maintenance burden, complicate reviews, etc. :)

src/common.nim Outdated

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

Implement Rolling Bloom Filter for ReliabilityManager
2 participants