peel_payment_onion static fn in channelmanager by Evanfeenstra · Pull Request #2700 · lightningdevkit/rust-lightning

Evanfeenstra · 2023-10-31T23:58:17Z

Adds a public peel_payment_onion function that takes msgs::UpdateAddHTLC and returns PendingHTLCInfo.

I separated the internals of decode_update_add_htlc_onion, construct_recv_pending_htlc_info, and construct_fwd_pending_htlc_info into static functions, and used them to make the PendingHTLCInfo. There are two validation closures passed to peel_payment_onion... some of the outbound channel checking is done in these closures, so its up to the caller to validate against current channel state.

I am unsure about phantom_shared_secret and allow_underpay in create_recv_pending_htlc_info so i left them None/false when called from peel_payment_onion

Not sure if this is the best approach! But its much better than #2677

TheBlueMatt · 2023-11-01T22:04:10Z

+ }
+ };
+
+ let validate_peer_chan = |


Rather than making these lambdas and passing them into decode_incoming_update_add_htlc_onion, can we just check them after running decode_incoming_update_add_htlc_onion?

Ok, its much cleaner now. I repeated the CLTV checks in the peel_payment_onion fn though, but I guess that's fine.
Also added a test for peel_payment_onion

codecov-commenter · 2023-11-01T23:20:25Z

Codecov Report

Attention: 44 lines in your changes are missing coverage. Please review.

Comparison is base (1f399b0) 88.71% compared to head (192fe05) 88.72%.
Report is 63 commits behind head on main.

Files	Patch %	Lines
lightning/src/ln/channelmanager.rs	89.21%	37 Missing and 7 partials ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@ Coverage Diff @@## main #2700 +/- ##
=========================================
Coverage 88.71% 88.72% =========================================
Files 112 113 +1 Lines 88502 89533 +1031 Branches 88502 89533 +1031 =========================================
+ Hits 78517 79434 +917 - Misses 7752 7839 +87 - Partials 2233 2260 +27

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

TheBlueMatt

Nice! This turned out pretty good. I think we'll basically be able to use this for LSP 4 too.

TheBlueMatt · 2023-11-02T22:44:33Z

+ if msg.cltv_expiry > cur_height + CLTV_FAR_FAR_AWAY as u32 { // expiry_too_far
+ return Err("CLTV expiry is too far in the future");
+ }
+ if (outgoing_cltv_value) as u64 <= (cur_height + LATENCY_GRACE_PERIOD_BLOCKS) as u64 {


Why keep the redundant one in decode_update_add_htlc_onion, can we just drop the redundant checks and leave it only doing the next-hop-channels-is-real checks.

Trying to figure out how to do this... the ChannelManager::decode_update_add_htlc_onion CLTV checks may generate a chan_update, while the checks in peel_payment_onion are simpler (since peel_payment_onion is called externally, without the context).
Are you saying we should move the checks inside the inner decode_incoming_update_add_htlc_onion fn?

ok! got it figured out so there is no redundancy

TheBlueMatt · 2023-11-02T22:46:46Z

+}
+
+/// Peel one layer off an incoming onion, returning PendingHTLCInfo (either Forward or Receive).
+/// Validates the outgoing CLTV value against the cur_height.


Docs should describe some amount of "this does all the relevant context-free checks that LDK requires for payment relay or acceptance. If the payment is to be received, and the amount matches the expected amount for a given invoice, this indicates the UpdateAddHTLC, once fully committed in the channel, will generate an Event::PaymentReceived".

TheBlueMatt · 2023-11-07T04:41:32Z

Oops, needs a small rebase after the other PR.

valentinewallace

Still making my way through

valentinewallace

Almost there!

valentinewallace

Needs rebase unfortunately :(

LGTM after this. Also fine with fixing these in follow-up.

valentinewallace · 2023-11-14T15:39:55Z

+	pub routing: PendingHTLCRouting,
+	/// Shared secret from the previous hop.
+	pub incoming_shared_secret: [u8; 32],
 	payment_hash: PaymentHash,


Why leave this as the only unexposed field?

Evanfeenstra · 2023-11-14T17:36:23Z

Needs rebase unfortunately :(
LGTM after this. Also fine with fixing these in follow-up.

OK, happy to do a follow-up on these items as soon as it gets merged

TheBlueMatt · 2023-11-14T18:34:18Z

Rather than using merge commits to rebase, please use rebase directly. See https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#rebasing-changes.

Evanfeenstra · 2023-11-14T19:59:20Z

Rather than using merge commits to rebase, please use rebase directly. See https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#rebasing-changes.

Oh, thanks for the tip. I reverted the merge commit. Also reverted the PeelOnionError stuff for a follow-up PR instead, per @valentinewallace's suggestion

valentinewallace · 2023-11-14T23:43:44Z

Having done more research into CLN and LND's htlc interception APIs, I don't think we need to follow-up adding PeelOnionError.

Both APIs take either an error code or a decoded onion error packet, so our current error should be sufficient for that.

I was also thinking the LSPS4 spec may need better error info, but from talking to @TheBlueMatt it sounds like the LSP would always just error with "unknown next channel."

So excuse the misleading direction, I think this PR is fine as-is.

TheBlueMatt

LGTM, though the docs on newly-public fields and structs really need to communicate a lot more information. Generally, any public field or struct needs docs that explain (a) what it is (often in terms of how we calculated it) and (b) what its used for, often how a user would use it (if there's no reason for them to use it, it shouldn't be public).

TheBlueMatt · 2023-11-15T19:28:50Z

- incoming_cltv_expiry: u32, // Used to track when we should expire pending HTLCs that go unclaimed
+ /// Used to track when we should expire pending HTLCs that go unclaimed.
+ incoming_cltv_expiry: u32,
+ /// Optional shared secret for phantom node.


This tells a user almost nothing about what this is or what its used for.

TheBlueMatt · 2023-11-15T19:29:20Z

 /// generated using `get_fake_scid` from the scid_utils::fake_scid module.
 short_channel_id: u64, // This should be NonZero<u64> eventually when we bump MSRV
 	},
+	/// An HTLC paid to an invoice we generated.


nit: might be worth noting that, at this point, we have not checked that the invoice being paid was actually generated by us, but rather its claiming to pay an invoice of ours.

TheBlueMatt · 2023-11-15T19:31:57Z

 /// See [`RecipientOnionFields::custom_tlvs`] for more info.
 custom_tlvs: Vec<(u64, Vec<u8>)>,
 	},
+	/// Incoming keysend (sender provided the preimage in a TLV).


Only kinda specific to this PR, but the public interface is now kinda confused between keysend and spontaneous payments. We originally tried to use "spontaneous payments" for this publicly, but some keysend references slipped in, and now we're adding more. We should make everything consistent in a followup rename.

TheBlueMatt · 2023-11-15T19:32:30Z

+ /// See [`RecipientOnionFields::payment_metadata`] for more info.
 payment_metadata: Option<Vec<u8>>,
- incoming_cltv_expiry: u32, // Used to track when we should expire pending HTLCs that go unclaimed
+ /// Used to track when we should expire pending HTLCs that go unclaimed.


nit: would be nice to also repeat what it is, not just what its used for.

TheBlueMatt · 2023-11-15T19:43:46Z

+pub struct PendingHTLCInfo {
+	/// Further routing details based on whether the HTLC is being forwarded or received.
+	pub routing: PendingHTLCRouting,
+	/// Shared secret from the previous hop.


This doesn't really say much about what this is used for/why its here.

TheBlueMatt · 2023-11-16T16:52:36Z

 	}
 }

