Default to padding blinded paths #4213
Conversation
TheBlueMatt
commented
Nov 10, 2025
TheBlueMatt
commented
Because they end up both being used to validate a `Bolt12Invoice`, we ended up with a single `OffersContext` both for inclusion in a `Refund` and an `InvoiceRequest`. However, this is ambiguous, and while it doesn't seem like an issue, it also seems like a nice property to only use a given `OffersContext` in one place. Further, in the next commit, we use `OffersContext` to figure out what we're building a blinded path for and changing behavior based on it, so its nice to be unambiguous. Thus, we split the single existing context into `OutboundPaymentInRefund` and `OutboundPaymentInInvReq`.
|
👋 Thanks for assigning @valentinewallace as a reviewer! |
After much discussion in lightningdevkit#3246 we mostly decided to allow downstream developers to override whatever decisions the `DefaultMessageRouter` makes regarding blinded path selection by providing easy overrides for the selected `OnionMessageRouter`. We did not, however, actually select good defaults for `DefaultMessageRouter`. Here we add those defaults, taking advantage of the `MessageContext` we're given to detect why we're building a blinded path and selecting blinding and compaction parameters based on it. Specifically, if the blinded path is not being built for an offers context, we always use a non-compact blinded path and always pad it to four hops (including the recipient). However, if the blinded path is being built for an `Offers` context which implies it might need to fit in a QR code (or, worse, a payment onion), we reduce our padding and try to build a compact blinded path if possible. We retain the `NodeIdMessageRouter` to disable compact blinded path creation but use the same path-padding heuristic as for `DefaultMessageRouter`.
TheBlueMatt
force-pushed
the
2025-10-pad-use-non-compact-bps
branch
from
65206f0 to
c7d1621
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4213 +/- ##
==========================================
+ Coverage 89.33% 89.34% +0.01%
==========================================
Files 180 180
Lines 138055 138086 +31
Branches 138055 138086 +31
==========================================
+ Hits 123326 123368 +42
+ Misses 12122 12116 -6
+ Partials 2607 2602 -5
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:
|
|
🔔 1st Reminder Hey @valentinewallace! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @valentinewallace! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @valentinewallace! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @valentinewallace! This PR has been waiting for your review. |
|
🔔 5th Reminder Hey @valentinewallace! This PR has been waiting for your review. |
|
🔔 6th Reminder Hey @valentinewallace! This PR has been waiting for your review. |
|
🔔 7th Reminder Hey @valentinewallace! This PR has been waiting for your review. |
|
🔔 8th Reminder Hey @valentinewallace! This PR has been waiting for your review. |
|
🔔 9th Reminder Hey @valentinewallace! This PR has been waiting for your review. |
|
🔔 10th Reminder Hey @valentinewallace! This PR has been waiting for your review. |
| expected_path_length: usize, | ||
| ) -> bool { | ||
| let introduction_node_id = resolve_introduction_node(lookup_node, path); | ||
| introduction_node_id == expected_introduction_node |
There was a problem hiding this comment.
We previously had a check that the intro node was encoded as an scid, should we restore that? Or maybe it changed due to the never-compact option
There was a problem hiding this comment.
We quite often use non-compact paths now, as we generally try to use them any time where we're not space-constrained, so such a check fails a handful of test.
|
|
||
| let round_off = if compact_padding { | ||
| // We can only pad by a minimum of two bytes. Thus, if there are any intermediate hops that | ||
| // need to be padded by exactly one byte, we have to instead pad everything by two. |
There was a problem hiding this comment.
Not sure there's a clear solution, but "padding" in this PR can refer to either whole-path padding (i.e. dummy hops) or per-hop padding (i.e. making (at least some of) the hops equal size), and it's a bit confusing. Would prefer using the dummy hop terminology for whole-path padding.
There was a problem hiding this comment.
Rewrote a bunch of comments/consts to try to be more consistent.
| intermediate_tlvs.clone().any(|tlvs| tlvs.serialized_length() == max_intermediate_len - 1); | ||
|
|
||
| let round_off = if compact_padding { | ||
| // We can only pad by a minimum of two bytes. Thus, if there are any intermediate hops that |
There was a problem hiding this comment.
I didn't review the prior padding PRs -- why can we only pad by a minimum of two bytes?
There was a problem hiding this comment.
Its a TLV - type and length are two bytes.
| /// message, and thus an `Err` is returned. | ||
| /// message, and thus an `Err` is returned. The impact of this may be somewhat muted when | ||
| /// additional padding is added to the blinded path, but this protection is not complete. | ||
| pub struct NodeIdMessageRouter<G: Deref<Target = NetworkGraph<L>>, L: Deref, ES: Deref> |
There was a problem hiding this comment.
Not related to the PR per se, I'm wondering if we can get rid of this struct? I don't see it used anywhere and the docs don't indicate when it should be used
There was a problem hiding this comment.
The intent is that someone may want to override the router (eg using the flow's create_offer_builder_using_router).
|
👋 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. |
If we're building a blinded message path with extra dummy hops, we have to ensure we at least hide the length of the data in pre-final hops as otherwise the dummy hops are trivially obvious. Here we do so, taking an extra `bool` parameter to `BlindedMessagePath` constructors to decide whether to pad every hop to the existing `MESSAGE_PADDING_ROUND_OFF` or whether to only ensure that each non-final hop has an identical hop data length. In cases where the `DefaultMessageRouter` opts to use compact paths, it now also selects compact padding, whether short channel IDs are available or not.
TheBlueMatt
force-pushed
the
2025-10-pad-use-non-compact-bps
branch
from
c7d1621 to
649ba34
Compare
3 participants
