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

Rewrite Noise implementation #870

Merged
merged 5 commits into from
Jul 8, 2023
Merged

Rewrite Noise implementation #870

merged 5 commits into from
Jul 8, 2023

Conversation

tomaka
Copy link
Contributor

@tomaka tomaka commented Jul 8, 2023

cc #133

This PR switches from snow to directly using cryptographic primitives for the Noise protocol.

As you can see, this "only" adds 735 lines of code to noise.rs, which to me shows that the 6000 lines of code in the snow library are a bit exaggerated.

The benefits are:

  • We get rid of snow that is unmaintained and dependencies are more up-to-date.
  • It's a step towards Make the library compile for #![no_std] #133.
  • We can finally fix the fact that every single networking message gets memcpy'ed 2 or 3 times due to API issues.
  • We can finally fix the fact that the connection state machine might stall if the outgoing buffer is too small, which fortunately isn't an issue in practice but has always bothered me.

@tomaka
Copy link
Contributor Author

tomaka commented Jul 8, 2023

PRs like these really should be reviewed, but I know that nobody is going to review it anyway, so there's no point in waiting to merge.

@tomaka tomaka added this pull request to the merge queue Jul 8, 2023
Merged via the queue into smol-dot:main with commit fbcb60f Jul 8, 2023
@tomaka tomaka deleted the noise-rewrite branch July 8, 2023 16:45
@olanod
Copy link

olanod commented Aug 1, 2023

PRs like these really should be reviewed, but I know that nobody is going to review it anyway, so there's no point in waiting to merge.

Sounds sad, how could this be improved in the future? growing your team with more resources? would it help to place some kind of financial incentive on PRs to be reviewed(making incentives part of your budget) and share it with a competent group of people you know or more openly in a group like the fellowship?

Btw excited about no_std support :)

@tomaka
Copy link
Contributor Author

tomaka commented Aug 2, 2023

Sounds sad, how could this be improved in the future? growing your team with more resources? would it help to place some kind of financial incentive on PRs to be reviewed(making incentives part of your budget) and share it with a competent group of people you know or more openly in a group like the fellowship?

The problem is that there's "review" and "review". It's easy to skim 5 minutes over the code and say "looks good to me".
Reviewing this PR for example requires knowledge not only of the smoldot networking but also of the Noise protocol. A proper review would take 10-ish hours. Nobody is going to do that properly.

I don't really have a solution here.

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

Successfully merging this pull request may close these issues.

2 participants