Implement non-strict forwarding by wvanlint · Pull Request #3127 · lightningdevkit/rust-lightning

wvanlint · 2024-06-14T23:26:57Z

This change implements non-strict forwarding, allowing the node to forward an HTLC along a channel other than the one specified by short_channel_id in the onion message, so long as the receiver has the same node public key intended by short_channel_id (BOLT). This can improve payment reliability when there are multiple channels with the same peer e.g. when outbound liquidity is replenished by opening a new channel.

The implemented forwarding strategy now chooses the channel with the least amount of outbound liquidity that can forward an HTLC to maximize the probability of being able to successfully forward a subsequent HTLC.

Fixes#1278.

I also tested a refactoring to reduce duplication by processing all HTLCForwardInfo::AddHTLCs separately first. However, the current approach is a more minimal and clearer change and we can assume that the order of magnitude of pending forwards will be small per channel.

codecov-commenter · 2024-06-14T23:43:35Z

Codecov Report

Attention: Patch coverage is 85.89744% with 22 lines in your changes missing coverage. Please review.

Project coverage is 89.90%. Comparing base (07f3380) to head (8d915a8).

Files	Patch %	Lines
lightning/src/ln/channelmanager.rs	70.76%	14 Missing and 5 partials ⚠️
lightning/src/ln/payment_tests.rs	96.70%	1 Missing and 2 partials ⚠️

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

Additional details and impacted files

@@ Coverage Diff @@## main #3127 +/- ##
==========================================
- Coverage 89.92% 89.90% -0.03% 
==========================================
Files 121 121 Lines 99172 99290 +118 Branches 99172 99290 +118 ==========================================
+ Hits 89180 89263 +83 - Misses 7391 7415 +24 - Partials 2601 2612 +11

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

TheBlueMatt

Thanks!

TheBlueMatt · 2024-06-17T14:29:58Z

+ let mut channels_with_peer = peer_state.channel_by_id.values_mut().filter_map(|phase| match phase {
+ ChannelPhase::Funded(chan) => Some(chan),
+ _ => None,
+ }).collect::<Vec<&mut Channel<_>>>();


Rather than iterate + collect, we should be able to just pick a singular channel we want to use by iterating, filter_map'ing, and min_by'ing using Channel::get_available_balances and selecting the channel that has the lowest next_outbound_htlc_limit_msat above the amount we're trying to send. That would also simplify the patch as we don't have to iterate channels and try to add anymore.

Done. This was mainly done to avoid duplication with the validation in send_htlc. Do we also want to verify the establishment/shutdown state?
rust-lightning/lightning/src/ln/channel.rs
Lines 7049 to 7083 in 737df0f
if !matches!(self.context.channel_state,ChannelState::ChannelReady(_)) ||
self.context.channel_state.is_local_shutdown_sent() ||
self.context.channel_state.is_remote_shutdown_sent()
{
returnErr(ChannelError::Ignore("Cannot send HTLC until channel is fully established and we haven't started shutting down".to_owned()));
}
let channel_total_msat = self.context.channel_value_satoshis*1000;
if amount_msat > channel_total_msat {
returnErr(ChannelError::Ignore(format!("Cannot send amount {}, because it is more than the total value of the channel {}", amount_msat, channel_total_msat)));
}
if amount_msat == 0{
returnErr(ChannelError::Ignore("Cannot send 0-msat HTLC".to_owned()));
}
let available_balances = self.context.get_available_balances(fee_estimator);
if amount_msat < available_balances.next_outbound_htlc_minimum_msat{
returnErr(ChannelError::Ignore(format!("Cannot send less than our next-HTLC minimum - {} msat",
available_balances.next_outbound_htlc_minimum_msat)));
}
if amount_msat > available_balances.next_outbound_htlc_limit_msat{
returnErr(ChannelError::Ignore(format!("Cannot send more than our next-HTLC maximum - {} msat",
available_balances.next_outbound_htlc_limit_msat)));
}
ifself.context.channel_state.is_peer_disconnected(){
// Note that this should never really happen, if we're !is_live() on receipt of an
// incoming HTLC for relay will result in us rejecting the HTLC and we won't allow
// the user to send directly into a !is_live() channel. However, if we
// disconnected during the time the previous hop was doing the commitment dance we may
// end up getting here after the forwarding delay. In any case, returning an
// IgnoreError will get ChannelManager to do the right thing and fail backwards now.
returnErr(ChannelError::Ignore("Cannot send an HTLC while disconnected from channel counterparty".to_owned()));
}

Had to adjust full_stack::tests::test_no_existing_test_breakage to account for the additional fee estimation call in get_available_balances.

Do we also want to verify the establishment/shutdown state?
To check back in here, I did add an is_usable() check which I believe is necessary, but let me know if that's not the case.

tnull

Mostly looks good to me, just some questions.

Feel free to squash the fixup!

