Add helpers for circular rebalancing#4616
Conversation
|
👋 I see @wpaulino was un-assigned. |
Review SummaryMost prior review issues have been addressed in this revision:
Still Open from Prior Review
No New Issues FoundAfter thorough analysis of the entire diff — including the route construction, CLTV/fee propagation through the route hint, onion payload construction, receiver-side keysend processing, |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4616 +/- ##
==========================================
+ Coverage 86.15% 86.58% +0.42%
==========================================
Files 158 159 +1
Lines 109323 110515 +1192
Branches 109323 110515 +1192
==========================================
+ Hits 94189 95686 +1497
+ Misses 12518 12291 -227
+ Partials 2616 2538 -78
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:
|
| 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?
There was a problem hiding this comment.
Kept them as single channels since the implementation is single-path only for now. Happy to change outbound to a list, straightforward with first_hops, but inbound as a list needs more thought on distributing paths across channels...
| 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.
There was a problem hiding this comment.
I've refactored this in latest commit
| }; | ||
| 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).
There was a problem hiding this comment.
makes sense, can add a variant to make it distinguishable, or a boolean flag to existing events, which do you prefer?
Ferryx349
force-pushed
the
cir-reb
branch
2 times, most recently
from
db9e7b7 to
a02b75f
Compare
…_route' to ChannelManager Signed-off-by: ABHAY PANDEY <pandeyabhay967@gmail.com>
| }]); | ||
|
|
||
| let route_params = RouteParameters::from_payment_params_and_value( | ||
| PaymentParameters::from_node_id(dummy_payee, MIN_FINAL_CLTV_EXPIRY_DELTA as u32) |
There was a problem hiding this comment.
The doc at line 6182 states "Only single-path routing (no MPP support) is currently available", but PaymentParameters::from_node_id defaults max_path_count to DEFAULT_MAX_PATH_COUNT (10). If the router finds multiple independent routes between the outbound and inbound counterparties, it could split the payment into multiple paths. Since spontaneous_empty has no payment_secret, MPP aggregation on the receiver side won't work — each HTLC arrives as a separate spontaneous payment.
This is unlikely in practice (single first_hop + single route hint constrains the router), but for defense-in-depth the code should enforce what the doc promises:
| PaymentParameters::from_node_id(dummy_payee, MIN_FINAL_CLTV_EXPIRY_DELTA as u32) | |
| let mut payment_params = PaymentParameters::from_node_id(dummy_payee, MIN_FINAL_CLTV_EXPIRY_DELTA as u32) | |
| .with_route_hints(vec![route_hint]) | |
| .map_err(|_| RetryableSendFailure::RouteNotFound)?; | |
| payment_params.max_path_count = 1; | |
| let route_params = RouteParameters::from_payment_params_and_value( | |
| payment_params, |
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