blindedpath: avoid uint32 overflow in accumulated fee calc

[allenpiscitello]

Change Description

calcNextTotalBaseFee and calcNextTotalFeeRate accumulate a blinded path's payinfo using uint32 arithmetic (oneMillion = uint32(1_000_000)). Once the summed fee rate passes ~4,295 ppm, or the summed base fee ~4,295 msat, the multiplication by one million wraps and the advertised fees are far lower than the real ones. Payers then underpay, the introduction node still charges its real fee, and the receiver gets an HTLC below the invoice amount that is failed back after the MPP timeout.

With the default config (1.1x policy buffer plus one dummy hop at the average policy), a single real hop at 1,100 msat + 2,750 ppm advertises a fee rate of 1,213 ppm instead of 5,508. This has been present since blinded paths were added for receiving (v0.18.3).

The fee policy advertised in the invoice must exactly match the aggregate of the policies enforced inside the blinded path. This change:

• computes both aggregates in uint64 with checked multiplication and addition (bits.Mul64 / bits.Add64);
• rejects an aggregate base fee or fee rate that doesn't fit in the invoice's uint32 payinfo fields, instead of clamping or truncating it;
• makes calcBlindedPathPolicies return an error wrapping errInvalidBlindedPath, so BuildBlindedPaymentPaths skips only that candidate route and still builds paths from the other usable routes.

Steps to Test

go test ./routing/blindedpath/ -run 'TestBlindedPathAccumulatedPolicyCalc|TestBuildBlindedPathSkipsFeeOverflow'

TestBlindedPathAccumulatedPolicyCalcLargeFees covers the original wrap, which fails on master (1,213 instead of 5,508 ppm, 1,412 instead of 10,001 msat). For both the base fee and the fee rate it also covers an aggregate of exactly math.MaxUint32 (accepted), math.MaxUint32 + 1 (rejected) and a uint64 intermediate overflow (rejected). TestBuildBlindedPathSkipsFeeOverflow checks that a candidate whose fees can't be represented is skipped while a valid candidate is still returned.

Pull Request Checklist

Testing

• Your PR passes all CI checks.
• Tests covering the positive and negative (error paths) are included.
• Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

• The change is not insubstantial. Typo fixes are not accepted to fight bot spam.
• The change obeys the Code Documentation and Commenting guidelines, and lines wrap at 80.
• Commits follow the Ideal Git Commit Structure.
• Any new logging statements use an appropriate subsystem and logging level.
• Any new lncli commands have appropriate tags in the comments for the rpc in the proto file.
• There is a change description in the release notes, or [skip ci] in the commit message for small changes.

📝 Please see our Contribution Guidelines for further guidance.

[github-actions]

🟠 PR Severity: HIGH

gh pr view | 3 files | 90 lines changed

🟠 High (1 file)

