Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement non-strict forwarding #3127

Merged
merged 1 commit into from
Jun 20, 2024

Conversation

wvanlint
Copy link
Contributor

@wvanlint wvanlint commented Jun 14, 2024

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
Copy link

codecov-commenter commented Jun 14, 2024

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.

@wvanlint wvanlint force-pushed the non_strict_forwarding branch 5 times, most recently from fa5ec86 to 8d915a8 Compare June 17, 2024 02:02
@wvanlint wvanlint marked this pull request as ready for review June 17, 2024 02:04
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Thanks!

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<_>>>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. This was mainly done to avoid duplication with the validation in send_htlc. Do we also want to verify the establishment/shutdown state?

if !matches!(self.context.channel_state, ChannelState::ChannelReady(_)) ||
self.context.channel_state.is_local_shutdown_sent() ||
self.context.channel_state.is_remote_shutdown_sent()
{
return Err(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 {
return Err(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 {
return Err(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 {
return Err(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 {
return Err(ChannelError::Ignore(format!("Cannot send more than our next-HTLC maximum - {} msat",
available_balances.next_outbound_htlc_limit_msat)));
}
if self.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.
return Err(ChannelError::Ignore("Cannot send an HTLC while disconnected from channel counterparty".to_owned()));
}

Copy link
Contributor Author

@wvanlint wvanlint Jun 18, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@wvanlint wvanlint Jun 20, 2024

Choose a reason for hiding this comment

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

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 tnull requested review from tnull June 17, 2024 20:42
@wvanlint wvanlint force-pushed the non_strict_forwarding branch 2 times, most recently from 6c7a6b2 to ca19602 Compare June 18, 2024 01:35
TheBlueMatt
TheBlueMatt previously approved these changes Jun 18, 2024
Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

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

Mostly looks good to me, just some questions.

Feel free to squash the fixup!

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
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.

Fixes lightningdevkit#1278.
@wvanlint
Copy link
Contributor Author

Thanks! Squashed the fixups.

@tnull
Copy link
Contributor

tnull commented Jun 20, 2024

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.

Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

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

LGTM

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

@TheBlueMatt TheBlueMatt merged commit 3828516 into lightningdevkit:main Jun 20, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use substitude channel for forwarding if we have another with the same peer
4 participants