+fn create_fwd_pending_htlc_info(


Now that these are all freestanding functions, they (and the associated return/error structs) should probably move to some other file, if only to make channelmanager.rs slightly less huge.

Improve docs on newly-public structs after #2700

Evanfeenstra · 2023-10-31T23:58:17Z

Adds a public peel_payment_onion function that takes msgs::UpdateAddHTLC and returns PendingHTLCInfo.

I separated the internals of decode_update_add_htlc_onion, construct_recv_pending_htlc_info, and construct_fwd_pending_htlc_info into static functions, and used them to make the PendingHTLCInfo. There are two validation closures passed to peel_payment_onion... some of the outbound channel checking is done in these closures, so its up to the caller to validate against current channel state.

I am unsure about phantom_shared_secret and allow_underpay in create_recv_pending_htlc_info so i left them None/false when called from peel_payment_onion

Not sure if this is the best approach! But its much better than #2677

TheBlueMatt · 2023-11-01T22:04:10Z

+ }
+ };
+
+ let validate_peer_chan = |


Rather than making these lambdas and passing them into decode_incoming_update_add_htlc_onion, can we just check them after running decode_incoming_update_add_htlc_onion?

Ok, its much cleaner now. I repeated the CLTV checks in the peel_payment_onion fn though, but I guess that's fine.
Also added a test for peel_payment_onion

codecov-commenter · 2023-11-01T23:20:25Z

Codecov Report

Attention: 44 lines in your changes are missing coverage. Please review.

Comparison is base (1f399b0) 88.71% compared to head (192fe05) 88.72%.
Report is 63 commits behind head on main.

Files	Patch %	Lines
lightning/src/ln/channelmanager.rs	89.21%	37 Missing and 7 partials ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@ Coverage Diff @@## main #2700 +/- ##
=========================================
Coverage 88.71% 88.72% =========================================
Files 112 113 +1 Lines 88502 89533 +1031 Branches 88502 89533 +1031 =========================================
+ Hits 78517 79434 +917 - Misses 7752 7839 +87 - Partials 2233 2260 +27

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

TheBlueMatt

Nice! This turned out pretty good. I think we'll basically be able to use this for LSP 4 too.

TheBlueMatt · 2023-11-02T22:44:33Z

+ if msg.cltv_expiry > cur_height + CLTV_FAR_FAR_AWAY as u32 { // expiry_too_far
+ return Err("CLTV expiry is too far in the future");
+ }
+ if (outgoing_cltv_value) as u64 <= (cur_height + LATENCY_GRACE_PERIOD_BLOCKS) as u64 {


Why keep the redundant one in decode_update_add_htlc_onion, can we just drop the redundant checks and leave it only doing the next-hop-channels-is-real checks.

Trying to figure out how to do this... the ChannelManager::decode_update_add_htlc_onion CLTV checks may generate a chan_update, while the checks in peel_payment_onion are simpler (since peel_payment_onion is called externally, without the context).
Are you saying we should move the checks inside the inner decode_incoming_update_add_htlc_onion fn?

ok! got it figured out so there is no redundancy

TheBlueMatt · 2023-11-02T22:46:46Z

+}
+
+/// Peel one layer off an incoming onion, returning PendingHTLCInfo (either Forward or Receive).
+/// Validates the outgoing CLTV value against the cur_height.


Docs should describe some amount of "this does all the relevant context-free checks that LDK requires for payment relay or acceptance. If the payment is to be received, and the amount matches the expected amount for a given invoice, this indicates the UpdateAddHTLC, once fully committed in the channel, will generate an Event::PaymentReceived".

TheBlueMatt · 2023-11-07T04:41:32Z

Oops, needs a small rebase after the other PR.

valentinewallace

Still making my way through

valentinewallace

Almost there!

valentinewallace

Needs rebase unfortunately :(

LGTM after this. Also fine with fixing these in follow-up.

valentinewallace · 2023-11-14T15:39:55Z

+	pub routing: PendingHTLCRouting,
+	/// Shared secret from the previous hop.
+	pub incoming_shared_secret: [u8; 32],
 	payment_hash: PaymentHash,


Why leave this as the only unexposed field?

Evanfeenstra · 2023-11-14T17:36:23Z

Needs rebase unfortunately :(
LGTM after this. Also fine with fixing these in follow-up.

OK, happy to do a follow-up on these items as soon as it gets merged

TheBlueMatt · 2023-11-14T18:34:18Z

Rather than using merge commits to rebase, please use rebase directly. See https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#rebasing-changes.

Evanfeenstra · 2023-11-14T19:59:20Z

Rather than using merge commits to rebase, please use rebase directly. See https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#rebasing-changes.

Oh, thanks for the tip. I reverted the merge commit. Also reverted the PeelOnionError stuff for a follow-up PR instead, per @valentinewallace's suggestion

valentinewallace · 2023-11-14T23:43:44Z

Having done more research into CLN and LND's htlc interception APIs, I don't think we need to follow-up adding PeelOnionError.

Both APIs take either an error code or a decoded onion error packet, so our current error should be sufficient for that.

I was also thinking the LSPS4 spec may need better error info, but from talking to @TheBlueMatt it sounds like the LSP would always just error with "unknown next channel."

So excuse the misleading direction, I think this PR is fine as-is.

TheBlueMatt

LGTM, though the docs on newly-public fields and structs really need to communicate a lot more information. Generally, any public field or struct needs docs that explain (a) what it is (often in terms of how we calculated it) and (b) what its used for, often how a user would use it (if there's no reason for them to use it, it shouldn't be public).

TheBlueMatt · 2023-11-15T19:28:50Z

- incoming_cltv_expiry: u32, // Used to track when we should expire pending HTLCs that go unclaimed
+ /// Used to track when we should expire pending HTLCs that go unclaimed.
+ incoming_cltv_expiry: u32,
+ /// Optional shared secret for phantom node.


This tells a user almost nothing about what this is or what its used for.

TheBlueMatt · 2023-11-15T19:29:20Z

 /// generated using `get_fake_scid` from the scid_utils::fake_scid module.
 short_channel_id: u64, // This should be NonZero<u64> eventually when we bump MSRV
 	},
+	/// An HTLC paid to an invoice we generated.


nit: might be worth noting that, at this point, we have not checked that the invoice being paid was actually generated by us, but rather its claiming to pay an invoice of ours.

TheBlueMatt · 2023-11-15T19:31:57Z

 /// See [`RecipientOnionFields::custom_tlvs`] for more info.
 custom_tlvs: Vec<(u64, Vec<u8>)>,
 	},
+	/// Incoming keysend (sender provided the preimage in a TLV).


Only kinda specific to this PR, but the public interface is now kinda confused between keysend and spontaneous payments. We originally tried to use "spontaneous payments" for this publicly, but some keysend references slipped in, and now we're adding more. We should make everything consistent in a followup rename.

TheBlueMatt · 2023-11-15T19:32:30Z

+ /// See [`RecipientOnionFields::payment_metadata`] for more info.
 payment_metadata: Option<Vec<u8>>,
- incoming_cltv_expiry: u32, // Used to track when we should expire pending HTLCs that go unclaimed
+ /// Used to track when we should expire pending HTLCs that go unclaimed.


nit: would be nice to also repeat what it is, not just what its used for.

TheBlueMatt · 2023-11-15T19:43:46Z

+pub struct PendingHTLCInfo {
+	/// Further routing details based on whether the HTLC is being forwarded or received.
+	pub routing: PendingHTLCRouting,
+	/// Shared secret from the previous hop.


This doesn't really say much about what this is used for/why its here.

TheBlueMatt · 2023-11-16T16:52:36Z

 	}
 }

+fn create_fwd_pending_htlc_info(


Now that these are all freestanding functions, they (and the associated return/error structs) should probably move to some other file, if only to make channelmanager.rs slightly less huge.

Improve docs on newly-public structs after #2700

Evanfeenstra · 2023-10-31T23:58:17Z

Adds a public peel_payment_onion function that takes msgs::UpdateAddHTLC and returns PendingHTLCInfo.

I separated the internals of decode_update_add_htlc_onion, construct_recv_pending_htlc_info, and construct_fwd_pending_htlc_info into static functions, and used them to make the PendingHTLCInfo. There are two validation closures passed to peel_payment_onion... some of the outbound channel checking is done in these closures, so its up to the caller to validate against current channel state.

I am unsure about phantom_shared_secret and allow_underpay in create_recv_pending_htlc_info so i left them None/false when called from peel_payment_onion

Not sure if this is the best approach! But its much better than #2677

TheBlueMatt · 2023-11-01T22:04:10Z

+ }
+ };
+
+ let validate_peer_chan = |


