-
Notifications
You must be signed in to change notification settings - Fork 432
Add support for "phantom" BOLT 12 offers, up to the invoice_request step #4335
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
base: main
Are you sure you want to change the base?
Add support for "phantom" BOLT 12 offers, up to the invoice_request step #4335
Conversation
It turns out we also switched the key we use to authenticate offers *created* in the 0.2 upgrade and as a result downgrading to 0.2 will break any offers created on 0.2. This wasn't intentional but it doesn't really seem worth fixing at this point, so just document it.
In the coming commits we'll add support for building a blinded path which can be received to any one of several nodes in a "phantom" configuration (terminology we retain from BOLT 11 though there are no longer any phantom nodes in the paths). Here we adda new key in `ExpandedKey` which we can use to authenticate blinded paths as coming from a phantom node participant.
In the next commit we'll add support for building a BOLT 12 offer which can be paid to any one of a number of participant nodes. Here we add support for validating blinded paths as coming from one of the participating nodes by deriving a new key as a part of the `ExpandedKey`. We keep this separate from the existing `ReceiveAuthKey` which is node-specific to ensure that we only allow this key to be used for blinded payment paths and contexts in `invoice_request` messages. This ensures that normal onion messages are still tied to specific nodes. Note that we will not yet use the blinded payment path phantom support which requires additional future work. However, allowing them to be authenticated in a phantom configuration should allow for compatibility across versions once the building logic lands.
|
👋 Thanks for assigning @joostjager as a reviewer! |
| /// checked, effectively treating the contents as the AAD for the AAD-containing MAC but behaving | ||
| /// like classic ChaCha20Poly1305 for the non-AAD-containing MAC. | ||
| pub(crate) struct ChaChaDualPolyReadAdapter<R: Readable> { | ||
| pub(crate) struct ChaChaTriPolyReadAdapter<R: Readable> { |
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.
Would you say there is sufficient test coverage on this? I think there is some higher-level coverage, but no direct unit test.
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.
I think so? I mean if its horribly broken the higher-level code will also break. Its also somewhat indirectly fuzzed through the onion decoding fuzzing.
| let ChaChaDualPolyReadAdapter { readable, used_aad } = | ||
| ChaChaDualPolyReadAdapter::read(&mut reader, (rho, receive_auth_key.0)) | ||
| .map_err(|_| ())?; | ||
| let ChaChaTriPolyReadAdapter { readable, used_aad_a, used_aad_b } = |
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.
Names like used_local and used_phantom could be helpful
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.
Sadly I can't rename the fields since I switched to an enum.
lightning/src/ln/msgs.rs
Outdated
| }), | ||
| used_aad, | ||
| used_aad_a, | ||
| used_aad_b, |
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.
local/phantom in name?
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.
Sadly I can't rename the fields since I switched to an enum.
| .iter() | ||
| .filter(|chan| chan.is_usable) | ||
| .filter_map(|chan| chan.short_channel_id) | ||
| .min(), |
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.
Why min?
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.
Its the oldest, matching the existing logic we have for the non-phantom case.
| node_signer.ecdh(Recipient::Node, &self.inner_path.blinding_point, None)?; | ||
| let rho = onion_utils::gen_rho_from_shared_secret(&control_tlvs_ss.secret_bytes()); | ||
| let receive_auth_key = node_signer.get_receive_auth_key(); | ||
| let phantom_auth_key = node_signer.get_expanded_key().phantom_node_blinded_path_key; |
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.
You could have None here in case the node isn't configured for phantom payments. I think that might help keep that case in mind for readers, and also saves some computation?
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.
Yea, we don't currently have a separate method on the signer that returns None in cases where we aren't doing phantom so we'd have to add a new key-fetcher (or bool-fetcher) to decide whether to do phantom validation. I thought about doing that but it seemed way cleaner to reuse the ExpandedKey (which is already used for all the inbound-payment key logic, including phantom). Its also not very much compute at all to just do one round of poly1305 + the finish logic so I'm not sure its worth the extra work.
| /// | ||
| /// [`ExpandedKey`]: inbound_payment::ExpandedKey | ||
| pub fn create_phantom_offer_builder( | ||
| &$self, other_nodes_channels: Vec<(PublicKey, Vec<ChannelDetails>)>, |
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.
Wouldn't passing in MessageForwardNode directly create more flexibility?
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.
Hmm, in theory, but I'm not quite sure what the use-case for it would be? Someone could already filter peers by filtering the ChannelDetails. I guess it would let someone include a MessageForwardNode for a peer they don't have a channel with, but that seems a bit strange to do especially for a phantom setup. Avoiding it means we don't have to add a new public method on ChannelManager which seems marginally nicer than not.
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
8bbc7b1 to
736cab5
Compare
In the BOLT 11 world, we have specific support for what we call "phantom nodes" - creating invoices which can be paid to any one of a number of nodes by adding route-hints which represent nodes that do not exist. In BOLT 12, blinded paths make a similar feature much simpler - we can simply add blinded paths which terminate at different nodes. The blinding means that the sender is none the wiser. Here we add logic to fetch an `OfferBuilder` which can generate an offer payable to any one of a set of nodes. We retain the "phantom" terminology even though there are no longer any "phantom" nodes. Note that the current logic only supports the `invoice_request` message going to any of the participating nodes, it then replies with a `Bolt12Invoice` which can only be paid to the responding node. Future work may relax this restriction.
736cab5 to
5ef29ff
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4335 +/- ##
==========================================
- Coverage 86.53% 86.08% -0.45%
==========================================
Files 158 156 -2
Lines 103188 102542 -646
Branches 103188 102542 -646
==========================================
- Hits 89292 88274 -1018
- Misses 11471 11772 +301
- Partials 2425 2496 +71
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
In the BOLT 11 world, we have specific support for what we call
"phantom nodes" - creating invoices which can be paid to any one of
a number of nodes by adding route-hints which represent nodes that
do not exist.
In BOLT 12, blinded paths make a similar feature much simpler - we
can simply add blinded paths which terminate at different nodes.
The blinding means that the sender is none the wiser.
Here we add logic to fetch an
OfferBuilderwhich can generate anoffer payable to any one of a set of nodes. We retain the "phantom"
terminology even though there are no longer any "phantom" nodes.
Note that the current logic only supports the
invoice_requestmessage going to any of the participating nodes, it then replies
with a
Bolt12Invoicewhich can only be paid to the respondingnode. Future work may relax this restriction.
Also,
Progress towards full solution for #4313, but enough to get folks started.