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.

Sign up for GitHub

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

Jump to bottom

Introduce RouteParametersConfig #3342

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Introduce RouteParametersConfig #3342

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
69c3499
Introduce RouteParametersConfig
shaavan Sep 25, 2024
583d686
Update `AwaitingInvoice` to include `RouteParametersConfig`
shaavan Sep 25, 2024
db71b52
f: Retain max_total_routing_fee_msat value
shaavan Oct 22, 2024
6ccb7b6
Extend pay_for_offer to accept RouteParametersOverride
shaavan Oct 13, 2024
2e60588
Extend create_refund_builder to accept RouteParametersOverride
shaavan Oct 22, 2024
a5cd262
Use RouteParametersConfig to create new RouteParameters
shaavan Oct 13, 2024
4221929
f: fix comment
shaavan Oct 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 12 additions & 9 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ use crate::ln::channel_state::ChannelDetails;
use crate::types::features::{Bolt12InvoiceFeatures, ChannelFeatures, ChannelTypeFeatures, InitFeatures, NodeFeatures};
#[cfg(any(feature = "_test_utils", test))]
use crate::types::features::Bolt11InvoiceFeatures;
use crate::routing::router::{BlindedTail, InFlightHtlcs, Path, Payee, PaymentParameters, Route, RouteParameters, Router};
use crate::routing::router::{BlindedTail, InFlightHtlcs, Path, Payee, PaymentParameters, Route, RouteParameters, RouteParametersConfig, Router};
use crate::ln::onion_payment::{check_incoming_htlc_cltv, create_recv_pending_htlc_info, create_fwd_pending_htlc_info, decode_incoming_update_add_htlc_onion, InboundHTLCErr, NextPacketDetails};
use crate::ln::msgs;
use crate::ln::onion_utils;
Expand Down Expand Up @@ -1847,6 +1847,7 @@ where
/// # use lightning::events::{Event, EventsProvider, PaymentPurpose};
/// # use lightning::ln::channelmanager::AChannelManager;
/// # use lightning::offers::parse::Bolt12SemanticError;
/// # use lightning::routing::router::RouteParametersConfig;
/// #
/// # fn example<T: AChannelManager>(channel_manager: T) -> Result<(), Bolt12SemanticError> {
/// # let channel_manager = channel_manager.get_cm();
Expand Down Expand Up @@ -1894,15 +1895,16 @@ where
/// # use lightning::events::{Event, EventsProvider};
/// # use lightning::ln::channelmanager::{AChannelManager, PaymentId, RecentPaymentDetails, Retry};
/// # use lightning::offers::offer::Offer;
/// # use lightning::routing::router::RouteParametersConfig;
/// #
/// # fn example<T: AChannelManager>(
/// # channel_manager: T, offer: &Offer, quantity: Option<u64>, amount_msats: Option<u64>,
/// # payer_note: Option<String>, retry: Retry, max_total_routing_fee_msat: Option<u64>
/// # payer_note: Option<String>, retry: Retry, route_params_config: Option<RouteParametersConfig>
/// # ) {
/// # let channel_manager = channel_manager.get_cm();
/// let payment_id = PaymentId([42; 32]);
/// match channel_manager.pay_for_offer(
/// offer, quantity, amount_msats, payer_note, payment_id, retry, max_total_routing_fee_msat
/// offer, quantity, amount_msats, payer_note, payment_id, retry, route_params_config
/// ) {
/// Ok(()) => println!("Requesting invoice for offer"),
/// Err(e) => println!("Unable to request invoice for offer: {:?}", e),
Expand Down Expand Up @@ -1950,16 +1952,17 @@ where
/// # use lightning::events::{Event, EventsProvider};
/// # use lightning::ln::channelmanager::{AChannelManager, PaymentId, RecentPaymentDetails, Retry};
/// # use lightning::offers::parse::Bolt12SemanticError;
/// # use lightning::routing::router::RouteParametersConfig;
/// #
/// # fn example<T: AChannelManager>(
/// # channel_manager: T, amount_msats: u64, absolute_expiry: Duration, retry: Retry,
/// # max_total_routing_fee_msat: Option<u64>
/// # route_params_config: Option<RouteParametersConfig>
/// # ) -> Result<(), Bolt12SemanticError> {
/// # let channel_manager = channel_manager.get_cm();
/// let payment_id = PaymentId([42; 32]);
/// let refund = channel_manager
/// .create_refund_builder(
/// amount_msats, absolute_expiry, payment_id, retry, max_total_routing_fee_msat
/// amount_msats, absolute_expiry, payment_id, retry, route_params_config
/// )?
/// # ;
/// # // Needed for compiling for c_bindings
Expand Down Expand Up @@ -9187,7 +9190,7 @@ macro_rules! create_refund_builder { ($self: ident, $builder: ty) => {
/// [Avoiding Duplicate Payments]: #avoiding-duplicate-payments
pub fn create_refund_builder(
&$self, amount_msats: u64, absolute_expiry: Duration, payment_id: PaymentId,
retry_strategy: Retry, max_total_routing_fee_msat: Option<u64>
retry_strategy: Retry, route_params_config: Option<RouteParametersConfig>
) -> Result<$builder, Bolt12SemanticError> {
let node_id = $self.get_our_node_id();
let expanded_key = &$self.inbound_payment_key;
Expand All @@ -9212,7 +9215,7 @@ macro_rules! create_refund_builder { ($self: ident, $builder: ty) => {
let expiration = StaleExpiration::AbsoluteTimeout(absolute_expiry);
$self.pending_outbound_payments
.add_new_awaiting_invoice(
payment_id, expiration, retry_strategy, max_total_routing_fee_msat, None,
payment_id, expiration, retry_strategy, route_params_config, None,
)
.map_err(|_| Bolt12SemanticError::DuplicatePaymentId)?;

Expand Down Expand Up @@ -9305,7 +9308,7 @@ where
pub fn pay_for_offer(
&self, offer: &Offer, quantity: Option<u64>, amount_msats: Option<u64>,
payer_note: Option<String>, payment_id: PaymentId, retry_strategy: Retry,
max_total_routing_fee_msat: Option<u64>
route_params_config: Option<RouteParametersConfig>
) -> Result<(), Bolt12SemanticError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

OK no needed option here but: what do you think about having the function call like pay_for_offer(offer, OfferParams::default()) where OfferParams is something like that

struct OfferParams {
    quantity: Option<u64>,
    amount_msats: Option<u64>,
    manual_routing_params: Option<ManualRoutingParameters>,
}

I think is a lot of more verbose in this way, but I also this that this will be a lot of cleaner.

Now I do not think this will fit well in this PR maybe can be a followup one, but with this patter, it is possible to hide future updates to offers (e.g: recurrence or market place feature) without breaking too much the code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is a great improvement! It will help keep the function signature cleaner while remaining maintainable for future parameter increases.

Maybe we can take this in a follow-up. @TheBlueMatt, what do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No strong opinions.

let expanded_key = &self.inbound_payment_key;
let entropy = &*self.entropy_source;
Expand Down Expand Up @@ -9347,7 +9350,7 @@ where
};
self.pending_outbound_payments
.add_new_awaiting_invoice(
payment_id, expiration, retry_strategy, max_total_routing_fee_msat,
payment_id, expiration, retry_strategy, route_params_config,
Some(retryable_invoice_request)
)
.map_err(|_| Bolt12SemanticError::DuplicatePaymentId)?;
Expand Down
72 changes: 54 additions & 18 deletions lightning/src/ln/outbound_payment.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ use crate::ln::onion_utils::{DecodedOnionFailure, HTLCFailReason};
use crate::offers::invoice::Bolt12Invoice;
use crate::offers::invoice_request::InvoiceRequest;
use crate::offers::nonce::Nonce;
use crate::routing::router::{BlindedTail, InFlightHtlcs, Path, PaymentParameters, Route, RouteParameters, Router};
use crate::routing::router::{BlindedTail, InFlightHtlcs, RouteParametersConfig, Path, PaymentParameters, Route, RouteParameters, Router};
use crate::sign::{EntropySource, NodeSigner, Recipient};
use crate::util::errors::APIError;
use crate::util::logger::Logger;
Expand Down Expand Up @@ -61,7 +61,11 @@ pub(crate) enum PendingOutboundPayment {
AwaitingInvoice {
expiration: StaleExpiration,
retry_strategy: Retry,
// Deprecated: Retained for backward compatibility.
// If set during read, this field overrides `RouteParameters::max_total_routing_fee_msat`
// instead of `RouteParametersConfig::max_total_routing_fee_msat`.
max_total_routing_fee_msat: Option<u64>,
route_params_config: RouteParametersConfig,
retryable_invoice_request: Option<RetryableInvoiceRequest>
},
// This state will never be persisted to disk because we transition from `AwaitingInvoice` to
Expand All @@ -70,9 +74,12 @@ pub(crate) enum PendingOutboundPayment {
InvoiceReceived {
payment_hash: PaymentHash,
retry_strategy: Retry,
// Note this field is currently just replicated from AwaitingInvoice but not actually
// used anywhere.
// Deprecated: Retained for backward compatibility.
max_total_routing_fee_msat: Option<u64>,
// Currently unused, but replicated from `AwaitingInvoice` to avoid potential
// race conditions where this field might be missing upon reload. It may be required
// for future retries.
route_params_config: RouteParametersConfig,
},
// This state applies when we are paying an often-offline recipient and another node on the
// network served us a static invoice on the recipient's behalf in response to our invoice
Expand Down Expand Up @@ -844,19 +851,26 @@ impl OutboundPayments {
SP: Fn(SendAlongPathArgs) -> Result<(), APIError>,
{
let payment_hash = invoice.payment_hash();
let max_total_routing_fee_msat;
let params_config;
let retry_strategy;
match self.pending_outbound_payments.lock().unwrap().entry(payment_id) {
hash_map::Entry::Occupied(entry) => match entry.get() {
PendingOutboundPayment::AwaitingInvoice {
retry_strategy: retry, max_total_routing_fee_msat: max_total_fee, ..
retry_strategy: retry, max_total_routing_fee_msat: max_total_fee, route_params_config, ..
} => {
retry_strategy = *retry;
max_total_routing_fee_msat = *max_total_fee;
// If max_total_fee is present, update route_params_config with the specified fee.
// This supports the standard behavior during upgrades.
let route_params_config = max_total_fee.map_or(
*route_params_config,
|fee_msat| route_params_config.with_max_total_routing_fee_msat(fee_msat),
);
params_config = route_params_config;
*entry.into_mut() = PendingOutboundPayment::InvoiceReceived {
payment_hash,
retry_strategy: *retry,
max_total_routing_fee_msat,
max_total_routing_fee_msat: *max_total_fee,
route_params_config,
};
},
_ => return Err(Bolt12PaymentError::DuplicateInvoice),
Expand All @@ -872,11 +886,13 @@ impl OutboundPayments {
}

let mut route_params = RouteParameters::from_payment_params_and_value(
PaymentParameters::from_bolt12_invoice(&invoice), invoice.amount_msats()
PaymentParameters::from_bolt12_invoice(&invoice).with_user_config(params_config), invoice.amount_msats()
);
if let Some(max_fee_msat) = max_total_routing_fee_msat {
route_params.max_total_routing_fee_msat = Some(max_fee_msat);

if let Some(fee_msat) = params_config.max_total_routing_fee_msat {
route_params.max_total_routing_fee_msat = Some(fee_msat);
}

self.send_payment_for_bolt12_invoice_internal(
payment_id, payment_hash, None, route_params, retry_strategy, router, first_hops,
inflight_htlcs, entropy_source, node_signer, node_id_lookup, secp_ctx, best_block_height,
Expand Down Expand Up @@ -1004,7 +1020,7 @@ impl OutboundPayments {
match self.pending_outbound_payments.lock().unwrap().entry(payment_id) {
hash_map::Entry::Occupied(mut entry) => match entry.get() {
PendingOutboundPayment::AwaitingInvoice {
retry_strategy, retryable_invoice_request, max_total_routing_fee_msat, ..
retry_strategy, retryable_invoice_request, max_total_routing_fee_msat, route_params_config, ..
} => {
let invreq = &retryable_invoice_request
.as_ref()
Expand All @@ -1027,11 +1043,22 @@ impl OutboundPayments {
return Err(Bolt12PaymentError::UnknownRequiredFeatures)
}
};

// If max_total_fee is present, update route_params_config with the specified fee.
// This supports the standard behavior during downgrades.
let params_config = max_total_routing_fee_msat.map_or(
*route_params_config,
|fee_msat| route_params_config.with_max_total_routing_fee_msat(fee_msat),
);

let keysend_preimage = PaymentPreimage(entropy_source.get_secure_random_bytes());
let payment_hash = PaymentHash(Sha256::hash(&keysend_preimage.0).to_byte_array());
let pay_params = PaymentParameters::from_static_invoice(invoice);
let pay_params = PaymentParameters::from_static_invoice(invoice).with_user_config(params_config);
let mut route_params = RouteParameters::from_payment_params_and_value(pay_params, amount_msat);
route_params.max_total_routing_fee_msat = *max_total_routing_fee_msat;

if let Some(fee_msat) = params_config.max_total_routing_fee_msat {
route_params.max_total_routing_fee_msat = Some(fee_msat);
}

if let Err(()) = onion_utils::set_max_path_length(
&mut route_params, &RecipientOnionFields::spontaneous_empty(), Some(keysend_preimage),
Expand Down Expand Up @@ -1596,9 +1623,10 @@ impl OutboundPayments {

pub(super) fn add_new_awaiting_invoice(
&self, payment_id: PaymentId, expiration: StaleExpiration, retry_strategy: Retry,
max_total_routing_fee_msat: Option<u64>, retryable_invoice_request: Option<RetryableInvoiceRequest>
route_params_config: Option<RouteParametersConfig>, retryable_invoice_request: Option<RetryableInvoiceRequest>
) -> Result<(), ()> {
let mut pending_outbounds = self.pending_outbound_payments.lock().unwrap();
let route_params_config = route_params_config.unwrap_or(RouteParametersConfig::new());
shaavan marked this conversation as resolved.
Show resolved Hide resolved
match pending_outbounds.entry(payment_id) {
hash_map::Entry::Occupied(_) => Err(()),
hash_map::Entry::Vacant(entry) => {
Expand All @@ -1608,7 +1636,9 @@ impl OutboundPayments {
entry.insert(PendingOutboundPayment::AwaitingInvoice {
expiration,
retry_strategy,
max_total_routing_fee_msat,
route_params_config,
// Retained for downgrade support.
max_total_routing_fee_msat: route_params_config.max_total_routing_fee_msat,
retryable_invoice_request,
});

Expand Down Expand Up @@ -2240,10 +2270,12 @@ impl_writeable_tlv_based_enum_upgradable!(PendingOutboundPayment,
(2, retry_strategy, required),
(4, max_total_routing_fee_msat, option),
shaavan marked this conversation as resolved.
Show resolved Hide resolved
(5, retryable_invoice_request, option),
(7, route_params_config, (default_value, RouteParametersConfig::new())),
},
(7, InvoiceReceived) => {
(0, payment_hash, required),
(2, retry_strategy, required),
(3, route_params_config, (default_value, RouteParametersConfig::new())),
(4, max_total_routing_fee_msat, option),
shaavan marked this conversation as resolved.
Show resolved Hide resolved
},
// Added in 0.0.125. Prior versions will drop these outbounds on downgrade, which is safe because
Expand Down Expand Up @@ -2275,7 +2307,7 @@ mod tests {
use crate::offers::offer::OfferBuilder;
use crate::offers::test_utils::*;
use crate::routing::gossip::NetworkGraph;
use crate::routing::router::{InFlightHtlcs, Path, PaymentParameters, Route, RouteHop, RouteParameters};
use crate::routing::router::{InFlightHtlcs, Path, PaymentParameters, Route, RouteHop, RouteParameters, RouteParametersConfig};
use crate::sync::{Arc, Mutex, RwLock};
use crate::util::errors::APIError;
use crate::util::hash_tables::new_hash_map;
Expand Down Expand Up @@ -2689,10 +2721,12 @@ mod tests {
.build().unwrap()
.sign(recipient_sign).unwrap();

let route_params_config = RouteParametersConfig::new().with_max_total_routing_fee_msat(invoice.amount_msats() / 100 + 50_000);

assert!(
outbound_payments.add_new_awaiting_invoice(
payment_id, expiration, Retry::Attempts(0),
Some(invoice.amount_msats() / 100 + 50_000), None,
Some(route_params_config), None,
).is_ok()
);
assert!(outbound_payments.has_pending_payments());
Expand Down Expand Up @@ -2790,9 +2824,11 @@ mod tests {
assert!(!outbound_payments.has_pending_payments());
assert!(pending_events.lock().unwrap().is_empty());

let route_params_config = RouteParametersConfig::new().with_max_total_routing_fee_msat(1234);

assert!(
outbound_payments.add_new_awaiting_invoice(
payment_id, expiration, Retry::Attempts(0), Some(1234), None,
payment_id, expiration, Retry::Attempts(0), Some(route_params_config), None,
).is_ok()
);
assert!(outbound_payments.has_pending_payments());
Expand Down
95 changes: 95 additions & 0 deletions lightning/src/routing/router.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -875,6 +875,16 @@ impl PaymentParameters {
}
}

/// Update the parameters with the configuration provided by user.
pub fn with_user_config(self, params_config: RouteParametersConfig) -> Self {
Self {
max_total_cltv_expiry_delta: params_config.max_total_cltv_expiry_delta,
max_path_count: params_config.max_path_count,
max_channel_saturation_power_of_half: params_config.max_channel_saturation_power_of_half,
.. self
}
}

/// Includes the payee's features. Errors if the parameters were not initialized with
/// [`PaymentParameters::from_bolt12_invoice`].
///
Expand Down Expand Up @@ -962,6 +972,91 @@ impl PaymentParameters {
}
}

/// A struct for configuring parameters for routing the payment.
#[derive(Clone, Copy)]
pub struct RouteParametersConfig {
/// The maximum total fees, in millisatoshi, that may accrue during route finding.
///
/// This limit also applies to the total fees that may arise while retrying failed payment
/// paths.
///
/// Note that values below a few sats may result in some paths being spuriously ignored.
///
/// Defaults to 1% of the payment amount + 50 sats
pub max_total_routing_fee_msat: Option<u64>,
shaavan marked this conversation as resolved.
Show resolved Hide resolved

/// The maximum total CLTV delta we accept for the route.
/// Defaults to [`DEFAULT_MAX_TOTAL_CLTV_EXPIRY_DELTA`].
pub max_total_cltv_expiry_delta: u32,

/// The maximum number of paths that may be used by (MPP) payments.
/// Defaults to [`DEFAULT_MAX_PATH_COUNT`].
pub max_path_count: u8,

/// Selects the maximum share of a channel's total capacity which will be sent over a channel,
/// as a power of 1/2. A higher value prefers to send the payment using more MPP parts whereas
/// a lower value prefers to send larger MPP parts, potentially saturating channels and
/// increasing failure probability for those paths.
///
/// Note that this restriction will be relaxed during pathfinding after paths which meet this
/// restriction have been found. While paths which meet this criteria will be searched for, it
/// is ultimately up to the scorer to select them over other paths.
///
/// A value of 0 will allow payments up to and including a channel's total announced usable
/// capacity, a value of one will only use up to half its capacity, two 1/4, etc.
///
/// Default value: 2
pub max_channel_saturation_power_of_half: u8,
}

impl_writeable_tlv_based!(RouteParametersConfig, {
(1, max_total_routing_fee_msat, option),
(3, max_total_cltv_expiry_delta, required),
(5, max_path_count, required),
(7, max_channel_saturation_power_of_half, required),
});

impl RouteParametersConfig {
/// Initates an new set of route parameter configs with default parameters.
pub fn new() -> Self {
shaavan marked this conversation as resolved.
Show resolved Hide resolved
Self {
max_total_routing_fee_msat: None,
max_total_cltv_expiry_delta: DEFAULT_MAX_TOTAL_CLTV_EXPIRY_DELTA,
max_path_count: DEFAULT_MAX_PATH_COUNT,
max_channel_saturation_power_of_half: DEFAULT_MAX_CHANNEL_SATURATION_POW_HALF,
}
}

/// Set the maximum total fees, in millisatoshi, that may accrue during route finding.
///
/// This is not exported to bindings users since bindings don't support move semantics
pub fn with_max_total_routing_fee_msat(self, fee_msat: u64) -> Self {
Self { max_total_routing_fee_msat: Some(fee_msat), ..self }
}

/// Includes a limit for the total CLTV expiry delta which is considered during routing
///
/// This is not exported to bindings users since bindings don't support move semantics
pub fn with_max_total_cltv_expiry_delta(self, max_total_cltv_expiry_delta: u32) -> Self {
Self { max_total_cltv_expiry_delta, ..self }
}

/// Includes a limit for the maximum number of payment paths that may be used.
///
/// This is not exported to bindings users since bindings don't support move semantics
pub fn with_max_path_count(self, max_path_count: u8) -> Self {
Self { max_path_count, ..self }
}

/// Includes a limit for the maximum share of a channel's total capacity that can be sent over, as
/// a power of 1/2. See [`PaymentParameters::max_channel_saturation_power_of_half`].
///
/// This is not exported to bindings users since bindings don't support move semantics
pub fn with_max_channel_saturation_power_of_half(self, max_channel_saturation_power_of_half: u8) -> Self {
Self { max_channel_saturation_power_of_half, ..self }
}
}

/// The recipient of a payment, differing based on whether they've hidden their identity with route
/// blinding.
#[derive(Clone, Debug, Hash, PartialEq, Eq)]
Expand Down
Loading