Prefactor for inbound Trampoline parsing/decryption #3595
Conversation
In a subsequent commit, we will be storing `TrampolineOnionPacket`s within `PendingHTLCRouting`, requiring that they be serialized for storage. To do so, `RequiredWrapper`'s requirements must be loosened to only require `LengthReadable` instead of `Readable`.
arik-so
force-pushed
the
arik/trampoline/inbound-prefactors
branch
2 times, most recently
from
2458f09 to
c30ea82
Compare
| /// 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>, |
There was a problem hiding this comment.
Hmm? We can have a trampoline in a blinded path? I didn't think we could.
lightning/src/ln/channelmanager.rs
Outdated
| @@ -131,7 +132,6 @@ pub use crate::ln::outbound_payment::{Bolt12PaymentError, ProbeSendFailure, Retr | |||
| #[cfg(test)] | |||
| pub(crate) use crate::ln::outbound_payment::PaymentSendFailure; | |||
| use crate::ln::script::ShutdownScript; | |||
|
|
|||
| @@ -169,6 +169,23 @@ 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. | |||
| TrampolineForward { | |||
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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 🤔
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
ok, I'll cfg-gate the forwards
lightning/src/ln/channelmanager.rs
Outdated
| @@ -4298,7 +4321,13 @@ where | |||
| fn can_forward_htlc( | |||
| &self, msg: &msgs::UpdateAddHTLC, next_packet_details: &NextPacketDetails | |||
| ) -> Result<(), (&'static str, u16)> { | |||
| match self.do_funded_channel_callback(next_packet_details.outgoing_scid, |chan: &mut FundedChannel<SP>| { | |||
| let outgoing_scid = match next_packet_details.outgoing_connector { | |||
There was a problem hiding this comment.
ISTM we should be changing can_forward_htlc's API to take an SCID instead of NextPacketDetails if we're changing NextPacketDetails. We still want to run this after we calculate a route when doing trampoline, I imagine.
arik-so
force-pushed
the
arik/trampoline/inbound-prefactors
branch
3 times, most recently
from
6809c8f to
9c493d8
Compare
| @@ -12477,6 +12504,42 @@ impl_writeable_tlv_based_enum!(PendingHTLCRouting, | |||
| (11, invoice_request, option), | |||
| }, | |||
| ); | |||
| #[cfg(trampoline)] | |||
There was a problem hiding this comment.
it would be nice to avoid this duplication but I can't think of a way to do so
There was a problem hiding this comment.
Eh, we'll survive for a month.
arik-so
force-pushed
the
arik/trampoline/inbound-prefactors
branch
from
9c493d8 to
45ac91d
Compare
|
LGTM, I guess you removed the commit with #3595 (comment) ? Feel free to squash and we can land. |
Forwarding Trampoline packets requires storing their shared secrets on top of the outer onion's shared secrets, as well as referencing the next hop by its node ID as opposed to by an SCID. We modify PendingHTLCRouting to adequately represent this information.
arik-so
force-pushed
the
arik/trampoline/inbound-prefactors
branch
from
45ac91d to
37ab8b9
Compare
|
ok, CI passes post-squash. And yes, as addressed offline, the HopConnector enum needs a slightly different layout so I'm moving that commit out. |
ci/ci-tests.sh
Outdated
| @@ -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=taproot" cargo test --verbose --color always -p lightning | |||
There was a problem hiding this comment.
s/taproot/trampoline
arik-so
force-pushed
the
arik/trampoline/inbound-prefactors
branch
from
37ab8b9 to
2c685d2
Compare
Parsing inbound Trampoline packets (#3585) requires a somewhat awkward trail of refactors and potential code duplication. This PR is meant to prefactor some of the changes that ought to take up less iteration.