Skip to content

Prefactor for inbound Trampoline parsing/decryption #3595

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Feb 13, 2025
Merged

Prefactor for inbound Trampoline parsing/decryption #3595

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4d9deff
Switch RequiredWrapper to LengthReadable
arik-so Feb 10, 2025
b616559
Add TrampolineForward variant to PendingHTLCRouting
arik-so Feb 10, 2025
2c685d2
Test trampoline-cfg in CI
arik-so Feb 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ check-cfg = [
"cfg(ldk_bench)",
"cfg(ldk_test_vectors)",
"cfg(taproot)",
"cfg(trampoline)",
"cfg(require_route_graph_test)",
"cfg(splicing)",
"cfg(async_payments)",
Expand Down
2 changes: 2 additions & 0 deletions ci/ci-tests.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,8 @@ RUSTFLAGS="--cfg=taproot" cargo test --verbose --color always -p lightning
[ "$CI_MINIMIZE_DISK_USAGE" != "" ] && cargo clean
RUSTFLAGS="--cfg=splicing" cargo test --verbose --color always -p lightning
[ "$CI_MINIMIZE_DISK_USAGE" != "" ] && cargo clean
RUSTFLAGS="--cfg=trampoline" cargo test --verbose --color always -p lightning
[ "$CI_MINIMIZE_DISK_USAGE" != "" ] && cargo clean
RUSTFLAGS="--cfg=async_payments" cargo test --verbose --color always -p lightning
[ "$CI_MINIMIZE_DISK_USAGE" != "" ] && cargo clean
RUSTFLAGS="--cfg=lsps1_service" cargo test --verbose --color always -p lightning-liquidity
63 changes: 63 additions & 0 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ use crate::ln::channel_state::ChannelDetails;
use crate::types::features::{Bolt12InvoiceFeatures, ChannelFeatures, ChannelTypeFeatures, InitFeatures, NodeFeatures};
#[cfg(any(feature = "_test_utils", test))]
use crate::types::features::Bolt11InvoiceFeatures;
#[cfg(trampoline)]
use crate::routing::gossip::NodeId;
use crate::routing::router::{BlindedTail, FixedRouter, InFlightHtlcs, Path, Payee, PaymentParameters, Route, RouteParameters, Router};
use crate::ln::onion_payment::{check_incoming_htlc_cltv, create_recv_pending_htlc_info, create_fwd_pending_htlc_info, decode_incoming_update_add_htlc_onion, InboundHTLCErr, NextPacketDetails};
use crate::ln::msgs;
Expand Down Expand Up @@ -169,6 +171,24 @@ pub enum PendingHTLCRouting {
/// The absolute CLTV of the inbound HTLC
incoming_cltv_expiry: Option<u32>,
},
/// An HTLC which should be forwarded on to another Trampoline node.
#[cfg(trampoline)]
TrampolineForward {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't this need to be cfg-tagged? After adding this we can panic in process_pending_htlc_forwards with "short_channel_id == 0 should imply any pending_forward entries are of type Receive"

Copy link
Contributor

Choose a reason for hiding this comment

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

Not opposed to cfg-gating, but I don't think we'll hit this panic right now since we fail during process_pending_update_add_htlcs 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

perhaps cfg-gating might prove useful for the ability to split up PRs between decrypting the inbound onion and handling forwarding, which are otherwise fairly tightly coupled

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, my concern is we end up shipping 0.2 without adding the missing cfg-gate or fully handling trampoline forwards and someone downgrades to 0.2, hitting the panic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I'll cfg-gate the forwards

/// The onion shared secret we build with the sender (or the preceding Trampoline node) used
/// to decrypt the onion.
///
/// This is later used to encrypt failure packets in the event that the HTLC is failed.
incoming_shared_secret: [u8; 32],
/// The onion which should be included in the forwarded HTLC, telling the next hop what to
/// do with the HTLC.
onion_packet: msgs::TrampolineOnionPacket,
/// The node ID of the Trampoline node which we need to route this HTLC to.
node_id: NodeId,
/// Set if this HTLC is being forwarded within a blinded path.
blinded: Option<BlindedForward>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm? We can have a trampoline in a blinded path? I didn't think we could.

/// The absolute CLTV of the inbound HTLC
incoming_cltv_expiry: u32,
},
/// The onion indicates that this is a payment for an invoice (supposedly) generated by us.
///
/// Note that at this point, we have not checked that the invoice being paid was actually
Expand Down Expand Up @@ -270,6 +290,8 @@ impl PendingHTLCRouting {
fn blinded_failure(&self) -> Option<BlindedFailure> {
match self {
Self::Forward { blinded: Some(BlindedForward { failure, .. }), .. } => Some(*failure),
#[cfg(trampoline)]
Self::TrampolineForward { blinded: Some(BlindedForward { failure, .. }), .. } => Some(*failure),
Self::Receive { requires_blinded_error: true, .. } => Some(BlindedFailure::FromBlindedNode),
Self::ReceiveKeysend { requires_blinded_error: true, .. } => Some(BlindedFailure::FromBlindedNode),
_ => None,
Expand All @@ -279,6 +301,8 @@ impl PendingHTLCRouting {
fn incoming_cltv_expiry(&self) -> Option<u32> {
match self {
Self::Forward { incoming_cltv_expiry, .. } => *incoming_cltv_expiry,
#[cfg(trampoline)]
Self::TrampolineForward { incoming_cltv_expiry, .. } => Some(*incoming_cltv_expiry),
Self::Receive { incoming_cltv_expiry, .. } => Some(*incoming_cltv_expiry),
Self::ReceiveKeysend { incoming_cltv_expiry, .. } => Some(*incoming_cltv_expiry),
}
Expand Down Expand Up @@ -8909,6 +8933,8 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
for (forward_info, prev_htlc_id) in pending_forwards.drain(..) {
let scid = match forward_info.routing {
PendingHTLCRouting::Forward { short_channel_id, .. } => short_channel_id,
#[cfg(trampoline)]
PendingHTLCRouting::TrampolineForward { .. } => 0,
PendingHTLCRouting::Receive { .. } => 0,
PendingHTLCRouting::ReceiveKeysend { .. } => 0,
};
Expand Down Expand Up @@ -12449,6 +12475,7 @@ impl_writeable_tlv_based!(BlindedForward, {
(3, next_blinding_override, option),
});

#[cfg(not(trampoline))]
impl_writeable_tlv_based_enum!(PendingHTLCRouting,
(0, Forward) => {
(0, onion_packet, required),
Expand Down Expand Up @@ -12477,6 +12504,42 @@ impl_writeable_tlv_based_enum!(PendingHTLCRouting,
(11, invoice_request, option),
},
);
#[cfg(trampoline)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

it would be nice to avoid this duplication but I can't think of a way to do so

Copy link
Collaborator

Choose a reason for hiding this comment

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

Eh, we'll survive for a month.

impl_writeable_tlv_based_enum!(PendingHTLCRouting,
(0, Forward) => {
(0, onion_packet, required),
(1, blinded, option),
(2, short_channel_id, required),
(3, incoming_cltv_expiry, option),
},
(1, Receive) => {
(0, payment_data, required),
(1, phantom_shared_secret, option),
(2, incoming_cltv_expiry, required),
(3, payment_metadata, option),
(5, custom_tlvs, optional_vec),
(7, requires_blinded_error, (default_value, false)),
(9, payment_context, option),
},
(2, ReceiveKeysend) => {
(0, payment_preimage, required),
(1, requires_blinded_error, (default_value, false)),
(2, incoming_cltv_expiry, required),
(3, payment_metadata, option),
(4, payment_data, option), // Added in 0.0.116
(5, custom_tlvs, optional_vec),
(7, has_recipient_created_payment_secret, (default_value, false)),
(9, payment_context, option),
(11, invoice_request, option),
},
(3, TrampolineForward) => {
(0, incoming_shared_secret, required),
(2, onion_packet, required),
(4, blinded, option),
(6, node_id, required),
(8, incoming_cltv_expiry, required),
}
);

impl_writeable_tlv_based!(PendingHTLCInfo, {
(0, routing, required),
Expand Down
6 changes: 3 additions & 3 deletions lightning/src/util/ser.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,10 +403,10 @@ impl<T: Readable> MaybeReadable for T {
///
/// This is not exported to bindings users as manual TLV building is not currently supported in bindings
pub struct RequiredWrapper<T>(pub Option<T>);
impl<T: Readable> Readable for RequiredWrapper<T> {
impl<T: LengthReadable> LengthReadable for RequiredWrapper<T> {
#[inline]
fn read<R: Read>(reader: &mut R) -> Result<Self, DecodeError> {
Ok(Self(Some(Readable::read(reader)?)))
fn read_from_fixed_length_buffer<R: LengthRead>(reader: &mut R) -> Result<Self, DecodeError> {
Ok(Self(Some(LengthReadable::read_from_fixed_length_buffer(reader)?)))
}
}
impl<A, T: ReadableArgs<A>> ReadableArgs<A> for RequiredWrapper<T> {
Expand Down
Loading