Skip to content

Conversation

@TheBlueMatt
Copy link
Collaborator

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.

Also,

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.

Progress towards full solution for #4313, but enough to get folks started.

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.
@TheBlueMatt TheBlueMatt added this to the 0.3 milestone Jan 22, 2026
@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Jan 22, 2026

👋 Thanks for assigning @joostjager as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

/// 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> {
Copy link
Contributor

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.

Copy link
Collaborator Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think lower level unit tests do have value even if there are higher level tests already.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really sure what you'd want to test? That it can decrypt correctly? That's definitely well-covered. The one bug we did have in it was found by our fuzz test. More test coverage is great but more tests to get the same coverage seems like a net-negative.

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 } =
Copy link
Contributor

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

Copy link
Collaborator Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use Local and Phantom in the enum? This adapter isn't used for anything else.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because that's really confusing when you read the stream type that isn't dedicated to only use in blinded paths. I'm not at all convinced the callsites are unclear, and we also might need to/want to upstream this at some point to rust-bitcoin.

.iter()
.filter(|chan| chan.is_usable)
.filter_map(|chan| chan.short_channel_id)
.min(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why min?

Copy link
Collaborator Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that to maximize the chances that it still exist? Curious where the existing logic is located also.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, basically. Absent any better criteria we assume that the oldest channel is most likely to stick around. We do it in get_peers_for_blinded_path as well using funded_channel.context.channel_creation_height

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;
Copy link
Contributor

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?

Copy link
Collaborator Author

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.

@ldk-reviews-bot
Copy link

👋 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.

@TheBlueMatt TheBlueMatt force-pushed the 2025-01-phantom-bolt12 branch from 8bbc7b1 to 736cab5 Compare January 26, 2026 21:09
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.
@TheBlueMatt TheBlueMatt force-pushed the 2025-01-phantom-bolt12 branch from 736cab5 to 5ef29ff Compare January 26, 2026 23:10
@codecov
Copy link

codecov bot commented Jan 27, 2026

Codecov Report

❌ Patch coverage is 94.05405% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.08%. Comparing base (7785360) to head (6c4d335).
⚠️ Report is 17 commits behind head on main.

Files with missing lines Patch % Lines
lightning/src/ln/channelmanager.rs 92.68% 2 Missing and 1 partial ⚠️
lightning/src/util/test_utils.rs 76.92% 3 Missing ⚠️
lightning/src/offers/flow.rs 94.59% 1 Missing and 1 partial ⚠️
lightning/src/blinded_path/payment.rs 83.33% 0 Missing and 1 partial ⚠️
lightning/src/ln/msgs.rs 90.90% 0 Missing and 1 partial ⚠️
lightning/src/onion_message/packet.rs 95.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4335      +/-   ##
==========================================
- Coverage   86.53%   86.08%   -0.45%     
==========================================
  Files         158      156       -2     
  Lines      103188   102554     -634     
  Branches   103188   102554     -634     
==========================================
- Hits        89292    88287    -1005     
- Misses      11471    11772     +301     
- Partials     2425     2495      +70     
Flag Coverage Δ
fuzzing ?
tests 86.08% <94.05%> (+0.26%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

joostjager
joostjager previously approved these changes Jan 27, 2026
Copy link
Contributor

@joostjager joostjager left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking comments. I do think that renaming the enum values would be a good idea. Curious to hear from 2nd 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> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think lower level unit tests do have value even if there are higher level tests already.

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 } =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use Local and Phantom in the enum? This adapter isn't used for anything else.

.iter()
.filter(|chan| chan.is_usable)
.filter_map(|chan| chan.short_channel_id)
.min(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that to maximize the chances that it still exist? Curious where the existing logic is located also.

let keys_manager = if predefined_keys_ids.is_some() {
let mut seed = [i as u8; 32];
if phantom {
// We would ideally randomize keys on every test run, but some tests fail in that case.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are pros and cons. Fully deterministic tests are desirable too sometimes.

@ldk-reviews-bot
Copy link

✅ Added second reviewer: @jkczyz

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

3 participants