Serialize Trampoline payloads in outbound onions. #2756
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2756 +/- ##
==========================================
+ Coverage 89.18% 90.05% +0.86%
==========================================
Files 117 117
Lines 95541 99616 +4075
Branches 95541 99616 +4075
==========================================
+ Hits 85205 89705 +4500
+ Misses 7840 7676 -164
+ Partials 2496 2235 -261 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
The first draft of the trampoline support looks good.
Referring to Spec PR requirements:
- Correctly implements the
trampoline_packetfield to allow receiving of trampoline onion packets - Ensure the same construction of
trampoline_packetsas theOnionPacket.
I have a few questions I would love to discuss. Looking forward to your insights.
lightning/src/ln/features.rs
Outdated
| @@ -415,6 +417,9 @@ mod sealed { | |||
| define_feature!(55, Keysend, [NodeContext], | |||
| "Feature flags for keysend payments.", set_keysend_optional, set_keysend_required, | |||
| supports_keysend, requires_keysend); | |||
| define_feature!(57, Trampoline, [NodeContext], | |||
There was a problem hiding this comment.
I couldn't find any description of the trampoline feature bit in BOLT#9. Why was the feature bit selected to be 57?
There was a problem hiding this comment.
There was a problem hiding this comment.
Thanks a lot for the reference!
lightning/src/ln/msgs.rs
Outdated
| @@ -2276,6 +2278,7 @@ impl Writeable for OutboundOnionPayload { | |||
| (2, HighZeroBytesDroppedBigSize(*amt_msat), required), | |||
| (4, HighZeroBytesDroppedBigSize(*outgoing_cltv_value), required), | |||
| (8, payment_data, option), | |||
| (12, trampoline_packet, option), | |||
There was a problem hiding this comment.
Also, I couldn't wrap my head around using the number 12 here. Can you direct me to the reasoning behind it?
Thanks!
There was a problem hiding this comment.
There was a problem hiding this comment.
Note that this might still change depending on if it should play nicely with blinded paths, I believe? cc: @t-bast
There was a problem hiding this comment.
Yes, all the numbers in the current spec PR are obsolete, they are from many years ago and some of the TLV tags have already been allocated to other features. I was waiting for another implementation to seriously look at Trampoline before re-working the numbers. For now, I think you should use placeholder numbers here (large ones to avoid clashes), that will be replaced by the final numbers once we are ready for cross-compat tests!
There was a problem hiding this comment.
@t-bast, do you encode the hop_data length using a TLV inside the inner onion? Given that version, pubkey, and hmac are all constant length, it could in principle be optional, but would preclude the addition of other variable-length fields.
There was a problem hiding this comment.
I ran the Eclair unit tests and determined that you do not. Should LDK drop the length serialization, too, or should it be added in Eclair?
There was a problem hiding this comment.
For the time being, I dropped the length serialization to achieve full parity with Eclair.
|
Note that this PR is still in draft while I figure out some changes pertaining to the inner onion length. |
arik-so
force-pushed
the
arik/trampoline/2023-11-outbound
branch
3 times, most recently
from
0ec5232
to
528dc98
Compare
arik-so
force-pushed
the
arik/trampoline/2023-11-outbound
branch
2 times, most recently
from
7ee43a9
to
6870049
Compare
arik-so
requested review from
t-bast and
valentinewallace
and removed request for
t-bast
lightning/src/ln/msgs.rs
Outdated
| let keysend_tlv = keysend_preimage.map(|preimage| (5482373484, preimage.encode())); | ||
| let mut custom_tlvs: Vec<&(u64, Vec<u8>)> = custom_tlvs.iter().chain(keysend_tlv.iter()).collect(); | ||
| let trampoline_tlv = trampoline_packet.as_ref().map(|trampoline| (66100, trampoline.encode())); |
There was a problem hiding this comment.
Out of curiosity, I was able to find a reference for why keysend why keyed by 5482373484 in blip-0003, but I was able to find no such reference for 66100, for the trampoline. Is it an arbitrary choice for now, or is there some reasoning behind it?
There was a problem hiding this comment.
There was a problem hiding this comment.
Thanks a lot for the reference!
I think it will be a good idea to write a comment about it, explaining the reasoning behind it and the necessary caution one should keep.
Eclair's comment seems like a pretty solid one to take a reference from.
// Types below aren't specified - use cautiously when deploying (be careful with backwards-compatibility).
lightning/src/ln/msgs.rs
Outdated
| @@ -1787,6 +1789,29 @@ impl fmt::Debug for OnionPacket { | |||
| } | |||
| } | |||
|
|
|||
| /// BOLT 4 onion packet including hop data for the next peer. | |||
| #[derive(Clone, Hash, PartialEq, Eq)] | |||
| pub struct VariableLengthOnionPacket { | |||
There was a problem hiding this comment.
As per BOLT#9, the Trampoline is correctly introduced as a VariableLengthOnionPacket
The trampoline_onion_packet has a variable size to allow...
The struct looks good and is correctly implemented. And, I think it can use a little nudge on the documentation side so that its purpose is explained in more detail and it can be easily distinguishable on a documentation level from struct OnionPacket.
lightning/src/ln/msgs.rs
Outdated
| @@ -1789,6 +1789,29 @@ impl fmt::Debug for OnionPacket { | |||
| } | |||
| } | |||
|
|
|||
| /// BOLT 4 onion packet including hop data for the next peer. | |||
| #[derive(Clone, Hash, PartialEq, Eq)] | |||
| pub struct VariableLengthOnionPacket { | |||
There was a problem hiding this comment.
Can we consolidate this with onion_message::packet::Packet?
|
I will be closing this PR within the next couple days in favor of a more flexible implementation that does not rely on nesting onions. |
arik-so
force-pushed
the
arik/trampoline/2023-11-outbound
branch
from
1e7ca71
to
c7e1ee2
Compare
WalkthroughThe update introduces Trampoline routing to the Lightning Network's implementation, enhancing the protocol's scalability and privacy. Specifically, it adds a new feature flag for Trampoline routing, incorporates a Changes
Related issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (3)
- lightning/src/ln/features.rs (2 hunks)
- lightning/src/ln/msgs.rs (9 hunks)
- lightning/src/ln/onion_utils.rs (1 hunks)
Additional comments: 4
lightning/src/ln/features.rs (2)
- 425-427: The addition of the Trampoline feature flag is correctly implemented following the pattern established for other features. This change introduces support for Trampoline routing, aligning with the PR's objectives.
- 168-175: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-427]
The implementation and handling of feature flags, including the new Trampoline routing feature, follow the established patterns and conventions within the file. It's crucial to maintain consistency across the feature flag system to ensure interoperability and correct behavior in different contexts.
lightning/src/ln/msgs.rs (2)
- 4156-4188: The encoding for the final onion hop data with a trampoline packet is implemented correctly. The test checks that the trampoline packet is encoded as expected and matches the expected eclair trampoline packet format.
- 4195-4215: The test
encoding_final_onion_hop_data_with_eclair_trampoline_packetcorrectly verifies the encoding of a trampoline packet with a specific public key, hop data, and HMAC. The expected eclair trampoline packet is matched against the encoded trampoline packet to ensure correctness.
lightning/src/ln/onion_utils.rs
Outdated
| @@ -216,6 +216,7 @@ pub(super) fn build_onion_payloads(path: &Path, total_msat: u64, mut recipient_o | |||
| custom_tlvs: recipient_onion.custom_tlvs.clone(), | |||
| sender_intended_htlc_amt_msat: value_msat, | |||
| cltv_expiry_height: cltv, | |||
| trampoline_packet: None | |||
There was a problem hiding this comment.
tabs, not spaces, and in a few other places :)
There was a problem hiding this comment.
I'm also unhappy with my rebase, so I'll address it as part of resolving commit reordering conflicts.
lightning/src/ln/msgs.rs
Outdated
| let trampoline_tlv = trampoline_packet.as_ref().map(|trampoline| (66100, trampoline.encode())); | ||
| let mut custom_tlvs: Vec<&(u64, Vec<u8>)> = custom_tlvs.iter() | ||
| .chain(keysend_tlv.iter()) | ||
| .chain(trampoline_tlv.iter()) |
There was a problem hiding this comment.
These are required to be in order, do we require the custom_tlvs always be < 66k?
arik-so
force-pushed
the
arik/trampoline/2023-11-outbound
branch
from
c7e1ee2
to
8e01dc5
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- lightning/src/ln/features.rs (2 hunks)
- lightning/src/ln/msgs.rs (9 hunks)
- lightning/src/ln/onion_utils.rs (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- lightning/src/ln/features.rs
- lightning/src/ln/msgs.rs
- lightning/src/ln/onion_utils.rs
arik-so
force-pushed
the
arik/trampoline/2023-11-outbound
branch
from
8e01dc5
to
a678f76
Compare
lightning/src/ln/msgs.rs
Outdated
| } => { | ||
| // We need to update [`ln::outbound_payment::RecipientOnionFields::with_custom_tlvs`] | ||
| // to reject any reserved types in the experimental range if new ones are ever | ||
| // standardized. | ||
| let keysend_tlv = keysend_preimage.map(|preimage| (5482373484, preimage.encode())); | ||
| let mut custom_tlvs: Vec<&(u64, Vec<u8>)> = custom_tlvs.iter().chain(keysend_tlv.iter()).collect(); | ||
| let trampoline_tlv = trampoline_packet.as_ref().map(|trampoline| (66100, trampoline.encode())); |
There was a problem hiding this comment.
We need to refuse to allow custom TLVs of 66100 in RecipientOnionFields::with_custom_tlvs (and we probably need some kind of centralized place to list all the onion TLVs we understand so we can refuse users setting them and not have to update them in 10 places.
arik-so
force-pushed
the
arik/trampoline/2023-11-outbound
branch
from
a678f76
to
e5fb481
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- lightning/src/ln/features.rs (2 hunks)
- lightning/src/ln/msgs.rs (9 hunks)
- lightning/src/ln/onion_utils.rs (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- lightning/src/ln/features.rs
- lightning/src/ln/msgs.rs
- lightning/src/ln/onion_utils.rs
lightning/src/ln/msgs.rs
Outdated
| @@ -1740,6 +1740,7 @@ mod fuzzy_internal_msgs { | |||
| custom_tlvs: Vec<(u64, Vec<u8>)>, | |||
| sender_intended_htlc_amt_msat: u64, | |||
| cltv_expiry_height: u32, | |||
| trampoline_packet: Option<crate::onion_message::packet::Packet> | |||
There was a problem hiding this comment.
Huh? Why is this an onion message packet?
There was a problem hiding this comment.
IMO let's go ahead and get the next PR open to evaluate whether its right to reuse this struct for trampoline packets, then come back here. Luckily this PR is super small so we could even just inline it in a PR that uses the serialization.
There was a problem hiding this comment.
CC: @vladimirfomene: #2906 constructs variable-length onion packets and is awaiting a concept ACK besides @valentinewallace's, whereupon I'd like to undraft it. If that approach seems sensible to you, we would refactor oniion_message::packet to have a more generically applicable location and name.
arik-so
force-pushed
the
arik/trampoline/2023-11-outbound
branch
from
65688f9
to
4021400
Compare
arik-so
force-pushed
the
arik/trampoline/2023-11-outbound
branch
from
ff22217
to
e763184
Compare
arik-so
force-pushed
the
arik/trampoline/2023-11-outbound
branch
3 times, most recently
from
e165834
to
33afb56
Compare
arik-so
force-pushed
the
arik/trampoline/2023-11-outbound
branch
from
33afb56
to
37bb7e5
Compare
lightning/src/ln/msgs.rs
Outdated
| TrampolineEntry { | ||
| amt_to_forward: u64, | ||
| outgoing_cltv_value: u32, | ||
| payment_data: Option<FinalOnionHopData>, |
There was a problem hiding this comment.
I'm confused by this, presumably we don't store the PaymentSecret here, but should always have a total_mst? Or, why is this here?
There was a problem hiding this comment.
It's part of the spec. The last hop of the outer onion, where the Trampoline portion of the route is entered, contains a payment secret which is distinct from the "real" payment secret, which, in turn, is stored in the final hop of the inner onion. It's also part of the test vectors: https://github.com/lightning/bolts/blob/0c636259b41cb4c9e5cf7d8cfa84eb6e0397a30f/bolt04/trampoline-payment-onion-test.json#L45
@t-bast, do you figure it'd be worthwhile expanding a bit in the spec on how the roles of the fields in the outer onion are altered in a Trampoline context? As well as any arithmetic considerations for routing fees for a path whose length isn't known a priori?
There was a problem hiding this comment.
Ah, okay, interesting. Maybe lts name this field something different to differentiate from payments? Like, multipath_trampoline_data?
There was a problem hiding this comment.
No objection from me personally, though for our users' sake, it might be prudent to maintain fidelity to the spec terminology.
There was a problem hiding this comment.
In general we've avoided trying to match the spec exactly if we think it would make our code more readable. I'd strongly prefer to stick with that here.
There was a problem hiding this comment.
All of those fields work exactly the same as for normal payments, as the behavior of an intermediate trampoline node,
when processing HTLCs that contain a trampoline onion is to:
- first act as if it were a payment recipient, aggregating incoming payment parts
- then forward the "received" payment to a relayer component instead of treating it to a payment to us
Rusty also had a question about the payment_secret in the outer onion, as that part is a bit subtle: lightning/bolts#836 (comment).
In eclair we use the following trait hierarchy: https://github.com/ACINQ/eclair/blob/c8184b3e43a603454d672aed61b2ae461a80e1a8/eclair-core/src/main/scala/fr/acinq/eclair/wire/protocol/PaymentOnion.scala#L197. Once decrypted, we combine the outer and inner onion information received by an intermediate trampoline node as NodeRelayPayload, to show that this data is used for relay.
@t-bast, do you figure it'd be worthwhile expanding a bit in the lightning/bolts#836 on how the roles of the fields in the outer onion are altered in a Trampoline context?
I thought that was already detailed well enough in the requirements section of the specification, if there is a specific point you'd like to be more detailed can you put a comment on the spec?
As well as any arithmetic considerations for routing fees for a path whose length isn't known a priori?
That's something I described in the "trampoline fees" section of this spec PR: lightning/bolts#829
Let me know directly on that PR if the section is unclear or if you disagree with some of it. In eclair we previously used a very basic trial-and-error approach (start with a low fee, increase it on errors), and recently moved to a different model where the sender always uses 4% for the trampoline fees, and the trampoline node tries to deal with that (and could in the future do it at a loss in some payments as long as other payments compensate that loss).
arik-so
force-pushed
the
arik/trampoline/2023-11-outbound
branch
from
37bb7e5
to
e8f04f4
Compare
arik-so
force-pushed
the
arik/trampoline/2023-11-outbound
branch
from
e8f04f4
to
4761577
Compare
arik-so
force-pushed
the
arik/trampoline/2023-11-outbound
branch
2 times, most recently
from
a5ce9e3
to
9f55747
Compare
arik-so
force-pushed
the
arik/trampoline/2023-11-outbound
branch
from
9f55747
to
f15c538
Compare
| // Unlike the onion packets used for payments, Trampoline onion packets have to be shorter than | ||
| // 1300 bytes. The expected default is 650 bytes. | ||
| // TODO: if 650 ends up being the most common size, optimize this to be: | ||
| // enum { ThirteenHundred([u8; 650]), VarLen(Vec<u8>) } |
There was a problem hiding this comment.
nit: s/ThirteenHundred/SixFifty, I guess
There was a problem hiding this comment.
Addressed in #2906.
valentinewallace
merged commit 6d11111
into
lightningdevkit:main
This PR alters the outbound onion payload to optionally accommodate a Trampoline packet.
Additionally, we add the Trampoline support feature flag.