-
Notifications
You must be signed in to change notification settings - Fork 384
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
Implement non-strict forwarding #3127
Conversation
bbd46e5
to
694ed23
Compare
Codecov ReportAttention: Patch coverage is
❗ 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. |
fa5ec86
to
8d915a8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
lightning/src/ln/channelmanager.rs
Outdated
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<_>>>(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
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() | |
{ | |
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())); | |
} |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
6c7a6b2
to
ca19602
Compare
There was a problem hiding this 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!
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.
ca19602
to
05e6252
Compare
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. |
There was a problem hiding this 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.
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::AddHTLC
s 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.