funding: reject commitment feerates below relay floor

[tayfuryldz]

Change Description

Fixes #11201.

The fundee currently accepts an inbound open_channel whose feerate_per_kw is below the absolute relayable commitment fee floor. The initial commitment is then created with that peer-provided rate, leaving a channel that cannot make normal progress.

Reject the open before pending-channel database work and before the ChannelAcceptor is called when the proposed commitment fee rate is below chainfee.AbsoluteFeePerKwFloor (250 sat/kw). This matches the minimum value produced by lnd's valid --max-commit-fee-rate-anchors=1 configuration, so upgraded lnd peers remain interoperable at the configured minimum.

Steps to Test

go test ./funding -run '^(TestFundingManagerRejectLowCommitFeeRate|TestFundingManagerMinAnchorCommitFeeRate)$' -count=1
go test ./funding -count=1
go vet ./funding/ ./lnwallet/
git diff --check

The regression coverage pins both sides of the inbound boundary and also opens an anchor channel from an lnd initiator capped at the minimum configured 1 sat/vB (250 sat/kw), preventing the interoperability regression found during review.

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.
• The change obeys the code documentation/commenting guidelines and lines wrap at 80.
• Commits follow the ideal git commit structure.
• No new logging statements are added.
• No new lncli commands are added.
• A change description is included in the v0.22.0 release notes.

[Lrifton92]

Reviewed at a8be9ad.

The check does what #11201 asks for on the fundee side, and the test pins both sides of the boundary. I confirmed that by mutation: disabling the check fails below floor, and turning < into <= fails at floor. go test ./funding/ passes at this commit, and go vet ./funding/ ./lnwallet/ is clean.

One blocking issue, then smaller points.

lnd's own opens can land below this floor, so two lnd nodes can stop opening anchor channels with each other. The initiator caps its anchor commitment fee rate at MaxAnchorsCommitFeeRate (funding/manager.go:5026-5029). That value comes from --max-commit-fee-rate-anchors, which config.go:1253 accepts down to 1 sat/vB. SatPerVByte(1).FeePerKWeight() is 250 sat/kw (lnwallet/chainfee/rates.go:26-27), while this check rejects anything under FeePerKwFloor = 253 (rates.go:14). I reproduced it with a throwaway test on top of this commit: setupFundingManagers, anchors on both sides, Alice's MaxAnchorsCommitFeeRate set to SatPerVByte(1).FeePerKWeight(), then openChannel. Bob answers with commitment fee rate 250 sat/kw is too small, min is 253 sat/kw instead of AcceptChannel. So a node running the lowest value its config allows can no longer open anchor channels to upgraded lnd peers. The test-config change in createTestFundingManager is the same class of problem showing up in tests.

Either side can close this. The initiator could clamp its cap to at least FeePerKwFloor (or the config minimum could be raised to match). Alternatively, the fundee could compare against chainfee.AbsoluteFeePerKwFloor (250, rates.go:19), which is what 1 sat/vB converts to. Whichever you pick, a test with lnd itself as initiator at the minimum configured cap would pin the interop case, which the fundee-only test does not cover.

Smaller:

• Placement. The check runs after the pending-channel DB query (manager.go:1546) and after the ChannelAcceptor has been asked (manager.go:1587). It is a stateless comparison on the message, so it can move up with the other cheap checks. That also keeps an external acceptor from seeing, and possibly logging as accepted, an open we then reject. The PR description mentions the zero-fee-commitments exception, but nothing here depends on the negotiated commitType, so placing it after negotiateCommitmentType is not needed for that either.
• Line length. The repo's ll linter (line-length: 80, tab-width: 8, .golangci.yml:215-222) applies to tests as well. Added lines over 80 columns: lnwallet/errors.go:103 (92), and funding/manager_test.go 4171 (81), 4208 (85), 4214 (84), 4219 (85), 4237 (84), 4240 (87), 4241 (84). CI has not run lint on this PR yet.
• require.ErrorContains(t, errMsg, "commitment fee rate") matches on the message text. Asserting on the fee-rate values (for example "253 sat/kw") would also catch a wrong comparison value, not just the presence of the error.

[Lrifton92]

An lnd initiator with --max-commit-fee-rate-anchors=1 (the minimum config.go:1253 accepts) caps anchor opens at SatPerVByte(1).FeePerKWeight() = 250 sat/kw (manager.go:5026-5029), which this rejects as below 253. I reproduced it with openChannel between two test managers: Bob replies commitment fee rate 250 sat/kw is too small, min is 253 sat/kw. Comparing against chainfee.AbsoluteFeePerKwFloor here, or clamping the initiator's cap to FeePerKwFloor, would keep lnd↔lnd opens working.

[Lrifton92]

92 columns with the repo's ll settings. Wrapping the parameters as the neighbouring constructors do (e.g. ErrMinHtlcTooLarge just below) keeps it under 80, and the blank line after the signature then matches the multi-line style.

[Lrifton92]

Re-reviewed at 84eda45. All three points are addressed:

• The check now compares against chainfee.AbsoluteFeePerKwFloor (250 sat/kw), so an lnd initiator at the minimum --max-commit-fee-rate-anchors=1 can open to another lnd node again. TestFundingManagerMinAnchorCommitFeeRate pins exactly that case: I put the comparison back to FeePerKwFloor and it fails with commitment fee rate 250 sat/kw is too small, and it passes at head together with TestFundingManagerRejectLowCommitFeeRate.
• The check moved to the top of fundeeProcessOpenChannel, ahead of the pending-channel DB query and the ChannelAcceptor call.
• The ErrCommitFeeRateTooSmall signature is wrapped, and the test now asserts the reported minimum (min is 250 sat/kw) rather than just the prefix.

One thing left for CI: ll applies to _test.go files in this repo (only gosec is excluded for tests in .golangci.yml), and six added lines in funding/manager_test.go are over 80 columns at tab width 8: 4205 (81), 4242 (85), 4248 (84), 4253 (85), 4271 (84) and 4277 (84). Once those are wrapped, LGTM.

[saubyk]

Hi @tayfuryldz, thanks for taking the time to put this together.

Per the new contributor section of the contribution guidelines, we don't prioritize review of PRs from authors without a track record in the project. Given the current review load, I'm closing this rather than letting it sit.

This is already being discussed in #11201, which is the right place for it. Maintainers will decide there whether and how to fix it.

The way to get future PRs reviewed promptly is to build some history first: PR reviews and issue triage are where that usually starts. Once there's a track record, new code from you becomes much easier to prioritize.