lnrpc: fix contradictory unit descriptions for fee_per_mil and fee_rate - #11155
lnrpc: fix contradictory unit descriptions for fee_per_mil and fee_rate#11155freeport-porter wants to merge 1 commit into
Conversation
The comments for `fee_per_mil` and `fee_rate` in `ChannelFeeReport` describe incompatible units. `fee_per_mil` is documented as an absolute amount -- "the amount charged per milli-satoshis transferred expressed in millionths of a satoshi" -- while `fee_rate` is documented as "the effective fee rate in milli-satoshis... computed by dividing the fee_per_mil value by 1 million". An absolute amount divided by a million does not yield a rate, and a rate is not itself denominated in milli-satoshis, so the two descriptions cannot both be right. Reading the code that populates the message, `fee_per_mil` is set from `edgePolicy.FeeProportionalMillionths` and `fee_rate` is that value divided by `feeBase = 1000000` (rpcserver.go). The proportional fee is applied as `rate * amt_msat / 1000000` (htlcswitch/link.go `ExpectedFee`, graph/db/models `ComputeFee`), so the field is a parts-per-million share of the amount forwarded: a dimensionless ratio, not an amount. `fee_rate` is the same ratio as a plain fraction. Document both fields accordingly, and apply the same correction to `inbound_fee_per_mil`, which duplicated the original wording verbatim and is populated from the equivalent inbound rate (graph/db/models/inbound_fee.go `CalcFee`, also over 1e6). The chosen "parts per million" framing matches how the same quantity is already described elsewhere in this file (`PolicyUpdateRequest. fee_rate_ppm`) and in `lncli updatechanpolicy`. Documentation only: no field renames, no reordering, no behaviour or wire-format change. Generated stubs and the swagger definition are regenerated from the updated proto. Fixes lightningnetwork#4155
42835cc to
d9d0010
Compare
🟠 PR Severity: HIGH
🟠 High (3 files)
🟢 Low (1 file)
AnalysisThis PR only edits field-comment text in Severity is HIGH because the touched files fall under To override, add a |
Lrifton92
left a comment
There was a problem hiding this comment.
Checked the claim against the code rather than the thread. ChannelFeeReport.fee_per_mil is populated from the edge policy's FeeProportionalMillionths (feeRateFixedPoint in rpcserver.go), and that value is applied as BaseFee + amt * rate / 1000000 in htlcswitch.ExpectedFee and ChannelEdgePolicy.ComputeFee; the inbound rate goes through InboundFee.CalcFee with the same feeRateParts = 1e6 divisor. So "N milli-satoshis for every 1,000,000 milli-satoshis forwarded" is exactly what the code does, and fee_rate = fee_per_mil / 1e6 is that ratio written as a fraction. The old prose was wrong in both directions, as described.
The three artefacts move together: lightning.proto, the lightning.pb.go struct comments and the lightning.swagger.json descriptions carry the same wording, and only comment lines change in the generated code, so there is no wire or behaviour impact. The wording matches how the same quantity is already described on fee_rate_ppm in this file ("parts per million"), which seems the right default for a docs-only fix; the nanosat/millisat framing can still be settled in the issue without blocking this.
Change Description
Fixes #4155.
The comments for
fee_per_milandfee_rateinChannelFeeReportdescribe units that cannot both be right.fee_per_milis documented as an absolute amount — "the amount charged per milli-satoshis transferred expressed in millionths of a satoshi" — whilefee_rateis documented as "the effective fee rate in milli-satoshis... computed by dividing the fee_per_mil value by 1 million". An absolute amount divided by a million is not a rate, and a rate is not itself denominated in milli-satoshis.Reading the code:
fee_per_milis set fromedgePolicy.FeeProportionalMillionthsandfee_rateis that value divided byfeeBase = 1000000(rpcserver.go). The proportional fee is applied asrate * amt_msat / 1000000(htlcswitch/link.goExpectedFee, andComputeFeeingraph/db/models). So the field is a parts-per-million share of the amount forwarded — a dimensionless ratio, not an amount — andfee_rateis the same ratio written as a plain fraction. The values returned bylncli feereportwere always correct; only the prose was wrong.The identical wording had since been copied onto
inbound_fee_per_mil, which is populated from the equivalent inbound rate (CalcFeeingraph/db/models/inbound_fee.go, also over 1e6), so this corrects that too.On the wording — this part is a proposal, please pick
The thread did not settle on a framing and I do not want to pre-empt that. @yyforyongyu wrote:
and derived the rate as nano-satoshis per milli-satoshi. @jhoenicke argued the opposite:
Both are dimensionally valid; they differ only in presentation. I went with the plain per-million framing because it is already how this same quantity is described elsewhere in this file —
PolicyUpdateRequest.fee_rate_ppmis "parts per million" — and inlncli updatechanpolicy. Consistency with existing wording seemed the least surprising choice for a docs fix, but I am happy to switch to the nanosat/millisat framing, or to any wording a maintainer prefers. Just say which and I will push it.I deliberately did not act on two adjacent suggestions from the thread, as both are larger than a documentation fix and want their own discussion:
fee_per_mil(@jhoenicke notes it can be misread as "per mille", i.e. per thousand). That is a breaking API change. The new comment says "parts per million" explicitly, which resolves the ambiguity in prose without touching the wire API.lnwire.NanoSatoshitype (@yyforyongyu's second suggestion).Steps to Test
No behaviour change, so there is nothing to test at runtime. To verify the generated artefacts match the proto:
Pull Request Checklist
Testing
Code Style and Documentation
lncli feereportandlnd -happeared to contradict each other. The issue was triaged and labelleddocumentationby a maintainer.