This change implements non-strict forwarding, allowing the node to forward an HTLC along a channel other than the one specified by short_channel_id in the onion message, so long as the receiver has the same node public key intended by short_channel_id ([BOLT](https://github.com/lightning/bolts/blob/57ce4b1e05c996fa649f00dc13521f6d496a288f/04-onion-routing.md#non-strict-forwarding)). This can improve payment reliability when there are multiple channels with the same peer e.g. when outbound liquidity is replenished by opening a new channel. The implemented forwarding strategy now chooses the channel with the lowest outbound liquidity that can forward an HTLC to maximize the probability of being able to successfully forward a subsequent HTLC. Fixeslightningdevkit#1278.

wvanlint · 2024-06-20T04:37:18Z

Thanks! Squashed the fixups.

tnull · 2024-06-20T07:29:01Z

Thanks! Squashed the fixups.

Thanks, just a quick note: next time it would help reviewing if you left them up for ~another round of review (i.e., ask reviewers before squashing them). Given that the whole block was re-indented, its now non-trivial to see what really changed with the last force-push.

tnull

LGTM

I'll leave the last ACK to Matt as he requested some of the changes added since he ACKed last.

wvanlint · 2024-06-14T23:26:57Z

This change implements non-strict forwarding, allowing the node to forward an HTLC along a channel other than the one specified by short_channel_id in the onion message, so long as the receiver has the same node public key intended by short_channel_id (BOLT). This can improve payment reliability when there are multiple channels with the same peer e.g. when outbound liquidity is replenished by opening a new channel.

The implemented forwarding strategy now chooses the channel with the least amount of outbound liquidity that can forward an HTLC to maximize the probability of being able to successfully forward a subsequent HTLC.

Fixes#1278.

I also tested a refactoring to reduce duplication by processing all HTLCForwardInfo::AddHTLCs separately first. However, the current approach is a more minimal and clearer change and we can assume that the order of magnitude of pending forwards will be small per channel.

codecov-commenter · 2024-06-14T23:43:35Z

Codecov Report

Attention: Patch coverage is 85.89744% with 22 lines in your changes missing coverage. Please review.

Project coverage is 89.90%. Comparing base (07f3380) to head (8d915a8).

Files	Patch %	Lines
lightning/src/ln/channelmanager.rs	70.76%	14 Missing and 5 partials ⚠️
lightning/src/ln/payment_tests.rs	96.70%	1 Missing and 2 partials ⚠️

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

Additional details and impacted files

@@ Coverage Diff @@## main #3127 +/- ##
==========================================
- Coverage 89.92% 89.90% -0.03% 
==========================================
Files 121 121 Lines 99172 99290 +118 Branches 99172 99290 +118 ==========================================
+ Hits 89180 89263 +83 - Misses 7391 7415 +24 - Partials 2601 2612 +11

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

TheBlueMatt

Thanks!

TheBlueMatt · 2024-06-17T14:29:58Z

+ let mut channels_with_peer = peer_state.channel_by_id.values_mut().filter_map(|phase| match phase {
+ ChannelPhase::Funded(chan) => Some(chan),
+ _ => None,
+ }).collect::<Vec<&mut Channel<_>>>();


Rather than iterate + collect, we should be able to just pick a singular channel we want to use by iterating, filter_map'ing, and min_by'ing using Channel::get_available_balances and selecting the channel that has the lowest next_outbound_htlc_limit_msat above the amount we're trying to send. That would also simplify the patch as we don't have to iterate channels and try to add anymore.

Done. This was mainly done to avoid duplication with the validation in send_htlc. Do we also want to verify the establishment/shutdown state?
rust-lightning/lightning/src/ln/channel.rs
Lines 7049 to 7083 in 737df0f
if !matches!(self.context.channel_state,ChannelState::ChannelReady(_)) ||
self.context.channel_state.is_local_shutdown_sent() ||
self.context.channel_state.is_remote_shutdown_sent()
{
returnErr(ChannelError::Ignore("Cannot send HTLC until channel is fully established and we haven't started shutting down".to_owned()));
}
let channel_total_msat = self.context.channel_value_satoshis*1000;
if amount_msat > channel_total_msat {
returnErr(ChannelError::Ignore(format!("Cannot send amount {}, because it is more than the total value of the channel {}", amount_msat, channel_total_msat)));
}
if amount_msat == 0{
returnErr(ChannelError::Ignore("Cannot send 0-msat HTLC".to_owned()));
}
let available_balances = self.context.get_available_balances(fee_estimator);
if amount_msat < available_balances.next_outbound_htlc_minimum_msat{
returnErr(ChannelError::Ignore(format!("Cannot send less than our next-HTLC minimum - {} msat",
available_balances.next_outbound_htlc_minimum_msat)));
}
if amount_msat > available_balances.next_outbound_htlc_limit_msat{
returnErr(ChannelError::Ignore(format!("Cannot send more than our next-HTLC maximum - {} msat",
available_balances.next_outbound_htlc_limit_msat)));
}
ifself.context.channel_state.is_peer_disconnected(){
// Note that this should never really happen, if we're !is_live() on receipt of an
// incoming HTLC for relay will result in us rejecting the HTLC and we won't allow
// the user to send directly into a !is_live() channel. However, if we
// disconnected during the time the previous hop was doing the commitment dance we may
// end up getting here after the forwarding delay. In any case, returning an
// IgnoreError will get ChannelManager to do the right thing and fail backwards now.
returnErr(ChannelError::Ignore("Cannot send an HTLC while disconnected from channel counterparty".to_owned()));
}

Had to adjust full_stack::tests::test_no_existing_test_breakage to account for the additional fee estimation call in get_available_balances.

Do we also want to verify the establishment/shutdown state?
To check back in here, I did add an is_usable() check which I believe is necessary, but let me know if that's not the case.

tnull

Mostly looks good to me, just some questions.

Feel free to squash the fixup!

This change implements non-strict forwarding, allowing the node to forward an HTLC along a channel other than the one specified by short_channel_id in the onion message, so long as the receiver has the same node public key intended by short_channel_id ([BOLT](https://github.com/lightning/bolts/blob/57ce4b1e05c996fa649f00dc13521f6d496a288f/04-onion-routing.md#non-strict-forwarding)). This can improve payment reliability when there are multiple channels with the same peer e.g. when outbound liquidity is replenished by opening a new channel. The implemented forwarding strategy now chooses the channel with the lowest outbound liquidity that can forward an HTLC to maximize the probability of being able to successfully forward a subsequent HTLC. Fixeslightningdevkit#1278.

wvanlint · 2024-06-20T04:37:18Z

Thanks! Squashed the fixups.

tnull · 2024-06-20T07:29:01Z

Thanks! Squashed the fixups.

Thanks, just a quick note: next time it would help reviewing if you left them up for ~another round of review (i.e., ask reviewers before squashing them). Given that the whole block was re-indented, its now non-trivial to see what really changed with the last force-push.

tnull

LGTM

I'll leave the last ACK to Matt as he requested some of the changes added since he ACKed last.

wvanlint · 2024-06-14T23:26:57Z

This change implements non-strict forwarding, allowing the node to forward an HTLC along a channel other than the one specified by short_channel_id in the onion message, so long as the receiver has the same node public key intended by short_channel_id (BOLT). This can improve payment reliability when there are multiple channels with the same peer e.g. when outbound liquidity is replenished by opening a new channel.

The implemented forwarding strategy now chooses the channel with the least amount of outbound liquidity that can forward an HTLC to maximize the probability of being able to successfully forward a subsequent HTLC.

Fixes#1278.

I also tested a refactoring to reduce duplication by processing all HTLCForwardInfo::AddHTLCs separately first. However, the current approach is a more minimal and clearer change and we can assume that the order of magnitude of pending forwards will be small per channel.

codecov-commenter · 2024-06-14T23:43:35Z

Codecov Report

Attention: Patch coverage is 85.89744% with 22 lines in your changes missing coverage. Please review.

Project coverage is 89.90%. Comparing base (07f3380) to head (8d915a8).

Files	Patch %	Lines
lightning/src/ln/channelmanager.rs	70.76%	14 Missing and 5 partials ⚠️
lightning/src/ln/payment_tests.rs	96.70%	1 Missing and 2 partials ⚠️

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

Additional details and impacted files

@@ Coverage Diff @@## main #3127 +/- ##
==========================================
- Coverage 89.92% 89.90% -0.03% 
==========================================
Files 121 121 Lines 99172 99290 +118 Branches 99172 99290 +118 ==========================================
+ Hits 89180 89263 +83 - Misses 7391 7415 +24 - Partials 2601 2612 +11

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

TheBlueMatt

Thanks!

TheBlueMatt · 2024-06-17T14:29:58Z

+ let mut channels_with_peer = peer_state.channel_by_id.values_mut().filter_map(|phase| match phase {
+ ChannelPhase::Funded(chan) => Some(chan),
+ _ => None,
+ }).collect::<Vec<&mut Channel<_>>>();


Rather than iterate + collect, we should be able to just pick a singular channel we want to use by iterating, filter_map'ing, and min_by'ing using Channel::get_available_balances and selecting the channel that has the lowest next_outbound_htlc_limit_msat above the amount we're trying to send. That would also simplify the patch as we don't have to iterate channels and try to add anymore.

Done. This was mainly done to avoid duplication with the validation in send_htlc. Do we also want to verify the establishment/shutdown state?
rust-lightning/lightning/src/ln/channel.rs
Lines 7049 to 7083 in 737df0f
if !matches!(self.context.channel_state,ChannelState::ChannelReady(_)) ||
self.context.channel_state.is_local_shutdown_sent() ||
self.context.channel_state.is_remote_shutdown_sent()
{
returnErr(ChannelError::Ignore("Cannot send HTLC until channel is fully established and we haven't started shutting down".to_owned()));
}
let channel_total_msat = self.context.channel_value_satoshis*1000;
if amount_msat > channel_total_msat {
returnErr(ChannelError::Ignore(format!("Cannot send amount {}, because it is more than the total value of the channel {}", amount_msat, channel_total_msat)));
}
if amount_msat == 0{
returnErr(ChannelError::Ignore("Cannot send 0-msat HTLC".to_owned()));
}
let available_balances = self.context.get_available_balances(fee_estimator);
if amount_msat < available_balances.next_outbound_htlc_minimum_msat{
returnErr(ChannelError::Ignore(format!("Cannot send less than our next-HTLC minimum - {} msat",
available_balances.next_outbound_htlc_minimum_msat)));
}
if amount_msat > available_balances.next_outbound_htlc_limit_msat{
returnErr(ChannelError::Ignore(format!("Cannot send more than our next-HTLC maximum - {} msat",
available_balances.next_outbound_htlc_limit_msat)));
}
ifself.context.channel_state.is_peer_disconnected(){
// Note that this should never really happen, if we're !is_live() on receipt of an
// incoming HTLC for relay will result in us rejecting the HTLC and we won't allow
// the user to send directly into a !is_live() channel. However, if we
// disconnected during the time the previous hop was doing the commitment dance we may
// end up getting here after the forwarding delay. In any case, returning an
// IgnoreError will get ChannelManager to do the right thing and fail backwards now.
returnErr(ChannelError::Ignore("Cannot send an HTLC while disconnected from channel counterparty".to_owned()));
}

Had to adjust full_stack::tests::test_no_existing_test_breakage to account for the additional fee estimation call in get_available_balances.

Do we also want to verify the establishment/shutdown state?
To check back in here, I did add an is_usable() check which I believe is necessary, but let me know if that's not the case.

tnull

Mostly looks good to me, just some questions.

Feel free to squash the fixup!

This change implements non-strict forwarding, allowing the node to forward an HTLC along a channel other than the one specified by short_channel_id in the onion message, so long as the receiver has the same node public key intended by short_channel_id ([BOLT](https://github.com/lightning/bolts/blob/57ce4b1e05c996fa649f00dc13521f6d496a288f/04-onion-routing.md#non-strict-forwarding)). This can improve payment reliability when there are multiple channels with the same peer e.g. when outbound liquidity is replenished by opening a new channel. The implemented forwarding strategy now chooses the channel with the lowest outbound liquidity that can forward an HTLC to maximize the probability of being able to successfully forward a subsequent HTLC. Fixeslightningdevkit#1278.

wvanlint · 2024-06-20T04:37:18Z

Thanks! Squashed the fixups.

tnull · 2024-06-20T07:29:01Z

Thanks! Squashed the fixups.

Thanks, just a quick note: next time it would help reviewing if you left them up for ~another round of review (i.e., ask reviewers before squashing them). Given that the whole block was re-indented, its now non-trivial to see what really changed with the last force-push.

tnull

LGTM

I'll leave the last ACK to Matt as he requested some of the changes added since he ACKed last.

wvanlint · 2024-06-14T23:26:57Z

This change implements non-strict forwarding, allowing the node to forward an HTLC along a channel other than the one specified by short_channel_id in the onion message, so long as the receiver has the same node public key intended by short_channel_id (BOLT). This can improve payment reliability when there are multiple channels with the same peer e.g. when outbound liquidity is replenished by opening a new channel.

The implemented forwarding strategy now chooses the channel with the least amount of outbound liquidity that can forward an HTLC to maximize the probability of being able to successfully forward a subsequent HTLC.

Fixes#1278.

I also tested a refactoring to reduce duplication by processing all HTLCForwardInfo::AddHTLCs separately first. However, the current approach is a more minimal and clearer change and we can assume that the order of magnitude of pending forwards will be small per channel.

codecov-commenter · 2024-06-14T23:43:35Z

Codecov Report

Attention: Patch coverage is 85.89744% with 22 lines in your changes missing coverage. Please review.

Project coverage is 89.90%. Comparing base (07f3380) to head (8d915a8).

Files	Patch %	Lines
lightning/src/ln/channelmanager.rs	70.76%	14 Missing and 5 partials ⚠️
lightning/src/ln/payment_tests.rs	96.70%	1 Missing and 2 partials ⚠️

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

Additional details and impacted files

@@ Coverage Diff @@## main #3127 +/- ##
==========================================
- Coverage 89.92% 89.90% -0.03% 
==========================================
Files 121 121 Lines 99172 99290 +118 Branches 99172 99290 +118 ==========================================
+ Hits 89180 89263 +83 - Misses 7391 7415 +24 - Partials 2601 2612 +11

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

TheBlueMatt

Thanks!

TheBlueMatt · 2024-06-17T14:29:58Z

+ let mut channels_with_peer = peer_state.channel_by_id.values_mut().filter_map(|phase| match phase {
+ ChannelPhase::Funded(chan) => Some(chan),
+ _ => None,
+ }).collect::<Vec<&mut Channel<_>>>();


Rather than iterate + collect, we should be able to just pick a singular channel we want to use by iterating, filter_map'ing, and min_by'ing using Channel::get_available_balances and selecting the channel that has the lowest next_outbound_htlc_limit_msat above the amount we're trying to send. That would also simplify the patch as we don't have to iterate channels and try to add anymore.

Done. This was mainly done to avoid duplication with the validation in send_htlc. Do we also want to verify the establishment/shutdown state?
rust-lightning/lightning/src/ln/channel.rs
Lines 7049 to 7083 in 737df0f
if !matches!(self.context.channel_state,ChannelState::ChannelReady(_)) ||
self.context.channel_state.is_local_shutdown_sent() ||
self.context.channel_state.is_remote_shutdown_sent()
{
returnErr(ChannelError::Ignore("Cannot send HTLC until channel is fully established and we haven't started shutting down".to_owned()));
}
let channel_total_msat = self.context.channel_value_satoshis*1000;
if amount_msat > channel_total_msat {
returnErr(ChannelError::Ignore(format!("Cannot send amount {}, because it is more than the total value of the channel {}", amount_msat, channel_total_msat)));
}
if amount_msat == 0{
returnErr(ChannelError::Ignore("Cannot send 0-msat HTLC".to_owned()));
}
let available_balances = self.context.get_available_balances(fee_estimator);
if amount_msat < available_balances.next_outbound_htlc_minimum_msat{
returnErr(ChannelError::Ignore(format!("Cannot send less than our next-HTLC minimum - {} msat",
available_balances.next_outbound_htlc_minimum_msat)));
}
if amount_msat > available_balances.next_outbound_htlc_limit_msat{
returnErr(ChannelError::Ignore(format!("Cannot send more than our next-HTLC maximum - {} msat",
available_balances.next_outbound_htlc_limit_msat)));
}
ifself.context.channel_state.is_peer_disconnected(){
// Note that this should never really happen, if we're !is_live() on receipt of an
// incoming HTLC for relay will result in us rejecting the HTLC and we won't allow
// the user to send directly into a !is_live() channel. However, if we
// disconnected during the time the previous hop was doing the commitment dance we may
// end up getting here after the forwarding delay. In any case, returning an
// IgnoreError will get ChannelManager to do the right thing and fail backwards now.
returnErr(ChannelError::Ignore("Cannot send an HTLC while disconnected from channel counterparty".to_owned()));
}

Had to adjust full_stack::tests::test_no_existing_test_breakage to account for the additional fee estimation call in get_available_balances.

Do we also want to verify the establishment/shutdown state?
To check back in here, I did add an is_usable() check which I believe is necessary, but let me know if that's not the case.

tnull

Mostly looks good to me, just some questions.

Feel free to squash the fixup!

This change implements non-strict forwarding, allowing the node to forward an HTLC along a channel other than the one specified by short_channel_id in the onion message, so long as the receiver has the same node public key intended by short_channel_id ([BOLT](https://github.com/lightning/bolts/blob/57ce4b1e05c996fa649f00dc13521f6d496a288f/04-onion-routing.md#non-strict-forwarding)). This can improve payment reliability when there are multiple channels with the same peer e.g. when outbound liquidity is replenished by opening a new channel. The implemented forwarding strategy now chooses the channel with the lowest outbound liquidity that can forward an HTLC to maximize the probability of being able to successfully forward a subsequent HTLC. Fixeslightningdevkit#1278.

wvanlint · 2024-06-20T04:37:18Z

Thanks! Squashed the fixups.

tnull · 2024-06-20T07:29:01Z

Thanks! Squashed the fixups.

Thanks, just a quick note: next time it would help reviewing if you left them up for ~another round of review (i.e., ask reviewers before squashing them). Given that the whole block was re-indented, its now non-trivial to see what really changed with the last force-push.

tnull

LGTM

I'll leave the last ACK to Matt as he requested some of the changes added since he ACKed last.

wvanlint · 2024-06-14T23:26:57Z

This change implements non-strict forwarding, allowing the node to forward an HTLC along a channel other than the one specified by short_channel_id in the onion message, so long as the receiver has the same node public key intended by short_channel_id (BOLT). This can improve payment reliability when there are multiple channels with the same peer e.g. when outbound liquidity is replenished by opening a new channel.

The implemented forwarding strategy now chooses the channel with the least amount of outbound liquidity that can forward an HTLC to maximize the probability of being able to successfully forward a subsequent HTLC.

Fixes#1278.

I also tested a refactoring to reduce duplication by processing all HTLCForwardInfo::AddHTLCs separately first. However, the current approach is a more minimal and clearer change and we can assume that the order of magnitude of pending forwards will be small per channel.

codecov-commenter · 2024-06-14T23:43:35Z

Codecov Report

Attention: Patch coverage is 85.89744% with 22 lines in your changes missing coverage. Please review.

Project coverage is 89.90%. Comparing base (07f3380) to head (8d915a8).

Files	Patch %	Lines
lightning/src/ln/channelmanager.rs	70.76%	14 Missing and 5 partials ⚠️
lightning/src/ln/payment_tests.rs	96.70%	1 Missing and 2 partials ⚠️

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

Additional details and impacted files

@@ Coverage Diff @@## main #3127 +/- ##
==========================================
- Coverage 89.92% 89.90% -0.03% 
==========================================
Files 121 121 Lines 99172 99290 +118 Branches 99172 99290 +118 ==========================================
+ Hits 89180 89263 +83 - Misses 7391 7415 +24 - Partials 2601 2612 +11

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

TheBlueMatt

Thanks!

TheBlueMatt · 2024-06-17T14:29:58Z

+ let mut channels_with_peer = peer_state.channel_by_id.values_mut().filter_map(|phase| match phase {
+ ChannelPhase::Funded(chan) => Some(chan),
+ _ => None,
+ }).collect::<Vec<&mut Channel<_>>>();


Rather than iterate + collect, we should be able to just pick a singular channel we want to use by iterating, filter_map'ing, and min_by'ing using Channel::get_available_balances and selecting the channel that has the lowest next_outbound_htlc_limit_msat above the amount we're trying to send. That would also simplify the patch as we don't have to iterate channels and try to add anymore.

Done. This was mainly done to avoid duplication with the validation in send_htlc. Do we also want to verify the establishment/shutdown state?
rust-lightning/lightning/src/ln/channel.rs
Lines 7049 to 7083 in 737df0f
if !matches!(self.context.channel_state,ChannelState::ChannelReady(_)) ||
self.context.channel_state.is_local_shutdown_sent() ||
self.context.channel_state.is_remote_shutdown_sent()
{
returnErr(ChannelError::Ignore("Cannot send HTLC until channel is fully established and we haven't started shutting down".to_owned()));
}
let channel_total_msat = self.context.channel_value_satoshis*1000;
if amount_msat > channel_total_msat {
returnErr(ChannelError::Ignore(format!("Cannot send amount {}, because it is more than the total value of the channel {}", amount_msat, channel_total_msat)));
}
if amount_msat == 0{
returnErr(ChannelError::Ignore("Cannot send 0-msat HTLC".to_owned()));
}
let available_balances = self.context.get_available_balances(fee_estimator);
if amount_msat < available_balances.next_outbound_htlc_minimum_msat{
returnErr(ChannelError::Ignore(format!("Cannot send less than our next-HTLC minimum - {} msat",
available_balances.next_outbound_htlc_minimum_msat)));
}
if amount_msat > available_balances.next_outbound_htlc_limit_msat{
returnErr(ChannelError::Ignore(format!("Cannot send more than our next-HTLC maximum - {} msat",
available_balances.next_outbound_htlc_limit_msat)));
}
ifself.context.channel_state.is_peer_disconnected(){
// Note that this should never really happen, if we're !is_live() on receipt of an
// incoming HTLC for relay will result in us rejecting the HTLC and we won't allow
// the user to send directly into a !is_live() channel. However, if we
// disconnected during the time the previous hop was doing the commitment dance we may
// end up getting here after the forwarding delay. In any case, returning an
// IgnoreError will get ChannelManager to do the right thing and fail backwards now.
returnErr(ChannelError::Ignore("Cannot send an HTLC while disconnected from channel counterparty".to_owned()));
}

Had to adjust full_stack::tests::test_no_existing_test_breakage to account for the additional fee estimation call in get_available_balances.

Do we also want to verify the establishment/shutdown state?
To check back in here, I did add an is_usable() check which I believe is necessary, but let me know if that's not the case.

tnull

Mostly looks good to me, just some questions.

Feel free to squash the fixup!

This change implements non-strict forwarding, allowing the node to forward an HTLC along a channel other than the one specified by short_channel_id in the onion message, so long as the receiver has the same node public key intended by short_channel_id ([BOLT](https://github.com/lightning/bolts/blob/57ce4b1e05c996fa649f00dc13521f6d496a288f/04-onion-routing.md#non-strict-forwarding)). This can improve payment reliability when there are multiple channels with the same peer e.g. when outbound liquidity is replenished by opening a new channel. The implemented forwarding strategy now chooses the channel with the lowest outbound liquidity that can forward an HTLC to maximize the probability of being able to successfully forward a subsequent HTLC. Fixeslightningdevkit#1278.

wvanlint · 2024-06-20T04:37:18Z

Thanks! Squashed the fixups.

tnull · 2024-06-20T07:29:01Z

Thanks! Squashed the fixups.

Thanks, just a quick note: next time it would help reviewing if you left them up for ~another round of review (i.e., ask reviewers before squashing them). Given that the whole block was re-indented, its now non-trivial to see what really changed with the last force-push.

tnull

LGTM

I'll leave the last ACK to Matt as he requested some of the changes added since he ACKed last.

wvanlint · 2024-06-14T23:26:57Z

This change implements non-strict forwarding, allowing the node to forward an HTLC along a channel other than the one specified by short_channel_id in the onion message, so long as the receiver has the same node public key intended by short_channel_id (BOLT). This can improve payment reliability when there are multiple channels with the same peer e.g. when outbound liquidity is replenished by opening a new channel.

The implemented forwarding strategy now chooses the channel with the least amount of outbound liquidity that can forward an HTLC to maximize the probability of being able to successfully forward a subsequent HTLC.

Fixes#1278.

I also tested a refactoring to reduce duplication by processing all HTLCForwardInfo::AddHTLCs separately first. However, the current approach is a more minimal and clearer change and we can assume that the order of magnitude of pending forwards will be small per channel.

codecov-commenter · 2024-06-14T23:43:35Z

Codecov Report

Attention: Patch coverage is 85.89744% with 22 lines in your changes missing coverage. Please review.

Project coverage is 89.90%. Comparing base (07f3380) to head (8d915a8).

Files	Patch %	Lines
lightning/src/ln/channelmanager.rs	70.76%	14 Missing and 5 partials ⚠️
lightning/src/ln/payment_tests.rs	96.70%	1 Missing and 2 partials ⚠️

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

Additional details and impacted files

@@ Coverage Diff @@## main #3127 +/- ##
==========================================
- Coverage 89.92% 89.90% -0.03% 
==========================================
Files 121 121 Lines 99172 99290 +118 Branches 99172 99290 +118 ==========================================
+ Hits 89180 89263 +83 - Misses 7391 7415 +24 - Partials 2601 2612 +11

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

TheBlueMatt

Thanks!

TheBlueMatt · 2024-06-17T14:29:58Z

+ let mut channels_with_peer = peer_state.channel_by_id.values_mut().filter_map(|phase| match phase {
+ ChannelPhase::Funded(chan) => Some(chan),
+ _ => None,
+ }).collect::<Vec<&mut Channel<_>>>();


Rather than iterate + collect, we should be able to just pick a singular channel we want to use by iterating, filter_map'ing, and min_by'ing using Channel::get_available_balances and selecting the channel that has the lowest next_outbound_htlc_limit_msat above the amount we're trying to send. That would also simplify the patch as we don't have to iterate channels and try to add anymore.

Done. This was mainly done to avoid duplication with the validation in send_htlc. Do we also want to verify the establishment/shutdown state?
rust-lightning/lightning/src/ln/channel.rs
Lines 7049 to 7083 in 737df0f
if !matches!(self.context.channel_state,ChannelState::ChannelReady(_)) ||
self.context.channel_state.is_local_shutdown_sent() ||
self.context.channel_state.is_remote_shutdown_sent()
{
returnErr(ChannelError::Ignore("Cannot send HTLC until channel is fully established and we haven't started shutting down".to_owned()));
}
let channel_total_msat = self.context.channel_value_satoshis*1000;
if amount_msat > channel_total_msat {
returnErr(ChannelError::Ignore(format!("Cannot send amount {}, because it is more than the total value of the channel {}", amount_msat, channel_total_msat)));
}
if amount_msat == 0{
returnErr(ChannelError::Ignore("Cannot send 0-msat HTLC".to_owned()));
}
let available_balances = self.context.get_available_balances(fee_estimator);
if amount_msat < available_balances.next_outbound_htlc_minimum_msat{
returnErr(ChannelError::Ignore(format!("Cannot send less than our next-HTLC minimum - {} msat",
available_balances.next_outbound_htlc_minimum_msat)));
}
if amount_msat > available_balances.next_outbound_htlc_limit_msat{
returnErr(ChannelError::Ignore(format!("Cannot send more than our next-HTLC maximum - {} msat",
available_balances.next_outbound_htlc_limit_msat)));
}
ifself.context.channel_state.is_peer_disconnected(){
// Note that this should never really happen, if we're !is_live() on receipt of an
// incoming HTLC for relay will result in us rejecting the HTLC and we won't allow
// the user to send directly into a !is_live() channel. However, if we
// disconnected during the time the previous hop was doing the commitment dance we may
// end up getting here after the forwarding delay. In any case, returning an
// IgnoreError will get ChannelManager to do the right thing and fail backwards now.
returnErr(ChannelError::Ignore("Cannot send an HTLC while disconnected from channel counterparty".to_owned()));
}

Had to adjust full_stack::tests::test_no_existing_test_breakage to account for the additional fee estimation call in get_available_balances.

Do we also want to verify the establishment/shutdown state?
To check back in here, I did add an is_usable() check which I believe is necessary, but let me know if that's not the case.

tnull

Mostly looks good to me, just some questions.

Feel free to squash the fixup!

This change implements non-strict forwarding, allowing the node to forward an HTLC along a channel other than the one specified by short_channel_id in the onion message, so long as the receiver has the same node public key intended by short_channel_id ([BOLT](https://github.com/lightning/bolts/blob/57ce4b1e05c996fa649f00dc13521f6d496a288f/04-onion-routing.md#non-strict-forwarding)). This can improve payment reliability when there are multiple channels with the same peer e.g. when outbound liquidity is replenished by opening a new channel. The implemented forwarding strategy now chooses the channel with the lowest outbound liquidity that can forward an HTLC to maximize the probability of being able to successfully forward a subsequent HTLC. Fixeslightningdevkit#1278.

wvanlint · 2024-06-20T04:37:18Z

Thanks! Squashed the fixups.

tnull · 2024-06-20T07:29:01Z

Thanks! Squashed the fixups.

Thanks, just a quick note: next time it would help reviewing if you left them up for ~another round of review (i.e., ask reviewers before squashing them). Given that the whole block was re-indented, its now non-trivial to see what really changed with the last force-push.

tnull

LGTM

I'll leave the last ACK to Matt as he requested some of the changes added since he ACKed last.

wvanlint · 2024-06-14T23:26:57Z

This change implements non-strict forwarding, allowing the node to forward an HTLC along a channel other than the one specified by short_channel_id in the onion message, so long as the receiver has the same node public key intended by short_channel_id (BOLT). This can improve payment reliability when there are multiple channels with the same peer e.g. when outbound liquidity is replenished by opening a new channel.

The implemented forwarding strategy now chooses the channel with the least amount of outbound liquidity that can forward an HTLC to maximize the probability of being able to successfully forward a subsequent HTLC.

Fixes#1278.

I also tested a refactoring to reduce duplication by processing all HTLCForwardInfo::AddHTLCs separately first. However, the current approach is a more minimal and clearer change and we can assume that the order of magnitude of pending forwards will be small per channel.

codecov-commenter · 2024-06-14T23:43:35Z

Codecov Report

Attention: Patch coverage is 85.89744% with 22 lines in your changes missing coverage. Please review.

Project coverage is 89.90%. Comparing base (07f3380) to head (8d915a8).

Files	Patch %	Lines
lightning/src/ln/channelmanager.rs	70.76%	14 Missing and 5 partials ⚠️
lightning/src/ln/payment_tests.rs	96.70%	1 Missing and 2 partials ⚠️

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

Additional details and impacted files

@@ Coverage Diff @@## main #3127 +/- ##
==========================================
- Coverage 89.92% 89.90% -0.03% 
==========================================
Files 121 121 Lines 99172 99290 +118 Branches 99172 99290 +118 ==========================================
+ Hits 89180 89263 +83 - Misses 7391 7415 +24 - Partials 2601 2612 +11

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

TheBlueMatt

Thanks!

TheBlueMatt · 2024-06-17T14:29:58Z

+ let mut channels_with_peer = peer_state.channel_by_id.values_mut().filter_map(|phase| match phase {
+ ChannelPhase::Funded(chan) => Some(chan),
+ _ => None,
+ }).collect::<Vec<&mut Channel<_>>>();


Rather than iterate + collect, we should be able to just pick a singular channel we want to use by iterating, filter_map'ing, and min_by'ing using Channel::get_available_balances and selecting the channel that has the lowest next_outbound_htlc_limit_msat above the amount we're trying to send. That would also simplify the patch as we don't have to iterate channels and try to add anymore.

Done. This was mainly done to avoid duplication with the validation in send_htlc. Do we also want to verify the establishment/shutdown state?
rust-lightning/lightning/src/ln/channel.rs
Lines 7049 to 7083 in 737df0f
if !matches!(self.context.channel_state,ChannelState::ChannelReady(_)) ||
self.context.channel_state.is_local_shutdown_sent() ||
self.context.channel_state.is_remote_shutdown_sent()
{
returnErr(ChannelError::Ignore("Cannot send HTLC until channel is fully established and we haven't started shutting down".to_owned()));
}
let channel_total_msat = self.context.channel_value_satoshis*1000;
if amount_msat > channel_total_msat {
returnErr(ChannelError::Ignore(format!("Cannot send amount {}, because it is more than the total value of the channel {}", amount_msat, channel_total_msat)));
}
if amount_msat == 0{
returnErr(ChannelError::Ignore("Cannot send 0-msat HTLC".to_owned()));
}
let available_balances = self.context.get_available_balances(fee_estimator);
if amount_msat < available_balances.next_outbound_htlc_minimum_msat{
returnErr(ChannelError::Ignore(format!("Cannot send less than our next-HTLC minimum - {} msat",
available_balances.next_outbound_htlc_minimum_msat)));
}
if amount_msat > available_balances.next_outbound_htlc_limit_msat{
returnErr(ChannelError::Ignore(format!("Cannot send more than our next-HTLC maximum - {} msat",
available_balances.next_outbound_htlc_limit_msat)));
}
ifself.context.channel_state.is_peer_disconnected(){
// Note that this should never really happen, if we're !is_live() on receipt of an
// incoming HTLC for relay will result in us rejecting the HTLC and we won't allow
// the user to send directly into a !is_live() channel. However, if we
// disconnected during the time the previous hop was doing the commitment dance we may
// end up getting here after the forwarding delay. In any case, returning an
// IgnoreError will get ChannelManager to do the right thing and fail backwards now.
returnErr(ChannelError::Ignore("Cannot send an HTLC while disconnected from channel counterparty".to_owned()));
}

Had to adjust full_stack::tests::test_no_existing_test_breakage to account for the additional fee estimation call in get_available_balances.

Do we also want to verify the establishment/shutdown state?
To check back in here, I did add an is_usable() check which I believe is necessary, but let me know if that's not the case.

tnull

Mostly looks good to me, just some questions.

Feel free to squash the fixup!

This change implements non-strict forwarding, allowing the node to forward an HTLC along a channel other than the one specified by short_channel_id in the onion message, so long as the receiver has the same node public key intended by short_channel_id ([BOLT](https://github.com/lightning/bolts/blob/57ce4b1e05c996fa649f00dc13521f6d496a288f/04-onion-routing.md#non-strict-forwarding)). This can improve payment reliability when there are multiple channels with the same peer e.g. when outbound liquidity is replenished by opening a new channel. The implemented forwarding strategy now chooses the channel with the lowest outbound liquidity that can forward an HTLC to maximize the probability of being able to successfully forward a subsequent HTLC. Fixeslightningdevkit#1278.

wvanlint · 2024-06-20T04:37:18Z

Thanks! Squashed the fixups.

tnull · 2024-06-20T07:29:01Z

Thanks! Squashed the fixups.

Thanks, just a quick note: next time it would help reviewing if you left them up for ~another round of review (i.e., ask reviewers before squashing them). Given that the whole block was re-indented, its now non-trivial to see what really changed with the last force-push.

tnull

LGTM

I'll leave the last ACK to Matt as he requested some of the changes added since he ACKed last.

wvanlint force-pushed the non_strict_forwarding branch from bbd46e5 to 694ed23Compare June 14, 2024 23:32

wvanlint force-pushed the non_strict_forwarding branch 5 times, most recently from fa5ec86 to 8d915a8Compare June 17, 2024 02:02

wvanlint marked this pull request as ready for review June 17, 2024 02:04

TheBlueMatt reviewed Jun 17, 2024
View reviewed changes

tnull requested review from tnull June 17, 2024 20:42

wvanlint force-pushed the non_strict_forwarding branch 2 times, most recently from 6c7a6b2 to ca19602Compare June 18, 2024 01:35

TheBlueMatt previously approved these changes Jun 18, 2024
View reviewed changes

TheBlueMatt added the Seeking Code Review label Jun 18, 2024

tnull reviewed Jun 19, 2024
View reviewed changes

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

tnull removed the Seeking Code Review label Jun 19, 2024

wvanlint dismissed TheBlueMatt’s stale review via 05e6252June 20, 2024 04:24

wvanlint force-pushed the non_strict_forwarding branch from ca19602 to 05e6252Compare June 20, 2024 04:24

tnull approved these changes Jun 20, 2024
View reviewed changes

TheBlueMatt approved these changes Jun 20, 2024
View reviewed changes

TheBlueMatt merged commit 3828516 into lightningdevkit:mainJun 20, 2024

wvanlint deleted the non_strict_forwarding branch October 17, 2024 22:08

tnull mentioned this pull request Jun 2, 2025
Allow non-strict forwarding for spliced/closed channels #3821
Open