• routing/blindedpath/blinded_path.go - routing/* package: blinded path construction is part of payment pathfinding/routing logic

🟢 Low (2 files)

• docs/release-notes/release-notes-0.22.0.md - release notes
• routing/blindedpath/blinded_path_test.go - test-only change

Analysis

The only non-test, non-docs file changed is routing/blindedpath/blinded_path.go, which falls under routing/* (payment pathfinding algorithms), classifying this PR as HIGH severity. The change is small (29 lines added/removed in the source file) and does not meet any of the thresholds for a severity bump (not >20 files, not >500 lines, no multiple critical packages touched).

---

_{To override, add a severity-override-{critical,high,medium,low} label.}

[ziggie1984]

The wider intermediates fix the reported overflow around 4,295 msat/ppm. To make the invoice construction robust across the complete valid input range, I think this PR should additionally:

1. Make calcBlindedPathPolicies return an error.
2. Use checked multiplication and addition when calculating both aggregate fees, since valid uint32 fee rates can still overflow a uint64 numerator.
3. Reject an aggregate base fee or fee rate greater than math.MaxUint32.
4. Remove both forms of silent under-reporting:
   • the unchecked uint32(baseFee) conversion;
   • clamping the proportional fee to math.MaxUint32.
5. Propagate an errInvalidBlindedPath error so BuildBlindedPaymentPaths skips only that candidate and can continue with other usable paths.

The important invariant is that the fee policy advertised in the invoice must exactly represent the aggregate policies enforced inside the blinded path. If the exact aggregate cannot be encoded in the invoice's uint32 fields, that path should not be advertised.

I would also expect boundary tests covering:

• an aggregate exactly equal to math.MaxUint32, which should succeed;
• an aggregate one above math.MaxUint32, which should reject the path;
• a uint64 intermediate arithmetic overflow, which should reject the path;
• one invalid candidate alongside another valid candidate, confirming invoice construction retains the valid path.

[ziggie1984]

Missing your name in the release notes

[allenpiscitello]

Added in 68ec747, thanks.

[allenpiscitello]

Thanks, agreed on the invariant. Done in b561474:

1. calcBlindedPathPolicies now returns an error. buildBlindedPaymentPath wraps it.
2. Both aggregates use checked multiplication and addition (bits.Mul64 / bits.Add64). An intermediate overflow returns an error.
3. An aggregate base fee or fee rate above math.MaxUint32 is rejected. The running totals are kept as uint32. The aggregate only grows hop by hop, so checking each step is the same as checking the final value.
4. Removed the math.MaxUint32 clamp on the fee rate. calcBlindedPathPolicies now returns the base fee as a uint32 that has already been checked, so the unchecked uint32(baseFee) conversion in the payinfo construction is gone.
5. Both errors wrap errInvalidBlindedPath, so BuildBlindedPaymentPaths logs at debug and skips only that candidate.

Tests: TestBlindedPathAccumulatedPolicyCalcLargeFees is now table-driven. It keeps the original two overflow cases and adds these cases for both the base fee and the fee rate: an aggregate exactly math.MaxUint32 (accepted), an aggregate of math.MaxUint32 + 1 (rejected), and a uint64 intermediate overflow (rejected). The fee rate cases go through two hops, so the cross term is exercised. TestBuildBlindedPathSkipsFeeOverflow passes one candidate whose base fee doesn't fit together with one normal candidate, and checks that only the valid path ends up in the result.

I also updated the release note in bcd9571 to say that such paths are now skipped.

[ziggie1984]

Looking good, had some final comments

[ziggie1984]

Nice that the overflow errors carry the exact aggregate and limit now, but they never reach the logs: the errInvalidBlindedPath branch in BuildBlindedPaymentPaths only logs

Not using route (%s) as a blinded path since it resulted in an invalid blinded path

without err. If all candidates get rejected, the operator only sees could not build any blinded paths with no hint that fees were the reason. Could we include the error in that debug log, e.g. "...invalid blinded path: %v", route, err?

[allenpiscitello]

Done, the debug log in BuildBlindedPaymentPaths now includes the error, so a skipped candidate shows the aggregate and the limit it exceeded.

[ziggie1984]

Could we use descriptive names for these booleans instead of ok1 through ok4, for example baseTermOK, totalTermOK, termsSumOK, and roundingOK? Each flag maps to a distinct part of the formula, so naming them accordingly would make this combined overflow check easier to audit. The same applies to the fee-rate calculation below.

[allenpiscitello]

Renamed to baseTermOK, totalTermOK, termsSumOK and roundingOK, and to sumTermOK, productTermOK, termsSumOK and roundingOK in the fee-rate calculation.

[ziggie1984]

Could we clean up the commit history before merging? The implementation and tests are currently split across two commits, while the release-note and contributor changes are spread across three commits. I suggest squashing this into two focused commits: (1) the implementation and its tests, and (2) the release note and contributor entry. That would leave only one commit touching the release notes and make the history easier to follow.

[allenpiscitello]

Squashed into two commits: the fix with its tests, and the release note with the contributor entry.