Rather than making these lambdas and passing them into decode_incoming_update_add_htlc_onion, can we just check them after running decode_incoming_update_add_htlc_onion?

Ok, its much cleaner now. I repeated the CLTV checks in the peel_payment_onion fn though, but I guess that's fine.
Also added a test for peel_payment_onion

codecov-commenter · 2023-11-01T23:20:25Z

Codecov Report

Attention: 44 lines in your changes are missing coverage. Please review.

Comparison is base (1f399b0) 88.71% compared to head (192fe05) 88.72%.
Report is 63 commits behind head on main.

Files	Patch %	Lines
lightning/src/ln/channelmanager.rs	89.21%	37 Missing and 7 partials ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@ Coverage Diff @@## main #2700 +/- ##
=========================================
Coverage 88.71% 88.72% =========================================
Files 112 113 +1 Lines 88502 89533 +1031 Branches 88502 89533 +1031 =========================================
+ Hits 78517 79434 +917 - Misses 7752 7839 +87 - Partials 2233 2260 +27

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

TheBlueMatt

Nice! This turned out pretty good. I think we'll basically be able to use this for LSP 4 too.

TheBlueMatt · 2023-11-02T22:44:33Z

+ if msg.cltv_expiry > cur_height + CLTV_FAR_FAR_AWAY as u32 { // expiry_too_far
+ return Err("CLTV expiry is too far in the future");
+ }
+ if (outgoing_cltv_value) as u64 <= (cur_height + LATENCY_GRACE_PERIOD_BLOCKS) as u64 {


Why keep the redundant one in decode_update_add_htlc_onion, can we just drop the redundant checks and leave it only doing the next-hop-channels-is-real checks.

Trying to figure out how to do this... the ChannelManager::decode_update_add_htlc_onion CLTV checks may generate a chan_update, while the checks in peel_payment_onion are simpler (since peel_payment_onion is called externally, without the context).
Are you saying we should move the checks inside the inner decode_incoming_update_add_htlc_onion fn?

ok! got it figured out so there is no redundancy

TheBlueMatt · 2023-11-02T22:46:46Z

+}
+
+/// Peel one layer off an incoming onion, returning PendingHTLCInfo (either Forward or Receive).
+/// Validates the outgoing CLTV value against the cur_height.


Docs should describe some amount of "this does all the relevant context-free checks that LDK requires for payment relay or acceptance. If the payment is to be received, and the amount matches the expected amount for a given invoice, this indicates the UpdateAddHTLC, once fully committed in the channel, will generate an Event::PaymentReceived".

TheBlueMatt · 2023-11-07T04:41:32Z

Oops, needs a small rebase after the other PR.

valentinewallace

Still making my way through

valentinewallace

Almost there!

valentinewallace

Needs rebase unfortunately :(

LGTM after this. Also fine with fixing these in follow-up.

valentinewallace · 2023-11-14T15:39:55Z

+	pub routing: PendingHTLCRouting,
+	/// Shared secret from the previous hop.
+	pub incoming_shared_secret: [u8; 32],
 	payment_hash: PaymentHash,


Why leave this as the only unexposed field?

Evanfeenstra · 2023-11-14T17:36:23Z

Needs rebase unfortunately :(
LGTM after this. Also fine with fixing these in follow-up.

OK, happy to do a follow-up on these items as soon as it gets merged

TheBlueMatt · 2023-11-14T18:34:18Z

Rather than using merge commits to rebase, please use rebase directly. See https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#rebasing-changes.

Evanfeenstra · 2023-11-14T19:59:20Z

Rather than using merge commits to rebase, please use rebase directly. See https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#rebasing-changes.

Oh, thanks for the tip. I reverted the merge commit. Also reverted the PeelOnionError stuff for a follow-up PR instead, per @valentinewallace's suggestion

valentinewallace · 2023-11-14T23:43:44Z

Having done more research into CLN and LND's htlc interception APIs, I don't think we need to follow-up adding PeelOnionError.

Both APIs take either an error code or a decoded onion error packet, so our current error should be sufficient for that.

I was also thinking the LSPS4 spec may need better error info, but from talking to @TheBlueMatt it sounds like the LSP would always just error with "unknown next channel."

So excuse the misleading direction, I think this PR is fine as-is.

TheBlueMatt

LGTM, though the docs on newly-public fields and structs really need to communicate a lot more information. Generally, any public field or struct needs docs that explain (a) what it is (often in terms of how we calculated it) and (b) what its used for, often how a user would use it (if there's no reason for them to use it, it shouldn't be public).

TheBlueMatt · 2023-11-15T19:28:50Z

- incoming_cltv_expiry: u32, // Used to track when we should expire pending HTLCs that go unclaimed
+ /// Used to track when we should expire pending HTLCs that go unclaimed.
+ incoming_cltv_expiry: u32,
+ /// Optional shared secret for phantom node.


This tells a user almost nothing about what this is or what its used for.

TheBlueMatt · 2023-11-15T19:29:20Z

 /// generated using `get_fake_scid` from the scid_utils::fake_scid module.
 short_channel_id: u64, // This should be NonZero<u64> eventually when we bump MSRV
 	},
+	/// An HTLC paid to an invoice we generated.


nit: might be worth noting that, at this point, we have not checked that the invoice being paid was actually generated by us, but rather its claiming to pay an invoice of ours.

TheBlueMatt · 2023-11-15T19:31:57Z

 /// See [`RecipientOnionFields::custom_tlvs`] for more info.
 custom_tlvs: Vec<(u64, Vec<u8>)>,
 	},
+	/// Incoming keysend (sender provided the preimage in a TLV).


Only kinda specific to this PR, but the public interface is now kinda confused between keysend and spontaneous payments. We originally tried to use "spontaneous payments" for this publicly, but some keysend references slipped in, and now we're adding more. We should make everything consistent in a followup rename.

TheBlueMatt · 2023-11-15T19:32:30Z

+ /// See [`RecipientOnionFields::payment_metadata`] for more info.
 payment_metadata: Option<Vec<u8>>,
- incoming_cltv_expiry: u32, // Used to track when we should expire pending HTLCs that go unclaimed
+ /// Used to track when we should expire pending HTLCs that go unclaimed.


nit: would be nice to also repeat what it is, not just what its used for.

TheBlueMatt · 2023-11-15T19:43:46Z

+pub struct PendingHTLCInfo {
+	/// Further routing details based on whether the HTLC is being forwarded or received.
+	pub routing: PendingHTLCRouting,
+	/// Shared secret from the previous hop.


This doesn't really say much about what this is used for/why its here.

TheBlueMatt · 2023-11-16T16:52:36Z

 	}
 }

+fn create_fwd_pending_htlc_info(


Now that these are all freestanding functions, they (and the associated return/error structs) should probably move to some other file, if only to make channelmanager.rs slightly less huge.

Improve docs on newly-public structs after #2700

Evanfeenstra · 2023-10-31T23:58:17Z

Adds a public peel_payment_onion function that takes msgs::UpdateAddHTLC and returns PendingHTLCInfo.

I separated the internals of decode_update_add_htlc_onion, construct_recv_pending_htlc_info, and construct_fwd_pending_htlc_info into static functions, and used them to make the PendingHTLCInfo. There are two validation closures passed to peel_payment_onion... some of the outbound channel checking is done in these closures, so its up to the caller to validate against current channel state.

I am unsure about phantom_shared_secret and allow_underpay in create_recv_pending_htlc_info so i left them None/false when called from peel_payment_onion

Not sure if this is the best approach! But its much better than #2677

TheBlueMatt · 2023-11-01T22:04:10Z

+ }
+ };
+
+ let validate_peer_chan = |


