Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
72 changes: 65 additions & 7 deletions src/event.rs
Original file line number Diff line number Diff line change
Expand Up @@ -707,15 +707,24 @@ where
}
}

fn lsps2_max_total_opening_fee_msat(payment_metadata: &[u8], amount_msat: u64) -> Option<u64> {
fn lsps2_max_total_opening_fee_msat(
payment_metadata: &[u8], gross_amount_msat: u64,
) -> Option<u64> {
let metadata = PaymentMetadata::read(&mut &payment_metadata[..]).ok()?;
let lsps2_parameters = metadata.lsps2_parameters?;
lsps2_parameters.max_total_opening_fee_msat.or_else(|| {
lsps2_parameters.max_proportional_opening_fee_ppm_msat.and_then(|max_prop_fee| {
// If it's a variable amount payment, compute the actual fee.
compute_opening_fee(amount_msat, 0, max_prop_fee)
lsps2_parameters
.max_total_opening_fee_msat
.or_else(|| {
lsps2_parameters.max_proportional_opening_fee_ppm_msat.and_then(|max_prop_fee| {
// If it's a variable amount payment, compute the actual fee.
compute_opening_fee(
gross_amount_msat,
lsps2_parameters.min_opening_fee_msat,
max_prop_fee,
)
})
})
})
.filter(|fee| *fee <= gross_amount_msat)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Claude:

This filter also applies to max_total_opening_fee_msat, but no test exercises that path; lsps2_payment_metadata_decodes_total_fee_limit only passes a gross amount above the limit. Worth a case with the gross amount below 42_000 returning None.

}

async fn resolve_inbound_payment_id(
Expand Down Expand Up @@ -1058,7 +1067,9 @@ where
.as_ref()
.and_then(|fields| fields.payment_metadata.as_ref())
.and_then(|metadata| {
Self::lsps2_max_total_opening_fee_msat(metadata, amount_msat)
let gross_amount_msat =
amount_msat.checked_add(counterparty_skimmed_fee_msat)?;
Self::lsps2_max_total_opening_fee_msat(metadata, gross_amount_msat)
}),
_ => None,
};
Expand Down Expand Up @@ -2803,6 +2814,7 @@ mod tests {
lsps2_parameters: Some(LSPS2Parameters {
max_total_opening_fee_msat: Some(42_000),
max_proportional_opening_fee_ppm_msat: None,
min_opening_fee_msat: 1_000,
}),
};

Expand All @@ -2815,13 +2827,59 @@ mod tests {
);
}

#[test]
fn lsps2_payment_metadata_honors_minimum_fee() {
// Variable-amount metadata with 10,000 ppm (type 2) and a 1,000-msat minimum
// (type 3). Use the wire format so this also exercises readers predating the minimum.
let metadata =
crate::hex_utils::to_vec("1700151402080000000000002710030800000000000003e8").unwrap();
Comment on lines +2825 to +2828

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not sure I follow the comment. How is this exercising readers predating the minimum?

for (amount_msat, expected_fee_msat) in
[(50_000, 1_000), (100_000, 1_000), (100_001, 1_001), (200_000, 2_000)]
{
assert_eq!(
EventHandler::<Arc<TestLogger>>::lsps2_max_total_opening_fee_msat(
&metadata,
amount_msat,
),
Some(expected_fee_msat),
"The fee allowance must include the negotiated minimum and round up proportional fees"
);
}
}

#[test]
fn lsps2_payment_metadata_bounds_variable_fees_by_gross_amount() {
for (gross_amount_msat, proportional, expected) in [
(100_000, 600_000, Some(60_000)),
(100_000, 1_000_000, Some(100_000)),
(100_000, 1_000_001, None),
(100_000, u64::MAX, None),
] {
let metadata = PaymentMetadata {
lsps2_parameters: Some(LSPS2Parameters {
max_total_opening_fee_msat: None,
max_proportional_opening_fee_ppm_msat: Some(proportional),
min_opening_fee_msat: 0,
}),
};
assert_eq!(
EventHandler::<Arc<TestLogger>>::lsps2_max_total_opening_fee_msat(
&metadata.encode(),
gross_amount_msat,
),
expected
);
}
}

#[test]
fn lsps2_payment_metadata_missing_or_malformed_limit_is_rejected() {
let empty_metadata = PaymentMetadata { lsps2_parameters: None }.encode();
let metadata_without_fee_limit = PaymentMetadata {
lsps2_parameters: Some(LSPS2Parameters {
max_total_opening_fee_msat: None,
max_proportional_opening_fee_ppm_msat: None,
min_opening_fee_msat: 1_000,
}),
}
.encode();
Expand Down
33 changes: 17 additions & 16 deletions src/liquidity/client/lsps2.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,14 +93,14 @@ where
Error::LiquidityRequestFailed
})?;

if let Some(max_total_lsp_fee_limit_msat) = max_total_lsp_fee_limit_msat {
if min_total_fee_msat > max_total_lsp_fee_limit_msat {
log_error!(self.logger,
"Failed to request inbound JIT channel as LSP's requested total opening fee of {}msat exceeds our fee limit of {}msat",
min_total_fee_msat, max_total_lsp_fee_limit_msat
);
return Err(Error::LiquidityFeeTooHigh);
}
let max_total_lsp_fee_limit_msat =
max_total_lsp_fee_limit_msat.unwrap_or(amount_msat).min(amount_msat);
Comment on lines +96 to +97

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Claude:

The receive_via_jit_channel doc for max_total_lsp_fee_limit_msat (bolt11.rs:676-677) still says we use the cheapest offer when it's unset. It should mention the fee is now also bounded by amount_msat, with LiquidityFeeTooHigh returned either way.

if min_total_fee_msat > max_total_lsp_fee_limit_msat {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This allows fee == amount, but the LSP side doesn't: lightning-liquidity rejects a buy request when opening_fee >= payment_size_msat, and only opens the channel if amt_to_forward_msat > 0.

So fee == amount still goes to the LSP and gets rejected there. Since we don't handle BuyRequestFailed, the user gets LiquidityRequestFailed after the 5s timeout. I think that's what the (1_000, None, Err(LiquidityRequestFailed)) test case is hitting.

Should this be >=? Same for .filter(|fee| *fee <= gross_amount_msat) in event.rs.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes, we're more liberal what we accept here. No, this is intentional == as in theory it could make sense that the client just wants a channel and is fine if the payment all goes to fees. That said, we have higher HTLC minimum values anyways, so in practice we won't ever see a 0-value HTLC coming from the LSP.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Makes sense.

log_error!(self.logger,
"Failed to request inbound JIT channel as LSP's requested total opening fee of {}msat exceeds our fee limit of {}msat",
min_total_fee_msat, max_total_lsp_fee_limit_msat
);
return Err(Error::LiquidityFeeTooHigh);
}

log_debug!(
Expand All @@ -110,18 +110,18 @@ where
min_total_fee_msat
);

let lsps2_parameters = LSPS2Parameters {
max_total_opening_fee_msat: Some(min_total_fee_msat),
max_proportional_opening_fee_ppm_msat: None,
min_opening_fee_msat: min_opening_params.min_fee_msat,
};
let buy_response = self
.lsps2_send_buy_request(
Some(amount_msat),
min_opening_params,
Some(&cheapest_lsp.node_id),
)
.await?;
let lsps2_parameters = LSPS2Parameters {
max_total_opening_fee_msat: Some(min_total_fee_msat),
max_proportional_opening_fee_ppm_msat: None,
};

let invoice = self.lsps2_create_jit_invoice(
buy_response,
Some(amount_msat),
Expand Down Expand Up @@ -178,13 +178,14 @@ where
min_prop_fee_ppm_msat
);

let buy_response = self
.lsps2_send_buy_request(None, min_opening_params, Some(&cheapest_lsp.node_id))
.await?;
let lsps2_parameters = LSPS2Parameters {
max_total_opening_fee_msat: None,
max_proportional_opening_fee_ppm_msat: Some(min_prop_fee_ppm_msat),
min_opening_fee_msat: min_opening_params.min_fee_msat,
};
let buy_response = self
.lsps2_send_buy_request(None, min_opening_params, Some(&cheapest_lsp.node_id))
.await?;
let invoice = self.lsps2_create_jit_invoice(
buy_response,
None,
Expand Down
1 change: 1 addition & 0 deletions src/payment/bolt11.rs
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,7 @@ mod tests {
let lsps2_parameters = LSPS2Parameters {
max_total_opening_fee_msat: Some(42_000),
max_proportional_opening_fee_ppm_msat: Some(17_000),
min_opening_fee_msat: 1_000,
};
let metadata = PaymentMetadata { lsps2_parameters: Some(lsps2_parameters) };

Expand Down
19 changes: 17 additions & 2 deletions src/payment/store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -689,11 +689,15 @@ pub struct LSPS2Parameters {
/// The maximal proportional fee, in parts-per-million millisatoshi, we allow any configured
/// LSP withhold from us when forwarding the payment.
pub max_proportional_opening_fee_ppm_msat: Option<u64>,
/// The minimum opening fee, in millisatoshi, from the selected LSP offer. For variable-amount
/// payments, the allowed fee is the greater of this minimum and the proportional fee.
pub min_opening_fee_msat: u64,
}

impl_writeable_tlv_based!(LSPS2Parameters, {
(0, max_total_opening_fee_msat, option),
(2, max_proportional_opening_fee_ppm_msat, option),
(3, min_opening_fee_msat, (default_value, 0)),
});

#[derive(Clone, Debug, PartialEq, Eq)]
Expand Down Expand Up @@ -996,13 +1000,24 @@ mod tests {
}
}

#[derive(Clone, Debug, PartialEq, Eq)]
struct LegacyLSPS2Parameters {
max_total_opening_fee_msat: Option<u64>,
max_proportional_opening_fee_ppm_msat: Option<u64>,
}

impl_writeable_tlv_based!(LegacyLSPS2Parameters, {
(0, max_total_opening_fee_msat, option),
(2, max_proportional_opening_fee_ppm_msat, option),
});

#[derive(Clone, Debug, PartialEq, Eq)]
struct LegacyBolt11JitKind {
hash: PaymentHash,
counterparty_skimmed_fee_msat: Option<u64>,
preimage: Option<PaymentPreimage>,
secret: Option<PaymentSecret>,
lsp_fee_limits: LSPS2Parameters,
lsp_fee_limits: LegacyLSPS2Parameters,
}

impl_writeable_tlv_based!(LegacyBolt11JitKind, {
Expand All @@ -1019,7 +1034,7 @@ mod tests {
let preimage = Some(PaymentPreimage([43u8; 32]));
let secret = Some(PaymentSecret([44u8; 32]));
let counterparty_skimmed_fee_msat = Some(7_777u64);
let lsp_fee_limits = LSPS2Parameters {
let lsp_fee_limits = LegacyLSPS2Parameters {
max_total_opening_fee_msat: Some(46_000),
max_proportional_opening_fee_ppm_msat: Some(47_000),
};
Expand Down
41 changes: 40 additions & 1 deletion tests/integration_tests_rust.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5291,7 +5291,7 @@ async fn do_lsps2_client_service_integration(client_trusts_lsp: bool) {
max_payment_size_msat: 1_000_000_000,
min_payment_size_msat: 0,
min_channel_lifetime: 100,
min_channel_opening_fee_msat: 0,
min_channel_opening_fee_msat: 1_000,
max_client_to_self_delay: 1024,
client_trusts_lsp,
disable_client_reserve: false,
Expand Down Expand Up @@ -5332,6 +5332,26 @@ async fn do_lsps2_client_service_integration(client_trusts_lsp: bool) {
let client_node = client_builder.build(client_config.node_entropy.into()).unwrap();
client_node.start().unwrap();

let fee_test_description =
Bolt11InvoiceDescription::Direct(Description::new(String::from("fee limit")).unwrap())
.into();
// Distinguish client fee rejection from the LSP's LiquidityRequestFailed.
for (amount, fee_limit, expected) in [
(999, None, Err(NodeError::LiquidityFeeTooHigh)),
(999, Some(2_000), Err(NodeError::LiquidityFeeTooHigh)),
(1_001, Some(999), Err(NodeError::LiquidityFeeTooHigh)),
(1_000, None, Err(NodeError::LiquidityRequestFailed)),
(1_001, None, Ok(())),
] {
assert_eq!(
client_node
.bolt11_payment()
.receive_via_jit_channel(amount, &fee_test_description, 1024, fee_limit)
.map(|_| ()),
expected
);
}

let payer_config = random_config();
setup_builder!(payer_builder, payer_config.node_config);
payer_builder.set_chain_source_esplora(esplora_url.clone(), Some(sync_config));
Expand Down Expand Up @@ -5569,6 +5589,25 @@ async fn do_lsps2_client_service_integration(client_trusts_lsp: bool) {
client_node.payment(&client_payment_id).unwrap().unwrap().status,
PaymentStatus::Failed
);

// Variable-amount fees must be computed from the gross payment.
let variable_invoice = client_node
.bolt11_payment()
.receive_variable_amount_via_jit_channel(&invoice_description, 1024, None)
.unwrap();
let variable_amount_msat = 20_000_000;
let variable_fee_msat = (variable_amount_msat * channel_opening_fee_ppm as u64) / 1_000_000;
let variable_payment_id = payer_node
.bolt11_payment()
.send_using_amount(&variable_invoice, variable_amount_msat, None)
.unwrap();
expect_channel_pending_event!(service_node, client_node.node_id());
expect_channel_ready_event!(service_node, client_node.node_id());
expect_channel_pending_event!(client_node, service_node.node_id());
expect_channel_ready_event!(client_node, service_node.node_id());
expect_payment_received_event!(client_node, variable_amount_msat - variable_fee_msat);
expect_event!(service_node, PaymentForwarded);
expect_payment_successful_event!(payer_node, variable_payment_id, None);
}

#[tokio::test(flavor = "multi_thread", worker_threads = 1)]
Expand Down
Loading