Add helpers for circular rebalancing#4616
Conversation
|
👋 I see @wpaulino was un-assigned. |
Review SummaryNew Issues FoundInline comment posted:
Prior Comments StatusThe issues from the previous review pass appear to have been addressed:
The prior comments about doc quality on Cross-cutting notes
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4616 +/- ##
==========================================
+ Coverage 86.11% 86.43% +0.32%
==========================================
Files 157 158 +1
Lines 108841 109420 +579
Branches 108841 109420 +579
==========================================
+ Hits 93725 94578 +853
+ Misses 12497 12299 -198
+ Partials 2619 2543 -76
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…_route' to ChannelManager Signed-off-by: ABHAY PANDEY <pandeyabhay967@gmail.com>
| let forwarding_fee = forwarding_info.fee_base_msat as u64 | ||
| + (forwarding_info.fee_proportional_millionths as u64 * amount_msat) / 1000000; |
There was a problem hiding this comment.
Nit/correctness: The multiplication fee_proportional_millionths as u64 * amount_msat can overflow u64 for extreme (but valid) values of the two operands. The channel forwarding code at channel.rs:11335 uses checked_mul for the same fee calculation:
let fee = amt_to_forward.checked_mul(config.forwarding_fee_proportional_millionths as u64)
.and_then(|prop_fee| (prop_fee / 1000000).checked_add(config.forwarding_fee_base_msat as u64));Consider using checked arithmetic here to be consistent and avoid a wrapping fee (which would cause a confusing async payment failure):
| let forwarding_fee = forwarding_info.fee_base_msat as u64 | |
| + (forwarding_info.fee_proportional_millionths as u64 * amount_msat) / 1000000; | |
| let forwarding_fee = (forwarding_info.fee_proportional_millionths as u64) | |
| .checked_mul(amount_msat) | |
| .and_then(|prop_fee| (prop_fee / 1_000_000) | |
| .checked_add(forwarding_info.fee_base_msat as u64)) | |
| .ok_or(RetryableSendFailure::RouteNotFound)?; |
|
🔔 1st Reminder Hey @wpaulino! This PR has been waiting for your review. |
Ferryx349
changed the title
| /// [`Route`]: crate::routing::router::Route | ||
| /// [`Event::PaymentFailed`]: crate::events::Event::PaymentFailed | ||
| pub fn send_circular_payment( | ||
| &self, outbound_channel_id: ChannelId, inbound_channel_id: ChannelId, amount_msat: u64, |
There was a problem hiding this comment.
Should the inbound/outbound edges be a list?
| prev_last.fee_msat = forwarding_fee; | ||
| prev_last.cltv_expiry_delta = cltv_expiry_delta; | ||
| } | ||
| path.hops.push(last_hop.clone()); |
There was a problem hiding this comment.
It might be marginally simpler to do a find_route to a dummy recipient pubkey with the last-hop pre-filled in the route params so we don't have to manually build a last-hop and push.
| }; | ||
| for path in route.paths.iter_mut() { | ||
| if let Some(prev_last) = path.hops.last_mut() { | ||
| prev_last.fee_msat = forwarding_fee; |
There was a problem hiding this comment.
This may break the path cause we have to compute an additional proportional fee on previous hops.
| /// LDK will not automatically retry this payment, though it may be manually | ||
| /// re-sent after an | ||
| /// [`Event::PaymentFailed`] is generated. | ||
| pub fn send_spontaneous_payment_with_route( |
There was a problem hiding this comment.
Adding this method is fine, but I'm not actually sure we want the rebalance "payment" to be a spontaneous payment - it maybe should be its own class of payment and generate a different set of events (or, maybe, if we don't want yet more events, at least result in a flag in the payment claimed/sent events being set that marks it as a rebalance payment).
4 participants
Fixes- #3791
This PR adds two new methods to
ChannelManager:send_circular_payment
A helper for circular channel rebalancing. It:
find_routetargeting the inbound channel's counterparty, withfirst_hopsrestricted to only the outbound channel to force the router to use it as the first hop.RouteHopback to our own node over the inbound channel's SCID, withfee_msatset to the full payment amount as required for the last hop and the preceding hop'sfee_msatadjusted to counterparty's forwarding fee.send_spontaneous_payment_with_route.The existing guard in
pay_route_internalalready permits our node as the last hop in a path ("simple rebalance loop to us"), so no changes to the payment sending internals were needed.Tests
test_circular_payment_rebalance: covers the happy path (0→1→2→0), same-channel error, and channel-not-found error.test_circular_payment_no_route: verifiesRouteNotFoundis returned when no path exists between the two counterparties.This is my first contribution to LDK. I tried to follow the patterns established by
send_payment_with_routeandsend_spontaneous_preflight_probesas closely as possible. Happy to revise anything that doesn't match project conventions.CC: @tnull