Rather than making these lambdas and passing them into decode_incoming_update_add_htlc_onion, can we just check them after running decode_incoming_update_add_htlc_onion?

Ok, its much cleaner now. I repeated the CLTV checks in the peel_payment_onion fn though, but I guess that's fine.
Also added a test for peel_payment_onion

codecov-commenter · 2023-11-01T23:20:25Z

Codecov Report

Attention: 44 lines in your changes are missing coverage. Please review.

Comparison is base (1f399b0) 88.71% compared to head (192fe05) 88.72%.
Report is 63 commits behind head on main.

Files	Patch %	Lines
lightning/src/ln/channelmanager.rs	89.21%	37 Missing and 7 partials ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@ Coverage Diff @@## main #2700 +/- ##
=========================================
Coverage 88.71% 88.72% =========================================
Files 112 113 +1 Lines 88502 89533 +1031 Branches 88502 89533 +1031 =========================================
+ Hits 78517 79434 +917 - Misses 7752 7839 +87 - Partials 2233 2260 +27

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

TheBlueMatt

Nice! This turned out pretty good. I think we'll basically be able to use this for LSP 4 too.

TheBlueMatt · 2023-11-02T22:44:33Z

+ if msg.cltv_expiry > cur_height + CLTV_FAR_FAR_AWAY as u32 { // expiry_too_far
+ return Err("CLTV expiry is too far in the future");
+ }
+ if (outgoing_cltv_value) as u64 <= (cur_height + LATENCY_GRACE_PERIOD_BLOCKS) as u64 {


Why keep the redundant one in decode_update_add_htlc_onion, can we just drop the redundant checks and leave it only doing the next-hop-channels-is-real checks.

Trying to figure out how to do this... the ChannelManager::decode_update_add_htlc_onion CLTV checks may generate a chan_update, while the checks in peel_payment_onion are simpler (since peel_payment_onion is called externally, without the context).
Are you saying we should move the checks inside the inner decode_incoming_update_add_htlc_onion fn?

ok! got it figured out so there is no redundancy

TheBlueMatt · 2023-11-02T22:46:46Z

+}
+
+/// Peel one layer off an incoming onion, returning PendingHTLCInfo (either Forward or Receive).
+/// Validates the outgoing CLTV value against the cur_height.


Docs should describe some amount of "this does all the relevant context-free checks that LDK requires for payment relay or acceptance. If the payment is to be received, and the amount matches the expected amount for a given invoice, this indicates the UpdateAddHTLC, once fully committed in the channel, will generate an Event::PaymentReceived".

TheBlueMatt · 2023-11-07T04:41:32Z

Oops, needs a small rebase after the other PR.

valentinewallace

Still making my way through

valentinewallace

Almost there!

valentinewallace

Needs rebase unfortunately :(

LGTM after this. Also fine with fixing these in follow-up.

valentinewallace · 2023-11-14T15:39:55Z

+	pub routing: PendingHTLCRouting,
+	/// Shared secret from the previous hop.
+	pub incoming_shared_secret: [u8; 32],
 	payment_hash: PaymentHash,


Why leave this as the only unexposed field?

Evanfeenstra · 2023-11-14T17:36:23Z

Needs rebase unfortunately :(
LGTM after this. Also fine with fixing these in follow-up.

OK, happy to do a follow-up on these items as soon as it gets merged

TheBlueMatt · 2023-11-14T18:34:18Z

Rather than using merge commits to rebase, please use rebase directly. See https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#rebasing-changes.

Evanfeenstra · 2023-11-14T19:59:20Z

Rather than using merge commits to rebase, please use rebase directly. See https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#rebasing-changes.

Oh, thanks for the tip. I reverted the merge commit. Also reverted the PeelOnionError stuff for a follow-up PR instead, per @valentinewallace's suggestion

valentinewallace · 2023-11-14T23:43:44Z

Having done more research into CLN and LND's htlc interception APIs, I don't think we need to follow-up adding PeelOnionError.

Both APIs take either an error code or a decoded onion error packet, so our current error should be sufficient for that.

I was also thinking the LSPS4 spec may need better error info, but from talking to @TheBlueMatt it sounds like the LSP would always just error with "unknown next channel."

So excuse the misleading direction, I think this PR is fine as-is.

TheBlueMatt

LGTM, though the docs on newly-public fields and structs really need to communicate a lot more information. Generally, any public field or struct needs docs that explain (a) what it is (often in terms of how we calculated it) and (b) what its used for, often how a user would use it (if there's no reason for them to use it, it shouldn't be public).

TheBlueMatt · 2023-11-15T19:28:50Z

- incoming_cltv_expiry: u32, // Used to track when we should expire pending HTLCs that go unclaimed
+ /// Used to track when we should expire pending HTLCs that go unclaimed.
+ incoming_cltv_expiry: u32,
+ /// Optional shared secret for phantom node.


This tells a user almost nothing about what this is or what its used for.

TheBlueMatt · 2023-11-15T19:29:20Z

 /// generated using `get_fake_scid` from the scid_utils::fake_scid module.
 short_channel_id: u64, // This should be NonZero<u64> eventually when we bump MSRV
 	},
+	/// An HTLC paid to an invoice we generated.


nit: might be worth noting that, at this point, we have not checked that the invoice being paid was actually generated by us, but rather its claiming to pay an invoice of ours.

TheBlueMatt · 2023-11-15T19:31:57Z

 /// See [`RecipientOnionFields::custom_tlvs`] for more info.
 custom_tlvs: Vec<(u64, Vec<u8>)>,
 	},
+	/// Incoming keysend (sender provided the preimage in a TLV).


Only kinda specific to this PR, but the public interface is now kinda confused between keysend and spontaneous payments. We originally tried to use "spontaneous payments" for this publicly, but some keysend references slipped in, and now we're adding more. We should make everything consistent in a followup rename.

TheBlueMatt · 2023-11-15T19:32:30Z

+ /// See [`RecipientOnionFields::payment_metadata`] for more info.
 payment_metadata: Option<Vec<u8>>,
- incoming_cltv_expiry: u32, // Used to track when we should expire pending HTLCs that go unclaimed
+ /// Used to track when we should expire pending HTLCs that go unclaimed.


nit: would be nice to also repeat what it is, not just what its used for.

TheBlueMatt · 2023-11-15T19:43:46Z

+pub struct PendingHTLCInfo {
+	/// Further routing details based on whether the HTLC is being forwarded or received.
+	pub routing: PendingHTLCRouting,
+	/// Shared secret from the previous hop.


This doesn't really say much about what this is used for/why its here.

TheBlueMatt · 2023-11-16T16:52:36Z

 	}
 }

+fn create_fwd_pending_htlc_info(


Now that these are all freestanding functions, they (and the associated return/error structs) should probably move to some other file, if only to make channelmanager.rs slightly less huge.

Improve docs on newly-public structs after #2700

Evanfeenstra · 2023-10-31T23:58:17Z

Adds a public peel_payment_onion function that takes msgs::UpdateAddHTLC and returns PendingHTLCInfo.

I separated the internals of decode_update_add_htlc_onion, construct_recv_pending_htlc_info, and construct_fwd_pending_htlc_info into static functions, and used them to make the PendingHTLCInfo. There are two validation closures passed to peel_payment_onion... some of the outbound channel checking is done in these closures, so its up to the caller to validate against current channel state.

I am unsure about phantom_shared_secret and allow_underpay in create_recv_pending_htlc_info so i left them None/false when called from peel_payment_onion

Not sure if this is the best approach! But its much better than #2677

TheBlueMatt · 2023-11-01T22:04:10Z

+ }
+ };
+
+ let validate_peer_chan = |


Rather than making these lambdas and passing them into decode_incoming_update_add_htlc_onion, can we just check them after running decode_incoming_update_add_htlc_onion?

Ok, its much cleaner now. I repeated the CLTV checks in the peel_payment_onion fn though, but I guess that's fine.
Also added a test for peel_payment_onion

codecov-commenter · 2023-11-01T23:20:25Z

Codecov Report

