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

Mixnet: Sphinx packet specification #46

Closed
wants to merge 1 commit into from

Conversation

youngjoon-lee
Copy link
Contributor

@youngjoon-lee youngjoon-lee commented Jan 11, 2024

This is the 2nd PR of the series of Mixnet specification.

Coverage

This PR covers https://www.notion.so/Sphinx-Packet-Specification-1c96e0e8aa8a4241ae9c1417c67714b7.
As mentioned in Notion, this Sphinx specification has been already fully implemented by https://github.com/nymtech/sphinx, and I think we can just use their implementation as it is. I've read almost everything from their source code and understand how it works. I summarized my understanding in the Notion above, so that you don't need to read the entire Sphinx paper.
At first, I felt there was no need to write this in Python. However, their Rust implementation isn't very clean to understand, and I thought it would be better than leaving this as a black box.

Code review tips

This PR is huge, but I think it's better than splitting this into several PRs, because I think that doesn't help you understand the big picture.
I would say we don't need to review every details line by line because I didn’t make any modification from the original spec and this is not what we have to focus on right now. I’m actually thinking about publishing this as a package to somewhere else and use it here, so that we don’t waste our time to maintain this later.
Some parts are not pretty in perspective of engineering, but I tried to make it as similar as possible with https://github.com/nymtech/sphinx using the terms defined in the Sphinx paper.
I recommend to read the unit test mixnet/sphinx/test_sphinx.py, which gives you information about how to use Sphinx, and the Notion that has some diagrams.
If you understand the big picture, I'd say you can just jump to the next PR.

p.s.
I added only developers as code reviewers because this PR is just about what already fully designed by Sphinx paper and doesn't have any modification from it. But, please feel free to have a look if anyone is interested in. For next PRs, I'm going to any more people if necessary.

@danielSanchezQ
Copy link
Collaborator

This is full package outside of the scope of the specification we need itself. It is super nice work though, deserves to be published.

@youngjoon-lee
Copy link
Contributor Author

This is full package outside of the scope of the specification we need itself. It is super nice work though, deserves to be published.

Yes, thanks. As we discussed, I'll move this code to a new repo somewhere else, so that it can be used in this repo.
I'm closing this now, so others don't waste their time to review this.

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.

None yet

2 participants