Simplify onion message codec #2060
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The scodec magic was quite hard to read, and the use of the prefix wasn't very intuitive since Sphinx uses both a prefix and a suffix. Also added more codec tests.
Codecov Report
@@ Coverage Diff @@
## master #2060 +/- ##
==========================================
- Coverage 87.57% 87.57% -0.01%
==========================================
Files 165 165
Lines 12696 12694 -2
Branches 555 553 -2
==========================================
- Hits 11119 11117 -2
Misses 1577 1577
|
thomash-acinq
approved these changes
Nov 9, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR was quite a trip. It started because I realized after merging #1957 that since the sphinx packet doesn't really use a prefix, but rather a mix of prefix and suffix (since the mac is encoded after the payload), I actually wasn't sure the
_ - 66, _ + 66trick was correct.So I dived into the tests and added new ones playing with length more extensively, and I ended up with some failing tests. At some point I thought our
bytes32codec was the culprit, so I added tests for that, but it turned out it wasn't the culprit (but I think it's worth keeping the tests, we didn't have enough coverage there). It took me quite a while to realize that it was actually my test that wasn't correctly written 🤕Now everything is green, we have more coverage (in particular for the remaining bytes / not enough bytes cases) and I think the codec is simpler to understand, let me know what you think.
Note that the previous codec was correct, it did pass all tests, but I found it harder to understand (scodec wizardry).