Attention: 44 lines in your changes are missing coverage. Please review.

Comparison is base (1f399b0) 88.71% compared to head (192fe05) 88.72%.
Report is 63 commits behind head on main.

Files	Patch %	Lines
lightning/src/ln/channelmanager.rs	89.21%	37 Missing and 7 partials ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@ Coverage Diff @@## main #2700 +/- ##
=========================================
Coverage 88.71% 88.72% =========================================
Files 112 113 +1 Lines 88502 89533 +1031 Branches 88502 89533 +1031 =========================================
+ Hits 78517 79434 +917 - Misses 7752 7839 +87 - Partials 2233 2260 +27

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

TheBlueMatt

Nice! This turned out pretty good. I think we'll basically be able to use this for LSP 4 too.

TheBlueMatt · 2023-11-02T22:44:33Z

+ if msg.cltv_expiry > cur_height + CLTV_FAR_FAR_AWAY as u32 { // expiry_too_far
+ return Err("CLTV expiry is too far in the future");
+ }
+ if (outgoing_cltv_value) as u64 <= (cur_height + LATENCY_GRACE_PERIOD_BLOCKS) as u64 {


Why keep the redundant one in decode_update_add_htlc_onion, can we just drop the redundant checks and leave it only doing the next-hop-channels-is-real checks.

Trying to figure out how to do this... the ChannelManager::decode_update_add_htlc_onion CLTV checks may generate a chan_update, while the checks in peel_payment_onion are simpler (since peel_payment_onion is called externally, without the context).
Are you saying we should move the checks inside the inner decode_incoming_update_add_htlc_onion fn?

ok! got it figured out so there is no redundancy

TheBlueMatt · 2023-11-02T22:46:46Z

+}
+
+/// Peel one layer off an incoming onion, returning PendingHTLCInfo (either Forward or Receive).
+/// Validates the outgoing CLTV value against the cur_height.


Docs should describe some amount of "this does all the relevant context-free checks that LDK requires for payment relay or acceptance. If the payment is to be received, and the amount matches the expected amount for a given invoice, this indicates the UpdateAddHTLC, once fully committed in the channel, will generate an Event::PaymentReceived".

TheBlueMatt · 2023-11-07T04:41:32Z

Oops, needs a small rebase after the other PR.

valentinewallace

Still making my way through

valentinewallace

Almost there!

valentinewallace

Needs rebase unfortunately :(

LGTM after this. Also fine with fixing these in follow-up.

valentinewallace · 2023-11-14T15:39:55Z

+	pub routing: PendingHTLCRouting,
+	/// Shared secret from the previous hop.
+	pub incoming_shared_secret: [u8; 32],
 	payment_hash: PaymentHash,


Why leave this as the only unexposed field?

Evanfeenstra · 2023-11-14T17:36:23Z

Needs rebase unfortunately :(
LGTM after this. Also fine with fixing these in follow-up.

OK, happy to do a follow-up on these items as soon as it gets merged

TheBlueMatt · 2023-11-14T18:34:18Z

Rather than using merge commits to rebase, please use rebase directly. See https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#rebasing-changes.

Evanfeenstra · 2023-11-14T19:59:20Z

Rather than using merge commits to rebase, please use rebase directly. See https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#rebasing-changes.

Oh, thanks for the tip. I reverted the merge commit. Also reverted the PeelOnionError stuff for a follow-up PR instead, per @valentinewallace's suggestion

valentinewallace · 2023-11-14T23:43:44Z

Having done more research into CLN and LND's htlc interception APIs, I don't think we need to follow-up adding PeelOnionError.

Both APIs take either an error code or a decoded onion error packet, so our current error should be sufficient for that.

I was also thinking the LSPS4 spec may need better error info, but from talking to @TheBlueMatt it sounds like the LSP would always just error with "unknown next channel."

So excuse the misleading direction, I think this PR is fine as-is.

TheBlueMatt

LGTM, though the docs on newly-public fields and structs really need to communicate a lot more information. Generally, any public field or struct needs docs that explain (a) what it is (often in terms of how we calculated it) and (b) what its used for, often how a user would use it (if there's no reason for them to use it, it shouldn't be public).

TheBlueMatt · 2023-11-15T19:28:50Z

- incoming_cltv_expiry: u32, // Used to track when we should expire pending HTLCs that go unclaimed
+ /// Used to track when we should expire pending HTLCs that go unclaimed.
+ incoming_cltv_expiry: u32,
+ /// Optional shared secret for phantom node.


This tells a user almost nothing about what this is or what its used for.

TheBlueMatt · 2023-11-15T19:29:20Z

 /// generated using `get_fake_scid` from the scid_utils::fake_scid module.
 short_channel_id: u64, // This should be NonZero<u64> eventually when we bump MSRV
 	},
+	/// An HTLC paid to an invoice we generated.


nit: might be worth noting that, at this point, we have not checked that the invoice being paid was actually generated by us, but rather its claiming to pay an invoice of ours.

TheBlueMatt · 2023-11-15T19:31:57Z

 /// See [`RecipientOnionFields::custom_tlvs`] for more info.
 custom_tlvs: Vec<(u64, Vec<u8>)>,
 	},
+	/// Incoming keysend (sender provided the preimage in a TLV).


Only kinda specific to this PR, but the public interface is now kinda confused between keysend and spontaneous payments. We originally tried to use "spontaneous payments" for this publicly, but some keysend references slipped in, and now we're adding more. We should make everything consistent in a followup rename.

TheBlueMatt · 2023-11-15T19:32:30Z

+ /// See [`RecipientOnionFields::payment_metadata`] for more info.
 payment_metadata: Option<Vec<u8>>,
- incoming_cltv_expiry: u32, // Used to track when we should expire pending HTLCs that go unclaimed
+ /// Used to track when we should expire pending HTLCs that go unclaimed.


nit: would be nice to also repeat what it is, not just what its used for.

TheBlueMatt · 2023-11-15T19:43:46Z

+pub struct PendingHTLCInfo {
+	/// Further routing details based on whether the HTLC is being forwarded or received.
+	pub routing: PendingHTLCRouting,
+	/// Shared secret from the previous hop.


This doesn't really say much about what this is used for/why its here.

TheBlueMatt · 2023-11-16T16:52:36Z

 	}
 }

+fn create_fwd_pending_htlc_info(


Now that these are all freestanding functions, they (and the associated return/error structs) should probably move to some other file, if only to make channelmanager.rs slightly less huge.

Improve docs on newly-public structs after #2700

Evanfeenstra · 2023-10-31T23:58:17Z

Adds a public peel_payment_onion function that takes msgs::UpdateAddHTLC and returns PendingHTLCInfo.

I separated the internals of decode_update_add_htlc_onion, construct_recv_pending_htlc_info, and construct_fwd_pending_htlc_info into static functions, and used them to make the PendingHTLCInfo. There are two validation closures passed to peel_payment_onion... some of the outbound channel checking is done in these closures, so its up to the caller to validate against current channel state.

I am unsure about phantom_shared_secret and allow_underpay in create_recv_pending_htlc_info so i left them None/false when called from peel_payment_onion

Not sure if this is the best approach! But its much better than #2677

TheBlueMatt · 2023-11-01T22:04:10Z

+ }
+ };
+
+ let validate_peer_chan = |


Rather than making these lambdas and passing them into decode_incoming_update_add_htlc_onion, can we just check them after running decode_incoming_update_add_htlc_onion?

Ok, its much cleaner now. I repeated the CLTV checks in the peel_payment_onion fn though, but I guess that's fine.
Also added a test for peel_payment_onion

codecov-commenter · 2023-11-01T23:20:25Z

Codecov Report

Attention: 44 lines in your changes are missing coverage. Please review.

Comparison is base (1f399b0) 88.71% compared to head (192fe05) 88.72%.
Report is 63 commits behind head on main.

