Skip to content

c-lightning: MaxFeePercent is ignored for invoices that carry an amount - #186

Open
atharrva01 wants to merge 1 commit into
btcpayserver:masterfrom
atharrva01:cln-maxfeepercent-all-invoices
Open

c-lightning: MaxFeePercent is ignored for invoices that carry an amount#186
atharrva01 wants to merge 1 commit into
btcpayserver:masterfrom
atharrva01:cln-maxfeepercent-all-invoices

Conversation

@atharrva01

Copy link
Copy Markdown

Follow up to #183, and to the chat discussion about keeping the same behaviour we had with the old pay API.

What the old behaviour was

Before the move to xpay, MaxFeePercent was passed straight to pay as its native maxfeepercent argument, for every invoice:

var feePercent = maxFeeFlat is null ? payParams?.MaxFeePercent : null;
// pay: bolt11 [msatoshi] [label] [riskfactor] [maxfeepercent] [retry_for] [maxdelay] [exemptfee] ...
: new object[] { bolt11, explicitAmount?.MilliSatoshi, null, null, feePercent, null, null, ... }

exemptfee was left null, so c-lightning's 5000msat default applied. Per the pay docs, exemptfee "allows the maxfeepercent check to be skipped on fees that are smaller than exemptfee". So the effective ceiling was max(5000msat, percentage).

What changed

xpay takes only an absolute maxfee, so the percentage now has to be converted into one. That conversion is guarded on explicitAmount is not null:

if (payParams?.MaxFeePercent is { } feePercent && explicitAmount is not null)

explicitAmount is only set when the invoice carries no amount, since xpay accepts amount_msat only in that case. So for a normal amount-carrying invoice the branch never runs, maxfee goes out as null, and xpay applies its own default of "5000msat, or 1% (whatever is greater)" instead of the configured ceiling.

#183 corrected the unit of that computation but kept the guard, and noted the gap at the time.

Fix

Derive the percentage from the amount actually being paid (the explicit amount when we supply one, ie. keysend or a bolt11 without an amount, otherwise the amount on the invoice) and floor it at exemptfee's default so small payments keep the allowance pay gave them:

var feeBase = explicitAmount ?? pr?.MinimumAmount;
if (feeBase is not null && feeBase != LightMoney.Zero)
    maxFeeFlat = Math.Max(
        (long)(feeBase.ToDecimal(LightMoneyUnit.MilliSatoshi) * (decimal)feePercent / 100m),
        CLightningExemptFeeMilliSatoshi);

Without the floor a 100 sat payment at 0.5% would drop from a 5000msat allowance to 500msat, which is tighter than pay ever was.

Tests

Three cases in the harness #183 added, all backend-free (an in-process TCP server stands in for lightningd and captures the outgoing request):

  • PayMaxFeePercentIsSentInMilliSatoshi: existing, unchanged.
  • PayMaxFeePercentIsAppliedToInvoiceAmount: amount-carrying invoice, 1% of 250,000 sat, asserts maxfee is 2,500,000 msat. Fails before this change (maxfee is null).
  • PayMaxFeePercentKeepsExemptFeeFloorOnSmallPayments: 100 sat at 0.5%, asserts the 5000msat floor wins over the 500msat percentage.

Also verified end to end against a live node. Setup is cln -> lnd -> cln_dest on regtest with CLN v26.06.1, the middle hop charging 20,000msat base so the route costs 20,250msat. Paying a 250,000 sat invoice with MaxFeePercent low enough that the ceiling falls back to the 5000msat floor:

maxfee sent result
1.7.6 as released none Ok, fee paid 20,250 msat
with this change 5,000 msat Error, "Could not find route without excessive cost"

Worth noting for anyone reproducing this that it needs three nodes. With two the channel is direct, there is no routing fee, and maxfee never binds. The percentage used above is deliberately small so the ceiling lands under a cheap regtest route; on mainnet the same gap shows up whenever the configured percentage is below the 1% xpay assumes, e.g. a caller asking for 0.5% currently gets 1%.

Notes

Reachable from the Greenfield pay-invoice endpoint, which is the only place MaxFeePercent is set in btcpayserver. MaxFeeFlat is unaffected; it was already applied unconditionally.

Before the move to xpay, MaxFeePercent was passed to the 'pay' command as
its native 'maxfeepercent' argument for every invoice, with 'exemptfee'
left at its 5000msat default. A payment was accepted when its fee fell
below either the percentage or 5000msat.

xpay takes only an absolute 'maxfee', so the percentage is now converted
to one. That conversion was guarded on 'explicitAmount is not null', which
only holds for invoices that carry no amount, so for a normal invoice
'maxfee' was left null and xpay applied its own default instead of the
configured ceiling.

Derive the percentage from the amount actually being paid, and floor it at
exemptfee's default so small payments keep the allowance 'pay' gave them.
@atharrva01

Copy link
Copy Markdown
Author

hi @NicolasDorier , would love ur thoughts on this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant