Uh oh!
There was an error while loading. Please reload this page.
Onion messages: support reply paths - #1652
Conversation
Prior to this change, we could have failed to decode a valid payload of size >253. This is because we were decoding the length (a BigSize, big-endian) as a VarInt (little-endian). Found in lightningdevkit#1652.
Prior to this change, we could have failed to decode a valid payload of size >253. This is because we were decoding the length (a BigSize, big-endian) as a VarInt (little-endian). Found in lightningdevkit#1652.
TheBlueMatt
left a comment
There was a problem hiding this comment.
Looks pretty straightforward, only one real comment.
Uh oh!
There was an error while loading. Please reload this page.
| /// Send an empty onion message to `destination`, routing it through `intermediate_nodes`. | ||
| /// See [`OnionMessenger`] for example usage. | ||
| pub fn send_onion_message(&self, intermediate_nodes: &[PublicKey], destination: Destination) -> Result<(), SendError> { | ||
| pub fn send_onion_message(&self, intermediate_nodes: &[PublicKey], destination: Destination, reply_path: Option<BlindedRoute>) -> Result<(), SendError> { |
There was a problem hiding this comment.
I assume in the future once we have a router to figure out message routes we'll make the argument a simple boolean and do a path lookup for a reply path route (or in a new function or whatever)?
There was a problem hiding this comment.
Hmm I think that makes sense. This API may remain pretty low-level and keep a version of the parameter, but we'll need some way of getting a reply_path for outbound invoice_requests, likely via the router you mention.
b698401 to
5afb049ComparePrior to this change, we could have failed to decode a valid payload of size >253. This is because we were decoding the length (a BigSize, big-endian) as a VarInt (little-endian). Found in lightningdevkit#1652.
Prior to this change, we could have failed to decode a valid payload of size >253. This is because we were decoding the length (a BigSize, big-endian) as a VarInt (little-endian). Found in lightningdevkit#1652.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
5afb049 to
9e6c7dcCompare9e6c7dc to
132d7f5CompareTheBlueMatt
commented
Aug 24, 2022
Fuzz test is failing now. |
TheBlueMatt
commented
Aug 24, 2022
Otherwise LGTM. |
And fix one test to be uniform with the others
Previously, we were decoding payload lengths as a VarInt. Per the spec, this is wrong -- it should be decoded as a BigSize. This bug also exists in our payment payload decoding, to be fixed separately. Upcoming reply path tests caught this bug because we hadn't encoded a payload greater than 253 before, so we hadn't hit the problem that VarInts are encoded as little-endian whereas BigSizes are encoded as big-endian.
132d7f5 to
950b7d7Comparevalentinewallace
commented
Aug 24, 2022
Had to rebase to repro the test failure locally. Also removed some warnings that I didn't realize appeared on the last push. |
codecov-commenter
commented
Aug 24, 2022
Codecov Report
@@ Coverage Diff @@## main #1652 +/- ##
==========================================
+ Coverage 90.85% 90.87% +0.01%
==========================================
Files 85 85 Lines 45714 45755 +41 Branches 45714 45755 +41 ==========================================
+ Hits 41534 41580 +46 + Misses 4180 4175 -5
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
TheBlueMatt
left a comment
There was a problem hiding this comment.
LGTM assuming CI passes.
| } else if let Some(encrypted_payload) = enc_payload_opt { | ||
| payloads.push((Payload::Receive { | ||
| control_tlvs: ReceiveControlTlvs::Blinded(encrypted_payload), | ||
| reply_path: reply_path.take(), |
There was a problem hiding this comment.
IIUC, reply_path is captured by reference (I think?) in the closure, so it will be None if this case is hit more than once when called by construct_keys_callback. Is this the desired behavior? I guess so because there would only be one Payload::Receive?
Related, the same may occur below where reply_path.take() is called again outside of the closure. Partly trying to check my understanding if that case will be hit below but not in the closure.
There was a problem hiding this comment.
Yep, that's intentional. There should only be one Payload::Receive pushed, so reply_path should only be taken once.
Adds support for sending reply paths in our onion messages, and for decoding them on onion message read.