Files	Patch %	Lines
lightning/src/ln/channelmanager.rs	89.21%	37 Missing and 7 partials ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@ Coverage Diff @@## main #2700 +/- ##
=========================================
Coverage 88.71% 88.72% =========================================
Files 112 113 +1 Lines 88502 89533 +1031 Branches 88502 89533 +1031 =========================================
+ Hits 78517 79434 +917 - Misses 7752 7839 +87 - Partials 2233 2260 +27

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

TheBlueMatt

Nice! This turned out pretty good. I think we'll basically be able to use this for LSP 4 too.

TheBlueMatt · 2023-11-02T22:44:33Z

+ if msg.cltv_expiry > cur_height + CLTV_FAR_FAR_AWAY as u32 { // expiry_too_far
+ return Err("CLTV expiry is too far in the future");
+ }
+ if (outgoing_cltv_value) as u64 <= (cur_height + LATENCY_GRACE_PERIOD_BLOCKS) as u64 {


Why keep the redundant one in decode_update_add_htlc_onion, can we just drop the redundant checks and leave it only doing the next-hop-channels-is-real checks.

Trying to figure out how to do this... the ChannelManager::decode_update_add_htlc_onion CLTV checks may generate a chan_update, while the checks in peel_payment_onion are simpler (since peel_payment_onion is called externally, without the context).
Are you saying we should move the checks inside the inner decode_incoming_update_add_htlc_onion fn?

ok! got it figured out so there is no redundancy

TheBlueMatt · 2023-11-02T22:46:46Z

+}
+
+/// Peel one layer off an incoming onion, returning PendingHTLCInfo (either Forward or Receive).
+/// Validates the outgoing CLTV value against the cur_height.


Docs should describe some amount of "this does all the relevant context-free checks that LDK requires for payment relay or acceptance. If the payment is to be received, and the amount matches the expected amount for a given invoice, this indicates the UpdateAddHTLC, once fully committed in the channel, will generate an Event::PaymentReceived".

TheBlueMatt · 2023-11-07T04:41:32Z

Oops, needs a small rebase after the other PR.

valentinewallace

Still making my way through

valentinewallace

Almost there!

valentinewallace

Needs rebase unfortunately :(

LGTM after this. Also fine with fixing these in follow-up.

valentinewallace · 2023-11-14T15:39:55Z

+	pub routing: PendingHTLCRouting,
+	/// Shared secret from the previous hop.
+	pub incoming_shared_secret: [u8; 32],
 	payment_hash: PaymentHash,


Why leave this as the only unexposed field?

Evanfeenstra · 2023-11-14T17:36:23Z

Needs rebase unfortunately :(
LGTM after this. Also fine with fixing these in follow-up.

OK, happy to do a follow-up on these items as soon as it gets merged

TheBlueMatt · 2023-11-14T18:34:18Z

Rather than using merge commits to rebase, please use rebase directly. See https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#rebasing-changes.

Evanfeenstra · 2023-11-14T19:59:20Z

Rather than using merge commits to rebase, please use rebase directly. See https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#rebasing-changes.

Oh, thanks for the tip. I reverted the merge commit. Also reverted the PeelOnionError stuff for a follow-up PR instead, per @valentinewallace's suggestion

valentinewallace · 2023-11-14T23:43:44Z

Having done more research into CLN and LND's htlc interception APIs, I don't think we need to follow-up adding PeelOnionError.

Both APIs take either an error code or a decoded onion error packet, so our current error should be sufficient for that.

I was also thinking the LSPS4 spec may need better error info, but from talking to @TheBlueMatt it sounds like the LSP would always just error with "unknown next channel."

So excuse the misleading direction, I think this PR is fine as-is.

TheBlueMatt

LGTM, though the docs on newly-public fields and structs really need to communicate a lot more information. Generally, any public field or struct needs docs that explain (a) what it is (often in terms of how we calculated it) and (b) what its used for, often how a user would use it (if there's no reason for them to use it, it shouldn't be public).

TheBlueMatt · 2023-11-15T19:28:50Z

- incoming_cltv_expiry: u32, // Used to track when we should expire pending HTLCs that go unclaimed
+ /// Used to track when we should expire pending HTLCs that go unclaimed.
+ incoming_cltv_expiry: u32,
+ /// Optional shared secret for phantom node.


This tells a user almost nothing about what this is or what its used for.

TheBlueMatt · 2023-11-15T19:29:20Z

 /// generated using `get_fake_scid` from the scid_utils::fake_scid module.
 short_channel_id: u64, // This should be NonZero<u64> eventually when we bump MSRV
 	},
+	/// An HTLC paid to an invoice we generated.


nit: might be worth noting that, at this point, we have not checked that the invoice being paid was actually generated by us, but rather its claiming to pay an invoice of ours.

TheBlueMatt · 2023-11-15T19:31:57Z

 /// See [`RecipientOnionFields::custom_tlvs`] for more info.
 custom_tlvs: Vec<(u64, Vec<u8>)>,
 	},
+	/// Incoming keysend (sender provided the preimage in a TLV).


Only kinda specific to this PR, but the public interface is now kinda confused between keysend and spontaneous payments. We originally tried to use "spontaneous payments" for this publicly, but some keysend references slipped in, and now we're adding more. We should make everything consistent in a followup rename.

TheBlueMatt · 2023-11-15T19:32:30Z

+ /// See [`RecipientOnionFields::payment_metadata`] for more info.
 payment_metadata: Option<Vec<u8>>,
- incoming_cltv_expiry: u32, // Used to track when we should expire pending HTLCs that go unclaimed
+ /// Used to track when we should expire pending HTLCs that go unclaimed.


nit: would be nice to also repeat what it is, not just what its used for.

TheBlueMatt · 2023-11-15T19:43:46Z

+pub struct PendingHTLCInfo {
+	/// Further routing details based on whether the HTLC is being forwarded or received.
+	pub routing: PendingHTLCRouting,
+	/// Shared secret from the previous hop.


This doesn't really say much about what this is used for/why its here.

TheBlueMatt · 2023-11-16T16:52:36Z

 	}
 }

+fn create_fwd_pending_htlc_info(


Now that these are all freestanding functions, they (and the associated return/error structs) should probably move to some other file, if only to make channelmanager.rs slightly less huge.

Improve docs on newly-public structs after #2700

Evanfeenstra · 2023-10-31T23:58:17Z

Adds a public peel_payment_onion function that takes msgs::UpdateAddHTLC and returns PendingHTLCInfo.

I separated the internals of decode_update_add_htlc_onion, construct_recv_pending_htlc_info, and construct_fwd_pending_htlc_info into static functions, and used them to make the PendingHTLCInfo. There are two validation closures passed to peel_payment_onion... some of the outbound channel checking is done in these closures, so its up to the caller to validate against current channel state.

I am unsure about phantom_shared_secret and allow_underpay in create_recv_pending_htlc_info so i left them None/false when called from peel_payment_onion

Not sure if this is the best approach! But its much better than #2677

TheBlueMatt · 2023-11-01T22:04:10Z

+ }
+ };
+
+ let validate_peer_chan = |


Rather than making these lambdas and passing them into decode_incoming_update_add_htlc_onion, can we just check them after running decode_incoming_update_add_htlc_onion?

Ok, its much cleaner now. I repeated the CLTV checks in the peel_payment_onion fn though, but I guess that's fine.
Also added a test for peel_payment_onion

codecov-commenter · 2023-11-01T23:20:25Z

Codecov Report

Attention: 44 lines in your changes are missing coverage. Please review.

Comparison is base (1f399b0) 88.71% compared to head (192fe05) 88.72%.
Report is 63 commits behind head on main.

Files	Patch %	Lines
lightning/src/ln/channelmanager.rs	89.21%	37 Missing and 7 partials ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@ Coverage Diff @@## main #2700 +/- ##
=========================================
Coverage 88.71% 88.72% =========================================
Files 112 113 +1 Lines 88502 89533 +1031 Branches 88502 89533 +1031 =========================================
+ Hits 78517 79434 +917 - Misses 7752 7839 +87 - Partials 2233 2260 +27

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

TheBlueMatt

Nice! This turned out pretty good. I think we'll basically be able to use this for LSP 4 too.

TheBlueMatt · 2023-11-02T22:44:33Z

+ if msg.cltv_expiry > cur_height + CLTV_FAR_FAR_AWAY as u32 { // expiry_too_far
+ return Err("CLTV expiry is too far in the future");
+ }
+ if (outgoing_cltv_value) as u64 <= (cur_height + LATENCY_GRACE_PERIOD_BLOCKS) as u64 {


Why keep the redundant one in decode_update_add_htlc_onion, can we just drop the redundant checks and leave it only doing the next-hop-channels-is-real checks.

Trying to figure out how to do this... the ChannelManager::decode_update_add_htlc_onion CLTV checks may generate a chan_update, while the checks in peel_payment_onion are simpler (since peel_payment_onion is called externally, without the context).
Are you saying we should move the checks inside the inner decode_incoming_update_add_htlc_onion fn?

ok! got it figured out so there is no redundancy

TheBlueMatt · 2023-11-02T22:46:46Z

+}
+
+/// Peel one layer off an incoming onion, returning PendingHTLCInfo (either Forward or Receive).
+/// Validates the outgoing CLTV value against the cur_height.


Docs should describe some amount of "this does all the relevant context-free checks that LDK requires for payment relay or acceptance. If the payment is to be received, and the amount matches the expected amount for a given invoice, this indicates the UpdateAddHTLC, once fully committed in the channel, will generate an Event::PaymentReceived".

TheBlueMatt · 2023-11-07T04:41:32Z

Oops, needs a small rebase after the other PR.

valentinewallace

Still making my way through

valentinewallace

Almost there!

valentinewallace

Needs rebase unfortunately :(

LGTM after this. Also fine with fixing these in follow-up.

valentinewallace · 2023-11-14T15:39:55Z

+	pub routing: PendingHTLCRouting,
+	/// Shared secret from the previous hop.
+	pub incoming_shared_secret: [u8; 32],
 	payment_hash: PaymentHash,


Why leave this as the only unexposed field?

Evanfeenstra · 2023-11-14T17:36:23Z

Needs rebase unfortunately :(
LGTM after this. Also fine with fixing these in follow-up.

OK, happy to do a follow-up on these items as soon as it gets merged

TheBlueMatt · 2023-11-14T18:34:18Z

Rather than using merge commits to rebase, please use rebase directly. See https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#rebasing-changes.

Evanfeenstra · 2023-11-14T19:59:20Z

Rather than using merge commits to rebase, please use rebase directly. See https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#rebasing-changes.

Oh, thanks for the tip. I reverted the merge commit. Also reverted the PeelOnionError stuff for a follow-up PR instead, per @valentinewallace's suggestion

valentinewallace · 2023-11-14T23:43:44Z

Having done more research into CLN and LND's htlc interception APIs, I don't think we need to follow-up adding PeelOnionError.

Both APIs take either an error code or a decoded onion error packet, so our current error should be sufficient for that.

I was also thinking the LSPS4 spec may need better error info, but from talking to @TheBlueMatt it sounds like the LSP would always just error with "unknown next channel."

So excuse the misleading direction, I think this PR is fine as-is.

TheBlueMatt

LGTM, though the docs on newly-public fields and structs really need to communicate a lot more information. Generally, any public field or struct needs docs that explain (a) what it is (often in terms of how we calculated it) and (b) what its used for, often how a user would use it (if there's no reason for them to use it, it shouldn't be public).

TheBlueMatt · 2023-11-15T19:28:50Z

- incoming_cltv_expiry: u32, // Used to track when we should expire pending HTLCs that go unclaimed
+ /// Used to track when we should expire pending HTLCs that go unclaimed.
+ incoming_cltv_expiry: u32,
+ /// Optional shared secret for phantom node.


This tells a user almost nothing about what this is or what its used for.

TheBlueMatt · 2023-11-15T19:29:20Z

 /// generated using `get_fake_scid` from the scid_utils::fake_scid module.
 short_channel_id: u64, // This should be NonZero<u64> eventually when we bump MSRV
 	},
+	/// An HTLC paid to an invoice we generated.


nit: might be worth noting that, at this point, we have not checked that the invoice being paid was actually generated by us, but rather its claiming to pay an invoice of ours.

TheBlueMatt · 2023-11-15T19:31:57Z

 /// See [`RecipientOnionFields::custom_tlvs`] for more info.
 custom_tlvs: Vec<(u64, Vec<u8>)>,
 	},
+	/// Incoming keysend (sender provided the preimage in a TLV).


Only kinda specific to this PR, but the public interface is now kinda confused between keysend and spontaneous payments. We originally tried to use "spontaneous payments" for this publicly, but some keysend references slipped in, and now we're adding more. We should make everything consistent in a followup rename.

TheBlueMatt · 2023-11-15T19:32:30Z

+ /// See [`RecipientOnionFields::payment_metadata`] for more info.
 payment_metadata: Option<Vec<u8>>,
- incoming_cltv_expiry: u32, // Used to track when we should expire pending HTLCs that go unclaimed
+ /// Used to track when we should expire pending HTLCs that go unclaimed.


nit: would be nice to also repeat what it is, not just what its used for.

TheBlueMatt · 2023-11-15T19:43:46Z

+pub struct PendingHTLCInfo {
+	/// Further routing details based on whether the HTLC is being forwarded or received.
+	pub routing: PendingHTLCRouting,
+	/// Shared secret from the previous hop.


This doesn't really say much about what this is used for/why its here.

TheBlueMatt · 2023-11-16T16:52:36Z

 	}
 }

+fn create_fwd_pending_htlc_info(


Now that these are all freestanding functions, they (and the associated return/error structs) should probably move to some other file, if only to make channelmanager.rs slightly less huge.

Improve docs on newly-public structs after #2700

Evanfeenstra · 2023-10-31T23:58:17Z

Adds a public peel_payment_onion function that takes msgs::UpdateAddHTLC and returns PendingHTLCInfo.

I separated the internals of decode_update_add_htlc_onion, construct_recv_pending_htlc_info, and construct_fwd_pending_htlc_info into static functions, and used them to make the PendingHTLCInfo. There are two validation closures passed to peel_payment_onion... some of the outbound channel checking is done in these closures, so its up to the caller to validate against current channel state.

I am unsure about phantom_shared_secret and allow_underpay in create_recv_pending_htlc_info so i left them None/false when called from peel_payment_onion

Not sure if this is the best approach! But its much better than #2677

TheBlueMatt · 2023-11-01T22:04:10Z

+ }
+ };
+
+ let validate_peer_chan = |


Rather than making these lambdas and passing them into decode_incoming_update_add_htlc_onion, can we just check them after running decode_incoming_update_add_htlc_onion?

Ok, its much cleaner now. I repeated the CLTV checks in the peel_payment_onion fn though, but I guess that's fine.
Also added a test for peel_payment_onion

codecov-commenter · 2023-11-01T23:20:25Z

Codecov Report

Attention: 44 lines in your changes are missing coverage. Please review.

Comparison is base (1f399b0) 88.71% compared to head (192fe05) 88.72%.
Report is 63 commits behind head on main.

Files	Patch %	Lines
lightning/src/ln/channelmanager.rs	89.21%	37 Missing and 7 partials ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@ Coverage Diff @@## main #2700 +/- ##
=========================================
Coverage 88.71% 88.72% =========================================
Files 112 113 +1 Lines 88502 89533 +1031 Branches 88502 89533 +1031 =========================================
+ Hits 78517 79434 +917 - Misses 7752 7839 +87 - Partials 2233 2260 +27

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

TheBlueMatt

Nice! This turned out pretty good. I think we'll basically be able to use this for LSP 4 too.

TheBlueMatt · 2023-11-02T22:44:33Z

