Large final onion payload fixes #2752
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
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2752 +/- ##
=======================================
Coverage 88.53% 88.54%
=======================================
Files 115 115
Lines 91011 91094 +83
Branches 91011 91094 +83
=======================================
+ Hits 80580 80659 +79
- Misses 8004 8013 +9
+ Partials 2427 2422 -5 ☔ View full report in Codecov by Sentry. |
valentinewallace
force-pushed
the
2023-11-large-final-onion-payload-fixes
branch
from
d907b9a
to
020eb9a
Compare
valentinewallace
force-pushed
the
2023-11-large-final-onion-payload-fixes
branch
from
020eb9a
to
6b8735b
Compare
TheBlueMatt
reviewed
Nov 30, 2023
valentinewallace
force-pushed
the
2023-11-large-final-onion-payload-fixes
branch
2 times, most recently
from
610e876
to
5494ddd
Compare
|
Rebased to fix CI. |
TheBlueMatt
reviewed
Dec 5, 2023
lightning/src/ln/onion_utils.rs
Outdated
| @@ -322,7 +322,15 @@ fn construct_onion_packet_with_init_noise<HD: Writeable, P: Packet>( | |||
| let mut res = Vec::with_capacity(ONION_HOP_DATA_LEN * (payloads.len() - 1)); | |||
|
|
|||
| let mut pos = 0; | |||
| let mut packet_len_without_filler = 0; | |||
There was a problem hiding this comment.
This variable can be elided entirely, just use (or rename) pos.
There was a problem hiding this comment.
Removed the extra var. It means we'll unnecessarily seek in chacha on the final payload, though.
valentinewallace
force-pushed
the
2023-11-large-final-onion-payload-fixes
branch
from
5494ddd
to
ca25ca6
Compare
We previously assumed that the final node's payload would be ~93 bytes, and had code to ensure that the filler encoded after that payload is not all 0s. Now with custom TLVs and metadata supported, the final node's payload may take up the entire onion packet, so we can't assume that there are 64 bytes of filler to check.
Ensure that if we call construct_onion_packet and friends where payloads are too large for the allotted packet length, we'll fail to construct. Previously, senders would happily construct invalid packets by array-shifting the final node's HMAC out of the packet when adding an intermediate onion layer, causing the receiver to error with "final payload provided for us as an intermediate node."
valentinewallace
force-pushed
the
2023-11-large-final-onion-payload-fixes
branch
from
ca25ca6
to
80ba9ac
Compare
|
Rebased to fix CI. |
TheBlueMatt
approved these changes
Dec 8, 2023
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.
See the last two commit messages. We had a debug assertion and packet construction code that are no longer valid now that final payloads may be large due to custom TLVs/metadata. H/t to @Evanfeenstra for pointing out the latter, which helped uncover the former.
Based on #2739.