Repository navigation
Conversation
|
👋 Thanks for assigning @jkczyz as a reviewer! |
| } | ||
| let max_total_lsp_fee_limit_msat = | ||
| max_total_lsp_fee_limit_msat.unwrap_or(amount_msat).min(amount_msat); | ||
| if min_total_fee_msat > max_total_lsp_fee_limit_msat { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| .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, 0, max_prop_fee) |
There was a problem hiding this comment.
For variable-amount payments this uses 0 as the minimum fee, but the LSP charges max(min_fee_msat, ceil(gross * ppm / 1e6)). We don't store min_fee_msat in LSPS2Parameters, so when the minimum fee is the bigger one, we reject a correctly charged payment, after the LSP has already opened the channel.
With this test's LSP config (1_000 msat min, 10_000 ppm), any variable payment under 100_000 msat would hit this. It was already there before this PR, so a follow-up is fine too.
There was a problem hiding this comment.
Good point, we should finally persist min_fee_msat and use it when checking the opening fees. I believe we did it in another PR already, but that never landed. Will update shortly.
|
Rebased to fix CI. |
21125ab to
23ba291
Compare
Variable-amount JIT invoices retained only a proportional fee limit. When an LSP's minimum fee was larger, the client rejected an otherwise valid payment after the channel had been opened. Retain the negotiated minimum in invoice metadata and use it when validating variable payments. Continue decoding historical payment records that do not carry a minimum. Co-Authored-By: HAL 9000
LSPS2 clients could request a fixed-amount invoice whose opening fee exceeded the payment, leaving rejection to the LSP. Variable-amount payments also checked their fee against the net receipt, rejecting otherwise valid payments. Bound fixed-amount fees by the invoice amount even without an application cap. Check variable-amount fees against the gross payment when it arrives, allowing equality while rejecting excessive fees and arithmetic overflow. Co-Authored-By: HAL 9000
23ba291 to
bdc6484
Compare
| }) | ||
| }) | ||
| }) | ||
| .filter(|fee| *fee <= gross_amount_msat) |
There was a problem hiding this comment.
Claude:
This filter also applies to
max_total_opening_fee_msat, but no test exercises that path;lsps2_payment_metadata_decodes_total_fee_limitonly passes a gross amount above the limit. Worth a case with the gross amount below 42_000 returningNone.
| // 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(); |
There was a problem hiding this comment.
Not sure I follow the comment. How is this exercising readers predating the minimum?
| let max_total_lsp_fee_limit_msat = | ||
| max_total_lsp_fee_limit_msat.unwrap_or(amount_msat).min(amount_msat); |
There was a problem hiding this comment.
Claude:
The
receive_via_jit_channeldoc formax_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 byamount_msat, withLiquidityFeeTooHighreturned either way.
4 participants
LSPS2 clients could request a fixed-amount invoice whose opening fee exceeded the payment, leaving rejection to the LSP. Variable-amount payments also checked their fee against the net receipt, rejecting otherwise valid payments.
Bound fixed-amount fees by the invoice amount even without an application cap. Check variable-amount fees against the gross payment when it arrives, allowing equality while rejecting excessive fees and arithmetic overflow.
Co-Authored-By: HAL 9000