-
Notifications
You must be signed in to change notification settings - Fork 125
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
Implement the onion-routing as per spec #2
Conversation
Let me know if I should squash the PR into one commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Christian!
Everything looks good, other than the fact that the latest commit breaks the tests since it ignores the e2e payload:
--- FAIL: TestSphinxCorrectness (0.07s)
sphinx_test.go:83: Final message parsed incorrectly at final destination!Should be [116 101 115 116 105 110 103], is instead []
Once that's fixed, I'll get this merged swiftly 👍
// Randomize the DH group element for the next hop using the | ||
// deterministic blinding factor. | ||
blindingFactor := computeBlindingFactor(dhKey, sharedSecret[:]) | ||
nextDHKey := blindGroupElement(dhKey, blindingFactor[:]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
I really like the change to unconditionally re-blind the current group element as this eliminates a timing leak in the previous code as the exit-node would skip that step.
Also, if you could squash everything into a single commit after fixing the tests, that'd be great. Thanks! |
This includes the creation of a command line tool to cross-check with the C implementation, adding of per-hop payloads and switch of cipher suite to ChaCha20 + SHA256-HMAC. Also streamlined some of the processing and externalized some of the parameters to get deterministic onions.
As discussed in milan we no longer have an e2e payload in version 0 of the spec.
Associated data is appended to the serialized packet when computing the HMAC, this allos for example to commit to data that is transferred alongside the onion packet, e.g., the payment hash of an HTLC.
Ok, I took the liberty of also adding the associated data patch in there and keeping the removal of the e2e payload in a separate commit so we can dig it up should we need it later. Travis is not updating the build status due to the DNS outage today, but the build passed :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The unit test has been amended to no longer test for e2e payload, since it was removed completely from the spec.
A new unit test for the associated data was included to make sure we are actually including it in the HMAC computation.
I forgot to mention, the 0.5 release of our |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM ⚡️
This implementation is compatible with the latest c-lightning implementation of the sphinx onion routing protocol: cdecker/lightning@04ea0c8
The implementation now uses a spec compatible serialization, uses CHACHA20 for exncryption and cipher stream generation, adds per-hop payloads, and includes the payloads into the per-hop HMACs.
Simplified the padding generation and packet construction, and externalized the sessionKey for deterministic testing.
In addition it gets rid of the additional 40 bytes in the routing-info that were unused. It creates a new command line tool to generate and process onions from the command line, that is then used to check compatibility with other implementations. For a simple test including bytewise comparisons and cross encoding/decoding see https://gist.github.com/cdecker/19086ba600de5d6981661fde14e72409