+ if msg.cltv_expiry > cur_height + CLTV_FAR_FAR_AWAY as u32 { // expiry_too_far
+ return Err("CLTV expiry is too far in the future");
+ }
+ if (outgoing_cltv_value) as u64 <= (cur_height + LATENCY_GRACE_PERIOD_BLOCKS) as u64 {


Why keep the redundant one in decode_update_add_htlc_onion, can we just drop the redundant checks and leave it only doing the next-hop-channels-is-real checks.

Trying to figure out how to do this... the ChannelManager::decode_update_add_htlc_onion CLTV checks may generate a chan_update, while the checks in peel_payment_onion are simpler (since peel_payment_onion is called externally, without the context).
Are you saying we should move the checks inside the inner decode_incoming_update_add_htlc_onion fn?

ok! got it figured out so there is no redundancy

TheBlueMatt · 2023-11-02T22:46:46Z

+}
+
+/// Peel one layer off an incoming onion, returning PendingHTLCInfo (either Forward or Receive).
+/// Validates the outgoing CLTV value against the cur_height.


Docs should describe some amount of "this does all the relevant context-free checks that LDK requires for payment relay or acceptance. If the payment is to be received, and the amount matches the expected amount for a given invoice, this indicates the UpdateAddHTLC, once fully committed in the channel, will generate an Event::PaymentReceived".

TheBlueMatt · 2023-11-07T04:41:32Z

Oops, needs a small rebase after the other PR.

valentinewallace

Still making my way through

valentinewallace

Almost there!

valentinewallace

Needs rebase unfortunately :(

LGTM after this. Also fine with fixing these in follow-up.

valentinewallace · 2023-11-14T15:39:55Z

+	pub routing: PendingHTLCRouting,
+	/// Shared secret from the previous hop.
+	pub incoming_shared_secret: [u8; 32],
 	payment_hash: PaymentHash,


Why leave this as the only unexposed field?

Evanfeenstra · 2023-11-14T17:36:23Z

Needs rebase unfortunately :(
LGTM after this. Also fine with fixing these in follow-up.

OK, happy to do a follow-up on these items as soon as it gets merged

TheBlueMatt · 2023-11-14T18:34:18Z

Rather than using merge commits to rebase, please use rebase directly. See https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#rebasing-changes.

Evanfeenstra · 2023-11-14T19:59:20Z

Rather than using merge commits to rebase, please use rebase directly. See https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#rebasing-changes.

Oh, thanks for the tip. I reverted the merge commit. Also reverted the PeelOnionError stuff for a follow-up PR instead, per @valentinewallace's suggestion

valentinewallace · 2023-11-14T23:43:44Z

Having done more research into CLN and LND's htlc interception APIs, I don't think we need to follow-up adding PeelOnionError.

Both APIs take either an error code or a decoded onion error packet, so our current error should be sufficient for that.

I was also thinking the LSPS4 spec may need better error info, but from talking to @TheBlueMatt it sounds like the LSP would always just error with "unknown next channel."

So excuse the misleading direction, I think this PR is fine as-is.

TheBlueMatt

LGTM, though the docs on newly-public fields and structs really need to communicate a lot more information. Generally, any public field or struct needs docs that explain (a) what it is (often in terms of how we calculated it) and (b) what its used for, often how a user would use it (if there's no reason for them to use it, it shouldn't be public).

TheBlueMatt · 2023-11-15T19:28:50Z

- incoming_cltv_expiry: u32, // Used to track when we should expire pending HTLCs that go unclaimed
+ /// Used to track when we should expire pending HTLCs that go unclaimed.
+ incoming_cltv_expiry: u32,
+ /// Optional shared secret for phantom node.


This tells a user almost nothing about what this is or what its used for.

TheBlueMatt · 2023-11-15T19:29:20Z

 /// generated using `get_fake_scid` from the scid_utils::fake_scid module.
 short_channel_id: u64, // This should be NonZero<u64> eventually when we bump MSRV
 	},
+	/// An HTLC paid to an invoice we generated.


nit: might be worth noting that, at this point, we have not checked that the invoice being paid was actually generated by us, but rather its claiming to pay an invoice of ours.

TheBlueMatt · 2023-11-15T19:31:57Z

 /// See [`RecipientOnionFields::custom_tlvs`] for more info.
 custom_tlvs: Vec<(u64, Vec<u8>)>,
 	},
+	/// Incoming keysend (sender provided the preimage in a TLV).


Only kinda specific to this PR, but the public interface is now kinda confused between keysend and spontaneous payments. We originally tried to use "spontaneous payments" for this publicly, but some keysend references slipped in, and now we're adding more. We should make everything consistent in a followup rename.

TheBlueMatt · 2023-11-15T19:32:30Z

+ /// See [`RecipientOnionFields::payment_metadata`] for more info.
 payment_metadata: Option<Vec<u8>>,
- incoming_cltv_expiry: u32, // Used to track when we should expire pending HTLCs that go unclaimed
+ /// Used to track when we should expire pending HTLCs that go unclaimed.


nit: would be nice to also repeat what it is, not just what its used for.

TheBlueMatt · 2023-11-15T19:43:46Z

+pub struct PendingHTLCInfo {
+	/// Further routing details based on whether the HTLC is being forwarded or received.
+	pub routing: PendingHTLCRouting,
+	/// Shared secret from the previous hop.


This doesn't really say much about what this is used for/why its here.

TheBlueMatt · 2023-11-16T16:52:36Z

 	}
 }

+fn create_fwd_pending_htlc_info(


Now that these are all freestanding functions, they (and the associated return/error structs) should probably move to some other file, if only to make channelmanager.rs slightly less huge.

Improve docs on newly-public structs after #2700

TheBlueMatt reviewed Nov 1, 2023
View reviewed changes

TheBlueMatt mentioned this pull request Nov 2, 2023
public create_payment_onion in onion_utils #2677
Merged

valentinewallace self-requested a review November 2, 2023 22:42

TheBlueMatt reviewed Nov 2, 2023
View reviewed changes

TheBlueMatt assigned valentinewallace Nov 7, 2023

TheBlueMatt self-assigned this Nov 7, 2023

Evanfeenstra force-pushed the pub-htlc-routing branch from 947590c to 729e5a7Compare November 7, 2023 07:14

valentinewallace reviewed Nov 7, 2023
View reviewed changes

Evanfeenstra force-pushed the pub-htlc-routing branch from 1f5a5f7 to c173aafCompare November 7, 2023 21:09

valentinewallace reviewed Nov 8, 2023
View reviewed changes

Evanfeenstra force-pushed the pub-htlc-routing branch 2 times, most recently from 1a37835 to 98bc9a9Compare November 9, 2023 00:56

valentinewallace reviewed Nov 10, 2023
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs

Evanfeenstra force-pushed the pub-htlc-routing branch from d3cc8a3 to 3ddb6aaCompare November 11, 2023 21:13

valentinewallace reviewed Nov 14, 2023
View reviewed changes

Evanfeenstra force-pushed the pub-htlc-routing branch 2 times, most recently from 3a9fe46 to 3ddb6aaCompare November 14, 2023 19:50

peel_payment_onion static fn in channelmanager
4b5db8c

Evanfeenstra force-pushed the pub-htlc-routing branch from ad173b3 to 4b5db8cCompare November 14, 2023 19:54

remove obsolete comment
378885d

valentinewallace previously approved these changes Nov 14, 2023
View reviewed changes

valentinewallace reviewed Nov 15, 2023
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated

InboundOnionErr fields public
192fe05

Evanfeenstra dismissed valentinewallace’s stale review via 192fe05November 15, 2023 21:09

valentinewallace approved these changes Nov 15, 2023
View reviewed changes

TheBlueMatt approved these changes Nov 16, 2023
View reviewed changes

TheBlueMatt merged commit 870a0f1 into lightningdevkit:mainNov 16, 2023

Evanfeenstra mentioned this pull request Nov 16, 2023
move static channelmanager functions into their own file #2739
Merged

tnull added a commit that referenced this pull request Dec 6, 2023
Merge pull request#2762from TheBlueMatt/2023-11-htlc-docs
63497e9
Improve docs on newly-public structs after #2700